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

feat: paging results for the Typescript SDK #698

Merged
merged 27 commits into from
Jun 30, 2021
Merged
Show file tree
Hide file tree
Changes from 26 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
06de862
WIP TS SDK pagination implementation
jkaster May 20, 2021
f399f10
Merge branch 'main' into jk/pagination
jkaster Jun 4, 2021
062309a
`linkHeaderParser()` works
jkaster Jun 5, 2021
a51e9ca
feat: paged SDK results via Link header
jkaster Jun 14, 2021
d1da812
corrected comment example
jkaster Jun 14, 2021
7fc8f3b
improved paging function names, added comments
jkaster Jun 15, 2021
9ae801c
Added pagination.md and cleaned up some code
jkaster Jun 15, 2021
56fe422
doc tweak
jkaster Jun 15, 2021
24aa480
improved node transport error handling
jkaster Jun 16, 2021
e2abd40
Merge remote-tracking branch 'origin/main' into jk/pagination
jkaster Jun 17, 2021
129e3bb
Merge branch 'main' into jk/pagination
jkaster Jun 18, 2021
f20c7af
Merge branch 'jk/pagination' of github.com:looker-open-source/sdk-cod…
jkaster Jun 18, 2021
1b679fc
Improved pagination.md
jkaster Jun 18, 2021
8bfb0e2
Functional tests for SDK pagination work
jkaster Jun 19, 2021
7766a3b
Merge branch 'main' into jk/pagination
jkaster Jun 25, 2021
7ecf22a
Merge branch 'main' into jk/pagination
jkaster Jun 28, 2021
6ed5636
Updated pagination notes
jkaster Jun 28, 2021
37f430e
Merge remote-tracking branch 'origin/jk/pagination' into jk/pagination
jkaster Jun 28, 2021
a38b963
Updated pagination notes
jkaster Jun 28, 2021
b608855
Skip pagination tests until headers are available
jkaster Jun 28, 2021
8b16379
Merge branch 'main' into jk/pagination
jkaster Jun 28, 2021
f12337e
Change "pagination" to "paging"
jkaster Jun 29, 2021
ec60987
Merge remote-tracking branch 'origin/jk/pagination' into jk/pagination
jkaster Jun 29, 2021
5035439
Merge remote-tracking branch 'origin/main' into jk/pagination
jkaster Jun 30, 2021
d37062f
Address PR feedback
jkaster Jun 30, 2021
ca6618a
PR feedback for unit test clarifications
jkaster Jun 30, 2021
2a84405
Skip functional tests relying on unreleased features
jkaster Jun 30, 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
108 changes: 108 additions & 0 deletions docs/paging.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
# API response paging

Looker is adding [alpha-level](#alpha-support-level) support for API response paging in Looker API 4.0.

Any endpoint that accepts `limit` and `offset` parameters can support generic paging. Starting with Looker release 21.12, Looker is adding paging support for API 4.0 endpoints (until all endpoints that accept `limit` and `offset` provide the headers).

| Parameter | Description |
| --------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| `limit` | If provided, this value sets the number of results to return per _page_ and triggers paging headers to be provided. |
| `offset` | This value sets the starting position of the results to return. A value of `0` (zero) is used for the first result. `offset` defaults to 0 if `limit` is provided and `offset` is not. |

Some endpoints have `page` and `per_page` parameters instead of, or in addition to, `limit` and `offset`. The `limit` and `offset` parameters take precedence over the `page` and `per_page` parameters for endpoints that support both.
Only API calls specifying `limit` will produce paging headers for those endpoints that provide paging headers.

**NOTE**: The `page` and `per_page` parameters may be removed for API 4.0. Also, the Looker API does not support cursor-based paging.

## Paging headers

The [`X-Total-Count`](https://stackoverflow.com/a/43968710) and [`Link`](https://datatracker.ietf.org/doc/html/rfc5988) headers provide all the information required for an SDK to generically page API calls that return a collection of items.

### X-Total-Count header

If the `total count` of items can be known, the value of this header is that count. If `total count` is unknown, this header is not in the endpoint response.

Because many Looker endpoints restrict the user's ability to view individual items of a collection based on complex access constraints, sometimes calculating the total count degrades performance too much to calculate it.

### Link header

The Looker API adopts the [GitHub Link header values](https://docs.github.com/en/rest/overview/resources-in-the-rest-api#link-header).

Paging responses always include `Link` headers. Different **Link Relation Type** (`rel`) values may or may not exist in the Link header.

The table below explains Looker's use of the `rel` values adopted from GitHub.

| Rel | Description |
| ------- | ------------------------------------------------------------------------------------------------------------------------------------- |
| `first` | The URI to the first page of results. This link is always provided. |
| `next` | The URI to the next page of results. This link is provided when `total count` is known or the number of items returned == the `limit` |
| `prev` | The URI to the previous page of results. This link is provided when there **is** a previous page. |
| `last` | The URI to the last page of results. This link can only be provided when `total count` is known. |

Here is an example of a "full" Link header's content:

```
<http://localhost/api/4.0/alerts/search?imit=2&offset=0>; rel="first",
<http://localhost/api/4.0/alerts/search?limit=2&offset=8>; rel="last",
<http://localhost/api/4.0/alerts/search?limit=2&offset=7>; rel="next",
<http://localhost/api/4.0/alerts/search?limit=2&offset=3>; rel="prev"
```

## SDK Paging

Thanks to the adoption of the "standard" paging headers shown above, the SDKs can implement API result paging generically.

The current SDK-based paging pattern prototype is in the `@looker/sdk-rtl` TypeScript/Javascript package.

### Paging interface

The main routines that initialize SDK paging are the functions `pager` and `pageAll`, and the class `Paging` defined in [paging.ts](/packages/sdk-rtl/src/paging.ts).


### Page iteration example

Results can be retrieved a page at a time with code like this sample:

```ts
// "Monolithic" SDK search function
async function dashboardSearchResultsByPage(inTitle: string, limit: number = 100) {
const sdk = new Looker40SDK(session)
return await pager(sdk, () =>
sdk.search_dashboards({title: inTitle, limit})
)
}

const pagedDashboards = await dashboardSearchResultsByPage('JOEL')
Copy link
Contributor

Choose a reason for hiding this comment

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

you stinker

for (const dash of pagedDashboards.items) {
console.log(dash.title)
}
while (pagedDashboards.more()) {
for (const dash of await pagedDashboards.nextPage()) {
console.log(dash.title)
}
}
```

For the functional SDK, the syntax is almost identical (the imports will vary). The search function can be changed to:

```ts
// Functional SDK search function
Copy link
Contributor

Choose a reason for hiding this comment

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

not actually sure on the syntax here but something like this might make it more clear what you mean by "Functional SDK" w/out sending the reader elsewhere to do research.

Suggested change
// Functional SDK search function
// Functional SDK search function
import {Looker40SDK, search_dashboards} from '@looker/sdk'

async function dashboardSearchResultsByPage(inTitle: string, limit: number = 100) {
const sdk = new Looker40SDK(session)
return await pager(sdk, () =>
search_dashboards(sdk, {title: inTitle, limit})
)
}

```

**Note** The above examples will only work correctly when a Looker release with paging headers for the API 4.0 implementation of `search_dashboards` is available.

## Alpha support level

Support for paging headers is currently at alpha level. This means that:

- Not all endpoints with `limit` and `offset` parameters provide paging headers.
- Paging performance may vary for large results sets. We recommend making the `limit` size a larger value (half or a quarter of the total count, perhaps) to reduce paging if performance degradation is noticed as the `offset` grows larger.
- Currently, SDK support for paging is only available in the Typescript SDK prototype.
- While SDK paging routines **should** work for API endpoints that provide paging headers, reliability is not guaranteed, and SDK paging routines are only "community supported." This means that issues can be filed in this repository and Looker engineering will attempt to address them, but no timeframe or response is guaranteed.
11 changes: 11 additions & 0 deletions packages/hackathon/src/authToken/extensionProxyTransport.ts
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,16 @@ export class ExtensionProxyTransport extends BaseTransport {
return props
}

parseResponse<TSuccess, TError>(
_raw: IRawResponse
): Promise<SDKResponse<TSuccess, TError>> {
const result: SDKResponse<TSuccess, TError> = {
ok: false,
error: new Error('Should not be called!') as unknown as TError,
}
return Promise.resolve(result)
}

async rawRequest(
Copy link
Contributor

Choose a reason for hiding this comment

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

why doesn't this class use the RawObserver hook?

method: HttpMethod,
path: string,
Expand Down Expand Up @@ -131,6 +141,7 @@ export class ExtensionProxyTransport extends BaseTransport {
ok: true,
statusCode: res.status,
statusMessage: `${res.status} fetched`,
headers: res.headers,
}
}

Expand Down
4 changes: 1 addition & 3 deletions packages/sdk-node/src/nodeTransport.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,9 +57,7 @@ describe('NodeTransport', () => {
expect(response.ok).toEqual(false)
expect(response.statusCode).toEqual(404)
expect(response.body).toBeDefined()
expect(response.body.indexOf(errorMessage)).toEqual(0)
expect(response.body.length).toBeGreaterThan(0)
expect(response.statusMessage.indexOf('"type":"Buffer":')).toEqual(-1)
expect(response.statusMessage.indexOf('"type":"Buffer"')).toEqual(-1)
expect(response.statusMessage.indexOf(errorMessage)).toEqual(0)
})
})
Expand Down
156 changes: 91 additions & 65 deletions packages/sdk-node/src/nodeTransport.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,25 +31,39 @@ import rp from 'request-promise-native'
import { PassThrough, Readable } from 'readable-stream'
import { StatusCodeError } from 'request-promise-native/errors'
import {
Authenticator,
BaseTransport,
ResponseMode,
defaultTimeout,
responseMode,
trace,
LookerAppId,
agentPrefix,
safeBase64,
} from '@looker/sdk-rtl'
import type {
Authenticator,
HttpMethod,
ISDKError,
ITransportSettings,
responseMode,
ResponseMode,
SDKResponse,
trace,
Values,
IRequestHeaders,
LookerAppId,
IRawResponse,
agentPrefix,
safeBase64,
BaseTransport,
ICryptoHash,
} from '@looker/sdk-rtl'

const utf8 = 'utf8'

const asString = (value: any): string => {
if (value instanceof Buffer) {
return Buffer.from(value).toString(utf8)
}
if (value instanceof Object) {
return JSON.stringify(value)
}
return value.toString()
}

export class NodeCryptoHash implements ICryptoHash {
secureRandom(byteCount: number): string {
return nodeCrypto.randomBytes(byteCount).toString('hex')
Expand All @@ -62,38 +76,9 @@ export class NodeCryptoHash implements ICryptoHash {
}
}

export type RequestOptions = rq.RequiredUriUrl & rp.RequestPromiseOptions

async function parseResponse(res: IRawResponse) {
const mode = responseMode(res.contentType)
const utf8 = 'utf8'
let result = await res.body
if (mode === ResponseMode.string) {
if (res.contentType.match(/^application\/.*\bjson\b/g)) {
try {
if (result instanceof Buffer) {
result = (result as Buffer).toString(utf8)
}
if (result instanceof Object) {
return result
}
return JSON.parse(result.toString())
} catch (error) {
return Promise.reject(error)
}
}
if (result instanceof Buffer) {
result = (result as Buffer).toString(utf8)
}
return result.toString()
} else {
try {
return (result as Buffer).toString('binary')
} catch (error) {
return Promise.reject(error)
}
}
}
export type RequestOptions = rq.RequiredUriUrl &
rp.RequestPromiseOptions &
rq.OptionsWithUrl

export class NodeTransport extends BaseTransport {
constructor(protected readonly options: ITransportSettings) {
Expand All @@ -108,7 +93,7 @@ export class NodeTransport extends BaseTransport {
authenticator?: Authenticator,
options?: Partial<ITransportSettings>
): Promise<IRawResponse> {
const init = await this.initRequest(
const init: RequestOptions = await this.initRequest(
method,
path,
queryParams,
Expand All @@ -117,51 +102,97 @@ export class NodeTransport extends BaseTransport {
options
)
const req = rp(init).promise()
let rawResponse: IRawResponse
try {
const res = await req
const resTyped = res as rq.Response
return {
url: resTyped.url || '',
rawResponse = {
url: resTyped.url || init.url.toString() || '',
body: await resTyped.body,
contentType: String(resTyped.headers['content-type']),
ok: true,
statusCode: resTyped.statusCode,
statusMessage: resTyped.statusMessage,
headers: res.headers,
}
} catch (e) {
const statusMessage = `${method} ${path}`
let statusMessage = `${method} ${path}`
let statusCode = 404
let contentType = 'text'
let body: string
let body
if (e instanceof StatusCodeError) {
statusCode = e.statusCode
const text = e.message
body = e.message
// Need to re-parse the error message
const matches = /^\d+\s*-\s*({.*})/gim.exec(text)
if (matches && matches.length > 1) {
const json = matches[1]
const obj = JSON.parse(json)
if (obj.data) {
body = Buffer.from(obj.data).toString('utf8')
}
if (e.error instanceof Buffer) {
body = asString(e.error)
statusMessage += `: ${statusCode}`
} else if (e.error instanceof Object) {
// Capture error object as body
body = e.error
statusMessage += `: ${e.message}`
// Clarify the error message
body.message = statusMessage
contentType = 'application/json'
}
body = `${statusMessage} ${body}`
} else if (e.error instanceof Buffer) {
body = Buffer.from(e.error).toString('utf8')
body = asString(e.error)
} else {
body = JSON.stringify(e)
contentType = 'application/json'
}
return {
url: this.makeUrl(path, { ...this.options, ...options }, queryParams),
rawResponse = {
url: init.url.toString(),
body,
contentType,
ok: false,
statusCode,
statusMessage,
headers: {},
}
}
return this.observer ? this.observer(rawResponse) : rawResponse
}

async parseResponse<TSuccess, TError>(res: IRawResponse) {
Copy link
Contributor

Choose a reason for hiding this comment

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

it looks like you moved this function from above to below and changed it? It's hard to tell what changed - blurring my eye's I can tell it's a bit longer but can you give me an overview of how/why parseResponse changed?

const mode = responseMode(res.contentType)
let response: SDKResponse<TSuccess, TError>
let error
if (!res.ok) {
// Raw request had an error. Make sure it's a string before parsing the result
error = res.body
if (typeof error === 'string') error = JSON.parse(error)
response = { ok: false, error }
return response
}
let result = await res.body
if (mode === ResponseMode.string) {
if (res.contentType.match(/^application\/.*\bjson\b/g)) {
try {
if (result instanceof Buffer) {
result = Buffer.from(result).toString(utf8)
}
if (!(result instanceof Object)) {
result = JSON.parse(result.toString())
}
} catch (err) {
error = err
}
} else if (!error) {
// Convert to string otherwise
result = asString(result)
}
} else {
try {
result = Buffer.from(result).toString('binary')
} catch (err) {
error = err
}
}
if (!error) {
response = { ok: true, value: result }
} else {
response = { ok: false, error }
}
return response
}

async request<TSuccess, TError>(
Expand All @@ -181,12 +212,7 @@ export class NodeTransport extends BaseTransport {
authenticator,
options
)
const parsed = await parseResponse(res)
if (this.ok(res)) {
return { ok: true, value: parsed }
} else {
return { error: parsed, ok: false }
}
return await this.parseResponse<TSuccess, TError>(res)
} catch (e) {
const error: ISDKError = {
message:
Expand Down
Loading