-
Notifications
You must be signed in to change notification settings - Fork 748
[NSJ -> STJ]Migrate AutoComplete #7298
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Changes from 4 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| // Copyright (c) .NET Foundation. All rights reserved. | ||
| // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||
|
|
||
| #if !NET9_0_OR_GREATER | ||
| namespace System.Diagnostics.CodeAnalysis | ||
| { | ||
| [AttributeUsage(AttributeTargets.Property, Inherited = false)] | ||
| internal sealed class FeatureSwitchDefinitionAttribute : Attribute | ||
| { | ||
| public FeatureSwitchDefinitionAttribute(string switchName) => SwitchName = switchName; | ||
| public string SwitchName { get; } | ||
| } | ||
| } | ||
| #endif |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| // Copyright (c) .NET Foundation. All rights reserved. | ||
| // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||
|
|
||
| using System; | ||
| using System.Diagnostics.CodeAnalysis; | ||
| using NuGet.Common; | ||
|
|
||
| namespace NuGet.Shared | ||
| { | ||
| internal static class NuGetFeatureFlags | ||
| { | ||
| internal const string UseNSJDeserializationSwitchName = "NuGet.UseNSJDeserialization"; | ||
| internal const string UseNSJDeserializationEnvVar = "NUGET_USE_NSJ_DESERIALIZATION"; | ||
|
|
||
| private static readonly Lazy<bool> _isNSJDeserializationEnabledByEnvironment = | ||
| new Lazy<bool>(() => IsNSJDeserializationEnabledByEnvironment(EnvironmentVariableWrapper.Instance)); | ||
|
|
||
| /// <summary>Feature switch for NSJ deserialization. Defaults to <see langword="false"/> (STJ is the default).</summary> | ||
| [FeatureSwitchDefinition(UseNSJDeserializationSwitchName)] | ||
| internal static bool UseNSJDeserializationFeatureSwitch { get; } = | ||
| AppContext.TryGetSwitch(UseNSJDeserializationSwitchName, out bool value) && value; | ||
|
|
||
| /// <summary>Returns <see langword="true"/> when env var <c>NUGET_USE_NSJ_DESERIALIZATION</c> is <c>true</c>.</summary> | ||
| internal static bool IsNSJDeserializationEnabledByEnvironment(IEnvironmentVariableReader? env = null) | ||
| { | ||
| if (env is null) | ||
| { | ||
| return _isNSJDeserializationEnabledByEnvironment.Value; | ||
| } | ||
|
|
||
| string? envValue = env.GetEnvironmentVariable(UseNSJDeserializationEnvVar); | ||
| return string.Equals(envValue, "true", StringComparison.OrdinalIgnoreCase); | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| // Copyright (c) .NET Foundation. All rights reserved. | ||
| // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||
|
|
||
| using System.Text.Json.Serialization; | ||
|
|
||
| namespace NuGet.Protocol.Model | ||
| { | ||
| internal sealed class AutoCompleteModel | ||
| { | ||
| [JsonPropertyName("data")] | ||
| public string[]? Data { get; set; } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,26 +8,42 @@ | |
| using System.Globalization; | ||
| using System.Linq; | ||
| using System.Net; | ||
| using System.Text.Json; | ||
| using System.Threading; | ||
| using System.Threading.Tasks; | ||
| using Newtonsoft.Json.Linq; | ||
| using NuGet.Common; | ||
| using NuGet.Protocol.Core.Types; | ||
| using NuGet.Protocol.Model; | ||
| using NuGet.Protocol.Utility; | ||
| using NuGet.Shared; | ||
| using NuGet.Versioning; | ||
|
|
||
| namespace NuGet.Protocol | ||
| { | ||
| public class AutoCompleteResourceV3 : AutoCompleteResource | ||
| { | ||
| private readonly RegistrationResourceV3 _regResource; | ||
| private readonly ServiceIndexResourceV3 _serviceIndex; | ||
| private readonly HttpSource _client; | ||
| internal readonly RegistrationResourceV3 _regResource; | ||
| internal readonly ServiceIndexResourceV3 _serviceIndex; | ||
| internal readonly HttpSource _client; | ||
| private readonly IEnvironmentVariableReader _environmentVariableReader; | ||
|
|
||
| public AutoCompleteResourceV3(HttpSource client, ServiceIndexResourceV3 serviceIndex, RegistrationResourceV3 regResource) | ||
| : base() | ||
| { | ||
| _regResource = regResource; | ||
| _serviceIndex = serviceIndex; | ||
| _client = client; | ||
| _environmentVariableReader = EnvironmentVariableWrapper.Instance; | ||
| } | ||
|
|
||
| internal AutoCompleteResourceV3(HttpSource client, ServiceIndexResourceV3 serviceIndex, RegistrationResourceV3 regResource, IEnvironmentVariableReader environmentVariableReader) | ||
| : base() | ||
| { | ||
| _regResource = regResource; | ||
| _serviceIndex = serviceIndex; | ||
| _client = client; | ||
| _environmentVariableReader = environmentVariableReader; | ||
| } | ||
|
|
||
| public override async Task<IEnumerable<string>> IdStartsWith( | ||
|
|
@@ -53,17 +69,51 @@ public override async Task<IEnumerable<string>> IdStartsWith( | |
| queryUrl.Query = queryString; | ||
|
|
||
| Common.ILogger logger = log ?? Common.NullLogger.Instance; | ||
|
|
||
| var queryUri = queryUrl.Uri; | ||
|
|
||
| if (NuGetFeatureFlags.UseNSJDeserializationFeatureSwitch || NuGetFeatureFlags.IsNSJDeserializationEnabledByEnvironment(_environmentVariableReader)) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should learn from #7266, because this pattern here will read the environment variable every time, and on .NET Framework that causes memory allocations every time. Making it both testable and avoid allocations is difficult, but we need to find a solution. When that other PR was open, I remember someone sharing an idea that
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should have been caching. The idea was to pass null in production and the caching is done in the NuGetFeatureClassFlag class. And pass env value during test to avoid the caching.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Line 37 isn't using null, it's using EnvironmentVariableWrapper.Instance, so it's not going to cache. This shows a risk of this design. It's very prone to error, preventing caching. Even if you manually change it, I'm wondering if AI generated code is going to try to optimisticly keep using EnvironmentVariableWrapper.Instance since that's what's done in most other parts of the code. From an API design point of view, the best APIs are hard or impossible to use wrong. So, fixing the null value is an option, but if we can find another design, we might be able to reduce risk of repeating the same error.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To reduce the risk of future mistakes, I've added an XML docomment on
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think the comment is helping since I'd say we'll lean towards making it testable as much as wee can There's other ways we can cache things and we can talk about that as you make progress. An example could be to cache it at the provider level. We reuse SourceRepository instances, and we reuse providers in restore https://github.com/NuGet/NuGet.Client/blob/dev/src/NuGet.Core/NuGet.Protocol/CachingSourceProvider.cs The provider can initialize the resource based on that. So cache it in the repository provider and pass it down (non public way). Sure that means every provider has to cache it, but I don't know if that's necessarily a big deal. We're only hitting these codepaths when restore things it needs to download something. Note that caching idea is similar to 01276fc. We can chat more ideas, but having code that's actually not being exercised is unnecessary. |
||
| { | ||
| return await IdStartsWithNsjAsync(packageIdPrefix, logger, queryUri, token); | ||
|
Nigusu-Allehu marked this conversation as resolved.
Outdated
|
||
| } | ||
|
|
||
| AutoCompleteModel results = await _client.ProcessStreamAsync( | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am surprised that Does this library not have nullable enabled?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not yet. It's #nullable disabled for now, but it's a work in progress |
||
| new HttpSourceRequest(queryUri, logger), | ||
| async stream => | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't know what the team code style is, if they link lamdas defined ahead of the method call or within it. |
||
| { | ||
| if (stream == null) | ||
| { | ||
| return null; | ||
| } | ||
|
|
||
| return await JsonSerializer.DeserializeAsync(stream, JsonContext.Default.AutoCompleteModel, token); | ||
| }, | ||
| logger, | ||
| token); | ||
|
|
||
| token.ThrowIfCancellationRequested(); | ||
|
|
||
| return results?.Data?.Where(item => item != null && item.StartsWith(packageIdPrefix, StringComparison.OrdinalIgnoreCase)) | ||
| ?? []; | ||
| } | ||
|
|
||
| private async Task<IEnumerable<string>> IdStartsWithNsjAsync( | ||
| string packageIdPrefix, | ||
| Common.ILogger logger, | ||
| Uri queryUri, | ||
| CancellationToken token) | ||
| { | ||
| var results = await _client.GetJObjectAsync( | ||
| new HttpSourceRequest(queryUri, logger), | ||
| logger, | ||
| token); | ||
|
|
||
| token.ThrowIfCancellationRequested(); | ||
|
|
||
| if (results == null) | ||
| { | ||
| return Enumerable.Empty<string>(); | ||
| } | ||
|
|
||
| var data = results.Value<JArray>("data"); | ||
| if (data == null) | ||
| { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| // Copyright (c) .NET Foundation. All rights reserved. | ||
| // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||
|
|
||
| using System.Collections.Generic; | ||
| using NuGet.Shared; | ||
| using Test.Utility; | ||
| using Xunit; | ||
|
|
||
| namespace NuGet.Protocol.Tests | ||
| { | ||
| public class NuGetFeatureFlagsTests | ||
| { | ||
| [Fact] | ||
| public void UseNSJDeserializationFeatureSwitch_Default_ReturnsFalse() | ||
| { | ||
| Assert.False(NuGetFeatureFlags.UseNSJDeserializationFeatureSwitch); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void IsNSJDeserializationEnabledByEnvironment_WhenEnvVarNotSet_ReturnsFalse() | ||
| { | ||
| Assert.False(NuGetFeatureFlags.IsNSJDeserializationEnabledByEnvironment(TestEnvironmentVariableReader.EmptyInstance)); | ||
| } | ||
|
|
||
| [Theory] | ||
| [InlineData("true")] | ||
| [InlineData("True")] | ||
| [InlineData("TRUE")] | ||
| public void IsNSJDeserializationEnabledByEnvironment_WhenEnvVarSetToTrue_ReturnsTrue(string value) | ||
| { | ||
| var env = new TestEnvironmentVariableReader( | ||
| new Dictionary<string, string> { [NuGetFeatureFlags.UseNSJDeserializationEnvVar] = value }); | ||
|
|
||
| Assert.True(NuGetFeatureFlags.IsNSJDeserializationEnabledByEnvironment(env)); | ||
| } | ||
|
|
||
| [Theory] | ||
| [InlineData("false")] | ||
| [InlineData("0")] | ||
| [InlineData("1")] | ||
| [InlineData("anything")] | ||
| public void IsNSJDeserializationEnabledByEnvironment_WhenEnvVarSetToFalseOrUnrecognized_ReturnsFalse(string value) | ||
| { | ||
| var env = new TestEnvironmentVariableReader( | ||
| new Dictionary<string, string> { [NuGetFeatureFlags.UseNSJDeserializationEnvVar] = value }); | ||
|
|
||
| Assert.False(NuGetFeatureFlags.IsNSJDeserializationEnabledByEnvironment(env)); | ||
| } | ||
| } | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.