Skip to content

Commit

Permalink
Sort tags by explicit Swagger annotation first (#3587)
Browse files Browse the repository at this point in the history 
I believe it what most people would expect, since the order of
tags extracted from methods is harder to control, especially if
swagger file is merged from multiple proto files.
  • Loading branch information
@same-id
same-id authored Sep 11, 2023
1 parent 5074ccc commit 46a0044
Show file tree
Hide file tree
Showing 6 changed files with 275 additions and 28 deletions.
6 changes: 3 additions & 3 deletions examples/internal/clients/abe/api/swagger.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@ info:
url: "https://github.com/grpc-ecosystem/grpc-gateway/blob/main/LICENSE.txt"
x-something-something: "yadda"
tags:
- name: "echo rpc"
description: "Echo Rpc description"
x-traitTag: true
- name: "ABitOfEverythingService"
description: "ABitOfEverythingService description -- which should not be used in\
\ place of the documentation comment!"
Expand All @@ -21,9 +24,6 @@ tags:
- name: "camelCaseServiceName"
- name: "AnotherServiceWithNoBindings"
- name: "SnakeEnumService"
- name: "echo rpc"
description: "Echo Rpc description"
x-traitTag: true
schemes:
- "http"
- "https"
Expand Down
10 changes: 5 additions & 5 deletions examples/internal/clients/unannotatedecho/api/swagger.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,17 +16,17 @@ info:
url: "https://github.com/grpc-ecosystem/grpc-gateway/blob/main/LICENSE.txt"
x-something-something: "yadda"
tags:
- name: "Echo"
description: "Echo description"
- name: "Internal"
description: "Internal description"
x-traitTag: true
- name: "UnannotatedEchoService"
description: "UnannotatedEchoService description -- which should not be used in\
\ place of the documentation comment!"
externalDocs:
description: "Find out more about UnannotatedEchoService"
url: "https://github.com/grpc-ecosystem/grpc-gateway"
- name: "Echo"
description: "Echo description"
- name: "Internal"
description: "Internal description"
x-traitTag: true
schemes:
- "http"
- "https"
Expand Down
10 changes: 5 additions & 5 deletions examples/internal/proto/examplepb/a_bit_of_everything.swagger.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,11 @@
"x-something-something": "yadda"
},
"tags": [
{
"name": "echo rpc",
"description": "Echo Rpc description",
"x-traitTag": true
},
{
"name": "ABitOfEverythingService",
"description": "ABitOfEverythingService description -- which should not be used in place of the documentation comment!",
Expand All @@ -31,11 +36,6 @@
},
{
"name": "SnakeEnumService"
},
{
"name": "echo rpc",
"description": "Echo Rpc description",
"x-traitTag": true
}
],
"schemes": [
Expand Down
16 changes: 8 additions & 8 deletions examples/internal/proto/examplepb/unannotated_echo_service.swagger.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,6 @@
"x-something-something": "yadda"
},
"tags": [
{
"name": "UnannotatedEchoService",
"description": "UnannotatedEchoService description -- which should not be used in place of the documentation comment!",
"externalDocs": {
"description": "Find out more about UnannotatedEchoService",
"url": "https://github.com/grpc-ecosystem/grpc-gateway"
}
},
{
"name": "Echo",
"description": "Echo description"
Expand All @@ -32,6 +24,14 @@
"name": "Internal",
"description": "Internal description",
"x-traitTag": true
},
{
"name": "UnannotatedEchoService",
"description": "UnannotatedEchoService description -- which should not be used in place of the documentation comment!",
"externalDocs": {
"description": "Find out more about UnannotatedEchoService",
"url": "https://github.com/grpc-ecosystem/grpc-gateway"
}
}
],
"schemes": [
Expand Down
53 changes: 49 additions & 4 deletions protoc-gen-openapiv2/internal/genopenapi/template.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1746,10 +1746,6 @@ func applyTemplate(p param) (*openapiSwaggerObject, error) {
panic(err)
}

if !p.reg.GetDisableServiceTags() {
s.Tags = append(s.Tags, renderServiceTags(p.Services, p.reg)...)
}

messages := messageMap{}
streamingMessages := messageMap{}
enums := enumMap{}
Expand Down Expand Up @@ -2017,6 +2013,10 @@ func applyTemplate(p param) (*openapiSwaggerObject, error) {
// should be added here, once supported in the proto.
}

if !p.reg.GetDisableServiceTags() {
s.Tags = mergeTags(s.Tags, renderServiceTags(p.Services, p.reg))
}

// Finally add any references added by users that aren't
// otherwise rendered.
if err := addCustomRefs(s.Definitions, p.reg, customRefs); err != nil {
Expand All @@ -2026,6 +2026,51 @@ func applyTemplate(p param) (*openapiSwaggerObject, error) {
return &s, nil
}

func mergeTags(existingTags []openapiTagObject, tags []openapiTagObject) []openapiTagObject {
for _, tag := range tags {
matched := false
for i, existingTag := range existingTags {
if existingTag.Name == tag.Name {
if existingTag.Description == "" {
existingTags[i].Description = tag.Description
}
if existingTag.ExternalDocs == nil {
existingTags[i].ExternalDocs = tag.ExternalDocs
} else if tag.ExternalDocs != nil {
if existingTag.ExternalDocs.Description == "" {
existingTags[i].ExternalDocs.Description = tag.ExternalDocs.Description
}
if existingTag.ExternalDocs.URL == "" {
existingTags[i].ExternalDocs.URL = tag.ExternalDocs.URL
}
}
if existingTag.extensions == nil {
existingTags[i].extensions = tag.extensions
} else if tag.extensions != nil {
for _, ext := range tag.extensions {
matchedExt := false
for _, existingExt := range existingTag.extensions {
if existingExt.key == ext.key {
matchedExt = true
break
}
}
if !matchedExt {
existingTags[i].extensions = append(existingTags[i].extensions, ext)
}
}
}
matched = true
break
}
}
if !matched {
existingTags = append(existingTags, tag)
}
}
return existingTags
}

func processExtensions(inputExts map[string]*structpb.Value) ([]extension, error) {
exts := make([]extension, 0, len(inputExts))
for k, v := range inputExts {
Expand Down
208 changes: 205 additions & 3 deletions protoc-gen-openapiv2/internal/genopenapi/template_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2110,10 +2110,11 @@ func TestApplyTemplateExtensions(t *testing.T) {
t.Errorf("applyTemplate(%#v).%s = %s want to be %s", file, name, is, want)
}

var tag openapiTagObject
for _, v := range result.Tags {
tag = v
if len(result.Tags) == 0 {
t.Errorf("No tags found in result")
return
}
tag := result.Tags[0]
if want, is, name := []extension{
{key: "x-traitTag", value: json.RawMessage("true")},
}, tag.extensions, "Tags[0].Extensions"; !reflect.DeepEqual(is, want) {
Expand Down Expand Up @@ -10096,3 +10097,204 @@ func TestUpdatePaths(t *testing.T) {
})
}
}

func MustMarshal(v interface{}) []byte {
b, err := json.Marshal(v)
if err != nil {
panic(err)
}
return b
}

func TestMergeTags(t *testing.T) {
testCases := [...]struct {
testName string
existingTags []openapiTagObject
newTags []openapiTagObject
expectedMergedTags []openapiTagObject
}{
{
testName: "Simple merge.",
existingTags: []openapiTagObject{{
Name: "tag1",
Description: "tag1 description",
}},
newTags: []openapiTagObject{{
Name: "tag2",
Description: "tag2 description",
}},
expectedMergedTags: []openapiTagObject{{
Name: "tag1",
Description: "tag1 description",
}, {
Name: "tag2",
Description: "tag2 description",
}},
},
{
testName: "Merge description",
existingTags: []openapiTagObject{{
Name: "tag1",
Description: "tag1 description",
}, {
Name: "tag2",
}, {
Name: "tag3",
Description: "tag3 description",
}},
newTags: []openapiTagObject{{
Name: "tag2",
Description: "tag2 description",
}},
expectedMergedTags: []openapiTagObject{{
Name: "tag1",
Description: "tag1 description",
}, {
Name: "tag2",
Description: "tag2 description",
}, {
Name: "tag3",
Description: "tag3 description",
}},
},
{
testName: "Merge external docs",
existingTags: []openapiTagObject{{
Name: "tag1",
ExternalDocs: &openapiExternalDocumentationObject{},
}, {
Name: "tag2",
}, {
Name: "tag3",
ExternalDocs: &openapiExternalDocumentationObject{
Description: "tag3 description",
},
}, {
Name: "tag4",
ExternalDocs: &openapiExternalDocumentationObject{
URL: "tag4 url",
},
}},
newTags: []openapiTagObject{{
Name: "tag1",
ExternalDocs: &openapiExternalDocumentationObject{
Description: "tag1 description",
},
}, {
Name: "tag2",
ExternalDocs: &openapiExternalDocumentationObject{
Description: "tag2 description",
URL: "tag2 url",
},
}, {
Name: "tag3",
ExternalDocs: &openapiExternalDocumentationObject{
Description: "ignored tag3 description",
URL: "tag3 url",
},
}, {
Name: "tag4",
ExternalDocs: &openapiExternalDocumentationObject{
Description: "tag4 description",
},
}},
expectedMergedTags: []openapiTagObject{{
Name: "tag1",
ExternalDocs: &openapiExternalDocumentationObject{
Description: "tag1 description",
},
}, {
Name: "tag2",
ExternalDocs: &openapiExternalDocumentationObject{
Description: "tag2 description",
URL: "tag2 url",
},
}, {
Name: "tag3",
ExternalDocs: &openapiExternalDocumentationObject{
Description: "tag3 description",
URL: "tag3 url",
},
}, {
Name: "tag4",
ExternalDocs: &openapiExternalDocumentationObject{
Description: "tag4 description",
URL: "tag4 url",
},
}},
},
{
testName: "Merge extensions",
existingTags: []openapiTagObject{{
Name: "tag1",
extensions: []extension{{key: "x-key1", value: MustMarshal("key1 extension")}},
}, {
Name: "tag2",
extensions: []extension{
{key: "x-key1", value: MustMarshal("key1 extension")},
{key: "x-key2", value: MustMarshal("key2 extension")},
},
}, {
Name: "tag3",
extensions: []extension{
{key: "x-key1", value: MustMarshal("key1 extension")},
},
}, {
Name: "tag4",
extensions: nil,
}},
newTags: []openapiTagObject{{
Name: "tag1",
extensions: []extension{{key: "x-key2", value: MustMarshal("key2 extension")}},
}, {
Name: "tag2",
extensions: []extension{
{key: "x-key1", value: MustMarshal("key1 extension")},
{key: "x-key2", value: MustMarshal("ignored key2 extension")},
{key: "x-key3", value: MustMarshal("key3 extension")},
},
}, {
Name: "tag3",
extensions: nil,
}, {
Name: "tag4",
extensions: []extension{
{key: "x-key1", value: MustMarshal("key1 extension")},
},
}},
expectedMergedTags: []openapiTagObject{{
Name: "tag1",
extensions: []extension{
{key: "x-key1", value: MustMarshal("key1 extension")},
{key: "x-key2", value: MustMarshal("key2 extension")},
},
}, {
Name: "tag2",
extensions: []extension{
{key: "x-key1", value: MustMarshal("key1 extension")},
{key: "x-key2", value: MustMarshal("key2 extension")},
{key: "x-key3", value: MustMarshal("key3 extension")},
},
}, {
Name: "tag3",
extensions: []extension{
{key: "x-key1", value: MustMarshal("key1 extension")},
},
}, {
Name: "tag4",
extensions: []extension{
{key: "x-key1", value: MustMarshal("key1 extension")},
},
}},
},
}
for _, tc := range testCases {
tc := tc
t.Run(tc.testName, func(t *testing.T) {
mergedTags := mergeTags(tc.existingTags, tc.newTags)
if !reflect.DeepEqual(tc.expectedMergedTags, mergedTags) {
t.Fatalf("%s: Tags not correctly merged. Want %#v, got %#v", tc.testName, tc.expectedMergedTags, mergedTags)
}
})
}
}

0 comments on commit 46a0044

Please sign in to comment.