Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Default pagination.paginate function doesn't check for empty string link headers #1741

Closed
2 of 3 tasks
joshua-leyshon-canva opened this issue Jun 3, 2021 · 5 comments · Fixed by #1768
Closed
2 of 3 tasks
Labels
bug Something does not work as it should ✭ help wanted ✭

Comments

@joshua-leyshon-canva
Copy link
Contributor

joshua-leyshon-canva commented Jun 3, 2021

Describe the bug

  • Node.js version: v12.22.1
  • OS & version: MacOS Big Sur, Ubuntu 20

Sometimes (e.g. with Buildkite REST API), a response can contain a link header that is an empty string. The default paginate logic (see just below) doesn't handle this case and will throw when it tries to parse the string. This means we need to add our own paginate logic that is almost identical to the default.

Current default function:

if (typeof response.headers.link !== 'string') {

A fix:

if (typeof response.headers.link !== 'string' || response.headers.link === '') {
  return false;
}

Actual behavior

got throws when parsing an empty string link header.

Expected behavior

The default pagination.paginate function should return false when the link header is an empty string.

Code to reproduce

const res = got.paginate.all('https://somewebsite.com/that-returns-empty-link-header');
// An example endpoint (requires Buildkite authentication):
// "https://api.buildkite.com/v2/organizations/{org.slug}/pipelines/{pipeline.slug}/builds/{build.number}/jobs/{job.id}/artifacts"

Checklist

  • I have read the documentation.
  • I have tried my code with the latest version of Got.
  • I have tried my code with the latest version of Node.js.
@joshua-leyshon-canva
Copy link
Contributor Author

Note that I have separately raised the issue of returning an empty link header with Buildkite as well, but I figured we could also improve the default paginate function here 🙂

@sindresorhus
Copy link
Owner

What does the spec say about empty link? If it's a spec violation, we should at least emit a warning, even if we decide to gracefully handle it.

@joshua-leyshon-canva
Copy link
Contributor Author

Good question. This doc is the closest spec I found, which outlines what a link string should look like, but doesn't explicitly say anything about an empty link. Might be up to you how to handle this case 😄

@szmarczak
Copy link
Collaborator

Actually might be empty. The specification defines #link-value, which means from 0 to infinity occurrences.

https://datatracker.ietf.org/doc/html/rfc1945#section-2.1

@joshua-leyshon-canva
Copy link
Contributor Author

Hey @sindresorhus @szmarczak I opened #1768 to fix this issue 🦆

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bug Something does not work as it should ✭ help wanted ✭
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants