Add optional type generation for inline schemas#2366
Conversation
Greptile SummaryThis PR adds optional named type generation for inline schemas. The main changes are:
Confidence Score: 5/5This looks safe to merge.
|
| Filename | Overview |
|---|---|
| pkg/codegen/schema.go | Marks auto-hoisted inline schemas as referenced named types while preserving their additional type declarations. |
| pkg/codegen/codegen.go | Splits component and operation type collection while keeping declaration and boilerplate emission paths aligned. |
| pkg/codegen/configuration.go | Adds cross-field warnings for anonymous schema hoisting without model generation. |
Reviews (4): Last reviewed commit: "codegen: warn when generate-types-for-an..." | Re-trigger Greptile
jamietanna
left a comment
There was a problem hiding this comment.
Functionally it looks good - I wonder if we can improve the generated Description that gets added to it?
It's generally an existing issue, but especially where we're generating this new anonymous schema, it'd be great to improve
I..e we get:
// AddMicrosoftTeamsChatTabTaskParams defines model for add_microsoft_teams_chat_tab_task_params.
type AddMicrosoftTeamsChatTabTaskParams struct {
- Chat struct {
- ID *string `json:"id,omitempty"`
- Name *string `json:"name,omitempty"`
- } `json:"chat"`
+ Chat AddMicrosoftTeamsChatTabTaskParams_Chat `json:"chat"`
// Link The tab link
Link string `json:"link"`
@@ -39399,15 +39402,18 @@ type AddMicrosoftTeamsChatTabTaskParams struct {
Title string `json:"title"`
}
+// AddMicrosoftTeamsChatTabTaskParams_Chat defines model for AddMicrosoftTeamsChatTabTaskParams.Chat.
+type AddMicrosoftTeamsChatTabTaskParams_Chat struct {
+ ID *string `json:"id,omitempty"`
+ Name *string `json:"name,omitempty"`
+}I wonder if we could instead generate it as i.e.
// AddMicrosoftTeamsChatTabTaskParams_Chat defines model for the anonymous schema `chat` under path ..., or something similar?
Ok, so the code which generates those comments is kinda broken. It calls everything a "parameter". I'm going to make a minimal fix to this PR to inject the correct string with the field name, eg, Update: |
Adds output-options.generate-types-for-anonymous-schemas (default false). When set, every inline schema that would otherwise generate as an anonymous Go struct is instead emitted as a named type with a path- derived name (e.g. GetRolesId200JSONResponseBody_Data). Equivalent to adding x-go-type-name to every inline schema; when both are present at the same site, x-go-type-name wins. Test coverage reorganized: internal/test/anonymous_inner_hoisting/ now has two subdirectories. The existing always-on hoisting test (oneOf, anyOf, additionalProperties) moves to implicit/. The new flag-gated behavior gets its own test in global/, exercising the canonical issue shape (allOf-merged response with a sibling inline `data` object that itself contains a nested `role` reference). Closes: oapi-codegen#1139
476b896 to
4e9b14d
Compare
|
Greptile had a good finding. This refactor breaks the splitting of code generation into (types, server, strict-server), etc, because of references among the types. Operation Definitions can introduce new types (the anonymous inner types), but those are only emitted when the fix is non-trivial. It's a big refactor of the codegen structure. |
…s this config does not emit
The new `output-options.generate-types-for-anonymous-schemas` flag (issue
flow through the same emission path that `generate.models` gates. In a
single-config setup where the flag is set with `models: false` but a
client or server generator is enabled, the generated code emits
references to the hoisted names without declaring them, and `go build`
fails.
Multi-config setups where one config emits `models: true` and a sibling
emits a client/server into the same Go package work correctly today;
the hoisting decisions are deterministic across the two codegen
invocations and the sibling's declarations resolve same-package
references. The failure mode is specifically the single-config case.
The two cases are indistinguishable from a single codegen invocation
(both look like `flag: true, models: false, <ops>: true`), so this
warns rather than errors. Split-config users see an informational
notice they can ignore; misconfigured single-config users get a
heads-up before `go build` complains.
Changes:
- pkg/codegen/configuration.go:
- Add `GenerateOptions.AnyOperationGenerator()` reporting whether any
client/server/strict generator is enabled.
- Add `Configuration.Warnings()`, a cross-field diagnostic that fires
when `GenerateTypesForAnonymousSchemas && !Models &&
AnyOperationGenerator()`. Follows the existing
`GenerateOptions.Warnings()` pattern used for the std-http-server
Go-version check.
- Strengthen the doc comment on `GenerateTypesForAnonymousSchemas` to
call out the single-config failure mode and the multi-config
consistency requirement explicitly.
- configuration-schema.json: matching description text for the
IDE-surfaced schema doc.
- cmd/oapi-codegen/oapi-codegen.go: wire `opts.Warnings()` to stderr
immediately after the existing `Generate.Warnings()` printout, under a
"cross-field configuration warning(s)" header.
Helper-function cleanup in pkg/codegen/codegen.go:
- Decompose the monolithic `GenerateTypeDefinitions` into
`collectComponentTypes` (gathers #/components-derived TypeDefinitions),
`collectOperationTypes` (gathers op-derived TypeDefinitions for the
boilerplate scan), and `renderBoilerplate` (runs enum / additional-
properties / union / union+additional passes over the union). The
emission gating is unchanged — still a single `if opts.Generate.Models`
block — so this is a pure refactor with no behavioral effect on any
existing config. Generated output is byte-identical.
No new fixtures or behavioral changes; this commit is intended to be
squashed with the parent `Add optional type generation for inline
schemas` commit before the PR lands.
121ad2f to
15508f2
Compare
Resolved one conflict in pkg/codegen/codegen.go: upstream now uses the collectComponentTypes / collectOperationTypes / renderBoilerplate helper decomposition (from PR #2366). Kept that structure and changed the op- related helper calls to use allOps (paths + webhooks + callbacks) rather than ops, matching what the previous GenerateTypeDefinitions(t, spec, allOps, ...) call did, so webhook/callback-derived types continue to be emitted.
* Add optional type generation for inline schemas Adds output-options.generate-types-for-anonymous-schemas (default false). When set, every inline schema that would otherwise generate as an anonymous Go struct is instead emitted as a named type with a path- derived name (e.g. GetRolesId200JSONResponseBody_Data). Equivalent to adding x-go-type-name to every inline schema; when both are present at the same site, x-go-type-name wins. Test coverage reorganized: internal/test/anonymous_inner_hoisting/ now has two subdirectories. The existing always-on hoisting test (oneOf, anyOf, additionalProperties) moves to implicit/. The new flag-gated behavior gets its own test in global/, exercising the canonical issue shape (allOf-merged response with a sibling inline `data` object that itself contains a nested `role` reference). Closes: oapi-codegen#1139 * codegen: warn when generate-types-for-anonymous-schemas requires types this config does not emit The new `output-options.generate-types-for-anonymous-schemas` flag (issue flow through the same emission path that `generate.models` gates. In a single-config setup where the flag is set with `models: false` but a client or server generator is enabled, the generated code emits references to the hoisted names without declaring them, and `go build` fails. Multi-config setups where one config emits `models: true` and a sibling emits a client/server into the same Go package work correctly today; the hoisting decisions are deterministic across the two codegen invocations and the sibling's declarations resolve same-package references. The failure mode is specifically the single-config case. The two cases are indistinguishable from a single codegen invocation (both look like `flag: true, models: false, <ops>: true`), so this warns rather than errors. Split-config users see an informational notice they can ignore; misconfigured single-config users get a heads-up before `go build` complains. Changes: - pkg/codegen/configuration.go: - Add `GenerateOptions.AnyOperationGenerator()` reporting whether any client/server/strict generator is enabled. - Add `Configuration.Warnings()`, a cross-field diagnostic that fires when `GenerateTypesForAnonymousSchemas && !Models && AnyOperationGenerator()`. Follows the existing `GenerateOptions.Warnings()` pattern used for the std-http-server Go-version check. - Strengthen the doc comment on `GenerateTypesForAnonymousSchemas` to call out the single-config failure mode and the multi-config consistency requirement explicitly. - configuration-schema.json: matching description text for the IDE-surfaced schema doc. - cmd/oapi-codegen/oapi-codegen.go: wire `opts.Warnings()` to stderr immediately after the existing `Generate.Warnings()` printout, under a "cross-field configuration warning(s)" header. Helper-function cleanup in pkg/codegen/codegen.go: - Decompose the monolithic `GenerateTypeDefinitions` into `collectComponentTypes` (gathers #/components-derived TypeDefinitions), `collectOperationTypes` (gathers op-derived TypeDefinitions for the boilerplate scan), and `renderBoilerplate` (runs enum / additional- properties / union / union+additional passes over the union). The emission gating is unchanged — still a single `if opts.Generate.Models` block — so this is a pure refactor with no behavioral effect on any existing config. Generated output is byte-identical. No new fixtures or behavioral changes; this commit is intended to be squashed with the parent `Add optional type generation for inline schemas` commit before the PR lands.
|
In rootlyhq/rootly-go#131 I've noticed that some |
Adds output-options.generate-types-for-anonymous-schemas (default false). When set, every inline schema that would otherwise generate as an anonymous Go struct is instead emitted as a named type with a path- derived name (e.g. GetRolesId200JSONResponseBody_Data). Equivalent to adding x-go-type-name to every inline schema; when both are present at the same site, x-go-type-name wins.
I will need to document this, since it's still better, from a usability perspective, to put concrete types of interest in component/schemas or as a second alternative, use x-go-type-name. However, this is requested often enough, that I'd thing it's worth exposing as a feature.
Test coverage reorganized: internal/test/anonymous_inner_hoisting/ now has two subdirectories. The existing always-on hoisting test (oneOf, anyOf, additionalProperties) moves to implicit/. The new flag-gated behavior gets its own test in global/, exercising the canonical issue shape (allOf-merged response with a sibling inline
dataobject that itself contains a nestedrolereference).Closes: #1139