fix: apply query variable defaults when undefined is passed explicitly - #13364
Conversation
When query variables like { show: undefined } are passed, the cache's
diffQueryAgainstStore would overwrite GraphQL-specified default values
with undefined by spreading the user-provided variables directly.
This caused @include/@Skip directives to fail with 'Invalid variable
referenced' errors.
Filter out undefined values before merging with defaults, matching the
behavior already implemented in QueryManager.getVariables.
Fixes apollographql#13345
🦋 Changeset detectedLatest commit: ecd2e19 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✅ AI Style Review — No Changes DetectedNo MDX files were changed in this pull request. Review Log: View detailed log
|
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe cache reader now removes explicitly undefined variables before merging query defaults. A test verifies that an undefined variable uses its default value when evaluating an ChangesUndefined variable default handling
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
jerelmiller
left a comment
There was a problem hiding this comment.
Thanks for the submission! I had a couple small points of feedback. Before we merge, can you also make sure this PR has a changeset? Otherwise this won't end up in the changelog. Thanks!
| const rawVariables = variables ?? {}; | ||
| variables = { | ||
| ...getDefaultValues(getQueryDefinition(query)), | ||
| ...variables!, | ||
| ...Object.fromEntries( | ||
| Object.entries(rawVariables).filter(([, v]) => v !== undefined) | ||
| ), | ||
| }; |
There was a problem hiding this comment.
Rather than spreading variables, then spreading it again (with a filter applied), lets instead use the compact utility which merges objects except undefined values.
| const rawVariables = variables ?? {}; | |
| variables = { | |
| ...getDefaultValues(getQueryDefinition(query)), | |
| ...variables!, | |
| ...Object.fromEntries( | |
| Object.entries(rawVariables).filter(([, v]) => v !== undefined) | |
| ), | |
| }; | |
| variables = compact( | |
| getDefaultValues(getQueryDefinition(query)), | |
| variables | |
| ); |
| }); | ||
| }); | ||
|
|
||
| it("applies defaults when variable is explicitly undefined", () => { |
There was a problem hiding this comment.
Great test! Can we also verify it correctly uses defaults if show is omitted altogether as well? I don't think we have any other test in this suite that verifies that. That just makes sure we don't add any regressions there as well 🙂
There was a problem hiding this comment.
I think the test doesn't work at all since the field has no value in the store. So even if show: true was used (or defaulted), the query would return only { id: "abcd" } as the result?
There was a problem hiding this comment.
@ab-pm the readQueryFromStore helper that this test uses sets returnPartialData: false by default so if @include really weren't evaluated, the result would be null instead of { id: "abcd" } due to the result being a partial result. I think its ok 🙂
|
Hey @jerelmiller , I've addressed all your feedback:
Would appreciate if you could take another look when you get a chance. Thanks! |
jerelmiller
left a comment
There was a problem hiding this comment.
Thanks again for the contribution! I'll get this out in the next patch.
commit: |
|
Hey @jerelmiller, thanks for the quick merge on this one! Really appreciate it. If you have a moment, could you also take a look at #13365? It's the docs-only unique-names anchor fix - small change and already reviewed. Thanks! |
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @apollo/client@4.2.9 ### Patch Changes - [#13364](#13364) [`2f383e7`](2f383e7) Thanks [@atharv-sys32](https://github.com/atharv-sys32)! - Fix a bug where GraphQL variable default values were not applied during cache reads when variables with defaults were explicitly set to `undefined`. This caused `@include`/`@skip` directives to throw "Invalid variable referenced" errors when the variable was passed as `undefined` instead of being omitted entirely. - [#13367](#13367) [`2b39cc8`](2b39cc8) Thanks [@jerelmiller](https://github.com/jerelmiller)! - Fix an issue where some `@export` queries would not react to cache updates when the fields keyed by exported variables were updated. Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Fixes #13345
Summary by CodeRabbit
undefined.@includereturn the expected results.