Skip to content

[NSJ -> STJ]Migrate ServiceIndexResource deserialization #7297

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.

Sign up for GitHub

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

Draft
Nigusu-Allehu wants to merge 1 commit into
base: dev-feature-nsj-stj-migration
Choose a base branch
from
Draft

[NSJ -> STJ]Migrate ServiceIndexResource deserialization #7297

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 33 additions & 0 deletions src/NuGet.Core/NuGet.Protocol/Model/ServiceIndexModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
// 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 System.Text.Json;
using System.Text.Json.Serialization;

namespace NuGet.Protocol.Model
{
internal sealed class ServiceIndexModel
{
[JsonPropertyName("version")]
public string? Version { get; set; }

[JsonPropertyName("resources")]
public IReadOnlyList<ServiceIndexEntryModel>? Resources { get; set; }
}

internal sealed class ServiceIndexEntryModel
{
[JsonPropertyName("@id")]
public string? Id { get; set; }

/// <summary>JSON string or array of strings.</summary>
[JsonPropertyName("@type")]
public JsonElement Type { get; set; }

/// <summary>Optional JSON string or array of strings.</summary>
[JsonPropertyName("clientVersion")]
[JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingDefault)]
public JsonElement ClientVersion { get; set; }
}
}
50 changes: 26 additions & 24 deletions src/NuGet.Core/NuGet.Protocol/Providers/ServiceIndexResourceV3Provider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,14 @@
using System.Collections.Concurrent;
using System.Globalization;
using System.IO;
using System.Text.Json;
using System.Threading;
using System.Threading.Tasks;
using Newtonsoft.Json.Linq;
using NuGet.Common;
using NuGet.Configuration;
using NuGet.Protocol.Core.Types;
using NuGet.Protocol.Model;
using NuGet.Protocol.Utility;
using NuGet.Versioning;

namespace NuGet.Protocol
Expand Down Expand Up @@ -190,34 +192,34 @@ ex.InnerException is IOException &&

private static async Task<ServiceIndexResourceV3> ConsumeServiceIndexStreamAsync(Stream stream, DateTime utcNow, PackageSource source, CancellationToken token)
{
// Parse the JSON
JObject json = await stream.AsJObjectAsync(token);

// Use SemVer instead of NuGetVersion, the service index should always be
// in strict SemVer format
JToken versionToken;
if (json.TryGetValue("version", out versionToken) &&
versionToken.Type == JTokenType.String)
ServiceIndexModel index;
try
{
SemanticVersion version;
if (SemanticVersion.TryParse((string)versionToken, out version) &&
version.Major == 3)
{
return new ServiceIndexResourceV3(json, utcNow, source);
}
else
{
string errorMessage = string.Format(
CultureInfo.CurrentCulture,
Strings.Protocol_UnsupportedVersion,
(string)versionToken);
throw new InvalidDataException(errorMessage);
}
index = await JsonSerializer.DeserializeAsync(stream, JsonContext.Default.ServiceIndexModel, token);
}
else
catch (JsonException ex)
{
throw new InvalidDataException(string.Format(
CultureInfo.CurrentCulture,
Strings.Protocol_InvalidJsonObject,
source.Source), ex);
}

if (index?.Version is not string versionString)
Copy link
Copy Markdown

@richlander richlander Apr 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not is null? Seems simpler and equivalent.

{
throw new InvalidDataException(Strings.Protocol_MissingVersion);
}

// Use SemVer instead of NuGetVersion; the service index should always be in strict SemVer format.
if (!SemanticVersion.TryParse(versionString, out SemanticVersion version) || version.Major != 3)
{
throw new InvalidDataException(string.Format(
CultureInfo.CurrentCulture,
Strings.Protocol_UnsupportedVersion,
versionString));
}

return new ServiceIndexResourceV3(index, utcNow, source);
}

protected class ServiceIndexCacheInfo
Expand Down
97 changes: 93 additions & 4 deletions src/NuGet.Core/NuGet.Protocol/Resources/ServiceIndexResourceV3.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,14 @@
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text.Json;
using Newtonsoft.Json.Linq;
using NuGet.Configuration;
using NuGet.Packaging;
using NuGet.Protocol.Core.Types;
using NuGet.Protocol.Events;
using NuGet.Protocol.Model;
using NuGet.Protocol.Utility;
using NuGet.Versioning;

namespace NuGet.Protocol
Expand All @@ -21,6 +24,7 @@ namespace NuGet.Protocol
public class ServiceIndexResourceV3 : INuGetResource
{
private readonly string _json;
private readonly ServiceIndexModel _model;
private readonly IDictionary<string, List<ServiceIndexEntry>> _index;
private readonly DateTime _requestTime;
private static readonly IReadOnlyList<ServiceIndexEntry> _emptyEntries = new List<ServiceIndexEntry>();
Expand All @@ -35,6 +39,13 @@ internal ServiceIndexResourceV3(JObject index, DateTime requestTime, PackageSour

public ServiceIndexResourceV3(JObject index, DateTime requestTime) : this(index, requestTime, null) { }

internal ServiceIndexResourceV3(ServiceIndexModel model, DateTime requestTime, PackageSource packageSource)
{
_model = model;
_index = MakeLookup(model, packageSource);
_requestTime = requestTime;
}

/// <summary>
/// Time the index was requested
/// </summary>
Expand All @@ -56,10 +67,7 @@ public virtual IReadOnlyList<ServiceIndexEntry> Entries

public virtual string Json
{
get
{
return _json;
}
get { return _json ?? JsonSerializer.Serialize(_model, JsonContext.Default.ServiceIndexModel); }
Copy link
Copy Markdown

@richlander richlander Apr 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pattern results in serialization occuring once and seems have similar characteristics as the old code.

Suggested change
get { return _json ?? JsonSerializer.Serialize(_model, JsonContext.Default.ServiceIndexModel); }
get { return _json ??= JsonSerializer.Serialize(_model, JsonContext.Default.ServiceIndexModel); }

}

/// <summary>
Expand Down Expand Up @@ -148,6 +156,87 @@ public virtual IReadOnlyList<Uri> GetServiceEntryUris(NuGetVersion clientVersion
return GetServiceEntries(clientVersion, orderedTypes).Select(e => e.Uri).ToList();
}

private static IDictionary<string, List<ServiceIndexEntry>> MakeLookup(ServiceIndexModel index, PackageSource packageSource)
{
var result = new Dictionary<string, List<ServiceIndexEntry>>(StringComparer.Ordinal);

if (index?.Resources is null)
{
return result;
}

foreach (var resource in index.Resources)
{
var id = resource.Id;
if (string.IsNullOrEmpty(id) || !Uri.TryCreate(id, UriKind.Absolute, out Uri uri))
{
continue;
}

if (packageSource != null && uri.Scheme == Uri.UriSchemeHttp && packageSource.IsHttps)
{
ProtocolDiagnostics.RaiseEvent(new ProtocolDiagnosticServiceIndexEntryEvent(source: packageSource.Source, httpsSourceHasHttpResource: true));
}

var types = GetStringValues(resource.Type).ToArray();
Copy link
Copy Markdown

@richlander richlander Apr 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we get an IEnumerable<T> instead and skip the costly array creation? It looks like the target is foreach.


var clientVersions = new List<SemanticVersion>();
if (resource.ClientVersion.ValueKind == JsonValueKind.Undefined)
{
clientVersions.Add(_defaultVersion);
}
else
{
foreach (var versionString in GetStringValues(resource.ClientVersion))
{
if (SemanticVersion.TryParse(versionString, out SemanticVersion semVer))
{
clientVersions.Add(semVer);
}
}
}

foreach (var type in types)
{
foreach (var clientVersion in clientVersions)
{
if (!result.TryGetValue(type, out List<ServiceIndexEntry> entries))
{
entries = new List<ServiceIndexEntry>();
result.Add(type, entries);
}

entries.Add(new ServiceIndexEntry(uri, type, clientVersion));
}
}
}

foreach (var type in result.Keys.ToArray())
Copy link
Copy Markdown

@richlander richlander Apr 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same thing here. Generating an array for foreach is wasteful.

{
result[type] = result[type].OrderByDescending(e => e.ClientVersion).ToList();
}

return result;
Copy link
Copy Markdown

@richlander richlander Apr 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should validate that the caller actually needs a materialized list.

}

private static IEnumerable<string> GetStringValues(JsonElement element)
{
if (element.ValueKind == JsonValueKind.Array)
{
foreach (var item in element.EnumerateArray())
Copy link
Copy Markdown

@richlander richlander Apr 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider writing an extension method that is AsIEnumerable. You can then use it in more places.

{
if (item.ValueKind == JsonValueKind.String)
{
yield return item.GetString();
}
}
}
else if (element.ValueKind == JsonValueKind.String)
{
yield return element.GetString();
}
}

private static IDictionary<string, List<ServiceIndexEntry>> MakeLookup(JObject index, PackageSource packageSource)
{
var result = new Dictionary<string, List<ServiceIndexEntry>>(StringComparer.Ordinal);
Expand Down
2 changes: 1 addition & 1 deletion src/NuGet.Core/NuGet.Protocol/Utility/JsonContext.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,12 @@ namespace NuGet.Protocol.Utility
#pragma warning disable CS3016 // Arrays as attribute arguments is not CLS-compliant
[JsonSourceGenerationOptions(PropertyNameCaseInsensitive = true,
PropertyNamingPolicy = JsonKnownNamingPolicy.CamelCase,
GenerationMode = JsonSourceGenerationMode.Metadata,
Converters = [typeof(VersionRangeStjConverter)])]
#pragma warning restore CS3016 // Arrays as attribute arguments is not CLS-compliant
[JsonSerializable(typeof(HttpFileSystemBasedFindPackageByIdResource.FlatContainerVersionList))]
[JsonSerializable(typeof(IReadOnlyList<V3VulnerabilityIndexEntry>), TypeInfoPropertyName = "VulnerabilityIndex")]
[JsonSerializable(typeof(CaseInsensitiveDictionary<IReadOnlyList<PackageVulnerabilityInfo>>), TypeInfoPropertyName = "VulnerabilityPage")]
[JsonSerializable(typeof(ServiceIndexModel))]
internal partial class JsonContext : JsonSerializerContext
{
}
Expand Down
36 changes: 29 additions & 7 deletions test/NuGet.Core.Tests/NuGet.Protocol.Tests/ServiceIndexResourceV3ProviderTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) .NET Foundation. All rights reserved.
// 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.

#nullable disable
Expand Down Expand Up @@ -105,8 +105,8 @@ public async Task TryCreate_Throws_IfSourceLocationDoesNotReturnValidJson(string
}

[Theory]
[InlineData("{ version: \"not-semver\" } ")]
[InlineData("{ version: \"3.0.0.0\" } ")] // not strict semver
[InlineData("{ \"version\": \"not-semver\" }")]
[InlineData("{ \"version\": \"3.0.0.0\" }")] // not strict semver
public async Task TryCreate_Throws_IfInvalidVersionInJson(string content)
{
// Arrange
Expand All @@ -127,9 +127,7 @@ public async Task TryCreate_Throws_IfInvalidVersionInJson(string content)
}

[Theory]
[InlineData("{ json: \"that does not contain version.\" }")]
[InlineData("{ version: 3 } ")] // version is not a string
[InlineData("{ version: { value: 3 } } ")] // version is not a string
[InlineData("{ \"json\": \"that does not contain version.\" }")]
public async Task TryCreate_Throws_IfNoVersionInJson(string content)
{
// Arrange
Expand All @@ -149,12 +147,36 @@ public async Task TryCreate_Throws_IfNoVersionInJson(string content)
Assert.Equal("The source does not have the 'version' property.", exception.InnerException.Message);
}

[Theory]
[InlineData("{ \"version\": 3 }")] // version is not a string
[InlineData("{ \"version\": { \"value\": 3 } }")] // version is not a string
public async Task TryCreate_Throws_IfVersionFieldIsWrongType(string content)
{
// Arrange
// STJ throws JsonException for type mismatches during deserialization, which
// maps to Protocol_InvalidJsonObject rather than Protocol_MissingVersion.
var source = "https://contoso.test/index.json";
var httpProvider = StaticHttpSource.CreateHttpSource(new Dictionary<string, string> { { source, content } });
var provider = new ServiceIndexResourceV3Provider();
var sourceRepository = new SourceRepository(new PackageSource(source),
new INuGetResourceProvider[] { httpProvider, provider });

// Act
var exception = await Assert.ThrowsAsync<FatalProtocolException>(async () =>
{
var result = await provider.TryCreate(sourceRepository, default(CancellationToken));
});

// Assert
Assert.IsType<InvalidDataException>(exception.InnerException);
}

[Fact]
public async Task TryCreate_ReturnsTrue_IfSourceLocationReturnsValidJson()
{
// Arrange
var source = $"https://some-site-{new Guid().ToString()}.org/test.json";
var content = @"{ version: '3.1.0-beta' }";
var content = @"{ ""version"": ""3.1.0-beta"" }";
var httpProvider = StaticHttpSource.CreateHttpSource(new Dictionary<string, string> { { source, content } });
var provider = new ServiceIndexResourceV3Provider();
var sourceRepository = new SourceRepository(new PackageSource(source),
Expand Down
Loading
Loading