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

Fixes and enhancements in JSON Configuration schema generator #4441

Merged
merged 20 commits into from
Jun 28, 2024
Merged

Fixes and enhancements in JSON Configuration schema generator #4441

merged 20 commits into from
Jun 28, 2024

Conversation

bart-vmware
Copy link
Contributor

@bart-vmware bart-vmware commented Jun 10, 2024

This PR contains bugfixes and enhancements to the JSON Configuration schema generator, which I'm trying to use outside of Aspire. I've tested with VS 2022 v17.10.1. Please see the individual commit messages for change details. Hopefully, this PR brings the tool a step closer to achieving #3309.

I'm curious as to why JSON properties are sorted alphabetically. I found it makes it harder to compare against the original source code it was generated from.

Note

This includes a fix in the RuntimeSource directory to prevent a StackOverflowException. It should probably be synced with the original runtime source code. Tracked at dotnet/runtime#103802.

/cc @eerhardt

Microsoft Reviewers: Open in CodeFlow

This initially broke the integration test, which loads reference assemblies from the 'refs' subdirectory.
But Microsoft.Extensions.Configuration.Abstractions.dll (which contains ConfigurationKeyNameAttribute) isn't copied there during build,
which is why I changed the test to not use the refs subdirectory.
…ecause they have no semantic meaning. IDEs auto-break lines when rendering the text, depending on screen space. However, do convert <br/> and <para/> tags into visible line breaks.

While this doesn't have much effect on Aspire itself, it matters greatly to my team because we have configured Resharper to auto-format doc-comments, which takes the configured maximum line length into account. So it merges and breaks lines in doc-comments all the time.

The changes on Aspire .json files are the following, which can be seen after replacing all \n with a single space in the diff.
- ` ,` becomes `,`
- `( 'SomeType' )` becomes `('SomeType')`
- `Some.  Other` becomes `Some. Other`
- `Some. Other` becomes `Some.\n\nOther` (because of <para> usage)
- `"Some ."` becomes `"Some."`
…s an array of numbers, as well as a base64-encoded string. See dotnet/runtime#43150.
Because roslyn provides no public API to expand inherited doc-comments (see dotnet/csharplang#313), I'm using the internal Microsoft.CodeAnalysis.Shared.Extensions.ISymbolExtensions.GetDocumentationComment method. The method is made accessible by the IgnoresAccessChecksToGenerator NuGet package. I borrowed this approach from docfx.

This method behaves a bit odd though: If there's no doc-comment on a member, it internally assumes that the member contains "<doc><inheritdoc/></doc>" (which is completely invalid) and feeds that to itself. As a consequence, the method may return something wrapped in <doc>, instead of the expected <member> element. I didn't want to write a test for this undocumented behavior, but it explains the fallback to <doc> when <member> does not exist.

The IgnoresAccessChecksToGenerator package generates IgnoresAccessChecksTo.cs, which violates code style settings. I've tried adding an .editorconfig at various places to suppress rules, which didn't work. The only way I could get the suppressions to work was via GlobalSuppressions.cs.
…s, allow all types as component, preserve existing JSON structure on empty nodes.

Note this commit includes a fix in the RuntimeSource directory to prevent a StackOverflowException.
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-integrations Issues pertaining to Aspire Integrations packages label Jun 10, 2024
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jun 10, 2024
@eerhardt
Copy link
Member

I'm curious as to why JSON properties are sorted alphabetically.

It makes it "stable", so the .json file can be checked in. It makes version-to-version updates of a library easier, so when new properties are added, you can see what properties are added, and they don't become all mixed up. See #2663 for an example, a new property was added and we can easily diff and see the new property (the properties aren't in a new order after the update).

@@ -68,7 +68,12 @@ private static PortableExecutableReference CreateMetadataReference(string path)
}

var path = (string)args[0].Value;
(configurationPaths ??= new()).Add((string)args[0].Value);
if (string.IsNullOrEmpty(path))
Copy link
Member

Choose a reason for hiding this comment

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

I don't think path should ever be null or empty. What's the scenario here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is new: an empty path allows to define properties in the configuration root. See ShouldAllowEmptyComponentPath.

@@ -84,94 +81,241 @@ private JsonObject GenerateGraph()
var type = spec.ConfigurationTypes[i];
var path = spec.ConfigurationPaths[i];

GenerateProperties(root, type, path);
var pathSegments = ImmutableQueue<string>.Empty;
Copy link
Member

Choose a reason for hiding this comment

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

Does the immutability matter?
I couldn't find any place where it would.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it doesn't. I'll change to use a regular queue.

@bart-vmware
Copy link
Contributor Author

I'm curious as to why JSON properties are sorted alphabetically.

It makes it "stable", so the .json file can be checked in. It makes version-to-version updates of a library easier, so when new properties are added, you can see what properties are added, and they don't become all mixed up. See #2663 for an example, a new property was added and we can easily diff and see the new property (the properties aren't in a new order after the update).

Thanks for the clarification. I suppose it depends on whether to optimize for the development of the generator versus its consumption. Although member order in assemblies is already deterministic, according to the Remarks section at https://learn.microsoft.com/en-us/dotnet/api/system.type.getproperties?view=net-8.0#system-type-getproperties:

In .NET 6 and earlier versions, the GetProperties method does not return properties in a particular order, such as alphabetical or declaration order. Your code must not depend on the order in which properties are returned, because that order varies. However, starting with .NET 7, the ordering is deterministic based upon the metadata ordering in the assembly.

Because https://github.com/dotnet/aspire/pull/2663/files#diff-ba29f362bde4fa8dfaf456726c5d33b10aec019ce5485384d75974e2c98b9277 did not shuffle the order of existing properties, the diff in Aspire would still remain minimal.

@bart-vmware
Copy link
Contributor Author

bart-vmware commented Jun 11, 2024

Regarding the failed checks: I've followed the steps at https://github.com/dotnet/aspire/tree/main/tests/Aspire.Workload.Tests#readme and can't reproduce the failure locally. Here's what's in the failure log:

   Failed Aspire.Workload.Tests.StarterTemplateWithWithRedisCacheTests.WebFrontendWorks(urlPrefix: "https://") [13 s]
  Error Message:
   Microsoft.Playwright.PlaywrightException : net::ERR_NETWORK_CHANGED at https://localhost:17003/login?t=d2e8dbf1418a5877c9cd259dff91a759

I don't suspect this is related to the changes in this PR, but please let me know if there's anything I can do to get a green build.

Edit: it's all green this time.

@eerhardt eerhardt requested a review from radical as a code owner June 21, 2024 00:06
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.

Sorry for the delay here @bart-vmware, I was on vacation for a while. But I'm back now.

First off, this is really great! Thank you for the contribution here. These fixes and enhancements really help.

I pushed some changes that I could (mostly to get rid of the extra dependency and to just use normal reflection). I left some other comments. Once those are addressed, I can merge this.

Thanks again.


var schema = GenerateSchemaFromCode(source, []);

Assert.Contains("FormatSettings", schema);
Copy link
Member

Choose a reason for hiding this comment

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

Why not validate the whole schmea like we do elsewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In general, get-only properties are excluded in the schema. But collections and non-primitive types are exceptions to that. This test verifies the property gets emitted, even if the declared type is get-only.

I didn't assert on the full tree, because most of its content isn't relevant here. This way, future refactorings of other tree parts won't require "fixing" this test.

Copy link
Member

Choose a reason for hiding this comment

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

But it doesn't catch other bugs. It also doesn't validate that "FormatSettings" gets put in the right location. Since we are validating the full tree everywhere else, it feels like we should just do the same here. It gets us better test coverage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Existing tests already cover all other concerns. It's duplicating test coverage, not making it better. But I've added it anyway.

Comment on lines 519 to 530
// Example appsettings.json:
// {
// "TestComponent": {
// "TestProperty": {
// "UserName": "[email protected]",
// "Password": "P@ssw0rd!",
// "Options": {
// "RequireSSL": "false"
// }
// }
// }
// }
Copy link
Member

Choose a reason for hiding this comment

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

  1. This doesn't seem to match the test below.
  2. There's no real way we can support TypeConverter in a static analysis mechanism.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a scenario we have in our codebase. The spec we're implementing there dictates a free-format substructure, basically the meaning of object in JSON. In .NET, this is modeled as a recursive string dictionary. It took me quite a while to find a way to make that fully work, so I included it for future reference, ensuring the proper schema is produced.

My first attempt was to add a custom TypeConverter on the dictionary and let ConfigurationBinder create the instance, but that doesn't work because ConfigurationBinder doesn't run custom converters on collections. The next best thing is to add a constructor with parameters, which ConfigurationBinder will pick up, but the roslyn parts in this schema generator can't handle that. Adding a parameterless constructor makes ConfigurationBinder pick that one instead, so that doesn't work either.

The scenario provided here works when binding configuration to options, the generator can handle it, and VS provides the correct IntelliSense (not suggesting prohibited nodes such as arrays, and not warning on unknown property names).

Copy link
Member

Choose a reason for hiding this comment

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

My biggest concern is that this comment doesn't match the code below. I don't see it using TestProperty1 or TestProperty2. I think the comment is confusing since it doesn't match.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. I've replaced the comment with compilable code.

}

private void GenerateProperties(JsonObject parent, TypeSpec type, string path)
private bool GenerateComponent(JsonObject currentNode, TypeSpec type, Queue<string> pathSegments)
Copy link
Member

Choose a reason for hiding this comment

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

What does "Component" mean in GenerateComponent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In:

[assembly: ConfigurationSchema("One.Two.Three", typeof(ExampleSettings))]

"One", "Two" and "Three" are components.

Copy link
Member

Choose a reason for hiding this comment

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

Can we use a different term? "Components" is kind of an overloaded term in the .NET Aspire world. How about "GeneratePathSegment"?

@bart-vmware bart-vmware requested a review from eerhardt June 21, 2024 11:53
@bart-vmware
Copy link
Contributor Author

@eerhardt I've addressed all comments. Can you take another look? Please don't hesitate to resolve the open conversations if they've been addressed properly.

I don't think the failing checks are related to these changes, but I can't rerun them.

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.

Thanks again for this great contribution, @bart-vmware!

The changes look good to me. I'm ready to merge once CI is green. We had some flaky failures in the past, but we should be in a better state now.

@dotnet-bot
Copy link
Contributor

‼️ Found issues ‼️

Project Coverage Type Expected Actual
Microsoft.Extensions.ServiceDiscovery Branch 81 79.31 🔻
Microsoft.Extensions.ServiceDiscovery Line 81 79.01 🔻

Full code coverage report: https://dev.azure.com/dnceng-public/public/_build/results?buildId=723407&view=codecoverage-tab

@eerhardt eerhardt merged commit f79f9f0 into dotnet:main Jun 28, 2024
9 checks passed
@bart-vmware bart-vmware deleted the config-schema-types branch July 1, 2024 08:01
@github-actions github-actions bot locked and limited conversation to collaborators Aug 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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
Development

Successfully merging this pull request may close these issues.

4 participants