diff --git a/README.md b/README.md index 3516103..114f9b5 100644 --- a/README.md +++ b/README.md @@ -152,8 +152,10 @@ for _, shape := range shapes { } ``` -A union *without* a discriminator still fails when no variant matches, since there -is nothing to identify the payload by. +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 +variant matches. When the schema declares a `discriminator` but no `mapping`, the +variant's schema name is used as the discriminator value, per the OpenAPI spec. ## License diff --git a/internal/analyzer/operations.go b/internal/analyzer/operations.go index 3af8495..31516b6 100644 --- a/internal/analyzer/operations.go +++ b/internal/analyzer/operations.go @@ -92,7 +92,7 @@ func (a *Analyzer) convertOperation(httpMethod, path string, pathItem *v3high.Pa // Responses. if op.Responses != nil { - a.convertResponses(op.Responses, opDef, name+"Response") + a.convertResponses(op.Responses, opDef, name) } // Security requirements. @@ -296,10 +296,17 @@ func (a *Analyzer) convertRequestBody(rb *v3high.RequestBody, nameHint string) ( } // convertResponses converts operation responses into the OperationDef fields. -func (a *Analyzer) convertResponses(responses *v3high.Responses, opDef *ir.OperationDef, nameHint string) { +func (a *Analyzer) convertResponses(responses *v3high.Responses, opDef *ir.OperationDef, opName string) { if responses.Codes != nil { for code, resp := range responses.Codes.FromOldest() { - rd := a.convertSingleResponse(code, resp, nameHint) + // Only the success body reaches the method signature, so it keeps the + // plain Response hint; the others carry their status code so two + // inline bodies of one operation can't land on the same name. + hint := opName + "Response" + code + if isSuccessCode(code) && opDef.SuccessResponse == nil { + hint = opName + "Response" + } + rd := a.convertSingleResponse(code, resp, hint) opDef.Responses = append(opDef.Responses, rd) if isSuccessCode(code) { @@ -315,7 +322,7 @@ func (a *Analyzer) convertResponses(responses *v3high.Responses, opDef *ir.Opera // Handle the default response. if responses.Default != nil { - rd := a.convertSingleResponse("default", responses.Default, nameHint) + rd := a.convertSingleResponse("default", responses.Default, opName+"DefaultResponse") rd.IsError = true opDef.Responses = append(opDef.Responses, rd) opDef.ErrorResponses = append(opDef.ErrorResponses, rd) diff --git a/internal/analyzer/schemas.go b/internal/analyzer/schemas.go index 1907563..9a2e638 100644 --- a/internal/analyzer/schemas.go +++ b/internal/analyzer/schemas.go @@ -301,6 +301,25 @@ func (a *Analyzer) convertUnion(goName string, schema *highbase.Schema, variants discMapping[v] = k } } + // Without an explicit mapping the spec says the discriminator value is the + // variant's schema name; deriving it keeps the decoder from treating every + // payload as an unknown variant. + if len(td.Discriminator.Mapping) == 0 { + discMapping = make(map[string]string) + for _, proxy := range variants { + ref := proxy.GetReference() + refName := refToSchemaName(ref) + if refName == "" { + continue + } + goTypeName := naming.Exported(refName) + if existing, ok := a.typesBySchema[refName]; ok { + goTypeName = existing.Name + } + td.Discriminator.Mapping[refName] = goTypeName + discMapping[ref] = refName + } + } } for _, proxy := range variants { @@ -337,7 +356,7 @@ func (a *Analyzer) convertUnion(goName string, schema *highbase.Schema, variants func (a *Analyzer) convertObject(goName string, schema *highbase.Schema, nullable bool) (*ir.TypeDef, error) { // If no defined properties and additionalProperties is set, generate a map alias. hasProperties := schema.Properties != nil && schema.Properties.Len() > 0 - if !hasProperties && schema.AdditionalProperties != nil { + if !hasProperties && allowsAdditionalProperties(schema) { return a.convertAdditionalPropertiesMap(goName, schema, nullable) } @@ -393,10 +412,10 @@ func (a *Analyzer) convertObject(goName string, schema *highbase.Schema, nullabl // If the object has both properties and additionalProperties, add an extra field. // The generator gives such structs MarshalJSON/UnmarshalJSON so the map is // inlined into the object rather than nested under a key of its own. - if schema.AdditionalProperties != nil { + if allowsAdditionalProperties(schema) { mapValueType := a.resolveAdditionalPropertiesType(schema, goName) td.Fields = append(td.Fields, &ir.Field{ - Name: "AdditionalProperties", + Name: catchAllFieldName(td.Fields), JSONName: "-", Type: "map[string]" + mapValueType, Description: "Properties not defined by the schema.", @@ -407,6 +426,30 @@ func (a *Analyzer) convertObject(goName string, schema *highbase.Schema, nullabl return td, nil } +// allowsAdditionalProperties reports whether the schema permits undeclared +// properties. `additionalProperties: false` forbids them, so no catch-all is +// generated for it. +func allowsAdditionalProperties(schema *highbase.Schema) bool { + ap := schema.AdditionalProperties + if ap == nil { + return false + } + return !ap.IsB() || ap.B +} + +// catchAllFieldName picks a Go name for the synthetic additionalProperties field +// that no declared property has already taken. +func catchAllFieldName(fields []*ir.Field) string { + name := "AdditionalProperties" + taken := func(candidate string) bool { + return slices.ContainsFunc(fields, func(f *ir.Field) bool { return f.Name == candidate }) + } + for i := 2; taken(name); i++ { + name = "AdditionalProperties" + strconv.Itoa(i) + } + return name +} + // convertAdditionalPropertiesMap creates a map alias when an object has // additionalProperties but no defined properties. func (a *Analyzer) convertAdditionalPropertiesMap(goName string, schema *highbase.Schema, nullable bool) (*ir.TypeDef, error) { @@ -507,7 +550,7 @@ func (a *Analyzer) resolveGoType(schema *highbase.Schema, nameHint string) strin case "object": // Inline objects without properties -> a map of the additionalProperties type. if schema.Properties == nil || schema.Properties.Len() == 0 { - if schema.AdditionalProperties != nil { + if allowsAdditionalProperties(schema) { return "map[string]" + a.resolveAdditionalPropertiesType(schema, nameHint) } return "map[string]any" diff --git a/internal/generator/e2e_additional_properties_test.go b/internal/generator/e2e_additional_properties_test.go index b90dda0..5915b82 100644 --- a/internal/generator/e2e_additional_properties_test.go +++ b/internal/generator/e2e_additional_properties_test.go @@ -1,13 +1,8 @@ package generator import ( - "os" - "os/exec" - "path/filepath" + "strings" "testing" - - "github.com/parallelworks/openapi-client-generator/internal/analyzer" - "github.com/parallelworks/openapi-client-generator/internal/parser" ) const additionalPropertiesSpec = `openapi: 3.1.0 @@ -38,49 +33,14 @@ components: owner: { type: string } additionalProperties: type: string + Sealed: + type: object + properties: + only: { type: string } + additionalProperties: false ` -// TestE2E_AdditionalPropertiesRoundTrip generates a client for schemas that mix -// declared properties with additionalProperties, then compiles and RUNS a test -// proving unknown keys survive an unmarshal/marshal round trip instead of being -// dropped or emitted under a literal "-" key. -func TestE2E_AdditionalPropertiesRoundTrip(t *testing.T) { - specDir := t.TempDir() - specPath := filepath.Join(specDir, "spec.yaml") - if err := os.WriteFile(specPath, []byte(additionalPropertiesSpec), 0o644); err != nil { - t.Fatalf("writing spec: %v", err) - } - - result, err := parser.Parse(specPath, parser.Config{}) - if err != nil { - t.Fatalf("Parse: %v", err) - } - - a := analyzer.New(result.Model) - pkg, err := a.Analyze("petsapi") - if err != nil { - t.Fatalf("Analyze: %v", err) - } - - gen, err := New(pkg) - if err != nil { - t.Fatalf("New generator: %v", err) - } - files, err := gen.Generate() - if err != nil { - t.Fatalf("Generate: %v", err) - } - - tmpDir := t.TempDir() - goMod := []byte("module additionalprops-e2e-test\n\ngo 1.25.5\n") - if err := os.WriteFile(filepath.Join(tmpDir, "go.mod"), goMod, 0o644); err != nil { - t.Fatalf("writing go.mod: %v", err) - } - if err := WriteFiles(tmpDir, files); err != nil { - t.Fatalf("WriteFiles: %v", err) - } - - runtimeTest := []byte(`package petsapi +const additionalPropertiesRuntimeTest = `package petsapi import ( "encoding/json" @@ -131,6 +91,45 @@ func TestUnknownKeysSurviveRoundTrip(t *testing.T) { } } +// encoding/json falls back to a case-insensitive tag match, so a differently +// cased declared property must not also be collected as an unknown one -- that +// would emit the same property twice on the way back out. +func TestDifferentlyCasedDeclaredPropertyIsNotDuplicated(t *testing.T) { + var p Pet + if err := json.Unmarshal([]byte(` + "`" + `{"Name":"rex","extra":1}` + "`" + `), &p); err != nil { + t.Fatalf("unmarshal: %v", err) + } + if p.Name != "rex" { + t.Errorf("Name = %q, want rex", p.Name) + } + if _, ok := p.AdditionalProperties["Name"]; ok { + t.Fatalf("declared property leaked into AdditionalProperties: %v", p.AdditionalProperties) + } + + out, err := json.Marshal(p) + if err != nil { + t.Fatalf("marshal: %v", err) + } + var got map[string]any + if err := json.Unmarshal(out, &got); err != nil { + t.Fatalf("re-unmarshal: %v", err) + } + if len(got) != 2 { + t.Errorf("round trip = %s, want exactly name and extra", out) + } +} + +// A decode must not carry over values from a previous one. +func TestDecodeResetsAdditionalProperties(t *testing.T) { + p := Pet{AdditionalProperties: map[string]any{"stale": true}} + if err := json.Unmarshal([]byte(` + "`" + `{"name":"rex","fresh":1}` + "`" + `), &p); err != nil { + t.Fatalf("unmarshal: %v", err) + } + if _, ok := p.AdditionalProperties["stale"]; ok { + t.Errorf("stale additional property survived: %v", p.AdditionalProperties) + } +} + func TestTypedAdditionalProperties(t *testing.T) { var l Labels if err := json.Unmarshal([]byte(` + "`" + `{"owner":"me","env":"prod"}` + "`" + `), &l); err != nil { @@ -161,16 +160,34 @@ func TestEmptyAdditionalPropertiesOmitsNothingExtra(t *testing.T) { t.Errorf("marshal = %s, want {\"name\":\"rex\"}", out) } } -`) - if err := os.WriteFile(filepath.Join(tmpDir, "additional_properties_test.go"), runtimeTest, 0o644); err != nil { - t.Fatalf("writing runtime test: %v", err) - } +` - cmd := exec.Command("go", "test", "./...") - cmd.Dir = tmpDir - output, err := cmd.CombinedOutput() - if err != nil { - t.Fatalf("go test on generated code failed: %v\n%s", err, string(output)) +// TestE2E_AdditionalPropertiesRoundTrip generates a client for schemas that mix +// declared properties with additionalProperties, then compiles and RUNS a test +// proving unknown keys survive an unmarshal/marshal round trip instead of being +// dropped or emitted under a literal "-" key. +func TestE2E_AdditionalPropertiesRoundTrip(t *testing.T) { + files, _ := generateFromSpec(t, additionalPropertiesSpec, "petsapi") + runGeneratedWireTest(t, files, "additionalprops", additionalPropertiesRuntimeTest) +} + +// additionalProperties: false forbids unknown properties, so no catch-all field +// (and no custom marshalers) may be generated for such a schema. +func TestE2E_AdditionalPropertiesFalseHasNoCatchAll(t *testing.T) { + files, _ := generateFromSpec(t, additionalPropertiesSpec, "petsapi") + + var types string + for _, f := range files { + if f.Name == "types.go" { + types = string(f.Content) + } + } + sealed := types[strings.Index(types, "type Sealed struct"):] + sealed = sealed[:strings.Index(sealed, "\n}")] + if strings.Contains(sealed, "AdditionalProperties") { + t.Errorf("Sealed got a catch-all field despite additionalProperties: false:\n%s", sealed) + } + if strings.Contains(types, "func (t Sealed) MarshalJSON") { + t.Error("Sealed got additionalProperties marshalers despite additionalProperties: false") } - t.Logf("additionalProperties round trip test passed:\n%s", string(output)) } diff --git a/internal/generator/e2e_inline_union_test.go b/internal/generator/e2e_inline_union_test.go index 680afe7..1f646b6 100644 --- a/internal/generator/e2e_inline_union_test.go +++ b/internal/generator/e2e_inline_union_test.go @@ -135,6 +135,25 @@ func TestUnknownVariantDoesNotFailSiblings(t *testing.T) { t.Error("shapes[b] should be an unknown variant") } } + +// A payload with no discriminator at all identifies nothing, so it must stay an +// error rather than masquerading as an unknown variant. +func TestMissingDiscriminatorIsAnError(t *testing.T) { + var v ShapeCollectionShapesValue + if err := json.Unmarshal([]byte(` + "`" + `{"radius":2.5}` + "`" + `), &v); err == nil { + t.Fatal("expected an error for a payload with no shapeType, got nil") + } +} + +func TestNullUnionDecodesToTheZeroValue(t *testing.T) { + var v ShapeCollectionShapesValue + if err := json.Unmarshal([]byte("null"), &v); err != nil { + t.Fatalf("null should decode as a no-op: %v", err) + } + if v.Value != nil || v.IsUnknownVariant() { + t.Errorf("null produced Value=%v unknown=%v, want the zero union", v.Value, v.IsUnknownVariant()) + } +} `) if err := os.WriteFile(filepath.Join(tmpDir, "union_runtime_test.go"), runtimeTest, 0o644); err != nil { t.Fatalf("writing runtime test: %v", err) diff --git a/internal/generator/funcmap.go b/internal/generator/funcmap.go index 5af17c1..f0a1a98 100644 --- a/internal/generator/funcmap.go +++ b/internal/generator/funcmap.go @@ -192,7 +192,6 @@ func jsonTag(f *ir.Field) string { return tag } -// catchAllField returns the synthetic additionalProperties field of a struct, if any. func catchAllField(td *ir.TypeDef) *ir.Field { if td.Kind != ir.TypeKindStruct { return nil @@ -205,13 +204,10 @@ func catchAllField(td *ir.TypeDef) *ir.Field { return nil } -// catchAllValueType returns the map value type of a catch-all field, e.g. "any" -// for a map[string]any. func catchAllValueType(f *ir.Field) string { return strings.TrimPrefix(f.Type, "map[string]") } -// declaredJSONNames returns the wire names of a struct's non-catch-all fields. func declaredJSONNames(td *ir.TypeDef) []string { var names []string for _, f := range td.Fields { @@ -223,7 +219,6 @@ func declaredJSONNames(td *ir.TypeDef) []string { return names } -// hasCatchAllTypes returns true if any type needs the additionalProperties marshalers. func hasCatchAllTypes(types []*ir.TypeDef) bool { return slices.ContainsFunc(types, func(td *ir.TypeDef) bool { return catchAllField(td) != nil diff --git a/internal/templates/types.go.tmpl b/internal/templates/types.go.tmpl index cacb291..dcefa51 100644 --- a/internal/templates/types.go.tmpl +++ b/internal/templates/types.go.tmpl @@ -6,9 +6,28 @@ import ( {{- if or (hasUnions .Types) (hasCatchAllTypes .Types) }} "encoding/json" "fmt" +{{- end }} +{{- if hasCatchAllTypes .Types }} + "strings" {{- end }} "time" ) +{{- if hasCatchAllTypes .Types }} + +// deleteDeclaredProperties drops the keys encoding/json already consumed into +// struct fields. The match is case-insensitive because that is the fallback +// encoding/json itself uses, so "Name" must not also land in the catch-all map. +func deleteDeclaredProperties(obj map[string]json.RawMessage, declared []string) { + for key := range obj { + for _, name := range declared { + if strings.EqualFold(key, name) { + delete(obj, key) + break + } + } + } +} +{{- end }} {{ range .Types }} {{ $typeDoc := typeDocComment . }}{{ if eq .Kind 0 }}{{ $typeName := .Name }}{{ $td := . }}{{ if $typeDoc }}{{ $typeDoc }} @@ -30,45 +49,47 @@ func (t {{ $typeName }}) MarshalJSON() ([]byte, error) { return data, nil } - obj := make(map[string]json.RawMessage, len(t.{{ .Name }})) + extra := make(map[string]json.RawMessage, len(t.{{ .Name }})) for key, value := range t.{{ .Name }} { raw, err := json.Marshal(value) if err != nil { return nil, fmt.Errorf("marshaling additional property %q: %w", key, err) } - obj[key] = raw + extra[key] = raw } - for _, name := range []string{ {{ range $i, $n := declaredJSONNames $td }}{{ if $i }}, {{ end }}{{ printf "%q" $n }}{{ end }} } { - delete(obj, name) + deleteDeclaredProperties(extra, []string{ {{ range $i, $n := declaredJSONNames $td }}{{ if $i }}, {{ end }}{{ printf "%q" $n }}{{ end }} }) + if len(extra) == 0 { + return data, nil } - - var declared map[string]json.RawMessage - if err := json.Unmarshal(data, &declared); err != nil { + encoded, err := json.Marshal(extra) + if err != nil { return nil, err } - for key, raw := range declared { - obj[key] = raw - } - return json.Marshal(obj) + + // Splice the two objects together so the declared properties keep their + // struct field order instead of being re-sorted through a map. + merged := make([]byte, 0, len(data)+len(encoded)) + merged = append(merged, data[:len(data)-1]...) + if len(data) > 2 { + merged = append(merged, ',') + } + return append(merged, encoded[1:]...), nil } // UnmarshalJSON implements json.Unmarshaler for {{ $typeName }}, collecting // properties the schema does not declare into {{ .Name }}. func (t *{{ $typeName }}) UnmarshalJSON(data []byte) error { type shadow {{ $typeName }} - var s shadow - if err := json.Unmarshal(data, &s); err != nil { + t.{{ .Name }} = nil + if err := json.Unmarshal(data, (*shadow)(t)); err != nil { return err } - *t = {{ $typeName }}(s) var obj map[string]json.RawMessage if err := json.Unmarshal(data, &obj); err != nil { return err } - for _, name := range []string{ {{ range $i, $n := declaredJSONNames $td }}{{ if $i }}, {{ end }}{{ printf "%q" $n }}{{ end }} } { - delete(obj, name) - } + deleteDeclaredProperties(obj, []string{ {{ range $i, $n := declaredJSONNames $td }}{{ if $i }}, {{ end }}{{ printf "%q" $n }}{{ end }} }) if len(obj) == 0 { return nil } @@ -134,6 +155,9 @@ func (u {{ .Name }}) MarshalJSON() ([]byte, error) { // UnmarshalJSON implements json.Unmarshaler for {{ .Name }}. func (u *{{ $typeName }}) UnmarshalJSON(data []byte) error { + if string(data) == "null" { + return nil + } {{- if .Discriminator }} var disc struct { {{ discriminatorFieldName .Discriminator.PropertyName }} string `json:"{{ .Discriminator.PropertyName }}"` @@ -150,6 +174,10 @@ func (u *{{ $typeName }}) UnmarshalJSON(data []byte) error { } *u = {{ $typeName }}{Value: v} return nil +{{- end }} +{{- if not (index .Discriminator.Mapping "") }} + case "": + return fmt.Errorf("unmarshaling {{ $typeName }}: missing {{ .Discriminator.PropertyName }} discriminator") {{- end }} default: // Adding a variant to a oneOf is meant to be a backward-compatible change,