Core Data: Don't return partial data when selecting a complete item #71474
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Mamaduka
added
[Type] Bug
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Mamaduka
commented
Sep 3, 2025
| const context = query?.context ?? 'default'; | ||
|
|
||
| if ( query === undefined ) { | ||
| if ( ! query || ! query._fields ) { |
Member
Author
There was a problem hiding this comment.
The new condition is the same as that used in the reducer.
gutenberg/packages/core-data/src/queried-data/reducer.js
Lines 153 to 161 in 2e69c4a
6 tasks
|
Size Change: +13 B (0%) Total Size: 1.93 MB
ℹ️ View Unchanged
|
Mamaduka
commented
Sep 3, 2025
packages/core-data/src/test/store.js
Outdated
Member
Author
There was a problem hiding this comment.
The filename is just a placeholder, unless we have better ideas 😅
Member
There was a problem hiding this comment.
Curious why we wouldn't want this in packages/core-data/src/test/selectors.js ?
Member
Author
There was a problem hiding this comment.
These are more like integration tests. I guess we can also convert those tests to be integration, but that's outside of the scope for this PR, IMO.
Member
Author
There was a problem hiding this comment.
Turns out we already had unfinished unit tests for precisely this case. I've added the missing unit tests and removed the integration one.
P.S. I think I was too eager to introduce integration tests; we've been discussing them for a while. But I'm working on a different bug, where I think they'll be more suitable.
aduth
reviewed
Sep 3, 2025
| const context = query?.context ?? 'default'; | ||
|
|
||
| if ( query === undefined ) { | ||
| if ( ! query || ! query._fields ) { |
Member
There was a problem hiding this comment.
Does this now make the && query._fields on the condition below redundant?
gutenberg/packages/core-data/src/selectors.ts
Line 378 in 0e2eb9f
Member
Author
There was a problem hiding this comment.
Why would it?
It's good practice to check argument existence before iterating on them. I think we should leave the linked condition.
Member
There was a problem hiding this comment.
What I mean is that line 378 can now never be reached if query._fields is falsey, so there's no need in checking that it's truthy.
Member
Author
There was a problem hiding this comment.
You're right. I don't have a strong opinion here, but it seems like a good check to keep in place "just in case" 😄
Updated in d1b7dca.
packages/core-data/src/test/store.js
Outdated
Member
There was a problem hiding this comment.
Curious why we wouldn't want this in packages/core-data/src/test/selectors.js ?
Mamaduka
commented
Sep 7, 2025
Comment on lines
+396
to
+397
| const queriedState = | ||
| state.entities.records?.[ kind ]?.[ name ]?.queriedData; |
Member
Author
There was a problem hiding this comment.
I just extracted it into a reusable variable.
|
Flaky tests detected in f0c164d. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/17540850163
|
Mamaduka
commented
Sep 8, 2025
Comment on lines
-28
to
-32
| // getEntityRecord and __experimentalGetEntityRecordNoResolver selectors share the same tests. | ||
| describe.each( [ | ||
| [ getEntityRecord ], | ||
| [ __experimentalGetEntityRecordNoResolver ], | ||
| ] )( '%p', ( selector ) => { |
Member
Author
There was a problem hiding this comment.
I've removed coverage for __experimentalGetEntityRecordNoResolver. It's a leftover from when the data package was introduced and hasn't been used in the core.
I think we should just remove or deprecate it.
aduth
approved these changes
Sep 8, 2025
| setNestedValue( filteredItem, field, value ); | ||
| } | ||
| return filteredItem as EntityRecord; | ||
| if ( ! item ) { |
Member
There was a problem hiding this comment.
I like the flattening with early return 👍
Member
Author
|
Thanks for the review, @aduth 🙇 |
adamsilverstein
pushed a commit
to adamsilverstein/gutenberg
that referenced
this pull request
Sep 11, 2025
…ordPress#71474) Co-authored-by: Mamaduka <[email protected]> Co-authored-by: aduth <[email protected]> Co-authored-by: albanyacademy <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Closes #71375.
PR fixes a bug in the data store that would return subset record properties (as an intermediate response) when the consumer was requesting and expecting the whole record.
Note: This regressed sometime after initial
_fieldssupport was implemented. The lack of integration tests made it harder to catch similar regressions.I've added coverage for the current case, plus some basic uses as integration tests. Hopefully, we can expand on those in the future.
Why?
When a component requests a full entity record, it expects to receive
undefinedor the whole record. Returning partial data can lead to unexpected failures as described in the issue.How?
Fixes the complete item check for the
getEntityRecordselector to match a similar condition in the resolver.Testing Instructions
Integration tests
npm run test:unit -- packages/core-data/src/test/store.jsManual
Testing Instructions for Keyboard
Same.