OpenAPI: Fix generation of correct fields #21942

maxb · 2023-07-19T14:30:48Z

Currently, the OpenAPI generator logic is wrong about how it maps from
Vault framework fields to OpenAPI. This manifests most obviously with
endpoints making use of framework.OptionalParamRegex or similar
regex-level optional path parameters, and results in various incorrect
fields showing up in the generated request structures.

The fix is a bit complicated, but in essence is just rewriting the
OpenAPI logic to properly parallel the real request processing logic.

With these changes:

A path parameter in an optional part of the regex, no longer gets
erroneously treated as a body parameter when creating OpenAPI
endpoints that do not include the optional parameter.
A field marked as Query: true no longer gets incorrectly skipped
when creating OpenAPI POST operations.

Currently, the OpenAPI generator logic is wrong about how it maps from Vault framework fields to OpenAPI. This manifests most obviously with endpoints making use of `framework.OptionalParamRegex` or similar regex-level optional path parameters, and results in various incorrect fields showing up in the generated request structures. The fix is a bit complicated, but in essence is just rewriting the OpenAPI logic to properly parallel the real request processing logic. With these changes: * A path parameter in an optional part of the regex, no longer gets erroneously treated as a body parameter when creating OpenAPI endpoints that do not include the optional parameter. * A field marked as `Query: true` no longer gets incorrectly skipped when creating OpenAPI `POST` operations.

maxb · 2023-07-19T14:38:21Z

Please consult hashicorp/vault-client-go#199 for a more direct view of the results of this change, and why we need it.

maxb · 2023-07-21T08:20:54Z

sdk/framework/openapi.go

-
-			if field == nil {
-				continue
-			}


Note for reviewer: I have deliberately deleted this nil check. I am confident it is redundant. There is no reason for this map to contain nil values, and furthermore, the other field maps are not similarly checked. If I was wrong about that, the tests, which do test generation of the full OpenAPI document, would have revealed the issue.

maxb · 2023-07-24T08:44:34Z

sdk/framework/openapi.go

@@ -268,34 +268,22 @@ func documentPath(p *Path, backend *Backend, requestResponsePrefix string, doc *

 		// Process path and header parameters, which are common to all operations.
 		// Body fields will be added to individual operations.
-		pathFields, bodyFields := splitFields(p.Fields, path)
+		pathFields, queryFields, bodyFields := splitFields(p.Fields, path, captures)


Note for reviewer: expandPattern will now collect extra information about capture groups found in the regex, which gets relayed to splitFields. splitFields will now make use of that, and cleanly separate path parameters from Query: true parameters, which were previously being combined into pathFields.

maxb · 2023-07-24T08:45:00Z

sdk/framework/openapi.go

-				location = "query"
-				required = false
-			}
-


Note for reviewer: This loop is now handling path parameters exclusively.

maxb · 2023-07-24T08:45:44Z

sdk/framework/openapi.go

+			// prepare the request body definition
+			case logical.CreateOperation:
+				fallthrough
+			case logical.UpdateOperation:


Note for reviewer: Swapping if for switch because I need to increase the number of cases, and can make good use of fallthrough.

maxb · 2023-07-24T08:48:06Z

sdk/framework/openapi.go

-					if field.Required {
-						s.Required = append(s.Required, name)
-					}
+					addFieldToOASSchema(s, name, field)


Note for reviewer: the removed code here is being factored out to addFieldToOASSchema so that I can call it twice, once ranging over bodyFields, once ranging over queryFields.

maxb · 2023-07-24T08:50:13Z

sdk/framework/openapi.go

+						Deprecated: field.Deprecated,
+					}
+					op.Parameters = append(op.Parameters, p)
+				}


Note for reviewer: This added code is an approximate copy of the version handling path parameters at the top of this function.

maxb · 2023-07-24T08:53:06Z

sdk/framework/openapi.go

-func expandPattern(pattern string) ([]string, error) {
+// and changing named parameters into their {openapi} equivalents. It also returns the names of all capturing groups
+// observed in the pattern.
+func expandPattern(pattern string) (paths []string, captures map[string]struct{}, err error) {
 	// Happily, the Go regexp library exposes its underlying "parse to AST" functionality, so we can rely on that to do


Note for reviewer: The next several chunks of changes, throughout expandPattern, collectPathsFromRegexpAST and collectPathsFromRegexpASTInternal are all just about recording the name of every observed regex capture group, and returning that data through several layers of function.

maxb · 2023-07-24T08:55:26Z

sdk/framework/openapi.go

+	allFields map[string]*FieldSchema,
+	openAPIPathPattern string,
+	captures map[string]struct{},
+) (pathFields, queryFields, bodyFields map[string]*FieldSchema) {


Note for reviewer: And here, right at the end of the diff, is the actual point of this PR - to correctly categorize all defined fields, in the same way the actual request processing will.

averche · 2023-07-25T03:03:37Z

Please consult hashicorp/vault-client-go#199 for a more direct view of the results of this change, and why we need it.

This was super helpful to understand the changes, thanks!

averche

Thanks a lot again for this work, @maxb! I had previously noticed the potential issues with /v1/sys/tools/random/{source}/{urlbytes} and similar requests but was not brave enough to attempt a fix. This is awesome! 🎉

In my recent hashicorp#21942, I overlooked the need to sort another part of the OpenAPI document to ensure stable output. I've also removed `strings.ToLower()` from the code I copied from, as this code is sorting Vault API parameter names, which are all lowercase anyway!

In my recent #21942, I overlooked the need to sort another part of the OpenAPI document to ensure stable output. I've also removed `strings.ToLower()` from the code I copied from, as this code is sorting Vault API parameter names, which are all lowercase anyway!

This includes the results of: * hashicorp/vault#21949 * hashicorp/vault#21942 * hashicorp/vault#22027 The general direction of change is pretty positive, but in reviewing the generated diffs, I have spotted an issue: we are now generating correct parameters for query parameters... but those query parameters are being handled as individual function parameters... which are generated in the order listed in the OpenAPI spec... which is dependent on random Go hash iteration ordering. Ugh. Oh well, let's catch up with latest developments in the Vault repo for now, and I'll go put in a new PR to at least sort the parameters alphabetically. Longer term, this is probably going to push us in the direction of excluding the GET version of APIs with equivalent GET and POST versions from the generated libraries.

…150) * Sync OpenAPI; Multiple parameter correctness changes This includes the results of: * hashicorp/vault#21949 * hashicorp/vault#21942 * hashicorp/vault#22027 The general direction of change is pretty positive, but in reviewing the generated diffs, I have spotted an issue: we are now generating correct parameters for query parameters... but those query parameters are being handled as individual function parameters... which are generated in the order listed in the OpenAPI spec... which is dependent on random Go hash iteration ordering. Ugh. Oh well, let's catch up with latest developments in the Vault repo for now, and I'll go put in a new PR to at least sort the parameters alphabetically. Longer term, this is probably going to push us in the direction of excluding the GET version of APIs with equivalent GET and POST versions from the generated libraries. * Update the templates with a missing using directive

maxb added 2 commits July 19, 2023 15:19

changelog

cfeb6a7

maxb mentioned this pull request Jul 19, 2023

Preview change from Vault PR hashicorp/vault-client-go#199

Closed

averche self-requested a review July 19, 2023 14:53

averche self-assigned this Jul 19, 2023

averche added the core/openapi label Jul 19, 2023

maxb commented Jul 21, 2023

View reviewed changes

maxb mentioned this pull request Jul 21, 2023

Unable to set query params on api_auth.OIDCReadCallback on v0.2 hashicorp/vault-client-go#155

Closed

maxb commented Jul 24, 2023

View reviewed changes

averche added this to the 1.15 milestone Jul 25, 2023

averche approved these changes Jul 25, 2023

View reviewed changes

averche merged commit 8e4409d into hashicorp:main Jul 25, 2023

maxb deleted the fix-openapi-fields branch July 25, 2023 03:40

This was referenced Jul 25, 2023

Sync OpenAPI; Multiple parameter correctness changes hashicorp/vault-client-go#206

Merged

Sync OpenAPI & template fix; Multiple parameter correctness changes hashicorp/vault-client-dotnet#150

Merged

OpenAPI: Sort parameters for stable output #22043

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OpenAPI: Fix generation of correct fields #21942

OpenAPI: Fix generation of correct fields #21942

maxb commented Jul 19, 2023

maxb commented Jul 19, 2023

maxb Jul 21, 2023

maxb Jul 24, 2023

maxb Jul 24, 2023

maxb Jul 24, 2023

maxb Jul 24, 2023

maxb Jul 24, 2023

maxb Jul 24, 2023

maxb Jul 24, 2023

averche commented Jul 25, 2023

averche left a comment

OpenAPI: Fix generation of correct fields #21942

OpenAPI: Fix generation of correct fields #21942

Conversation

maxb commented Jul 19, 2023

maxb commented Jul 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

averche commented Jul 25, 2023

averche left a comment

Choose a reason for hiding this comment