From fdc6c6a6ebf3f65241d9809134044283ff6e12dc Mon Sep 17 00:00:00 2001 From: Marcin Romaszewicz Date: Thu, 16 Jul 2026 10:29:31 -0700 Subject: [PATCH] Fix ValueByDiscriminator() with external refs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes: #2470 When a discriminated `oneOf` union is defined in an external file and pulled into an `allOf` from another package via `import-mapping`, the generated `ValueByDiscriminator()` interpolated the raw discriminator mapping value (e.g. `externalRef0.GitDiffFile`) directly into the method name, producing invalid Go: `t.AsexternalRef0.GitDiffFile()`. The fix computes the switch arms in Go rather than in the template. A new `Schema.DiscriminatorCases()` helper maps each discriminator value to its union element's `Method()` — the same name used by the generated `As*`/`From*`/`Merge*` helpers — so the call becomes `t.AsExternalRef0GitDiffFile()`. `union.tmpl` now just ranges over those precomputed `{Value, Method}` cases, replacing the nested `range UnionElements` / `eq (print $el)` matching logic. Local-ref unions are unaffected: the element `Method` equals the mapping value there, so existing generated code is byte-identical. Adds a regression fixture under `internal/test/references/multipackage/discriminated_union_allof` that reproduces the two-file, import-mapped, discriminated-union-in-allOf case and exercises `ValueByDiscriminator()` end-to-end. --- .../common/spec.yaml | 31 ++++ .../discriminated_union_allof/config.api.yaml | 9 + .../config.common.yaml | 7 + .../discriminated_union_allof/doc.go | 4 + .../gen/api/api.gen.go | 159 ++++++++++++++++++ .../gen/common/common.gen.go | 129 ++++++++++++++ .../discriminated_union_allof/issue_test.go | 39 +++++ .../discriminated_union_allof/spec.yaml | 33 ++++ pkg/codegen/schema.go | 34 ++++ pkg/codegen/templates/union.tmpl | 9 +- 10 files changed, 450 insertions(+), 4 deletions(-) create mode 100644 internal/test/references/multipackage/discriminated_union_allof/common/spec.yaml create mode 100644 internal/test/references/multipackage/discriminated_union_allof/config.api.yaml create mode 100644 internal/test/references/multipackage/discriminated_union_allof/config.common.yaml create mode 100644 internal/test/references/multipackage/discriminated_union_allof/doc.go create mode 100644 internal/test/references/multipackage/discriminated_union_allof/gen/api/api.gen.go create mode 100644 internal/test/references/multipackage/discriminated_union_allof/gen/common/common.gen.go create mode 100644 internal/test/references/multipackage/discriminated_union_allof/issue_test.go create mode 100644 internal/test/references/multipackage/discriminated_union_allof/spec.yaml diff --git a/internal/test/references/multipackage/discriminated_union_allof/common/spec.yaml b/internal/test/references/multipackage/discriminated_union_allof/common/spec.yaml new file mode 100644 index 000000000..6bfcfbfa4 --- /dev/null +++ b/internal/test/references/multipackage/discriminated_union_allof/common/spec.yaml @@ -0,0 +1,31 @@ +# issue-2470: an external file defines a discriminated oneOf union that is later +# pulled into an allOf from another package via import-mapping. +openapi: "3.0.4" +info: + title: Diff + version: "0.0.1" +paths: {} +components: + schemas: + GitDiffFile: + type: object + properties: + diff_type: + type: string + patch: + type: string + required: [diff_type] + AddedImageDiffFile: + type: object + properties: + diff_type: + type: string + image_url: + type: string + required: [diff_type] + DiffFile: + oneOf: + - $ref: "#/components/schemas/GitDiffFile" + - $ref: "#/components/schemas/AddedImageDiffFile" + discriminator: + propertyName: diff_type diff --git a/internal/test/references/multipackage/discriminated_union_allof/config.api.yaml b/internal/test/references/multipackage/discriminated_union_allof/config.api.yaml new file mode 100644 index 000000000..8dc6e6b31 --- /dev/null +++ b/internal/test/references/multipackage/discriminated_union_allof/config.api.yaml @@ -0,0 +1,9 @@ +# yaml-language-server: $schema=../../../../../configuration-schema.json +package: api +generate: + models: true +import-mapping: + ./common/spec.yaml: github.com/oapi-codegen/oapi-codegen/v2/internal/test/references/multipackage/discriminated_union_allof/gen/common +output: gen/api/api.gen.go +output-options: + skip-prune: true diff --git a/internal/test/references/multipackage/discriminated_union_allof/config.common.yaml b/internal/test/references/multipackage/discriminated_union_allof/config.common.yaml new file mode 100644 index 000000000..9c51eaa7d --- /dev/null +++ b/internal/test/references/multipackage/discriminated_union_allof/config.common.yaml @@ -0,0 +1,7 @@ +# yaml-language-server: $schema=../../../../../configuration-schema.json +package: common +generate: + models: true +output: gen/common/common.gen.go +output-options: + skip-prune: true diff --git a/internal/test/references/multipackage/discriminated_union_allof/doc.go b/internal/test/references/multipackage/discriminated_union_allof/doc.go new file mode 100644 index 000000000..9c0943f1f --- /dev/null +++ b/internal/test/references/multipackage/discriminated_union_allof/doc.go @@ -0,0 +1,4 @@ +package discriminatedunionallof + +//go:generate go run github.com/oapi-codegen/oapi-codegen/v2/cmd/oapi-codegen --config=config.common.yaml common/spec.yaml +//go:generate go run github.com/oapi-codegen/oapi-codegen/v2/cmd/oapi-codegen --config=config.api.yaml spec.yaml diff --git a/internal/test/references/multipackage/discriminated_union_allof/gen/api/api.gen.go b/internal/test/references/multipackage/discriminated_union_allof/gen/api/api.gen.go new file mode 100644 index 000000000..36c7636e6 --- /dev/null +++ b/internal/test/references/multipackage/discriminated_union_allof/gen/api/api.gen.go @@ -0,0 +1,159 @@ +// Package api provides primitives to interact with the openapi HTTP API. +// +// Code generated by github.com/oapi-codegen/oapi-codegen/v2 version v2.0.0-00010101000000-000000000000 DO NOT EDIT. +package api + +import ( + "encoding/json" + "errors" + "fmt" + + externalRef0 "github.com/oapi-codegen/oapi-codegen/v2/internal/test/references/multipackage/discriminated_union_allof/gen/common" + "github.com/oapi-codegen/runtime" +) + +// PRFile defines model for PRFile. +type PRFile struct { + Viewed *bool `json:"viewed,omitempty"` + union json.RawMessage +} + +// ViewedStatus defines model for ViewedStatus. +type ViewedStatus struct { + Viewed *bool `json:"viewed,omitempty"` +} + +// AsExternalRef0GitDiffFile returns the union data inside the PRFile as a externalRef0.GitDiffFile +func (t PRFile) AsExternalRef0GitDiffFile() (externalRef0.GitDiffFile, error) { + var body externalRef0.GitDiffFile + err := json.Unmarshal(t.union, &body) + return body, err +} + +// FromExternalRef0GitDiffFile overwrites any union data inside the PRFile as the provided externalRef0.GitDiffFile +func (t *PRFile) FromExternalRef0GitDiffFile(v externalRef0.GitDiffFile) error { + b, err := json.Marshal(v) + if err != nil { + return err + } + b, err = runtime.JSONMerge(b, []byte(`{"diff_type":"GitDiffFile"}`)) + t.union = b + return err +} + +// MergeExternalRef0GitDiffFile performs a merge with any union data inside the PRFile, using the provided externalRef0.GitDiffFile +func (t *PRFile) MergeExternalRef0GitDiffFile(v externalRef0.GitDiffFile) error { + b, err := json.Marshal(v) + if err != nil { + return err + } + b, err = runtime.JSONMerge(b, []byte(`{"diff_type":"GitDiffFile"}`)) + if err != nil { + return err + } + + merged, err := runtime.JSONMerge(t.union, b) + t.union = merged + return err +} + +// AsExternalRef0AddedImageDiffFile returns the union data inside the PRFile as a externalRef0.AddedImageDiffFile +func (t PRFile) AsExternalRef0AddedImageDiffFile() (externalRef0.AddedImageDiffFile, error) { + var body externalRef0.AddedImageDiffFile + err := json.Unmarshal(t.union, &body) + return body, err +} + +// FromExternalRef0AddedImageDiffFile overwrites any union data inside the PRFile as the provided externalRef0.AddedImageDiffFile +func (t *PRFile) FromExternalRef0AddedImageDiffFile(v externalRef0.AddedImageDiffFile) error { + b, err := json.Marshal(v) + if err != nil { + return err + } + b, err = runtime.JSONMerge(b, []byte(`{"diff_type":"AddedImageDiffFile"}`)) + t.union = b + return err +} + +// MergeExternalRef0AddedImageDiffFile performs a merge with any union data inside the PRFile, using the provided externalRef0.AddedImageDiffFile +func (t *PRFile) MergeExternalRef0AddedImageDiffFile(v externalRef0.AddedImageDiffFile) error { + b, err := json.Marshal(v) + if err != nil { + return err + } + b, err = runtime.JSONMerge(b, []byte(`{"diff_type":"AddedImageDiffFile"}`)) + if err != nil { + return err + } + + merged, err := runtime.JSONMerge(t.union, b) + t.union = merged + return err +} + +func (t PRFile) Discriminator() (string, error) { + var discriminator struct { + Discriminator string `json:"diff_type"` + } + err := json.Unmarshal(t.union, &discriminator) + return discriminator.Discriminator, err +} + +func (t PRFile) ValueByDiscriminator() (interface{}, error) { + discriminator, err := t.Discriminator() + if err != nil { + return nil, err + } + switch discriminator { + case "AddedImageDiffFile": + return t.AsExternalRef0AddedImageDiffFile() + case "GitDiffFile": + return t.AsExternalRef0GitDiffFile() + default: + return nil, errors.New("unknown discriminator value: " + discriminator) + } +} + +func (t PRFile) MarshalJSON() ([]byte, error) { + b, err := t.union.MarshalJSON() + if err != nil { + return nil, err + } + object := make(map[string]json.RawMessage) + if t.union != nil { + err = json.Unmarshal(b, &object) + if err != nil { + return nil, err + } + } + + if t.Viewed != nil { + object["viewed"], err = json.Marshal(t.Viewed) + if err != nil { + return nil, fmt.Errorf("error marshaling 'viewed': %w", err) + } + } + b, err = json.Marshal(object) + return b, err +} + +func (t *PRFile) UnmarshalJSON(b []byte) error { + err := t.union.UnmarshalJSON(b) + if err != nil { + return err + } + object := make(map[string]json.RawMessage) + err = json.Unmarshal(b, &object) + if err != nil { + return err + } + + if raw, found := object["viewed"]; found { + err = json.Unmarshal(raw, &t.Viewed) + if err != nil { + return fmt.Errorf("error reading 'viewed': %w", err) + } + } + + return err +} diff --git a/internal/test/references/multipackage/discriminated_union_allof/gen/common/common.gen.go b/internal/test/references/multipackage/discriminated_union_allof/gen/common/common.gen.go new file mode 100644 index 000000000..f2e2137f6 --- /dev/null +++ b/internal/test/references/multipackage/discriminated_union_allof/gen/common/common.gen.go @@ -0,0 +1,129 @@ +// Package common provides primitives to interact with the openapi HTTP API. +// +// Code generated by github.com/oapi-codegen/oapi-codegen/v2 version v2.0.0-00010101000000-000000000000 DO NOT EDIT. +package common + +import ( + "encoding/json" + "errors" + + "github.com/oapi-codegen/runtime" +) + +// AddedImageDiffFile defines model for AddedImageDiffFile. +type AddedImageDiffFile struct { + DiffType string `json:"diff_type"` + ImageUrl *string `json:"image_url,omitempty"` +} + +// DiffFile defines model for DiffFile. +type DiffFile struct { + union json.RawMessage +} + +// GitDiffFile defines model for GitDiffFile. +type GitDiffFile struct { + DiffType string `json:"diff_type"` + Patch *string `json:"patch,omitempty"` +} + +// AsGitDiffFile returns the union data inside the DiffFile as a GitDiffFile +func (t DiffFile) AsGitDiffFile() (GitDiffFile, error) { + var body GitDiffFile + err := json.Unmarshal(t.union, &body) + return body, err +} + +// FromGitDiffFile overwrites any union data inside the DiffFile as the provided GitDiffFile +func (t *DiffFile) FromGitDiffFile(v GitDiffFile) error { + b, err := json.Marshal(v) + if err != nil { + return err + } + b, err = runtime.JSONMerge(b, []byte(`{"diff_type":"GitDiffFile"}`)) + t.union = b + return err +} + +// MergeGitDiffFile performs a merge with any union data inside the DiffFile, using the provided GitDiffFile +func (t *DiffFile) MergeGitDiffFile(v GitDiffFile) error { + b, err := json.Marshal(v) + if err != nil { + return err + } + b, err = runtime.JSONMerge(b, []byte(`{"diff_type":"GitDiffFile"}`)) + if err != nil { + return err + } + + merged, err := runtime.JSONMerge(t.union, b) + t.union = merged + return err +} + +// AsAddedImageDiffFile returns the union data inside the DiffFile as a AddedImageDiffFile +func (t DiffFile) AsAddedImageDiffFile() (AddedImageDiffFile, error) { + var body AddedImageDiffFile + err := json.Unmarshal(t.union, &body) + return body, err +} + +// FromAddedImageDiffFile overwrites any union data inside the DiffFile as the provided AddedImageDiffFile +func (t *DiffFile) FromAddedImageDiffFile(v AddedImageDiffFile) error { + b, err := json.Marshal(v) + if err != nil { + return err + } + b, err = runtime.JSONMerge(b, []byte(`{"diff_type":"AddedImageDiffFile"}`)) + t.union = b + return err +} + +// MergeAddedImageDiffFile performs a merge with any union data inside the DiffFile, using the provided AddedImageDiffFile +func (t *DiffFile) MergeAddedImageDiffFile(v AddedImageDiffFile) error { + b, err := json.Marshal(v) + if err != nil { + return err + } + b, err = runtime.JSONMerge(b, []byte(`{"diff_type":"AddedImageDiffFile"}`)) + if err != nil { + return err + } + + merged, err := runtime.JSONMerge(t.union, b) + t.union = merged + return err +} + +func (t DiffFile) Discriminator() (string, error) { + var discriminator struct { + Discriminator string `json:"diff_type"` + } + err := json.Unmarshal(t.union, &discriminator) + return discriminator.Discriminator, err +} + +func (t DiffFile) ValueByDiscriminator() (interface{}, error) { + discriminator, err := t.Discriminator() + if err != nil { + return nil, err + } + switch discriminator { + case "AddedImageDiffFile": + return t.AsAddedImageDiffFile() + case "GitDiffFile": + return t.AsGitDiffFile() + default: + return nil, errors.New("unknown discriminator value: " + discriminator) + } +} + +func (t DiffFile) MarshalJSON() ([]byte, error) { + b, err := t.union.MarshalJSON() + return b, err +} + +func (t *DiffFile) UnmarshalJSON(b []byte) error { + err := t.union.UnmarshalJSON(b) + return err +} diff --git a/internal/test/references/multipackage/discriminated_union_allof/issue_test.go b/internal/test/references/multipackage/discriminated_union_allof/issue_test.go new file mode 100644 index 000000000..3ada93851 --- /dev/null +++ b/internal/test/references/multipackage/discriminated_union_allof/issue_test.go @@ -0,0 +1,39 @@ +package discriminatedunionallof + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/oapi-codegen/oapi-codegen/v2/internal/test/references/multipackage/discriminated_union_allof/gen/api" + "github.com/oapi-codegen/oapi-codegen/v2/internal/test/references/multipackage/discriminated_union_allof/gen/common" +) + +// TestValueByDiscriminatorExternalRef verifies that PRFile.ValueByDiscriminator() +// dispatches to the correct As* helper when the discriminated union is pulled in +// from another package via an external $ref inside an allOf. +// +// Before the fix, ValueByDiscriminator() interpolated the raw discriminator +// mapping value (externalRef0.GitDiffFile) into the method name, generating +// t.AsexternalRef0.GitDiffFile() — which does not compile. This package simply +// existing (its generated code compiling) is the primary regression guard; the +// round-trip below additionally exercises the dispatch at runtime. +// +// See https://github.com/oapi-codegen/oapi-codegen/issues/2470 +func TestValueByDiscriminatorExternalRef(t *testing.T) { + patch := "@@ -1 +1 @@" + var f api.PRFile + require.NoError(t, f.FromExternalRef0GitDiffFile(common.GitDiffFile{ + DiffType: "GitDiffFile", + Patch: &patch, + })) + + v, err := f.ValueByDiscriminator() + require.NoError(t, err) + + got, ok := v.(common.GitDiffFile) + require.True(t, ok, "expected common.GitDiffFile, got %T", v) + require.NotNil(t, got.Patch) + assert.Equal(t, patch, *got.Patch) +} diff --git a/internal/test/references/multipackage/discriminated_union_allof/spec.yaml b/internal/test/references/multipackage/discriminated_union_allof/spec.yaml new file mode 100644 index 000000000..0b358a178 --- /dev/null +++ b/internal/test/references/multipackage/discriminated_union_allof/spec.yaml @@ -0,0 +1,33 @@ +# issue-2470: PRFile is an allOf combining an external discriminated union +# (DiffFile, from ./common/spec.yaml) with a local object (ViewedStatus). The +# generated PRFile.ValueByDiscriminator() must call the As* helper whose name +# matches the union element's Method (AsExternalRef0GitDiffFile), not the raw +# discriminator mapping value (externalRef0.GitDiffFile), which is invalid Go. +openapi: "3.0.4" +info: + title: Pulls + version: "0.0.1" +paths: + /files: + get: + operationId: listFiles + responses: + "200": + description: OK + content: + application/json: + schema: + type: array + items: + $ref: "#/components/schemas/PRFile" +components: + schemas: + ViewedStatus: + type: object + properties: + viewed: + type: boolean + PRFile: + allOf: + - $ref: "./common/spec.yaml#/components/schemas/DiffFile" + - $ref: "#/components/schemas/ViewedStatus" diff --git a/pkg/codegen/schema.go b/pkg/codegen/schema.go index 0972159cc..e8d6500db 100644 --- a/pkg/codegen/schema.go +++ b/pkg/codegen/schema.go @@ -432,6 +432,40 @@ func (s Schema) DiscriminatorStampFor(element UnionElement) *DiscriminatorStamp return &stamp } +// DiscriminatorCase is one arm of a union's ValueByDiscriminator() switch: a +// discriminator value and the As* helper it dispatches to. +type DiscriminatorCase struct { + // Value is the discriminator value that selects this element, e.g. "GitDiffFile". + Value string + // Method is the union element method suffix, e.g. "ExternalRef0GitDiffFile", + // so the generated call is t.As(). + Method string +} + +// DiscriminatorCases returns the ValueByDiscriminator() switch arms for the +// union, sorted by discriminator value for deterministic output. Each mapping +// value is the Go type of a union element (generateUnion writes both from the +// same elementSchema.GoType), so it resolves to that element's Method(). A +// mapping value not found among the union elements is skipped: it has no As* +// helper to dispatch to, so no case is emitted and it falls through to the +// switch default. +func (s Schema) DiscriminatorCases() []DiscriminatorCase { + if s.Discriminator == nil { + return nil + } + known := make(map[string]UnionElement, len(s.UnionElements)) + for _, el := range s.UnionElements { + known[el.String()] = el + } + cases := make([]DiscriminatorCase, 0, len(s.Discriminator.Mapping)) + for _, value := range SortedMapKeys(s.Discriminator.Mapping) { + if el, ok := known[s.Discriminator.Mapping[value]]; ok { + cases = append(cases, DiscriminatorCase{Value: value, Method: el.Method()}) + } + } + return cases +} + // UnionElement describe union element, based on prefix externalRef\d+ and real ref name from external schema. type UnionElement string diff --git a/pkg/codegen/templates/union.tmpl b/pkg/codegen/templates/union.tmpl index a867c3b8e..a97ba863e 100644 --- a/pkg/codegen/templates/union.tmpl +++ b/pkg/codegen/templates/union.tmpl @@ -67,16 +67,17 @@ return discriminator.Discriminator, err } - {{if ne 0 (len $discriminator.Mapping)}} + {{$cases := $schema.DiscriminatorCases -}} + {{if $cases}} func (t {{.TypeName}}) ValueByDiscriminator() (interface{}, error) { discriminator, err := t.Discriminator() if err != nil { return nil, err } switch discriminator{ - {{range $value, $type := $discriminator.Mapping -}} - case "{{$value}}": - return t.As{{$type}}() + {{range $cases -}} + case "{{.Value}}": + return t.As{{.Method}}() {{end -}} default: return nil, errors.New("unknown discriminator value: "+discriminator)