Add optional type generation for inline schemas#2366
Conversation
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 `data` object that itself contains a nested `role` reference). Closes: oapi-codegen#1139
Greptile SummaryAdds Confidence Score: 4/5Safe to merge; all findings are P2 (cosmetic / maintainability / test coverage) with no runtime correctness issues in the exercised cases. All three findings are P2: a misleading doc-comment on hoisted types (pre-existing pattern made more prominent), a maintenance divergence between the new hasInlineStructuralContent helper and the inlined conditions in operations.go, and limited test coverage that leaves request-body / parameter / additionalProperties hoisting paths untested. No P0 or P1 issues found. pkg/codegen/schema.go and pkg/codegen/operations.go for the helper-reuse divergence; internal/test/anonymous_inner_hoisting/global/spec.yaml for broader test coverage.
|
| Filename | Overview |
|---|---|
| pkg/codegen/configuration.go | Adds GenerateTypesForAnonymousSchemas bool field to OutputOptions with correct YAML tag; documentation is clear and accurate. |
| configuration-schema.json | Adds the new generate-types-for-anonymous-schemas entry, keeping the schema in sync with configuration.go as required. |
| pkg/codegen/schema.go | Auto-hoisting block added for nested inline schemas (path>1); hasInlineStructuralContent helper defined but not reused in operations.go parallel checks, creating a silent maintenance divergence risk. |
| pkg/codegen/operations.go | Two parallel hoisting conditions updated; each adds GenerateTypesForAnonymousSchemas && len(Properties)>0 rather than reusing hasInlineStructuralContent, but the result is semantically equivalent because HasAdditionalProperties and UnionElements are already covered by the first two OR clauses. |
| internal/test/anonymous_inner_hoisting/global/client.gen.go | Generated file looks correct for the flag-enabled case; both GetRolesId200JSONResponseBody_Data and GetRolesId200JSONResponseBody are emitted as named types, but both carry misleading "defines parameters for" doc comments instead of a response-body-oriented comment. |
| internal/test/anonymous_inner_hoisting/implicit/client.gen.go | Renamed/moved from anonymous_inner_hoisting/ root; only package name and header comment changed; content is identical to the previous file. |
| internal/test/anonymous_inner_hoisting/global/spec.yaml | New test spec exercising the canonical issue-1139 shape (allOf + inline data object containing a $ref Role); well-targeted. |
| internal/test/anonymous_inner_hoisting/global/client_test.go | Two compile-time and round-trip tests for the hoisted types; covers the primary goal of the feature. |
Reviews (1): Last reviewed commit: "Add optional type generation for inline ..." | 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?| models: true | ||
| client: true | ||
| output-options: | ||
| generate-types-for-anonymous-schemas: true |
There was a problem hiding this comment.
IMO, I'd prefer this by default - most folks don't control their specs, and so they get anonymous structs used which can be awkward
I'd personally swap this with a compatibility-options to disable this functionality
Using an OpenAPI Overlay is one way to get around this, but managing all areas this may apply will be quite hard
There was a problem hiding this comment.
I have the opposite view.
In specs with lots of inline schemas, this positively litters the file with new type definitions, and in many cases, you don't need to ever refer to the type, so the code, IMO, is nicer. I'm open to defaulting it to be on, however, I dislike the output.
Whenever you have an inline schema in a request body, or a parameter, you will never need to create an instance of that type, deserialization does that for you.
The place where it is nice to have is when you have inline schemas in things you are constructing; usually response bodies, and in those cases, it's so much nicer to declare those specifically. I realize that some people don't control their specs.
Co-authored-by: Jamie Tanna <github@jamietanna.co.uk>
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.
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