Fixes and enhancements in JSON Configuration schema generator #4441

bart-vmware · 2024-06-10T15:54:30Z

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.

eerhardt · 2024-06-10T21:45:12Z

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).

src/Components/Aspire.Azure.Storage.Blobs/ConfigurationSchema.json

src/Tools/ConfigurationSchemaGenerator/ConfigurationSchemaGenerator.csproj

eerhardt · 2024-06-10T22:07:26Z

src/Tools/ConfigurationSchemaGenerator/ConfigSchemaGenerator.cs

@@ -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))


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

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

tests/ConfigurationSchemaGenerator.Tests/RecordWithPrimitives.cs

sebastienros · 2024-06-10T22:43:51Z

src/Tools/ConfigurationSchemaGenerator/ConfigSchemaEmitter.cs

@@ -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;


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

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

bart-vmware · 2024-06-11T09:55:35Z

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 · 2024-06-11T09:59:44Z

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.

…ction to reduce dependencies.

eerhardt

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.

eerhardt · 2024-06-20T23:34:18Z

tests/ConfigurationSchemaGenerator.Tests/GeneratorTests.cs

+
+        var schema = GenerateSchemaFromCode(source, []);
+
+        Assert.Contains("FormatSettings", schema);


Why not validate the whole schmea like we do elsewhere?

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.

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.

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

eerhardt · 2024-06-20T23:36:24Z

tests/ConfigurationSchemaGenerator.Tests/GeneratorTests.cs

+        // Example appsettings.json:
+        // {
+        //   "TestComponent": {
+        //     "TestProperty": {
+        //       "UserName": "john.doe@email.com",
+        //       "Password": "P@ssw0rd!",
+        //       "Options": {
+        //         "RequireSSL": "false"
+        //       }
+        //     }
+        //   }
+        // }


This doesn't seem to match the test below.

There's no real way we can support TypeConverter in a static analysis mechanism.

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).

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.

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

tests/ConfigurationSchemaGenerator.Tests/GeneratorTests.cs

...onSchemaGenerator/RuntimeSource/Configuration.Binder/ConfigurationBindingGenerator.Parser.cs

eerhardt · 2024-06-20T23:50:20Z

src/Tools/ConfigurationSchemaGenerator/ConfigSchemaEmitter.cs

    }

-    private void GenerateProperties(JsonObject parent, TypeSpec type, string path)
+    private bool GenerateComponent(JsonObject currentNode, TypeSpec type, Queue<string> pathSegments)


What does "Component" mean in GenerateComponent?

In:

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

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

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

src/Tools/ConfigurationSchemaGenerator/ConfigSchemaEmitter.cs

bart-vmware · 2024-06-26T09:19:48Z

@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.

- rename variables to remove "component"

eerhardt

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 · 2024-06-28T04:04:49Z

‼️ 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

bart-vmware added 7 commits June 10, 2024 17:42

Add tests to identify the current behavior (all tests are green).

ace04ef

Fix crash on property of type byte[]. The ConfigurationBinder support…

ceb0c3d

…s an array of numbers, as well as a base64-encoded string. See dotnet/runtime#43150.

Allow settings to appear top-level

141f49c

Refactor recursion to support collections and recursive/circular type…

39e1e32

…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 bot added the area-integrations Issues pertaining to Aspire Integrations packages label Jun 10, 2024

dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jun 10, 2024

Fix broken build after rebase on latest main

bce9dd1

eerhardt reviewed Jun 10, 2024

View reviewed changes

src/Components/Aspire.Azure.Storage.Blobs/ConfigurationSchema.json Show resolved Hide resolved

eerhardt reviewed Jun 10, 2024

View reviewed changes

src/Tools/ConfigurationSchemaGenerator/ConfigurationSchemaGenerator.csproj Outdated Show resolved Hide resolved

eerhardt reviewed Jun 10, 2024

View reviewed changes

tests/ConfigurationSchemaGenerator.Tests/RecordWithPrimitives.cs Show resolved Hide resolved

sebastienros reviewed Jun 10, 2024

View reviewed changes

Review feedback: use Queue instead of ImmutableQueue

b73ee51

bart-vmware requested review from sebastienros and eerhardt June 11, 2024 11:25

bart-vmware and others added 3 commits June 11, 2024 17:30

Preserve blank lines in doc-comments

81fbfcb

Remove IgnoresAccessChecksToGenerator usage and just use normal refle…

bd4ce2b

…ction to reduce dependencies.

Remove unnecessary #ifs

58d78a3

eerhardt requested a review from radical as a code owner June 21, 2024 00:06

eerhardt reviewed Jun 21, 2024

View reviewed changes

Add explaining comments for node backup strategy

2808792

bart-vmware mentioned this pull request Jun 21, 2024

StackOverflowException in Configuration.Binder source generator dotnet/runtime#103802

Closed

bart-vmware added 2 commits June 21, 2024 11:51

Fix normalization of whitespace characters, such as tabs

f61daf1

Add explaining comments for line break handling in doc-comments

9061445

bart-vmware requested a review from eerhardt June 21, 2024 11:53

bart-vmware added 3 commits June 24, 2024 07:38

Rename GenerateComponent to GeneratePathSegment

3917fbb

Provide compiled sample usage instead of appsettings.json comment

6f57568

Replace minimal assertions with full JSON comparison

6ded274

eerhardt added 2 commits June 27, 2024 18:10

Merge remote-tracking branch 'upstream/main' into config-schema-types

52328e9

PR feedback

c75ce14

- rename variables to remove "component"

eerhardt approved these changes Jun 27, 2024

View reviewed changes

eerhardt enabled auto-merge (squash) June 27, 2024 23:20

build-analysis bot mentioned this pull request Jun 28, 2024

The Operation will be canceled. The next steps may not contain expected logs. dotnet/dnceng#3008

Open

3 tasks

eerhardt merged commit f79f9f0 into dotnet:main Jun 28, 2024
9 checks passed

bart-vmware deleted the config-schema-types branch July 1, 2024 08:01

eerhardt mentioned this pull request Jul 2, 2024

Bump StackExchange.Redis from 2.7.33 to 2.8.0 #4671

Merged

github-actions bot locked and limited conversation to collaborators Aug 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes and enhancements in JSON Configuration schema generator #4441

Fixes and enhancements in JSON Configuration schema generator #4441

bart-vmware commented Jun 10, 2024 •

edited

Loading

eerhardt commented Jun 10, 2024

eerhardt Jun 10, 2024

bart-vmware Jun 11, 2024

sebastienros Jun 10, 2024

bart-vmware Jun 11, 2024

bart-vmware commented Jun 11, 2024

bart-vmware commented Jun 11, 2024 •

edited

Loading

eerhardt left a comment

eerhardt Jun 20, 2024

bart-vmware Jun 21, 2024

eerhardt Jun 21, 2024

bart-vmware Jun 24, 2024

eerhardt Jun 20, 2024

bart-vmware Jun 21, 2024

eerhardt Jun 21, 2024

bart-vmware Jun 24, 2024

eerhardt Jun 20, 2024

bart-vmware Jun 21, 2024

eerhardt Jun 21, 2024

bart-vmware commented Jun 26, 2024

eerhardt left a comment

dotnet-bot commented Jun 28, 2024


		var schema = GenerateSchemaFromCode(source, []);

		Assert.Contains("FormatSettings", schema);

Fixes and enhancements in JSON Configuration schema generator #4441

Fixes and enhancements in JSON Configuration schema generator #4441

Conversation

bart-vmware commented Jun 10, 2024 • edited Loading

Microsoft Reviewers: Open in CodeFlow

eerhardt commented Jun 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bart-vmware commented Jun 11, 2024

bart-vmware commented Jun 11, 2024 • edited Loading

eerhardt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bart-vmware commented Jun 26, 2024

eerhardt left a comment

Choose a reason for hiding this comment

dotnet-bot commented Jun 28, 2024

bart-vmware commented Jun 10, 2024 •

edited

Loading

bart-vmware commented Jun 11, 2024 •

edited

Loading