-
Notifications
You must be signed in to change notification settings - Fork 62
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
Migrate to OpenApi.NET V2.0 #606
base: main
Are you sure you want to change the base?
Conversation
Quality Gate failedFailed conditions |
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Recommendation for reviewers looking at integration files: use a semantic diff like https://yamldiff.com/ and https://jsondiff.com/ |
src/Microsoft.OpenApi.OData.Reader/Generator/OpenApiErrorSchemaGenerator.cs
Show resolved
Hide resolved
@@ -17873,14 +17609,6 @@ paths: | |||
required: true | |||
type: string | |||
x-ms-docs-key-type: Person | |||
- in: header |
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.
Any chance if this is intended? Looks like the annotations for the header are no longer propagated to the $count
segment.
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.
In my opinion this is correct: there's nothing that guarantees that a header supported on the collection will be supported by the raw count operation under it. This specific example might be misleading, because in that specific case, it's likely that mezzo supports the header and raw count.
Maybe @mikepizzo can @Licantrop0 can chime in here.
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.
@Licantrop0 reached out internally. The gist of our discussion is that in the case of consistency level, it should be retained because the service (DS API) does NOT support raw count, and the only way to get it is through mezzo which requires the header.
But we might just as well have cases that are the opposite. E.g. a "billing model" header, that's required to list the elements because it's a read heavy operation, but not for the row count because it's pre-calculated.
@mikepizzo to provide his input. We could decide to leave it like that (do not duplicate custom headers from list operations to raw count) or honour the previous behaviour (duplicate them).
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.
OData vocabularies don't differentiate between headers used when reading a collection versus reading count of a collection (or reading a collection and including the count). The simplifying assumption is that, if you can read the result, you can get the count and that, if the service doesn't need the header for just getting the count, they can ignore it. Doesn't cover certain edge cases, but adding that level of granularity to all of the different ways you could reference the collection didn't seem worth increasing the complexity/verbosity.
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 chiming in! I'll correct the behaviour to revert those changes when I have some times. Yes I agree with the complexity aspect. It'd also require having some additional semantics to say "this header only applies to raw count"... hard to justify without customer demand for it.
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.
actually, after looking at that a second time, all those removals are CORRECT.
they are all on a path that .../AddressInfo/$count or .../AddressInfo/Cast/$count.
They only types where consistency level header is defined as a read restrictions are:
- people
- trips
- person
Which are not in the type hierarchy of Address info.
I believe the previous version of the code was reading the wrong annotable, can coming to the wrong conclusion. Or copying references in a bad way.
Signed-off-by: Vincent Biret <[email protected]>
…ft/OpenAPI.NET.OData into mk/upgrade-oai.net-to-v2
No description provided.