-
Notifications
You must be signed in to change notification settings - Fork 192
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
Conversation
Very broken right now
WIP on `Paginator` and tests
Implemented for the TS SDK first - includes unit tests
packages/sdk-rtl/src/paginator.ts
Outdated
this.transport.observer = (response: IRawResponse) => { | ||
raw = response | ||
return response | ||
} |
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.
this is the cleanest way I could come up with capturing the raw request values for processing the X-Total-Count
and Link
headers while still allowing the consumer to just make a call to an SDK function.
packages/sdk-rtl/src/paginator.ts
Outdated
get page(): number { | ||
if (this.limit < 1 || this.offset < 0) return -1 | ||
const x = this.offset / this.limit + 1 | ||
return Math.ceil(x) | ||
} | ||
|
||
get pages(): number { | ||
if (this.total < 1 || this.limit < 1) return -1 | ||
const x = this.total / this.limit | ||
return Math.ceil(x) | ||
} |
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.
readonly properties for reporting/UI purposes. A setPage(page, limit)
function would be feasible to implement once we've got any result back because we can plug in the calculated values for limit
and offset
into the first
rel link url. If total
is known, we can also reject the request immediately if the page is beyond the number of available results.
packages/sdk-rtl/src/paginator.ts
Outdated
items: TSuccess = [] as unknown as TSuccess | ||
links: PageLinks = {} | ||
total = -1 | ||
offset = -1 | ||
limit = -1 |
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.
I'll eventually make these read-only
} | ||
|
||
export type RawObserver = (raw: IRawResponse) => IRawResponse |
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.
new hook required for all transports
Typescript Tests 4 files 73 suites 3m 2s ⏱️ For more details on these failures, see this check. Results for commit 7fc8f3b. |
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.
I don't see any issues with this aside from the use of new in the comment.
Having said that, I haven't tested and havent looked at the helltool side.
Basically, no issues identified thus far.
Typescript Tests 4 files 73 suites 3m 32s ⏱️ For more details on these failures, see this check. Results for commit 9ae801c. |
Typescript Tests 4 files 73 suites 3m 27s ⏱️ For more details on these failures, see this check. Results for commit 56fe422. |
things were broken after pagination refactoring. Now they're fixed and improved.
Typescript Tests 7 files 76 suites 3m 52s ⏱️ Results for commit 24aa480. |
Typescript Tests0 files 0 suites 0s ⏱️ Results for commit a38b963. |
Typescript Tests 7 files 76 suites 3m 46s ⏱️ Results for commit b608855. |
Typescript Tests 7 files 76 suites 3m 26s ⏱️ Results for commit 8b16379. |
Typescript Tests 7 files 76 suites 3m 32s ⏱️ Results for commit ec60987. |
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.
I need more time with this but here are some random comments as I'm still grokking the code.
I need to spend more time understanding what you did with raw response parsing.
I also want to think more about the pager
and pageAll
interface - they just seem a little clunky because I need to pass in an sdk
as well as a no-arg callback that calls an sdk.something()
. I wonder if that can be simplified somehow?
I'll pick it back up first thing tomorrow
packages/sdk-rtl/src/paging.spec.ts
Outdated
const mockRawResponse = (url: string, body?: any): IRawResponse => { | ||
const result = { ...mockedRawResponse, ...{ url: url } } | ||
if (body) { | ||
result.body = body | ||
} | ||
return result | ||
} |
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.
does this function exist only to override url
and body
of mockedRawResponse
? If so my brain works better with either of these alternatives:
- s/
mockRawResponse
/customizeMockedRawResponse
- I know you're not a fan of big ugly names but reading this code top to bottom it's clearer why you have both a module scope variable and a similarly named closure function that uses the module variable - put
mockedRawResponse
inside themockRawResponse
local scope and make theurl
parameter optional and always use that method to get a mocked raw response
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.
I haven't made any changes to these unit tests. I was focusing on the functional tests instead once I had the PoC unit tests completed. There will certainly be another PR for SDK paging when the row-at-a-time iterator is added
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.
made any changes to these unit tests
Let me know if I have this wrong - I think I'm hearing/seeing that given this PR is at least 15 days old you haven't made any changes recently to your initial PoC unittests on this branch? My perspective is that I'm reading this PR (sadly 15 days later: totally my bad!) as a complete and atomic unit of work ready to merge regardless of the eventually-to-be-deleted history of this branch.
Given that I was just reading this file top to bottom and came across a variable named mockedRawResponse
and subsequently a closure named mockRawResponse
that referenced mockedRawResponse
and thought, "hm, why does it seem to be setup so that there are a couple different ways of getting a mock that looks like a response?" - reading the code below it seems like you use the latter when you need to override url/body and the former when you don't. I feel like a single way of getting a mocked response would be cleaner. Unless I've misread something and there's another reason (besides the url/body overrides).
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.
I wrote the unit tests on spec while making the functional tests possible in Looker. The functional tests are what really matter more than the mocked unit tests. However, I did write the mocking override function after the initial unit tests that used the constant mocked value. They are working, and I was planning on updating the mocking when the row-at-a-time iterator has been implemented, which should have unit tests as well as functional tests.
It's a small change, so maybe I should do the reduction now instead. But it won't change the unit test logic in any way, really.
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.
either way - I agree that it won't change the test logic, just giving my perspective as a reviewer that if I came in to read and modify this code later I'd trip for a second on why there's two. Up to you if you want to consolidate now or later or never :-)
const obj: PageLinks = {} | ||
let arrRes | ||
|
||
links.forEach((link) => { |
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.
I know there's a lot of this callback style iteration in sdk-codegen but I sure do prefer the for (const link of links) {...}
syntax: easier to reason about variable scope, and easier to step through with an interactive debugger imo. But since you're the primary TS contributor I'll defer to your preference :-)
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.
I think I mentioned somewhere that row-at-a-time iteration will be in a subsequent PR. The generic iterator interface is complicated and taking too long to figure out for the link processing prototype that fulfills the page-at-a-time need right now.
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.
my comment here wasn't about "row-at-a-time"/generic iterator - more just a mundane style comment about using Array.forEach vs (for const foo of Array) style syntax
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.
Oh, I understand. In our internal codebase I think we use .forEach()
more frequently unless it's an async block
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.
got it, that's fine. I don't like it but I'll take it :-)
const lastLink = `< ${lastUrl} >; rel="\tlast (end of line!)\t"` | ||
const prevLink = `<\t${prevUrl}\n>;\nrel="prev"` |
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.
what's with the \t
s and \n
s and the (end of line!)
?
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.
Checking pattern recognition variance tolerance.
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.
that's what I figured but wanted to check.
Would you consider any of the following?
- code comment here to that effect
- larger code comment here, or above the
describe('linkHeaderParser'
line, or maybe even above the actual implementation showing the "common" string value of theLinks
header and potentially variants you want to account for (I think you have it in the readme - maybe also including examples with explicit\n
orCRLF
or\t
chars)
this.transport = sdk.authSession.transport as BaseTransport | ||
} | ||
|
||
private async rawCatch(func: () => any) { |
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.
maybe?
private async rawCatch(func: () => any) { | |
private async rawCatch(func: PagingFunc<TSuccess, TError>) { |
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.
rawRequest is generic and that constraint would be too strict.
this.transport.observer = (response: IRawResponse) => { | ||
raw = response | ||
return response | ||
} |
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.
feels like this should be before the try
(doesn't seem like you'd expect any kind of exception just assigning a function to this.transport.observer
. moving it out focuses the attention on the next line which is where things might go badly
I wonder if this class should maybe just build its own transport with the observer it wants instead of going through the hassle of saving, substituting, and restoring one?
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.
The TypeScript pattern I've found (and used from before TypeScript) is to do resource assignments that will be closed/restored inside the try
section, which IMO helps clarify that finally
is intended to restore or close them. Happy to discuss further.
The only thing happening is the capture of the observer from whatever transport is used for resolving the initial SDK call (functional or monolithic). I think we need to capture the SDK function's transport and intercept the call rather than creating a different transport that may be completely missing some of the configuration of the SDK's transport object.
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.
to do resource assignments that will be closed/restored inside the
try
section
then I'd argue that perhaps const saved = this.transport.observer
belongs inside the try
?
resolving the initial SDK call
ah, right - there's this asymmetry between the implementation of the initial call and subsequent nextPage()
-and-friends calls. I feel like I want to think about that a little more.
possibly related is the asymmetry you see in my example code: I need to have an initial "get the items" block of code before I can launch into a while loop of retrieving the remaining items from subsequent pages if there are any. Maybe there's a smoother way to write that example? or maybe that will come with the "row-at-a-time"/generic iterator? just thinking out loud for our in-person chat on this
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.
The problem with having to capture the first set of items outside the loop is that we won't have any Link
header until we make the initiating request that causes paging. However, this means that we will also be sure to get the requested rows for any SDK method that does not include the paging headers. Having to init a stateful object for subsequent paging requests is a necessary evil.
Maybe there's a cleaner way to do it, but I haven't figured one out yet.
Reminds me we should make sure pageAll
works with a non-paging sdk request, too. I'll add a functional test for that right now.
/** | ||
* Create an API paginator | ||
* @param sdk functional AuthSession or full SDK implementation | ||
* @param func sdk function to call |
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.
I started to get lost - coming back to this comment header helped but I think I wouldn't have gotten lost if the param was named sdkFunc
or sdkMethod
on that note, why do we need both sdk
and func
? it seems like the func
is a first class object (not a string method name that you call on sdk
) and that sdk
only is used to access sdk.authSession.transport
and sdk.authSession.authenticate
. Perhaps the constructor should be changed to take a transport and an authenticate? or just an authSession?
how would a user of the functional sdk use this?
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.
sdk
needs to be a separate argument so we can capture the transport. We have no reliable way to capture the transport from the PagingFunc
call. (Which already works for either functional or monolithic SDK calls.)
async function dashboardSearchResultsByPage(inTitle: string) {
const sdk = new LookerSDK(session)
return await pager(sdk, () =>
search_dashboards(sdk, {title: inTitle, limit: 2})
)
}
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.
right, we need the transport
and the authenticate
... I want to chat in person about this. I don't have a good answer but I'd love for the interface to be simpler if possible so as a consumer I could just do:
OO sdk:
const pagedDashboards = await pager(sdk.search_dashboards(...))
Functional sdk:
const pagedDashboards = await pager(search_dashboards(sdk, ...))
I understand that there are technical limitations given the current implementation you have but I'm wondering if there's a way we can get there?
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.
What you show above is what I originally tried to do. I don't know a way to extract the values needed from an anonymous function call. Even if we figure out an introspection trick for Typescript, will other languages support something similar?
Typescript Tests 4 files 73 suites 1m 21s ⏱️ For more details on these failures, see this check. Results for commit d37062f. |
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.
A response to your response! Still haven't reviewed your changes to node/browserTransport
but getting a better idea of what I want to discuss in person (in the interface and asymmetry between initial sdk call and subsequent paging calls).
Also, I was getting tired last night and forgot to include this in my initial review: Great work John! 🥇 📟
const obj: PageLinks = {} | ||
let arrRes | ||
|
||
links.forEach((link) => { |
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.
my comment here wasn't about "row-at-a-time"/generic iterator - more just a mundane style comment about using Array.forEach vs (for const foo of Array) style syntax
packages/sdk-rtl/src/paging.spec.ts
Outdated
const mockRawResponse = (url: string, body?: any): IRawResponse => { | ||
const result = { ...mockedRawResponse, ...{ url: url } } | ||
if (body) { | ||
result.body = body | ||
} | ||
return result | ||
} |
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.
made any changes to these unit tests
Let me know if I have this wrong - I think I'm hearing/seeing that given this PR is at least 15 days old you haven't made any changes recently to your initial PoC unittests on this branch? My perspective is that I'm reading this PR (sadly 15 days later: totally my bad!) as a complete and atomic unit of work ready to merge regardless of the eventually-to-be-deleted history of this branch.
Given that I was just reading this file top to bottom and came across a variable named mockedRawResponse
and subsequently a closure named mockRawResponse
that referenced mockedRawResponse
and thought, "hm, why does it seem to be setup so that there are a couple different ways of getting a mock that looks like a response?" - reading the code below it seems like you use the latter when you need to override url/body and the former when you don't. I feel like a single way of getting a mocked response would be cleaner. Unless I've misread something and there's another reason (besides the url/body overrides).
const lastLink = `< ${lastUrl} >; rel="\tlast (end of line!)\t"` | ||
const prevLink = `<\t${prevUrl}\n>;\nrel="prev"` |
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.
that's what I figured but wanted to check.
Would you consider any of the following?
- code comment here to that effect
- larger code comment here, or above the
describe('linkHeaderParser'
line, or maybe even above the actual implementation showing the "common" string value of theLinks
header and potentially variants you want to account for (I think you have it in the readme - maybe also including examples with explicit\n
orCRLF
or\t
chars)
/** | ||
* Create an API paginator | ||
* @param sdk functional AuthSession or full SDK implementation | ||
* @param func sdk function to call |
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.
right, we need the transport
and the authenticate
... I want to chat in person about this. I don't have a good answer but I'd love for the interface to be simpler if possible so as a consumer I could just do:
OO sdk:
const pagedDashboards = await pager(sdk.search_dashboards(...))
Functional sdk:
const pagedDashboards = await pager(search_dashboards(sdk, ...))
I understand that there are technical limitations given the current implementation you have but I'm wondering if there's a way we can get there?
this.transport.observer = (response: IRawResponse) => { | ||
raw = response | ||
return response | ||
} |
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.
to do resource assignments that will be closed/restored inside the
try
section
then I'd argue that perhaps const saved = this.transport.observer
belongs inside the try
?
resolving the initial SDK call
ah, right - there's this asymmetry between the implementation of the initial call and subsequent nextPage()
-and-friends calls. I feel like I want to think about that a little more.
possibly related is the asymmetry you see in my example code: I need to have an initial "get the items" block of code before I can launch into a while loop of retrieving the remaining items from subsequent pages if there are any. Maybe there's a smoother way to write that example? or maybe that will come with the "row-at-a-time"/generic iterator? just thinking out loud for our in-person chat on this
Also added functional test to verify pageAll works with non-paging endpoints
Typescript Tests 4 files 73 suites 1m 29s ⏱️ For more details on these failures, see this check. Results for commit ca6618a. |
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.
some more questions for ya. I think I'm done now till we meet
) | ||
} | ||
|
||
const pagedDashboards = await dashboardSearchResultsByPage('JOEL') |
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.
you stinker
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 |
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.
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.
// Functional SDK search function | |
// Functional SDK search function | |
import {Looker40SDK, search_dashboards} from '@looker/sdk' |
packages/sdk-rtl/src/paging.spec.ts
Outdated
const mockRawResponse = (url: string, body?: any): IRawResponse => { | ||
const result = { ...mockedRawResponse, ...{ url: url } } | ||
if (body) { | ||
result.body = body | ||
} | ||
return result | ||
} |
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.
either way - I agree that it won't change the test logic, just giving my perspective as a reviewer that if I came in to read and modify this code later I'd trip for a second on why there's two. Up to you if you want to consolidate now or later or never :-)
return this.observer ? this.observer(rawResponse) : rawResponse | ||
} | ||
|
||
async parseResponse<TSuccess, TError>(res: IRawResponse) { |
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.
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 obj: PageLinks = {} | ||
let arrRes | ||
|
||
links.forEach((link) => { |
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.
got it, that's fine. I don't like it but I'll take it :-)
} | ||
return Promise.resolve(result) | ||
} | ||
|
||
async rawRequest( |
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.
why doesn't this class use the RawObserver
hook?
Typescript Tests 7 files 76 suites 3m 40s ⏱️ Results for commit 2a84405. |
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.
looks good. let's ship this and then I'll muddle around with the python version and see if I can convince you about tweaking the pager
signature.
nice work and thanks for your patience
This PR is for the first version of the SDK response paging prototype.
This supports the "page at a time" retrieval.
The "row at a time while paged behind the scenes" iterator will come later.
See
doc/paging.md
for the documentation.