Skip to content

Commit

Permalink
Core Data: Stop sending duplicate requests in canUser resolver (#43480)
Browse files Browse the repository at this point in the history 
* Don't run duplicate requests in canUser resolver
* Remove the checkAllowedActions function
* Update packages/core-data/src/resolvers.js
* Check resolution status for resources without an ID
* Throw an error early
* Receive update and delete permissions even if no ID was passed

Co-authored-by: George Mamadashvili <[email protected]>
  • Loading branch information
@adamziel @Mamaduka
adamziel and Mamaduka authored Aug 30, 2022
1 parent 8760105 commit e97cfa1
Show file tree
Hide file tree
Showing 2 changed files with 164 additions and 27 deletions.
69 changes: 46 additions & 23 deletions packages/core-data/src/resolvers.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -273,34 +273,42 @@ export const getEmbedPreview =
* Checks whether the current user can perform the given action on the given
* REST resource.
*
* @param {string} action Action to check. One of: 'create', 'read', 'update',
* 'delete'.
* @param {string} resource REST resource to check, e.g. 'media' or 'posts'.
* @param {?string} id ID of the rest resource to check.
* @param {string} requestedAction Action to check. One of: 'create', 'read', 'update',
* 'delete'.
* @param {string} resource REST resource to check, e.g. 'media' or 'posts'.
* @param {?string} id ID of the rest resource to check.
*/
export const canUser =
( action, resource, id ) =>
async ( { dispatch } ) => {
const methods = {
create: 'POST',
read: 'GET',
update: 'PUT',
delete: 'DELETE',
};
( requestedAction, resource, id ) =>
async ( { dispatch, registry } ) => {
const { hasStartedResolution } = registry.select( STORE_NAME );

const resourcePath = id ? `${ resource }/${ id }` : resource;
const retrievedActions = [ 'create', 'read', 'update', 'delete' ];

const method = methods[ action ];
if ( ! method ) {
throw new Error( `'${ action }' is not a valid action.` );
if ( ! retrievedActions.includes( requestedAction ) ) {
throw new Error( `'${ requestedAction }' is not a valid action.` );
}

const path = id
? `/wp/v2/${ resource }/${ id }`
: `/wp/v2/${ resource }`;
// Prevent resolving the same resource twice.
for ( const relatedAction of retrievedActions ) {
if ( relatedAction === requestedAction ) {
continue;
}
const isAlreadyResolving = hasStartedResolution( 'canUser', [
relatedAction,
resource,
id,
] );
if ( isAlreadyResolving ) {
return;
}
}

let response;
try {
response = await apiFetch( {
path,
path: `/wp/v2/${ resourcePath }`,
method: 'OPTIONS',
parse: false,
} );
Expand All @@ -314,10 +322,25 @@ export const canUser =
// return the expected result in the native version. Instead, API requests
// only return the result, without including response properties like the headers.
const allowHeader = response.headers?.get( 'allow' );
const key = [ action, resource, id ].filter( Boolean ).join( '/' );
const isAllowed =
allowHeader?.includes?.( method ) || allowHeader?.allow === method;
dispatch.receiveUserPermission( key, isAllowed );
const allowedMethods = allowHeader?.allow || allowHeader || '';

const permissions = {};
const methods = {
create: 'POST',
read: 'GET',
update: 'PUT',
delete: 'DELETE',
};
for ( const [ actionName, methodName ] of Object.entries( methods ) ) {
permissions[ actionName ] = allowedMethods.includes( methodName );
}

for ( const action of retrievedActions ) {
dispatch.receiveUserPermission(
`${ action }/${ resourcePath }`,
permissions[ action ]
);
}
};

/**
Expand Down
122 changes: 118 additions & 4 deletions packages/core-data/src/test/resolvers.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -291,7 +291,13 @@ describe( 'getEmbedPreview', () => {
} );

describe( 'canUser', () => {
let registry;
beforeEach( async () => {
registry = {
select: jest.fn( () => ( {
hasStartedResolution: () => false,
} ) ),
};
triggerFetch.mockReset();
} );

Expand All @@ -304,7 +310,7 @@ describe( 'canUser', () => {
Promise.reject( { status: 404 } )
);

await canUser( 'create', 'media' )( { dispatch } );
await canUser( 'create', 'media' )( { dispatch, registry } );

expect( triggerFetch ).toHaveBeenCalledWith( {
path: '/wp/v2/media',
Expand All @@ -324,7 +330,7 @@ describe( 'canUser', () => {
headers: new Map( [ [ 'allow', 'GET' ] ] ),
} ) );

await canUser( 'create', 'media' )( { dispatch } );
await canUser( 'create', 'media' )( { dispatch, registry } );

expect( triggerFetch ).toHaveBeenCalledWith( {
path: '/wp/v2/media',
Expand All @@ -347,7 +353,7 @@ describe( 'canUser', () => {
headers: new Map( [ [ 'allow', 'POST, GET, PUT, DELETE' ] ] ),
} ) );

await canUser( 'create', 'media' )( { dispatch } );
await canUser( 'create', 'media' )( { dispatch, registry } );

expect( triggerFetch ).toHaveBeenCalledWith( {
path: '/wp/v2/media',
Expand All @@ -370,7 +376,7 @@ describe( 'canUser', () => {
headers: new Map( [ [ 'allow', 'POST, GET, PUT, DELETE' ] ] ),
} ) );

await canUser( 'create', 'blocks', 123 )( { dispatch } );
await canUser( 'create', 'blocks', 123 )( { dispatch, registry } );

expect( triggerFetch ).toHaveBeenCalledWith( {
path: '/wp/v2/blocks/123',
Expand All @@ -383,6 +389,114 @@ describe( 'canUser', () => {
true
);
} );

it( 'runs apiFetch only once per resource', async () => {
const dispatch = Object.assign( jest.fn(), {
receiveUserPermission: jest.fn(),
} );

registry = {
select: () => ( {
hasStartedResolution: ( _, [ action ] ) => action === 'read',
} ),
};

triggerFetch.mockImplementation( () => ( {
headers: new Map( [ [ 'allow', 'POST, GET' ] ] ),
} ) );

await canUser( 'create', 'blocks' )( { dispatch, registry } );
await canUser( 'read', 'blocks' )( { dispatch, registry } );

expect( triggerFetch ).toHaveBeenCalledTimes( 1 );

expect( dispatch.receiveUserPermission ).toHaveBeenCalledWith(
'create/blocks',
true
);
expect( dispatch.receiveUserPermission ).toHaveBeenCalledWith(
'read/blocks',
true
);
} );

it( 'retrieves all permissions even when ID is not given', async () => {
const dispatch = Object.assign( jest.fn(), {
receiveUserPermission: jest.fn(),
} );

registry = {
select: () => ( {
hasStartedResolution: ( _, [ action ] ) => action === 'read',
} ),
};

triggerFetch.mockImplementation( () => ( {
headers: new Map( [ [ 'allow', 'POST, GET' ] ] ),
} ) );

await canUser( 'create', 'blocks' )( { dispatch, registry } );
await canUser( 'read', 'blocks' )( { dispatch, registry } );
await canUser( 'update', 'blocks' )( { dispatch, registry } );
await canUser( 'delete', 'blocks' )( { dispatch, registry } );

expect( dispatch.receiveUserPermission ).toHaveBeenCalledWith(
'create/blocks',
true
);
expect( dispatch.receiveUserPermission ).toHaveBeenCalledWith(
'read/blocks',
true
);
expect( dispatch.receiveUserPermission ).toHaveBeenCalledWith(
'update/blocks',
false
);
expect( dispatch.receiveUserPermission ).toHaveBeenCalledWith(
'delete/blocks',
false
);
} );

it( 'runs apiFetch only once per resource ID', async () => {
const dispatch = Object.assign( jest.fn(), {
receiveUserPermission: jest.fn(),
} );

registry = {
select: () => ( {
hasStartedResolution: ( _, [ action ] ) => action === 'create',
} ),
};

triggerFetch.mockImplementation( () => ( {
headers: new Map( [ [ 'allow', 'POST, GET, PUT, DELETE' ] ] ),
} ) );

await canUser( 'create', 'blocks', 123 )( { dispatch, registry } );
await canUser( 'read', 'blocks', 123 )( { dispatch, registry } );
await canUser( 'update', 'blocks', 123 )( { dispatch, registry } );
await canUser( 'delete', 'blocks', 123 )( { dispatch, registry } );

expect( triggerFetch ).toHaveBeenCalledTimes( 1 );

expect( dispatch.receiveUserPermission ).toHaveBeenCalledWith(
'create/blocks/123',
true
);
expect( dispatch.receiveUserPermission ).toHaveBeenCalledWith(
'read/blocks/123',
true
);
expect( dispatch.receiveUserPermission ).toHaveBeenCalledWith(
'update/blocks/123',
true
);
expect( dispatch.receiveUserPermission ).toHaveBeenCalledWith(
'delete/blocks/123',
true
);
} );
} );

describe( 'getAutosaves', () => {
Expand Down

0 comments on commit e97cfa1

Please sign in to comment.