Skip to content

fix(core): Propagate field descriptions to generated Filter/Sort parameters - #5065

Open
grolmus wants to merge 2 commits into
vendurehq:masterfrom
grolmus:mgrolmus/oss-659-propagate-graphql-field-descriptions-to-auto-generated
Open

fix(core): Propagate field descriptions to generated Filter/Sort parameters#5065
grolmus wants to merge 2 commits into
vendurehq:masterfrom
grolmus:mgrolmus/oss-659-propagate-graphql-field-descriptions-to-auto-generated

Conversation

@grolmus

@grolmus grolmus commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

Summary

Auto-generated {Type}FilterParameter and {Type}SortParameter input 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's type, omitting its description. The ListOptions wrapper fields (skip/take/sort/filter) carry descriptions, but the per-entity filter/sort fields did not — so a documented field (e.g. Product.name) produced ProductFilterParameter.name / ProductSortParameter.name with a null description.

Change

In createSortParameter and createFilterParameter (packages/core/src/api/config/generate-list-options.ts), copy field.description onto each generated fieldConfig. Two lines, mirroring the source field's documentation. The synthetic _and / _or filter fields are untouched and remain undescribed.

Test plan

Automated — added a regression test in generate-list-options.spec.ts asserting that a source type with """...""" field descriptions produces matching descriptions on both the generated *SortParameter and *FilterParameter fields, and that the synthetic _and field stays undescribed. Fails without the fix (descriptions come back undefined).

✓ src/api/config/generate-list-options.spec.ts (8 tests)
✓ src/api/config/graphql-custom-fields.spec.ts (15 tests)
✓ src/service/helpers/list-query-builder/parse-sort-params.spec.ts (7 tests)
✓ src/service/helpers/list-query-builder/parse-filter-params.spec.ts (29 tests)

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


View with [code]smith Autofix with [code]smith Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

grolmus added 2 commits July 30, 2026 11:36
…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
@vercel

vercel Bot commented Jul 30, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
vendure-storybook Ready Ready Preview Jul 30, 2026 9:47am

Request Review

@coderabbitai

coderabbitai Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

generateListOptions() now copies source GraphQL field descriptions into generated sort and filter parameter fields. A Vitest case verifies descriptions for Person.name and Person.age in both generated inputs and confirms the synthetic _and field remains undescribed.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The changes satisfy #5032 by copying source field descriptions into generated FilterParameter and SortParameter fields.
Out of Scope Changes check ✅ Passed The diff stays focused on description propagation and its regression test, with no unrelated code changes.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Title check ✅ Passed The title clearly and concisely describes the main change: propagating field descriptions to generated filter and sort parameters.
Description check ✅ Passed The description explains the change, root cause, testing, impact, and linked issue, but omits the template checklist and explicit breaking-changes section.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
packages/core/src/api/config/generate-list-options.spec.ts (1)

353-380: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Also assert that synthetic _or remains undescribed.

The objective covers both _and and _or, but the regression test only checks _and. Add the matching assertion for _or to 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

📥 Commits

Reviewing files that changed from the base of the PR and between fe68a92 and 614700b.

📒 Files selected for processing (2)
  • packages/core/src/api/config/generate-list-options.spec.ts
  • packages/core/src/api/config/generate-list-options.ts

@michaelbromley michaelbromley added the T1: Fast track Clearly understood fix with limited blast radius. Fast lane. label Jul 30, 2026

@HouseinIsProgramming HouseinIsProgramming left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +369 to +380
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();
});

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 from generate-list-options.ts): Tests 2 failed | 7 passed — both new tests fail
Suggested change
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!]
}`),
);
});

@biggamesmallworld

Copy link
Copy Markdown
Collaborator

Agree with @HouseinIsProgramming , can approve after the tests are updated

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

T1: Fast track Clearly understood fix with limited blast radius. Fast lane.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Propagate GraphQL field descriptions to auto-generated FilterParameter / SortParameter

4 participants