Skip to content

Core Data: Fix early return check for the record field-level resolutions #71541

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Mamaduka merged 5 commits into from
Sep 12, 2025
Merged

Core Data: Fix early return check for the record field-level resolutions #71541

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5004b0d
Introduce 'hasEntityRecord' selector
Mamaduka Sep 8, 2025
d8c6aea
Fix record check inside 'getEntityRecord'
Mamaduka Sep 8, 2025
1c4d439
Add unit test for the resolver
Mamaduka Sep 8, 2025
1294c4a
Add unit tests for the 'hasEntityRecord' selector
Mamaduka Sep 8, 2025
31cdbbb
Use integration tests
Mamaduka Sep 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions docs/reference-guides/data/data-core.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -620,6 +620,24 @@ _Returns_

- `boolean`: Whether the entity record has edits or not.

### hasEntityRecord

Returns true if a record has been received for the given set of parameters, or false otherwise.

Note: This action does not trigger a request for the entity record from the API if it's not available in the local state.

_Parameters_

- _state_ `State`: State tree
- _kind_ `string`: Entity kind.
- _name_ `string`: Entity name.
- _key_ `EntityRecordKey`: Record's key.
- _query_ `GetRecordsHttpQuery`: Optional query.

_Returns_

- `boolean`: Whether an entity record has been received.

### hasEntityRecords

Returns true if records have been received for the given set of parameters, or false otherwise.
Expand Down
18 changes: 18 additions & 0 deletions packages/core-data/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -842,6 +842,24 @@ _Returns_

- `boolean`: Whether the entity record has edits or not.

### hasEntityRecord

Returns true if a record has been received for the given set of parameters, or false otherwise.

Note: This action does not trigger a request for the entity record from the API if it's not available in the local state.

_Parameters_

- _state_ `State`: State tree
- _kind_ `string`: Entity kind.
- _name_ `string`: Entity name.
- _key_ `EntityRecordKey`: Record's key.
- _query_ `GetRecordsHttpQuery`: Optional query.

_Returns_

- `boolean`: Whether an entity record has been received.

### hasEntityRecords

Returns true if records have been received for the given set of parameters, or false otherwise.
Expand Down
31 changes: 11 additions & 20 deletions packages/core-data/src/resolvers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,37 +141,28 @@ export const getEntityRecord =
};
}

// Disable reason: While true that an early return could leave `path`
// unused, it's important that path is derived using the query prior to
// additional query modifications in the condition below, since those
// modifications are relevant to how the data is tracked in state, and not
// for how the request is made to the REST API.

// eslint-disable-next-line @wordpress/no-unused-vars-before-return
const path = addQueryArgs(
entityConfig.baseURL + ( key ? '/' + key : '' ),
{
...entityConfig.baseURLParams,
...query,
}
);

if ( query !== undefined && query._fields ) {
query = { ...query, include: [ key ] };

// The resolution cache won't consider query as reusable based on the
// fields, so it's tested here, prior to initiating the REST request,
// and without causing `getEntityRecords` resolution to occur.
const hasRecords = select.hasEntityRecords(
// and without causing `getEntityRecord` resolution to occur.
const hasRecord = select.hasEntityRecord(
kind,
name,
key,
query
);
if ( hasRecords ) {
if ( hasRecord ) {
return;
}
}

const path = addQueryArgs(
entityConfig.baseURL + ( key ? '/' + key : '' ),
{
...entityConfig.baseURLParams,
...query,
}
);
const response = await apiFetch( { path, parse: false } );
const record = await response.json();
const permissions = getUserPermissionsFromAllowHeader(
Expand Down
56 changes: 56 additions & 0 deletions packages/core-data/src/selectors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -420,6 +420,62 @@ getEntityRecord.__unstableNormalizeArgs = (
return newArgs;
};

/**
* Returns true if a record has been received for the given set of parameters, or false otherwise.
*
* Note: This action does not trigger a request for the entity record from the API
* if it's not available in the local state.
*
* @param state State tree
* @param kind Entity kind.
* @param name Entity name.
* @param key Record's key.
* @param query Optional query.
*
* @return Whether an entity record has been received.
*/
export function hasEntityRecord(
state: State,
kind: string,
name: string,
key?: EntityRecordKey,
query?: GetRecordsHttpQuery
): boolean {
const queriedState =
state.entities.records?.[ kind ]?.[ name ]?.queriedData;
if ( ! queriedState ) {
return false;
}
const context = query?.context ?? 'default';

// If expecting a complete item, validate that completeness.
if ( ! query || ! query._fields ) {
return !! queriedState.itemIsComplete[ context ]?.[ key ];
}

const item = queriedState.items[ context ]?.[ key ];
Copy link
Contributor

@priethor priethor Sep 10, 2025

Choose a reason for hiding this comment

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

Thinking out loud: if we are checking for an item with specific fields and they are all cached, is it relevant that the context is not the same if we have the data?

Copy link
Member Author

@Mamaduka Mamaduka Sep 10, 2025

Choose a reason for hiding this comment

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

The data is stored by context, so looking it up by context is important. When there's no data for the same context, we'll have to make a request.

if ( ! item ) {
return false;
}

// When `query._fields` is provided, check that each requested field exists,
// including any nested paths, on the item; return false if any part is missing.
const fields = getNormalizedCommaSeparable( query._fields ) ?? [];
for ( let i = 0; i < fields.length; i++ ) {
const path = fields[ i ].split( '.' );
let value = item;
for ( let p = 0; p < path.length; p++ ) {
const part = path[ p ];
if ( ! value || ! Object.hasOwn( value, part ) ) {
return false;
}
value = value[ part ];
}
}
Comment on lines +464 to +474
Copy link
Member

@aduth aduth Sep 10, 2025

Choose a reason for hiding this comment

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

As a micro-optimization, I was curious if for ... of would be better than iterating index and assigning a variable, but alas the simple for loop continues to prevail (as of Node v22.16.0)

for loop x 12,295,933 ops/sec ±0.46% (99 runs sampled)
for...of x 12,141,949 ops/sec ±0.59% (97 runs sampled)
Benchmarking code 
import Benchmark from "benchmark";

const fields = ["a", "b.c", "d"];
const item = { a: 1, b: { c: 2 }, e: 3 };

const suite = new Benchmark.Suite();

suite.add("before", () => {
  for (let i = 0; i < fields.length; i++) {
    const path = fields[i].split(".");
    let value = item;
    for (let p = 0; p < path.length; p++) {
      const part = path[p];
      if (!value || !Object.hasOwn(value, part)) {
        return false;
      }
      value = value[part];
    }
  }
});

suite.add("after", () => {
  for (const field of fields) {
    const path = field.split(".");
    let value = item;
    for (const part of path) {
      if (!value || !Object.hasOwn(value, part)) {
        return false;
      }
      value = value[part];
    }
  }
});

suite.on("cycle", (event) => {
  console.log(String(event.target));
});

suite.run({ async: true });

Copy link
Member Author

@Mamaduka Mamaduka Sep 10, 2025

Choose a reason for hiding this comment

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

Thanks for checking. That's also my experience that simple for loops perform better for now.


return true;
}

/**
* Returns the Entity's record object by key. Doesn't trigger a resolver nor requests the entity records from the API if the entity record isn't available in the local state.
*
Expand Down
6 changes: 1 addition & 5 deletions packages/core-data/src/test/resolvers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,10 +79,6 @@ describe( 'getEntityRecord', () => {
it( 'accepts a query that overrides default api path', async () => {
const query = { context: 'view', _envelope: '1' };

const select = {
hasEntityRecords: jest.fn( () => {} ),
};

// Provide response
triggerFetch.mockImplementation( () => POST_TYPE_RESPONSE );

Expand All @@ -91,7 +87,7 @@ describe( 'getEntityRecord', () => {
'postType',
'post',
query
)( { dispatch, select, registry, resolveSelect } );
)( { dispatch, registry, resolveSelect } );

// Trigger apiFetch, test that the query is present in the url.
expect( triggerFetch ).toHaveBeenCalledWith( {
Expand Down
128 changes: 128 additions & 0 deletions packages/core-data/src/test/selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import deepFreeze from 'deep-freeze';
*/
import {
getEntityRecord,
hasEntityRecord,
hasEntityRecords,
getEntityRecords,
getRawEntityRecord,
Expand Down Expand Up @@ -279,6 +280,133 @@ describe( 'getEntityRecord', () => {
} );
} );

describe( 'hasEntityRecord', () => {
it( 'returns false if entity record has not been received', () => {
const state = deepFreeze( {
entities: {
records: {
postType: {
post: {
queriedData: {
items: {},
itemIsComplete: {},
queries: {},
},
},
},
},
},
} );
expect( hasEntityRecord( state, 'postType', 'post', 1 ) ).toBe( false );
} );

it( 'returns true when full record exists and no fields query', () => {
const state = deepFreeze( {
entities: {
records: {
postType: {
post: {
queriedData: {
items: {
default: {
1: { id: 1, content: 'hello' },
},
},
itemIsComplete: {
default: {
1: true,
},
},
queries: {},
},
},
},
},
},
} );
expect( hasEntityRecord( state, 'postType', 'post', 1 ) ).toBe( true );
} );

it( 'returns true when requested fields exist on the item', () => {
const state = deepFreeze( {
entities: {
records: {
postType: {
post: {
queriedData: {
items: {
default: {
1: {
id: 1,
content: 'chicken',
title: { raw: 'egg' },
author: 'bob',
},
},
},
itemIsComplete: {
default: {
1: true,
},
},
queries: {},
},
},
},
},
},
} );
expect(
hasEntityRecord( state, 'postType', 'post', 1, {
_fields: [ 'id', 'content' ],
} )
).toBe( true );
// Test nested field.
expect(
hasEntityRecord( state, 'postType', 'post', 1, {
_fields: [ 'id', 'title.raw' ],
} )
).toBe( true );
} );

it( 'returns false when a requested fields are missing', () => {
const state = deepFreeze( {
entities: {
records: {
postType: {
post: {
queriedData: {
items: {
default: {
1: { id: 1, author: 'bob' },
},
},
itemIsComplete: {
default: {
1: true,
},
},
queries: {},
},
},
},
},
},
} );
expect(
hasEntityRecord( state, 'postType', 'post', 1, {
_fields: [ 'id', 'content' ],
} )
).toBe( false );
// Test nested field.
expect(
hasEntityRecord( state, 'postType', 'post', 1, {
_fields: [ 'id', 'title.raw' ],
} )
).toBe( false );
} );
} );

describe( 'hasEntityRecords', () => {
it( 'returns false if entity records have not been received', () => {
const state = deepFreeze( {
Expand Down
Loading
Loading