codegen: avoid name collisions for wrapped object payloads (#3907)

Service codegen wraps raw object payloads/results in synthetic user types named
from the method (e.g. Foo -> FooPayload). When NameScope counters have already
reserved FooPayload and FooPayload2, using NameScope.Name can emit a colliding
identifier and produce invalid generated code.

Introduce NameScope.PeekUnique to compute the Unique result without mutating the
scope and use it when synthesizing wrapper type names. Add a service golden that
locks the collision case. Also fix generated error name docs to reflect the
returned value.

Author: raphael

codegen/scope.go

@@ -76,6 +76,32 @@ func (s *NameScope) Unique(name string, suffix ...string) string {
 	}
 }
 
+// PeekUnique returns the name that Unique would return for the same inputs,
+// without mutating the scope.
+//
+// This is useful when synthesizing type names or identifiers that are later
+// reserved via Hash-based naming (e.g., GoTypeName/HashedUnique) and therefore
+// must not increment the scope counters twice.
+func (s *NameScope) PeekUnique(name string, suffix ...string) string {
+	c, ok := s.counts[name]
+	if !ok {
+		return name
+	}
+	if len(suffix) > 0 {
+		name += suffix[0]
+		c, ok = s.counts[name]
+		if !ok {
+			return name
+		}
+	}
+	for i := c; ; i++ {
+		ret := name + strconv.Itoa(i+1)
+		if _, ok := s.counts[ret]; !ok {
+			return ret
+		}
+	}
+}
+
 // Name returns a unique name for the given name by adding a counter value to
 // the name until unique. It returns the same value when called multiple times
 // for the same given name.

codegen/scope_test.go

@@ -65,3 +65,34 @@ func TestNameScope_GoFullTypeName_UsesScopedNameWhenQualified(t *testing.T) {
 		t.Fatalf("expected qualified base name %q with fresh scope, got %q", want, got)
 	}
 }
+
+func TestNameScope_PeekUnique_MatchesUniqueWithoutMutation(t *testing.T) {
+	seed := func(scope *NameScope) {
+		scope.Unique("a")
+		scope.Unique("a")
+		scope.Unique("a2")
+		scope.Unique("hello")
+		scope.Unique("hello1")
+	}
+
+	peek := NewNameScope()
+	seed(peek)
+
+	mutating := NewNameScope()
+	seed(mutating)
+
+	if got, want := peek.PeekUnique("a"), mutating.Unique("a"); got != want {
+		t.Fatalf("expected peek %q, got %q", want, got)
+	}
+	if got, want := peek.PeekUnique("hel", "lo"), mutating.Unique("hel", "lo"); got != want {
+		t.Fatalf("expected peek %q, got %q", want, got)
+	}
+	if got, want := peek.PeekUnique("hello", "1"), mutating.Unique("hello", "1"); got != want {
+		t.Fatalf("expected peek %q, got %q", want, got)
+	}
+
+	// PeekUnique must not mutate the scope.
+	if got, want := peek.Unique("a"), "a3"; got != want {
+		t.Fatalf("expected scope unchanged, got %q", got)
+	}
+}

codegen/service/service_data.go

@@ -804,7 +804,7 @@ func (d *ServicesData) analyze(service *expr.ServiceExpr) *Data {
 		if _, ok := att.Type.(*expr.Object); ok {
 			att.Type = &expr.UserTypeExpr{
 				AttributeExpr: expr.DupAtt(att),
-				TypeName:      scope.Name(name),
+				TypeName:      scope.PeekUnique(name),
 				UID:           id,
 			}
 		}

codegen/service/service_test.go

@@ -58,6 +58,7 @@ func TestService(t *testing.T) {
 		{"service-bidirectional-streaming-result-with-explicit-view", testdata.BidirectionalStreamingResultWithExplicitViewMethodDSL},
 		{"service-multiple-api-key-security", testdata.MultipleAPIKeySecurityDSL},
 		{"service-mixed-and-multiple-api-key-security", testdata.MixedAndMultipleAPIKeySecurityDSL},
+		{"service-raw-object-payload-type-name-collision", testdata.RawObjectPayloadTypeNameCollisionDSL},
 	}
 	for _, c := range cases {
 		t.Run(c.Name, func(t *testing.T) {

codegen/service/templates/error.go.tpl

@@ -3,14 +3,14 @@ func (e {{ .Ref }}) Error() string {
 	return {{ printf "%q" .Description }}
 }
 
-// ErrorName returns {{ printf "%q" .Name }}.
+// ErrorName returns the error name.
 //
 // Deprecated: Use GoaErrorName - https://github.com/goadesign/goa/issues/3105
 func (e {{ .Ref }}) ErrorName() string {
 	return e.GoaErrorName()
 }
 
-// GoaErrorName returns {{ printf "%q" .Name }}.
+// GoaErrorName returns the error name.
 func (e {{ .Ref }}) GoaErrorName() string {
 	return {{ errorName . }}
 }

codegen/service/testdata/golden/service_service-custom-errors-custom-field.go.golden

@@ -30,14 +30,14 @@ func (e *GoaError) Error() string {
 	return ""
 }
 
-// ErrorName returns "GoaError".
+// ErrorName returns the error name.
 //
 // Deprecated: Use GoaErrorName - https://github.com/goadesign/goa/issues/3105
 func (e *GoaError) ErrorName() string {
 	return e.GoaErrorName()
 }
 
-// GoaErrorName returns "GoaError".
+// GoaErrorName returns the error name.
 func (e *GoaError) GoaErrorName() string {
 	return e.ErrorCode
 }

codegen/service/testdata/golden/service_service-custom-errors.go.golden

@@ -42,14 +42,14 @@ func (e *APayload) Error() string {
 	return ""
 }
 
-// ErrorName returns "APayload".
+// ErrorName returns the error name.
 //
 // Deprecated: Use GoaErrorName - https://github.com/goadesign/goa/issues/3105
 func (e *APayload) ErrorName() string {
 	return e.GoaErrorName()
 }
 
-// GoaErrorName returns "APayload".
+// GoaErrorName returns the error name.
 func (e *APayload) GoaErrorName() string {
 	return "user_type"
 }
@@ -59,14 +59,14 @@ func (e *Result) Error() string {
 	return ""
 }
 
-// ErrorName returns "Result".
+// ErrorName returns the error name.
 //
 // Deprecated: Use GoaErrorName - https://github.com/goadesign/goa/issues/3105
 func (e *Result) ErrorName() string {
 	return e.GoaErrorName()
 }
 
-// GoaErrorName returns "Result".
+// GoaErrorName returns the error name.
 func (e *Result) GoaErrorName() string {
 	return e.B
 }
@@ -76,14 +76,14 @@ func (e Primitive) Error() string {
 	return "primitive error description"
 }
 
-// ErrorName returns "primitive".
+// ErrorName returns the error name.
 //
 // Deprecated: Use GoaErrorName - https://github.com/goadesign/goa/issues/3105
 func (e Primitive) ErrorName() string {
 	return e.GoaErrorName()
 }
 
-// GoaErrorName returns "primitive".
+// GoaErrorName returns the error name.
 func (e Primitive) GoaErrorName() string {
 	return "primitive"
 }

codegen/service/testdata/golden/service_service-mixed-results.go.golden

@@ -57,4 +57,4 @@ type Payload struct {
 // MixedResultsMethod method.
 type ResultType struct {
 	OK bool
-}
+}

codegen/service/testdata/golden/service_service-raw-object-payload-type-name-collision.go.golden

@@ -0,0 +1,70 @@
+
+// Service is the RawObjectPayloadTypeNameCollision service interface.
+type Service interface {
+	// Foo implements Foo.
+	Foo(context.Context, *FooPayload3) (err error)
+}
+
+// APIName is the name of the API as defined in the design.
+const APIName = "test api"
+
+// APIVersion is the version of the API as defined in the design.
+const APIVersion = "0.0.1"
+
+// 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 = "RawObjectPayloadTypeNameCollision"
+
+// 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 = [1]string{"Foo"}
+
+type FooPayload struct {
+	X *string
+}
+
+type FooPayload2 struct {
+	Y *string
+}
+
+// FooPayload3 is the payload type of the RawObjectPayloadTypeNameCollision
+// service Foo method.
+type FooPayload3 struct {
+	A string
+}
+
+// Error returns an error description.
+func (e *FooPayload) Error() string {
+	return ""
+}
+
+// ErrorName returns the error name.
+//
+// Deprecated: Use GoaErrorName - https://github.com/goadesign/goa/issues/3105
+func (e *FooPayload) ErrorName() string {
+	return e.GoaErrorName()
+}
+
+// GoaErrorName returns the error name.
+func (e *FooPayload) GoaErrorName() string {
+	return "reserve_foo_payload"
+}
+
+// Error returns an error description.
+func (e *FooPayload2) Error() string {
+	return ""
+}
+
+// ErrorName returns the error name.
+//
+// Deprecated: Use GoaErrorName - https://github.com/goadesign/goa/issues/3105
+func (e *FooPayload2) ErrorName() string {
+	return e.GoaErrorName()
+}
+
+// GoaErrorName returns the error name.
+func (e *FooPayload2) GoaErrorName() string {
+	return "reserve_foo_payload2"
+}

codegen/service/testdata/service_dsls.go

@@ -1100,6 +1100,36 @@ var MixedAndMultipleAPIKeySecurityDSL = func() {
 	})
 }
 
+// RawObjectPayloadTypeNameCollisionDSL exercises the naming of synthetic user
+// types created when wrapping raw object payloads.
+//
+// The service generator wraps raw object payloads in a synthetic user type
+// named after the method (e.g. Foo -> FooPayload). That name must be computed
+// using NameScope uniqueness rules even when a would-be suffix (FooPayload2) is
+// already taken, otherwise codegen can emit invalid references.
+var RawObjectPayloadTypeNameCollisionDSL = func() {
+	var FooPayload = Type("FooPayload", func() {
+		Attribute("x", String)
+	})
+	var FooPayload2 = Type("FooPayload2", func() {
+		Attribute("y", String)
+	})
+
+	Service("RawObjectPayloadTypeNameCollision", func() {
+		// Reserve FooPayload and FooPayload2 in the NameScope *before* raw-object
+		// payload wrapping occurs (wrapObject runs before method data is built).
+		Error("reserve_foo_payload", FooPayload)
+		Error("reserve_foo_payload2", FooPayload2)
+
+		Method("Foo", func() {
+			Payload(func() {
+				Attribute("a", String)
+				Required("a")
+			})
+		})
+	})
+}
+
 // UnionCustomKeysDSL tests union with custom type and value keys via Meta tags.
 var UnionCustomKeysDSL = func() {
 	var CustomUnion = Type("CustomUnion", func() {