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

OpenAPI: Define default response structure for ListOperations #21934

Merged
merged 6 commits into from Jul 25, 2023

Conversation

maxb
Copy link
Contributor

@maxb maxb commented Jul 19, 2023

Almost all Vault ListOperation responses have an identical response
schema. Update the OpenAPI generator to know this, and remove a few
instances where that standard response schema had been manually
copy/pasted into place in individual endpoints.

Almost all Vault ListOperation responses have an identical response
schema. Update the OpenAPI generator to know this, and remove a few
instances where that standard response schema had been manually
copy/pasted into place in individual endpoints.
@maxb maxb requested a review from a team July 19, 2023 08:11
@maxb
Copy link
Contributor Author

maxb commented Jul 19, 2023

See above two auto-referenced PRs:

  • Some cleanup in KV plugin repo too

  • Draft PR exhibiting preview of affect on vault-client-go code generation

Copy link
Contributor

@cipherboy cipherboy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

General commentary, but you could define the standard ListResponseWithInfo(...):

// ListResponseWithInfo is used to format a response to a list operation and
// return the keys as well as a map with corresponding key info.
func ListResponseWithInfo(keys []string, keyInfo map[string]interface{}) *Response {
resp := ListResponse(keys)
keyInfoData := make(map[string]interface{})
for _, key := range keys {
val, ok := keyInfo[key]
if ok {
keyInfoData[key] = val
}
}
if len(keyInfoData) > 0 {
resp.Data["key_info"] = keyInfoData
}
return resp
}

This should get you a standard schema for a few other list endpoints, such as in SSH, PKI, and apparently some Core stuff too. :-)

@maxb
Copy link
Contributor Author

maxb commented Jul 19, 2023

General commentary, but you could define the standard ListResponseWithInfo(...)

However, each user of ListResponseWithInfo(...) gets to define their own schema on a case-by-case basis for the info payload values in the key_info map, therefore there is no standard schema I can apply generally - unlike the base keys field, which is standardized to be a slice of string in every case.

In this PR, I'd like to just cover what can be done centrally from sdk/framework/openapi.go.

I do see that it would be nice in future to add a helper function to abstract away the common parts of the structure created in ListResponseWithInfo(...) but it would need to have a parameter to allow each individual caller to specify their variant of the nested info structure.

@cipherboy
Copy link
Contributor

@maxb said:

therefore there is no standard schema I can apply generally

I don't disagree, but many of the interfaces simply use, e.g.:

"key_info": {
Type: framework.TypeMap,
Description: `Key info with issuer name`,
Required: false,
},

which seems to be a generic schema? Or is what this endpoint is doing incorrect, and there shouldn't be a generic TypeMap in the response info, and instead every inner field of key_info also defined?

@maxb
Copy link
Contributor Author

maxb commented Jul 19, 2023

Or is what this endpoint is doing incorrect, and there shouldn't be a generic TypeMap in the response info, and instead every inner field of key_info also defined?

Yes, this is exactly what we need to happen, if generated client libraries are to be able to render responses into endpoint specific data structures, consistent with the rest of the initiative of documenting response structures.

An additional consideration, is that there is currently no declarative specification of which endpoints actually return key_info on list - so there's no way to handle this other than adding individual configuration to each endpoint that does that.

@averche averche self-requested a review July 19, 2023 14:57
averche
averche previously approved these changes Jul 24, 2023
Copy link
Contributor

@averche averche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for the contribution!

@averche averche added this to the 1.15 milestone Jul 24, 2023
@averche
Copy link
Contributor

averche commented Jul 24, 2023

Actually, I just noticed there are some OpenAPI related test failures, e.g. (TestSystemBackend_OpenAPI). Could you please take a look, @maxb.

@maxb
Copy link
Contributor Author

maxb commented Jul 25, 2023

I just noticed there are some OpenAPI related test failures

Sorry about that... there are three PR checks which always fail on community PRs in this repo so I'm habituated to ignoring the red Xs :-(

Fixed.

@averche
Copy link
Contributor

averche commented Jul 25, 2023

Sorry about that... there are three PR checks which always fail on community PRs in this repo so I'm habituated to ignoring the red Xs :-(

We should really fix these sometime :)

Copy link
Contributor

@averche averche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing the tests! 🎉

@averche averche merged commit e057ee0 into hashicorp:main Jul 25, 2023
85 of 88 checks passed
averche pushed a commit to hashicorp/vault-plugin-secrets-kv that referenced this pull request Jul 25, 2023
@maxb maxb deleted the openapi-standard-list-response branch July 25, 2023 15:37
averche pushed a commit to hashicorp/vault-client-go that referenced this pull request Jul 25, 2023
* hashicorp/vault#21934 - a standard response structure is applied to
  all list operations that do not explicitly override it

* hashicorp/vault#22043 - query parameters will now appear as method
  parameters in alphabetic order, rather than at the whims of Go map
  iteration order
averche pushed a commit to hashicorp/vault-client-dotnet that referenced this pull request Jul 25, 2023
Sync OpenAPI: StandardListResponse & sorted query parameters

* hashicorp/vault#21934 - a standard response structure is applied to
  all list operations that do not explicitly override it

* hashicorp/vault#22043 - query parameters will now appear as method
  parameters in alphabetic order, rather than at the whims of Go map
  iteration order
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants