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.

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

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

Merged
merged 9 commits into from
Dec 9, 2024

Conversation

bart-vmware
Copy link
Contributor

@bart-vmware bart-vmware commented Oct 15, 2024

Description

Adapt the Config Schema Generator logic to deal with the breaking change in System.Text.Json v9, reported at dotnet/runtime#108790.

More specifically, this PR fixes the JsonNode handling inside Config Schema Generator to work with STJ v9. The generator project still targets .NET 8, but now references STJ v9 explicitly. Its test project now multi-targets against .NET 8 and 9.

Before this PR, the generator and its tests only ran on .NET 8 (referencing STJ v8), which is why there were no test failures.

Checklist

  • Is this feature complete?
    • Yes. Ready to ship.
    • No. Follow-up changes expected.
  • Are you including unit tests for the changes and scenario tests if relevant?
    • Yes
    • No
  • Did you add public API?
    • Yes
      • If yes, did you have an API Review for it?
        • Yes
        • No
      • Did you add <remarks /> and <code /> elements on your triple slash comments?
        • Yes
        • No
    • No
  • Does the change make any security assumptions or guarantees?
    • Yes
      • If yes, have you done a threat model and had a security review?
        • Yes
        • No
    • No
  • Does the change require an update in our Aspire docs?
    • Yes
      • Link to aspire-docs issue:
    • No

/cc @eerhardt

Microsoft Reviewers: Open in CodeFlow

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Oct 15, 2024
@joperezr
Copy link
Member

Hello @bart-vmware, thanks for the contribution. This PR is currently set to target release/9.0-rc.1 which is no longer an active branch. Assuming you are okay with me re-targeting it against main ?

@bart-vmware
Copy link
Contributor Author

@joperezr It probably won't work with main as-is, due to different TFMs and version numbers.

Can I update this PR to make it compatible with release/9.0 and target that? If it doesn't meet the bar (no worries), I'd rather wait until the final 9.0 release has shipped, and then update and retarget this PR.

@joperezr
Copy link
Member

We are pretty locked down at this point for release/9.0 branch, so unfortunately wouldn't be able to take this into there. main is already a net9 TFM too, in fact main is our 9.1 branch now, so that would be the best place to target for your PR.

@joperezr
Copy link
Member

Given the above, I'm retargeting this to go against main as the rc.1 branch is frozen anyway.

@eerhardt
Copy link
Member

eerhardt commented Nov 8, 2024

(Sorry for the late response. We were busy locking down for .NET Aspire 9.0, which will be released next week.)

@eerhardt eerhardt marked this pull request as ready for review November 27, 2024 21:44
@eerhardt
Copy link
Member

Thanks for the PR here @bart-vmware. I've pushed some updates to make this ready for review. I took your suggestion of always using System.Text.Json 9.0.0 in the tool. This allows for much simpler code.

@bart-vmware
Copy link
Contributor Author

@eerhardt Thanks for doing the updates, this is basically what I had in mind, so LGTM from me.

I've renamed ReplaceNode to ReplaceNodeWithKeyCasingChange for clarity, and fixed the broken test on .NET 9. I don't know why the explicit VersionOverride solves the problem, a multi-targeting glitch perhaps? Without it, the dependencies look like this:

image

Copy link
Member

@eiriktsarpalis eiriktsarpalis left a comment

Choose a reason for hiding this comment

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

Thanks

{
// In System.Text.Json v9, the casing of the new key is not adapted. See https://github.com/dotnet/runtime/issues/108790.
// So instead, remove the existing node and insert a new one with the updated key.
var index = jsonObject.IndexOf(key);
Copy link
Member

Choose a reason for hiding this comment

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

This workaround requires dual lookup if the key is missing, which suggests to me that a JsonObject.TryAddmethod similar to the one in dotnet/runtime#107947 might be necessary.

@bart-vmware
Copy link
Contributor Author

Is this PR ready to be merged?

The Microsoft.Extensions.Hosting version used should be based on TFM.
Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

LGTM.

Thanks for the contribution @bart-vmware!

@eerhardt eerhardt enabled auto-merge (squash) December 9, 2024 17:33
@eerhardt eerhardt merged commit 403da28 into dotnet:main Dec 9, 2024
9 checks passed
@bart-vmware bart-vmware deleted the config-schema-generator-net90 branch December 10, 2024 11:16
@github-actions github-actions bot locked and limited conversation to collaborators Jan 10, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants