Skip to content
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.

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

Implement fetchAllMiddleware to handle per_page=-1 through pagination #10762

Merged
merged 21 commits into from
Oct 21, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
e24f311
Implement fetchAllMiddleware to handle per_page=-1 through pagination
kadamwhite Oct 18, 2018
a3f3517
Move fetchAllMiddleware to its own module
kadamwhite Oct 18, 2018
0b13284
Use @wordpress/url module to modify query strings within fetchAllMidd…
kadamwhite Oct 18, 2018
c54aa57
Make fetchAllMiddleware optional within apiFetch and add it with inli…
kadamwhite Oct 19, 2018
ed69cb5
Improve comments for fetchAllMiddleware module
kadamwhite Oct 19, 2018
49d80cd
Improve fetchAllMiddleware: use links for pagination.
kadamwhite Oct 19, 2018
c615efd
Remove leftover debugging comment
kadamwhite Oct 19, 2018
d122d87
Apply fetchAllMiddleware within apiFetch, not client-scripts.php
kadamwhite Oct 19, 2018
e11f34c
Simplify obtuse code in query modification logic
kadamwhite Oct 19, 2018
7703ce2
Fix improperly reverted parsing code change
kadamwhite Oct 19, 2018
c2c2f6c
Fix unit tests
youknowriad Oct 21, 2018
158f088
Avoid circular dependency
youknowriad Oct 21, 2018
5c7a2f2
Adding some unit tests
youknowriad Oct 21, 2018
b7bb2cd
Remove useless per_page argument
youknowriad Oct 21, 2018
5436e18
Update the lock file
youknowriad Oct 21, 2018
8803b88
Update api-fetch CHANGELOG
youknowriad Oct 21, 2018
0a08388
Fetch as many attachments as we can, even if its not all of them
danielbachhuber Oct 21, 2018
1ce1f2b
Remove `per_page=-1` support shim in REST API
danielbachhuber Oct 21, 2018
93eeecc
Ensure the next page of results is actually fetched
danielbachhuber Oct 21, 2018
08e8ca8
Tweak the middleware chain to avoid mutations
youknowriad Oct 21, 2018
562250d
Testing fetching consequential pages
youknowriad Oct 21, 2018
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
2 changes: 1 addition & 1 deletion lib/client-assets.php
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,7 @@ function gutenberg_register_scripts_and_styles() {
gutenberg_override_script(
'wp-api-fetch',
gutenberg_url( 'build/api-fetch/index.js' ),
array( 'wp-polyfill', 'wp-hooks', 'wp-i18n' ),
array( 'wp-polyfill', 'wp-hooks', 'wp-i18n', 'wp-url' ),
Copy link
Contributor

Choose a reason for hiding this comment

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

@atimmer Something we should update upstream.

filemtime( gutenberg_dir_path() . 'build/api-fetch/index.js' ),
true
);
Expand Down
148 changes: 0 additions & 148 deletions lib/rest-api.php
Original file line number Diff line number Diff line change
Expand Up @@ -290,158 +290,10 @@ function gutenberg_register_post_prepare_functions( $post_type ) {
add_filter( "rest_prepare_{$post_type}", 'gutenberg_add_permalink_template_to_posts', 10, 3 );
add_filter( "rest_prepare_{$post_type}", 'gutenberg_add_block_format_to_post_content', 10, 3 );
add_filter( "rest_prepare_{$post_type}", 'gutenberg_add_target_schema_to_links', 10, 3 );
add_filter( "rest_{$post_type}_collection_params", 'gutenberg_filter_post_collection_parameters', 10, 2 );
add_filter( "rest_{$post_type}_query", 'gutenberg_filter_post_query_arguments', 10, 2 );
return $post_type;
}
add_filter( 'registered_post_type', 'gutenberg_register_post_prepare_functions' );

/**
* Whenever a taxonomy is registered, ensure we're hooked into its WP REST API response.
*
* @param string $taxonomy The newly registered taxonomy.
*/
function gutenberg_register_taxonomy_prepare_functions( $taxonomy ) {
add_filter( "rest_{$taxonomy}_collection_params", 'gutenberg_filter_term_collection_parameters', 10, 2 );
add_filter( "rest_{$taxonomy}_query", 'gutenberg_filter_term_query_arguments', 10, 2 );
}
add_filter( 'registered_taxonomy', 'gutenberg_register_taxonomy_prepare_functions' );

/**
* Handle any necessary checks early.
*
* @param WP_HTTP_Response $response Result to send to the client. Usually a WP_REST_Response.
* @param WP_REST_Server $handler ResponseHandler instance (usually WP_REST_Server).
* @param WP_REST_Request $request Request used to generate the response.
*/
function gutenberg_handle_early_callback_checks( $response, $handler, $request ) {
if ( 0 === strpos( $request->get_route(), '/wp/v2/' ) ) {
$can_unbounded_query = false;
$types = get_post_types( array( 'show_in_rest' => true ), 'objects' );
foreach ( $types as $type ) {
if ( current_user_can( $type->cap->edit_posts ) ) {
$can_unbounded_query = true;
}
}
if ( $request['per_page'] < 0 ) {
if ( ! $can_unbounded_query ) {
return new WP_Error( 'rest_forbidden_per_page', __( 'Sorry, you are not allowed to make unbounded queries.', 'gutenberg' ), array( 'status' => rest_authorization_required_code() ) );
}
}
}
return $response;
}
add_filter( 'rest_request_before_callbacks', 'gutenberg_handle_early_callback_checks', 10, 3 );

/**
* Include additional query parameters on the posts query endpoint.
*
* @see https://core.trac.wordpress.org/ticket/43998
*
* @param array $query_params JSON Schema-formatted collection parameters.
* @param WP_Post_Type $post_type Post type object being accessed.
* @return array
*/
function gutenberg_filter_post_collection_parameters( $query_params, $post_type ) {
if (
isset( $query_params['per_page'] ) &&
( $post_type->hierarchical || 'wp_block' === $post_type->name )
) {
// Change from '1' to '-1', which means unlimited.
$query_params['per_page']['minimum'] = -1;
// Default sanitize callback is 'absint', which won't work in our case.
$query_params['per_page']['sanitize_callback'] = 'rest_sanitize_request_arg';
}
return $query_params;
}

/**
* Filter post collection query parameters to include specific behavior.
*
* @see https://core.trac.wordpress.org/ticket/43998
*
* @param array $prepared_args Array of arguments for WP_Query.
* @param WP_REST_Request $request The current request.
* @return array
*/
function gutenberg_filter_post_query_arguments( $prepared_args, $request ) {
if (
is_post_type_hierarchical( $prepared_args['post_type'] ) ||
'wp_block' === $prepared_args['post_type']
) {
// Avoid triggering 'rest_post_invalid_page_number' error
// which will need to be addressed in https://core.trac.wordpress.org/ticket/43998.
if ( -1 === $prepared_args['posts_per_page'] ) {
$prepared_args['posts_per_page'] = 100000;
}
}
return $prepared_args;
}

/**
* Include additional query parameters on the terms query endpoint.
*
* @see https://core.trac.wordpress.org/ticket/43998
*
* @param array $query_params JSON Schema-formatted collection parameters.
* @param object $taxonomy Taxonomy being accessed.
* @return array
*/
function gutenberg_filter_term_collection_parameters( $query_params, $taxonomy ) {
if ( $taxonomy->show_in_rest
&& ( false === $taxonomy->rest_controller_class
|| 'WP_REST_Terms_Controller' === $taxonomy->rest_controller_class )
&& isset( $query_params['per_page'] ) ) {
// Change from '1' to '-1', which means unlimited.
$query_params['per_page']['minimum'] = -1;
// Default sanitize callback is 'absint', which won't work in our case.
$query_params['per_page']['sanitize_callback'] = 'rest_sanitize_request_arg';
}
return $query_params;
}

/**
* Filter term collection query parameters to include specific behavior.
*
* @see https://core.trac.wordpress.org/ticket/43998
*
* @param array $prepared_args Array of arguments for WP_Term_Query.
* @param WP_REST_Request $request The current request.
* @return array
*/
function gutenberg_filter_term_query_arguments( $prepared_args, $request ) {
// Can't check the actual taxonomy here because it's not
// passed through in $prepared_args (or the filter generally).
if ( 0 === strpos( $request->get_route(), '/wp/v2/' ) ) {
if ( -1 === $prepared_args['number'] ) {
// This should be unset( $prepared_args['number'] )
// but WP_REST_Terms Controller needs to be updated to support
// unbounded queries.
// Will be addressed in https://core.trac.wordpress.org/ticket/43998.
$prepared_args['number'] = 100000;
}
}
return $prepared_args;
}

/**
* Include additional query parameters on the user query endpoint.
*
* @see https://core.trac.wordpress.org/ticket/43998
*
* @param array $query_params JSON Schema-formatted collection parameters.
* @return array
*/
function gutenberg_filter_user_collection_parameters( $query_params ) {
if ( isset( $query_params['per_page'] ) ) {
// Change from '1' to '-1', which means unlimited.
$query_params['per_page']['minimum'] = -1;
// Default sanitize callback is 'absint', which won't work in our case.
$query_params['per_page']['sanitize_callback'] = 'rest_sanitize_request_arg';
}
return $query_params;
}
add_filter( 'rest_user_collection_params', 'gutenberg_filter_user_collection_parameters' );

/**
* Silence PHP Warnings and Errors in JSON requests
Expand Down
3 changes: 2 additions & 1 deletion package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions packages/api-fetch/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 2.1.0 (Unreleased)

- Support `per_page=-1` paginated requests.

## 2.0.0 (2018-09-05)

### Breaking Change
Expand Down
3 changes: 2 additions & 1 deletion packages/api-fetch/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@
"dependencies": {
"@babel/runtime": "^7.0.0",
"@wordpress/hooks": "file:../hooks",
"@wordpress/i18n": "file:../i18n"
"@wordpress/i18n": "file:../i18n",
"@wordpress/url": "file:../url"
},
"publishConfig": {
"access": "public"
Expand Down
13 changes: 9 additions & 4 deletions packages/api-fetch/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import { __ } from '@wordpress/i18n';
import createNonceMiddleware from './middlewares/nonce';
import createRootURLMiddleware from './middlewares/root-url';
import createPreloadingMiddleware from './middlewares/preloading';
import fetchAllMiddleware from './middlewares/fetch-all-middleware';
import namespaceEndpointMiddleware from './middlewares/namespace-endpoint';
import httpV1Middleware from './middlewares/http-v1';

Expand Down Expand Up @@ -105,22 +106,26 @@ function apiFetch( options ) {

const steps = [
raw,
fetchAllMiddleware,
httpV1Middleware,
namespaceEndpointMiddleware,
...middlewares,
];
const next = ( nextOptions ) => {
const nextMiddleware = steps.pop();
].reverse();

const runMiddleware = ( index ) => ( nextOptions ) => {
const nextMiddleware = steps[ index ];
const next = runMiddleware( index + 1 );
return nextMiddleware( nextOptions, next );
};

return next( options );
return runMiddleware( 0 )( options );
}

apiFetch.use = registerMiddleware;

apiFetch.createNonceMiddleware = createNonceMiddleware;
apiFetch.createPreloadingMiddleware = createPreloadingMiddleware;
apiFetch.createRootURLMiddleware = createRootURLMiddleware;
apiFetch.fetchAllMiddleware = fetchAllMiddleware;

export default apiFetch;
93 changes: 93 additions & 0 deletions packages/api-fetch/src/middlewares/fetch-all-middleware.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
/**
* WordPress dependencies
*/
import { addQueryArgs } from '@wordpress/url';

// Apply query arguments to both URL and Path, whichever is present.
const modifyQuery = ( { path, url, ...options }, queryArgs ) => ( {
...options,
url: url && addQueryArgs( url, queryArgs ),
path: path && addQueryArgs( path, queryArgs ),
} );

// Duplicates parsing functionality from apiFetch.
const parseResponse = ( response ) => response.json ?
response.json() :
Promise.reject( response );

const parseLinkHeader = ( linkHeader ) => {
if ( ! linkHeader ) {
return {};
}
const match = linkHeader.match( /<([^>]+)>; rel="next"/ );
return match ? {
next: match[ 1 ],
} : {};
};

const getNextPageUrl = ( response ) => {
const { next } = parseLinkHeader( response.headers.get( 'link' ) );
return next;
};

const requestContainsUnboundedQuery = ( options ) => {
const pathIsUnbounded = options.path && options.path.indexOf( 'per_page=-1' ) !== -1;
const urlIsUnbounded = options.url && options.url.indexOf( 'per_page=-1' ) !== -1;
return pathIsUnbounded || urlIsUnbounded;
};

// The REST API enforces an upper limit on the per_page option. To handle large
// collections, apiFetch consumers can pass `per_page=-1`; this middleware will
// then recursively assemble a full response array from all available pages.
const fetchAllMiddleware = async ( options, next ) => {
if ( options.parse === false ) {
// If a consumer has opted out of parsing, do not apply middleware.
return next( options );
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we throw an error here if we detect per_page: -1 in the request as the server won't know how to deal with it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I feel like we should let it through; we don't do that anywhere ourselves, and the server itself will throw the error, which should make the issue pretty clear.

}
if ( ! requestContainsUnboundedQuery( options ) ) {
// If neither url nor path is requesting all items, do not apply middleware.
return next( options );
}

// Retrieve requested page of results.
const response = await next( {
...modifyQuery( options, {
per_page: 100,
} ),
// Ensure headers are returned for page 1.
parse: false,
} );

const results = await parseResponse( response );

if ( ! Array.isArray( results ) ) {
// We have no reliable way of merging non-array results.
return results;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is a good solution. As we changed the intent of the request but didn't took it into consideration when providing the result.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What would you suggest doing in this case? Endpoints which return an object will almost never return a next link header anyway, so there's not a huge impact to this that I can see.

Copy link
Contributor

Choose a reason for hiding this comment

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

Endpoints which return an object will almost never return a next link header anyway

It's the "almost" that's worrying me why can't we just merge the objects?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have not seen an endpoint following our REST API's conventions which would both expose pagination headers and return an object, so I don't know how I should begin to attempt to merge object responses. If we assume that each subsequent page should overwrite the values from the prior, there may be data loss. If we assume that we should only merge array children, that could work, but again, there could be data loss.

Given that we do not have an immediate need to support this for core WordPress endpoints (that I know of), I suggest we look for prior art and revisit the question to add object merging logic once we encounter a specific situation where this becomes an issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have not seen an endpoint following our REST API's conventions which would both expose pagination headers and return an object,

Right now in Gutenberg we pass per_page=-1 in a request to fetch taxonomies. This might be wrong though?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not wrong so much as unneedd -- the taxonomies controller does not accept a per_page argument, so that query parameter gets ignored. We use get_object_taxonomies and get_taxonomies within wp-rest-taxonomies-controller.php, niether of which accepts a limiting argument.

}

let nextPage = getNextPageUrl( response );

if ( ! nextPage ) {
// There are no further pages to request.
return results;
}

// Iteratively fetch all remaining pages until no "next" header is found.
let mergedResults = [].concat( results );
while ( nextPage ) {
const nextResponse = await next( {
...options,
// Ensure the URL for the next page is used instead of any provided path.
path: undefined,
url: nextPage,
// Ensure we still get headers so we can identify the next page.
parse: false,
} );
const nextResults = await parseResponse( nextResponse );
mergedResults = mergedResults.concat( nextResults );
nextPage = getNextPageUrl( nextResponse );
}
return mergedResults;
};

export default fetchAllMiddleware;
51 changes: 51 additions & 0 deletions packages/api-fetch/src/middlewares/test/fetch-all-middleware.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
/**
* Internal dependencies
*/
import fetchAllMiddleware from '../fetch-all-middleware';

describe( 'Fetch All Middleware', async () => {
it( 'should defer with the same options to the next middleware', async () => {
expect.hasAssertions();
const originalOptions = { path: '/posts' };
const next = ( options ) => {
expect( options ).toBe( originalOptions );
return Promise.resolve( 'ok' );
};

await fetchAllMiddleware( originalOptions, next );
} );

it( 'should paginate the request', async () => {
expect.hasAssertions();
const originalOptions = { url: '/posts?per_page=-1' };
let counter = 1;
const next = ( options ) => {
if ( counter === 1 ) {
expect( options.url ).toBe( '/posts?per_page=100' );
} else {
expect( options.url ).toBe( '/posts?per_page=100&page=2' );
}
const response = Promise.resolve( {
status: 200,
headers: {
get() {
return options.url === '/posts?per_page=100' ?
'</posts?per_page=100&page=2>; rel="next"' :
'';
},
},
json() {
return Promise.resolve( [ 'item' ] );
},
} );

counter++;

return response;
};

const result = await fetchAllMiddleware( originalOptions, next );

expect( result ).toEqual( [ 'item', 'item' ] );
} );
} );
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,8 @@ const getAttachmentsCollection = ( ids ) => {
return wp.media.query( {
order: 'ASC',
orderby: 'post__in',
per_page: -1,
danielbachhuber marked this conversation as resolved.
Show resolved Hide resolved
post__in: ids,
per_page: 100,
query: true,
type: 'image',
} );
Expand Down
Loading