Fix resolver resolution status key types mismatch causing unwanted re…

…solver calls for identical state data (#52120)

* Normalize

* Add optional method property to normalize arguments before usage

* Check for ID-like keys rather than numeric things

Accounts for cases such as ‘this-is-a-slug-1234’

* Extract util and update to use suggestion from Code Review

* Test for util

* Add tests for normalizeArgs

* Fix bug with normalizeArgs called even if there are no args

* Add direct test on `normalizeArgs` method of getEntityRecord

* Allow defining normalize method on selector and call for both selector and resolver

* Move normalization function to getEntityRecord selector

* Move normalization functino to getEntityRecord selector

* Add rough outline of documentation

* Add test to ensure normalization is applied to selector even without matching resolver

* Improve documentation to better explain concept of normalizeArgs

* Correct grammar

* Apply types optimisation from code review

See #52120 (comment)

* Extract conditionals to helper function

As per code review

* Document getEntityRecord normalization function

* Add type defs to normalization function

* Fix nits in README

* Remove new line

* Add test for arguments length before invoking normalization function

* Remove unwanted comment

* Addition to docs as per code review

#52120 (comment)

* Fix bug with metadata selector args not being normalized

* Add tests for normalization to metadata selector tests

* Add test case for decimals

* Prefix with __unstable to denote non “settled” approach amongst contributors

* Remove check for args and conditionally access args index

Addresses #52120 (comment)

* Simplify coercing to number

* Revert confusing DRY typescript

* Fix renaming selector to `unstable`

* Copy to new args

Addresses https://github.com/WordPress/gutenberg/pull/52120/files#r1260851646

packages/core-data/src/selectors.ts

@@ -20,6 +20,7 @@ import {
 	getNormalizedCommaSeparable,
 	isRawAttribute,
 	setNestedValue,
+	isNumericID,
 } from './utils';
 import type * as ET from './entity-types';
 import type { UndoManager } from '@wordpress/undo-manager';
@@ -95,6 +96,13 @@ type Optional< T > = T | undefined;
  */
 type GetRecordsHttpQuery = Record< string, any >;
 
+/**
+ * Arguments for EntityRecord selectors.
+ */
+type EntityRecordArgs =
+	| [ string, string, EntityRecordKey ]
+	| [ string, string, EntityRecordKey, GetRecordsHttpQuery ];
+
 /**
  * Shared reference to an empty object for cases where it is important to avoid
  * returning a new object reference on every invocation, as in a connected or
@@ -292,6 +300,7 @@ export interface GetEntityRecord {
 		key: EntityRecordKey,
 		query?: GetRecordsHttpQuery
 	) => EntityRecord | undefined;
+	__unstableNormalizeArgs?: ( args: EntityRecordArgs ) => EntityRecordArgs;
 }
 
 /**
@@ -365,6 +374,24 @@ export const getEntityRecord = createSelector(
 	}
 ) as GetEntityRecord;
 
+/**
+ * Normalizes `recordKey`s that look like numeric IDs to numbers.
+ *
+ * @param args EntityRecordArgs the selector arguments.
+ * @return EntityRecordArgs the normalized arguments.
+ */
+getEntityRecord.__unstableNormalizeArgs = (
+	args: EntityRecordArgs
+): EntityRecordArgs => {
+	const newArgs = [ ...args ] as EntityRecordArgs;
+	const recordKey = newArgs?.[ 2 ];
+
+	// If recordKey looks to be a numeric ID then coerce to number.
+	newArgs[ 2 ] = isNumericID( recordKey ) ? Number( recordKey ) : recordKey;
+
+	return newArgs;
+};
+
 /**
  * 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.
  *

packages/core-data/src/test/selectors.js

@@ -28,6 +28,29 @@ describe.each( [
 	[ getEntityRecord ],
 	[ __experimentalGetEntityRecordNoResolver ],
 ] )( '%p', ( selector ) => {
+	describe( 'normalizing Post ID passed as recordKey', () => {
+		it( 'normalizes any Post ID recordKey argument to a Number via `__unstableNormalizeArgs` method', async () => {
+			const normalized = getEntityRecord.__unstableNormalizeArgs( [
+				'postType',
+				'some_post',
+				'123',
+			] );
+			expect( normalized ).toEqual( [ 'postType', 'some_post', 123 ] );
+		} );
+
+		it( 'does not normalize recordKey argument unless it is a Post ID', async () => {
+			const normalized = getEntityRecord.__unstableNormalizeArgs( [
+				'postType',
+				'some_post',
+				'i-am-a-slug-with-a-number-123',
+			] );
+			expect( normalized ).toEqual( [
+				'postType',
+				'some_post',
+				'i-am-a-slug-with-a-number-123',
+			] );
+		} );
+	} );
 	it( 'should return undefined for unknown entity kind, name', () => {
 		const state = deepFreeze( {
 			entities: {

packages/core-data/src/utils/index.js

@@ -8,3 +8,4 @@ export { default as withWeakMapCache } from './with-weak-map-cache';
 export { default as isRawAttribute } from './is-raw-attribute';
 export { default as setNestedValue } from './set-nested-value';
 export { default as getNestedValue } from './get-nested-value';
+export { default as isNumericID } from './is-numeric-id';

packages/core-data/src/utils/is-numeric-id.js

@@ -0,0 +1,10 @@
+/**
+ * Checks argument to determine if it's a numeric ID.
+ * For example, '123' is a numeric ID, but '123abc' is not.
+ *
+ * @param {any} id the argument to determine if it's a numeric ID.
+ * @return {boolean} true if the string is a numeric ID, false otherwise.
+ */
+export default function isNumericID( id ) {
+	return /^\s*\d+\s*$/.test( id );
+}

packages/core-data/src/utils/test/is-numeric-id.js

@@ -0,0 +1,22 @@
+/**
+ * Internal dependencies
+ */
+import isNumericID from '../is-numeric-id';
+
+describe( 'isNumericID', () => {
+	test.each( [
+		[ true, '12345' ],
+		[ true, '42' ],
+		[ true, '   42   ' ],
+		[ false, 'abc123' ],
+		[ false, '123abc' ],
+		[ false, '' ],
+		[ false, '123-abc' ],
+		[ false, 'abc-123' ],
+		[ false, '42-test-123' ],
+		[ false, '3.42' ],
+		[ true, 123 ],
+	] )( `should return %s for input "%s"`, ( expected, input ) => {
+		expect( isNumericID( input ) ).toBe( expected );
+	} );
+} );

packages/data/README.md

@@ -1127,6 +1127,86 @@ _Returns_
 
 -   `boolean`: Whether resolution is in progress.
 
+### Normalizing Selector Arguments
+
+In specific circumstances it may be necessary to normalize the arguments passed to a given _call_ of a selector/resolver pairing.
+
+Each resolver has [its resolution status cached in an internal state](https://github.com/WordPress/gutenberg/blob/e244388d8669618b76c966cc33d48df9156c2db6/packages/data/src/redux-store/metadata/reducer.ts#L39) where the [key is the arguments supplied to the selector](https://github.com/WordPress/gutenberg/blob/e244388d8669618b76c966cc33d48df9156c2db6/packages/data/src/redux-store/metadata/utils.ts#L48) at _call_ time.
+
+For example for a selector with a single argument, the related resolver would generate a cache key of: `[ 123 ]`.
+
+[This cache is used to determine the resolution status of a given resolver](https://github.com/WordPress/gutenberg/blob/e244388d8669618b76c966cc33d48df9156c2db6/packages/data/src/redux-store/metadata/selectors.js#L22-L29) which is used to [avoid unwanted additional invocations of resolvers](https://github.com/WordPress/gutenberg/blob/e244388d8669618b76c966cc33d48df9156c2db6/packages/data/src/redux-store/index.js#L469-L474) (which often undertake "expensive" operations such as network requests).
+
+As a result it's important that arguments remain _consistent_ when calling the selector. For example, by _default_ these two calls will not be cached using the same key, even though they are likely identical:
+
+```js
+// Arg as number
+getSomeDataById( 123 );
+
+// Arg as string
+getSomeDataById( '123' );
+```
+
+This is an opportunity to utilize the `__unstableNormalizeArgs` property to guarantee consistency by protecting callers from passing incorrect types.
+
+#### Example
+
+The _3rd_ argument of the following selector is intended to be a `Number`:
+
+```js
+const getItemsSelector = ( name, type, id ) => {
+	return state.items[ name ][ type ][ id ] || null;
+};
+```
+
+However, it is possible that the `id` parameter will be passed as a `String`. In this case, the `__unstableNormalizeArgs` method (property) can be defined on the _selector_ to coerce the arguments to the desired type even if they are provided "incorrectly":
+
+```js
+// Define normalization method.
+getItemsSelector.__unstableNormalizeArgs = ( args ) {
+	// "id" argument at the 2nd index
+	if (args[2] && typeof args[2] === 'string' ) {
+		args[2] === Number(args[2]);
+	}
+
+	return args;
+}
+```
+
+With this in place the following code will behave consistently:
+
+```js
+const getItemsSelector = ( name, type, id ) => {
+	// here 'id' is now guaranteed to be a number.
+	return state.items[ name ][ type ][ id ] || null;
+};
+
+const getItemsResolver = ( name, type, id ) => {
+	// 'id' is also guaranteed to be a number in the resolver.
+	return {};
+};
+
+registry.registerStore( 'store', {
+	// ...
+	selectors: {
+		getItems: getItemsSelector,
+	},
+	resolvers: {
+		getItems: getItemsResolver,
+	},
+} );
+
+// Call with correct number type.
+registry.select( 'store' ).getItems( 'foo', 'bar', 54 );
+
+// Call with the wrong string type, **but** here we have avoided an
+// wanted resolver call because '54' is guaranteed to have been
+// coerced to a number by the `__unstableNormalizeArgs` method.
+registry.select( 'store' ).getItems( 'foo', 'bar', '54' );
+```
+
+Ensuring consistency of arguments for a given selector call is [an important optimization to help improve performance in the data layer](https://github.com/WordPress/gutenberg/pull/52120). However, this type of problem can be usually be avoided by ensuring selectors don't use variable types for their arguments.
+
 ## Going further
 
 -   [What is WordPress Data?](https://unfoldingneurons.com/2020/what-is-wordpress-data/)

packages/data/src/redux-store/index.js

@@ -235,11 +235,19 @@ export default function createReduxStore( key, options ) {
 					selector.registry = registry;
 				}
 				const boundSelector = ( ...args ) => {
+					args = normalize( selector, args );
 					const state = store.__unstableOriginalGetState();
 					return selector( state.root, ...args );
 				};
 
+				// Expose normalization method on the bound selector
+				// in order that it can be called when fullfilling
+				// the resolver.
+				boundSelector.__unstableNormalizeArgs =
+					selector.__unstableNormalizeArgs;
+
 				const resolver = resolvers[ selectorName ];
+
 				if ( ! resolver ) {
 					boundSelector.hasResolver = false;
 					return boundSelector;
@@ -254,10 +262,24 @@ export default function createReduxStore( key, options ) {
 				);
 			}
 
-			function bindMetadataSelector( selector ) {
+			function bindMetadataSelector( metaDataSelector ) {
 				const boundSelector = ( ...args ) => {
 					const state = store.__unstableOriginalGetState();
-					return selector( state.metadata, ...args );
+
+					const originalSelectorName = args && args[ 0 ];
+					const originalSelectorArgs = args && args[ 1 ];
+					const targetSelector =
+						options?.selectors?.[ originalSelectorName ];
+
+					// Normalize the arguments passed to the target selector.
+					if ( originalSelectorName && targetSelector ) {
+						args[ 1 ] = normalize(
+							targetSelector,
+							originalSelectorArgs
+						);
+					}
+
+					return metaDataSelector( state.metadata, ...args );
 				};
 				boundSelector.hasResolver = false;
 				return boundSelector;
@@ -604,9 +626,29 @@ function mapSelectorWithResolver(
 	}
 
 	const selectorResolver = ( ...args ) => {
+		args = normalize( selector, args );
 		fulfillSelector( args );
 		return selector( ...args );
 	};
 	selectorResolver.hasResolver = true;
 	return selectorResolver;
 }
+
+/**
+ * Applies selector's normalization function to the given arguments
+ * if it exists.
+ *
+ * @param {Object} selector The selector potentially with a normalization method property.
+ * @param {Array}  args     selector arguments to normalize.
+ * @return {Array} Potentially normalized arguments.
+ */
+function normalize( selector, args ) {
+	if (
+		selector.__unstableNormalizeArgs &&
+		typeof selector.__unstableNormalizeArgs === 'function' &&
+		args?.length
+	) {
+		return selector.__unstableNormalizeArgs( args );
+	}
+	return args;
+}

packages/data/src/redux-store/metadata/test/selectors.js

@@ -3,6 +3,8 @@
  */
 import { createRegistry } from '@wordpress/data';
 
+const getFooSelector = ( state ) => state;
+
 const testStore = {
 	reducer: ( state = null, action ) => {
 		if ( action.type === 'RECEIVE' ) {
@@ -12,7 +14,7 @@ const testStore = {
 		return state;
 	},
 	selectors: {
-		getFoo: ( state ) => state,
+		getFoo: getFooSelector,
 	},
 };
 
@@ -55,7 +57,7 @@ describe( 'getIsResolving', () => {
 		expect( result ).toBe( true );
 	} );
 
-	it( 'should normalize args ard return the right value', () => {
+	it( 'should normalize args and return the right value', () => {
 		registry.dispatch( 'testStore' ).startResolution( 'getFoo', [] );
 		const { getIsResolving } = registry.select( 'testStore' );
 
@@ -443,3 +445,37 @@ describe( 'countSelectorsByStatus', () => {
 		expect( result1 ).not.toBe( result2 );
 	} );
 } );
+
+describe( 'Selector arguments normalization', () => {
+	let registry;
+	beforeEach( () => {
+		registry = createRegistry();
+		registry.registerStore( 'testStore', testStore );
+	} );
+
+	it( 'should call normalization method on target selector if exists', () => {
+		const normalizationFunction = jest.fn( ( args ) => {
+			return args.map( Number );
+		} );
+		getFooSelector.__unstableNormalizeArgs = normalizationFunction;
+
+		registry.dispatch( 'testStore' ).startResolution( 'getFoo', [ 123 ] );
+		const { getIsResolving, hasStartedResolution, hasFinishedResolution } =
+			registry.select( 'testStore' );
+
+		expect( getIsResolving( 'getFoo', [ '123' ] ) ).toBe( true );
+		expect( normalizationFunction ).toHaveBeenCalledWith( [ '123' ] );
+
+		expect( hasStartedResolution( 'getFoo', [ '123' ] ) ).toBe( true );
+		expect( normalizationFunction ).toHaveBeenCalledWith( [ '123' ] );
+
+		expect( normalizationFunction ).toHaveBeenCalledTimes( 2 );
+
+		registry.dispatch( 'testStore' ).finishResolution( 'getFoo', [ 123 ] );
+
+		expect( hasFinishedResolution( 'getFoo', [ '123' ] ) ).toBe( true );
+		expect( normalizationFunction ).toHaveBeenCalledWith( [ '123' ] );
+
+		getFooSelector.__unstableNormalizeArgs = undefined;
+	} );
+} );