-
Notifications
You must be signed in to change notification settings - Fork 263
Validate OpenAPI schema references #2459
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
Validate OpenAPI schema references #2459
Conversation
src/Microsoft.OpenApi/Validations/Rules/OpenApiDocumentRules.cs
Outdated
Show resolved
Hide resolved
c461489
to
45e3cac
Compare
There was a problem hiding this 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 adds validation for OpenAPI schema references to ensure they point to existing schemas in the document. The validation rule detects both invalid references and circular reference patterns.
- Adds a new validation rule
OpenApiDocumentReferencesAreValid
that checks schema reference validity - Implements detection of circular references with appropriate error messaging
- Updates test counts to reflect the addition of the new validation rule
Reviewed Changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
src/Microsoft.OpenApi/Validations/Rules/OpenApiDocumentRules.cs |
Implements the main validation logic with schema reference visitor |
src/Microsoft.OpenApi/Properties/SRResource.resx |
Adds error message for invalid schema references |
test/Microsoft.OpenApi.Tests/Validations/OpenApiDocumentValidationTests.cs |
Comprehensive test coverage for valid, invalid, and circular references |
test/Microsoft.OpenApi.Tests/Validations/ValidationRuleSetTests.cs |
Updates expected rule count from 19 to 20 |
test/Microsoft.OpenApi.Tests/PublicApi/PublicApi.approved.txt |
Adds new public API for the validation rule |
Files not reviewed (1)
- src/Microsoft.OpenApi/Properties/SRResource.Designer.cs: Language not supported
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
src/Microsoft.OpenApi/Validations/Rules/OpenApiDocumentRules.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.OpenApi/Validations/Rules/OpenApiDocumentRules.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.OpenApi/Validations/Rules/OpenApiDocumentRules.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution!
src/Microsoft.OpenApi/Validations/Rules/OpenApiDocumentRules.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.OpenApi/Validations/Rules/OpenApiDocumentRules.cs
Outdated
Show resolved
Hide resolved
Add validation rule for OpenAPI document schema references. Resolves microsoft#2453.
Add two basic unit tests and a fast path for when the components are registered.
- Improve handling of circular references. - Improve the path.
Add a test for circular schema references.
Copy-pasted into the wrong place during a refactor.
Avoid allocating the segment for the context if the reference is valid.
Remove reparsing of the document and just validate the in-memory `OpenApiDocument` instead.
39c040a
to
216d62b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for making the changes!
@martincostello this change resulted in a 15% memory allocation increase for smaller descriptions Would you please mind looking into it, and see if you can reduce allocations? or follow through with an update to the baseline? |
I'll have a quick look later, but I would imagine it'll be a baseline change. My guess would be that it's paying for enumerators for walking over the whole document tree, but that's the point of the validator (and it's leveraging the existing infrastructure for that). Did your baseline/performance test just never touch the walker before? |
I haven't checked in details but I think it did since the walker is used by other default validation rules. |
Initial findings:
|
You also might find this interesting: Continuous Benchmarks on a Budget This is something I set up for my own projects' benchmarks that lets me track trends in the benchmark results over time and visualise them. |
Update benchmarks for microsoft#2459 investigation.
Update benchmarks for microsoft#2459 investigation after changes.
Having done item 3, there's definitely a difference before and after #2459. The TL;DR for each benchmark is:
As you've noted, there's not much actively being allocated by the rule in and of itself (just My hunch is either something to do with calls into Before
After
|
Thank you for the additional information. The article is great! I had not come across it before. I think so far we've established the memory increase is due to the code change, would you agree with that statement? |
With these changes ce6497e (needs further cleanup before a PR) I get these numbers compared to the re-baseline before #2459:
This reduces the ratios, and I think reinforces my view that the "regression" is just "does more work" rather than any due to any deficiencies in the change made (plus
|
This is great! Yes that's what I meant, as opposed to "is a random change in how Benchmark.net counts things up" |
Opening a PR shortly, but with the latest round of tweaks I get these results:
|
Add validation rule for OpenAPI document schema references.
Resolves #2453.
Initial draft for now based on testing this approach with dotnet/aspnetcore#63095. Needs tests, plus rebasing after #2460 is merged.