chore: implement test cases for types of References in an OpenApi document #2352
Conversation
MaggieKimani1
commented
May 14, 2025
MaggieKimani1
commented
- Adds test cases for types of references contained in an OpenApi document as covered in this spec
- Partial - doesn't cover all the cases
There was a problem hiding this comment.
Here are a couple of comments to help move things along.
What I've done:
- high level review of the code.
- review of the samples that are present.
(before reading the spec to come with the freshest eyes possible into that)
What I have not done:
- check against the spec for missing samples.
- deep review of the code.
.../Microsoft.OpenApi.Readers.Tests/V31Tests/ReferenceSamples/rootComponentSchemaReference.yaml
Show resolved
Hide resolved
| - 'null' | ||
| properties: | ||
| b: | ||
| $ref: '#/properties/b' |
There was a problem hiding this comment.
This one looks invalid to me.
If I go back up one "schema level", I'm within the b schema definition, (L12), and there's no b property there. Am I reading it wrong?
There was a problem hiding this comment.
I'm getting it the same way, but this is the same example given in the spec and Safia's example.
So in your opinion the JSON pointer should be #/a/properties/b since a has a b property defined within it?
Maybe @darrelmiller can offer more insight?
There was a problem hiding this comment.
@MaggieKimani1 @baywet You don't "go back up one schema level". You go back up until you reach a "JSON Schema Resource". This is either a schema with a $Id, the root of the document, or in the case of OpenAPI to the component schema. In this case "a" is the JSON Schema Resource which is effectively the root document as far as JSON Schema is concerned. The label "a" is outside of the JSON Schema Resource which is why it is not part of the $ref.
There was a problem hiding this comment.
Thanks for clarifying. I made a similar comment on the spec side PR. I think that "routine" should be explicitly called out there as it's not "broadly known"
| var propName = pathSegments[0]; | ||
| if (schema.Properties != null && schema.Properties.TryGetValue(propName, out var propSchema)) | ||
| return ResolveSubSchema(propSchema, [.. pathSegments.Skip(1)]); | ||
| break; |
There was a problem hiding this comment.
no need for a break after a return
There was a problem hiding this comment.
The break is necessary here to provide a clean exit during execution in case the condition in the if statement doesn't pass
There was a problem hiding this comment.
sorry, my earlier comment was unclear. Could you not leverage a when in the case? and clean all of this up in the process?
There was a problem hiding this comment.
What's wrong with using the current approach?
There was a problem hiding this comment.
you can treat that comment as nitpicking. when usually provides better readability IMHO
Co-authored-by: Vincent Biret <[email protected]>
|
