From b032fe48dfafa93164d4aa40bfd270a2b708f59c Mon Sep 17 00:00:00 2001 From: Ryan Doyle Date: Wed, 3 Jun 2020 17:15:54 -0700 Subject: [PATCH 1/2] protoc-gen-swagger: Fix message recursion detection - Fixes a bug introduced during 9e25b2b which introduced recursion detection within messages. Briefly, the nature of the bug was that references to messages not in the current recursion traversal were remaining in the map used to track those occurrences. - Includes a test case outlining the edge case that prompted this fix. --- go.sum | 1 + .../internal/genopenapi/template.go | 20 +++- .../internal/genopenapi/template_test.go | 113 ++++++++++++++++++ 3 files changed, 128 insertions(+), 6 deletions(-) diff --git a/go.sum b/go.sum index 96c5ca33e99..80934605ae7 100644 --- a/go.sum +++ b/go.sum @@ -70,6 +70,7 @@ golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= golang.org/x/tools v0.0.0-20190114222345-bf090417da8b/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ= golang.org/x/tools v0.0.0-20190226205152-f727befe758c/go.mod h1:9Yl7xja0Znq3iFh3HoIrodX9oNMXvdceNzlUR8zjMvY= golang.org/x/tools v0.0.0-20190311212946-11955173bddd/go.mod h1:LCzVGOaR6xXOjkQ3onu1FJEFr0SW1gC7cKk1uF8kGRs= +golang.org/x/tools v0.0.0-20190524140312-2c0ae7006135 h1:5Beo0mZN8dRzgrMMkDp0jc8YXQKx9DiJ2k1dkvGsn5A= golang.org/x/tools v0.0.0-20190524140312-2c0ae7006135/go.mod h1:RgjU9mgBXZiqYHBnxXauZ1Gv1EHHAz9KjViQ78xBX0Q= golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543 h1:E7g+9GITq07hpfrRu66IVDexMakfv52eLZ2CXBWiKr4= golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= diff --git a/protoc-gen-openapiv2/internal/genopenapi/template.go b/protoc-gen-openapiv2/internal/genopenapi/template.go index 3b97fab6d2c..fcb302b47ac 100644 --- a/protoc-gen-openapiv2/internal/genopenapi/template.go +++ b/protoc-gen-openapiv2/internal/genopenapi/template.go @@ -131,7 +131,7 @@ func queryParams(message *descriptor.Message, field *descriptor.Field, prefix st // touched map[string]bool // If a cycle is discovered, an error is returned, as cyclical data structures aren't allowed // in query parameters. -func nestedQueryParams(message *descriptor.Message, field *descriptor.Field, prefix string, reg *descriptor.Registry, pathParams []descriptor.Parameter, touched map[string]bool) (params []openapiParameterObject, err error) { +func nestedQueryParams(message *descriptor.Message, field *descriptor.Field, prefix string, reg *descriptor.Registry, pathParams []descriptor.Parameter, touchedIn map[string]bool) (params []openapiParameterObject, err error) { // make sure the parameter is not already listed as a path parameter for _, pathParam := range pathParams { if pathParam.Target == field { @@ -226,13 +226,21 @@ func nestedQueryParams(message *descriptor.Message, field *descriptor.Field, pre if err != nil { return nil, fmt.Errorf("unknown message type %s", fieldType) } + // Check for cyclical message reference: - isCycle := touched[*msg.Name] + isCycle := touchedIn[*msg.Name] if isCycle { - return nil, fmt.Errorf("recursive types are not allowed for query parameters, cycle found on %q", fieldType) + return nil, fmt.Errorf("Recursive types are not allowed for query parameters, cycle found on %q", fieldType) + } + + // Construct a new map with the message name so a cycle further down the recursive path can be detected. + // Do not keep anything in the original touched reference and do not pass that reference along. This will + // prevent clobbering adjacent records while recursing. + touchedOut := make(map[string]bool) + for k, v := range touchedIn { + touchedOut[k] = v } - // Update map with the massage name so a cycle further down the recursive path can be detected. - touched[*msg.Name] = true + touchedOut[*msg.Name] = true for _, nestedField := range msg.Fields { var fieldName string @@ -241,7 +249,7 @@ func nestedQueryParams(message *descriptor.Message, field *descriptor.Field, pre } else { fieldName = field.GetName() } - p, err := nestedQueryParams(msg, nestedField, prefix+fieldName+".", reg, pathParams, touched) + p, err := nestedQueryParams(msg, nestedField, prefix+fieldName+".", reg, pathParams, touchedOut) if err != nil { return nil, err } diff --git a/protoc-gen-openapiv2/internal/genopenapi/template_test.go b/protoc-gen-openapiv2/internal/genopenapi/template_test.go index 526d1c18581..df9e5d3dd4e 100644 --- a/protoc-gen-openapiv2/internal/genopenapi/template_test.go +++ b/protoc-gen-openapiv2/internal/genopenapi/template_test.go @@ -411,6 +411,119 @@ func TestMessageToQueryParameters(t *testing.T) { } } +// TestMessagetoQueryParametersNoRecursive, is a check that cyclical references between messages +// are not falsely detected given previous known edge-cases. +func TestMessageToQueryParametersNoRecursive(t *testing.T) { + type test struct { + MsgDescs []*descriptorpb.DescriptorProto + Message string + } + + tests := []test{ + // First test: + // Here is a message that has two of another message adjacent to one another in a nested message. + // There is no loop but this was previouly falsely flagged as a cycle. + // Example proto: + // message NonRecursiveMessage { + // string field = 1; + // } + // message BaseMessage { + // NonRecursiveMessage first = 1; + // NonRecursiveMessage second = 2; + // } + // message QueryMessage { + // BaseMessage first = 1; + // string second = 2; + // } + { + MsgDescs: []*descriptorpb.DescriptorProto{ + &descriptorpb.DescriptorProto{ + Name: proto.String("QueryMessage"), + Field: []*descriptorpb.FieldDescriptorProto{ + { + Name: proto.String("first"), + Type: descriptorpb.FieldDescriptorProto_TYPE_MESSAGE.Enum(), + TypeName: proto.String(".example.BaseMessage"), + Number: proto.Int32(1), + }, + { + Name: proto.String("second"), + Type: descriptorpb.FieldDescriptorProto_TYPE_STRING.Enum(), + Number: proto.Int32(2), + }, + }, + }, + &descriptorpb.DescriptorProto{ + Name: proto.String("BaseMessage"), + Field: []*descriptorpb.FieldDescriptorProto{ + { + Name: proto.String("first"), + Type: descriptorpb.FieldDescriptorProto_TYPE_MESSAGE.Enum(), + TypeName: proto.String(".example.NonRecursiveMessage"), + Number: proto.Int32(1), + }, + { + Name: proto.String("second"), + Type: descriptorpb.FieldDescriptorProto_TYPE_MESSAGE.Enum(), + TypeName: proto.String(".example.NonRecursiveMessage"), + Number: proto.Int32(2), + }, + }, + }, + // Note there is no recursive nature to this message + &descriptorpb.DescriptorProto{ + Name: proto.String("NonRecursiveMessage"), + Field: []*descriptorpb.FieldDescriptorProto{ + { + Name: proto.String("field"), + //Label: descriptorpb.FieldDescriptorProto_LABEL_OPTIONAL.Enum(), + Type: descriptorpb.FieldDescriptorProto_TYPE_STRING.Enum(), + Number: proto.Int32(1), + }, + }, + }, + }, + Message: "QueryMessage", + }, + } + + for _, test := range tests { + reg := descriptor.NewRegistry() + msgs := []*descriptor.Message{} + for _, msgdesc := range test.MsgDescs { + msgs = append(msgs, &descriptor.Message{DescriptorProto: msgdesc}) + } + file := descriptor.File{ + FileDescriptorProto: &descriptorpb.FileDescriptorProto{ + SourceCodeInfo: &descriptorpb.SourceCodeInfo{}, + Name: proto.String("example.proto"), + Package: proto.String("example"), + Dependency: []string{}, + MessageType: test.MsgDescs, + Service: []*descriptorpb.ServiceDescriptorProto{}, + }, + GoPkg: descriptor.GoPackage{ + Path: "example.com/path/to/example/example.pb", + Name: "example_pb", + }, + Messages: msgs, + } + reg.Load(&pluginpb.CodeGeneratorRequest{ + ProtoFile: []*descriptorpb.FileDescriptorProto{file.FileDescriptorProto}, + }) + + message, err := reg.LookupMsg("", ".example."+test.Message) + if err != nil { + t.Fatalf("failed to lookup message: %s", err) + } + + _, err = messageToQueryParameters(message, reg, []descriptor.Parameter{}) + if err != nil { + t.Fatalf("No recursion error should be thrown: %s", err) + } + } +} + // TestMessagetoQueryParametersRecursive, is a check that cyclical references between messages // are handled gracefully. The goal is to insure that attempts to add messages with cyclical // references to query-parameters returns an error message. From 233ad9e0b1116b8c11645fbe773f144ff5e673ea Mon Sep 17 00:00:00 2001 From: Ryan Doyle Date: Thu, 4 Jun 2020 10:30:54 -0700 Subject: [PATCH 2/2] protoc-gen-swagger: Satisfy linter on recursion error message --- protoc-gen-openapiv2/internal/genopenapi/template.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/protoc-gen-openapiv2/internal/genopenapi/template.go b/protoc-gen-openapiv2/internal/genopenapi/template.go index fcb302b47ac..6d809976d25 100644 --- a/protoc-gen-openapiv2/internal/genopenapi/template.go +++ b/protoc-gen-openapiv2/internal/genopenapi/template.go @@ -230,7 +230,7 @@ func nestedQueryParams(message *descriptor.Message, field *descriptor.Field, pre // Check for cyclical message reference: isCycle := touchedIn[*msg.Name] if isCycle { - return nil, fmt.Errorf("Recursive types are not allowed for query parameters, cycle found on %q", fieldType) + return nil, fmt.Errorf("recursive types are not allowed for query parameters, cycle found on %q", fieldType) } // Construct a new map with the message name so a cycle further down the recursive path can be detected.