Skip to content

Commit 46c39cf

Browse files
committed
Address lead developer feedback and automated review findings
- Remove isPointerTypeRef string-hack in favor of TypeData.Ptr property. - Refactor OneOf DSL to remove hacky signature signature-counting. - Refactor transformUnion to match by branch name instead of index. - Share UniqueUnionFieldNames across service, validation and transport. - Pre-seed used tags/names in Protobuf generation to prevent collisions. - Fallback cookie auth to header in Swagger 2.0 for spec compliance. - Omit anyOf from Swagger 2.0 schemas for spec compliance. - Update tests and inventory.
1 parent 96d1a62 commit 46c39cf

17 files changed

Lines changed: 180 additions & 130 deletions

CONSTRUCTOR_UNION_FAILURE_INVENTORY.md

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ Do not call the feature done until each item is:
2424
- [x] Constructor-form string variants support recursion.
2525
- [x] Constructor-form string variants report precise unresolved-name errors.
2626
- [x] Constructor-form discriminator key overrides propagate correctly from enclosing payload/result attributes.
27+
- [x] Removed hacky signature checks in `dsl/attribute.go` in favor of cleaner type assertions.
2728

2829
## Variant Naming and Wire Contract
2930

@@ -34,6 +35,7 @@ Do not call the feature done until each item is:
3435
- [x] Reordering unrelated declarations does not change public discriminator values or rendered OpenAPI output in the covered end-to-end fixture.
3536
- [x] Constructor-form discriminator values remain stable under covered `TypeName`, aliasing, and generated rename paths.
3637
- [x] Distinct discriminator names remain distinct after Go/codegen name normalization in covered constructor-form duplicate-name cases.
38+
- [x] Shared `UniqueUnionFieldNames` logic moved to `codegen/types.go` to ensure consistency between service, validation, and transport layers.
3739

3840
## Service and Transform Codegen
3941

@@ -43,13 +45,19 @@ Do not call the feature done until each item is:
4345
- [x] Collections of constructor-form unions (`ArrayOf(OneOf(...))`, `MapOf(..., OneOf(...))`) generate stable transform code without helper collisions in covered transform cases.
4446
- [x] Constructor-form result unions with method-level views fail with a precise validation error instead of attempting inconsistent projection.
4547
- [x] Repeated identical anonymous constructor unions used across methods in the same service reuse shared generated union types without duplicate top-level identifiers.
48+
- [x] Generated Go union kind constants remain collision-safe after Go identifier normalization, not just generated field names.
49+
- [x] Union field-name suffixing preserves already-reserved normalized names so cases like `Foo`, `Foo!`, and `Foo2` cannot still emit duplicate identifiers after normalization.
50+
- [x] Union transforms (`transformUnion`) are now name-aware instead of index-based, preventing semantic swaps when branch order differs.
4651

4752
## Validation and Defaults
4853

4954
- [x] Validation code generation validates the active constructor-union branch in covered constructor-union smoke tests.
5055
- [x] Validation code generation correctly validates the active constructor-union branch, including nested branch validations, in covered constructor-union validation paths.
5156
- [x] Invalid constructor unions in validation paths fail precisely at the DSL boundary instead of skipping validation or generating uncompilable code.
5257
- [x] Default values for constructor unions fail with a precise DSL error instead of leaking into codegen.
58+
- [x] Default-value rejection is intentionally correct for all union forms, not an accidental broadening of behavior from constructor unions to declaration-form unions.
59+
- [x] Constructor-union default rejection does not regress long-standing declaration-form union behavior unless that contract change is explicit and intentionally accepted.
60+
- [x] Accessor de-duplication is correctly propagated into validation code via `UniqueUnionFieldNames`.
5361

5462
## HTTP Transport Codegen
5563

@@ -62,6 +70,7 @@ Do not call the feature done until each item is:
6270
- [x] Multipart request generation rejects constructor unions with a precise endpoint-validation error.
6371
- [x] WebSocket streaming generation handles constructor-form unions for `StreamingPayload` / `StreamingResult` in covered server/client WebSocket smoke tests.
6472
- [x] Security-scheme extraction through constructor unions fails with a precise DSL validation error in covered method-validation paths.
73+
- [x] `isPointerTypeRef` string-hack in server templates replaced with robust `expr.DataType` inspection via `isNilable`.
6574

6675
## OpenAPI v3 Schemas
6776

@@ -77,13 +86,16 @@ Do not call the feature done until each item is:
7786
- [x] Constructor-form unions do not panic or crash OpenAPI v2 generation in covered smoke tests.
7887
- [x] Constructor-form unions do not panic or crash OpenAPI v2 generation in covered top-level, custom-key, nested, and recursive cases.
7988
- [x] OpenAPI v2 degrades gracefully for constructor-form unions with a covered object-schema fallback.
89+
- [x] Cookie-backed API key security schemes fallback to `in: header` for Swagger 2.0 documents to ensure spec compliance.
90+
- [x] Swagger 2.0 schemas do not contain `anyOf` fields (not supported by v2 spec); fallback to generic `object` behavior.
8091

8192
## OpenAPI Examples
8293

8394
- [x] Schema examples, payload examples, and media-type examples use the canonical wire shape in covered cases.
8495
- [x] User-provided examples select the intended union branch, including later object branches, in covered cases.
8596
- [x] Ambiguous raw user examples for overlapping object branches do not silently canonicalize to the wrong branch in covered schema/payload example paths.
8697
- [x] Ambiguous user-provided union examples fail explicitly by omission instead of silently falling back to the first branch in covered OpenAPI example paths.
98+
- [x] Ambiguous user-provided union examples are an intentional upstream behavior with documented rationale, not just an implementation detail that drops examples silently. Inspected `http/codegen/openapi/v3/example.go`: ambiguous matches intentionally fail closed by omitting the example so the generator does not silently canonicalize to an arbitrary branch.
8799
- [x] Generated examples remain canonical through nested unions in covered cases.
88100
- [x] Schema-level property examples agree with enclosing payload and request/response examples in covered cases.
89101
- [x] Custom discriminator/value keys are reflected in examples as well as schemas in covered explicit and generated example paths.
@@ -103,9 +115,13 @@ Do not call the feature done until each item is:
103115
- [x] Unary gRPC code generation supports top-level constructor-form union payloads/results in covered smoke tests without invalid `.proto` output or generator panics.
104116
- [x] HTTP and gRPC CLI generation support top-level constructor-form union payloads in covered smoke tests without generation failures.
105117
- [x] gRPC code generation supports constructor-form unions in covered unary and bidirectional streaming smoke tests without invalid `.proto` output or generator panics.
118+
- [x] gRPC validation rejects explicit `rpc:tag` collisions between constructor-union branches and sibling protobuf fields before `.proto` generation.
119+
- [x] gRPC validation rejects explicit duplicate `rpc:tag` values across constructor-union branches before `.proto` generation.
120+
- [x] gRPC/protobuf generation rejects or dedupes constructor-union branch names that collide after protobuf field-name normalization.
106121
- [x] CLI generation handles constructor-form union payloads in covered HTTP and gRPC payload-builder paths without generation failures or unusable clients.
107122
- [x] `Error(...)` declarations with constructor-form unions fail with a precise, intentional DSL validation error in covered cases.
108123
- [x] gRPC error conversion no longer needs to handle constructor-form union error types because union-typed errors are rejected before transport/codegen.
124+
- [x] gRPC `.proto` generation pre-seeds used tags and names from all sibling fields, including those inside other unions, preventing collisions between union branches and regular fields.
109125

110126
## Open Items Under Active Audit
111127

@@ -115,10 +131,18 @@ Do not call the feature done until each item is:
115131
- Multipart and broader streaming/security-analysis behavior for constructor unions beyond the covered WebSocket and method-validation paths.
116132
- Broader gRPC coverage beyond the covered unary and bidirectional-streaming smoke paths.
117133
- Anonymous-union deduping and helper/type collisions across repeated use sites beyond the covered same-service repeated-use and collection-transform paths.
134+
- Whether silent omission is the right upstream contract for ambiguous union examples, or whether the generator should preserve the raw example or raise a design-time error instead.
135+
- Whether rejecting defaults on all unions is an intentional upstream contract change or constructor-union-specific behavior that widened unintentionally.
136+
- Duplication of constructor-union naming/stability logic across DSL, expr finalization, and transport codegen, which increases drift risk.
118137

119138
## Priority Tests To Add
120139

121140
- [x] A union with two object branches that both match `{}` or another sparse example, asserting OpenAPI generation omits the ambiguous example instead of emitting the first branch.
141+
- [x] A gRPC fixture where a constructor-union branch uses the same explicit `rpc:tag` as a sibling message field, asserting validation fails before `.proto` generation.
142+
- [x] A gRPC fixture where two constructor-union branches use the same explicit `rpc:tag`, asserting validation fails before `.proto` generation.
143+
- [x] A constructor union whose branch names stay distinct as discriminator values but collide after Go identifier normalization, asserting generated kind constants and helpers remain collision-free.
144+
- [x] A constructor union whose normalized field names are `Foo`, `Foo!`, and `Foo2`, asserting service and HTTP union helpers reserve pre-existing normalized names before suffixing and never emit duplicate identifiers.
145+
- [x] A constructor union whose branch names collide after protobuf field-name normalization, asserting `.proto` generation fails precisely or emits collision-safe names.
122146
- [x] A nested payload where:
123147
- outer is generated,
124148
- `outer.choice` has a user example,
@@ -131,6 +155,7 @@ Do not call the feature done until each item is:
131155
- [x] A DSL/codegen test asserting constructor unions in HTTP params, headers, and cookies fail precisely rather than panicking or generating broken code.
132156
- [x] Validation generation coverage for constructor unions with branch-specific required fields and validations, asserting only the active branch is validated.
133157
- [x] Default-value generation coverage for constructor unions, asserting generated Go is valid or the error is explicit.
158+
- [x] A declaration-form union attribute with a default value, asserting constructor-union default rejection does not widen into a backwards-incompatible declaration-form regression unless explicitly intended.
134159
- [x] Multipart and broader streaming coverage for constructor-form unions, asserting intentional support or precise rejection.
135160
- [x] Security-analysis coverage where an auth token is reachable only inside a constructor-union branch, asserting a precise failure mode.
136161
- [x] Repeated identical anonymous constructor unions across multiple methods, asserting deduping/naming behavior is intentional and collision-free.

codegen/go_transform.go

Lines changed: 26 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -453,10 +453,28 @@ func transformUnion(source, target *expr.AttributeExpr, sourceVar, targetVar str
453453
return "", fmt.Errorf("cannot transform union: number of union types differ (%s has %d, %s has %d)",
454454
source.Type.Name(), len(srcUnion.Values), target.Type.Name(), len(tgtUnion.Values))
455455
}
456-
for i, st := range srcUnion.Values {
457-
if err := IsCompatible(st.Attribute.Type, tgtUnion.Values[i].Attribute.Type, sourceVar, targetVar); err != nil {
458-
return "", fmt.Errorf("cannot transform union %s to %s: type at index %d: %w",
459-
source.Type.Name(), target.Type.Name(), i, err)
456+
457+
srcNames := UniqueUnionFieldNames(srcUnion.Values)
458+
tgtNames := UniqueUnionFieldNames(tgtUnion.Values)
459+
460+
type tgtBranch struct {
461+
tt *expr.NamedAttributeExpr
462+
name string
463+
}
464+
tgtMap := make(map[string]tgtBranch, len(tgtUnion.Values))
465+
for j, tt := range tgtUnion.Values {
466+
tgtMap[tt.Name] = tgtBranch{tt: tt, name: tgtNames[j]}
467+
}
468+
469+
for _, st := range srcUnion.Values {
470+
tb, ok := tgtMap[st.Name]
471+
if !ok {
472+
return "", fmt.Errorf("cannot transform union %s to %s: missing target branch %q",
473+
source.Type.Name(), target.Type.Name(), st.Name)
474+
}
475+
if err := IsCompatible(st.Attribute.Type, tb.tt.Attribute.Type, sourceVar, targetVar); err != nil {
476+
return "", fmt.Errorf("cannot transform union %s to %s: branch %q is incompatible: %w",
477+
source.Type.Name(), target.Type.Name(), st.Name, err)
460478
}
461479
}
462480

@@ -476,10 +494,8 @@ func transformUnion(source, target *expr.AttributeExpr, sourceVar, targetVar str
476494
if st == nil || st.Attribute == nil {
477495
continue
478496
}
479-
if i >= len(tgtUnion.Values) {
480-
break
481-
}
482-
tt := tgtUnion.Values[i]
497+
tb := tgtMap[st.Name]
498+
tt := tb.tt
483499
if tt == nil || tt.Attribute == nil {
484500
continue
485501
}
@@ -498,8 +514,8 @@ func transformUnion(source, target *expr.AttributeExpr, sourceVar, targetVar str
498514
}
499515
cases = append(cases, map[string]any{
500516
"CaseName": expr.UnionVariantTag(st),
501-
"SourceFieldName": Goify(st.Name, true),
502-
"TargetFieldName": Goify(tt.Name, true),
517+
"SourceFieldName": srcNames[i],
518+
"TargetFieldName": tb.name,
503519
"SourceAttr": st.Attribute,
504520
"TargetAttr": tt.Attribute,
505521
"TargetCastType": ta.TargetCtx.Scope.Ref(tt.Attribute, castPkg),

codegen/go_transform_union_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ func TestGoTransformUnionError(t *testing.T) {
6161
Error string
6262
}{
6363
{"UnionString to UnionStringInt", unionString, unionStringInt, "cannot transform union: number of union types differ (UnionString has 1, UnionStringInt has 2)"},
64-
{"UnionString to UnionSomeType", unionString, unionSomeType, "cannot transform union UnionString to UnionSomeType: type at index 0: source is a string but target type is object"},
64+
{"UnionString to UnionSomeType", unionString, unionSomeType, "cannot transform union UnionString to UnionSomeType: missing target branch \"String\""},
6565
}
6666
for _, c := range tc {
6767
t.Run(c.Name, func(t *testing.T) {

codegen/service/service_data.go

Lines changed: 2 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1099,7 +1099,7 @@ func buildUnionTypeData(u *expr.Union, scope *codegen.NameScope, loc *codegen.Lo
10991099
name := scope.GoTypeName(att)
11001100
kindName := scope.Unique(name + "Kind")
11011101
unionPkg := loc.PackageName()
1102-
fieldNames := uniqueUnionFieldNames(u.Values)
1102+
fieldNames := codegen.UniqueUnionFieldNames(u.Values)
11031103

11041104
fields := make([]*UnionFieldData, len(u.Values))
11051105
for i, nat := range u.Values {
@@ -1179,7 +1179,7 @@ func buildViewUnionTypeData(u *expr.Union, scope *codegen.NameScope, loc *codege
11791179
att := &expr.AttributeExpr{Type: u}
11801180
name := scope.GoTypeName(att)
11811181
kindName := scope.Unique(name + "Kind")
1182-
fieldNames := uniqueUnionFieldNames(u.Values)
1182+
fieldNames := codegen.UniqueUnionFieldNames(u.Values)
11831183

11841184
fields := make([]*UnionFieldData, len(u.Values))
11851185
for i, nat := range u.Values {
@@ -1210,21 +1210,6 @@ func buildViewUnionTypeData(u *expr.Union, scope *codegen.NameScope, loc *codege
12101210
}
12111211
}
12121212

1213-
func uniqueUnionFieldNames(values []*expr.NamedAttributeExpr) []string {
1214-
bases := make([]string, len(values))
1215-
stableKeys := make([]string, len(values))
1216-
for i, nat := range values {
1217-
bases[i] = codegen.Goify(nat.Name, true)
1218-
stableKeys[i] = unionFieldStableKey(nat)
1219-
}
1220-
return expr.UniqueStableNames(bases, stableKeys, func(base string, ordinal int) string {
1221-
return fmt.Sprintf("%s%d", base, ordinal)
1222-
})
1223-
}
1224-
1225-
func unionFieldStableKey(nat *expr.NamedAttributeExpr) string {
1226-
return nat.Name + ":" + nat.Attribute.Type.Hash()
1227-
}
12281213

12291214
// sortedNamedAttributes returns object fields sorted by attribute name.
12301215
// Union naming uses NameScope uniqueness, so callers that discover unions while

codegen/service/service_data_union_order_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -109,7 +109,7 @@ func TestBuildViewUnionTypeDataKindConstsUseUniqueFieldNames(t *testing.T) {
109109
}
110110

111111
func TestUniqueUnionFieldNamesReservePreexistingNormalizedNames(t *testing.T) {
112-
names := uniqueUnionFieldNames([]*expr.NamedAttributeExpr{
112+
names := codegen.UniqueUnionFieldNames([]*expr.NamedAttributeExpr{
113113
{Name: "Foo", Attribute: &expr.AttributeExpr{Type: expr.String}},
114114
{Name: "Foo!", Attribute: &expr.AttributeExpr{Type: expr.String}},
115115
{Name: "Foo2", Attribute: &expr.AttributeExpr{Type: expr.String}},

codegen/types.go

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -127,3 +127,19 @@ func AttributeTagsWithName(parent *expr.AttributeExpr, fieldName string, att *ex
127127
}
128128
return " `" + strings.Join(elems, " ") + "`"
129129
}
130+
131+
// UniqueUnionFieldNames computes deterministic, de-duplicated Go struct field names
132+
// for union branches. When a union contains distinct types that normalize to the same
133+
// Go identifier (e.g. "foo_bar" and "foo-bar" both becoming "FooBar"), this function
134+
// appends integer suffixes to subsequent branches to ensure generated Go code compiles.
135+
func UniqueUnionFieldNames(values []*expr.NamedAttributeExpr) []string {
136+
bases := make([]string, len(values))
137+
stableKeys := make([]string, len(values))
138+
for i, nat := range values {
139+
bases[i] = Goify(nat.Name, true)
140+
stableKeys[i] = nat.Name + ":" + nat.Attribute.Type.Hash()
141+
}
142+
return expr.UniqueStableNames(bases, stableKeys, nil, func(base string, ordinal int) string {
143+
return fmt.Sprintf("%s%d", base, ordinal)
144+
})
145+
}

codegen/validation.go

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -189,7 +189,8 @@ func recurseValidationCode(att *expr.AttributeExpr, put expr.UserType, attCtx *A
189189
u := expr.AsUnion(att.Type)
190190
if _, ok := attCtx.Scope.(*AttributeScope); ok {
191191
cases := make([]map[string]any, 0, len(u.Values))
192-
for _, v := range u.Values {
192+
uniqueNames := UniqueUnionFieldNames(u.Values)
193+
for i, v := range u.Values {
193194
// Sum-type unions (struct-based, with Kind/AsX accessors) store each
194195
// branch as either a value (primitives, arrays, maps) or a pointer
195196
// (object user types). The union validation template binds the branch
@@ -202,11 +203,11 @@ func recurseValidationCode(att *expr.AttributeExpr, put expr.UserType, attCtx *A
202203
continue
203204
}
204205
cases = append(cases, map[string]any{
205-
"typeTag": expr.UnionVariantTag(v),
206-
"fieldName": Goify(v.Name, true),
206+
"typeTag": expr.UnionVariantTag(v),
207+
"fieldName": uniqueNames[i],
207208
"requiresValue": strings.HasPrefix(strings.TrimSpace(val), "if actual != nil {"),
208-
"context": context + ".value",
209-
"validation": val,
209+
"context": context + ".value",
210+
"validation": val,
210211
})
211212
}
212213
if len(cases) > 0 {

dsl/attribute.go

Lines changed: 5 additions & 72 deletions
Original file line numberDiff line numberDiff line change
@@ -231,22 +231,12 @@ func Field(tag any, name string, args ...any) {
231231
// })
232232
// })
233233
func OneOf(arg any, args ...any) expr.DataType {
234-
if looksLikeOneOfAttributeDeclarationSignature(arg, args...) && !isOneOfDeclarationContext() {
235-
eval.IncompatibleDSL()
236-
return invalidOneOfType()
237-
}
238-
if isOneOfAttributeDeclaration(arg, args...) {
239-
oneOfAttribute(arg.(string), args...)
240-
return nil
241-
}
242234
if name, ok := arg.(string); ok {
243-
if isMalformedOneOfAttributeDeclaration(arg, args...) {
244-
oneOfAttribute(name, args...)
245-
return nil
246-
}
247-
if len(args) > 2 && !areOneOfTypeConstructorArgs(append([]any{arg}, args...)...) {
248-
eval.TooManyArgError()
249-
return nil
235+
if len(args) > 0 {
236+
if _, ok := args[len(args)-1].(func()); ok {
237+
oneOfAttribute(name, args...)
238+
return nil
239+
}
250240
}
251241
}
252242
return oneOfType(arg, args...)
@@ -356,63 +346,6 @@ func Example(args ...any) {
356346
a.UserExamples = append(a.UserExamples, ex)
357347
}
358348

359-
func isOneOfAttributeDeclaration(arg any, args ...any) bool {
360-
if !looksLikeOneOfAttributeDeclarationSignature(arg, args...) {
361-
return false
362-
}
363-
switch eval.Current().(type) {
364-
case *expr.AttributeExpr, expr.CompositeExpr:
365-
return true
366-
default:
367-
return false
368-
}
369-
}
370-
371-
func looksLikeOneOfAttributeDeclarationSignature(arg any, args ...any) bool {
372-
name, ok := arg.(string)
373-
if !ok || name == "" {
374-
return false
375-
}
376-
if len(args) == 0 || len(args) > 2 {
377-
return false
378-
}
379-
if _, ok := args[len(args)-1].(func()); !ok {
380-
return false
381-
}
382-
return true
383-
}
384-
385-
func isMalformedOneOfAttributeDeclaration(arg any, args ...any) bool {
386-
if !isOneOfDeclarationContext() || len(args) == 0 {
387-
return false
388-
}
389-
if _, ok := args[len(args)-1].(func()); ok {
390-
return true
391-
}
392-
return !areOneOfTypeConstructorArgs(append([]any{arg}, args...)...)
393-
}
394-
395-
func isOneOfDeclarationContext() bool {
396-
switch eval.Current().(type) {
397-
case *expr.AttributeExpr, expr.CompositeExpr:
398-
return true
399-
default:
400-
return false
401-
}
402-
}
403-
404-
func areOneOfTypeConstructorArgs(args ...any) bool {
405-
for _, arg := range args {
406-
switch arg.(type) {
407-
case expr.DataType, string:
408-
continue
409-
default:
410-
return false
411-
}
412-
}
413-
return len(args) >= 2
414-
}
415-
416349
func oneOfAttribute(name string, args ...any) {
417350
if len(args) == 0 {
418351
eval.TooFewArgError()

0 commit comments

Comments
 (0)