Repository: goadesign/goa
Pull request #3015 (2022-02-28 19:27:16)

Correctly handle multiple user types with common path


Patch:
diff --git a/codegen/generator/service.go b/codegen/generator/service.go
index 92e8c602..d9ea36f4 100644
--- a/codegen/generator/service.go
+++ b/codegen/generator/service.go
@@ -14,13 +14,14 @@ import (
 // a goa design.
 func Service(genpkg string, roots []eval.Root) ([]*codegen.File, error) {
 	var files []*codegen.File
+	var userTypePkgs = make(map[string][]string)
 	for _, root := range roots {
 		switch r := root.(type) {
 		case *expr.RootExpr:
 			for _, s := range r.Services {
 				// Make sure service is first so name scope is
 				// properly initialized.
-				files = append(files, service.Files(genpkg, s)...)
+				files = append(files, service.Files(genpkg, s, userTypePkgs)...)
 				files = append(files, service.EndpointFile(genpkg, s))
 				files = append(files, service.ClientFile(genpkg, s))
 				if f := service.ViewsFile(genpkg, s); f != nil {
diff --git a/codegen/service/service.go b/codegen/service/service.go
index 31a040c3..d47dd54f 100644
--- a/codegen/service/service.go
+++ b/codegen/service/service.go
@@ -9,24 +9,35 @@ import (
 	"goa.design/goa/v3/expr"
 )
 
-// Files returns the service and user type files for the given service.
-func Files(genpkg string, service *expr.ServiceExpr) []*codegen.File {
+// Files returns the generated files for the given service as well as a map
+// indexing user type names by custom path as defined by the "struct:pkg:path"
+// metadata. The map is built over each invocation of Files to avoid duplicate
+// type definitions.
+func Files(genpkg string, service *expr.ServiceExpr, userTypePkgs map[string][]string) []*codegen.File {
 	svc := Services.Get(service.Name)
 	svc.initUserTypeImports(genpkg)
 	svcName := svc.PathName
 	svcPath := filepath.Join(codegen.Gendir, svcName, "service.go")
-
 	seen := make(map[string]struct{})
-	sectionsByPath := make(map[string][]*codegen.SectionTemplate)
-	sectionsByPath[svcPath] = []*codegen.SectionTemplate{}
+	typeDefSections := make(map[string]map[string]*codegen.SectionTemplate)
+	typesByPath := make(map[string][]string)
+	var svcSections []*codegen.SectionTemplate
+
+	addTypeDefSection := func(path, name string, section *codegen.SectionTemplate) {
+		if typeDefSections[path] == nil {
+			typeDefSections[path] = make(map[string]*codegen.SectionTemplate)
+		}
+		typeDefSections[path][name] = section
+		typesByPath[path] = append(typesByPath[path], name)
+		seen[name] = struct{}{}
+	}
 
 	for _, m := range svc.Methods {
 		payloadPath := pathWithDefault(m.PayloadLoc, svcPath)
 		resultPath := pathWithDefault(m.ResultLoc, svcPath)
 		if m.PayloadDef != "" {
 			if _, ok := seen[m.Payload]; !ok {
-				seen[m.Payload] = struct{}{}
-				sectionsByPath[payloadPath] = append(sectionsByPath[payloadPath], &codegen.SectionTemplate{
+				addTypeDefSection(payloadPath, m.Payload, &codegen.SectionTemplate{
 					Name:   "service-payload",
 					Source: payloadT,
 					Data:   m,
@@ -35,8 +46,7 @@ func Files(genpkg string, service *expr.ServiceExpr) []*codegen.File {
 		}
 		if m.StreamingPayloadDef != "" {
 			if _, ok := seen[m.StreamingPayload]; !ok {
-				seen[m.StreamingPayload] = struct{}{}
-				sectionsByPath[payloadPath] = append(sectionsByPath[payloadPath], &codegen.SectionTemplate{
+				addTypeDefSection(payloadPath, m.StreamingPayload, &codegen.SectionTemplate{
 					Name:   "service-streamig-payload",
 					Source: streamingPayloadT,
 					Data:   m,
@@ -45,8 +55,7 @@ func Files(genpkg string, service *expr.ServiceExpr) []*codegen.File {
 		}
 		if m.ResultDef != "" {
 			if _, ok := seen[m.Result]; !ok {
-				seen[m.Result] = struct{}{}
-				sectionsByPath[resultPath] = append(sectionsByPath[resultPath], &codegen.SectionTemplate{
+				addTypeDefSection(resultPath, m.Result, &codegen.SectionTemplate{
 					Name:   "service-result",
 					Source: resultT,
 					Data:   m,
@@ -56,8 +65,7 @@ func Files(genpkg string, service *expr.ServiceExpr) []*codegen.File {
 	}
 	for _, ut := range svc.userTypes {
 		if _, ok := seen[ut.VarName]; !ok {
-			p := pathWithDefault(ut.Loc, svcPath)
-			sectionsByPath[p] = append(sectionsByPath[p], &codegen.SectionTemplate{
+			addTypeDefSection(pathWithDefault(ut.Loc, svcPath), ut.VarName, &codegen.SectionTemplate{
 				Name:   "service-user-type",
 				Source: userTypeT,
 				Data:   ut,
@@ -74,9 +82,7 @@ func Files(genpkg string, service *expr.ServiceExpr) []*codegen.File {
 		if _, ok := seenErrs[et.Name]; !ok {
 			seenErrs[et.Name] = struct{}{}
 			if _, ok := seen[et.Name]; !ok {
-				seen[et.Name] = struct{}{}
-				p := pathWithDefault(et.Loc, svcPath)
-				sectionsByPath[p] = append(sectionsByPath[p], &codegen.SectionTemplate{
+				addTypeDefSection(pathWithDefault(et.Loc, svcPath), et.Name, &codegen.SectionTemplate{
 					Name:   "error-user-type",
 					Source: userTypeT,
 					Data:   et,
@@ -87,8 +93,11 @@ func Files(genpkg string, service *expr.ServiceExpr) []*codegen.File {
 	}
 
 	for _, et := range errorTypes {
-		p := pathWithDefault(et.Loc, svcPath)
-		sectionsByPath[p] = append(sectionsByPath[p], &codegen.SectionTemplate{
+		// Don't override the section created for the error type
+		// declaration, make sure the key does not clash with existing
+		// type names, make it generated last.
+		key := "zz*" + et.Name
+		addTypeDefSection(pathWithDefault(et.Loc, svcPath), key, &codegen.SectionTemplate{
 			Name:    "service-error",
 			Source:  errorT,
 			FuncMap: map[string]interface{}{"errorName": errorName},
@@ -96,7 +105,7 @@ func Files(genpkg string, service *expr.ServiceExpr) []*codegen.File {
 		})
 	}
 	for _, er := range svc.errorInits {
-		sectionsByPath[svcPath] = append(sectionsByPath[svcPath], &codegen.SectionTemplate{
+		svcSections = append(svcSections, &codegen.SectionTemplate{
 			Name:   "error-init-func",
 			Source: errorInitT,
 			Data:   er,
@@ -105,12 +114,12 @@ func Files(genpkg string, service *expr.ServiceExpr) []*codegen.File {
 
 	// transform result type functions
 	for _, t := range svc.viewedResultTypes {
-		sectionsByPath[svcPath] = append(sectionsByPath[svcPath], &codegen.SectionTemplate{
+		svcSections = append(svcSections, &codegen.SectionTemplate{
 			Name:   "viewed-result-type-to-service-result-type",
 			Source: typeInitT,
 			Data:   t.ResultInit,
 		})
-		sectionsByPath[svcPath] = append(sectionsByPath[svcPath], &codegen.SectionTemplate{
+		svcSections = append(svcSections, &codegen.SectionTemplate{
 			Name:   "service-result-type-to-viewed-result-type",
 			Source: typeInitT,
 			Data:   t.Init,
@@ -120,7 +129,7 @@ func Files(genpkg string, service *expr.ServiceExpr) []*codegen.File {
 	for _, t := range svc.projectedTypes {
 		for _, i := range t.TypeInits {
 			projh = codegen.AppendHelpers(projh, i.Helpers)
-			sectionsByPath[svcPath] = append(sectionsByPath[svcPath], &codegen.SectionTemplate{
+			svcSections = append(svcSections, &codegen.SectionTemplate{
 				Name:   "projected-type-to-service-type",
 				Source: typeInitT,
 				Data:   i,
@@ -128,7 +137,7 @@ func Files(genpkg string, service *expr.ServiceExpr) []*codegen.File {
 		}
 		for _, i := range t.Projections {
 			projh = codegen.AppendHelpers(projh, i.Helpers)
-			sectionsByPath[svcPath] = append(sectionsByPath[svcPath], &codegen.SectionTemplate{
+			svcSections = append(svcSections, &codegen.SectionTemplate{
 				Name:   "service-type-to-projected-type",
 				Source: typeInitT,
 				Data:   i,
@@ -137,7 +146,7 @@ func Files(genpkg string, service *expr.ServiceExpr) []*codegen.File {
 	}
 
 	for _, h := range projh {
-		sectionsByPath[svcPath] = append(sectionsByPath[svcPath], &codegen.SectionTemplate{
+		svcSections = append(svcSections, &codegen.SectionTemplate{
 			Name:   "transform-helpers",
 			Source: transformHelperT,
 			Data:   h,
@@ -159,11 +168,29 @@ func Files(genpkg string, service *expr.ServiceExpr) []*codegen.File {
 		Data:    svc,
 		FuncMap: map[string]interface{}{"streamInterfaceFor": streamInterfaceFor},
 	}
-	sections := append([]*codegen.SectionTemplate{header, def}, sectionsByPath[svcPath]...)
+
+	// service.go
+	var sections []*codegen.SectionTemplate
+	{
+		sections = []*codegen.SectionTemplate{header, def}
+		names := make([]string, len(typeDefSections[svcPath]))
+		i := 0
+		for n := range typeDefSections[svcPath] {
+			names[i] = n
+			i++
+		}
+		sort.Strings(names)
+		for _, n := range names {
+			sections = append(sections, typeDefSections[svcPath][n])
+		}
+		sections = append(sections, svcSections...)
+	}
 	files := []*codegen.File{{Path: svcPath, SectionTemplates: sections}}
-	paths := make([]string, len(sectionsByPath))
+
+	// user types
+	paths := make([]string, len(typeDefSections))
 	i := 0
-	for p := range sectionsByPath {
+	for p := range typesByPath {
 		paths[i] = p
 		i++
 	}
@@ -172,10 +199,30 @@ func Files(genpkg string, service *expr.ServiceExpr) []*codegen.File {
 		if p == svcPath {
 			continue
 		}
+		var secs []*codegen.SectionTemplate
+		ts := typesByPath[p]
+		sort.Strings(ts)
+		for _, name := range ts {
+			hasName := false
+			for _, n := range userTypePkgs[p] {
+				if hasName = n == name; hasName {
+					break
+				}
+			}
+			if hasName {
+				continue
+			}
+			userTypePkgs[p] = append(userTypePkgs[p], name)
+			secs = append(secs, typeDefSections[p][name])
+		}
+		if len(secs) == 0 {
+			continue
+		}
 		h := codegen.Header("User types", packageFromPath(p), nil)
-		sections := append([]*codegen.SectionTemplate{h}, sectionsByPath[p]...)
+		sections := append([]*codegen.SectionTemplate{h}, secs...)
 		files = append(files, &codegen.File{Path: filepath.Join(codegen.Gendir, p), SectionTemplates: sections})
 	}
+
 	return files
 }
 


Test patch:
diff --git a/codegen/service/service_test.go b/codegen/service/service_test.go
index b2565b3a..58a132c0 100644
--- a/codegen/service/service_test.go
+++ b/codegen/service/service_test.go
@@ -58,7 +58,7 @@ func TestService(t *testing.T) {
 			if len(expr.Root.Services) != 1 {
 				t.Fatalf("got %d services, expected 1", len(expr.Root.Services))
 			}
-			files := Files("goa.design/goa/example", expr.Root.Services[0])
+			files := Files("goa.design/goa/example", expr.Root.Services[0], make(map[string][]string))
 			if len(files) == 0 {
 				t.Fatalf("got no file, expected one")
 			}
@@ -74,31 +74,40 @@ func TestStructPkgPath(t *testing.T) {
 	cases := []struct {
 		Name      string
 		DSL       func()
-		Code      string
+		SvcCodes  []string
 		TypeFiles []string
 		TypeCodes []string
 	}{
-		{"none", testdata.SingleMethodDSL, testdata.SingleMethod, nil, nil},
-		{"single", testdata.PkgPathDSL, testdata.PkgPath, []string{fooPath}, []string{testdata.PkgPathFoo}},
-		{"multiple", testdata.MultiplePkgPathDSL, testdata.PkgPathMultiple, []string{barPath, bazPath}, []string{testdata.PkgPathBar, testdata.PkgPathBaz}},
-		{"nopkg", testdata.PkgPathNoDirDSL, testdata.PkgPathNoDir, nil, nil},
+		{"none", testdata.SingleMethodDSL, []string{testdata.SingleMethod}, nil, nil},
+		{"single", testdata.PkgPathDSL, []string{testdata.PkgPath}, []string{fooPath}, []string{testdata.PkgPathFoo}},
+		{"multiple", testdata.PkgPathMultipleDSL, []string{testdata.PkgPathMultiple}, []string{barPath, bazPath}, []string{testdata.PkgPathBar, testdata.PkgPathBaz}},
+		{"nopkg", testdata.PkgPathNoDirDSL, []string{testdata.PkgPathNoDir}, nil, nil},
+		{"dupes", testdata.PkgPathDupeDSL, []string{testdata.PkgPathDupe1, testdata.PkgPathDupe2}, []string{fooPath}, []string{testdata.PkgPathFooDupe}},
 	}
 	for _, c := range cases {
 		t.Run(c.Name, func(t *testing.T) {
+			userTypePkgs := make(map[string][]string)
 			codegen.RunDSLWithFunc(t, c.DSL, func() {
 				expr.Root.Types = []expr.UserType{testdata.APayload, testdata.AResult, testdata.Foo, testdata.Bar, testdata.Baz, testdata.NoDir}
 			})
-			if len(expr.Root.Services) != 1 {
-				t.Fatalf("got %d services, expected 1", len(expr.Root.Services))
+			if len(expr.Root.Services) != len(c.SvcCodes) {
+				t.Fatalf("got %d services, expected %d", len(expr.Root.Services), len(c.SvcCodes))
 			}
-			files := Files("goa.design/goa/example", expr.Root.Services[0])
+			files := Files("goa.design/goa/example", expr.Root.Services[0], userTypePkgs)
 			if len(files) != len(c.TypeFiles)+1 {
 				t.Fatalf("got %d files, expected %d", len(files), len(c.TypeFiles)+1)
 			}
-			validateFile(t, files[0], files[0].Path, c.Code)
+			validateFile(t, files[0], files[0].Path, c.SvcCodes[0])
 			for i, f := range c.TypeFiles {
 				validateFile(t, files[i+1], f, c.TypeCodes[i])
 			}
+			if len(c.SvcCodes) > 1 {
+				files = Files("goa.design/goa/example", expr.Root.Services[1], userTypePkgs)
+				if len(files) != 1 {
+					t.Fatalf("got %d files, expected 1", len(files))
+				}
+				validateFile(t, files[0], files[0].Path, c.SvcCodes[1])
+			}
 		})
 	}
 }
diff --git a/codegen/service/testdata/service_code.go b/codegen/service/testdata/service_code.go
index 1f8d6050..57126d5d 100644
--- a/codegen/service/testdata/service_code.go
+++ b/codegen/service/testdata/service_code.go
@@ -156,13 +156,13 @@ type BResult struct {
 	UserTypeField *Parent
 }
 
-type Parent struct {
-	C *Child
-}
-
 type Child struct {
 	P *Parent
 }
+
+type Parent struct {
+	C *Child
+}
 `
 
 const WithDefault = `
@@ -315,9 +315,6 @@ const ServiceName = "CustomErrors"
 // MethodKey key.
 var MethodNames = [1]string{"A"}
 
-// primitive error description
-type Primitive string
-
 type APayload struct {
 	IntField      int
 	StringField   string
@@ -331,15 +328,8 @@ type Result struct {
 	B string
 }
 
-// Error returns an error description.
-func (e Primitive) Error() string {
-	return "primitive error description"
-}
-
-// ErrorName returns "primitive".
-func (e Primitive) ErrorName() string {
-	return "primitive"
-}
+// primitive error description
+type Primitive string
 
 // Error returns an error description.
 func (e *APayload) Error() string {
@@ -360,6 +350,16 @@ func (e *Result) Error() string {
 func (e *Result) ErrorName() string {
 	return e.B
 }
+
+// Error returns an error description.
+func (e Primitive) Error() string {
+	return "primitive error description"
+}
+
+// ErrorName returns "primitive".
+func (e Primitive) ErrorName() string {
+	return "primitive"
+}
 `
 
 const CustomErrorsCustomField = `
@@ -659,15 +659,15 @@ const ServiceName = "ResultCollectionMultipleViewsMethod"
 // MethodKey key.
 var MethodNames = [1]string{"A"}
 
-// MultipleViewsCollection is the result type of the
-// ResultCollectionMultipleViewsMethod service A method.
-type MultipleViewsCollection []*MultipleViews
-
 type MultipleViews struct {
 	A string
 	B int
 }
 
+// MultipleViewsCollection is the result type of the
+// ResultCollectionMultipleViewsMethod service A method.
+type MultipleViewsCollection []*MultipleViews
+
 // NewMultipleViewsCollection initializes result type MultipleViewsCollection
 // from viewed result type MultipleViewsCollection.
 func NewMultipleViewsCollection(vres resultcollectionmultipleviewsmethodviews.MultipleViewsCollection) MultipleViewsCollection {
@@ -956,14 +956,14 @@ type RT struct {
 	A RT2Collection
 }
 
-type RT2Collection []*RT2
-
 type RT2 struct {
 	C string
 	D int
 	E *string
 }
 
+type RT2Collection []*RT2
+
 // NewRT initializes result type RT from viewed result type RT.
 func NewRT(vres *resultwithresulttypecollectionviews.RT) *RT {
 	var res *RT
@@ -1204,14 +1204,14 @@ type ApplicationDashedType struct {
 	Name *string
 }
 
+type ApplicationDashedTypeCollection []*ApplicationDashedType
+
 // ListResult is the result type of the ResultWithDashedMimeType service list
 // method.
 type ListResult struct {
 	Items ApplicationDashedTypeCollection
 }
 
-type ApplicationDashedTypeCollection []*ApplicationDashedType
-
 // NewApplicationDashedType initializes result type ApplicationDashedType from
 // viewed result type ApplicationDashedType.
 func NewApplicationDashedType(vres *resultwithdashedmimetypeviews.ApplicationDashedType) *ApplicationDashedType {
@@ -1644,18 +1644,6 @@ type StreamingPayloadMethodClientStream interface {
 	CloseAndRecv() (*AResult, error)
 }
 
-// BPayload is the payload type of the StreamingPayloadService service
-// StreamingPayloadMethod method.
-type BPayload struct {
-	ArrayField  []bool
-	MapField    map[int]string
-	ObjectField *struct {
-		IntField    *int
-		StringField *string
-	}
-	UserTypeField *Parent
-}
-
 // APayload is the streaming payload type of the StreamingPayloadService
 // service StreamingPayloadMethod method.
 type APayload struct {
@@ -1676,13 +1664,25 @@ type AResult struct {
 	OptionalField *string
 }
 
-type Parent struct {
-	C *Child
+// BPayload is the payload type of the StreamingPayloadService service
+// StreamingPayloadMethod method.
+type BPayload struct {
+	ArrayField  []bool
+	MapField    map[int]string
+	ObjectField *struct {
+		IntField    *int
+		StringField *string
+	}
+	UserTypeField *Parent
 }
 
 type Child struct {
 	P *Parent
 }
+
+type Parent struct {
+	C *Child
+}
 `
 
 const StreamingPayloadNoPayloadMethod = `
@@ -2040,18 +2040,6 @@ type BidirectionalStreamingMethodClientStream interface {
 	Close() error
 }
 
-// BPayload is the payload type of the BidirectionalStreamingService service
-// BidirectionalStreamingMethod method.
-type BPayload struct {
-	ArrayField  []bool
-	MapField    map[int]string
-	ObjectField *struct {
-		IntField    *int
-		StringField *string
-	}
-	UserTypeField *Parent
-}
-
 // APayload is the streaming payload type of the BidirectionalStreamingService
 // service BidirectionalStreamingMethod method.
 type APayload struct {
@@ -2072,13 +2060,25 @@ type AResult struct {
 	OptionalField *string
 }
 
-type Parent struct {
-	C *Child
+// BPayload is the payload type of the BidirectionalStreamingService service
+// BidirectionalStreamingMethod method.
+type BPayload struct {
+	ArrayField  []bool
+	MapField    map[int]string
+	ObjectField *struct {
+		IntField    *int
+		StringField *string
+	}
+	UserTypeField *Parent
 }
 
 type Child struct {
 	P *Parent
 }
+
+type Parent struct {
+	C *Child
+}
 `
 
 const BidirectionalStreamingNoPayloadMethod = `
@@ -2452,3 +2452,49 @@ type NoDir struct {
 	IntField *int
 }
 `
+
+const PkgPathDupe1 = `
+// Service is the PkgPathDupeMethod service interface.
+type Service interface {
+	// A implements A.
+	A(context.Context, *foo.Foo) (res *foo.Foo, err error)
+	// B implements B.
+	B(context.Context, *foo.Foo) (res *foo.Foo, err error)
+}
+
+// ServiceName is the name of the service as defined in the design. This is the
+// same value that is set in the endpoint request contexts under the ServiceKey
+// key.
+const ServiceName = "PkgPathDupeMethod"
+
+// MethodNames lists the service method names as defined in the design. These
+// are the same values that are set in the endpoint request contexts under the
+// MethodKey key.
+var MethodNames = [2]string{"A", "B"}
+`
+
+const PkgPathFooDupe = `// Foo is the payload type of the PkgPathDupeMethod service A method.
+type Foo struct {
+	IntField *int
+}
+`
+
+const PkgPathDupe2 = `
+// Service is the PkgPathDupeMethod2 service interface.
+type Service interface {
+	// A implements A.
+	A(context.Context, *foo.Foo) (res *foo.Foo, err error)
+	// B implements B.
+	B(context.Context, *foo.Foo) (res *foo.Foo, err error)
+}
+
+// ServiceName is the name of the service as defined in the design. This is the
+// same value that is set in the endpoint request contexts under the ServiceKey
+// key.
+const ServiceName = "PkgPathDupeMethod2"
+
+// MethodNames lists the service method names as defined in the design. These
+// are the same values that are set in the endpoint request contexts under the
+// MethodKey key.
+var MethodNames = [2]string{"A", "B"}
+`
diff --git a/codegen/service/testdata/service_dsls.go b/codegen/service/testdata/service_dsls.go
index 56b0920b..8aaf48ce 100644
--- a/codegen/service/testdata/service_dsls.go
+++ b/codegen/service/testdata/service_dsls.go
@@ -636,7 +636,7 @@ var PkgPathDSL = func() {
 	})
 }
 
-var MultiplePkgPathDSL = func() {
+var PkgPathMultipleDSL = func() {
 	Service("MultiplePkgPathMethod", func() {
 		Method("A", func() {
 			Payload(Bar)
@@ -658,3 +658,26 @@ var PkgPathNoDirDSL = func() {
 		})
 	})
 }
+
+var PkgPathDupeDSL = func() {
+	Service("PkgPathDupeMethod", func() {
+		Method("A", func() {
+			Payload(Foo)
+			Result(Foo)
+		})
+		Method("B", func() {
+			Payload(Foo)
+			Result(Foo)
+		})
+	})
+	Service("PkgPathDupeMethod2", func() {
+		Method("A", func() {
+			Payload(Foo)
+			Result(Foo)
+		})
+		Method("B", func() {
+			Payload(Foo)
+			Result(Foo)
+		})
+	})
+}