Fix circular reference resolution for schemas containing arrays #64109
Conversation
dotnet-policy-service
bot
added
the
community-contribution
There was a problem hiding this comment.
Pull Request Overview
This PR fixes OpenAPI schema generation for array circular references by modifying the component registration order during recursion, ensuring components are registered before descending into the recursion tree rather than registering leaves first. Additionally, it addresses an issue with default values for "local" attributes by ensuring they don't apply to componentized schemas.
Key changes:
- Fixed component registration timing in
ResolveReferenceForSchemamethod - Added proper handling of default values for referenced schemas using a new annotation
- Enhanced test coverage for circular references with arrays
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| OpenApiSchemaService.cs | Modified component registration logic to handle circular references properly and added early registration before recursion |
| OpenApiConstants.cs | Added new constant for reference default annotation |
| OpenApiJsonSchema.Helpers.cs | Added support for reading reference default annotations from JSON |
| OpenApiDocumentExtensions.cs | Updated schema reference creation to handle default values correctly |
| JsonNodeSchemaExtensions.cs | Modified default value application to distinguish between local and referenced schemas |
| OpenApiSchemaReferenceTransformerTests.cs | Added comprehensive test cases for circular references with arrays |
| OpenApiSchemaService.RequestBodySchemas.cs | Updated test assertions to work with new reference structure |
github-actions
bot
added
the
area-mvc
|
@captainsafia I am just putting it out there, there is still some "dirty" code. I know this has a bit of a "refactor" vibe, but that is not my intention. By diving deep into the "patching" of the JsonSchemaExporter's result, I think this would be the best solution for now. I've added two more tests for different orderings as the schema exporting logic sometimes feels a bit like a lottery. I needed to fix the "default" on the schema reference and the test, as "default" values from a controller action should not apply to a componetized schema. Do you want me to add a test to proof it was an issue. That is, the
Edit: will fix the nullability warnings in the tests tomorrow. |
|
It also fixes the component registration in the workspace, as they where not registered correctly. It makes #63606 obsolete. As we the schema is added immediately there was a check required to not overwrite the schema. |
|
@captainsafia I am sorry, the PR is now bigger.... I could not help it 🫣. I was going through the flows and if statements, and I noticed the cases at the end did nothing anymore (or would result in a undesired result, another issue which someone would need to fix). The resolving of references is now quite a bit simplified. I am fine to revert this commit (for portback) 32a3dd1 and do it as a seperate PR. |
desjoerd
force-pushed
the
fix/gh-64048
branch
2 times, most recently
from
6ac7483 to
32a3dd1
Compare
dotnet-policy-service
bot
added
the
pending-ci-rerun
desjoerd
changed the title
desjoerd
force-pushed
the
fix/gh-64048
branch
from
80720ab to
a8681a1
Compare
captainsafia
removed
the
pending-ci-rerun
dotnet-policy-service
bot
added
the
pending-ci-rerun
Add support for ASP.NET Core OpenAPI (.NET 10). There is one known issue around circular references which requires dotnet/aspnetcore#64109 to be merged.
The fix is done by registering components before going deep into the recursion tree (where the leaves would be registered first). Fixing this revealed an issue for default values for "local" attributes. Local attributes/parameter info should not apply to componetized schemas.
…hema-id is null or empty
desjoerd
force-pushed
the
fix/gh-64048
branch
from
606461e to
4a64e3f
Compare
|
@captainsafia and @martincostello I've fixed the comments. Just a note, I've also removed the x-ref-id Metadata which contained the "original" $ref from the JsonSchemaExporter as it was not used anymkre in any of the code paths. |
captainsafia
removed
the
pending-ci-rerun
captainsafia
changed the title
|
/backport to release/10.0 |
|
Started backporting to |
3 participants
OpenAPI: Fix component $ref registration into the workspace so references get resolved correctly
The fix is done by registering components before going deep into the recursion tree (where the leaves would be registered first).
Fixing this revealed an issue for default values for "local" attributes. Local attributes/parameter info should not apply to componetized schemas.
Background on the changes:
The main change is to register components in the
Workspacewith the correct id, this is done viadocument.AddComponent(schemaId, schema). This changes two things:document.AddComponent(schemaId, schema)does not overwrite an existing schema with the same id, this is completely different in how it worked, as the current implementation overwrites the schema. So from Last registration wins to the First registration wins.Because of this the components need to be registered before completely resolving references (to make sure the first one wins and it handles circular references). So the actual registration is moved up in
ResolveSchemaReferences. By having this moved up, and because the openapi document now resolves references correctly a lot of logic in ResolveSchemaReferences was not required any more.Subreferences
The logic around sub references is removed as this does not occur in any of the code paths, or it gave unwanted results, for example #64048.
Fixes #64048
Fixes #63598