-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Changes from 18 commits
e24f311
a3f3517
0b13284
c54aa57
ed69cb5
49d80cd
c615efd
d122d87
e11f34c
7703ce2
c2c2f6c
158f088
5c7a2f2
b7bb2cd
5436e18
8803b88
0a08388
1ce1f2b
93eeecc
08e8ca8
562250d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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 | ||
|
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 ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we throw an error here if we detect There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It's the "almost" that's worrying me why can't we just merge the objects? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Right now in Gutenberg we pass There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
} | ||
|
||
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; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
/** | ||
* 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 = { path: '/posts?per_page=-1' }; | ||
const next = ( options ) => { | ||
expect( options.path ).toBe( '/posts?per_page=100' ); | ||
return Promise.resolve( { | ||
status: 200, | ||
headers: { | ||
get() { | ||
return ''; | ||
}, | ||
}, | ||
json() { | ||
return Promise.resolve( { message: 'ok' } ); | ||
}, | ||
} ); | ||
}; | ||
|
||
await fetchAllMiddleware( originalOptions, next ); | ||
} ); | ||
} ); |
There was a problem hiding this comment.
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.