Repository: goadesign/goa
Pull request #3119 (2022-08-08 14:28:07)

Omit setting grpc fields to zero defaults
The expected behavior for zero-valued defaults bothered me; it needlessly compared the proto message fields against zeros only to set the goa fields to zeros. This PR explicitly tests a variety of built-in types and avoids the compare and set.

It's possible the isNonZero would be better written with reflect, to reduce its code. Or moved to, say, expr.AttributeType, so that we could instead write something like this in protobuf_transform:

```diff
- if tdef := tgtc.DefaultValue; tdef != nil && ta.TargetCtx.UseDefault && isNonZero(tdef) {
+ if tgtc.HasNonzeroDefault() && ta.TargetCtx.UseDefault {
+ 	tdef := tgtc.DefaultValue
```

MichaelUrman: I was slightly surprised to learn the following code issues an error. Should I file a bug report, and/or attempt to address it?

```go
    		Field(14, "typed", Int64, func() {
			Meta("struct:field:type", "time.Duration", "time")
			Default(0 * time.Second)
		})
```

> testing.go:18: [testdata/dsls.go:932] default value 0 is incompatible with attribute of type int64 in attribute

Namely it appears that a default of `time.Duration(0)` is not accepted, despite the field type being overridden to time.Duration.
MichaelUrman: While writing this I had several misgivings over the new-ish defaults behavior in gRPC at all. On the one hand, it means that setting a nonzero default makes it impossible to send a zero value over gRPC. On the other hand, without it a nonzero default silently does something you may not expect. I don't have a good resolution to offer here, but suspect we should err towards allowing it, and just clarify the documentation for dsl.Default(). If there's enough context to prove an unexpected interaction, perhaps issue a warning, or create a linter for this.

I'm assuming any of that would be better off in a separate issue or PR. Let me know if you think it should be here.
raphael: The PR looks good - thank you. Could you please rebase on the latest v3?

As you probably know the challenge with gRPC is that the encoding doesn't support the notion of "missing" vs "has default value" since primitive values used in a gRPC message cannot be nil. We had a proposal to tackle this explicitly: https://github.com/goadesign/goa/issues/2221 - however this never got implemented. The proposal allows designers to define the desired behavior (i.e., allow or prevent a primitive field to have a zero value).
MichaelUrman: Rebased.

Yes, the missing means zero, zero means default, etc. as covered on 2221 is why I was surprised to see missing mapped to defaults in v3. I must have missed the fanfare when it was introduced. :)

Any thoughts on the inability to specify a default value of the type specified by Meta("struct:field:type")? Since the error seems to come from Default(), which presumably can't see a later Meta call, I expect at best a non-trivial fix.
raphael: Wrt specifying default values for types equipped with `Meta("struct:field:type")` - Maybe there's a way to improve `IsCompatible` to look for that metadata and use reflection to figure out that the name of the Go literal value matches the name provided to `Meta`. See for example https://github.com/goadesign/goa/blob/951acc00173014741291e9522fb8ce524529903b/expr/types.go#L324 (every type implements this method).
MichaelUrman: I don't think IsCompatible has enough information. Maybe you can use `eval.Current().(*expr.AttributeExpr)` like its caller `dsls.Default`, but either location only works if `Meta("struct:field:type", ...)` precedes `Default(...)`. To become order-independent, we have to defer verification, say until after `dsl.Attribute`'s call to `eval.Execute`. This alters error reporting, at least for imported types. (I chose to still validate defaults of builtin types in the original location.)

```diff
- testing.go:18: [testdata/dsls.go:935] default value "" is incompatible with attribute of type int64 in attribute
+ testing.go:18: [../../dsl/attribute.go:236] default value _ is incompatible with attribute of type int64 in attribute
```

The attempt did find one [existing error worth fixing](https://github.com/goadesign/goa/commit/f4b5a44e79031d8092be3d19c75aea6482b19409#diff-8cf71849629683f5678c540aef44d9c718315219dffcc02f6947b8f7e378d4e8R199). I'm not satisfied with this approach yet so have kept it off the PR for now: https://github.com/goadesign/goa/commit/f4b5a44e79031d8092be3d19c75aea6482b19409

raphael: Yeah the type compatibility check for default values would have to be moved to the validation step of the DSL execution, probably here: https://github.com/goadesign/goa/blob/v3/expr/attribute.go#L189 (and it probably should have been there from the get go). Thank you again for this PR, merged!

Patch:
diff --git a/grpc/codegen/protobuf_transform.go b/grpc/codegen/protobuf_transform.go
index 87b184b8..9ee61c96 100644
--- a/grpc/codegen/protobuf_transform.go
+++ b/grpc/codegen/protobuf_transform.go
@@ -323,7 +323,7 @@ func transformObject(source, target *expr.AttributeExpr, sourceVar, targetVar st
 		// Default value handling. We need to handle default values if the target
 		// type uses default values (i.e. attributes with default values are
 		// non-pointers) and has a default value set.
-		if tdef := tgtc.DefaultValue; tdef != nil && ta.TargetCtx.UseDefault {
+		if tdef := tgtc.DefaultValue; tdef != nil && ta.TargetCtx.UseDefault && isNonZero(tdef) {
 			if ta.proto {
 				// We set default values in protocol buffer type only if the source type
 				// uses pointers to hold default values.
@@ -369,6 +369,20 @@ func transformObject(source, target *expr.AttributeExpr, sourceVar, targetVar st
 	return buffer.String(), nil
 }
 
+func isNonZero(v any) bool {
+	switch v := v.(type) {
+	case nil:
+		return false
+	case int, uint, int8, uint8, int16, uint16, int32, uint32, int64, uint64:
+		return v != 0
+	case float32, float64:
+		return v != 0.0
+	case string:
+		return v != ""
+	}
+	return true
+}
+
 // transformArray returns the code to transform source attribute of array
 // type to target attribute of array type. It returns an error if source
 // and target are not compatible for transformation.
@@ -963,7 +977,7 @@ case {{ . }}:
 	transformGoUnionFromProtoTmpl = `switch val := {{ .SourceVar }}.(type) {
 {{- range $i, $ref := .SourceValueTypeRefs }}
 case {{ . }}:
-	{{- $field := (print "val." (index $.SourceFieldNames $i)) }} 
+	{{- $field := (print "val." (index $.SourceFieldNames $i)) }}
 	{{ $.TargetVar }} = {{ convertType (index $.SourceValues $i).Attribute (index $.TargetValues $i).Attribute $field $.TransformAttrs }}
 {{- end }}
 }`


Test patch:
diff --git a/grpc/codegen/client_types_test.go b/grpc/codegen/client_types_test.go
index e5e712d3..2d499233 100644
--- a/grpc/codegen/client_types_test.go
+++ b/grpc/codegen/client_types_test.go
@@ -23,6 +23,7 @@ func TestClientTypeFiles(t *testing.T) {
 		{"with-errors", testdata.UnaryRPCWithErrorsDSL, testdata.WithErrorsClientTypeCode},
 		{"bidirectional-streaming-same-type", testdata.BidirectionalStreamingRPCSameTypeDSL, testdata.BidirectionalStreamingRPCSameTypeClientTypeCode},
 		{"struct-meta-type", testdata.StructMetaTypeDSL, testdata.StructMetaTypeTypeCode},
+		{"default-fields", testdata.DefaultFieldsDSL, testdata.DefaultFieldsTypeCode},
 	}
 	for _, c := range cases {
 		t.Run(c.Name, func(t *testing.T) {
diff --git a/grpc/codegen/proto_test.go b/grpc/codegen/proto_test.go
index a1a76d49..5d3dfa16 100644
--- a/grpc/codegen/proto_test.go
+++ b/grpc/codegen/proto_test.go
@@ -26,6 +26,7 @@ func TestProtoFiles(t *testing.T) {
 		{"method-with-acronym", testdata.MethodWithAcronymDSL, testdata.MethodWithAcronymProtoCode},
 		{"custom-package-name", testdata.ServiceWithPackageDSL, testdata.ServiceWithPackageCode},
 		{"struct-meta-type", testdata.StructMetaTypeDSL, testdata.StructMetaTypePackageCode},
+		{"default-fields", testdata.DefaultFieldsDSL, testdata.DefaultFieldsPackageCode},
 	}
 	for _, c := range cases {
 		t.Run(c.Name, func(t *testing.T) {
diff --git a/grpc/codegen/server_types_test.go b/grpc/codegen/server_types_test.go
index 891f9f3d..e97aae3e 100644
--- a/grpc/codegen/server_types_test.go
+++ b/grpc/codegen/server_types_test.go
@@ -24,6 +24,7 @@ func TestServerTypeFiles(t *testing.T) {
 		{"elem-validation", testdata.ElemValidationDSL, testdata.ElemValidationServerTypesFile},
 		{"alias-validation", testdata.AliasValidationDSL, testdata.AliasValidationServerTypesFile},
 		{"struct-meta-type", testdata.StructMetaTypeDSL, testdata.StructMetaTypeServerTypeCode},
+		{"default-fields", testdata.DefaultFieldsDSL, testdata.DefaultFieldsServerTypeCode},
 	}
 	for _, c := range cases {
 		t.Run(c.Name, func(t *testing.T) {
diff --git a/grpc/codegen/testdata/client_type_code.go b/grpc/codegen/testdata/client_type_code.go
index c91cd409..5dfd15ec 100644
--- a/grpc/codegen/testdata/client_type_code.go
+++ b/grpc/codegen/testdata/client_type_code.go
@@ -357,11 +357,11 @@ func NewMethodResult(message *using_meta_typespb.MethodResponse) *usingmetatypes
 	}
 	var zeroMessageA int64
 	if message.A == zeroMessageA {
-		result.A = 0
+		result.A = 1
 	}
 	var zeroMessageB int64
 	if message.B == zeroMessageB {
-		result.B = 0
+		result.B = 2
 	}
 	if message.C != nil {
 		result.C = make([]time.Duration, len(message.C))
@@ -372,3 +372,31 @@ func NewMethodResult(message *using_meta_typespb.MethodResponse) *usingmetatypes
 	return result
 }
 `
+
+const DefaultFieldsTypeCode = `// NewProtoMethodRequest builds the gRPC request type from the payload of the
+// "Method" endpoint of the "DefaultFields" service.
+func NewProtoMethodRequest(payload *defaultfields.MethodPayload) *default_fieldspb.MethodRequest {
+	message := &default_fieldspb.MethodRequest{
+		Req:  payload.Req,
+		Def0: payload.Def0,
+		Def1: payload.Def1,
+		Def2: payload.Def2,
+		Reqs: payload.Reqs,
+		Defs: payload.Defs,
+		Defe: payload.Defe,
+		Rat:  payload.Rat,
+		Flt0: payload.Flt0,
+		Flt1: payload.Flt1,
+	}
+	if payload.Opt != nil {
+		message.Opt = *payload.Opt
+	}
+	if payload.Opts != nil {
+		message.Opts = *payload.Opts
+	}
+	if payload.Flt != nil {
+		message.Flt = *payload.Flt
+	}
+	return message
+}
+`
diff --git a/grpc/codegen/testdata/dsls.go b/grpc/codegen/testdata/dsls.go
index b98c975e..e6673aa3 100644
--- a/grpc/codegen/testdata/dsls.go
+++ b/grpc/codegen/testdata/dsls.go
@@ -870,11 +870,11 @@ var StructMetaTypeDSL = func() {
 			Payload(func() {
 				Field(1, "a", Int64, func() {
 					Meta("struct:field:type", "flag.ErrorHandling", "flag")
-					Default(0)
+					Default(1)
 				})
 				Field(2, "b", Int64, func() {
 					Meta("struct:field:type", "flag.ErrorHandling", "flag")
-					Default(0)
+					Default(2)
 				})
 				Field(3, "c", ArrayOf(Int64), func() {
 					Elem(func() {
@@ -888,11 +888,11 @@ var StructMetaTypeDSL = func() {
 			Result(func() {
 				Field(1, "a", Int64, func() {
 					Meta("struct:field:type", "flag.ErrorHandling", "flag")
-					Default(0)
+					Default(1)
 				})
 				Field(2, "b", Int64, func() {
 					Meta("struct:field:type", "flag.ErrorHandling", "flag")
-					Default(0)
+					Default(2)
 				})
 				Field(3, "c", ArrayOf(Int64), func() {
 					Elem(func() {
@@ -907,3 +907,27 @@ var StructMetaTypeDSL = func() {
 		})
 	})
 }
+
+var DefaultFieldsDSL = func() {
+	Service("DefaultFields", func() {
+		Method("Method", func() {
+			Payload(func() {
+				Field(1, "req", Int64)
+				Field(2, "opt", Int64)
+				Field(3, "def0", Int64, func() { Default(0) })
+				Field(4, "def1", Int64, func() { Default(1) })
+				Field(5, "def2", Int64, func() { Default(2) })
+				Field(6, "reqs", String)
+				Field(7, "opts", String)
+				Field(8, "defs", String, func() { Default("!") })
+				Field(9, "defe", String, func() { Default("") })
+				Field(10, "rat", Float64)
+				Field(11, "flt", Float64)
+				Field(12, "flt0", Float64, func() { Default(0.0) })
+				Field(13, "flt1", Float64, func() { Default(1.0) })
+				Required("req", "reqs", "rat")
+			})
+			GRPC(func() {})
+		})
+	})
+}
diff --git a/grpc/codegen/testdata/proto_code.go b/grpc/codegen/testdata/proto_code.go
index 1a8dea91..f664c855 100644
--- a/grpc/codegen/testdata/proto_code.go
+++ b/grpc/codegen/testdata/proto_code.go
@@ -471,3 +471,36 @@ message MethodResponse {
 	sint64 d = 4;
 }
 `
+
+const DefaultFieldsPackageCode = `
+syntax = "proto3";
+
+package default_fields;
+
+option go_package = "/default_fieldspb";
+
+// Service is the DefaultFields service interface.
+service DefaultFields {
+	// Method implements Method.
+	rpc Method (MethodRequest) returns (MethodResponse);
+}
+
+message MethodRequest {
+	sint64 req = 1;
+	sint64 opt = 2;
+	sint64 def0 = 3;
+	sint64 def1 = 4;
+	sint64 def2 = 5;
+	string reqs = 6;
+	string opts = 7;
+	string defs = 8;
+	string defe = 9;
+	double rat = 10;
+	double flt = 11;
+	double flt0 = 12;
+	double flt1 = 13;
+}
+
+message MethodResponse {
+}
+`
diff --git a/grpc/codegen/testdata/server_type_code.go b/grpc/codegen/testdata/server_type_code.go
index a4a02e9b..b878ea06 100644
--- a/grpc/codegen/testdata/server_type_code.go
+++ b/grpc/codegen/testdata/server_type_code.go
@@ -419,11 +419,11 @@ func NewMethodPayload(message *using_meta_typespb.MethodRequest) *usingmetatypes
 	}
 	var zeroMessageA int64
 	if message.A == zeroMessageA {
-		v.A = 0
+		v.A = 1
 	}
 	var zeroMessageB int64
 	if message.B == zeroMessageB {
-		v.B = 0
+		v.B = 2
 	}
 	if message.C != nil {
 		v.C = make([]time.Duration, len(message.C))
@@ -453,3 +453,50 @@ func NewProtoMethodResponse(result *usingmetatypes.MethodResult) *using_meta_typ
 	return message
 }
 `
+
+const DefaultFieldsServerTypeCode = `// NewMethodPayload builds the payload of the "Method" endpoint of the
+// "DefaultFields" service from the gRPC request type.
+func NewMethodPayload(message *default_fieldspb.MethodRequest) *defaultfields.MethodPayload {
+	v := &defaultfields.MethodPayload{
+		Req:  message.Req,
+		Def0: message.Def0,
+		Def1: message.Def1,
+		Def2: message.Def2,
+		Reqs: message.Reqs,
+		Defs: message.Defs,
+		Defe: message.Defe,
+		Rat:  message.Rat,
+		Flt0: message.Flt0,
+		Flt1: message.Flt1,
+	}
+	if message.Opt != 0 {
+		v.Opt = &message.Opt
+	}
+	if message.Opts != "" {
+		v.Opts = &message.Opts
+	}
+	if message.Flt != 0 {
+		v.Flt = &message.Flt
+	}
+	if message.Def1 == 0 {
+		v.Def1 = 1
+	}
+	if message.Def2 == 0 {
+		v.Def2 = 2
+	}
+	if message.Defs == "" {
+		v.Defs = "!"
+	}
+	if message.Flt1 == 0 {
+		v.Flt1 = 1
+	}
+	return v
+}
+
+// NewProtoMethodResponse builds the gRPC response type from the result of the
+// "Method" endpoint of the "DefaultFields" service.
+func NewProtoMethodResponse() *default_fieldspb.MethodResponse {
+	message := &default_fieldspb.MethodResponse{}
+	return message
+}
+`