Allow non-pointer structs not mapped to goa values

[Smerom]

Proposed fix for issue #1857

[raphael]

Can you please provide an example that includes a design and generated code for both ConvertTo and CreateFrom? Trying to wrap my head around the implications...

[Smerom]

Design File

package design

import (
	. "goa.design/goa/http/design"
	. "goa.design/goa/http/dsl"
)

var _ = Service("Example", func() {
	Method("show", func() {
		Payload(ExamplePayload)
		Error("internal_error", ErrorResult)
		HTTP(func() {
			GET("/id")
			Response(StatusOK)
			Response("internal_error", StatusInternalServerError)
		})
	})
})

var ExampleNestedPayload = Type("ExampleNestedPayload", func() {
	CreateFrom(ProtoExample_InternalMessage{})
	ConvertTo(ProtoExample_InternalMessage{})
	Attribute("someValue", String)
})

var ExamplePayload = Type("ExamplePayload", func() {
	CreateFrom(ProtoExample{})
	ConvertTo(ProtoExample{})
	Attribute("name", String)
	Attribute("value", Int64)
	Attribute("nestedStruct", ExampleNestedPayload)
})

Proto File

syntax = "proto3";

package design;
option go_package = "{path_to_package}/design";

message ProtoExample {
    string Name = 1;
    int64 Value = 2;
    message InternalMessage {
        string SomeValue = 1;
    }
    InternalMessage NestedStruct = 3;
}

Generated Proto File

// Code generated by protoc-gen-go. DO NOT EDIT.
// source: design/test.proto

package design // import "{path_to_package}/design"

import proto "github.com/golang/protobuf/proto"
import fmt "fmt"
import math "math"

// Reference imports to suppress errors if they are not otherwise used.
var _ = proto.Marshal
var _ = fmt.Errorf
var _ = math.Inf

// This is a compile-time assertion to ensure that this generated file
// is compatible with the proto package it is being compiled against.
// A compilation error at this line likely means your copy of the
// proto package needs to be updated.
const _ = proto.ProtoPackageIsVersion2 // please upgrade the proto package

type ProtoExample struct {
	Name                 string                        `protobuf:"bytes,1,opt,name=Name" json:"Name,omitempty"`
	Value                int64                         `protobuf:"varint,2,opt,name=Value" json:"Value,omitempty"`
	NestedStruct         *ProtoExample_InternalMessage `protobuf:"bytes,3,opt,name=NestedStruct" json:"NestedStruct,omitempty"`
	XXX_NoUnkeyedLiteral struct{}                      `json:"-"`
	XXX_unrecognized     []byte                        `json:"-"`
	XXX_sizecache        int32                         `json:"-"`
}

func (m *ProtoExample) Reset()         { *m = ProtoExample{} }
func (m *ProtoExample) String() string { return proto.CompactTextString(m) }
func (*ProtoExample) ProtoMessage()    {}
func (*ProtoExample) Descriptor() ([]byte, []int) {
	return fileDescriptor_test_4a72c2bb587f1384, []int{0}
}
func (m *ProtoExample) XXX_Unmarshal(b []byte) error {
	return xxx_messageInfo_ProtoExample.Unmarshal(m, b)
}
func (m *ProtoExample) XXX_Marshal(b []byte, deterministic bool) ([]byte, error) {
	return xxx_messageInfo_ProtoExample.Marshal(b, m, deterministic)
}
func (dst *ProtoExample) XXX_Merge(src proto.Message) {
	xxx_messageInfo_ProtoExample.Merge(dst, src)
}
func (m *ProtoExample) XXX_Size() int {
	return xxx_messageInfo_ProtoExample.Size(m)
}
func (m *ProtoExample) XXX_DiscardUnknown() {
	xxx_messageInfo_ProtoExample.DiscardUnknown(m)
}

var xxx_messageInfo_ProtoExample proto.InternalMessageInfo

func (m *ProtoExample) GetName() string {
	if m != nil {
		return m.Name
	}
	return ""
}

func (m *ProtoExample) GetValue() int64 {
	if m != nil {
		return m.Value
	}
	return 0
}

func (m *ProtoExample) GetNestedStruct() *ProtoExample_InternalMessage {
	if m != nil {
		return m.NestedStruct
	}
	return nil
}

type ProtoExample_InternalMessage struct {
	SomeValue            string   `protobuf:"bytes,1,opt,name=SomeValue" json:"SomeValue,omitempty"`
	XXX_NoUnkeyedLiteral struct{} `json:"-"`
	XXX_unrecognized     []byte   `json:"-"`
	XXX_sizecache        int32    `json:"-"`
}

func (m *ProtoExample_InternalMessage) Reset()         { *m = ProtoExample_InternalMessage{} }
func (m *ProtoExample_InternalMessage) String() string { return proto.CompactTextString(m) }
func (*ProtoExample_InternalMessage) ProtoMessage()    {}
func (*ProtoExample_InternalMessage) Descriptor() ([]byte, []int) {
	return fileDescriptor_test_4a72c2bb587f1384, []int{0, 0}
}
func (m *ProtoExample_InternalMessage) XXX_Unmarshal(b []byte) error {
	return xxx_messageInfo_ProtoExample_InternalMessage.Unmarshal(m, b)
}
func (m *ProtoExample_InternalMessage) XXX_Marshal(b []byte, deterministic bool) ([]byte, error) {
	return xxx_messageInfo_ProtoExample_InternalMessage.Marshal(b, m, deterministic)
}
func (dst *ProtoExample_InternalMessage) XXX_Merge(src proto.Message) {
	xxx_messageInfo_ProtoExample_InternalMessage.Merge(dst, src)
}
func (m *ProtoExample_InternalMessage) XXX_Size() int {
	return xxx_messageInfo_ProtoExample_InternalMessage.Size(m)
}
func (m *ProtoExample_InternalMessage) XXX_DiscardUnknown() {
	xxx_messageInfo_ProtoExample_InternalMessage.DiscardUnknown(m)
}

var xxx_messageInfo_ProtoExample_InternalMessage proto.InternalMessageInfo

func (m *ProtoExample_InternalMessage) GetSomeValue() string {
	if m != nil {
		return m.SomeValue
	}
	return ""
}

func init() {
	proto.RegisterType((*ProtoExample)(nil), "design.ProtoExample")
	proto.RegisterType((*ProtoExample_InternalMessage)(nil), "design.ProtoExample.InternalMessage")
}

func init() { proto.RegisterFile("design/test.proto", fileDescriptor_test_4a72c2bb587f1384) }

var fileDescriptor_test_4a72c2bb587f1384 = []byte{
	// 210 bytes of a gzipped FileDescriptorProto
	0x1f, 0x8b, 0x08, 0x00, 0x00, 0x00, 0x00, 0x00, 0x02, 0xff, 0xe2, 0x12, 0x4c, 0x49, 0x2d, 0xce,
	0x4c, 0xcf, 0xd3, 0x2f, 0x49, 0x2d, 0x2e, 0xd1, 0x2b, 0x28, 0xca, 0x2f, 0xc9, 0x17, 0x62, 0x83,
	0x08, 0x29, 0x6d, 0x66, 0xe4, 0xe2, 0x09, 0x00, 0x89, 0xb8, 0x56, 0x24, 0xe6, 0x16, 0xe4, 0xa4,
	0x0a, 0x09, 0x71, 0xb1, 0xf8, 0x25, 0xe6, 0xa6, 0x4a, 0x30, 0x2a, 0x30, 0x6a, 0x70, 0x06, 0x81,
	0xd9, 0x42, 0x22, 0x5c, 0xac, 0x61, 0x89, 0x39, 0xa5, 0xa9, 0x12, 0x4c, 0x0a, 0x8c, 0x1a, 0xcc,
	0x41, 0x10, 0x8e, 0x90, 0x07, 0x17, 0x8f, 0x5f, 0x6a, 0x71, 0x49, 0x6a, 0x4a, 0x70, 0x49, 0x51,
	0x69, 0x72, 0x89, 0x04, 0xb3, 0x02, 0xa3, 0x06, 0xb7, 0x91, 0x8a, 0x1e, 0xc4, 0x64, 0x3d, 0x64,
	0x53, 0xf5, 0x3c, 0xf3, 0x4a, 0x52, 0x8b, 0xf2, 0x12, 0x73, 0x7c, 0x53, 0x8b, 0x8b, 0x13, 0xd3,
	0x53, 0x83, 0x50, 0x74, 0x4a, 0xe9, 0x73, 0xf1, 0xa3, 0x29, 0x10, 0x92, 0xe1, 0xe2, 0x0c, 0xce,
	0xcf, 0x4d, 0x85, 0x58, 0x0b, 0x71, 0x0b, 0x42, 0xc0, 0x49, 0x3b, 0x4a, 0x33, 0x3d, 0xb3, 0x24,
	0xa3, 0x34, 0x49, 0x2f, 0x39, 0x3f, 0x57, 0xdf, 0x2f, 0xb5, 0xdc, 0x37, 0x33, 0x27, 0x27, 0x35,
	0x2f, 0x2f, 0x33, 0x31, 0xc7, 0x3d, 0x31, 0x37, 0xb5, 0x18, 0xec, 0x53, 0x7d, 0x88, 0x43, 0x92,
	0xd8, 0xc0, 0x3e, 0x36, 0x06, 0x04, 0x00, 0x00, 0xff, 0xff, 0x9c, 0x27, 0x36, 0x93, 0x06, 0x01,
	0x00, 0x00,
}

Generated Convert File

// Code generated by goa v2.0.0-wip, DO NOT EDIT.
//
// Example service type conversion functions
//
// Command:
// $ goa gen {path_to_package}/design -o test

package example

import (
	design "{path_to_package}/design"
)

// ConvertToProtoExample_InternalMessage creates an instance of
// ProtoExample_InternalMessage initialized from t.
func (t *ExampleNestedPayload) ConvertToProtoExample_InternalMessage() *design.ProtoExample_InternalMessage {
	v := &design.ProtoExampleInternalMessage{}
	if t.SomeValue != nil {
		v.SomeValue = *t.SomeValue
	}
	return v
}

// ConvertToProtoExample creates an instance of ProtoExample initialized from t.
func (t *ExamplePayload) ConvertToProtoExample() *design.ProtoExample {
	v := &design.ProtoExample{}
	if t.Name != nil {
		v.Name = *t.Name
	}
	if t.Value != nil {
		v.Value = *t.Value
	}
	if t.NestedStruct != nil {
		v.NestedStruct = marshalExampleNestedPayloadToProtoExampleInternalMessageExt(t.NestedStruct)
	}
	return v
}

// CreateFromProtoExample_InternalMessage initializes t from the fields of v
func (t *ExampleNestedPayload) CreateFromProtoExample_InternalMessage(v *design.ProtoExample_InternalMessage) {
	temp := &ExampleNestedPayload{
		SomeValue: &v.SomeValue,
	}
	*t = *temp
}

// CreateFromProtoExample initializes t from the fields of v
func (t *ExamplePayload) CreateFromProtoExample(v *design.ProtoExample) {
	temp := &ExamplePayload{
		Name:  &v.Name,
		Value: &v.Value,
	}
	if v.NestedStruct != nil {
		temp.NestedStruct = marshalProtoExampleInternalMessageExtToExampleNestedPayload(v.NestedStruct)
	}
	*t = *temp
}

// marshalExampleNestedPayloadToProtoExampleInternalMessageExt builds a value
// of type *design.ProtoExampleInternalMessage from a value of type
// *ExampleNestedPayload.
func marshalExampleNestedPayloadToProtoExampleInternalMessageExt(v *ExampleNestedPayload) *design.ProtoExampleInternalMessage {
	if v == nil {
		return nil
	}
	res := &design.ProtoExampleInternalMessage{}
	if v.SomeValue != nil {
		res.SomeValue = *v.SomeValue
	}

	return res
}

// marshalProtoExampleInternalMessageExtToExampleNestedPayload builds a value
// of type *ExampleNestedPayload from a value of type
// *design.ProtoExampleInternalMessage.
func marshalProtoExampleInternalMessageExtToExampleNestedPayload(v *design.ProtoExampleInternalMessage) *ExampleNestedPayload {
	if v == nil {
		return nil
	}
	res := &ExampleNestedPayload{
		SomeValue: &v.SomeValue,
	}

	return res
}

[Smerom]

Hmmm, looks like the marshal functions are dropping the _ from the ProtoExample_InternalMessage type

[raphael]

Yeah the lack of _ looks like a bug - the code must be calling Goify which removes underscores, not sure how easy that is to fix as I'm guessing the code in question must be called from different places. Also in terms of the original fix please see #1857 (comment).

[Smerom]

Failing on the struct test, not familiar enough with goa internals to know what behavior is expected when a nil reference object is passed (as in the tests), but the idea is to allow non-mapped structs and ones marked as external to be non-pointers.

Happy to update the code and tests if you can send me an idea of what that behavior should be.

[Smerom]

The missing "_" tracks to:

goa/codegen/scope.go

Line 189 in f14c160

n := s.HashedUnique(actual, Goify(actual.Name(), true), "")

Called from:

goa/codegen/transform.go

Line 526 in f14c160

ParamTypeRef: a.scope.GoFullTypeRef(source, a.sourcePkg),

[raphael]

Thanks for the PR! This is the right idea but I'm wondering if it is wise to couple the creation of the design type with the transform method this way (meaning that the way the attribute name mapping is done is now in two places).

The idea behind the conversion algorithm is to first create a design type and then feed that design type to the existing transform code generation algorithm. Creating a design type from a Go type should be independent of the semantic of the transform. Thinking more about this I'm wondering if we couldn't simply skip attributes that are (non pointer) structs instead of erroring. We would also add a nice comment to the ConvertTo and CreateFrom DSL functions that explain that attributes on the external type that are non pointer structs are ignored. This would make it so that the Go type to design type and code transform algorithm stay decoupled. Does that make sense to you? happy to take more input on this...

[Smerom]

Yeah, that sounds like a better place to fix this. The above workaround is working for us at the moment, so I'm not sure when I might have time to understand the goa internals enough to make that change. But happy to at some point.