Repository: goadesign/goa
Pull request #2330 (2019-10-21 07:44:20)

Fix grpc field tag inheritance and validation
- Correctly merge grpc endpoint and response validation errors (bug found in testing)
- Allow rpc:tag from reference field to be overridden (#2303)

raphael: It so happens that https://github.com/goadesign/goa/pull/2336 already introduced `Last` so just rename `GetFieldTag` to `FieldTag` and make it use `Last` instead of `GetLast`, thank you!
tmaflexera: Done - that was a nice coincidence!

Patch:
diff --git a/expr/attribute.go b/expr/attribute.go
index 62e98481..7217bd4a 100644
--- a/expr/attribute.go
+++ b/expr/attribute.go
@@ -389,6 +389,14 @@ func (a *AttributeExpr) HasTag(tag string) bool {
 	return false
 }
 
+// FieldTag returns the field tag if the attribute is a field.
+func (a *AttributeExpr) FieldTag() (tag string, found bool) {
+	if a == nil {
+		return
+	}
+	return a.Meta.Last("rpc:tag")
+}
+
 // HasDefaultValue returns true if the attribute with the given name has a
 // default value.
 func (a *AttributeExpr) HasDefaultValue(attName string) bool {
diff --git a/expr/grpc_endpoint.go b/expr/grpc_endpoint.go
index ed6f0b41..d2ff37c8 100644
--- a/expr/grpc_endpoint.go
+++ b/expr/grpc_endpoint.go
@@ -182,7 +182,7 @@ func (e *GRPCEndpointExpr) Validate() error {
 				msgFields = pobj
 			}
 			if len(*msgFields) > 0 {
-				validateRPCTags(msgFields, e)
+				verr.Merge(validateRPCTags(msgFields, e))
 			}
 		}
 	} else {
@@ -409,12 +409,12 @@ func validateRPCTags(fields *Object, e *GRPCEndpointExpr) *eval.ValidationErrors
 	verr := new(eval.ValidationErrors)
 	foundRPC := make(map[string]string)
 	for _, nat := range *fields {
-		if tag, ok := nat.Attribute.Meta["rpc:tag"]; !ok {
+		if tag, ok := nat.Attribute.FieldTag(); !ok {
 			verr.Add(e, "attribute %q does not have \"rpc:tag\" defined in the meta", nat.Name)
-		} else if a, ok := foundRPC[tag[0]]; ok {
-			verr.Add(e, "field number %s in attribute %q already exists for attribute %q", tag[0], nat.Name, a)
+		} else if a, ok := foundRPC[tag]; ok {
+			verr.Add(e, "field number %s in attribute %q already exists for attribute %q", tag, nat.Name, a)
 		} else {
-			foundRPC[tag[0]] = nat.Name
+			foundRPC[tag] = nat.Name
 		}
 	}
 	return verr
diff --git a/expr/grpc_response.go b/expr/grpc_response.go
index d740bd98..20bbaaaf 100644
--- a/expr/grpc_response.go
+++ b/expr/grpc_response.go
@@ -113,7 +113,7 @@ func (r *GRPCResponseExpr) Validate(e *GRPCEndpointExpr) *eval.ValidationErrors
 		case !hasMessage && !hasHeaders && !hasTrailers:
 			// no response message or metadata is defined. Ensure that the method
 			// result attributes have "rpc:tag" set
-			validateRPCTags(robj, e)
+			verr.Merge(validateRPCTags(robj, e))
 		}
 	} else {
 		switch {
diff --git a/grpc/codegen/protobuf.go b/grpc/codegen/protobuf.go
index f0c8ad65..93d989ac 100644
--- a/grpc/codegen/protobuf.go
+++ b/grpc/codegen/protobuf.go
@@ -386,8 +386,8 @@ func protoBufNativeGoTypeName(t expr.DataType) string {
 // rpcTag returns the unique numbered RPC tag from the given attribute.
 func rpcTag(a *expr.AttributeExpr) uint64 {
 	var tag uint64
-	if t, ok := a.Meta["rpc:tag"]; ok {
-		tn, err := strconv.ParseUint(t[0], 10, 64)
+	if t, ok := a.FieldTag(); ok {
+		tn, err := strconv.ParseUint(t, 10, 64)
 		if err != nil {
 			panic(err) // bug (should catch invalid field numbers in validation)
 		}


Test patch:
diff --git a/codegen/service/testdata/service_dsls.go b/codegen/service/testdata/service_dsls.go
index 68749cae..70af7e6f 100644
--- a/codegen/service/testdata/service_dsls.go
+++ b/codegen/service/testdata/service_dsls.go
@@ -529,12 +529,12 @@ var NamesWithSpacesDSL = func() {
 		})
 	})
 	var APayload = Type("Payload With Space", func() {
-		Attribute("String", String)
+		Field(1, "String", String)
 	})
 	var AResult = ResultType("application/vnd.goa.result", func() {
 		TypeName("Result With Space")
 		Attributes(func() {
-			Attribute("Int", Int)
+			Field(1, "Int", Int)
 		})
 	})
 	Service("Service With Spaces", func() {
diff --git a/expr/grpc_endpoint_test.go b/expr/grpc_endpoint_test.go
index 81159e84..b4a258cf 100644
--- a/expr/grpc_endpoint_test.go
+++ b/expr/grpc_endpoint_test.go
@@ -23,6 +23,22 @@ service "Service" gRPC endpoint "Method": Error "invalid_error_type" type is Any
 service "Service" gRPC endpoint "Method": Map element type is Any type which is not supported in gRPC`,
 			},
 		},
+		"endpoint-with-untagged-fields": {
+			DSL: testdata.GRPCEndpointWithUntaggedFields,
+			Errors: []string{`service "Service" gRPC endpoint "Method": attribute "req_not_field" does not have "rpc:tag" defined in the meta
+service "Service" gRPC endpoint "Method": attribute "resp_not_field" does not have "rpc:tag" defined in the meta`,
+			},
+		},
+		"endpoint-with-repeated-field-tags": {
+			DSL: testdata.GRPCEndpointWithRepeatedFieldTags,
+			Errors: []string{`service "Service" gRPC endpoint "Method": field number 1 in attribute "key_dup_id" already exists for attribute "key"
+service "Service" gRPC endpoint "Method": field number 2 in attribute "key_dup_id" already exists for attribute "key"`,
+			},
+		},
+		"endpoint-with-reference-types-field-inheritance": {
+			DSL:    testdata.GRPCEndpointWithReferenceTypes,
+			Errors: []string{},
+		},
 	}
 	for name, c := range cases {
 		t.Run(name, func(t *testing.T) {
diff --git a/expr/testdata/endpoint_dsls.go b/expr/testdata/endpoint_dsls.go
index 9765fcae..e672f89f 100644
--- a/expr/testdata/endpoint_dsls.go
+++ b/expr/testdata/endpoint_dsls.go
@@ -267,3 +267,56 @@ var GRPCEndpointWithAnyType = func() {
 		})
 	})
 }
+
+var GRPCEndpointWithUntaggedFields = func() {
+	var Req = Type("Req", func() {
+		Attribute("req_not_field", String)
+	})
+	var Resp = Type("Resp", func() {
+		Attribute("resp_not_field", String)
+	})
+	Service("Service", func() {
+		Method("Method", func() {
+			Payload(Req)
+			Result(Resp)
+			GRPC(func() {})
+		})
+	})
+}
+
+var GRPCEndpointWithRepeatedFieldTags = func() {
+	var Req = Type("Req", func() {
+		Field(1, "key", String)
+		Field(1, "key_dup_id", String)
+	})
+	var Resp = Type("Resp", func() {
+		Field(2, "key", String)
+		Field(2, "key_dup_id", String)
+	})
+	Service("Service", func() {
+		Method("Method", func() {
+			Payload(Req)
+			Result(Resp)
+			GRPC(func() {})
+		})
+	})
+}
+
+var GRPCEndpointWithReferenceTypes = func() {
+	var EntityReference = Type("EntityReference", func() {
+		Field(1, "name", String)
+	})
+
+	var Entity = Type("Entity", func() {
+		Reference(EntityReference)
+		Field(1, "id", String)
+		Field(2, "name")
+	})
+
+	Service("Service", func() {
+		Method("Method", func() {
+			Payload(Entity)
+			GRPC(func() {})
+		})
+	})
+}
diff --git a/grpc/codegen/testdata/dsls.go b/grpc/codegen/testdata/dsls.go
index 88b63891..ff0bf992 100644
--- a/grpc/codegen/testdata/dsls.go
+++ b/grpc/codegen/testdata/dsls.go
@@ -115,7 +115,7 @@ var ServerStreamingRPCDSL = func() {
 
 var ServerStreamingUserTypeDSL = func() {
 	var UT = Type("UserType", func() {
-		Attribute("IntField", Int)
+		Field(1, "IntField", Int)
 	})
 	Service("ServiceServerStreamingUserTypeRPC", func() {
 		Method("MethodServerStreamingUserTypeRPC", func() {
@@ -136,7 +136,7 @@ var ServerStreamingArrayDSL = func() {
 
 var ServerStreamingMapDSL = func() {
 	var UT = Type("UserType", func() {
-		Attribute("IntField", Int)
+		Field(1, "IntField", Int)
 	})
 	Service("ServiceServerStreamingMap", func() {
 		Method("MethodServerStreamingMap", func() {
@@ -150,8 +150,8 @@ var ServerStreamingResultWithViewsDSL = func() {
 	var RT = ResultType("application/vnd.result", func() {
 		TypeName("ResultType")
 		Attributes(func() {
-			Attribute("IntField", Int)
-			Attribute("DoubleField", Float64)
+			Field(1, "IntField", Int)
+			Field(2, "DoubleField", Float64)
 		})
 		View("default", func() {
 			Attribute("IntField")