Skip to content
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

Jump to bottom

Fix broken tests when running Config Schema Generator on .NET 9 #6312

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Fix broken tests when running Config Schema Generator on .NET 9 #6312

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
2 changes: 1 addition & 1 deletion src/Components/Directory.Build.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@
<Target Name="CalculateConfigurationSchemaGeneratorPath">
<MSBuild Projects="$(ConfigurationSchemaGeneratorProjectPath)"
Targets="GetTargetPath"
RemoveProperties="TargetFramework">
Properties="$(Properties);TargetFramework=$(DefaultTargetFramework)">
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this is right. (actually I'm pretty sure it is wrong). Can you explain why you are proposing this change? What doesn't work about the current code?

<Output TaskParameter="TargetOutputs" PropertyName="ConfigurationSchemaGeneratorPath" />
</MSBuild>
</Target>
Expand Down
42 changes: 32 additions & 10 deletions src/Tools/ConfigurationSchemaGenerator/ConfigSchemaEmitter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ private void GenerateLogCategories(JsonObject parent)
{
var categoryNode = new JsonObject();
categoryNode["$ref"] = "#/definitions/logLevelThreshold";
propertiesNode[categories[i]] = categoryNode;
ReplaceNodeWithKeyCasingChange(propertiesNode, categories[i], categoryNode);
Copy link
Member

Choose a reason for hiding this comment

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

Given that this is code copied from the config binder generator, is it appropriate to be making changes locally?

Copy link
Member

Choose a reason for hiding this comment

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

This part of the code isn't copied from the config binder generator. The part that is copied is inside the https://github.com/dotnet/aspire/tree/main/src/Tools/ConfigurationSchemaGenerator/RuntimeSource directory.

}

parent["definitions"] = new JsonObject
Expand Down Expand Up @@ -138,7 +138,7 @@ private bool GeneratePathSegment(JsonObject currentNode, TypeSpec type, Queue<st
else
{
pathSegmentNode = new JsonObject();
propertiesNode[pathSegment] = pathSegmentNode;
ReplaceNodeWithKeyCasingChange(propertiesNode, pathSegment, pathSegmentNode);
ownsPathSegment = true;
}
}
Expand All @@ -148,9 +148,7 @@ private bool GeneratePathSegment(JsonObject currentNode, TypeSpec type, Queue<st

if (backupCasingOfPathSegmentName != pathSegment)
{
// Re-add existing node, so the new casing of pathSegment is used.
propertiesNode.Remove(pathSegment);
propertiesNode[pathSegment] = pathSegmentNode;
ReplaceNodeWithKeyCasingChange(propertiesNode, pathSegment, pathSegmentNode);
}
else
{
Expand All @@ -173,10 +171,8 @@ private bool GeneratePathSegment(JsonObject currentNode, TypeSpec type, Queue<st
}
else if (backupCasingOfPathSegmentName != null)
{
// Revert casing change of pathSegment.
var existingValue = propertiesNode[pathSegment];
propertiesNode.Remove(pathSegment);
propertiesNode[backupCasingOfPathSegmentName] = existingValue;
ReplaceNodeWithKeyCasingChange(propertiesNode, backupCasingOfPathSegmentName, existingValue);
}
}

Expand Down Expand Up @@ -288,7 +284,7 @@ private bool GenerateProperty(JsonObject currentNode, PropertySpec property, IPr
var backupPropertyNode = currentNode[property.ConfigurationKeyName];

var propertyNode = new JsonObject();
currentNode[property.ConfigurationKeyName] = propertyNode;
ReplaceNodeWithKeyCasingChange(currentNode, property.ConfigurationKeyName, propertyNode);

var hasGenerated = GenerateType(propertyNode, propertyType);
if (hasGenerated)
Expand Down Expand Up @@ -343,7 +339,7 @@ private static void RestoreBackup(JsonNode? backupNode, string name, JsonObject
}
else
{
parentNode[name] = backupNode;
ReplaceNodeWithKeyCasingChange(parentNode, name, backupNode);
}
}

Expand Down Expand Up @@ -719,6 +715,32 @@ private static string[] CreateExclusionPaths(List<string>? exclusionPaths)
return result;
}

private static void ReplaceNodeWithKeyCasingChange(JsonObject jsonObject, string key, JsonNode value)
{
#if NET9_0_OR_GREATER
Comment on lines +718 to +720
Copy link
Member

@eerhardt eerhardt Nov 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@eiriktsarpalis - is this the recommended way to respond to the breaking change? Do we have documentation guidance on the break?

I almost wonder if we should be checking the System.Text.Json major version number instead. Or doing some sort of "runtime capability test" - for example, execute a method that checks for the new behavior and set a bool flag.

The situation that can break this is if you are targeting net8.0, but somehow the 9.0 System.Text.Json nuget package gets pulled into the app (ex. through a transitive reference). In that situation, the #else code will be compiled into the app, but the 9.0 STJ behavior will happen.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I fully understand what tests were broken by the particular change in STJ 9, but per the discussion in dotnet/runtime#108790 (comment) the pre-9 behaviour is undocumented, accidental behaviour that is inconsistent with how all other dictionary types work in the same context.

We've agreed to add a breaking change note in our documentation for completeness, but beyond that I don't think it's appropriate to introduce mitigations down the stack just so that the pre-9 behavior is preserved. Assuming there are failing tests that check against that behavior, then I would recommend just updating the tests.

// In .NET 9, the key casing is never adapted. See https://github.com/dotnet/runtime/issues/108790.
var index = jsonObject.IndexOf(key);
if (index != -1)
{
jsonObject.RemoveAt(index);
jsonObject.Insert(index, key, value);
}
else
{
jsonObject[key] = value;
}
#else
// In .NET 8, the key casing is adapted, except when the value doesn't change.
if (ReferenceEquals(jsonObject[key], value))
{
// There's no API to preserve property order.
jsonObject.Remove(key);
}

jsonObject[key] = value;
#endif
}

private sealed class SchemaOrderJsonNodeConverter : JsonConverter<JsonNode>
{
public static SchemaOrderJsonNodeConverter Instance { get; } = new SchemaOrderJsonNodeConverter();
Expand Down
2 changes: 1 addition & 1 deletion src/Tools/ConfigurationSchemaGenerator/ConfigurationSchemaGenerator.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<TargetFramework>$(DefaultTargetFramework)</TargetFramework>
<TargetFrameworks>$(AllTargetFrameworks)</TargetFrameworks>
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't need to multi-target the generator. It should only need to target net8.0.

<OutputType>Exe</OutputType>
<ImplicitUsings>enable</ImplicitUsings>
<Nullable>annotations</Nullable>
Expand Down
13 changes: 11 additions & 2 deletions tests/ConfigurationSchemaGenerator.Tests/ConfigurationSchemaGenerator.Tests.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<TargetFramework>$(DefaultTargetFramework)</TargetFramework>
<TargetFrameworks>$(AllTargetFrameworks)</TargetFrameworks>
<PreserveCompilationContext>true</PreserveCompilationContext>
<CopyDocumentationFilesFromPackages>true</CopyDocumentationFilesFromPackages>
</PropertyGroup>
Expand All @@ -18,10 +18,19 @@

<ItemGroup>
<PackageReference Include="Microsoft.DotNet.XUnitExtensions" />
<PackageReference Include="Microsoft.Extensions.Hosting" />
<PackageReference Include="Newtonsoft.Json" />
</ItemGroup>

<ItemGroup Condition="'$(TargetFramework)' != 'net9.0'">
<PackageReference Include="Microsoft.Extensions.Hosting" />
</ItemGroup>

<ItemGroup Condition="'$(TargetFramework)' == 'net9.0'">
<PackageReference Include="Microsoft.Extensions.Hosting" VersionOverride="9.0.0-rc.2.24473.5" />
<PackageReference Include="Microsoft.Extensions.Configuration.Binder" VersionOverride="9.0.0-rc.2.24473.5" />
<PackageReference Include="Microsoft.Extensions.Hosting.Abstractions" VersionOverride="9.0.0-rc.2.24473.5" />
</ItemGroup>
Comment on lines +28 to +32
Copy link
Contributor Author

@bart-vmware bart-vmware Oct 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I wonder if there's a better way. I've tried updating versions.props but that breaks other things, so I've made the changes local. This is needed because the listed packages reference 8.0.0 versions otherwise.

Copy link
Member

Choose a reason for hiding this comment

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

This should be fixed now. You should be able to revert these changes because of the following:

aspire/eng/Versions.props

Lines 76 to 79 in 2e60ee6

<PropertyGroup Condition="'$(TargetFramework)' == 'net9.0'">
<!-- Runtime -->
<MicrosoftExtensionsHostingAbstractionsPackageVersion>9.0.0-rc.2.24473.5</MicrosoftExtensionsHostingAbstractionsPackageVersion>
<MicrosoftExtensionsHostingPackageVersion>9.0.0-rc.2.24473.5</MicrosoftExtensionsHostingPackageVersion>

This will automatically reference the 9.0 hosting packages when targeting net9.0.


<ItemGroup>
<ProjectReference Include="..\..\src\Tools\ConfigurationSchemaGenerator\ConfigurationSchemaGenerator.csproj" />
</ItemGroup>
Expand Down
Loading