openapi: Improve operationId/request/response naming strategy (#19319)

This commit is contained in:
Anton Averchenkov 2023-04-04 13:14:40 -04:00 committed by GitHub
parent 28e68ae86d
commit 4564a3534b
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
8 changed files with 454 additions and 123 deletions

3
changelog/19319.txt Normal file
View File

@ -0,0 +1,3 @@
```release-note:improvement
openapi: Improve operationId/request/response naming strategy
```

View File

@ -244,7 +244,7 @@ func documentPath(p *Path, specialPaths *logical.Paths, requestResponsePrefix st
}
}
for _, path := range paths {
for pathIndex, path := range paths {
// Construct a top level PathItem which will be populated as the path is processed.
pi := OASPathItem{
Description: cleanString(p.HelpSynopsis),
@ -252,7 +252,7 @@ func documentPath(p *Path, specialPaths *logical.Paths, requestResponsePrefix st
pi.Sudo = specialPathMatch(path, sudoPaths)
pi.Unauthenticated = specialPathMatch(path, unauthPaths)
pi.DisplayAttrs = p.DisplayAttrs
pi.DisplayAttrs = withoutOperationHints(p.DisplayAttrs)
// If the newer style Operations map isn't defined, create one from the legacy fields.
operations := p.Operations
@ -294,7 +294,7 @@ func documentPath(p *Path, specialPaths *logical.Paths, requestResponsePrefix st
Pattern: t.pattern,
Enum: field.AllowedValues,
Default: field.Default,
DisplayAttrs: field.DisplayAttrs,
DisplayAttrs: withoutOperationHints(field.DisplayAttrs),
},
Required: required,
Deprecated: field.Deprecated,
@ -331,9 +331,19 @@ func documentPath(p *Path, specialPaths *logical.Paths, requestResponsePrefix st
op := NewOASOperation()
operationID := constructOperationID(
path,
pathIndex,
p.DisplayAttrs,
opType,
props.DisplayAttrs,
requestResponsePrefix,
)
op.Summary = props.Summary
op.Description = props.Description
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 {
@ -363,7 +373,7 @@ func documentPath(p *Path, specialPaths *logical.Paths, requestResponsePrefix st
Enum: field.AllowedValues,
Default: field.Default,
Deprecated: field.Deprecated,
DisplayAttrs: field.DisplayAttrs,
DisplayAttrs: withoutOperationHints(field.DisplayAttrs),
}
if openapiField.baseType == "array" {
p.Items = &OASSchema{
@ -381,7 +391,7 @@ func documentPath(p *Path, specialPaths *logical.Paths, requestResponsePrefix st
// Set the final request body. Only JSON request data is supported.
if len(s.Properties) > 0 || s.Example != nil {
requestName := constructRequestResponseName(path, requestResponsePrefix, "Request")
requestName := hyphenatedToTitleCase(operationID) + "Request"
doc.Components.Schemas[requestName] = s
op.RequestBody = &OASRequestBody{
Required: true,
@ -477,7 +487,7 @@ func documentPath(p *Path, specialPaths *logical.Paths, requestResponsePrefix st
Enum: field.AllowedValues,
Default: field.Default,
Deprecated: field.Deprecated,
DisplayAttrs: field.DisplayAttrs,
DisplayAttrs: withoutOperationHints(field.DisplayAttrs),
}
if openapiField.baseType == "array" {
p.Items = &OASSchema{
@ -488,7 +498,7 @@ func documentPath(p *Path, specialPaths *logical.Paths, requestResponsePrefix st
}
if len(resp.Fields) != 0 {
responseName := constructRequestResponseName(path, requestResponsePrefix, "Response")
responseName := hyphenatedToTitleCase(operationID) + "Response"
doc.Components.Schemas[responseName] = responseSchema
content = OASContent{
"application/json": &OASMediaTypeObject{
@ -520,33 +530,6 @@ func documentPath(p *Path, specialPaths *logical.Paths, requestResponsePrefix st
return nil
}
// constructRequestResponseName joins the given path with prefix & suffix into
// a CamelCase request or response name.
//
// For example, path=/config/lease/{name}, prefix="secret", suffix="request"
// will result in "SecretConfigLeaseRequest"
func constructRequestResponseName(path, prefix, suffix string) string {
var b strings.Builder
title := cases.Title(language.English)
b.WriteString(title.String(prefix))
// split the path by / _ - separators
for _, token := range strings.FieldsFunc(path, func(r rune) bool {
return r == '/' || r == '_' || r == '-'
}) {
// exclude request fields
if !strings.ContainsAny(token, "{}") {
b.WriteString(title.String(token))
}
}
b.WriteString(suffix)
return b.String()
}
// 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 {
@ -599,6 +582,117 @@ func specialPathMatch(path string, specialPaths []string) bool {
return false
}
// constructOperationID joins the given inputs into a hyphen-separated
// lower-case operation id, which is also used as a prefix for request and
// response names.
//
// The OperationPrefix / -Verb / -Suffix found in display attributes will be
// used, if provided. Otherwise, the function falls back to using the path and
// the operation.
//
// Examples of generated operation identifiers:
// - kvv2-write
// - kvv2-read
// - google-cloud-login
// - google-cloud-write-role
func constructOperationID(
path string,
pathIndex int,
pathAttributes *DisplayAttributes,
operation logical.Operation,
operationAttributes *DisplayAttributes,
defaultPrefix string,
) string {
var (
prefix string
verb string
suffix string
)
if operationAttributes != nil {
prefix = operationAttributes.OperationPrefix
verb = operationAttributes.OperationVerb
suffix = operationAttributes.OperationSuffix
}
if pathAttributes != nil {
if prefix == "" {
prefix = pathAttributes.OperationPrefix
}
if verb == "" {
verb = pathAttributes.OperationVerb
}
if suffix == "" {
suffix = pathAttributes.OperationSuffix
}
}
// A single suffix string can contain multiple pipe-delimited strings. To
// determine the actual suffix, we attempt to match it by the index of the
// paths returned from `expandPattern(...)`. For example:
//
// pki/
// Pattern: "keys/generate/(internal|exported|kms)",
// DisplayAttrs: {
// ...
// OperationSuffix: "internal-key|exported-key|kms-key",
// },
//
// will expand into three paths and corresponding suffixes:
//
// path 0: "keys/generate/internal" suffix: internal-key
// path 1: "keys/generate/exported" suffix: exported-key
// path 2: "keys/generate/kms" suffix: kms-key
//
pathIndexOutOfRange := false
if suffixes := strings.Split(suffix, "|"); len(suffixes) > 1 || pathIndex > 0 {
// if the index is out of bounds, fall back to the old logic
if pathIndex >= len(suffixes) {
suffix = ""
pathIndexOutOfRange = true
} else {
suffix = suffixes[pathIndex]
}
}
// a helper that hyphenates & lower-cases the slice except the empty elements
toLowerHyphenate := func(parts []string) string {
filtered := make([]string, 0, len(parts))
for _, e := range parts {
if e != "" {
filtered = append(filtered, e)
}
}
return strings.ToLower(strings.Join(filtered, "-"))
}
// fall back to using the path + operation to construct the operation id
var (
needPrefix = prefix == "" && verb == ""
needVerb = verb == ""
needSuffix = suffix == "" && (verb == "" || pathIndexOutOfRange)
)
if needPrefix {
prefix = defaultPrefix
}
if needVerb {
if operation == logical.UpdateOperation {
verb = "write"
} else {
verb = string(operation)
}
}
if needSuffix {
suffix = toLowerHyphenate(nonWordRe.Split(path, -1))
}
return toLowerHyphenate([]string{prefix, verb, suffix})
}
// 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) {
@ -890,6 +984,40 @@ func splitFields(allFields map[string]*FieldSchema, pattern string) (pathFields,
return pathFields, bodyFields
}
// withoutOperationHints returns a copy of the given DisplayAttributes without
// OperationPrefix / OperationVerb / OperationSuffix since we don't need these
// fields in the final output.
func withoutOperationHints(in *DisplayAttributes) *DisplayAttributes {
if in == nil {
return nil
}
copy := *in
copy.OperationPrefix = ""
copy.OperationVerb = ""
copy.OperationSuffix = ""
// return nil if all fields are empty to avoid empty JSON objects
if copy == (DisplayAttributes{}) {
return nil
}
return &copy
}
func hyphenatedToTitleCase(in string) string {
var b strings.Builder
title := cases.Title(language.English, cases.NoLower)
for _, word := range strings.Split(in, "-") {
b.WriteString(title.String(word))
}
return b.String()
}
// cleanedResponse is identical to logical.Response but with nulls
// removed from from JSON encoding
type cleanedResponse struct {
@ -924,6 +1052,9 @@ func cleanResponse(resp *logical.Response) *cleanedResponse {
// postSysToolsRandomUrlbytes_2
//
// An optional user-provided suffix ("context") may also be appended.
//
// Deprecated: operationID's are now populated using `constructOperationID`.
// This function is here for backwards compatibility with older plugins.
func (d *OASDocument) CreateOperationIDs(context string) {
opIDCount := make(map[string]int)
var paths []string
@ -951,6 +1082,10 @@ func (d *OASDocument) CreateOperationIDs(context string) {
continue
}
if oasOperation.OperationID != "" {
continue
}
// Discard "_mount_path" from any {thing_mount_path} parameters
path = strings.Replace(path, "_mount_path", "", 1)

View File

@ -564,66 +564,6 @@ func TestOpenAPI_Paths(t *testing.T) {
})
}
func TestOpenAPI_OperationID(t *testing.T) {
path1 := &Path{
Pattern: "foo/" + GenericNameRegex("id"),
Fields: map[string]*FieldSchema{
"id": {Type: TypeString},
},
Operations: map[logical.Operation]OperationHandler{
logical.ReadOperation: &PathOperation{},
logical.UpdateOperation: &PathOperation{},
logical.DeleteOperation: &PathOperation{},
},
}
path2 := &Path{
Pattern: "Foo/" + GenericNameRegex("id"),
Fields: map[string]*FieldSchema{
"id": {Type: TypeString},
},
Operations: map[logical.Operation]OperationHandler{
logical.ReadOperation: &PathOperation{},
},
}
for _, context := range []string{"", "bar"} {
doc := NewOASDocument("version")
err := documentPath(path1, nil, "kv", logical.TypeLogical, doc)
if err != nil {
t.Fatal(err)
}
err = documentPath(path2, nil, "kv", logical.TypeLogical, doc)
if err != nil {
t.Fatal(err)
}
doc.CreateOperationIDs(context)
tests := []struct {
path string
op string
opID string
}{
{"/Foo/{id}", "get", "getFooId"},
{"/foo/{id}", "get", "getFooId_2"},
{"/foo/{id}", "post", "postFooId"},
{"/foo/{id}", "delete", "deleteFooId"},
}
for _, test := range tests {
actual := getPathOp(doc.Paths[test.path], test.op).OperationID
expected := test.opID
if context != "" {
expected += "_" + context
}
if actual != expected {
t.Fatalf("expected %v, got %v", expected, actual)
}
}
}
}
func TestOpenAPI_CustomDecoder(t *testing.T) {
p := &Path{
Pattern: "foo",
@ -712,6 +652,216 @@ func TestOpenAPI_CleanResponse(t *testing.T) {
}
}
func TestOpenAPI_constructOperationID(t *testing.T) {
tests := map[string]struct {
path string
pathIndex int
pathAttributes *DisplayAttributes
operation logical.Operation
operationAttributes *DisplayAttributes
defaultPrefix string
expected string
}{
"empty": {
path: "",
pathIndex: 0,
pathAttributes: nil,
operation: logical.Operation(""),
operationAttributes: nil,
defaultPrefix: "",
expected: "",
},
"simple-read": {
path: "path/to/thing",
pathIndex: 0,
pathAttributes: nil,
operation: logical.ReadOperation,
operationAttributes: nil,
defaultPrefix: "test",
expected: "test-read-path-to-thing",
},
"simple-write": {
path: "path/to/thing",
pathIndex: 0,
pathAttributes: nil,
operation: logical.UpdateOperation,
operationAttributes: nil,
defaultPrefix: "test",
expected: "test-write-path-to-thing",
},
"operation-verb": {
path: "path/to/thing",
pathIndex: 0,
pathAttributes: &DisplayAttributes{OperationVerb: "do-something"},
operation: logical.UpdateOperation,
operationAttributes: nil,
defaultPrefix: "test",
expected: "do-something",
},
"operation-verb-override": {
path: "path/to/thing",
pathIndex: 0,
pathAttributes: &DisplayAttributes{OperationVerb: "do-something"},
operation: logical.UpdateOperation,
operationAttributes: &DisplayAttributes{OperationVerb: "do-something-else"},
defaultPrefix: "test",
expected: "do-something-else",
},
"operation-prefix": {
path: "path/to/thing",
pathIndex: 0,
pathAttributes: &DisplayAttributes{OperationPrefix: "my-prefix"},
operation: logical.UpdateOperation,
operationAttributes: nil,
defaultPrefix: "test",
expected: "my-prefix-write-path-to-thing",
},
"operation-prefix-override": {
path: "path/to/thing",
pathIndex: 0,
pathAttributes: &DisplayAttributes{OperationPrefix: "my-prefix"},
operation: logical.UpdateOperation,
operationAttributes: &DisplayAttributes{OperationPrefix: "better-prefix"},
defaultPrefix: "test",
expected: "better-prefix-write-path-to-thing",
},
"operation-prefix-and-suffix": {
path: "path/to/thing",
pathIndex: 0,
pathAttributes: &DisplayAttributes{OperationPrefix: "my-prefix", OperationSuffix: "my-suffix"},
operation: logical.UpdateOperation,
operationAttributes: nil,
defaultPrefix: "test",
expected: "my-prefix-write-my-suffix",
},
"operation-prefix-and-suffix-override": {
path: "path/to/thing",
pathIndex: 0,
pathAttributes: &DisplayAttributes{OperationPrefix: "my-prefix", OperationSuffix: "my-suffix"},
operation: logical.UpdateOperation,
operationAttributes: &DisplayAttributes{OperationPrefix: "better-prefix", OperationSuffix: "better-suffix"},
defaultPrefix: "test",
expected: "better-prefix-write-better-suffix",
},
"operation-prefix-verb-suffix": {
path: "path/to/thing",
pathIndex: 0,
pathAttributes: &DisplayAttributes{OperationPrefix: "my-prefix", OperationSuffix: "my-suffix", OperationVerb: "Create"},
operation: logical.UpdateOperation,
operationAttributes: &DisplayAttributes{OperationPrefix: "better-prefix", OperationSuffix: "better-suffix"},
defaultPrefix: "test",
expected: "better-prefix-create-better-suffix",
},
"operation-prefix-verb-suffix-override": {
path: "path/to/thing",
pathIndex: 0,
pathAttributes: &DisplayAttributes{OperationPrefix: "my-prefix", OperationSuffix: "my-suffix", OperationVerb: "Create"},
operation: logical.UpdateOperation,
operationAttributes: &DisplayAttributes{OperationPrefix: "better-prefix", OperationSuffix: "better-suffix", OperationVerb: "Login"},
defaultPrefix: "test",
expected: "better-prefix-login-better-suffix",
},
"operation-prefix-verb": {
path: "path/to/thing",
pathIndex: 0,
pathAttributes: nil,
operation: logical.UpdateOperation,
operationAttributes: &DisplayAttributes{OperationPrefix: "better-prefix", OperationVerb: "Login"},
defaultPrefix: "test",
expected: "better-prefix-login",
},
"operation-verb-suffix": {
path: "path/to/thing",
pathIndex: 0,
pathAttributes: nil,
operation: logical.UpdateOperation,
operationAttributes: &DisplayAttributes{OperationVerb: "Login", OperationSuffix: "better-suffix"},
defaultPrefix: "test",
expected: "login-better-suffix",
},
"pipe-delimited-suffix-0": {
path: "path/to/thing",
pathIndex: 0,
pathAttributes: nil,
operation: logical.UpdateOperation,
operationAttributes: &DisplayAttributes{OperationPrefix: "better-prefix", OperationSuffix: "suffix0|suffix1"},
defaultPrefix: "test",
expected: "better-prefix-write-suffix0",
},
"pipe-delimited-suffix-1": {
path: "path/to/thing",
pathIndex: 1,
pathAttributes: nil,
operation: logical.UpdateOperation,
operationAttributes: &DisplayAttributes{OperationPrefix: "better-prefix", OperationSuffix: "suffix0|suffix1"},
defaultPrefix: "test",
expected: "better-prefix-write-suffix1",
},
"pipe-delimited-suffix-2-fallback": {
path: "path/to/thing",
pathIndex: 2,
pathAttributes: nil,
operation: logical.UpdateOperation,
operationAttributes: &DisplayAttributes{OperationPrefix: "better-prefix", OperationSuffix: "suffix0|suffix1"},
defaultPrefix: "test",
expected: "better-prefix-write-path-to-thing",
},
}
for name, test := range tests {
name, test := name, test
t.Run(name, func(t *testing.T) {
t.Parallel()
actual := constructOperationID(
test.path,
test.pathIndex,
test.pathAttributes,
test.operation,
test.operationAttributes,
test.defaultPrefix,
)
if actual != test.expected {
t.Fatalf("expected: %s; got: %s", test.expected, actual)
}
})
}
}
func TestOpenAPI_hyphenatedToTitleCase(t *testing.T) {
tests := map[string]struct {
in string
expected string
}{
"simple": {
in: "test",
expected: "Test",
},
"two-words": {
in: "two-words",
expected: "TwoWords",
},
"three-words": {
in: "one-two-three",
expected: "OneTwoThree",
},
"not-hyphenated": {
in: "something_like_this",
expected: "Something_like_this",
},
}
for name, test := range tests {
name, test := name, test
t.Run(name, func(t *testing.T) {
t.Parallel()
actual := hyphenatedToTitleCase(test.in)
if actual != test.expected {
t.Fatalf("expected: %s; got: %s", test.expected, actual)
}
})
}
}
func testPath(t *testing.T, path *Path, sp *logical.Paths, expectedJSON string) {
t.Helper()

View File

@ -227,6 +227,24 @@ type DisplayAttributes struct {
// Action is the verb to use for the operation.
Action string `json:"action,omitempty"`
// OperationPrefix is a hyphenated lower-case string used to construct
// OpenAPI OperationID. It is typically the name of the plugin.
OperationPrefix string `json:"operationPrefix,omitempty"`
// OperationPrefix is a hyphenated lower-case string used to construct
// OpenAPI OperationID. It is typically an action to be performed
// (e.g. "generate", "sign", "login", etc.). If not specified, the verb
// defaults to `logical.Operation.String()` (e.g. "read", "delete", etc.).
OperationVerb string `json:"operationVerb,omitempty"`
// OperationPrefix is a hyphenated lower-case string used to construct
// OpenAPI OperationID. It is typically the name of the resource on which
// the action is performed (e.g. "role", "credentials", etc.). A pipe (|)
// separator can be used to list different suffixes for various permutations
// of the `Path.Pattern` regular expression. If not specified, the suffix
// defaults to the `Path.Pattern` split by dashes.
OperationSuffix string `json:"operationSuffix,omitempty"`
// EditType is the optional type of form field needed for a property
// This is only necessary for a "textarea" or "file"
EditType string `json:"editType,omitempty"`
@ -261,6 +279,7 @@ type PathOperation struct {
Deprecated bool
ForwardPerformanceSecondary bool
ForwardPerformanceStandby bool
DisplayAttrs *DisplayAttributes
}
func (p *PathOperation) Handler() OperationFunc {
@ -277,6 +296,7 @@ func (p *PathOperation) Properties() OperationProperties {
Deprecated: p.Deprecated,
ForwardPerformanceSecondary: p.ForwardPerformanceSecondary,
ForwardPerformanceStandby: p.ForwardPerformanceStandby,
DisplayAttrs: p.DisplayAttrs,
}
}

View File

@ -24,9 +24,11 @@
}
],
"get": {
"operationId": "getLookupId",
"operationId": "kv-read-lookup-id",
"summary": "Synopsis",
"tags": ["secrets"],
"tags": [
"secrets"
],
"responses": {
"200": {
"description": "OK"
@ -34,15 +36,17 @@
}
},
"post": {
"operationId": "postLookupId",
"operationId": "kv-write-lookup-id",
"summary": "Synopsis",
"tags": ["secrets"],
"tags": [
"secrets"
],
"requestBody": {
"required": true,
"content": {
"application/json": {
"schema": {
"$ref": "#/components/schemas/KvLookupRequest"
"$ref": "#/components/schemas/kv-write-lookup-id-request"
}
}
}
@ -57,7 +61,7 @@
},
"components": {
"schemas": {
"KvLookupRequest": {
"kv-write-lookup-id-request": {
"type": "object",
"properties": {
"token": {
@ -69,4 +73,3 @@
}
}
}

View File

@ -37,8 +37,10 @@
}
],
"get": {
"operationId": "getFooId",
"tags": ["secrets"],
"operationId": "kv-read-foo-id",
"tags": [
"secrets"
],
"summary": "My Summary",
"description": "My Description",
"responses": {
@ -58,8 +60,10 @@
]
},
"post": {
"operationId": "postFooId",
"tags": ["secrets"],
"operationId": "kv-write-foo-id",
"tags": [
"secrets"
],
"summary": "Update Summary",
"description": "Update Description",
"requestBody": {
@ -67,7 +71,7 @@
"content": {
"application/json": {
"schema": {
"$ref": "#/components/schemas/KvFooRequest"
"$ref": "#/components/schemas/kv-write-foo-id-request"
}
}
}
@ -82,9 +86,11 @@
},
"components": {
"schemas": {
"KvFooRequest": {
"kv-write-foo-id-request": {
"type": "object",
"required": ["age"],
"required": [
"age"
],
"properties": {
"flavors": {
"type": "array",
@ -96,7 +102,11 @@
"age": {
"type": "integer",
"description": "the age",
"enum": [1, 2, 3],
"enum": [
1,
2,
3
],
"x-vault-displayAttrs": {
"name": "Age",
"sensitive": true,
@ -113,9 +123,13 @@
"x-abc-token": {
"type": "string",
"description": "a header value",
"enum": ["a", "b", "c"]
"enum": [
"a",
"b",
"c"
]
},
"maximum" : {
"maximum": {
"type": "integer",
"description": "a maximum value",
"format": "int64"

View File

@ -36,8 +36,10 @@
}
],
"get": {
"operationId": "getFooId",
"tags": ["secrets"],
"operationId": "kv-list-foo-id",
"tags": [
"secrets"
],
"summary": "List Summary",
"description": "List Description",
"responses": {
@ -53,7 +55,9 @@
"in": "query",
"schema": {
"type": "string",
"enum": ["true"]
"enum": [
"true"
]
}
}
]
@ -61,7 +65,6 @@
}
},
"components": {
"schemas": {
}
"schemas": {}
}
}

View File

@ -14,8 +14,10 @@
"description": "Synopsis",
"x-vault-unauthenticated": true,
"delete": {
"operationId": "deleteFoo",
"tags": ["secrets"],
"operationId": "kv-delete-foo",
"tags": [
"secrets"
],
"summary": "Delete stuff",
"responses": {
"204": {
@ -24,8 +26,10 @@
}
},
"get": {
"operationId": "getFoo",
"tags": ["secrets"],
"operationId": "kv-read-foo",
"tags": [
"secrets"
],
"summary": "My Summary",
"description": "My Description",
"responses": {
@ -34,7 +38,7 @@
"content": {
"application/json": {
"schema": {
"$ref": "#/components/schemas/KvFooResponse"
"$ref": "#/components/schemas/kv-read-foo-response"
}
}
}
@ -45,7 +49,7 @@
},
"components": {
"schemas": {
"KvFooResponse": {
"kv-read-foo-response": {
"type": "object",
"properties": {
"field_a": {
@ -61,4 +65,3 @@
}
}
}