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.

Sign up for GitHub

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

Jump to bottom

fix: paging parsing issues #728

Merged
merged 7 commits into from
Jul 1, 2021
Merged

fix: paging parsing issues #728

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9a5a992
updates URL parse to works with relative path
annguy3n Jul 1, 2021
4379307
update total count header to be lower case
annguy3n Jul 1, 2021
0798b5c
empty commit to retest
annguy3n Jul 1, 2021
59f6843
Merge remote-tracking branch 'origin/main' into an/fix-parsing-error
jkaster Jul 1, 2021
fa7641b
fix: detect any casing of header
jkaster Jul 1, 2021
7b791b6
added a comment for `new URL()`` base path
jkaster Jul 1, 2021
4286ec7
Simplify `offset` and `limit` value extraction
jkaster Jul 1, 2021
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
13 changes: 13 additions & 0 deletions packages/sdk-rtl/src/paging.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,19 @@ describe('paging', () => {
expect(actual.options?.timeout).toEqual(99)
})

it('works on relative url', async () => {
const actual = await pager(sdk, () =>
mockRawResponseSuccess(
transport,
mockedRows,
mockRawResponse('/api/4.0/alerts/search?fields=id&limit=3&offset=0')
)
)
expect(actual).toBeDefined()
expect(actual.limit).toEqual(3)
expect(actual.offset).toEqual(0)
})

it('supports paging', async () => {
const paged = await pager(sdk, () =>
mockRawResponseSuccess(transport, mockedRows, mockRawResponse())
Expand Down
19 changes: 13 additions & 6 deletions packages/sdk-rtl/src/paging.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ import {
import { IAPIMethods } from './apiMethods'
import { BaseTransport } from './baseTransport'

export const LinkHeader = 'link'
export const LinkHeader = 'Link'
export const TotalCountHeader = 'X-Total-Count'
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like I see the capitalized form of http headers more often: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers

also, server/client should consider them case-insensitive: https://stackoverflow.com/a/5259004/13802304

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, it's a problem with the headers handling in JS. I responded via chat that I think we need to support both the Title case and lowercase at least.


/**
Expand Down Expand Up @@ -257,7 +257,7 @@ export class Paging<TSuccess extends ILength, TError>
this.transport.observer = saved
}
if (Object.keys(raw).length === 0 || Object.keys(raw.headers).length === 0)
throw new Error('No paging headers were found')
return Promise.reject(new Error('No paging headers were found'))
this.parse(raw)
return this
}
Expand Down Expand Up @@ -362,21 +362,28 @@ export class Paging<TSuccess extends ILength, TError>
return result
}

static findHeader(raw: IRawResponse, name: string) {
return (
raw.headers[name] ||
raw.headers[name.toLowerCase()] ||
raw.headers[name.toUpperCase()]
Comment on lines +367 to +369
Copy link
Contributor

Choose a reason for hiding this comment

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

Recognizes all three reasonable casing options for the header

)
}

parse(raw: IRawResponse): IPager<TSuccess, TError> {
const req = new URL(raw.url)
const params = req.searchParams
const params = new URLSearchParams(raw.url)
Copy link
Contributor

Choose a reason for hiding this comment

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

Handles full or relative urls

this.limit = Paging.paramDefault(params.get('limit'), -1)
this.offset = Paging.paramDefault(
params.get('offset'),
this.limit > 0 ? 0 : -1
)
const linkHeader = raw.headers.link || raw.headers.Link || raw.headers.LINK
const linkHeader = Paging.findHeader(raw, LinkHeader)
if (linkHeader) {
this.links = linkHeaderParser(linkHeader)
} else {
this.links = {}
}
const totalHeader = raw.headers[TotalCountHeader]
const totalHeader = Paging.findHeader(raw, TotalCountHeader)
if (totalHeader) {
this.total = parseInt(totalHeader.trim(), 10)
} else {
Expand Down