fix: http server codegen (#3944)

* fix: http server codegen

Sibling fields in result types were sharing the same AttributeExpr pointer
during projection, causing metadata (descriptions and JSON tags) to leak
between them. Added field name to the `seen` key in `projectRecursive`
to be ensured each sibling gets its own isolated entry.

Signed-off-by: Adam Bocim <adam.bocim@seznam.cz>

* Memoize projected types rather than field attributes in view projection

The projection cache stored entire field attributes, so any two fields
hashing to the same (type, view) key aliased one AttributeExpr and
leaked per-field metadata (descriptions, struct tags) across fields.
Keying by field name only narrowed the collisions: same-named fields of
the same type under different parent types still aliased.

Cache the projected types instead and give every field its own
attribute wrapping the shared projection. Single result types register
their projection before computing their fields so that recursive
references resolve to the in-flight projection and terminate.

---------

Signed-off-by: Adam Bocim <adam.bocim@seznam.cz>
Co-authored-by: Raphael Simon <simon.raphael@gmail.com>

Author: abocim

expr/project_test.go

@@ -4,6 +4,9 @@ import (
 	"fmt"
 	"strings"
 	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 var (
@@ -27,8 +30,9 @@ var (
 	compositeResultDefault = resultType("a", object(collectionResultDefault), "b", String)
 	compositeResultLink    = resultType("a", object(collectionResultLink))
 
-	recursiveResult         = resultRecursive("a", String, view("default", "a", object(String)))
-	embeddedRecursiveResult = resultType("a", String, "rec", recursiveResult)
+	// recursiveResult is its own expected projection: projecting a recursive
+	// result type yields a single projected type that references itself.
+	recursiveResult = resultRecursive("a", String, view("default", "a", object(String)))
 )
 
 func init() {
@@ -50,7 +54,7 @@ func TestProject(t *testing.T) {
 		{"collection-link", collectionResult, "link", collectionResultLink},
 		{"composite-default", compositeResult, "default", compositeResultDefault},
 		{"composite-link", compositeResult, "link", compositeResultLink},
-		{"recursive", recursiveResult, "default", embeddedRecursiveResult},
+		{"recursive", recursiveResult, "default", recursiveResult},
 	}
 	for _, k := range cases {
 		t.Run(k.Name, func(t *testing.T) {
@@ -78,6 +82,73 @@ func TestProject(t *testing.T) {
 	}
 }
 
+// TestProjectDoesNotAliasFieldAttributes verifies that fields sharing a type
+// share the projected type but never the AttributeExpr wrapping it, so that
+// per-field metadata such as descriptions does not leak across fields.
+func TestProjectDoesNotAliasFieldAttributes(t *testing.T) {
+	t.Run("sibling user type fields", func(t *testing.T) {
+		shared := userType("Shared", object(Int))
+		rt := resultType("a", shared, "b", shared,
+			view("default", "a", object(Int), "b", object(Int)))
+
+		projected, err := Project(rt, "default")
+		require.NoError(t, err)
+
+		obj := AsObject(projected.Type)
+		a, b := obj.Attribute("a"), obj.Attribute("b")
+		assert.NotSame(t, a, b)
+		assert.Equal(t, "desc a", a.Description)
+		assert.Equal(t, "desc b", b.Description)
+		assert.Same(t, a.Type, b.Type)
+	})
+
+	t.Run("sibling result type fields", func(t *testing.T) {
+		rt := resultType("x", simpleResult, "y", simpleResult,
+			view("default", "x", AsObject(simpleResult), "y", AsObject(simpleResult)))
+
+		projected, err := Project(rt, "default")
+		require.NoError(t, err)
+
+		obj := AsObject(projected.Type)
+		x, y := obj.Attribute("x"), obj.Attribute("y")
+		assert.NotSame(t, x, y)
+		assert.Equal(t, "desc x", x.Description)
+		assert.Equal(t, "desc y", y.Description)
+		assert.Same(t, x.Type, y.Type)
+	})
+
+	t.Run("same field in different parent types", func(t *testing.T) {
+		shared := userType("Shared", object(Int))
+		wrapper := userType("Wrapper", &Object{
+			{Name: "a", Attribute: &AttributeExpr{Type: shared, Description: "Inner A"}},
+		})
+		rt := resultType("a", shared, "nested", wrapper,
+			view("default",
+				"a", object(Int),
+				"nested", &Object{{Name: "a", Attribute: &AttributeExpr{Type: object(Int)}}}))
+
+		projected, err := Project(rt, "default")
+		require.NoError(t, err)
+
+		obj := AsObject(projected.Type)
+		outer := obj.Attribute("a")
+		assert.Equal(t, "desc a", outer.Description)
+		inner := AsObject(obj.Attribute("nested").Type).Attribute("a")
+		assert.NotSame(t, outer, inner)
+		assert.Equal(t, "Inner A", inner.Description)
+		assert.Same(t, outer.Type, inner.Type)
+	})
+
+	t.Run("recursive result type references its own projection", func(t *testing.T) {
+		projected, err := Project(recursiveResult, "default")
+		require.NoError(t, err)
+
+		rec := AsObject(projected.Type).Attribute("rec")
+		assert.Equal(t, "desc rec", rec.Description)
+		assert.Same(t, projected, rec.Type)
+	})
+}
+
 // view is a helper function for building view expressions used in tests. name
 // is the name of the view, attributes list the names of the attributes rendered
 // by the view. name may use the format "name:view" in which case view is the

expr/result_type.go

@@ -232,22 +232,32 @@ func (rt *ResultTypeExpr) ensureDefaultView() {
 // individual attributes may use a different view. In this case Project uses
 // that view and returns an error if it isn't defined on the attribute type.
 func Project(rt *ResultTypeExpr, view string) (*ResultTypeExpr, error) {
-	return project(rt, view, make(map[string]*AttributeExpr))
+	return project(rt, view, make(map[string]UserType))
 }
 
-func project(rt *ResultTypeExpr, view string, seen map[string]*AttributeExpr) (*ResultTypeExpr, error) {
+// project computes the projection of rt for view. seen memoizes projected
+// types keyed by (type hash, view). It caches types only - never field
+// attributes - so that sibling fields referencing the same type each keep
+// their own AttributeExpr (description and meta would otherwise leak across
+// fields). projectSingle registers its projection before computing the fields
+// so that recursive references resolve to the in-flight projection and the
+// recursion terminates.
+func project(rt *ResultTypeExpr, view string, seen map[string]UserType) (*ResultTypeExpr, error) {
 	_, params, _ := mime.ParseMediaType(rt.Identifier)
 	if params["view"] == view {
 		// nothing to do
 		return rt, nil
 	}
+	if p, ok := seen[hashTypeAndView(rt, view)]; ok {
+		return p.(*ResultTypeExpr), nil
+	}
 	if _, ok := rt.Type.(*Array); ok {
 		return projectCollection(rt, view, seen)
 	}
 	return projectSingle(rt, view, seen)
 }
 
-func projectSingle(rt *ResultTypeExpr, view string, seen map[string]*AttributeExpr) (*ResultTypeExpr, error) {
+func projectSingle(rt *ResultTypeExpr, view string, seen map[string]UserType) (*ResultTypeExpr, error) {
 	v := rt.View(view)
 	if v == nil {
 		return nil, fmt.Errorf("unknown view %#v", view)
@@ -280,26 +290,15 @@ func projectSingle(rt *ResultTypeExpr, view string, seen map[string]*AttributeEx
 		typeName += Title(view)
 	}
 
-	var ut *UserTypeExpr
-	if att, ok := seen[hashAttrAndView(rt.Attribute(), view)]; ok {
-		if rt, ok2 := att.Type.(*ResultTypeExpr); ok2 {
-			ut = &UserTypeExpr{
-				AttributeExpr: DupAtt(rt.Attribute()),
-				TypeName:      rt.TypeName,
-			}
-		}
-	}
 	id := rt.projectIdentifier(view)
-	if ut == nil {
-		ut = &UserTypeExpr{
-			AttributeExpr: &AttributeExpr{
-				Description: desc,
-				Validation:  val,
-			},
-		}
+	ut := &UserTypeExpr{
+		AttributeExpr: &AttributeExpr{
+			Description: desc,
+			Validation:  val,
+		},
+		TypeName: typeName,
+		UID:      id,
 	}
-	ut.TypeName = typeName
-	ut.UID = id
 	ut.Type = Dup(v.Type)
 	ut.UserExamples = v.UserExamples
 	projected := &ResultTypeExpr{
@@ -312,6 +311,11 @@ func projectSingle(rt *ResultTypeExpr, view string, seen map[string]*AttributeEx
 		Parent:        projected,
 	}}
 
+	// Register the projection before computing the fields so that recursive
+	// references to rt resolve to the in-flight projection - its fields are
+	// filled in place below - and the recursion terminates.
+	seen[hashTypeAndView(rt, view)] = projected
+
 	projectedObj := projected.Type.(*Object)
 	mtObj := AsObject(rt.Type)
 	for _, nat := range *viewObj {
@@ -326,7 +330,7 @@ func projectSingle(rt *ResultTypeExpr, view string, seen map[string]*AttributeEx
 	return projected, nil
 }
 
-func projectCollection(rt *ResultTypeExpr, view string, seen map[string]*AttributeExpr) (*ResultTypeExpr, error) {
+func projectCollection(rt *ResultTypeExpr, view string, seen map[string]UserType) (*ResultTypeExpr, error) {
 	// Project the collection element result type
 	e := rt.Type.(*Array).ElemType.Type.(*ResultTypeExpr) // validation checked this cast would work
 	pe, err2 := project(e, view, seen)
@@ -359,13 +363,16 @@ func projectCollection(rt *ResultTypeExpr, view string, seen map[string]*Attribu
 		return nil, eval.Context.Errors
 	}
 
+	seen[hashTypeAndView(rt, view)] = proj
 	return proj, nil
 }
 
-func projectRecursive(at *AttributeExpr, vat *NamedAttributeExpr, view string, seen map[string]*AttributeExpr) (*AttributeExpr, error) {
-	if att, ok := seen[hashAttrAndView(at, view)]; ok {
-		return att, nil
-	}
+// projectRecursive computes the projected attribute for the field described
+// by at within a result type being projected with view. vat is the matching
+// view attribute. It always returns a fresh attribute: projected types are
+// shared through seen but the attributes wrapping them never are, so that
+// per-field metadata does not leak across fields of the same type.
+func projectRecursive(at *AttributeExpr, vat *NamedAttributeExpr, view string, seen map[string]UserType) (*AttributeExpr, error) {
 	at = DupAtt(at)
 
 	if rt, ok := at.Type.(*ResultTypeExpr); ok {
@@ -378,7 +385,6 @@ func projectRecursive(at *AttributeExpr, vat *NamedAttributeExpr, view string, s
 				view = DefaultView
 			}
 		}
-		seen[hashAttrAndView(at, view)] = at
 		pr, err := project(rt, view, seen)
 		if err != nil {
 			return nil, fmt.Errorf("view %#v on field %#v cannot be computed: %w", view, vat.Name, err)
@@ -387,8 +393,15 @@ func projectRecursive(at *AttributeExpr, vat *NamedAttributeExpr, view string, s
 		return at, nil
 	}
 
-	if _, ok := at.Type.(*UserTypeExpr); ok {
-		seen[hashAttrAndView(at, view)] = at
+	if ut, ok := at.Type.(*UserTypeExpr); ok {
+		key := hashTypeAndView(ut, view)
+		if p, ok := seen[key]; ok {
+			at.Type = p
+			return at, nil
+		}
+		// Register before recursing into the fields below (they are projected
+		// in place) so that recursive user types terminate.
+		seen[key] = ut
 	}
 
 	if obj := AsObject(at.Type); obj != nil {
@@ -457,8 +470,8 @@ func (v *ViewExpr) EvalName() string {
 	return prefix + suffix
 }
 
-// hashAttrAndView computes a hash for an attribute and a view that returns the
-// same value for two attributes and views that produce the same projected type.
-func hashAttrAndView(att *AttributeExpr, view string) string {
-	return Hash(att.Type, false, false, false) + "::" + view
+// hashTypeAndView computes the projection cache key for the given type and
+// view. Two types with the same key project to the same type for the view.
+func hashTypeAndView(t DataType, view string) string {
+	return Hash(t, false, false, false) + "::" + view
 }

http/codegen/server_types_test.go

@@ -25,6 +25,9 @@ func TestServerTypes(t *testing.T) {
 		{"server-result-type-validate", testdata.ResultTypeValidateDSL},
 		{"server-with-result-collection", testdata.ResultWithResultCollectionDSL},
 		{"server-with-result-view", testdata.ResultWithResultViewDSL},
+		{"server-with-result-sibling-user-type-fields", testdata.ResultTypeSiblingUserTypeFieldsDSL},
+		{"server-with-result-collection-sibling-user-type-fields", testdata.ResultTypeCollectionSiblingUserTypeFieldsDSL},
+		{"server-with-result-nested-user-type-fields", testdata.ResultTypeNestedUserTypeFieldsDSL},
 		{"server-empty-error-response-body", testdata.EmptyErrorResponseBodyDSL},
 		{"server-with-error-custom-pkg", testdata.WithErrorCustomPkgDSL},
 		{"server-body-custom-name", testdata.PayloadBodyCustomNameDSL},

http/codegen/testdata/golden/server_types_server-with-result-collection-sibling-user-type-fields.go.golden

@@ -0,0 +1,33 @@
+// ResulttypesiblingcollectionResponseCollection is the type of the
+// "ServiceResultCollectionUserTypeSibling" service
+// "MethodResultCollectionUserTypeSibling" endpoint HTTP response body.
+type ResulttypesiblingcollectionResponseCollection []*ResulttypesiblingcollectionResponse
+
+// ResulttypesiblingcollectionResponse is used to define fields on response
+// body types.
+type ResulttypesiblingcollectionResponse struct {
+	// Attribute A
+	A *UserTypeResponse `json:"a"`
+	// Attribute B
+	B *UserTypeResponse `json:"b"`
+}
+
+// UserTypeResponse is used to define fields on response body types.
+type UserTypeResponse struct {
+	U *int `form:"u,omitempty" json:"u,omitempty" xml:"u,omitempty"`
+}
+
+// NewResulttypesiblingcollectionResponseCollection builds the HTTP response
+// body from the result of the "MethodResultCollectionUserTypeSibling" endpoint
+// of the "ServiceResultCollectionUserTypeSibling" service.
+func NewResulttypesiblingcollectionResponseCollection(res serviceresultcollectionusertypesiblingviews.ResulttypesiblingcollectionCollectionView) ResulttypesiblingcollectionResponseCollection {
+	body := make([]*ResulttypesiblingcollectionResponse, len(res))
+	for i, val := range res {
+		if val == nil {
+			body[i] = nil
+			continue
+		}
+		body[i] = marshalServiceresultcollectionusertypesiblingviewsResulttypesiblingcollectionViewToResulttypesiblingcollectionResponse(val)
+	}
+	return body
+}

http/codegen/testdata/golden/server_types_server-with-result-nested-user-type-fields.go.golden

@@ -0,0 +1,33 @@
+// MethodResultUserTypeNestedResponseBody is the type of the
+// "ServiceResultUserTypeNested" service "MethodResultUserTypeNested" endpoint
+// HTTP response body.
+type MethodResultUserTypeNestedResponseBody struct {
+	// Outer A
+	A      *UserTypeResponseBody `json:"outer_a"`
+	Nested *WrapperResponseBody  `form:"nested,omitempty" json:"nested,omitempty" xml:"nested,omitempty"`
+}
+
+// UserTypeResponseBody is used to define fields on response body types.
+type UserTypeResponseBody struct {
+	U *int `form:"u,omitempty" json:"u,omitempty" xml:"u,omitempty"`
+}
+
+// WrapperResponseBody is used to define fields on response body types.
+type WrapperResponseBody struct {
+	// Inner A
+	A *UserTypeResponseBody `json:"inner_a"`
+}
+
+// NewMethodResultUserTypeNestedResponseBody builds the HTTP response body from
+// the result of the "MethodResultUserTypeNested" endpoint of the
+// "ServiceResultUserTypeNested" service.
+func NewMethodResultUserTypeNestedResponseBody(res *serviceresultusertypenestedviews.ResulttypenestedView) *MethodResultUserTypeNestedResponseBody {
+	body := &MethodResultUserTypeNestedResponseBody{}
+	if res.A != nil {
+		body.A = marshalServiceresultusertypenestedviewsUserTypeViewToUserTypeResponseBody(res.A)
+	}
+	if res.Nested != nil {
+		body.Nested = marshalServiceresultusertypenestedviewsWrapperViewToWrapperResponseBody(res.Nested)
+	}
+	return body
+}

http/codegen/testdata/golden/server_types_server-with-result-sibling-user-type-fields.go.golden

@@ -0,0 +1,28 @@
+// MethodResultUserTypeSiblingResponseBody is the type of the
+// "ServiceResultUserTypeSibling" service "MethodResultUserTypeSibling"
+// endpoint HTTP response body.
+type MethodResultUserTypeSiblingResponseBody struct {
+	// Attribute A
+	A *UserTypeResponseBody `json:"a"`
+	// Attribute B
+	B *UserTypeResponseBody `json:"b"`
+}
+
+// UserTypeResponseBody is used to define fields on response body types.
+type UserTypeResponseBody struct {
+	U *int `form:"u,omitempty" json:"u,omitempty" xml:"u,omitempty"`
+}
+
+// NewMethodResultUserTypeSiblingResponseBody builds the HTTP response body
+// from the result of the "MethodResultUserTypeSibling" endpoint of the
+// "ServiceResultUserTypeSibling" service.
+func NewMethodResultUserTypeSiblingResponseBody(res *serviceresultusertypesiblingviews.ResulttypesiblingView) *MethodResultUserTypeSiblingResponseBody {
+	body := &MethodResultUserTypeSiblingResponseBody{}
+	if res.A != nil {
+		body.A = marshalServiceresultusertypesiblingviewsUserTypeViewToUserTypeResponseBody(res.A)
+	}
+	if res.B != nil {
+		body.B = marshalServiceresultusertypesiblingviewsUserTypeViewToUserTypeResponseBody(res.B)
+	}
+	return body
+}