fix: valid specs lose data, duplicate keys, or fail to compile

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
 

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 <Op>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)

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"

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))
 }

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)

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