diff --git a/README.md b/README.md index 56d0770..60755a3 100644 --- a/README.md +++ b/README.md @@ -152,9 +152,9 @@ for _, shape := range shapes { } ``` -When every variant composes the same schema through `allOf`, the wrapper also -exposes it, so the fields all variants share are readable without a type switch -that has to be revisited whenever a variant is added: +When every variant carries the same properties, the wrapper also exposes them, so +the fields all variants share are readable without a type switch that has to be +revisited whenever a variant is added: ```go for _, pet := range pets { @@ -164,8 +164,16 @@ for _, pet := range pets { } ``` -`Base()` returns nil for an unknown variant, and is generated only when *every* -variant composes the *same* single base. +`Base()` returns nil for an unknown variant. Variants that compose a shared schema +through `allOf` name it directly, and there has to be exactly one such schema. +Variants that inline the same properties instead, which is all some producers +emit, get a `Base` struct synthesized from the properties every variant +declares identically: same name, same type, same required-ness. That type is +derived from the variants rather than declared by the spec, so it changes when +they do. The discriminator +is left out, since it is how the variants differ and a spec that spells the base +out keeps it out of the shared schema too. The result is a copy, so writing to it +does not change the variant the union holds. A payload that carries no discriminator property at all is still an error — there is nothing to identify it by — as is a union *without* a discriminator when no diff --git a/internal/analyzer/analyzer.go b/internal/analyzer/analyzer.go index 605bf5a..74c4419 100644 --- a/internal/analyzer/analyzer.go +++ b/internal/analyzer/analyzer.go @@ -94,9 +94,9 @@ func (a *Analyzer) Analyze(packageName string) (*ir.Package, error) { breakAliasCycles(pkg.Types) breakStructCycles(pkg.Types) - // A union whose variants all compose the same schema can expose it directly, - // which depends on the variants' final field shapes. - linkUnionBases(pkg.Types) + // A union whose variants all carry the same properties can expose them + // directly, which depends on the variants' final field shapes. + a.linkUnionBases(pkg) // Detect paginated operations. a.detectPagination(pkg) diff --git a/internal/analyzer/analyzer_test.go b/internal/analyzer/analyzer_test.go index 1a9cea9..e9ece4e 100644 --- a/internal/analyzer/analyzer_test.go +++ b/internal/analyzer/analyzer_test.go @@ -797,6 +797,9 @@ func TestComplexSchemas_AllTypesPresent(t *testing.T) { "ShapeCollection", "ShapeCollectionShapesValue", // Unions used directly as a request or response body. "CreateShapeBody", "CreateShapeResponse", "NamedShape", + // Circle and Rectangle both declare shapeType, and the body union + // dispatches on kind, so the shared property becomes a base. + "CreateShapeBodyBase", } if len(pkg.Types) != len(expectedTypes) { diff --git a/internal/analyzer/unionbases.go b/internal/analyzer/unionbases.go index e510322..3b4792e 100644 --- a/internal/analyzer/unionbases.go +++ b/internal/analyzer/unionbases.go @@ -1,18 +1,44 @@ package analyzer -import "github.com/parallelworks/openapi-client-generator/internal/ir" +import ( + naming "github.com/giraffesyo/openapi-go-naming" -// linkUnionBases records on every union the one type all of its variants embed, -// when there is exactly one. Reading a field the variants share is otherwise a -// type switch over every variant, which the compiler cannot keep exhaustive as + "github.com/parallelworks/openapi-client-generator/internal/ir" +) + +// linkUnionBases gives every union the type holding the properties its variants +// all carry, when there is one. Reading such a property is otherwise a type +// switch over every variant, which the compiler cannot keep exhaustive as // variants are added. -func linkUnionBases(types []*ir.TypeDef) { - byName := ir.TypesByName(types) - for _, td := range types { +// +// A variant set that composes a shared schema through allOf names it directly. +// One that inlines the same properties instead, as generators that flatten +// composition emit, gets a base synthesized from the properties they share. +func (a *Analyzer) linkUnionBases(pkg *ir.Package) { + byName := ir.TypesByName(pkg.Types) + for _, td := range pkg.Types { if td == nil || td.Kind != ir.TypeKindUnion || len(td.UnionTypes) == 0 { continue } - td.BaseType = sharedEmbeddedType(byName, td.UnionTypes) + if base := sharedEmbeddedType(byName, td.UnionTypes); base != "" { + td.BaseType = base + td.BaseEmbedded = true + continue + } + fields := sharedFields(byName, td.UnionTypes, td.Discriminator) + if len(fields) == 0 { + continue + } + base := &ir.TypeDef{ + Name: a.namer.Unique(naming.Exported(td.Name + "Base")), + Description: "The properties every variant of " + td.Name + " declares.\n" + + "Derived from the variants rather than declared by the spec, so it\n" + + "changes when they do.", + Kind: ir.TypeKindStruct, + Fields: fields, + } + pkg.Types = append(pkg.Types, base) + td.BaseType = base.Name } } @@ -37,6 +63,87 @@ func sharedEmbeddedType(byName map[string]*ir.TypeDef, variants []*ir.UnionVaria return shared[0] } +// sharedFields returns copies of the fields every variant declares identically, +// in the first variant's order. It returns nil unless the union has at least two +// distinct struct variants: one variant shares everything with itself, which +// would make a base that only restates it. +func sharedFields(byName map[string]*ir.TypeDef, variants []*ir.UnionVariant, disc *ir.DiscriminatorDef) []*ir.Field { + var structs []*ir.TypeDef + seen := make(map[string]bool, len(variants)) + for _, v := range variants { + if seen[v.TypeName] { + continue + } + seen[v.TypeName] = true + td := ir.StructNamed(byName, v.TypeName) + if td == nil { + return nil + } + structs = append(structs, td) + } + if len(structs) < 2 { + return nil + } + + var shared []*ir.Field + for _, f := range structs[0].Fields { + if shareable(f, disc) { + shared = append(shared, f) + } + } + for _, td := range structs[1:] { + declared := make(map[string]bool, len(td.Fields)) + for _, f := range td.Fields { + if shareable(f, disc) { + declared[fieldKey(f)] = true + } + } + var kept []*ir.Field + for _, f := range shared { + if declared[fieldKey(f)] { + kept = append(kept, f) + } + } + shared = kept + if len(shared) == 0 { + return nil + } + } + + copies := make([]*ir.Field, len(shared)) + for i, f := range shared { + field := *f + copies[i] = &field + } + return copies +} + +// shareable reports whether a field can belong to a synthesized base: an embedded +// type is the other path's business, a catch-all holds what its own schema left +// undeclared rather than a property the variants agree on, and the discriminator +// is how the variants differ, which is also why a spec that spells the base out +// keeps it out of the shared schema. +func shareable(f *ir.Field, disc *ir.DiscriminatorDef) bool { + if f.Embedded || f.CatchAll { + return false + } + return disc == nil || f.JSONName != disc.PropertyName +} + +// fieldKey identifies a field by everything that shapes the Go it generates, so +// two variants agree on a property only when they declare it the same way. +func fieldKey(f *ir.Field) string { + key := f.Name + "\x00" + f.JSONName + "\x00" + f.Type + for _, flag := range []bool{f.Required, f.IsPointer, f.OmitEmpty, f.ReadOnly, f.WriteOnly, f.Deprecated} { + if flag { + key += "1" + } else { + key += "0" + } + } + return key +} + // embeddedTypeNames returns the struct types goType embeds by value. func embeddedTypeNames(byName map[string]*ir.TypeDef, goType string) []string { td := ir.StructNamed(byName, goType) diff --git a/internal/analyzer/unionbases_test.go b/internal/analyzer/unionbases_test.go index 01d0738..a57ad07 100644 --- a/internal/analyzer/unionbases_test.go +++ b/internal/analyzer/unionbases_test.go @@ -1,6 +1,11 @@ package analyzer -import "testing" +import ( + "reflect" + "testing" + + "github.com/parallelworks/openapi-client-generator/internal/ir" +) const sharedBaseSpec = `openapi: 3.1.0 info: { title: t, version: "1" } @@ -135,3 +140,276 @@ func TestUnionBase_AmbiguousBaseIsLeftAlone(t *testing.T) { t.Errorf("Either.BaseType = %q, want empty", either.BaseType) } } + +const inlinedSharedSpec = `openapi: 3.1.0 +info: { title: t, version: "1" } +paths: {} +components: + schemas: + Dog: + type: object + properties: + id: { type: string } + name: { type: string } + age: { type: integer } + kind: { type: string, enum: [dog] } + goodBoy: { type: boolean } + required: [id, name, age, kind, goodBoy] + Cat: + type: object + properties: + id: { type: string } + name: { type: string } + age: { type: integer } + kind: { type: string, enum: [cat] } + livesLeft: { type: integer } + required: [id, name, age, kind, livesLeft] + Pet: + oneOf: + - $ref: "#/components/schemas/Dog" + - $ref: "#/components/schemas/Cat" + discriminator: + propertyName: kind + mapping: { dog: "#/components/schemas/Dog", cat: "#/components/schemas/Cat" } +` + +// A spec that inlines the shared properties instead of composing them through +// allOf describes the same thing, and generators that flatten composition emit +// nothing else. +func TestUnionBase_SynthesizedFromInlinedProperties(t *testing.T) { + _, typeMap := analyzeSpec(t, inlinedSharedSpec) + + pet := typeMap["Pet"] + if pet == nil { + t.Fatal("Pet type not found") + } + if pet.BaseType != "PetBase" { + t.Fatalf("Pet.BaseType = %q, want PetBase", pet.BaseType) + } + if pet.BaseEmbedded { + t.Error("Pet.BaseEmbedded = true, want false: the variants declare the properties themselves") + } + + base := typeMap["PetBase"] + if base == nil { + t.Fatal("synthesized PetBase not found in package types") + } + if base.Kind != ir.TypeKindStruct { + t.Errorf("PetBase kind = %v, want struct", base.Kind) + } + var got []string + for _, f := range base.Fields { + got = append(got, f.Name+" "+f.Type) + } + want := []string{"ID string", "Name string", "Age int64"} + if len(got) != len(want) { + t.Fatalf("PetBase fields = %v, want %v", got, want) + } + for i, w := range want { + if got[i] != w { + t.Errorf("PetBase field %d = %q, want %q", i, got[i], w) + } + } +} + +func TestUnionBase_EmbeddedBaseWinsOverSynthesis(t *testing.T) { + _, typeMap := analyzeSpec(t, sharedBaseSpec) + + pet := typeMap["Pet"] + if pet == nil { + t.Fatal("Pet type not found") + } + if !pet.BaseEmbedded { + t.Error("Pet.BaseEmbedded = false, want true: the variants compose PetBase") + } + if typeMap["PetBase"] == nil { + t.Error("the spec's own PetBase should be the base, not a synthesized copy") + } +} + +const partiallySharedSpec = `openapi: 3.1.0 +info: { title: t, version: "1" } +paths: {} +components: + schemas: + Dog: + type: object + properties: + id: { type: string } + age: { type: integer } + kind: { type: string } + required: [id, age, kind] + Cat: + type: object + properties: + id: { type: string } + age: { type: string } + name: { type: string } + kind: { type: string } + required: [id, age, kind] + Pet: + oneOf: + - $ref: "#/components/schemas/Dog" + - $ref: "#/components/schemas/Cat" + discriminator: + propertyName: kind + mapping: { dog: "#/components/schemas/Dog", cat: "#/components/schemas/Cat" } +` + +// age is an integer in one variant and a string in the other, and name is absent +// from one, so neither is a property the variants agree on. +func TestUnionBase_SynthesizesOnlyIdenticalProperties(t *testing.T) { + _, typeMap := analyzeSpec(t, partiallySharedSpec) + + pet := typeMap["Pet"] + if pet == nil { + t.Fatal("Pet type not found") + } + base := typeMap[pet.BaseType] + if base == nil { + t.Fatalf("Pet.BaseType = %q, which is not a generated type", pet.BaseType) + } + if len(base.Fields) != 1 || base.Fields[0].Name != "ID" { + t.Errorf("%s fields = %+v, want ID alone", base.Name, base.Fields) + } +} + +const discriminatorOnlySpec = `openapi: 3.1.0 +info: { title: t, version: "1" } +paths: {} +components: + schemas: + Dog: + type: object + properties: + kind: { type: string } + goodBoy: { type: boolean } + required: [kind, goodBoy] + Cat: + type: object + properties: + kind: { type: string } + livesLeft: { type: integer } + required: [kind, livesLeft] + Pet: + oneOf: + - $ref: "#/components/schemas/Dog" + - $ref: "#/components/schemas/Cat" + discriminator: + propertyName: kind + mapping: { dog: "#/components/schemas/Dog", cat: "#/components/schemas/Cat" } +` + +// The discriminator is how the variants differ, and a spec that spells the base +// out keeps it out of the shared schema, so it is not a base of its own. +func TestUnionBase_DiscriminatorAloneIsNoBase(t *testing.T) { + _, typeMap := analyzeSpec(t, discriminatorOnlySpec) + + pet := typeMap["Pet"] + if pet == nil { + t.Fatal("Pet type not found") + } + if pet.BaseType != "" { + t.Errorf("Pet.BaseType = %q, want empty", pet.BaseType) + } +} + +const takenBaseNameSpec = `openapi: 3.1.0 +info: { title: t, version: "1" } +paths: {} +components: + schemas: + PetBase: + type: object + properties: + unrelated: { type: string } + Dog: + type: object + properties: + id: { type: string } + kind: { type: string } + required: [id, kind] + Cat: + type: object + properties: + id: { type: string } + kind: { type: string } + required: [id, kind] + Pet: + oneOf: + - $ref: "#/components/schemas/Dog" + - $ref: "#/components/schemas/Cat" + discriminator: + propertyName: kind + mapping: { dog: "#/components/schemas/Dog", cat: "#/components/schemas/Cat" } +` + +func TestUnionBase_SynthesizedNameAvoidsTheSpecsOwn(t *testing.T) { + _, typeMap := analyzeSpec(t, takenBaseNameSpec) + + pet := typeMap["Pet"] + if pet == nil { + t.Fatal("Pet type not found") + } + if pet.BaseType == "" || pet.BaseType == "PetBase" { + t.Fatalf("Pet.BaseType = %q, want a name the spec's own PetBase does not hold", pet.BaseType) + } + if base := typeMap[pet.BaseType]; base == nil || len(base.Fields) != 1 { + t.Errorf("%s = %+v, want the synthesized base holding id", pet.BaseType, base) + } + if spec := typeMap["PetBase"]; spec == nil || len(spec.Fields) != 1 || spec.Fields[0].Name != "Unrelated" { + t.Errorf("the spec's PetBase was overwritten: %+v", spec) + } +} + +const nullableStructSpec = `openapi: 3.1.0 +info: { title: t, version: "1" } +paths: {} +components: + schemas: + Person: + type: object + properties: + id: { type: string } + name: { type: string } + MaybePerson: + anyOf: + - $ref: "#/components/schemas/Person" + - type: "null" + PersonOrName: + anyOf: + - $ref: "#/components/schemas/Person" + - type: string +` + +// One variant shares every property with itself, and a variant with no fields at +// all shares none, so neither shape has a base to expose. +func TestUnionBase_NeedsTwoStructVariants(t *testing.T) { + _, typeMap := analyzeSpec(t, nullableStructSpec) + + for _, name := range []string{"MaybePerson", "PersonOrName"} { + td := typeMap[name] + if td == nil { + continue + } + if td.BaseType != "" { + t.Errorf("%s.BaseType = %q, want empty", name, td.BaseType) + } + } +} + +// sharedFields copies each field it keeps with *f, which is a complete copy only +// while ir.Field holds nothing by reference. A member added later that the copy +// would alias has to be copied deliberately, and the aliasing is silent until +// something writes through it. +func TestSharedFieldsCopyStaysComplete(t *testing.T) { + field := reflect.TypeFor[ir.Field]() + for i := range field.NumField() { + member := field.Field(i) + switch member.Type.Kind() { + case reflect.String, reflect.Bool: + default: + t.Errorf("ir.Field.%s is a %s, which sharedFields now aliases into the synthesized base", member.Name, member.Type.Kind()) + } + } +} diff --git a/internal/generator/e2e_union_base_test.go b/internal/generator/e2e_union_base_test.go index 126e1ef..1b0f4ea 100644 --- a/internal/generator/e2e_union_base_test.go +++ b/internal/generator/e2e_union_base_test.go @@ -141,3 +141,111 @@ func TestBaseDoesNotAliasTheVariant(t *testing.T) { } `) } + +const inlinedUnionBaseSpec = `openapi: 3.1.0 +info: { title: pets, version: "1" } +paths: {} +components: + schemas: + Dog: + type: object + properties: + id: { type: string } + name: { type: string } + age: { type: integer } + kind: { type: string, enum: [dog] } + goodBoy: { type: boolean } + required: [id, name, age, kind, goodBoy] + Cat: + type: object + properties: + id: { type: string } + name: { type: string } + age: { type: integer } + kind: { type: string, enum: [cat] } + livesLeft: { type: integer } + required: [id, name, age, kind, livesLeft] + Pet: + oneOf: + - $ref: "#/components/schemas/Dog" + - $ref: "#/components/schemas/Cat" + discriminator: + propertyName: kind + mapping: { dog: "#/components/schemas/Dog", cat: "#/components/schemas/Cat" } +` + +// TestE2E_UnionBaseFromInlinedProperties covers the same union written without +// allOf, which is all a producer that flattens composition can emit: the shared +// properties are still reachable off the wrapper. +func TestE2E_UnionBaseFromInlinedProperties(t *testing.T) { + files, _ := generateFromSpec(t, inlinedUnionBaseSpec, "petsapi") + + runGeneratedWireTest(t, files, "inlinedunionbase", `package petsapi + +import ( + "encoding/json" + "testing" +) + +func TestBaseCopiesTheSharedProperties(t *testing.T) { + var pets []Pet + payload := `+"`"+`[ + {"kind":"dog","id":"1","name":"Rex","age":4,"goodBoy":true}, + {"kind":"cat","id":"2","name":"Momo","age":7,"livesLeft":9} + ]`+"`"+` + if err := json.Unmarshal([]byte(payload), &pets); err != nil { + t.Fatalf("unmarshal: %v", err) + } + + want := []PetBase{{ID: "1", Name: "Rex", Age: 4}, {ID: "2", Name: "Momo", Age: 7}} + for i, p := range pets { + base := p.Base() + if base == nil { + t.Fatalf("pets[%d].Base() = nil, want the shared properties", i) + } + if *base != want[i] { + t.Errorf("pets[%d].Base() = %+v, want %+v", i, *base, want[i]) + } + } + if _, ok := pets[1].Value.(Cat); !ok { + t.Errorf("pets[1].Value = %T, want Cat", pets[1].Value) + } +} + +func TestBaseIsNilForAnUnknownVariant(t *testing.T) { + var p Pet + if err := json.Unmarshal([]byte(`+"`"+`{"kind":"parrot","id":"3","name":"Polly"}`+"`"+`), &p); err != nil { + t.Fatalf("unmarshal: %v", err) + } + if p.Base() != nil { + t.Errorf("Base() = %+v, want nil for an unknown variant", p.Base()) + } +} + +func TestVariantsKeepTheirOwnFields(t *testing.T) { + var p Pet + if err := json.Unmarshal([]byte(`+"`"+`{"kind":"dog","id":"1","name":"Rex","age":4,"goodBoy":true}`+"`"+`), &p); err != nil { + t.Fatalf("unmarshal: %v", err) + } + dog, ok := p.Value.(Dog) + if !ok { + t.Fatalf("Value = %T, want Dog", p.Value) + } + if dog.Name != "Rex" || !dog.GoodBoy { + t.Errorf("Dog = %+v, want its own fields intact", dog) + } + + out, err := json.Marshal(p) + if err != nil { + t.Fatalf("marshal: %v", err) + } + var back Pet + if err := json.Unmarshal(out, &back); err != nil { + t.Fatalf("round trip: %v", err) + } + if *back.Base() != *p.Base() { + t.Errorf("round trip changed the base: %+v vs %+v", *back.Base(), *p.Base()) + } +} +`) +} diff --git a/internal/generator/funcmap.go b/internal/generator/funcmap.go index 949f246..3e9667c 100644 --- a/internal/generator/funcmap.go +++ b/internal/generator/funcmap.go @@ -32,6 +32,7 @@ func FuncMap() template.FuncMap { "hasUnions": hasUnions, "hasUntypedVariant": hasUntypedVariant, "distinctVariants": distinctVariants, + "unionBaseFields": unionBaseFields, "catchAllField": catchAllField, "catchAllValueType": catchAllValueType, "hasCatchAllTypes": hasCatchAllTypes, @@ -572,6 +573,20 @@ func distinctVariants(td *ir.TypeDef) []string { return names } +// unionBaseFields returns the fields a union's Base accessor copies out of the +// variant it holds. It is empty when the variants embed the base, which the +// accessor takes the address of instead. +func unionBaseFields(pkg *ir.Package, td *ir.TypeDef) []*ir.Field { + if td.BaseType == "" || td.BaseEmbedded { + return nil + } + base := typeIndex(pkg).byName[td.BaseType] + if base == nil { + return nil + } + return base.Fields +} + // discriminatorFieldName converts a JSON property name to a Go field name // for use in the discriminator struct in UnmarshalJSON. func discriminatorFieldName(propertyName string) string { diff --git a/internal/ir/types.go b/internal/ir/types.go index 1562e0c..5c05100 100644 --- a/internal/ir/types.go +++ b/internal/ir/types.go @@ -79,7 +79,8 @@ type TypeDef struct { EnumValues []*EnumVal // For enums EnumGoType string // For enums: the underlying Go type (e.g., "string", "int") UnionTypes []*UnionVariant // For oneOf/anyOf unions - BaseType string // For unions: the type every variant embeds + BaseType string // For unions: the type holding the properties every variant shares + BaseEmbedded bool // For unions: whether the variants embed BaseType rather than declaring its fields Discriminator *DiscriminatorDef // If polymorphic via discriminator IsNullable bool } diff --git a/internal/templates/types.go.tmpl b/internal/templates/types.go.tmpl index 890fb08..2558392 100644 --- a/internal/templates/types.go.tmpl +++ b/internal/templates/types.go.tmpl @@ -290,15 +290,23 @@ func (u {{ .Name }}) Raw() json.RawMessage { return json.RawMessage(u.raw) } {{ end }} -{{- if .BaseType }}{{ $base := .BaseType }} -// Base returns the {{ .BaseType }} that every variant of {{ .Name }} composes, or +{{- if .BaseType }}{{ $base := .BaseType }}{{ $baseFields := unionBaseFields $ . }} +// Base returns the {{ .BaseType }} that every variant of {{ .Name }} carries, or // nil when Value holds none of them. It is a copy: writing to it does not change // the value the union holds. func (u {{ .Name }}) Base() *{{ .BaseType }} { switch v := u.Value.(type) { {{- range distinctVariants . }} case {{ . }}: +{{- if $baseFields }} + return &{{ $base }}{ +{{- range $baseFields }} + {{ .Name }}: v.{{ .Name }}, +{{- end }} + } +{{- else }} return &v.{{ $base }} +{{- end }} {{- end }} } return nil