diff --git a/internal/test/issues/issue-1189/issue1189.gen.go b/internal/test/issues/issue-1189/issue1189.gen.go index 0296cf0d9..630da8c4c 100644 --- a/internal/test/issues/issue-1189/issue1189.gen.go +++ b/internal/test/issues/issue-1189/issue1189.gen.go @@ -59,16 +59,16 @@ func (e TestFieldB) Valid() bool { // Defines values for TestFieldC1. const ( - Bar TestFieldC1 = "bar" - Foo TestFieldC1 = "foo" + TestFieldC1Bar TestFieldC1 = "bar" + TestFieldC1Foo TestFieldC1 = "foo" ) // Valid indicates whether the value is a known member of the TestFieldC1 enum. func (e TestFieldC1) Valid() bool { switch e { - case Bar: + case TestFieldC1Bar: return true - case Foo: + case TestFieldC1Foo: return true default: return false diff --git a/pkg/codegen/codegen.go b/pkg/codegen/codegen.go index 22317bda1..cd2b49fb7 100644 --- a/pkg/codegen/codegen.go +++ b/pkg/codegen/codegen.go @@ -1021,25 +1021,87 @@ func GenerateEnums(t *template.Template, types []TypeDefinition) (string, error) // Now, go through all the enums, and figure out if we have conflicts with // any others. - for i := range enums { - // Look through all other enums not compared so far. Make sure we don't - // compare against self. - e1 := enums[i] - for j := i + 1; j < len(enums); j++ { - e2 := enums[j] - - for e1key := range e1.GetValues() { - _, found := e2.GetValues()[e1key] - if found { - e1.PrefixTypeName = true - e2.PrefixTypeName = true - enums[i] = e1 - enums[j] = e2 - break + if globalState.options.Compatibility.OldEnumConflicts { + // Legacy behavior (pre-v2.7.1): compare generated constant names via + // GetValues(). This is order-dependent because GetValues() reflects + // already-applied prefixes, so enums processed later may miss conflicts + // with enums that were prefixed in an earlier iteration. Preserved here + // as an escape hatch for users who need to keep existing generated output. + for i := range enums { + e1 := enums[i] + for j := i + 1; j < len(enums); j++ { + e2 := enums[j] + for e1key := range e1.GetValues() { + if _, found := e2.GetValues()[e1key]; found { + e1.PrefixTypeName = true + e2.PrefixTypeName = true + enums[i] = e1 + enums[j] = e2 + break + } + } + } + } + } else { + // Stage 1: detect conflicts based on raw (unprefixed) values. This catches + // the case where two enums share the same value string (e.g. both have + // "running"), regardless of which order they appear. + for i := range enums { + e1 := enums[i] + for j := i + 1; j < len(enums); j++ { + e2 := enums[j] + for e1key := range e1.Schema.EnumValues { + if _, found := e2.Schema.EnumValues[e1key]; found { + e1.PrefixTypeName = true + e2.PrefixTypeName = true + enums[i] = e1 + enums[j] = e2 + break + } } } } + // Stage 2: iteratively detect effective-name conflicts. After Stage 1 some + // enums are now prefixed; their prefixed constant names (e.g. "Enum1One") + // may collide with another enum's raw constant name. We repeat until + // stable. + for { + changed := false + for i := range enums { + e1 := enums[i] + for j := i + 1; j < len(enums); j++ { + e2 := enums[j] + if e1.PrefixTypeName && e2.PrefixTypeName { + continue + } + for e1key := range e1.GetValues() { + if _, found := e2.GetValues()[e1key]; found { + if !e1.PrefixTypeName { + e1.PrefixTypeName = true + enums[i] = e1 + changed = true + } + if !e2.PrefixTypeName { + e2.PrefixTypeName = true + enums[j] = e2 + changed = true + } + break + } + } + } + } + if !changed { + break + } + } + } + + // Check each enum against global type names and self-conflict. + for i := range enums { + e1 := enums[i] + // now see if this enum conflicts with any global type names. for _, tp := range types { // Skip over enums, since we've handled those above. diff --git a/pkg/codegen/codegen_test.go b/pkg/codegen/codegen_test.go index 037c9e188..2ac29e93c 100644 --- a/pkg/codegen/codegen_test.go +++ b/pkg/codegen/codegen_test.go @@ -329,5 +329,135 @@ paths: assert.Contains(t, code, "roleName string") } +// TestEnumConflictDetectionOrderIndependent checks that conflict detection +// doesn't miss overlaps because an enum was already marked for prefixing. +func TestEnumConflictDetectionOrderIndependent(t *testing.T) { + // AState+BState share "running" (both prefixed), AState+CState share "migrating". + // The bug: once AState was marked, GetValues() returned prefixed names that + // no longer matched CState's raw values, so CState's conflict was missed. + const spec = ` +openapi: "3.0.0" +info: + version: 1.0.0 + title: Test Enum Conflict Detection +paths: {} +components: + schemas: + AState: + type: string + enum: + - running + - migrating + BState: + type: string + enum: + - running + CState: + type: string + enum: + - migrating +` + loader := openapi3.NewLoader() + swagger, err := loader.LoadFromData([]byte(spec)) + require.NoError(t, err) + + opts := Configuration{ + PackageName: "api", + Generate: GenerateOptions{ + Models: true, + }, + OutputOptions: OutputOptions{ + SkipPrune: true, + }, + } + + code, err := Generate(swagger, opts) + require.NoError(t, err) + + _, err = format.Source([]byte(code)) + require.NoError(t, err) + + // All three enums share values with at least one other enum; all must be prefixed. + assert.Contains(t, code, "AStateRunning") + assert.Contains(t, code, "AStateMigrating") + assert.Contains(t, code, "BStateRunning") + assert.Contains(t, code, "CStateMigrating") +} + +// TestEnumConflictDetectionBothOrders verifies that enum conflict detection +// produces identical output regardless of the order schemas appear in the spec. +// The old GetValues()-based approach was order-dependent: processing AState +// before CState caused CState to be left unprefixed, while reversing the order +// would prefix it. Go map iteration is non-deterministic (randomized since +// Go 1.12), so this was a latent correctness bug. +func TestEnumConflictDetectionBothOrders(t *testing.T) { + specAFirst := ` +openapi: "3.0.0" +info: + version: 1.0.0 + title: Test +paths: {} +components: + schemas: + AState: + type: string + enum: [running, migrating] + BState: + type: string + enum: [running] + CState: + type: string + enum: [migrating] +` + specCFirst := ` +openapi: "3.0.0" +info: + version: 1.0.0 + title: Test +paths: {} +components: + schemas: + CState: + type: string + enum: [migrating] + BState: + type: string + enum: [running] + AState: + type: string + enum: [running, migrating] +` + opts := Configuration{ + PackageName: "api", + Generate: GenerateOptions{Models: true}, + OutputOptions: OutputOptions{ + SkipPrune: true, + }, + } + + loader := openapi3.NewLoader() + + swaggerA, err := loader.LoadFromData([]byte(specAFirst)) + require.NoError(t, err) + codeA, err := Generate(swaggerA, opts) + require.NoError(t, err) + + swaggerC, err := loader.LoadFromData([]byte(specCFirst)) + require.NoError(t, err) + codeC, err := Generate(swaggerC, opts) + require.NoError(t, err) + + // Both orderings must produce fully prefixed constants. + for _, code := range []string{codeA, codeC} { + assert.Contains(t, code, "AStateRunning") + assert.Contains(t, code, "AStateMigrating") + assert.Contains(t, code, "BStateRunning") + assert.Contains(t, code, "CStateMigrating") + // Unprefixed names must not appear as standalone constants. + assert.NotContains(t, code, "\tRunning ") + assert.NotContains(t, code, "\tMigrating ") + } +} + //go:embed test_spec.yaml var testOpenAPIDefinition string diff --git a/pkg/codegen/configuration.go b/pkg/codegen/configuration.go index dbcb0a687..4dc953cfc 100644 --- a/pkg/codegen/configuration.go +++ b/pkg/codegen/configuration.go @@ -231,8 +231,13 @@ type CompatibilityOptions struct { // Enum values can generate conflicting typenames, so we've updated the // code for enum generation to avoid these conflicts, but it will result // in some enum types being renamed in existing code. Set OldEnumConflicts to true - // to revert to old behavior. Please see: + // to revert to old behavior. + // As of v2.7.1, this also reverts the order-independent conflict detection + // fix (issue #2391): the legacy GetValues()-based path is order-dependent + // and may produce different output on different runs, but is provided as + // an escape hatch for users who need to preserve existing generated code. // Please see https://github.com/oapi-codegen/oapi-codegen/issues/549 + // Please see https://github.com/oapi-codegen/oapi-codegen/issues/2391 OldEnumConflicts bool `yaml:"old-enum-conflicts,omitempty"` // It was a mistake to generate a go type definition for every $ref in // the OpenAPI schema. New behavior uses type aliases where possible, but @@ -299,6 +304,7 @@ type CompatibilityOptions struct { // are treated as required. // Please see https://github.com/oapi-codegen/oapi-codegen/issues/2267 HeadersImplicitlyRequired bool `yaml:"headers-implicitly-required,omitempty"` + } func (co CompatibilityOptions) Validate() map[string]string {