Skip to content
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

[BUG] Missing definitions of common/global parameters #167

Closed
Xtansia opened this issue Dec 4, 2023 · 4 comments · Fixed by #241
Closed

[BUG] Missing definitions of common/global parameters #167

Xtansia opened this issue Dec 4, 2023 · 4 comments · Fixed by #241
Labels
bug Something isn't working missing api Missing API or api parameter that needs to be added.

Comments

@Xtansia
Copy link
Collaborator

Xtansia commented Dec 4, 2023

What is the bug?

There are a handful of query parameters which are common across the whole OpenSearch REST API. These are missing from the Smithy specifications, in the legacy spec they were defined in a separate _common.json file: https://github.com/opensearch-project/OpenSearch/blob/main/rest-api-spec/src/main/resources/rest-api-spec/api/_common.json
See also: https://opensearch.org/docs/latest/api-reference/common-parameters/

These need to be added to the spec and preferably attributed as common/global in some manner, as at least the .NET client lifts these to a super class of all requests.

@Xtansia Xtansia added bug Something isn't working untriaged labels Dec 4, 2023
@dblock dblock added missing api Missing API or api parameter that needs to be added. and removed untriaged labels Dec 20, 2023
@nhtruong
Copy link
Collaborator

nhtruong commented Apr 2, 2024

@Xtansia to clarify, we're not going to make every operation explicitly reference these query params right?
We can include this in components.parameters of the root file.

@Xtansia
Copy link
Collaborator Author

Xtansia commented Apr 2, 2024

@nhtruong For the specification to be true to the API they need to be included in all operations one way or another. Can have them injected into each operation/path as part of the merging to the final spec to save having to hand write them in every spec file?

@nhtruong
Copy link
Collaborator

nhtruong commented Apr 2, 2024

Having it injected into single-file spec is a great idea since the filed is not meant to be read by humans anyway. Will update the merger to handle that.

@nhtruong
Copy link
Collaborator

nhtruong commented Apr 2, 2024

I think we should also add some form of extension to tell the generators that these params should be accepted when the user passes them to the method but we don't need to document them in every method description in the client code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working missing api Missing API or api parameter that needs to be added.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants