-
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
Ensure correct OpenAPI 3.1.0 spec. #646
Conversation
Changes AnalysisCommit SHA: 04716ca API ChangesSummary
ReportThe full API changes report is available at: https://github.com/opensearch-project/opensearch-api-specification/actions/runs/11672332764/artifacts/2143280585 API Coverage
|
8cf1c31
to
05c5254
Compare
Spec Test Coverage Analysis
|
Signed-off-by: dblock <dblock@amazon.com>
…refs. Signed-off-by: dblock <dblock@amazon.com>
Signed-off-by: dblock <dblock@amazon.com>
Signed-off-by: dblock <dblock@amazon.com>
Signed-off-by: dblock <dblock@amazon.com>
Signed-off-by: dblock <dblock@amazon.com>
Signed-off-by: dblock <dblock@amazon.com>
Signed-off-by: dblock <dblock@amazon.com>
Signed-off-by: dblock <dblock@amazon.com>
Signed-off-by: dblock <dblock@amazon.com>
Signed-off-by: dblock <dblock@amazon.com>
Signed-off-by: dblock <dblock@amazon.com>
Signed-off-by: dblock <dblock@amazon.com>
5e65b0a
to
04716ca
Compare
Signed-off-by: dblock <dblock@amazon.com>
Signed-off-by: dblock <dblock@amazon.com>
Signed-off-by: dblock <dblock@amazon.com>
Signed-off-by: dblock <dblock@amazon.com>
Signed-off-by: dblock <dblock@amazon.com>
Signed-off-by: dblock <dblock@amazon.com>
Signed-off-by: dblock <dblock@amazon.com>
Signed-off-by: dblock <dblock@amazon.com>
Signed-off-by: dblock <dblock@amazon.com>
Are there discrepancies between Ruby and Python's validators where they can throw different validation errors? Just wondering why we need both. |
Yes, they are very different codebases and throw some different errors. The Ruby one spits out all errors and was useful for the large categories like In general I like that these are acting like integration tests, ensure that the spec is usable with different libraries, and points to libraries that we know work with the spec. |
spec/_global_parameters.yaml
Outdated
description: Whether to pretty format the returned JSON response. | ||
schema: | ||
type: boolean | ||
default: false | ||
human: | ||
name: human | ||
in: query | ||
description: Whether to return human readable values for statistics. | ||
schema: | ||
type: boolean | ||
default: true | ||
error_trace: | ||
name: error_trace | ||
in: query | ||
description: Whether to include the stack trace of returned errors. | ||
schema: | ||
type: boolean | ||
default: false |
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.
We should move the default values into their respective schemas in this case.
We should also add x-default
extension for parameters when the default for the param differs from the default of the schema OR when a shared schema itself doesn't have a default but certain param using said schema does.
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.
I did the first part, didn't realize that default should/can be nested under schema
when I was removing this.
I don't think we have a case where the default differs, do you know of one?
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.
Haven't come across on where the schema itself has a default that is different from the param that uses it. But I've come across schemas that are used in different params of different operations but have different default values depending on the operation. But That was a long time ago, before we moved onto OpenAPI. But that's still a real possibility.
$ref: '#/components/responses/info@removed-2.0-refs' | ||
$ref: '#/components/responses/info.removed-2.0-refs' | ||
added-1.3-removed-2.0: | ||
$ref: '#/components/responses/info@added-1.3-removed-2.0' |
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.
This gets tricky with operation-group in non-root namespaces, say indices.create
. We will end up with indices.create.removed-2.0
. Visually it's hard to tell which is which. Is it a response for indices
that's named create.remove-2.0
or, a response for indices.create
that's named removed-2.0
.
This is not an issue on the technical term because the CodeGen does not parse the response names (at least the ones I work don't. @Xtansia can chime in), but we loses the visual cue with this change. I'm just stating a cost of this change. It's not a blocker.
We can also replace @
with ___
--> indices.create___removed-2.0
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.
I went with this map.
.replaceAll('::', '___')
.replaceAll('@', '__')
.replaceAll(':', '_')
- $ref: '#/components/schemas/_common:Type' | ||
- $ref: '#/components/schemas/_common:OldId' |
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.
Can we also replace :
with ___
in this case to separate the schema category from the type name? Similar to the previous comment regarding the response naming, this can get confusing with more complex schema categories say ai_dsl.aggregation_common_Type
. It's hard to visually tell where the category ends and where the schema name starts. ai_dsl.aggreation_common___Type
is slightly better and easier for the CodeGen to parse.
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.
Take a look at the update, we have to replace a few things, I went with the following map.
.replaceAll('::', '___')
.replaceAll('@', '__')
.replaceAll(':', '_')
.replaceAll('::', '___') .replaceAll('@', '__') .replaceAll(':', '_') Signed-off-by: dblock <dblock@amazon.com>
Signed-off-by: dblock <dblock@amazon.com>
e4de372
to
d1a1bac
Compare
tools/src/linter/SchemasValidator.ts
Outdated
const file = `schemas/${key.split('_')[0]}.yaml` | ||
const location = `#/components/schemas/${key.split('_')[1]}` |
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.
This won't work with _core.get_script_languages_LanguageContext
since _
is also used in the name of the schema category.
tools/src/merger/OpenApiMerger.ts
Outdated
#normalize_key(key: string): string { | ||
return key | ||
.replaceAll('::', '___') | ||
.replaceAll('@', '__') | ||
.replaceAll(':', '_') | ||
} |
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.
I think it's time for a quick trip down the memory lane to learn why ::
, @
, and :
were chosen as separators for parameter, response, and schema names. Long ago, before this repo was fully in OpenAPI, there were 2 criteria for #component
name separators:
- The character(s) used as a separator cannot appear anywhere else in the $ref. (This makes it easier to tell different component of a name apart, and easier for CodeGen to parse)
- The separators for parameters, responses, and schemas must be easily distinguishable from one another. (This encoding of component type into the separators helps the reader quickly identify where they are in the giant YAML file, and easier to grep for a component definition.
We can ignore criterion No.2 but I think No.1 is worth upholding. So, .replaceAll(':', '_')
is not good. We can use ___
as the separator for all 3 types of #components
if we let go of criteria No.2 OR come up with a different set of separators that still conform to strict OpenAPI 3.1's naming rules. Say ___
, ---
, and ...
.
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.
I believe we're only parsing ::
and @
and :
is pure decoration (otherwise ::
vs. :
would be a problem already). So, with this approach you can split by 3 ___
to get the equivalent of ::
and by 2 __
to get the equivalent of @
, as long as you parse in that order. We do it in tests.
I could definitely use things like ___
vs. ---
, but I find them rather ugly :) WDYT?
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.
It's not gonna be pretty with how restrictive OpenAPI 3.1 is unfortunately. We can simply do
key
.replaceAll('::', '___')
.replaceAll(':', '___')
.replaceAll('@', '___')
and can still tell which kind of OpenAPI #components
we're look at (tho harder):
- Parameters will end with
<path/query>.<param>
(e.g.indices.create___query.index
) - Responses will end with a number (most of the times) (e.g.
indices.create___201
) - Named schemas will end with a PascalCased string (e.g.
_common___ExpandedWildCard
The merged file is mostly read by programs anyway. So, making it easier to parse it more important than human readability.
Eventually we should replace these for all files in the ./spec
folder too so that we dont have 2 different sets of separators to keep track of and reduce overhead for maintainers.
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.
I like using ___
as the separator better, even though technically someone could create a key like ___
to mess with the merger, I don't think it's a real scenario.
I don't know whether we want to just replace all keys in the spec, it's nice to use a ::
and @
convention for readability/developer productivity.
Updated PR with ___
.
Signed-off-by: dblock <dblock@amazon.com>
@Xtansia take a look? |
Seems good to me, nothing that'll be a major issue for .NET or Java codegen |
FYI: this broke a link in the README which will now block all PRs based on this until resolved. i've created a fix for it: #657 |
Description
Use a Python and a Ruby OpenAPI spec validator and ensure correct OpenAPI 3.1.0 spec.
Issues Resolved
Closes #633.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.