Skip to content

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

Merged
Eric Erhardt (eerhardt) merged 9 commits into
microsoft:mainfrom
bart-vmware:config-schema-generator-net90
Dec 9, 2024
Merged

Fix broken tests when running Config Schema Generator on .NET 9#6312
Eric Erhardt (eerhardt) merged 9 commits into
microsoft:mainfrom
bart-vmware:config-schema-generator-net90

Conversation

@bart-vmware

@bart-vmware Bart Koelman (bart-vmware) commented Oct 15, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

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 Eric Erhardt (@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

Jose Perez Rodriguez (joperezr) commented Oct 21, 2024

Copy link
Copy Markdown
Member

Hello Bart Koelman (@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

Bart Koelman (bart-vmware) commented Oct 22, 2024

Copy link
Copy Markdown
Contributor Author

Jose Perez Rodriguez (@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

Jose Perez Rodriguez (joperezr) commented Oct 24, 2024

Copy link
Copy Markdown
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

Jose Perez Rodriguez (joperezr) commented Oct 25, 2024

Copy link
Copy Markdown
Member

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

@joperezr
Jose Perez Rodriguez (joperezr) changed the base branch from release/9.0-rc1 to main October 25, 2024 17:58
@build-analysis build-analysis Bot mentioned this pull request Oct 25, 2024
Open
3 tasks
@eerhardt

Eric Erhardt (eerhardt) commented Nov 8, 2024

Copy link
Copy Markdown
Member

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

@eerhardt
Eric Erhardt (eerhardt) marked this pull request as ready for review November 27, 2024 21:44
@eerhardt

Eric Erhardt (eerhardt) commented Nov 27, 2024

Copy link
Copy Markdown
Member

Thanks for the PR here Bart Koelman (@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

Bart Koelman (bart-vmware) commented Nov 28, 2024

Copy link
Copy Markdown
Contributor Author

Eric Erhardt (@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

eiriktsarpalis
Eirik Tsarpalis (eiriktsarpalis) approved these changes Nov 28, 2024
View reviewed changes

@eiriktsarpalis Eirik Tsarpalis (eiriktsarpalis) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks

eiriktsarpalis
Eirik Tsarpalis (eiriktsarpalis) reviewed Nov 28, 2024
View reviewed changes
Comment thread src/Tools/ConfigurationSchemaGenerator/ConfigSchemaEmitter.cs
{
// 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);

@eiriktsarpalis Eirik Tsarpalis (eiriktsarpalis) Nov 28, 2024

Copy link
Copy Markdown
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.

Closed
@bart-vmware

Bart Koelman (bart-vmware) commented Dec 9, 2024

Copy link
Copy Markdown
Contributor Author

Is this PR ready to be merged?

@eerhardt
Fix dependencies
fcd0c41 
The Microsoft.Extensions.Hosting version used should be based on TFM.
eerhardt
Eric Erhardt (eerhardt) approved these changes Dec 9, 2024
View reviewed changes

@eerhardt Eric Erhardt (eerhardt) left a comment

Copy link
Copy Markdown
Member

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 Koelman (@bart-vmware)!

@eerhardt
Eric Erhardt (eerhardt) enabled auto-merge (squash) December 9, 2024 17:33
@eerhardt
Eric Erhardt (eerhardt) merged commit 403da28 into microsoft:main Dec 9, 2024
@bart-vmware
Bart Koelman (bart-vmware) deleted the config-schema-generator-net90 branch December 10, 2024 11:16
@bart-vmware Bart Koelman (bart-vmware) mentioned this pull request Dec 20, 2024
Merged
2 of 5 tasks
@github-actions github-actions Bot locked and limited conversation to collaborators Jan 10, 2025
@github-actions github-actions Bot added the area-integrations Issues pertaining to Aspire Integrations packages label Mar 10, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@eerhardt Eric Erhardt (eerhardt) eerhardt approved these changes
+1 more reviewer
@eiriktsarpalis Eirik Tsarpalis (eiriktsarpalis) eiriktsarpalis approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-integrations Issues pertaining to Aspire Integrations packages community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@bart-vmware @joperezr @eerhardt @eiriktsarpalis