diff --git a/internal/analyzer/schemas.go b/internal/analyzer/schemas.go index 3bb8396..20c0f14 100644 --- a/internal/analyzer/schemas.go +++ b/internal/analyzer/schemas.go @@ -179,11 +179,25 @@ func (a *Analyzer) convertAllOf(goName string, schema *highbase.Schema, nullable IsNullable: nullable, } - // Collect required fields from the parent schema. + // Collect required fields from the parent schema. Every entry's requirements + // apply to the same value, so they are read before any property is built and + // a property required by one entry is required by the struct. requiredSet := make(map[string]bool, len(schema.Required)) for _, r := range schema.Required { requiredSet[r] = true } + for _, proxy := range schema.AllOf { + if entry, err := proxy.BuildSchema(); err == nil && entry != nil { + for _, r := range entry.Required { + requiredSet[r] = true + } + } + } + + // Entries compose one value, so a property more than one of them declares is + // one field. Appending a field per declaration gives two with the same JSON + // tag, which encoding/json resolves by ignoring both. + declared := make(map[string]bool) for _, proxy := range schema.AllOf { ref := proxy.GetReference() @@ -209,11 +223,6 @@ func (a *Analyzer) convertAllOf(goName string, schema *highbase.Schema, nullable continue } - // Merge required from the allOf entry. - for _, r := range entrySchema.Required { - requiredSet[r] = true - } - if entrySchema.Properties == nil { continue } @@ -227,6 +236,10 @@ func (a *Analyzer) convertAllOf(goName string, schema *highbase.Schema, nullable continue } + if declared[propName] { + continue + } + declared[propName] = true td.Fields = addField(td.Fields, a.convertProperty(goName, propName, propSchema, requiredSet[propName], multipartBody)) } } diff --git a/internal/analyzer/schemas_allof_property_test.go b/internal/analyzer/schemas_allof_property_test.go index 4aee7b9..02201ea 100644 --- a/internal/analyzer/schemas_allof_property_test.go +++ b/internal/analyzer/schemas_allof_property_test.go @@ -191,3 +191,83 @@ func TestAllOfProperty_MultipartBodyKeepsItsFileParts(t *testing.T) { t.Errorf("fields = %+v, want the embedded Meta beside the file", body.Fields) } } + +const allOfRedeclaredSpec = `openapi: 3.1.0 +info: { title: t, version: "1" } +paths: {} +components: + schemas: + Forkee: + allOf: + - type: object + properties: + name: { type: string } + archived: { type: boolean } + size: { type: integer } + - type: object + properties: + name: { type: string } + archived: { type: boolean } + required: [name] + Composed: + allOf: + - $ref: "#/components/schemas/Base" + - type: object + properties: + id: { type: string } + Base: + type: object + properties: + id: { type: string } +` + +// allOf entries compose one value, so a property more than one declares is one +// field. Two fields carrying one JSON tag is not just untidy: encoding/json +// resolves that by ignoring both, so the property stops round-tripping. +func TestAllOf_RedeclaredPropertyIsOneField(t *testing.T) { + _, typeMap := analyzeSpec(t, allOfRedeclaredSpec) + + forkee := typeMap["Forkee"] + if forkee == nil { + t.Fatal("Forkee not found") + } + + seen := map[string]int{} + for _, f := range forkee.Fields { + seen[f.JSONName]++ + } + for _, prop := range []string{"name", "archived"} { + if seen[prop] != 1 { + t.Errorf("%s appears %d times, want once", prop, seen[prop]) + } + } + if len(forkee.Fields) != 3 { + t.Errorf("fields = %+v, want name, archived, and size", forkee.Fields) + } + + // A property one entry requires is required by the struct, whichever entry + // declared it first. + for _, f := range forkee.Fields { + if f.JSONName == "name" && !f.Required { + t.Errorf("name = %+v, want required: the second entry requires it", f) + } + } +} + +// A property beside an embedded type is a different case: they live at +// different depths, so Go shadows one with the other and the JSON tags do not +// collide. +func TestAllOf_PropertyBesideAnEmbedIsKept(t *testing.T) { + _, typeMap := analyzeSpec(t, allOfRedeclaredSpec) + + composed := typeMap["Composed"] + if composed == nil { + t.Fatal("Composed not found") + } + if len(composed.Fields) != 2 { + t.Fatalf("fields = %+v, want the embed and the property", composed.Fields) + } + if !composed.Fields[0].Embedded || composed.Fields[1].JSONName != "id" { + t.Errorf("fields = %+v, want Base embedded beside id", composed.Fields) + } +} diff --git a/internal/generator/e2e_combinations_test.go b/internal/generator/e2e_combinations_test.go index 2077198..ab42930 100644 --- a/internal/generator/e2e_combinations_test.go +++ b/internal/generator/e2e_combinations_test.go @@ -79,6 +79,7 @@ func TestE2E_Combinations(t *testing.T) { {"operations.go", "params.Either", "a union-typed parameter"}, {"errors.go", "func (e *ProblemResponse) Error() string", "a typed error wrapper"}, {"errors.go", "e.Detail.Detail", "a message field found by its conventional name"}, + {"types.go", "Name string `json:\"name\"`", "a property two allOf entries declare, required by one"}, } { if !containsCollapsed(byName[want.file], want.decl) { t.Errorf("%s is missing %s (%s)", want.file, want.why, want.decl) diff --git a/testdata/combinations.yaml b/testdata/combinations.yaml index d2bd8fa..7a5bfcc 100644 --- a/testdata/combinations.yaml +++ b/testdata/combinations.yaml @@ -187,6 +187,18 @@ components: kind: { const: gadget } gears: { type: integer } required: [kind, gears] + # Two entries that redeclare properties: one field each, or encoding/json + # ignores both of them. + Refined: + allOf: + - type: object + properties: + name: { type: string } + size: { type: integer } + - type: object + properties: + name: { type: string } + required: [name] Alert: type: object properties: @@ -199,6 +211,7 @@ components: type: object patternProperties: "^x-": { type: string } + refined: { $ref: "#/components/schemas/Refined" } required: [id] Problem: type: object