support all x-* extensions and with ref#1880
Conversation
|
@greptileai Please review this commit, with a thorough focus on whether having an extension as a sibling of $ref is valid - specifically, see if you can infer how this will interact with request validation using the openapi3 filter (https://github.com/getkin/kin-openapi/tree/master/openapi3filter). More test cases might be needed. This code change is in a very sensitive path. |
|
This change makes sense to me. I'm totally fine doing things that aren't recommended by the spec, as long as they work for us. Normally, when $ref is set in a kin SchemaRef, the Schema value is inlined, so I'm going to dig into this a bit more thoroughly to see how this interacts with the schema lookup in our ref handling. I don't think we're consistent internally. |
Greptile SummaryThis PR extends Confidence Score: 5/5Safe to merge; only a minor test-spec inconsistency was found, no production logic bugs. All findings are P2 style issues. The core logic changes in schema.go, utils.go, codegen.go, merge_schemas.go, and operations.go are correct and well-tested by the new fixtures. The only issue is a stale entry in the required list of one test spec that does not affect runtime or generated output. internal/test/extensions/x-go-type/spec.yaml — required list references a non-existent property.
|
| Filename | Overview |
|---|---|
| pkg/codegen/schema.go | Key logic change: extensions now resolved via combinedSchemaExtensions (ref-side wins); x-go-type sibling on a $ref correctly overrides the ref-derived Go type; x-go-type-skip-optional-pointer propagation works correctly via combined extensions. |
| pkg/codegen/utils.go | ParseGoImportExtension now checks x-go-type on both ref-side and value-side extensions before collecting imports; import priority correctly prefers ref-side x-go-type-import. |
| pkg/codegen/codegen.go | GoSchemaImports now calls ParseGoImportExtension before the IsGoTypeReference guard, enabling import collection from sibling extensions on $ref properties. |
| pkg/codegen/merge_schemas.go | valueWithPropagatedRef now merges sibling extensions into the schema copy before returning, so allOf members carry per-use directives through mergeAllOf. |
| pkg/codegen/operations.go | Parameter extension merging now uses combinedSchemaExtensions so sibling extensions on a parameter's $ref schema are correctly included. |
| internal/test/extensions/x-go-type/spec.yaml | Test spec has int_as_big_int in the required list but no corresponding property, making the spec invalid per OpenAPI 3.0 rules. |
Reviews (2): Last reviewed commit: "Honor ref-sibling extensions in allOf me..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Greptile pointed out that we lost a guard on x-go-import, so put it back.
PR oapi-codegen#1880 introduced combinedSchemaExtensions so extensions placed next to a $ref are honored, with ref-side winning over value-side. The helper is wired in at GenerateGoSchema and Property.Extensions, but three other consumers of *openapi3.SchemaRef extensions still read value-side only and silently shadow the ref-side siblings. This commit closes the remaining gaps. * pkg/codegen/merge_schemas.go - valueWithPropagatedRef now folds ref.Extensions into the returned schema's Extensions via combinedSchemaExtensions, so allOf members can carry per-use sibling directives without mutating the referenced schema. Also returns a shallow copy unconditionally with a fresh Extensions map so callers can't back-mutate the source SchemaRef. - mergeAllOf routes each member through valueWithPropagatedRef instead of dereferencing schemaRef.Value directly, so the transitively-flattened nested-allOf path inherits the same fix. - The "real composition drops type-identity extensions" delete block at lines 105-109 is unchanged; it now strips ref-side x-go-type / x-go-type-name / x-go-type-import the same way it strips value-side, since both end up in schema.Extensions under the same keys. The allof-merge-extensions fixture remains bit-identical. * pkg/codegen/operations.go - GenerateParamsTypes uses combinedSchemaExtensions(param.Spec.Schema) when building the *Params struct field's extension map, so a sibling extension on a parameter's $ref schema (e.g. x-omitempty: false next to a $ref) reaches the field. * internal/test/extensions/param-ref-sibling-omitempty/ - New regression fixture exercising the Gap 3 fix: a query parameter whose schema is a $ref with sibling x-omitempty: false now generates json:"filter" instead of json:"filter,omitempty". * internal/test/go.mod - make tidy promotes golang.org/x/time from indirect to direct (used by the x-go-type fixture's generated file from PR oapi-codegen#1880).
|
@greptileai - new code has been pushed, re-evaluate on latest commit. I found some other places that needed improvement around losing sibling ref extensions. The only one with noticeable impact was on parameters. |
2 participants
This is meant to address #863 which asks for
x-go-typeto work with$ref. It also fixes my use case which is forx-oapi-codegen-extra-tagslike:I have added tests in the
internal/tests/extensionsdirectory to cover a few of the extensions, I can add a complete list if desired.We already treat
x-orderin this way (use the value next to the ref if provided) as of #1700 and this pr makes the functionality complete and consistent for all supported extensions.