CP-2322 Investigate pagination. #1
Conversation
| Paginator<EndpointAgent, ListEndpointAgentsResponse> paginator = new Paginator<>( | ||
| cursor -> { | ||
| try { | ||
| return apiInstance.getEndpointAgents(max, cursor, aid, List.of(), false, | ||
| false, null, "CSCO-W-PW02XZDB"); | ||
| } | ||
| catch (ApiException e) { | ||
| throw new RuntimeException(e); | ||
| } | ||
| }, | ||
| ListEndpointAgentsResponse::getAgents |
There was a problem hiding this comment.
ideally we would abstract away this logic. In the scope of the spike I didn't invest much time in figuring out how
There was a problem hiding this comment.
agree yeah, should be simple since we control 100% the SDK generation logic we can add a new method that already returns this Paginator instance at the api level.
There was a problem hiding this comment.
Agree it would be ideal to abstract it away, but, @brumarqu-te, I'm curious how exactly you imagine this working?
Because it needs to be at method/operation level, rather than API level, right? So are you thinking we will use some custom extension to define which operations are paginated, and what the primary field is?
Or maybe I'm totally missing something 🤔
There was a problem hiding this comment.
i imagined those being conditionally added as an additional operation during the post processing phase of the generator. haven't put much thought into what the condition would be but real quick and dirty: introspecting the input for cursor or output for _links.next ...
... maybe simple was not the best word 😅 but definitely "doable" 😄
| var getLinks = clazz.getMethod("getLinks", null); | ||
| var links = getLinks.invoke(result); | ||
|
|
||
| var getNext = links.getClass().getMethod("getNext", null); | ||
| var next = getNext.invoke(links, null); | ||
|
|
||
| if (next != null) { | ||
| var getHref = next.getClass().getMethod("getHref", null); |
There was a problem hiding this comment.
We need the 3 reflection gets because we have different Schemas for the PaginationLinks. AgentsApi uses their own PaginationNextAndSelfLink while AlertsApi uses a PaginationLinks. We could overcome this by slightly changing the OAS
| this.dataExtractor = dataExtractor; | ||
| } | ||
|
|
||
| public List<T> getAll() throws ApiException { |
There was a problem hiding this comment.
something for discussing i'm thinking that we might not want to make this operation available because it can generate a lot of requests 🤔
There was a problem hiding this comment.
Yeah, definitely something to consider
There was a problem hiding this comment.
IMO we better no have a get all option
| return allResults; | ||
| } | ||
|
|
||
| private String extractCursor(String href) { |
There was a problem hiding this comment.
i'm not sure if cursor is the only parameter needed for pagination, i think i recall some apis needing the time window as well (i'm not totally sure though). If this is true we'll need to take all parameters into consideration or just follow the link it might be safer.
There was a problem hiding this comment.
yeah, the time range parameters are needed for it to work properly
| ListEndpointAgentsResponse::getAgents | ||
| ); | ||
|
|
||
| StreamSupport.stream(paginator.spliterator(), false) |
There was a problem hiding this comment.
a simple example of a handy abstraction, AWS SDK "hides" this logic under a stream() method,
My idea is that we build 2 classes, one for fetching all results (makes all API calls in 1 call to the method) as well as an iterator (in here
PaginatorIterator) that the clients could use to fetch the results async - this can be useful for not exhausting rate-limits.