Skip to content

Conversation

desjoerd
Copy link
Contributor

@desjoerd desjoerd commented Oct 19, 2025

Fix array circular references.

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.

Fixes #64048
Fixes #63598

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.
@desjoerd desjoerd requested review from a team and captainsafia as code owners October 19, 2025 22:11
@Copilot Copilot AI review requested due to automatic review settings October 19, 2025 22:11
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Oct 19, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

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 ResolveReferenceForSchema method
  • 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 github-actions bot added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Oct 19, 2025
@desjoerd
Copy link
Contributor Author

desjoerd commented Oct 19, 2025

@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 Status enum is also used as the response, which should not have the default set based on the parameter info. As it's applied to the componetized schema it would set the default for this schema on all locations that it is used.

When you agree with this direction for the fix I am going to check if it possibly fixes #64024 as well.

Edit: will fix the nullability warnings in the tests tomorrow.

@desjoerd
Copy link
Contributor Author

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. document.AddComponent does this check for us, and registers the component in the correct location of the Workspace.

@desjoerd
Copy link
Contributor Author

desjoerd commented Oct 20, 2025

@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 desjoerd requested a review from Copilot October 20, 2025 21:30
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 8 comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates 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.

OpenApi components schema $ref may not correct OpenAPI: document.Workspace has schemas duplicated

1 participant