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.

Sign up for GitHub

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: Fix generation of correct fields #21942

Merged
merged 2 commits into from
Jul 25, 2023
Merged

OpenAPI: Fix generation of correct fields #21942

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c680341
OpenAPI: Fix generation of correct fields
maxb Jul 19, 2023
cfeb6a7
changelog
maxb Jul 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
OpenAPI: Fix generation of correct fields 
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.
  • Loading branch information
@maxb
maxb committed Jul 19, 2023
commit c68034157f188adfb937cf240cde7e40dd75ce0c
172 changes: 106 additions & 66 deletions sdk/framework/openapi.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ func documentPath(p *Path, backend *Backend, requestResponsePrefix string, doc *

// Convert optional parameters into distinct patterns to be processed independently.
forceUnpublished := false
paths, err := expandPattern(p.Pattern)
paths, captures, err := expandPattern(p.Pattern)
if err != nil {
if errors.Is(err, errUnsupportableRegexpOperationForOpenAPI) {
// Pattern cannot be transformed into sensible OpenAPI paths. In this case, we override the later
Expand Down Expand Up @@ -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)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


for name, field := range pathFields {
location := "path"
required := true

if field == nil {
continue
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


if field.Query {
location = "query"
required = false
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

t := convertType(field.Type)
p := OASParameter{
Name: name,
Description: cleanString(field.Description),
In: location,
In: "path",
Schema: &OASSchema{
Type: t.baseType,
Pattern: t.pattern,
Enum: field.AllowedValues,
Default: field.Default,
DisplayAttrs: withoutOperationHints(field.DisplayAttrs),
},
Required: required,
Required: true,
Deprecated: field.Deprecated,
}
pi.Parameters = append(pi.Parameters, p)
Expand Down Expand Up @@ -340,8 +328,12 @@ func documentPath(p *Path, backend *Backend, requestResponsePrefix string, doc *
op.Deprecated = props.Deprecated
op.OperationID = operationID

// Add any fields not present in the path as body parameters for POST.
if opType == logical.CreateOperation || opType == logical.UpdateOperation {
switch opType {
// For the operation types which map to POST/PUT methods, and so allow for request body parameters,
// prepare the request body definition
case logical.CreateOperation:
fallthrough
case logical.UpdateOperation:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

s := &OASSchema{
Type: "object",
Properties: make(map[string]*OASSchema),
Expand All @@ -355,27 +347,14 @@ func documentPath(p *Path, backend *Backend, requestResponsePrefix string, doc *
continue
}

openapiField := convertType(field.Type)
if field.Required {
s.Required = append(s.Required, name)
}
addFieldToOASSchema(s, name, field)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

}

p := OASSchema{
Type: openapiField.baseType,
Description: cleanString(field.Description),
Format: openapiField.format,
Pattern: openapiField.pattern,
Enum: field.AllowedValues,
Default: field.Default,
Deprecated: field.Deprecated,
DisplayAttrs: withoutOperationHints(field.DisplayAttrs),
}
if openapiField.baseType == "array" {
p.Items = &OASSchema{
Type: openapiField.items,
}
}
s.Properties[name] = &p
// Contrary to what one might guess, fields marked with "Query: true" are only query fields when the
// request method is one which does not allow for a request body - they are still body fields when
// dealing with a POST/PUT request.
for name, field := range queryFields {
addFieldToOASSchema(s, name, field)
}

// Make the ordering deterministic, so that the generated OpenAPI spec document, observed over several
Expand All @@ -401,19 +380,40 @@ func documentPath(p *Path, backend *Backend, requestResponsePrefix string, doc *
},
}
}
}

// LIST is represented as GET with a `list` query parameter. Code later on in this function will assign
// list operations to a path with an extra trailing slash, ensuring they do not collide with read
// operations.
if opType == logical.ListOperation {
// For the operation types which map to HTTP methods without a request body, populate query parameters
case logical.ListOperation:
// LIST is represented as GET with a `list` query parameter. Code later on in this function will assign
// list operations to a path with an extra trailing slash, ensuring they do not collide with read
// operations.
op.Parameters = append(op.Parameters, OASParameter{
Name: "list",
Description: "Must be set to `true`",
Required: true,
In: "query",
Schema: &OASSchema{Type: "string", Enum: []interface{}{"true"}},
})
fallthrough
case logical.DeleteOperation:
fallthrough
case logical.ReadOperation:
for name, field := range queryFields {
t := convertType(field.Type)
p := OASParameter{
Name: name,
Description: cleanString(field.Description),
In: "query",
Schema: &OASSchema{
Type: t.baseType,
Pattern: t.pattern,
Enum: field.AllowedValues,
Default: field.Default,
DisplayAttrs: withoutOperationHints(field.DisplayAttrs),
},
Deprecated: field.Deprecated,
}
op.Parameters = append(op.Parameters, p)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}

// Add tags based on backend type
Expand Down Expand Up @@ -587,6 +587,31 @@ func documentPath(p *Path, backend *Backend, requestResponsePrefix string, doc *
return nil
}

func addFieldToOASSchema(s *OASSchema, name string, field *FieldSchema) {
openapiField := convertType(field.Type)
if field.Required {
s.Required = append(s.Required, name)
}

p := OASSchema{
Type: openapiField.baseType,
Description: cleanString(field.Description),
Format: openapiField.format,
Pattern: openapiField.pattern,
Enum: field.AllowedValues,
Default: field.Default,
Deprecated: field.Deprecated,
DisplayAttrs: withoutOperationHints(field.DisplayAttrs),
}
if openapiField.baseType == "array" {
p.Items = &OASSchema{
Type: openapiField.items,
}
}

s.Properties[name] = &p
}

// specialPathMatch checks whether the given path matches one of the special
// paths, taking into account * and + wildcards (e.g. foo/+/bar/*)
func specialPathMatch(path string, specialPaths []string) bool {
Expand Down Expand Up @@ -751,8 +776,9 @@ func constructOperationID(
}

// expandPattern expands a regex pattern by generating permutations of any optional parameters
// and changing named parameters into their {openapi} equivalents.
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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

// the hard work of interpreting the regexp syntax.
rx, err := syntax.Parse(pattern, syntax.Perl)
Expand All @@ -762,12 +788,12 @@ func expandPattern(pattern string) ([]string, error) {
panic(err)
}

paths, err := collectPathsFromRegexpAST(rx)
paths, captures, err = collectPathsFromRegexpAST(rx)
if err != nil {
return nil, err
return nil, nil, err
}

return paths, nil
return paths, captures, nil
}

type pathCollector struct {
Expand All @@ -788,23 +814,28 @@ type pathCollector struct {
//
// Each named capture group - i.e. (?P<name>something here) - is replaced with an OpenAPI parameter - i.e. {name} - and
// the subtree of regexp AST inside the parameter is completely skipped.
func collectPathsFromRegexpAST(rx *syntax.Regexp) ([]string, error) {
pathCollectors, err := collectPathsFromRegexpASTInternal(rx, []*pathCollector{{}})
func collectPathsFromRegexpAST(rx *syntax.Regexp) (paths []string, captures map[string]struct{}, err error) {
captures = make(map[string]struct{})
pathCollectors, err := collectPathsFromRegexpASTInternal(rx, []*pathCollector{{}}, captures)
if err != nil {
return nil, err
return nil, nil, err
}
paths := make([]string, 0, len(pathCollectors))
paths = make([]string, 0, len(pathCollectors))
for _, collector := range pathCollectors {
if collector.conditionalSlashAppendedAtLength != collector.Len() {
paths = append(paths, collector.String())
}
}
return paths, nil
return paths, captures, nil
}

var errUnsupportableRegexpOperationForOpenAPI = errors.New("path regexp uses an operation that cannot be translated to an OpenAPI pattern")

func collectPathsFromRegexpASTInternal(rx *syntax.Regexp, appendingTo []*pathCollector) ([]*pathCollector, error) {
func collectPathsFromRegexpASTInternal(
rx *syntax.Regexp,
appendingTo []*pathCollector,
captures map[string]struct{},
) ([]*pathCollector, error) {
var err error

// Depending on the type of this regexp AST node (its Op, i.e. operation), figure out whether it contributes any
Expand All @@ -831,7 +862,7 @@ func collectPathsFromRegexpASTInternal(rx *syntax.Regexp, appendingTo []*pathCol
// those pieces.
case syntax.OpConcat:
for _, child := range rx.Sub {
appendingTo, err = collectPathsFromRegexpASTInternal(child, appendingTo)
appendingTo, err = collectPathsFromRegexpASTInternal(child, appendingTo, captures)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -862,7 +893,7 @@ func collectPathsFromRegexpASTInternal(rx *syntax.Regexp, appendingTo []*pathCol
childAppendingTo = append(childAppendingTo, newCollector)
}
}
childAppendingTo, err = collectPathsFromRegexpASTInternal(child, childAppendingTo)
childAppendingTo, err = collectPathsFromRegexpASTInternal(child, childAppendingTo, captures)
if err != nil {
return nil, err
}
Expand All @@ -880,7 +911,7 @@ func collectPathsFromRegexpASTInternal(rx *syntax.Regexp, appendingTo []*pathCol
newCollector.conditionalSlashAppendedAtLength = collector.conditionalSlashAppendedAtLength
childAppendingTo = append(childAppendingTo, newCollector)
}
childAppendingTo, err = collectPathsFromRegexpASTInternal(child, childAppendingTo)
childAppendingTo, err = collectPathsFromRegexpASTInternal(child, childAppendingTo, captures)
if err != nil {
return nil, err
}
Expand All @@ -902,7 +933,7 @@ func collectPathsFromRegexpASTInternal(rx *syntax.Regexp, appendingTo []*pathCol
// In Vault, an unnamed capturing group is not actually used for capturing.
// We treat it exactly the same as OpConcat.
for _, child := range rx.Sub {
appendingTo, err = collectPathsFromRegexpASTInternal(child, appendingTo)
appendingTo, err = collectPathsFromRegexpASTInternal(child, appendingTo, captures)
if err != nil {
return nil, err
}
Expand All @@ -915,6 +946,7 @@ func collectPathsFromRegexpASTInternal(rx *syntax.Regexp, appendingTo []*pathCol
builder.WriteString(rx.Name)
builder.WriteRune('}')
}
captures[rx.Name] = struct{}{}
}

// Any other kind of operation is a problem, and will trigger an error, resulting in the pattern being left out of
Expand Down Expand Up @@ -1016,29 +1048,37 @@ func cleanString(s string) string {
return s
}

// splitFields partitions fields into path and body groups
// The input pattern is expected to have been run through expandPattern,
// with paths parameters denotes in {braces}.
func splitFields(allFields map[string]*FieldSchema, pattern string) (pathFields, bodyFields map[string]*FieldSchema) {
// splitFields partitions fields into path, query and body groups. It uses information on capturing groups previously
// collected by expandPattern, which is necessary to correctly match the treatment in (*Backend).HandleRequest:
// a field counts as a path field if it appears in any capture in the regex, and if that capture was inside an
// alternation or optional part of the regex which does not survive in the OpenAPI path pattern currently being
// processed, that field should NOT be rendered to the OpenAPI spec AT ALL.
func splitFields(
allFields map[string]*FieldSchema,
openAPIPathPattern string,
captures map[string]struct{},
) (pathFields, queryFields, bodyFields map[string]*FieldSchema) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

pathFields = make(map[string]*FieldSchema)
queryFields = make(map[string]*FieldSchema)
bodyFields = make(map[string]*FieldSchema)

for _, match := range pathFieldsRe.FindAllStringSubmatch(pattern, -1) {
for _, match := range pathFieldsRe.FindAllStringSubmatch(openAPIPathPattern, -1) {
name := match[1]
pathFields[name] = allFields[name]
}

for name, field := range allFields {
if _, ok := pathFields[name]; !ok {
// Any field which relates to a regex capture was already processed above, if it needed to be.
if _, ok := captures[name]; !ok {
if field.Query {
pathFields[name] = field
queryFields[name] = field
} else {
bodyFields[name] = field
}
}
}

return pathFields, bodyFields
return pathFields, queryFields, bodyFields
}

// withoutOperationHints returns a copy of the given DisplayAttributes without
Expand Down
Loading