From 1f7c42abf1894ff07514ed9b6ba859ebf1031684 Mon Sep 17 00:00:00 2001 From: Frederic BIDON Date: Sun, 21 Jun 2026 17:40:16 +0200 Subject: [PATCH] feat(schema): optional $ref-sibling rendering (EmitRefSiblings, SkipAllOfCompounding) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A struct field whose Go type resolves to a named model becomes a $ref. Strict JSON-Schema-draft-4 ignores siblings of a $ref, so codescan preserves a field's description / extensions / validations by wrapping the reference in an allOf compound. That shape is correct but breaks downstream consumers (e.g. go-swagger codegen) that expect a bare $ref. Two new Options give callers control, splitting $ref siblings into two classes — siblings-eligible (description, extensions) and compound-only (validations, externalDocs): - EmitRefSiblings: emit description & extensions as direct $ref siblings ({$ref, description, x-*}) instead of an allOf wrap. A validation or externalDocs still forces a compound. - SkipAllOfCompounding: never emit an allOf compound. Validations & externalDocs are dropped; description & extensions too, unless EmitRefSiblings keeps them as siblings. Each drop raises one validate.dropped-ref-sibling diagnostic via OnDiagnostic. DescWithRef is deprecated in favour of EmitRefSiblings but kept with its original semantics (a no-op when EmitRefSiblings is set). All defaults are unchanged — zero golden drift. `required:` is always preserved (a parent-side concern, not a $ref sibling). Includes the builder logic + diagnostic code, unit A/B tests, a tested doc-site example (docs/examples/shaping/refsiblings), and doc updates (schema README, doc-site options table + "Descriptions beside a $ref"). Co-Authored-By: Claude Opus 4.8 (1M context) Signed-off-by: Frederic BIDON --- .claude/CLAUDE.md | 10 +- .../getting-started/usage-as-a-library.md | 5 +- .../descriptions-beside-a-ref.md | 119 +++++++++-- .../shaping/refsiblings/refsiblings.go | 32 +++ .../shaping/refsiblings/refsiblings_test.go | 72 +++++++ .../shaping/refsiblings/testdata/default.json | 10 + .../refsiblings/testdata/siblings.json | 5 + .../shaping/refsiblings/testdata/skip.json | 3 + internal/builders/schema/README.md | 65 ++++-- internal/builders/schema/schema_test.go | 201 ++++++++++++++++++ internal/builders/schema/walker.go | 137 +++++++++--- internal/parsers/grammar/diagnostic.go | 9 + internal/scanner/options.go | 53 ++++- internal/scanner/scan_context.go | 8 + 14 files changed, 666 insertions(+), 63 deletions(-) create mode 100644 docs/examples/shaping/refsiblings/refsiblings.go create mode 100644 docs/examples/shaping/refsiblings/refsiblings_test.go create mode 100644 docs/examples/shaping/refsiblings/testdata/default.json create mode 100644 docs/examples/shaping/refsiblings/testdata/siblings.json create mode 100644 docs/examples/shaping/refsiblings/testdata/skip.json diff --git a/.claude/CLAUDE.md b/.claude/CLAUDE.md index 16f14986..9110bdb9 100644 --- a/.claude/CLAUDE.md +++ b/.claude/CLAUDE.md @@ -116,7 +116,15 @@ malformed input, the petstore, aliased schemas, go123-specific forms, and cross- - `ScanModels` — also emit definitions for `swagger:model` types. - `InputSpec` — overlay: merge discoveries on top of an existing spec. - `BuildTags`, `Include`/`Exclude`, `IncludeTags`/`ExcludeTags`, `ExcludeDeps` — scope control. - - `RefAliases`, `TransparentAliases`, `DescWithRef` — alias handling knobs. + - `RefAliases`, `TransparentAliases`, `DescWithRef` — alias handling knobs + (`DescWithRef` is deprecated; see `EmitRefSiblings`). + - `$ref`-sibling rendering (see `internal/builders/schema/README.md#ref-override`): + - `EmitRefSiblings` — emit a `$ref`'d field's description & extensions as direct + siblings (`{$ref, description, x-*}`) instead of an `allOf` wrap; validations/ + externalDocs still force a compound. + - `SkipAllOfCompounding` — never emit an `allOf` compound; validations/externalDocs + dropped (description/extensions too, unless `EmitRefSiblings` keeps them as + siblings), each with a diagnostic. For consumers (e.g. go-swagger) wanting bare refs. - `SetXNullableForPointers` — emit `x-nullable: true` on pointer fields. - `SkipExtensions` — suppress `x-go-*` vendor extensions. - `Debug` — verbose logging via `internal/logger`. diff --git a/docs/doc-site/getting-started/usage-as-a-library.md b/docs/doc-site/getting-started/usage-as-a-library.md index 9c6d404b..b47d7d80 100644 --- a/docs/doc-site/getting-started/usage-as-a-library.md +++ b/docs/doc-site/getting-started/usage-as-a-library.md @@ -47,7 +47,10 @@ an existing spec via `Options.InputSpec`. | `ScanModels` | Also emit definitions for `swagger:model` types. | | `InputSpec` | Overlay: merge discoveries on top of an existing spec. | | `BuildTags`, `Include`/`Exclude` | Scope control over what gets scanned. | -| `RefAliases`, `TransparentAliases`, `DescWithRef` | Alias-handling knobs. | +| `RefAliases`, `TransparentAliases` | Alias-handling knobs. | +| `EmitRefSiblings` | Emit a `$ref`'d field's description and extensions as direct `$ref` siblings instead of an `allOf` wrap — see [Descriptions beside a $ref]({{% relref "/shaping-the-output/descriptions-beside-a-ref" %}}). | +| `SkipAllOfCompounding` | Never wrap a `$ref`'d field in an `allOf`; emit a bare `$ref` and drop the decorations that need a compound — see [Descriptions beside a $ref]({{% relref "/shaping-the-output/descriptions-beside-a-ref" %}}). | +| `DescWithRef` | _Deprecated_ — preserve a description-only `$ref` field via a single-arm `allOf`; prefer `EmitRefSiblings`. | | `SkipExtensions` | Suppress `x-go-*` vendor extensions. | | `SkipEnumDescriptions` | Keep the `swagger:enum` const→value mapping out of property/parameter descriptions (it still rides `x-go-enum-desc`). | | `EmitXGoType` | Stamp `x-go-type` (the fully-qualified Go type) on every definition — see [Vendor extensions]({{% relref "/shaping-the-output/vendor-extensions#stamping-x-go-type" %}}). | diff --git a/docs/doc-site/shaping-the-output/descriptions-beside-a-ref.md b/docs/doc-site/shaping-the-output/descriptions-beside-a-ref.md index 17c1cb88..a7efdc92 100644 --- a/docs/doc-site/shaping-the-output/descriptions-beside-a-ref.md +++ b/docs/doc-site/shaping-the-output/descriptions-beside-a-ref.md @@ -2,34 +2,117 @@ title: Descriptions beside a $ref weight: 50 description: | - Keep a field's description when its type resolves to a $ref, by wrapping the - reference in a single-arm allOf (DescWithRef). + Control how a field's description and extensions are rendered when its type + resolves to a $ref — wrapped in an allOf, emitted as direct siblings + (EmitRefSiblings), or dropped (SkipAllOfCompounding). --- -When a struct field's only decoration is a description and its Go type resolves -to a named model (a `$ref`), JSON Schema draft 4 cannot carry a sibling -`description` next to a `$ref`. `Options.DescWithRef` decides what happens to -that description. +When a struct field's Go type resolves to a named model, the field becomes a +`$ref`. Strict JSON Schema draft 4 (the dialect OpenAPI 2.0 is built on) says a +`$ref` *replaces* its siblings — so a `description`, a validation, or an `x-*` +extension written on that field cannot simply sit next to the `$ref`. -{{< code file="shaping/descref/descref.go" lang="go" region="model" >}} +codescan's default is to preserve those decorations by wrapping the reference in +an **`allOf` compound**, which is the draft-4-correct shape. Three options tune +this behaviour. The decorations split into two classes: -By default the description is dropped (a bare `$ref`); with `DescWithRef` it is -preserved by wrapping the `$ref` in a single-arm `allOf`: +- **description & extensions** — *siblings-eligible*: modern tooling (OpenAPI + 3.1 / JSON Schema 2020-12, most Swagger-UI renderers) reads them directly + beside a `$ref`. +- **validations & `externalDocs`** — *compound-only*: they have no valid + bare-`$ref` form, so they can only ride an `allOf` compound. -{{< compare left="shaping/descref/testdata/off.json" leftlabel="Default — description dropped" - right="shaping/descref/testdata/on.json" rightlabel="DescWithRef: true" >}} +{{< code file="shaping/refsiblings/refsiblings.go" lang="go" region="model" >}} + +## The default — an `allOf` wrapper + +With no options set, the field's description and extension are preserved by +wrapping the `$ref` as the single member of an `allOf`; the decorations ride the +outer schema: + +{{< code file="shaping/refsiblings/testdata/default.json" lang="json" >}} + +This is the always-correct shape and needs no configuration — see also +[Decorating a `$ref`]({{% relref "/tutorials/model-definitions" %}}) in the +Model definitions tutorial. + +## Emit siblings directly — `EmitRefSiblings` + +Set `Options.EmitRefSiblings` to render the description and extensions as +**direct siblings** of the `$ref`, with no `allOf` wrapper — the leaner shape +modern tools expect: ```go codescan.Run(&codescan.Options{ - Packages: []string{"./..."}, - ScanModels: true, - DescWithRef: true, + Packages: []string{"./..."}, + ScanModels: true, + EmitRefSiblings: true, }) ``` +{{< compare left="shaping/refsiblings/testdata/default.json" leftlabel="Default — allOf wrapper" + right="shaping/refsiblings/testdata/siblings.json" rightlabel="EmitRefSiblings: true" >}} + {{% notice style="info" %}} -When the field carries **more** than a description — a validation override or a -user-authored extension — the `allOf` wrapper is emitted **regardless** of this -flag, because the override would otherwise be lost. `DescWithRef` only governs -the description-only case. +`EmitRefSiblings` only changes the cases where nothing else forces a compound. +When the field also carries a **validation** or **`externalDocs`** (which cannot +live beside a bare `$ref`), the `allOf` wrapper is still emitted and the +description / extensions ride its outer schema. +{{% /notice %}} + +## Drop the compound entirely — `SkipAllOfCompounding` + +Some downstream consumers — notably go-swagger's code generator — expect a field +that points at a model to be a **bare `$ref`** and do not handle the +`allOf`-compounded shape. Set `Options.SkipAllOfCompounding` to never emit an +`allOf` compound: + +```go +codescan.Run(&codescan.Options{ + Packages: []string{"./..."}, + ScanModels: true, + SkipAllOfCompounding: true, +}) +``` + +No compound is produced, so validations and `externalDocs` are dropped, and the +description and extension go with them — leaving a bare `$ref`: + +{{< code file="shaping/refsiblings/testdata/skip.json" lang="json" >}} + +Every dropped decoration is reported through `Options.OnDiagnostic` (code +`validate.dropped-ref-sibling`), so the loss is never silent. Combine it with +`EmitRefSiblings` to keep the description and extensions as siblings while still +dropping the compound-only validations: + +```go +codescan.Run(&codescan.Options{ + Packages: []string{"./..."}, + ScanModels: true, + EmitRefSiblings: true, // keep description / x-* as $ref siblings + SkipAllOfCompounding: true, // drop validations / externalDocs, no allOf +}) +``` + +{{% notice style="note" %}} +`required:` is never affected by any of these options. It is a property of the +*parent* object (it lands in the parent's `required` list), not a sibling of the +`$ref`, so it is always preserved. +{{% /notice %}} + +## `DescWithRef` (deprecated) + +`Options.DescWithRef` predates `EmitRefSiblings` and covers only the narrow +**description-only** case: a `$ref`'d field whose sole decoration is a +description. By default that description is dropped; `DescWithRef` preserves it +by wrapping the `$ref` in a single-arm `allOf`. + +{{< compare left="shaping/descref/testdata/off.json" leftlabel="Default — description dropped" + right="shaping/descref/testdata/on.json" rightlabel="DescWithRef: true" >}} + +{{% notice style="warning" %}} +`DescWithRef` is **deprecated** — prefer `EmitRefSiblings`, which preserves both +descriptions **and** extensions (as direct siblings). `DescWithRef` keeps its +original behaviour for compatibility and is a no-op when `EmitRefSiblings` is +set. {{% /notice %}} diff --git a/docs/examples/shaping/refsiblings/refsiblings.go b/docs/examples/shaping/refsiblings/refsiblings.go new file mode 100644 index 00000000..05c35bd8 --- /dev/null +++ b/docs/examples/shaping/refsiblings/refsiblings.go @@ -0,0 +1,32 @@ +// SPDX-License-Identifier: Apache-2.0 + +// Package refsiblings holds the annotated declarations used by the "$ref +// siblings" section of the "Descriptions beside a $ref" how-to. +// refsiblings_test.go scans it under the default, EmitRefSiblings and +// SkipAllOfCompounding options and writes the golden fragments the page renders. +package refsiblings + +// snippet:model + +// Address is a referenced model. +// +// swagger:model +type Address struct { + // Street is the street line. + Street string `json:"street"` +} + +// Person references Address through a field decorated with a description and a +// vendor extension — both can, in principle, sit beside the $ref. How they are +// rendered depends on the options. +// +// swagger:model +type Person struct { + // Home is where the person lives. + // + // extensions: + // x-ui-order: 3 + Home Address `json:"home"` +} + +// endsnippet:model diff --git a/docs/examples/shaping/refsiblings/refsiblings_test.go b/docs/examples/shaping/refsiblings/refsiblings_test.go new file mode 100644 index 00000000..7dd5aba9 --- /dev/null +++ b/docs/examples/shaping/refsiblings/refsiblings_test.go @@ -0,0 +1,72 @@ +// SPDX-License-Identifier: Apache-2.0 + +package refsiblings + +import ( + "encoding/json" + "os" + "path/filepath" + "runtime" + "testing" + + "github.com/go-openapi/codescan" + "github.com/go-openapi/spec" + "github.com/go-openapi/testify/v2/require" +) + +func examplesRoot(t *testing.T) string { + t.Helper() + _, thisFile, _, ok := runtime.Caller(0) + require.True(t, ok) + return filepath.Clean(filepath.Join(filepath.Dir(thisFile), "..", "..")) +} + +func scan(t *testing.T, opts codescan.Options) *spec.Swagger { + t.Helper() + opts.WorkDir = examplesRoot(t) + opts.Packages = []string{"./shaping/refsiblings"} + opts.ScanModels = true + doc, err := codescan.Run(&opts) + require.NoError(t, err) + require.NotNil(t, doc) + return doc +} + +// goldenHome marshals the Person.home property and compares it to (or, under +// UPDATE_GOLDEN, rewrites) testdata/.json. +// +// Regenerate with: UPDATE_GOLDEN=1 go test ./... +func goldenHome(t *testing.T, doc *spec.Swagger, feature string) { + t.Helper() + person, ok := doc.Definitions["Person"] + require.True(t, ok, "Person definition missing") + home, ok := person.Properties["home"] + require.True(t, ok, "home property missing") + got, err := json.MarshalIndent(home, "", " ") + require.NoError(t, err) + got = append(got, '\n') + + golden := filepath.Join("testdata", feature+".json") + if os.Getenv("UPDATE_GOLDEN") != "" { + require.NoError(t, os.WriteFile(golden, got, 0o600)) + } + want, err := os.ReadFile(golden) + require.NoError(t, err) + require.JSONEq(t, string(want), string(got)) +} + +// TestRefSiblings emits the three fragments rendered by the how-to: the default +// allOf wrap, the EmitRefSiblings direct-sibling shape, and the bare $ref left +// by SkipAllOfCompounding. +func TestRefSiblings(t *testing.T) { + // Default: extension present → single-arm allOf wrap; description and x-* + // ride the outer compound. + goldenHome(t, scan(t, codescan.Options{}), "default") + + // EmitRefSiblings: description and x-* ride directly beside the $ref. + goldenHome(t, scan(t, codescan.Options{EmitRefSiblings: true}), "siblings") + + // SkipAllOfCompounding: no compound is produced, so the description and + // extension are dropped and a bare $ref remains. + goldenHome(t, scan(t, codescan.Options{SkipAllOfCompounding: true}), "skip") +} diff --git a/docs/examples/shaping/refsiblings/testdata/default.json b/docs/examples/shaping/refsiblings/testdata/default.json new file mode 100644 index 00000000..c350e880 --- /dev/null +++ b/docs/examples/shaping/refsiblings/testdata/default.json @@ -0,0 +1,10 @@ +{ + "description": "Home is where the person lives.", + "allOf": [ + { + "$ref": "#/definitions/Address" + } + ], + "x-go-name": "Home", + "x-ui-order": 3 +} diff --git a/docs/examples/shaping/refsiblings/testdata/siblings.json b/docs/examples/shaping/refsiblings/testdata/siblings.json new file mode 100644 index 00000000..a2dbdd11 --- /dev/null +++ b/docs/examples/shaping/refsiblings/testdata/siblings.json @@ -0,0 +1,5 @@ +{ + "description": "Home is where the person lives.", + "x-ui-order": 3, + "$ref": "#/definitions/Address" +} diff --git a/docs/examples/shaping/refsiblings/testdata/skip.json b/docs/examples/shaping/refsiblings/testdata/skip.json new file mode 100644 index 00000000..aec770fb --- /dev/null +++ b/docs/examples/shaping/refsiblings/testdata/skip.json @@ -0,0 +1,3 @@ +{ + "$ref": "#/definitions/Address" +} diff --git a/internal/builders/schema/README.md b/internal/builders/schema/README.md index e09f374a..067a1aed 100644 --- a/internal/builders/schema/README.md +++ b/internal/builders/schema/README.md @@ -917,23 +917,47 @@ replaces siblings. The correct shape is an **allOf compound**: inside `allOf[1]` (go-swagger#2655). A non-ref field emits its externalDocs via `handlers.schemaRawHandler` instead. -### The `DescWithRef` toggle and the description-only case - -`scanner.Options.DescWithRef` controls how a description-only -override is emitted: - -- **DescWithRef=true**: a $ref'd field whose only field-level - decoration is a description produces a single-arm allOf - compound. The description rides the outer parent so JSON-Schema - consumers see it alongside the `$ref`. -- **DescWithRef=false** (the default — matches v1 strict - behaviour): the description is dropped and a bare `$ref` is - emitted. Users who want the description preserved via the - JSON-Schema-correct compound shape opt in explicitly. - -When validations or user-authored extensions are present, the -allOf wrap is mandatory regardless of the flag — the override -would be lost otherwise. +### Sibling-rendering toggles — two orthogonal axes + +`$ref` siblings split into two classes by how they can be emitted: + +- **description & extensions** — *siblings-eligible*: they can ride + directly beside the `$ref` (`{$ref, description, x-*}`), which strict + draft-4 ignores but OpenAPI 3.1 / JSON Schema 2020-12 / modern + Swagger-UI honour; or via the allOf wrap. +- **validations & externalDocs** — *compound-only*: they have no valid + bare-`$ref` form, so they can only ride an allOf compound (validations + on the override arm). + +Three options steer the rendering. The **defaults reproduce the legacy +behaviour byte-for-byte** — both new opt-ins off: + +- **`EmitRefSiblings`** (default false): when true, description and + extensions ride as **direct `$ref` siblings** (no allOf), *unless* a + validation/externalDocs already forces a compound — in which case they + ride the outer compound as before. Changes only the no-forced-compound + cases. +- **`DescWithRef`** (default false; **deprecated**, kept for + compatibility): governs only the *description-only* case in the legacy + wrap path — `true` preserves the description as a single-arm allOf + (`{description, allOf:[{$ref}]}`), `false` drops it. A no-op when + `EmitRefSiblings` is set. Prefer `EmitRefSiblings`. +- **`SkipAllOfCompounding`** (default false): when true, **no allOf + compound is ever produced**. Validations and externalDocs are + therefore **dropped**; description and extensions are dropped too + *unless* `EmitRefSiblings` keeps them as direct siblings. Each drop + raises one `CodeDroppedRefSibling` diagnostic through + `Options.OnDiagnostic`, so the loss is never silent. For downstream + consumers (e.g. go-swagger codegen) that expect a bare `$ref`. + +Invariants: + +- When validations or externalDocs are present, the allOf wrap is + mandatory (unless `SkipAllOfCompounding` drops them) — no toggle + promotes a validation to a bare sibling. +- **`required:` is always preserved.** It is a parent-side concern (it + lands on the enclosing object's `required` list, not as a `$ref` + sibling), applied during the collector Walk regardless of any flag. ### `refOverrideCollector` — accumulate-then-decide @@ -952,6 +976,13 @@ Walker has finished firing. Three flags track what was collected: true, the parsed `*ExternalDocumentation` is set on the outer compound (sibling of the `$ref`), mirroring the extension lift. +The collector also records each collected sibling in `collected` +(keyword, source position, and `siblingKind` — validation / extension +/ externalDoc). `applyRefSiblingDrop` consumes this under +`SkipAllOfCompounding`: extension-kind siblings survive when +`EmitRefSiblings` is set, everything else is dropped with one +`CodeDroppedRefSibling` diagnostic per keyword. + Splitting the collector out of `applyToRefField` keeps the per-shape Walker callbacks short and the orchestrator's cognitive complexity in check. The Walker fires; the collector records; the diff --git a/internal/builders/schema/schema_test.go b/internal/builders/schema/schema_test.go index a82ab4f9..97c371b4 100644 --- a/internal/builders/schema/schema_test.go +++ b/internal/builders/schema/schema_test.go @@ -5,6 +5,7 @@ package schema import ( "encoding/json" + "strings" "testing" "github.com/go-openapi/codescan/internal/parsers/grammar" @@ -1588,6 +1589,206 @@ func TestEmbeddedDescriptionAndTags_SkipExtensions(t *testing.T) { assert.MapNotContainsT(t, v2.Extensions, "x-nullable", "value2 has no x-nullable in source") } +// TestEmbeddedDescriptionAndTags_SkipAllOfCompounding is the A/B +// witness for the SkipAllOfCompounding option on the bugs/3125 +// fixture. Both Value1 (*ValueStruct, user-authored x-nullable + +// description) and Value2 (ValueStruct, example + description) are +// $ref'd fields whose siblings normally wrap into an allOf compound +// (see TestEmbeddedDescriptionAndTags). +// +// With SkipAllOfCompounding=true: +// +// - each field emits a BARE {$ref} — no AllOf wrapper, no +// description, no override extension/example sibling; +// - `required` (a parent-side concern) is PRESERVED on the enclosing +// object — it is not a $ref sibling; +// - every dropped sibling raises one CodeDroppedRefSibling diagnostic +// (x-nullable, example, and one per dropped description) so the +// loss is never silent. +func TestEmbeddedDescriptionAndTags_SkipAllOfCompounding(t *testing.T) { + packagePath := fixturesModule + "/" + fixtureMinimal3125 + + var diags []grammar.Diagnostic + ctx, err := scanner.NewScanCtx(&scanner.Options{ + Packages: []string{"./" + fixtureMinimal3125}, + WorkDir: scantest.FixturesDir(), + SkipAllOfCompounding: true, + OnDiagnostic: func(d grammar.Diagnostic) { diags = append(diags, d) }, + }) + require.NoError(t, err) + decl, _ := ctx.FindDecl(packagePath, "Item") + require.NotNil(t, decl) + prs := NewBuilder(ctx, decl) + models := make(map[string]oaispec.Schema) + require.NoError(t, prs.Build(WithDefinitions(models))) + schema := models[scantest.ResolveTestKey(t, models, "Item")] + + require.Len(t, schema.Properties, 2) + + // required is parent-side: preserved even with compounding disabled. + assert.ElementsMatch(t, []string{sampleValue1, sampleValue2}, schema.Required) + + refFrag := "#/definitions/" + fixturesModule + "/" + fixtureMinimal3125 + "/ValueStruct" + + // Value1: bare $ref — no allOf, no description, no x-nullable sibling. + v1 := schema.Properties[sampleValue1] + assert.Equal(t, refFrag, v1.Ref.String(), "value1 must be a bare $ref") + assert.Empty(t, v1.AllOf, "no allOf compound when compounding is disabled") + assert.Empty(t, v1.Description, "description dropped on a bare $ref") + assert.MapNotContainsT(t, v1.Extensions, "x-nullable", "override extension dropped on a bare $ref") + + // Value2: bare $ref — no allOf, no description, no example sibling. + v2 := schema.Properties[sampleValue2] + assert.Equal(t, refFrag, v2.Ref.String(), "value2 must be a bare $ref") + assert.Empty(t, v2.AllOf, "no allOf compound when compounding is disabled") + assert.Empty(t, v2.Description, "description dropped on a bare $ref") + assert.Nil(t, v2.Example, "override example dropped on a bare $ref") + + // Every dropped sibling is reported. Value1 → x-nullable + description; + // Value2 → example + description. All carry CodeDroppedRefSibling. + var keywords, descDrops int + for _, d := range diags { + if d.Code != grammar.CodeDroppedRefSibling { + continue + } + switch { + case strings.Contains(d.Message, "description dropped"): + descDrops++ + default: + keywords++ + } + } + assert.Equal(t, 2, descDrops, "one description-drop diagnostic per $ref'd field") + assert.Equal(t, 2, keywords, "x-nullable and example each raise a drop diagnostic") + + msgs := make([]string, 0, len(diags)) + for _, d := range diags { + msgs = append(msgs, d.Message) + } + assert.True(t, sliceContainsSubstr(msgs, "x-nullable"), "x-nullable drop reported: %v", msgs) + assert.True(t, sliceContainsSubstr(msgs, "example"), "example drop reported: %v", msgs) +} + +func sliceContainsSubstr(ss []string, sub string) bool { + for _, s := range ss { + if strings.Contains(s, sub) { + return true + } + } + return false +} + +// TestEmbeddedDescriptionAndTags_EmitRefSiblings exercises the +// EmitRefSiblings lenient mode (no SkipAllOfCompounding) on bugs/3125. +// +// - Value1 (*ValueStruct, user x-nullable + description): no +// validation forces a compound, so description and x-nullable ride +// as DIRECT $ref siblings — bare {$ref, description, x-nullable}, +// no allOf. +// - Value2 (ValueStruct, example + description): `example` is a +// validation-class override, which still forces an allOf compound; +// the description rides the outer compound and the example lands on +// the override arm. EmitRefSiblings does not change the +// forced-compound case. +func TestEmbeddedDescriptionAndTags_EmitRefSiblings(t *testing.T) { + packagePath := fixturesModule + "/" + fixtureMinimal3125 + + var diags []grammar.Diagnostic + ctx, err := scanner.NewScanCtx(&scanner.Options{ + Packages: []string{"./" + fixtureMinimal3125}, + WorkDir: scantest.FixturesDir(), + EmitRefSiblings: true, + OnDiagnostic: func(d grammar.Diagnostic) { diags = append(diags, d) }, + }) + require.NoError(t, err) + decl, _ := ctx.FindDecl(packagePath, "Item") + require.NotNil(t, decl) + prs := NewBuilder(ctx, decl) + models := make(map[string]oaispec.Schema) + require.NoError(t, prs.Build(WithDefinitions(models))) + schema := models[scantest.ResolveTestKey(t, models, "Item")] + + require.Len(t, schema.Properties, 2) + assert.ElementsMatch(t, []string{sampleValue1, sampleValue2}, schema.Required) + + refFrag := "#/definitions/" + fixturesModule + "/" + fixtureMinimal3125 + "/ValueStruct" + + // Value1: direct siblings, no allOf. + v1 := schema.Properties[sampleValue1] + assert.Equal(t, refFrag, v1.Ref.String(), "value1 keeps a bare $ref") + assert.Empty(t, v1.AllOf, "no allOf wrap for sibling-eligible decoration") + assert.EqualT(t, "Nullable value", v1.Description, "description rides as a $ref sibling") + assert.Equal(t, true, v1.Extensions["x-nullable"], "extension rides as a $ref sibling") + + // Value2: a validation (example) still forces the compound. + v2 := schema.Properties[sampleValue2] + assert.Empty(t, v2.Ref.String(), "value2 outer carries no ref — it is a compound") + require.Len(t, v2.AllOf, 2, "example (a validation) forces a two-arm allOf") + assert.Equal(t, refFrag, v2.AllOf[0].Ref.String()) + assert.EqualT(t, "Non-nullable value", v2.Description, "description rides the outer compound") + assert.Equal(t, map[string]any{"value": float64(42)}, v2.AllOf[1].Example) + + // Nothing was dropped → no drop diagnostics. + for _, d := range diags { + assert.NotEqual(t, grammar.CodeDroppedRefSibling, d.Code, "unexpected drop: %s", d.Message) + } +} + +// TestEmbeddedDescriptionAndTags_EmitRefSiblings_Skip covers the +// EmitRefSiblings + SkipAllOfCompounding combination on bugs/3125 +// (the "both on" quadrant). No allOf compound is ever produced: +// +// - Value1: description + x-nullable survive as direct $ref siblings. +// - Value2: description survives as a sibling, but `example` (a +// validation, which can only ride a compound) is dropped with a +// diagnostic. +func TestEmbeddedDescriptionAndTags_EmitRefSiblings_Skip(t *testing.T) { + packagePath := fixturesModule + "/" + fixtureMinimal3125 + + var diags []grammar.Diagnostic + ctx, err := scanner.NewScanCtx(&scanner.Options{ + Packages: []string{"./" + fixtureMinimal3125}, + WorkDir: scantest.FixturesDir(), + EmitRefSiblings: true, + SkipAllOfCompounding: true, + OnDiagnostic: func(d grammar.Diagnostic) { diags = append(diags, d) }, + }) + require.NoError(t, err) + decl, _ := ctx.FindDecl(packagePath, "Item") + require.NotNil(t, decl) + prs := NewBuilder(ctx, decl) + models := make(map[string]oaispec.Schema) + require.NoError(t, prs.Build(WithDefinitions(models))) + schema := models[scantest.ResolveTestKey(t, models, "Item")] + + require.Len(t, schema.Properties, 2) + assert.ElementsMatch(t, []string{sampleValue1, sampleValue2}, schema.Required) + + refFrag := "#/definitions/" + fixturesModule + "/" + fixtureMinimal3125 + "/ValueStruct" + + // Value1: siblings survive (no compound ever needed). + v1 := schema.Properties[sampleValue1] + assert.Equal(t, refFrag, v1.Ref.String()) + assert.Empty(t, v1.AllOf) + assert.EqualT(t, "Nullable value", v1.Description) + assert.Equal(t, true, v1.Extensions["x-nullable"]) + + // Value2: description survives as a sibling; example (validation) dropped. + v2 := schema.Properties[sampleValue2] + assert.Equal(t, refFrag, v2.Ref.String(), "value2 stays a bare $ref") + assert.Empty(t, v2.AllOf, "no compound produced under SkipAllOfCompounding") + assert.EqualT(t, "Non-nullable value", v2.Description, "description survives as a sibling") + assert.Nil(t, v2.Example, "example dropped — a validation cannot ride a bare $ref") + + var drops int + for _, d := range diags { + if d.Code == grammar.CodeDroppedRefSibling { + drops++ + } + } + assert.Equal(t, 1, drops, "only the example (a validation) is dropped") +} + // TestParamsShape_DescWithRef_BothModes covers the description-only // $ref'd field case where the user toggles DescWithRef: // diff --git a/internal/builders/schema/walker.go b/internal/builders/schema/walker.go index 132e5591..e0fc3cae 100644 --- a/internal/builders/schema/walker.go +++ b/internal/builders/schema/walker.go @@ -5,6 +5,7 @@ package schema import ( "go/ast" + "go/token" "strings" "github.com/go-openapi/codescan/internal/builders/handlers" @@ -176,12 +177,37 @@ func (s *Builder) applyToRefField(block grammar.Block, enclosing, ps *oaispec.Sc description := block.Prose() if !c.anyCollected() && description == "" { + return // bare {$ref}: nothing to attach + } + + // SkipAllOfCompounding: never emit an allOf compound. Validations and + // externalDocs can only ride a compound, so they are dropped; + // description and extensions are dropped too UNLESS EmitRefSiblings + // keeps them as direct $ref siblings. `required` already landed on the + // enclosing schema during the Walk (a parent-side concern, not a $ref + // sibling) and is unaffected. + if s.Ctx.SkipAllOfCompounding() { + s.applyRefSiblingDrop(c, ps, description, name, block.Pos()) return } - if !c.anyCollected() && !s.Ctx.DescWithRef() { + + // EmitRefSiblings: when nothing forces a compound (no validations, no + // externalDocs), description and extensions ride directly beside the + // $ref rather than in a single-arm allOf wrap. A forced compound falls + // through to the wrap path below, where they ride the outer compound. + forcedCompound := c.collectedValidation || c.collectedExternalDoc + if s.Ctx.EmitRefSiblings() && !forcedCompound { + ps.Description = description + for k, v := range c.override.Extensions { + ps.AddExtension(k, v) + } return } + if !c.anyCollected() && !s.Ctx.DescWithRef() { + return // description-only, not preserved → bare {$ref} + } + // Lift x-* siblings onto the outer compound (see §ref-override). liftedExtensions := c.override.Extensions c.override.Extensions = nil @@ -207,6 +233,37 @@ func (s *Builder) applyToRefField(block grammar.Block, enclosing, ps *oaispec.Sc } } +// applyRefSiblingDrop handles the SkipAllOfCompounding case: no allOf +// compound is produced, so the field keeps its bare {$ref}. Extensions +// survive as direct siblings when EmitRefSiblings is set; description +// likewise. Everything else (validations, externalDocs) — and, without +// EmitRefSiblings, description / extensions too — is dropped, each with +// one CodeDroppedRefSibling diagnostic so the loss is never silent. +func (s *Builder) applyRefSiblingDrop(c *refOverrideCollector, ps *oaispec.Schema, description, name string, blockPos token.Position) { + keepSiblings := s.Ctx.EmitRefSiblings() + + for _, d := range c.collected { + if keepSiblings && d.kind == siblingExtension { + ps.AddExtension(d.keyword, c.override.Extensions[d.keyword]) + continue + } + s.RecordDiagnostic(grammar.Warnf(d.pos, grammar.CodeDroppedRefSibling, + "field %q: %q dropped — not representable on a bare $ref (SkipAllOfCompounding)", + name, d.keyword)) + } + + if description == "" { + return + } + if keepSiblings { + ps.Description = description + return + } + s.RecordDiagnostic(grammar.Warnf(blockPos, grammar.CodeDroppedRefSibling, + "field %q: description dropped — not representable on a bare $ref (SkipAllOfCompounding)", + name)) +} + // refOverrideCollector accumulates field-level overrides into a // scratch schema for the allOf compound rewrite. // @@ -226,14 +283,45 @@ type refOverrideCollector struct { collectedValidation bool collectedExtension bool collectedExternalDoc bool + // collected records each collected sibling (keyword + source + // position + class) so applyToRefField can decide, per category, + // what to drop under SkipAllOfCompounding and raise a per-keyword + // diagnostic. See [§ref-override]. + collected []collectedSibling +} + +// siblingKind classifies a $ref sibling by how it can be emitted. +// Extensions can ride directly beside a $ref (EmitRefSiblings); +// validations and externalDocs can only ride an allOf compound. +type siblingKind int + +const ( + siblingValidation siblingKind = iota + siblingExtension + siblingExternalDoc +) + +// collectedSibling names one $ref-sibling keyword, where it was written, +// and its class — for the drop diagnostics and category-aware handling. +type collectedSibling struct { + keyword string + pos token.Position + kind siblingKind } func (c *refOverrideCollector) anyCollected() bool { return c.collectedValidation || c.collectedExtension || c.collectedExternalDoc } -func (c *refOverrideCollector) markValidation() { c.collectedValidation = true } -func (c *refOverrideCollector) markExtension() { c.collectedExtension = true } +func (c *refOverrideCollector) markValidation(p grammar.Property) { + c.collectedValidation = true + c.collected = append(c.collected, collectedSibling{keyword: p.Keyword.Name, pos: p.Pos, kind: siblingValidation}) +} + +func (c *refOverrideCollector) markExtension(ext grammar.Extension) { + c.collectedExtension = true + c.collected = append(c.collected, collectedSibling{keyword: ext.Name, pos: ext.Pos, kind: siblingExtension}) +} func (c *refOverrideCollector) onNumber(p grammar.Property, val float64, exclusive bool) { if !p.IsTyped() { @@ -242,13 +330,13 @@ func (c *refOverrideCollector) onNumber(p grammar.Property, val float64, exclusi switch p.Keyword.Name { case grammar.KwMaximum: c.valid.SetMaximum(val, exclusive) - c.markValidation() + c.markValidation(p) case grammar.KwMinimum: c.valid.SetMinimum(val, exclusive) - c.markValidation() + c.markValidation(p) case grammar.KwMultipleOf: c.valid.SetMultipleOf(val) - c.markValidation() + c.markValidation(p) } } @@ -259,22 +347,22 @@ func (c *refOverrideCollector) onInteger(p grammar.Property, val int64) { switch p.Keyword.Name { case grammar.KwMinLength: c.valid.SetMinLength(val) - c.markValidation() + c.markValidation(p) case grammar.KwMaxLength: c.valid.SetMaxLength(val) - c.markValidation() + c.markValidation(p) case grammar.KwMinItems: c.valid.SetMinItems(val) - c.markValidation() + c.markValidation(p) case grammar.KwMaxItems: c.valid.SetMaxItems(val) - c.markValidation() + c.markValidation(p) case grammar.KwMinProperties: c.valid.SetMinProperties(val) - c.markValidation() + c.markValidation(p) case grammar.KwMaxProperties: c.valid.SetMaxProperties(val) - c.markValidation() + c.markValidation(p) } } @@ -289,10 +377,10 @@ func (c *refOverrideCollector) onBool(p grammar.Property, val bool) { } case grammar.KwReadOnly: c.override.ReadOnly = val - c.markValidation() + c.markValidation(p) case grammar.KwUnique: c.valid.SetUnique(val) - c.markValidation() + c.markValidation(p) } } @@ -300,30 +388,30 @@ func (c *refOverrideCollector) onString(p grammar.Property, val string) { switch p.Keyword.Name { case grammar.KwPattern: handlers.ApplyPattern(p, c.valid, val, c.builder.RecordDiagnostic) - c.markValidation() + c.markValidation(p) case grammar.KwPatternProperties: handlers.ApplyPatternProperties(p, c.valid, val, c.builder.RecordDiagnostic) - c.markValidation() + c.markValidation(p) case grammar.KwAdditionalProperties: // On a $ref'd field, additionalProperties rides as an allOf sibling // (`{allOf: [{$ref}, {additionalProperties: …}]}`) so the reference is // preserved — JSON-Schema-draft-4 semantics, like the other siblings. if sob, ok := c.builder.resolveAdditionalPropertiesValue(strings.TrimSpace(val), p.Pos); ok { c.override.AdditionalProperties = sob - c.markValidation() + c.markValidation(p) } case grammar.KwDefault: // The $ref override arm carries no Type of its own, so a JSON // object/array literal is coerced structurally here rather than // type-driven via ParseDefault (quirk G3). c.valid.SetDefault(validations.CoerceJSONOrString(val)) - c.markValidation() + c.markValidation(p) case grammar.KwExample: c.valid.SetExample(validations.CoerceJSONOrString(val)) - c.markValidation() + c.markValidation(p) case grammar.KwEnum: c.valid.SetEnum(val) - c.markValidation() + c.markValidation(p) } } @@ -338,7 +426,7 @@ func (c *refOverrideCollector) onExtension(ext grammar.Extension) { return } c.override.AddExtension(ext.Name, ext.Value) - c.markExtension() + c.markExtension(ext) } func (c *refOverrideCollector) onRaw(p grammar.Property) { @@ -346,13 +434,13 @@ func (c *refOverrideCollector) onRaw(p grammar.Property) { case grammar.KwDefault: // See onString: type-unknown override arm → JSON-literal coercion. c.valid.SetDefault(validations.CoerceJSONOrString(p.Value)) - c.markValidation() + c.markValidation(p) case grammar.KwExample: c.valid.SetExample(validations.CoerceJSONOrString(p.Value)) - c.markValidation() + c.markValidation(p) case grammar.KwEnum: c.valid.SetEnum(p.Value) - c.markValidation() + c.markValidation(p) case grammar.KwExternalDocs: // externalDocs on a $ref'd field lifts onto the outer allOf // compound (see applyToRefField). A non-ref field handles it @@ -365,6 +453,7 @@ func (c *refOverrideCollector) onRaw(p grammar.Property) { if ed != nil { c.externalDocs = ed c.collectedExternalDoc = true + c.collected = append(c.collected, collectedSibling{keyword: p.Keyword.Name, pos: p.Pos, kind: siblingExternalDoc}) } } } diff --git a/internal/parsers/grammar/diagnostic.go b/internal/parsers/grammar/diagnostic.go index aba13b91..fc8e3dca 100644 --- a/internal/parsers/grammar/diagnostic.go +++ b/internal/parsers/grammar/diagnostic.go @@ -147,6 +147,15 @@ const ( // than surfacing a raw Go stack trace. See // go-swagger/go-swagger#2886. CodeInternalPanic Code = "scan.internal-panic" + + // CodeDroppedRefSibling fires when SkipAllOfCompounding is set and a + // $ref'd struct field carries sibling decoration (description, + // validations, vendor extensions, externalDocs) that cannot ride a + // bare $ref. With compounding disabled the field emits as a bare + // $ref and each such sibling is dropped — one diagnostic per dropped + // keyword so the loss is never silent. See scanner.Options + // SkipAllOfCompounding. + CodeDroppedRefSibling Code = "validate.dropped-ref-sibling" ) // Diagnostic is one observation about a comment block. diff --git a/internal/scanner/options.go b/internal/scanner/options.go index 85460366..a725b9a3 100644 --- a/internal/scanner/options.go +++ b/internal/scanner/options.go @@ -49,8 +49,57 @@ type Options struct { // allOf compound is mandatory regardless of this flag — the // override would be lost otherwise. // - // See [§descwithref](./README.md#descwithref). - DescWithRef bool + // Deprecated: prefer EmitRefSiblings, which preserves description + // AND extensions as direct $ref siblings (the modern, lenient + // shape). DescWithRef is retained with its original semantics (the + // strict draft-4 single-arm allOf wrap for the description-only + // case) and remains a no-op when EmitRefSiblings is set. + // + // See [§ref-override](../builders/schema/README.md#ref-override). + DescWithRef bool + + // EmitRefSiblings emits a $ref'd field's description and vendor + // extensions as DIRECT siblings of the `$ref` + // (`{$ref, description, x-*}`) instead of wrapping them in an allOf + // compound. Strict JSON-Schema-draft-4 ignores siblings of `$ref` + // (hence the default allOf wrap), but OpenAPI 3.1 / JSON Schema + // 2020-12 and most modern Swagger-UI renderers honour them. + // + // - false (default): description / extensions follow the legacy + // wrap behaviour (extensions lift onto a single-arm allOf; + // description-only is governed by DescWithRef). + // - true: description and extensions ride directly alongside the + // `$ref`, no allOf. + // + // Validations and externalDocs are NOT siblings-eligible: when + // present they still force an allOf compound (validations on the + // override arm), and description / extensions then ride the outer + // compound. This flag changes only the no-forced-compound cases. + // + // See [§ref-override](../builders/schema/README.md#ref-override). + EmitRefSiblings bool + + // SkipAllOfCompounding disables the allOf-compound rewrite for + // $ref'd struct fields entirely: no allOf compound is ever emitted. + // + // - false (default): siblings are preserved via the allOf compound + // (or, under EmitRefSiblings, as direct $ref siblings). + // - true: no compound is produced. Validations and externalDocs — + // which can only ride a compound — are DROPPED. Description and + // extensions are likewise dropped UNLESS EmitRefSiblings is also + // set, in which case they survive as direct `$ref` siblings. + // Every drop raises one diagnostic through OnDiagnostic — the + // loss is never silent. + // + // `required:` is a parent-side concern (it lands on the enclosing + // object's `required` list, not as a $ref sibling) and is preserved + // regardless of this flag. + // + // Intended for downstream consumers (e.g. go-swagger codegen) that + // expect a bare `$ref` for a field pointing at a model and do not + // handle the allOf-compounded shape. See [§ref-override]. + SkipAllOfCompounding bool + SkipExtensions bool // skip generating x-go-* vendor extensions in the spec // SkipEnumDescriptions controls whether the per-enum-value const-name diff --git a/internal/scanner/scan_context.go b/internal/scanner/scan_context.go index 579967ea..87c5e546 100644 --- a/internal/scanner/scan_context.go +++ b/internal/scanner/scan_context.go @@ -208,6 +208,14 @@ func (s *ScanCtx) DescWithRef() bool { return s.opts.DescWithRef } +func (s *ScanCtx) SkipAllOfCompounding() bool { + return s.opts.SkipAllOfCompounding +} + +func (s *ScanCtx) EmitRefSiblings() bool { + return s.opts.EmitRefSiblings +} + func (s *ScanCtx) SetXNullableForPointers() bool { return s.opts.SetXNullableForPointers }