Skip to content

Commit b892246

Browse files
aethanolaethanol
authored
Fixes additional paths generated when many methods have the same resource path (#2496)
* Fixes additional paths generated when many methods have the same http paths. Fixes #2489 * udpate the pathItemObject from the new path when we have a duplicate path * add comment for new change Co-authored-by: Ethan Anderson <[email protected]>
1 parent 02cb644 commit b892246

File tree

2 files changed

+310
-0
lines changed

2 files changed

+310
-0
lines changed

‎protoc-gen-openapiv2/internal/genopenapi/template.go‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1113,6 +1113,8 @@ func renderServices(services []*descriptor.Service, paths openapiPathsObject, re
11131113
existingOperationObject = nil
11141114
}
11151115
}
1116+
// update the pathItemObject we are adding to with the new path
1117+
pathItemObject = paths[newPath]
11161118
firstPathParameter.Name = newPathElement
11171119
path = newPath
11181120
parameters[firstParamIndex] = *firstPathParameter

‎protoc-gen-openapiv2/internal/genopenapi/template_test.go‎

Lines changed: 308 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5130,6 +5130,314 @@ func TestTemplateWithInvalidDuplicateOperations(t *testing.T) {
51305130
t.Error(err)
51315131
}
51325132
}
5133+
func TestSingleServiceTemplateWithDuplicateHttp1Operations(t *testing.T) {
5134+
fieldType := descriptorpb.FieldDescriptorProto_TYPE_STRING
5135+
field1 := &descriptorpb.FieldDescriptorProto{
5136+
Name: proto.String("name"),
5137+
Number: proto.Int32(1),
5138+
Type: &fieldType,
5139+
}
5140+
5141+
getFooMsgDesc := &descriptorpb.DescriptorProto{
5142+
Name: proto.String("GetFooRequest"),
5143+
Field: []*descriptorpb.FieldDescriptorProto{
5144+
field1,
5145+
},
5146+
}
5147+
getFooMsg := &descriptor.Message{
5148+
DescriptorProto: getFooMsgDesc,
5149+
}
5150+
deleteFooMsgDesc := &descriptorpb.DescriptorProto{
5151+
Name: proto.String("DeleteFooRequest"),
5152+
Field: []*descriptorpb.FieldDescriptorProto{
5153+
field1,
5154+
},
5155+
}
5156+
deleteFooMsg := &descriptor.Message{
5157+
DescriptorProto: deleteFooMsgDesc,
5158+
}
5159+
getFoo := &descriptorpb.MethodDescriptorProto{
5160+
Name: proto.String("GetFoo"),
5161+
InputType: proto.String("GetFooRequest"),
5162+
OutputType: proto.String("EmptyMessage"),
5163+
}
5164+
deleteFoo := &descriptorpb.MethodDescriptorProto{
5165+
Name: proto.String("DeleteFoo"),
5166+
InputType: proto.String("DeleteFooRequest"),
5167+
OutputType: proto.String("EmptyMessage"),
5168+
}
5169+
5170+
getBarMsgDesc := &descriptorpb.DescriptorProto{
5171+
Name: proto.String("GetBarRequest"),
5172+
Field: []*descriptorpb.FieldDescriptorProto{
5173+
field1,
5174+
},
5175+
}
5176+
getBarMsg := &descriptor.Message{
5177+
DescriptorProto: getBarMsgDesc,
5178+
}
5179+
deleteBarMsgDesc := &descriptorpb.DescriptorProto{
5180+
Name: proto.String("DeleteBarRequest"),
5181+
Field: []*descriptorpb.FieldDescriptorProto{
5182+
field1,
5183+
},
5184+
}
5185+
deleteBarMsg := &descriptor.Message{
5186+
DescriptorProto: deleteBarMsgDesc,
5187+
}
5188+
getBar := &descriptorpb.MethodDescriptorProto{
5189+
Name: proto.String("GetBar"),
5190+
InputType: proto.String("GetBarRequest"),
5191+
OutputType: proto.String("EmptyMessage"),
5192+
}
5193+
deleteBar := &descriptorpb.MethodDescriptorProto{
5194+
Name: proto.String("DeleteBar"),
5195+
InputType: proto.String("DeleteBarRequest"),
5196+
OutputType: proto.String("EmptyMessage"),
5197+
}
5198+
5199+
svc1 := &descriptorpb.ServiceDescriptorProto{
5200+
Name: proto.String("Service1"),
5201+
Method: []*descriptorpb.MethodDescriptorProto{getFoo, deleteFoo, getBar, deleteBar},
5202+
}
5203+
5204+
emptyMsgDesc := &descriptorpb.DescriptorProto{
5205+
Name: proto.String("EmptyMessage"),
5206+
}
5207+
emptyMsg := &descriptor.Message{
5208+
DescriptorProto: emptyMsgDesc,
5209+
}
5210+
5211+
file := descriptor.File{
5212+
FileDescriptorProto: &descriptorpb.FileDescriptorProto{
5213+
SourceCodeInfo: &descriptorpb.SourceCodeInfo{},
5214+
Name: proto.String("service1.proto"),
5215+
Package: proto.String("example"),
5216+
MessageType: []*descriptorpb.DescriptorProto{getBarMsgDesc, deleteBarMsgDesc, getFooMsgDesc, deleteFooMsgDesc, emptyMsgDesc},
5217+
Service: []*descriptorpb.ServiceDescriptorProto{svc1},
5218+
Options: &descriptorpb.FileOptions{
5219+
GoPackage: proto.String("github.com/grpc-ecosystem/grpc-gateway/runtime/internal/examplepb;example"),
5220+
},
5221+
},
5222+
GoPkg: descriptor.GoPackage{
5223+
Path: "example.com/path/to/example/example.pb",
5224+
Name: "example_pb",
5225+
},
5226+
Messages: []*descriptor.Message{getFooMsg, deleteFooMsg, getBarMsg, deleteBarMsg, emptyMsg},
5227+
Services: []*descriptor.Service{
5228+
{
5229+
ServiceDescriptorProto: svc1,
5230+
Methods: []*descriptor.Method{
5231+
{
5232+
MethodDescriptorProto: getFoo,
5233+
RequestType: getFooMsg,
5234+
ResponseType: getFooMsg,
5235+
Bindings: []*descriptor.Binding{
5236+
{
5237+
HTTPMethod: "GET",
5238+
PathTmpl: httprule.Template{
5239+
Version: 1,
5240+
OpCodes: []int{0, 0},
5241+
Template: "/v1/{name=foos/*}",
5242+
},
5243+
PathParams: []descriptor.Parameter{
5244+
{
5245+
Target: &descriptor.Field{
5246+
FieldDescriptorProto: field1,
5247+
Message: getFooMsg,
5248+
},
5249+
FieldPath: descriptor.FieldPath{
5250+
{
5251+
Name: "name",
5252+
},
5253+
},
5254+
},
5255+
},
5256+
Body: &descriptor.Body{
5257+
FieldPath: descriptor.FieldPath([]descriptor.FieldPathComponent{}),
5258+
},
5259+
},
5260+
},
5261+
},
5262+
{
5263+
MethodDescriptorProto: deleteFoo,
5264+
RequestType: deleteFooMsg,
5265+
ResponseType: emptyMsg,
5266+
Bindings: []*descriptor.Binding{
5267+
{
5268+
HTTPMethod: "DELETE",
5269+
PathTmpl: httprule.Template{
5270+
Version: 1,
5271+
OpCodes: []int{0, 0},
5272+
Template: "/v1/{name=foos/*}",
5273+
},
5274+
PathParams: []descriptor.Parameter{
5275+
{
5276+
Target: &descriptor.Field{
5277+
FieldDescriptorProto: field1,
5278+
Message: deleteFooMsg,
5279+
},
5280+
FieldPath: descriptor.FieldPath{
5281+
{
5282+
Name: "name",
5283+
},
5284+
},
5285+
},
5286+
},
5287+
Body: &descriptor.Body{
5288+
FieldPath: descriptor.FieldPath([]descriptor.FieldPathComponent{}),
5289+
},
5290+
},
5291+
},
5292+
},
5293+
{
5294+
MethodDescriptorProto: getBar,
5295+
RequestType: getBarMsg,
5296+
ResponseType: getBarMsg,
5297+
Bindings: []*descriptor.Binding{
5298+
{
5299+
HTTPMethod: "GET",
5300+
PathTmpl: httprule.Template{
5301+
Version: 1,
5302+
OpCodes: []int{0, 0},
5303+
Template: "/v1/{name=bars/*}",
5304+
},
5305+
PathParams: []descriptor.Parameter{
5306+
{
5307+
Target: &descriptor.Field{
5308+
FieldDescriptorProto: field1,
5309+
Message: getBarMsg,
5310+
},
5311+
FieldPath: descriptor.FieldPath{
5312+
{
5313+
Name: "name",
5314+
},
5315+
},
5316+
},
5317+
},
5318+
Body: &descriptor.Body{
5319+
FieldPath: descriptor.FieldPath([]descriptor.FieldPathComponent{}),
5320+
},
5321+
},
5322+
},
5323+
},
5324+
{
5325+
MethodDescriptorProto: deleteBar,
5326+
RequestType: deleteBarMsg,
5327+
ResponseType: emptyMsg,
5328+
Bindings: []*descriptor.Binding{
5329+
{
5330+
HTTPMethod: "DELETE",
5331+
PathTmpl: httprule.Template{
5332+
Version: 1,
5333+
OpCodes: []int{0, 0},
5334+
Template: "/v1/{name=bars/*}",
5335+
},
5336+
PathParams: []descriptor.Parameter{
5337+
{
5338+
Target: &descriptor.Field{
5339+
FieldDescriptorProto: field1,
5340+
Message: deleteBarMsg,
5341+
},
5342+
FieldPath: descriptor.FieldPath{
5343+
{
5344+
Name: "name",
5345+
},
5346+
},
5347+
},
5348+
},
5349+
Body: &descriptor.Body{
5350+
FieldPath: descriptor.FieldPath([]descriptor.FieldPathComponent{}),
5351+
},
5352+
},
5353+
},
5354+
},
5355+
},
5356+
},
5357+
},
5358+
}
5359+
reg := descriptor.NewRegistry()
5360+
err := reg.Load(&pluginpb.CodeGeneratorRequest{ProtoFile: []*descriptorpb.FileDescriptorProto{file.FileDescriptorProto}})
5361+
if err != nil {
5362+
t.Fatalf("failed to reg.Load(): %v", err)
5363+
}
5364+
result, err := applyTemplate(param{File: crossLinkFixture(&file), reg: reg})
5365+
if err != nil {
5366+
t.Fatalf("applyTemplate(%#v) failed with %v; want success", file, err)
5367+
}
5368+
5369+
if got, want := len(result.Paths), 2; got != want {
5370+
t.Fatalf("Results path length differed, got %d want %d", got, want)
5371+
}
5372+
5373+
firstOpGet := result.Paths["/v1/{name}"].Get
5374+
if got, want := firstOpGet.OperationID, "Service1_GetFoo"; got != want {
5375+
t.Fatalf("First operation GET id differed, got %s want %s", got, want)
5376+
}
5377+
if got, want := len(firstOpGet.Parameters), 2; got != want {
5378+
t.Fatalf("First operation GET params length differed, got %d want %d", got, want)
5379+
}
5380+
if got, want := firstOpGet.Parameters[0].Name, "name"; got != want {
5381+
t.Fatalf("First operation GET first param name differed, got %s want %s", got, want)
5382+
}
5383+
if got, want := firstOpGet.Parameters[0].Pattern, "foos/[^/]+"; got != want {
5384+
t.Fatalf("First operation GET first param pattern differed, got %s want %s", got, want)
5385+
}
5386+
if got, want := firstOpGet.Parameters[1].In, "body"; got != want {
5387+
t.Fatalf("First operation GET second param 'in' differed, got %s want %s", got, want)
5388+
}
5389+
5390+
firstOpDelete := result.Paths["/v1/{name}"].Delete
5391+
if got, want := firstOpDelete.OperationID, "Service1_DeleteFoo"; got != want {
5392+
t.Fatalf("First operation id DELETE differed, got %s want %s", got, want)
5393+
}
5394+
if got, want := len(firstOpDelete.Parameters), 2; got != want {
5395+
t.Fatalf("First operation DELETE params length differed, got %d want %d", got, want)
5396+
}
5397+
if got, want := firstOpDelete.Parameters[0].Name, "name"; got != want {
5398+
t.Fatalf("First operation DELETE first param name differed, got %s want %s", got, want)
5399+
}
5400+
if got, want := firstOpDelete.Parameters[0].Pattern, "foos/[^/]+"; got != want {
5401+
t.Fatalf("First operation DELETE first param pattern differed, got %s want %s", got, want)
5402+
}
5403+
if got, want := firstOpDelete.Parameters[1].In, "body"; got != want {
5404+
t.Fatalf("First operation DELETE second param 'in' differed, got %s want %s", got, want)
5405+
}
5406+
5407+
secondOpGet := result.Paths["/v1/{name"+pathParamUniqueSuffixDeliminator+"1}"].Get
5408+
if got, want := secondOpGet.OperationID, "Service1_GetBar"; got != want {
5409+
t.Fatalf("Second operation id GET differed, got %s want %s", got, want)
5410+
}
5411+
if got, want := len(secondOpGet.Parameters), 2; got != want {
5412+
t.Fatalf("Second operation GET params length differed, got %d want %d", got, want)
5413+
}
5414+
if got, want := secondOpGet.Parameters[0].Name, "name"+pathParamUniqueSuffixDeliminator+"1"; got != want {
5415+
t.Fatalf("Second operation GET first param name differed, got %s want %s", got, want)
5416+
}
5417+
if got, want := secondOpGet.Parameters[0].Pattern, "bars/[^/]+"; got != want {
5418+
t.Fatalf("Second operation GET first param pattern differed, got %s want %s", got, want)
5419+
}
5420+
if got, want := secondOpGet.Parameters[1].In, "body"; got != want {
5421+
t.Fatalf("Second operation GET second param 'in' differed, got %s want %s", got, want)
5422+
}
5423+
5424+
secondOpDelete := result.Paths["/v1/{name"+pathParamUniqueSuffixDeliminator+"1}"].Delete
5425+
if got, want := secondOpDelete.OperationID, "Service1_DeleteBar"; got != want {
5426+
t.Fatalf("Second operation id differed, got %s want %s", got, want)
5427+
}
5428+
if got, want := len(secondOpDelete.Parameters), 2; got != want {
5429+
t.Fatalf("Second operation params length differed, got %d want %d", got, want)
5430+
}
5431+
if got, want := secondOpDelete.Parameters[0].Name, "name"+pathParamUniqueSuffixDeliminator+"1"; got != want {
5432+
t.Fatalf("Second operation first param name differed, got %s want %s", got, want)
5433+
}
5434+
if got, want := secondOpDelete.Parameters[0].Pattern, "bars/[^/]+"; got != want {
5435+
t.Fatalf("Second operation first param pattern differed, got %s want %s", got, want)
5436+
}
5437+
if got, want := secondOpDelete.Parameters[1].In, "body"; got != want {
5438+
t.Fatalf("Second operation third param 'in' differed, got %s want %s", got, want)
5439+
}
5440+
}
51335441

51345442
func TestTemplateWithDuplicateHttp1Operations(t *testing.T) {
51355443
fieldType := descriptorpb.FieldDescriptorProto_TYPE_STRING

0 commit comments

Comments
 (0)