fix(core): Propagate field descriptions to generated Filter/Sort parameters - #5065
Conversation
…meters
The auto-generated {Type}FilterParameter and {Type}SortParameter input
types copied only the field type from the source object type, omitting
the field description. As a result introspection/GraphiQL showed docs on
the object type fields but empty descriptions on the matching filter/sort
input fields. Mirror the source field description onto each generated
field so API docs, codegen and introspection stay consistent.
Fixes vendurehq#5032
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthrough
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/core/src/api/config/generate-list-options.spec.ts (1)
353-380: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlso assert that synthetic
_orremains undescribed.The objective covers both
_andand_or, but the regression test only checks_and. Add the matching assertion for_orto prevent an asymmetric future regression.Suggested test addition
expect(filterParameter.getFields()._and.description).toBeUndefined(); + expect(filterParameter.getFields()._or.description).toBeUndefined();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/api/config/generate-list-options.spec.ts` around lines 353 - 380, The test case for generated filter parameters checks that synthetic _and lacks a description but does not cover _or. Extend the test around generateListOptions and filterParameter to assert filterParameter.getFields()._or.description is also undefined.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/core/src/api/config/generate-list-options.spec.ts`:
- Around line 353-380: The test case for generated filter parameters checks that
synthetic _and lacks a description but does not cover _or. Extend the test
around generateListOptions and filterParameter to assert
filterParameter.getFields()._or.description is also undefined.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5beee824-0b71-41d3-856b-7f31a5c3b326
📒 Files selected for processing (2)
packages/core/src/api/config/generate-list-options.spec.tspackages/core/src/api/config/generate-list-options.ts
michaelbromley
added
the
T1: Fast track
HouseinIsProgramming
left a comment
HouseinIsProgramming
left a comment
There was a problem hiding this comment.
Thanks, the fix is correct, including for fields merged from a pre-existing *FilterParameter/*SortParameter input.
One gap: the new test only covers fields from the source object type. The same code also runs for fields pulled in from a pre-declared filter/sort input (generate-list-options.ts L165-167), and that path has no coverage. That includes the name-collision case, where the existing input's field silently wins over the source type's, description included.
Could you add a test for that? One assertion that descriptions on fields from a pre-declared PersonFilterParameter/PersonSortParameter survive, and one for the collision precedence.
Will approve once that's in.
| const result = generateListOptions(buildSchema(input)); | ||
|
|
||
| const sortParameter = result.getType('PersonSortParameter') as any; | ||
| expect(sortParameter.getFields().name.description).toBe("The person's full name"); | ||
| expect(sortParameter.getFields().age.description).toBe('Age in years'); | ||
|
|
||
| const filterParameter = result.getType('PersonFilterParameter') as any; | ||
| expect(filterParameter.getFields().name.description).toBe("The person's full name"); | ||
| expect(filterParameter.getFields().age.description).toBe('Age in years'); | ||
| // synthetic fields without a source description remain undescribed | ||
| expect(filterParameter.getFields()._and.description).toBeUndefined(); | ||
| }); |
There was a problem hiding this comment.
The production change is correct — I reverted both description: lines locally and confirmed this test genuinely fails without them. Two things on the test itself.
Use printType against exact SDL, like every other test in this file. All 350 lines above assert via printType(...) + removeLeadingWhitespace(...); this one reaches into getFields().name.description behind as any. printType renders the descriptions as docstrings, so a single comparison per generated type asserts both fields, both types, and that _and/_or carry no docstring — in the form the rest of the suite uses, with no as any.
It also drops expect(..._and.description).toBeUndefined(), which pins an implementation detail: graphql-js or stitchSchemas normalising an absent description to null would fail that assertion for reasons unrelated to the behaviour. With printType the question doesn't arise — either a docstring prints or it doesn't. The // synthetic fields ... remain undescribed comment goes away with it.
Cover the pre-existing-input merge path. createSortParameter / createFilterParameter fold in fields from a hand-written {Type}SortParameter / {Type}FilterParameter when one exists (generate-list-options.ts:126-128, :167-169), and Vendure ships several — OrderFilterParameter, OrderSortParameter, ProductFilterParameter. Those fields flow through the same new field.description line, so documented hand-written SDL fields start emitting descriptions too. Currently untested. Second test below covers it (note the merged fields print before the source-type fields — that's the existing merge order, asserted as-is).
Verified locally:
- GREEN with the fix:
Tests 9 passed (9) - RED without it (both
description:lines deleted fromgenerate-list-options.ts):Tests 2 failed | 7 passed— both new tests fail
| const result = generateListOptions(buildSchema(input)); | |
| const sortParameter = result.getType('PersonSortParameter') as any; | |
| expect(sortParameter.getFields().name.description).toBe("The person's full name"); | |
| expect(sortParameter.getFields().age.description).toBe('Age in years'); | |
| const filterParameter = result.getType('PersonFilterParameter') as any; | |
| expect(filterParameter.getFields().name.description).toBe("The person's full name"); | |
| expect(filterParameter.getFields().age.description).toBe('Age in years'); | |
| // synthetic fields without a source description remain undescribed | |
| expect(filterParameter.getFields()._and.description).toBeUndefined(); | |
| }); | |
| const result = generateListOptions(buildSchema(input)); | |
| expect(printType(result.getType('PersonSortParameter')!)).toBe( | |
| removeLeadingWhitespace(` | |
| input PersonSortParameter { | |
| """The person's full name""" | |
| name: SortOrder | |
| """Age in years""" | |
| age: SortOrder | |
| }`), | |
| ); | |
| expect(printType(result.getType('PersonFilterParameter')!)).toBe( | |
| removeLeadingWhitespace(` | |
| input PersonFilterParameter { | |
| """The person's full name""" | |
| name: StringOperators | |
| """Age in years""" | |
| age: NumberOperators | |
| _and: [PersonFilterParameter!] | |
| _or: [PersonFilterParameter!] | |
| }`), | |
| ); | |
| }); | |
| it('propagates descriptions from a pre-existing sort and filter parameter', () => { | |
| const input = ` | |
| ${COMMON_TYPES} | |
| type Query { | |
| people: PersonList | |
| } | |
| type Person { | |
| name: String! | |
| } | |
| input PersonSortParameter { | |
| """Sort by relevance score""" | |
| score: SortOrder | |
| } | |
| input PersonFilterParameter { | |
| """Filter by nickname""" | |
| nickname: StringOperators | |
| } | |
| `; | |
| const result = generateListOptions(buildSchema(input)); | |
| expect(printType(result.getType('PersonSortParameter')!)).toBe( | |
| removeLeadingWhitespace(` | |
| input PersonSortParameter { | |
| """Sort by relevance score""" | |
| score: SortOrder | |
| name: SortOrder | |
| }`), | |
| ); | |
| expect(printType(result.getType('PersonFilterParameter')!)).toBe( | |
| removeLeadingWhitespace(` | |
| input PersonFilterParameter { | |
| """Filter by nickname""" | |
| nickname: StringOperators | |
| name: StringOperators | |
| _and: [PersonFilterParameter!] | |
| _or: [PersonFilterParameter!] | |
| }`), | |
| ); | |
| }); |
|
Agree with @HouseinIsProgramming , can approve after the tests are updated |
4 participants
Summary
Auto-generated
{Type}FilterParameterand{Type}SortParameterinput types now inherit the field descriptions from their source object type, so introspection / GraphiQL / codegen stay consistent with the documented object fields.Root cause
generateListOptions()builds each generated filter/sort input field from only the source field'stype, omitting itsdescription. TheListOptionswrapper fields (skip/take/sort/filter) carry descriptions, but the per-entity filter/sort fields did not — so a documented field (e.g.Product.name) producedProductFilterParameter.name/ProductSortParameter.namewith anulldescription.Change
In
createSortParameterandcreateFilterParameter(packages/core/src/api/config/generate-list-options.ts), copyfield.descriptiononto each generatedfieldConfig. Two lines, mirroring the source field's documentation. The synthetic_and/_orfilter fields are untouched and remain undescribed.Test plan
Automated — added a regression test in
generate-list-options.spec.tsasserting that a source type with"""..."""field descriptions produces matching descriptions on both the generated*SortParameterand*FilterParameterfields, and that the synthetic_andfield stays undescribed. Fails without the fix (descriptions come backundefined).Manual — no runtime behaviour change beyond schema documentation metadata; the generated SDL now shows the source field docs on filter/sort inputs. Verified no existing schema/introspection snapshot depended on the previously-empty descriptions.
Fixes #5032
@codesmith-botwith what you need. Autofix is disabled.