Skip to content
This repository was archived by the owner on Oct 10, 2022. It is now read-only.

Support pagination #39

Closed
bcomnes opened this issue Feb 25, 2019 · 7 comments
Closed

Support pagination #39

bcomnes opened this issue Feb 25, 2019 · 7 comments
Labels
area: api type: bug code to address defects in shipped code

Comments

@bcomnes
Copy link
Contributor

bcomnes commented Feb 25, 2019

We need to support pagination on calls.

@DavidWells
Copy link
Contributor

Bumping this.

If you have more than 100 site associated with your account some CLI commands don't function properly

  • netlify sites:list - only pulls 100
  • netlify link and connect to site via git remote fails because repo not in the 100 sites returned.

Current workaround for netlify link is manually connecting the site-id

This behavior is also visible on https://oauth-example.netlify.com/ also using the js-client

A strategy for pagination is overdue 😃

@erezrokah erezrokah added type: bug code to address defects in shipped code area: api labels Aug 19, 2020
@grahaml
Copy link

grahaml commented Sep 25, 2020

Obligatory +1 -- I have 125 sites... I need that other 25 so badly.

@grahaml
Copy link

grahaml commented Sep 25, 2020

Sorry for the cheeky post. Let me elaborate. I have been talking to Netlify support about this as well, but I thought it would be good to share the details directly on the issue here.

I am working on some analysis on all the sites in our account, specifically for user access management, and I have found that neither the JS client nor the CLI tool seem to support pagination. We have 125 sites live at the moment.

What I need to do is get a list of all sites under my account, which I believe the OpenAPI Endpoint listSitesForAccount should return.

When trying to use the JS client (or the CLI tool), calls that include page: n in the request return the same first 100 results (the first page).

Steps to Reproduce

Forgive the contrived example - trying to be explicit about pages for clarity

const Netlify = require('netlify');
const client = new Netlify('my_access_token');
const sitesPage1 = await client.listSitesForAccount({account_slug: 'my_account_slug'});
const sitesPage2 = await client.listSitesForAccount({account_slug: 'my_account_slug', page: 2});
const allSites = [...sitesPage1, ...sitesPage2];

Expected Results

  • sitesPage1 should contain 100 entries, the first page of sites
  • sitesPage2 should contain the remaining 25 sites that were not captured in the first page
  • Additionally I would expect the response from this call to inform me that the results are paginated, and that there are more pages to fetch

Actual Results

  • sitesPage1 does have the first 100 entries (the first page of sites)
  • sitesPage2 has the exact same 100 entries
  • There is no information about results being paginated or how to fetch the next page

Regarding the information about pagination, I realize that the docs say the Link header in the response should contain information about subsequent pages, as per https://docs.netlify.com/api/get-started/#link-header. The response headers, however, are not exposed in the JS client. Only the response body seems to be returned.


The same seems to be true with netlify-cli. I realize that this is in a different repository, but in case anyone else comes across this, I figure it doesn't hurt to include the information here as well.

This is my experience trying to use the CLI tool.

$ netlify login
# ...
$ netlify api listSitesForAccount --data '{"account_slug": "my_account_slug"}'
# ... returns the first 100 results
$ netlify api listSitesForAccount --data '{"account_slug": "my_account_slug", "page": 2}'
# ... returns the same first 100 results as above

@jverce
Copy link

jverce commented Sep 30, 2020

Should this pagination logic be exposed to users? Or could we implement a generator-based function that keeps returning items until the whole dataset is exhausted, and handle the pagination logic internally in the client?

@grahaml
Copy link

grahaml commented Sep 30, 2020

This is a good question @jverce, and I think the answer depends on this library's purpose and intent.

If the intent is to provide an SDK that mirrors Netlify's Open API spec, then I would say that we should expose the headers or include some kind of indicator in the response body that there is another page to fetch, which would of course be a breaking change as OP indicated.

If the intent is to provide an SDK that acts as an abstraction layer over the Open API spec, then I'd argue that handling the pagination logic internally would make sense.

Having said that - I don't think the Open API spec mentions pagination at all. The only thing I could find is in the Netlify docs. So with that in mind, I suppose technically this behaves as expected (because it is as per the spec).

To anyone looking to get around this - I opted to forego this library and call the API directly, parsing the headers to determine if another call should be made. This is roughly what I came up with (naively assuming a GET request and appending page as a query param, and removing error handling for brevity):

function hasNextPage(headers) {
  // See https://github.com/netlify/micro-api-client/blob/master/src/pagination.js for inspiration
}

async function getFullPaginatedList(endpoint) {
	return function makeRequestForPage(page = 1, accumulatedResults = []) {
		let fullResults = accumulatedResults;
		const paginatedEndpoint = `${endpoint}?page=${page}`;
		const res = await fetch(paginatedEndpoint, {
			headers: {
				'Authorization': `Bearer ${ACCESS_TOKEN}`,
			}
		});
		if (res.ok) {
			const currentCallResults = await res.json();
			fullResults = fullResults.concat(currentCallResults);
			if (hasNextPage(res.headers)) {
				return makeRequestForPage(++page, fullResults);
			}
		}
		return fullResults;
	}
}

const getAllSites = getFullPaginatedList(`https://api.netlify.com/api/v1/${ACCOUNT_SLUG}/sites`);
const allSites = await getAllSites();

@jverce
Copy link

jverce commented Oct 4, 2020

Yeah, I can relate with that @grahaml, I had to do the same thing in my project since I need to support such cases.

I created a PR for the JS client to provide such functionality: #167
Given that datasets can grow indefinitely (e.g. listSiteDeploys will grow monotonically with every commit to a main branch), and also to maintain backwards compatibility, I made this functionality optional.

Next thing we can do is add a parameter that indicates how many records/items the user would want to retrieve, and play with that instead of the current all-or-100 approach.

@JGAntunes
Copy link
Contributor

Hey folks 👋 we've just released v6.1.0:

This should cover pagination usage for the following endpoints:

listSites
listSiteSubmissions
listSiteDeploys
listSiteBuilds
listFormSubmissions
listAccountAuditEvents
listSitesForAccount

Check the relevant PR - netlify/open-api#278 - for more info

Right now it is possible to use the pagination params present in - https://open-api.netlify.com/ - like:

const NetlifyAPI = require('netlify')

const client = new NetlifyAPI('SOME-TOKEN')

const listSites = async function () {
  // Fetch sites
  const sites = await client.listSites({
    page: 2,
    per_page: 1,
  })

  console.log(sites)
}

listSites().then(() => console.log('done'))

We've created #285 to track the current issues of the present pagination solution, but if you feel like something isn't covered in that issue feel free to open a new one 👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: api type: bug code to address defects in shipped code
Projects
None yet
Development

No branches or pull requests

6 participants