Repository: goadesign/goa
Pull request #3010 (2022-02-25 17:45:08)

Fix default handling of custom type during Go transformation
For https://github.com/goadesign/goa/issues/3009

@raphael moving this here for easier discussion.

This change breaks the grpc test. The result expects something like this:
```go
if source.TimeSlice == "" {
    target.TimeSlice = []time.Time{}
}
```
Which is invalid, and I imagine grpc has no business supporting `time.Time`, but maybe I'm not thinking of valid use-cases for custom types within grpc? 

Would it be better to create a new test type to just test this kind of type with `go_transform`, or try and fix the grpc stuff, or something else...? 


raphael: My inclination would be to try and make it work for gRPC as well since protobuf does support extensions via include which could mean new types that are no built-in into Goa. That being said `time.Time` wouldn't work as is because of the way github.com/golang/protobuf/ptypes/timestamp works (the code would have to generate a `AsTime()` call. So, if it ends up being too difficult to implement for gRPC it's probably OK to change the scope of the tests for now. 
maxmcd: @raphael added code changes with attempt at supporting grpc. 

Now just one weird issue. 

This test failure is rendering `string(source.TimeSlice)` for the conversion:
```
--- FAIL: TestProtoBufTransform (0.04s)
    --- FAIL: TestProtoBufTransform/to-protobuf-type (0.02s)
        --- FAIL: TestProtoBufTransform/to-protobuf-type/defaults-to-defaults (0.00s)
            protobuf_transform_test.go:147: invalid code, got:
                func transform() {
                	target := &WithDefaults{
                		Int:            int32(source.Int),
                		TimeSlice:      string(source.TimeSlice),
                		RequiredInt:    int32(source.RequiredInt),
```

The code that is rendering that here: https://github.com/maxmcd/goa/blob/32f3808786c79abb0a8cfb4d5738d3c89f60a3b8/grpc/codegen/protobuf_transform.go#L495-L502

Strangely (to me) `targetType` is equal to `""` once we get here, so it default back to the string type. I might be misunderstanding a little, but I think I would expect the `Meta` to be populated for the target `expr.AttributeExpr`. 

Any thoughts? 
maxmcd: Ah, and should I replace `[]time.Time` with maybe `[]json.RawMessage` as a more representative test case? (Or maybe not important that it is a slice)>
maxmcd: Ok, this somewhat invasive change fixes the generated code (and causes no other test failures), but not sure if this is taking things a little too far.

```diff
diff --git a/grpc/codegen/protobuf.go b/grpc/codegen/protobuf.go
index 4f852f77..f5994b73 100644
--- a/grpc/codegen/protobuf.go
+++ b/grpc/codegen/protobuf.go
@@ -213,7 +213,7 @@ func protoBufGoFullTypeName(att *expr.AttributeExpr, pkg string, s *codegen.Name
 	case expr.UserType, expr.CompositeExpr:
 		return protoBufFullMessageName(att, pkg, s)
 	case expr.Primitive:
-		return protoBufNativeGoTypeName(actual)
+		return protoBufNativeGoTypeName(att)
 	case *expr.Array:
 		return "[]" + protoBufGoFullTypeRef(actual.ElemType, pkg, s)
 	case *expr.Map:
@@ -377,8 +377,12 @@ func protoBufNativeMessageTypeName(t expr.DataType) string {
 // protoBufNativeGoTypeName returns the Go type corresponding to the given
 // primitive type generated by the protocol buffer compiler after compiling
 // the ".proto" file (in *.pb.go).
-func protoBufNativeGoTypeName(t expr.DataType) string {
-	switch t.Kind() {
+func protoBufNativeGoTypeName(att *expr.AttributeExpr) string {
+	typeName, _ := codegen.GetMetaType(att)
+	if typeName != "" {
+		return typeName
+	}
+	switch att.Type.Kind() {
 	case expr.BooleanKind:
 		return "bool"
 	case expr.IntKind:
@@ -402,7 +406,7 @@ func protoBufNativeGoTypeName(t expr.DataType) string {
 	case expr.BytesKind:
 		return "[]byte"
 	default:
-		panic(fmt.Sprintf("cannot compute native protocol buffer type for %T", t)) // bug
+		panic(fmt.Sprintf("cannot compute native protocol buffer type for %T", att.Type)) // bug
 	}
 }

diff --git a/grpc/codegen/protobuf_transform.go b/grpc/codegen/protobuf_transform.go
index 4d346bbb..4aad94e4 100644
--- a/grpc/codegen/protobuf_transform.go
+++ b/grpc/codegen/protobuf_transform.go
@@ -102,7 +102,9 @@ func protoBufTransform(source, target *expr.AttributeExpr, sourceVar, targetVar
 // generating protobuf types (as protogen itself won't honor these overrides).
 func removeMeta(att *expr.AttributeExpr) {
 	codegen.Walk(att, func(a *expr.AttributeExpr) error {
-		a.Meta = nil
+		delete(a.Meta, "struct:field:name")
+		delete(a.Meta, "struct:field:external")
+		// a.Meta = nil
 		return nil
 	})
 }
@@ -493,14 +495,14 @@ func convertType(source, target *expr.AttributeExpr, sourceVar string, ta *trans
 	if source.Type.Kind() != expr.IntKind && source.Type.Kind() != expr.UIntKind {
 		if sourceType != "" || targetType != "" {
 			if ta.proto || targetType == "" {
-				targetType = protoBufNativeGoTypeName(target.Type)
+				targetType = protoBufNativeGoTypeName(target)
 			}
 			return fmt.Sprintf("%s(%s)", targetType, sourceVar)
 		}
 		return sourceVar
 	}
 	if ta.proto {
-		return fmt.Sprintf("%s(%s)", protoBufNativeGoTypeName(target.Type), sourceVar)
+		return fmt.Sprintf("%s(%s)", protoBufNativeGoTypeName(target), sourceVar)
 	}
 	if targetType == "" {
 		targetType = codegen.GoNativeTypeName(target.Type)
```


raphael: The change above makes sense! It may allow for using protobuf extensions that add support for specific types so seems to be warranted. Thank you for the great contribution!  
raphael: Thank you!

Patch:
diff --git a/codegen/go_transform.go b/codegen/go_transform.go
index 0e8aeda7..8a627db2 100644
--- a/codegen/go_transform.go
+++ b/codegen/go_transform.go
@@ -251,7 +251,9 @@ func transformObject(source, target *expr.AttributeExpr, sourceVar, targetVar st
 				// source attribute is a primitive with default value
 				// (the field is not a pointer in this case)
 				code += "{\n\t"
-				if _, ok := tgtc.Type.(expr.UserType); ok {
+				if typeName, _ := GetMetaType(tgtc); typeName != "" {
+					code += fmt.Sprintf("var zero %s\n\t", typeName)
+				} else if _, ok := tgtc.Type.(expr.UserType); ok {
 					// aliased primitive
 					code += fmt.Sprintf("var zero %s\n\t", ta.TargetCtx.Scope.Ref(tgtc, ta.TargetCtx.Pkg))
 				} else {
diff --git a/grpc/codegen/protobuf.go b/grpc/codegen/protobuf.go
index 4f852f77..f5994b73 100644
--- a/grpc/codegen/protobuf.go
+++ b/grpc/codegen/protobuf.go
@@ -213,7 +213,7 @@ func protoBufGoFullTypeName(att *expr.AttributeExpr, pkg string, s *codegen.Name
 	case expr.UserType, expr.CompositeExpr:
 		return protoBufFullMessageName(att, pkg, s)
 	case expr.Primitive:
-		return protoBufNativeGoTypeName(actual)
+		return protoBufNativeGoTypeName(att)
 	case *expr.Array:
 		return "[]" + protoBufGoFullTypeRef(actual.ElemType, pkg, s)
 	case *expr.Map:
@@ -377,8 +377,12 @@ func protoBufNativeMessageTypeName(t expr.DataType) string {
 // protoBufNativeGoTypeName returns the Go type corresponding to the given
 // primitive type generated by the protocol buffer compiler after compiling
 // the ".proto" file (in *.pb.go).
-func protoBufNativeGoTypeName(t expr.DataType) string {
-	switch t.Kind() {
+func protoBufNativeGoTypeName(att *expr.AttributeExpr) string {
+	typeName, _ := codegen.GetMetaType(att)
+	if typeName != "" {
+		return typeName
+	}
+	switch att.Type.Kind() {
 	case expr.BooleanKind:
 		return "bool"
 	case expr.IntKind:
@@ -402,7 +406,7 @@ func protoBufNativeGoTypeName(t expr.DataType) string {
 	case expr.BytesKind:
 		return "[]byte"
 	default:
-		panic(fmt.Sprintf("cannot compute native protocol buffer type for %T", t)) // bug
+		panic(fmt.Sprintf("cannot compute native protocol buffer type for %T", att.Type)) // bug
 	}
 }
 
diff --git a/grpc/codegen/protobuf_transform.go b/grpc/codegen/protobuf_transform.go
index 50c18f22..18910dd3 100644
--- a/grpc/codegen/protobuf_transform.go
+++ b/grpc/codegen/protobuf_transform.go
@@ -99,8 +99,9 @@ func protoBufTransform(source, target *expr.AttributeExpr, sourceVar, targetVar
 // needed to make sure that any field name overridding is removed when
 // generating protobuf types (as protogen itself won't honor these overrides).
 func removeMeta(att *expr.AttributeExpr) {
-	codegen.Walk(att, func(a *expr.AttributeExpr) error {
-		a.Meta = nil
+	_ = codegen.Walk(att, func(a *expr.AttributeExpr) error {
+		delete(a.Meta, "struct:field:name")
+		delete(a.Meta, "struct:field:external")
 		return nil
 	})
 }
@@ -307,7 +308,12 @@ func transformObject(source, target *expr.AttributeExpr, sourceVar, targetVar st
 				// buffer sets boolean fields as false. Changing them to the default
 				// value is counter-intuitive.
 				if !srcMatt.IsRequired(n) && srcc.Type != expr.Boolean {
-					code += fmt.Sprintf("if %s {\n\t", checkZeroValue(srcc.Type, srcVar, false))
+					if typeName, _ := codegen.GetMetaType(tgtc); typeName != "" {
+						code += fmt.Sprintf("var zero %s\n\t", typeName)
+						code += fmt.Sprintf("if %s == zero {\n\t", srcVar)
+					} else {
+						code += fmt.Sprintf("if %s {\n\t", checkZeroValue(srcc.Type, srcVar, false))
+					}
 					if ta.TargetCtx.IsPrimitivePointer(n, tgtMatt.AttributeExpr) && expr.IsPrimitive(tgtc.Type) {
 						code += fmt.Sprintf("var tmp %s = %#v\n\t%s = &tmp\n", codegen.GoNativeTypeName(tgtc.Type), tdef, tgtVar)
 					} else {
@@ -489,7 +495,7 @@ func convertType(src, tgt *expr.AttributeExpr, srcVar string, ta *transformAttrs
 	if src.Type != expr.Int && src.Type != expr.UInt {
 		if srcType != "" || tgtType != "" {
 			if ta.proto || tgtType == "" {
-				tgtType = protoBufNativeGoTypeName(tgt.Type)
+				tgtType = protoBufNativeGoTypeName(tgt)
 			}
 			return fmt.Sprintf("%s(%s)", tgtType, srcVar)
 		}
@@ -497,7 +503,7 @@ func convertType(src, tgt *expr.AttributeExpr, srcVar string, ta *transformAttrs
 	}
 
 	if ta.proto {
-		return fmt.Sprintf("%s(%s)", protoBufNativeGoTypeName(tgt.Type), srcVar)
+		return fmt.Sprintf("%s(%s)", protoBufNativeGoTypeName(tgt), srcVar)
 	}
 	if tgtType == "" {
 		tgtType = codegen.GoNativeTypeName(tgt.Type)


Test patch:
diff --git a/codegen/go_transform_test.go b/codegen/go_transform_test.go
index f1137fec..30df3933 100644
--- a/codegen/go_transform_test.go
+++ b/codegen/go_transform_test.go
@@ -37,6 +37,7 @@ func TestGoTransform(t *testing.T) {
 		recursiveMap   = root.UserType("RecursiveMap")
 		composite      = root.UserType("Composite")
 		customField    = root.UserType("CompositeWithCustomField")
+		defaults       = root.UserType("WithDefaults")
 
 		resultType = root.UserType("ResultType")
 		rtCol      = root.UserType("ResultTypeCollection")
@@ -106,6 +107,7 @@ func TestGoTransform(t *testing.T) {
 			{"composite-to-custom-field-pkg", composite, customField, defaultCtx, defaultCtxPkg, srcTgtUseDefaultCompositeToCustomFieldPkgCode},
 			{"result-type-to-result-type", resultType, resultType, defaultCtx, defaultCtx, srcTgtUseDefaultResultTypeToResultTypeCode},
 			{"result-type-collection-to-result-type-collection", rtCol, rtCol, defaultCtx, defaultCtx, srcTgtUseDefaultRTColToRTColCode},
+			{"defaults-to-defaults-types", defaults, defaults, defaultCtx, defaultCtx, srcTgtDefaultsToDefaultsCode},
 
 			// alias
 			{"simple-alias-to-simple", simpleAlias, simple, defaultCtx, defaultCtx, srcTgtUseDefaultSimpleAliasToSimpleCode},
@@ -724,6 +726,85 @@ const (
 		}
 	}
 }
+`
+
+	srcTgtDefaultsToDefaultsCode = `func transform() {
+	target := &WithDefaults{
+		Int:            source.Int,
+		RawJSON:        source.RawJSON,
+		RequiredInt:    source.RequiredInt,
+		String:         source.String,
+		RequiredString: source.RequiredString,
+		Bytes:          source.Bytes,
+		RequiredBytes:  source.RequiredBytes,
+		Any:            source.Any,
+		RequiredAny:    source.RequiredAny,
+	}
+	{
+		var zero int
+		if target.Int == zero {
+			target.Int = 100
+		}
+	}
+	{
+		var zero json.RawMessage
+		if target.RawJSON == zero {
+			target.RawJSON = json.RawMessage{0x66, 0x6f, 0x6f}
+		}
+	}
+	{
+		var zero string
+		if target.String == zero {
+			target.String = "foo"
+		}
+	}
+	{
+		var zero []byte
+		if target.Bytes == zero {
+			target.Bytes = []byte{0x66, 0x6f, 0x6f, 0x62, 0x61, 0x72}
+		}
+	}
+	{
+		var zero interface{}
+		if target.Any == zero {
+			target.Any = "something"
+		}
+	}
+	if source.Array != nil {
+		target.Array = make([]string, len(source.Array))
+		for i, val := range source.Array {
+			target.Array[i] = val
+		}
+	}
+	if source.Array == nil {
+		target.Array = []string{"foo", "bar"}
+	}
+	if source.RequiredArray != nil {
+		target.RequiredArray = make([]string, len(source.RequiredArray))
+		for i, val := range source.RequiredArray {
+			target.RequiredArray[i] = val
+		}
+	}
+	if source.Map != nil {
+		target.Map = make(map[int]string, len(source.Map))
+		for key, val := range source.Map {
+			tk := key
+			tv := val
+			target.Map[tk] = tv
+		}
+	}
+	if source.Map == nil {
+		target.Map = map[int]string{1: "foo"}
+	}
+	if source.RequiredMap != nil {
+		target.RequiredMap = make(map[int]string, len(source.RequiredMap))
+		for key, val := range source.RequiredMap {
+			tk := key
+			tv := val
+			target.RequiredMap[tk] = tv
+		}
+	}
+}
 `
 
 	srcTgtUseDefaultSimpleAliasToSimpleCode = `func transform() {
diff --git a/codegen/testdata/types_dsl.go b/codegen/testdata/types_dsl.go
index 4f94d526..af638f20 100644
--- a/codegen/testdata/types_dsl.go
+++ b/codegen/testdata/types_dsl.go
@@ -1,6 +1,10 @@
 package testdata
 
-import . "goa.design/goa/v3/dsl"
+import (
+	"encoding/json"
+
+	. "goa.design/goa/v3/dsl"
+)
 
 var TestTypesDSL = func() {
 	var (
@@ -191,6 +195,10 @@ var TestTypesDSL = func() {
 			Attribute("int", Int, func() {
 				Default(100)
 			})
+			Attribute("raw_json", func() {
+				Meta("struct:field:type", "json.RawMessage", "json")
+				Default(json.RawMessage("foo"))
+			})
 			Attribute("required_int", Int, func() {
 				Default(99)
 			})
diff --git a/grpc/codegen/protobuf_transform_test.go b/grpc/codegen/protobuf_transform_test.go
index bd7d5f8b..0339e0b1 100644
--- a/grpc/codegen/protobuf_transform_test.go
+++ b/grpc/codegen/protobuf_transform_test.go
@@ -258,11 +258,11 @@ const (
 
 	customSvcToCustomProtoCode = `func transform() {
 	target := &CustomTypes{
-		RequiredString: string(source.RequiredString),
-		DefaultBool:    bool(source.DefaultBool),
+		RequiredString: tdtypes.CustomString(source.RequiredString),
+		DefaultBool:    tdtypes.CustomBool(source.DefaultBool),
 	}
 	if source.Integer != nil {
-		target.Integer = int32(*source.Integer)
+		target.Integer = tdtypes.CustomInt(*source.Integer)
 	}
 }
 `
@@ -563,6 +563,7 @@ const (
 	defaultsSvcToDefaultsProtoCode = `func transform() {
 	target := &WithDefaults{
 		Int:            int32(source.Int),
+		RawJson:        json.RawMessage(source.RawJSON),
 		RequiredInt:    int32(source.RequiredInt),
 		String_:        source.String,
 		RequiredString: source.RequiredString,
@@ -969,6 +970,7 @@ const (
 	defaultsProtoToDefaultsSvcCode = `func transform() {
 	target := &WithDefaults{
 		Int:            int(source.Int),
+		RawJSON:        json.RawMessage(source.RawJson),
 		RequiredInt:    int(source.RequiredInt),
 		String:         source.String_,
 		RequiredString: source.RequiredString,
@@ -980,6 +982,10 @@ const (
 	if source.Int == 0 {
 		target.Int = 100
 	}
+	var zero json.RawMessage
+	if source.RawJson == zero {
+		target.RawJSON = json.RawMessage{0x66, 0x6f, 0x6f}
+	}
 	if source.String_ == "" {
 		target.String = "foo"
 	}