-
Notifications
You must be signed in to change notification settings - Fork 32
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
Feature: Implementing pagination strategy and data source driver interface #12
Feature: Implementing pagination strategy and data source driver interface #12
Conversation
f5ffb0e
to
184e2aa
Compare
59b8b30
to
faf6f08
Compare
184e2aa
to
7bf9ed3
Compare
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.
Other part LGTM
) { | ||
// TODO: implement query by dataset | ||
ctx.response.body = reqParams; | ||
ctx.response.body = { |
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.
Just want to confirm: Would we call templateEngine.execute(name, req, pagination)
here?
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.
Thanks for reviewing, yes, the original is called templateEngine.render
, right?
export class OffsetBasedStrategy extends PaginationStrategy<OffsetPagination> { | ||
public async transform(ctx: KoaRouterContext) { | ||
const checkFelidInHeader = ['limit', 'offset'].every((field) => | ||
Object.keys(ctx.request.header).includes(field) |
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.
Shall we move "paginate transforms" to "route-component" level? I guess you use header here in order because of using the same transformer in both restful and graphql routes, but indicating pagination in headers is strange.
We use in params or arguments more often:
/users?offset=100&limit=10
users(offset: 100, limit: 10) {
id
}
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.
Thanks for reviewing and finding the weird part, it should pass by query string would be correct.
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 has been fixed and move the "paginate transforms" to "route-component" level, please check it.
|
||
export class KeysetBasedStrategy extends PaginationStrategy<KeysetPagination> { | ||
public async transform(ctx: KoaRouterContext) { | ||
const checkFelidInHeader = ['limit', 'createAt'].every((field) => |
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.
- We won't always use the key "createAt" to paginate, I think we can let users definition their keys in schema file.
- I don't know the difference between keyset and cursor strategy. IMO, they both use a "key" to select limited data below or above the row and return a page. Can we merge these two strategies?
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.
Thanks for reviewing and suggesting.
After discussing with Ivan, we will keep the keyset strategy now for the second part, because not sure how the Driver section handles pagination.
For the first part - define the key name in the schema to define what key used in keyset pagination has been added in KeysetBaseStrategy
, please check it.
); | ||
if (!checkFelidInHeader) | ||
throw new Error( | ||
`The ${PaginationMode.KEYSET} must provide limit and offset in header.` |
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.
Wrong message here~
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.
Thanks for reviewing and finding wrong message, it has been fixed.
3d5a65e
to
1c16da8
Compare
|
||
export class CursorBasedStrategy extends PaginationStrategy<CursorPagination> { | ||
public async transform(ctx: KoaRouterContext) { | ||
const checkFelidInHeader = ['limit', 'cursor'].every((field) => |
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.
We're still checking the "header", it should be query too :D
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.
Yes, thanks founding the missing part, it has been fix~
@@ -0,0 +1,38 @@ | |||
import { KoaRouterContext } from '@route/route-component'; |
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 has been fixed and move the "paginate transforms" to "route-component" level, please check it.
Nice, but I think we should do some further action.
Shall we move "paginate transforms" to "route-component" level? I guess you use header here in order because of using the same transformer in both restful and graphql routes, but indicating pagination in headers is strange.
We use in params or arguments more often:
/users?offset=100&limit=10
users(offset: 100, limit: 10) { id }
I meant we might need to move the transform code into routers, because the pagination arguments come from different ways.
restfulRoute.ts
protected async handleRequest(
ctx: KoaRouterContext,
reqParams: RequestParameters,
) {
// handle user?offset=10&limit=10
const pagination = someFunctionToGetPaginationViaQueryParameters(ctx);
// TODO: implement query by dataset
ctx.response.body = {
reqParams,
pagination,
};
return;
}
graphQLRoute.ts
protected async handleRequest(
ctx: KoaRouterContext,
reqParams: RequestParameters,
) {
// handle user(offset: 10, limit; 10) { }
const pagination = someFunctionToGetPaginationViaRequestBody(ctx);
// TODO: implement query by dataset
ctx.response.body = {
reqParams,
pagination,
};
return;
}
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.
Hi, @oscar60310, thanks for catching the graphql and restful handle request content, not a consistent part!
Currently, I have refactored for separating the request and pagination transformer to a prepare
method, and made graphql* and restful route implemented prepare
method to handle request and pagination transformer
However, currently, the graphql route is not implemented the prepare
method because we have not yet developed graphql part ).
restfulRoute.ts
:
public async respond(ctx: KoaRouterContext) {
const transformed = await this.prepare(ctx);
await this.handle(transformed);
// TODO: get template engine handled result and return response by checking API schema
ctx.response.body = {
...transformed,
};
}
protected async prepare(ctx: KoaRouterContext) {
const reqParams = await this.reqTransformer.transform(ctx, this.apiSchema);
await this.reqValidator.validate(reqParams, this.apiSchema);
const pagination = await this.paginationTransformer.transform(
ctx,
this.apiSchema
);
return {
reqParams,
pagination,
};
}
grapqlRoute.ts
:
protected async prepare(ctx: KoaRouterContext) {
/**
* TODO: the graphql need to transform from body.
* Therefore, current request and pagination transformer not suitable (need to provide another graphql transform method or class)
*/
return {
reqParams: {},
};
}
public async respond(ctx: KoaRouterContext) {
const transformed = await this.prepare(ctx);
await this.handle(transformed);
// TODO: get template engine handled result and return response by checking API schema
return transformed;
}
b51e0c8
to
7dabe61
Compare
7bf9ed3
to
c36c855
Compare
…a query builder - add pagination field in "APISchema". - add "PaginationStrategy" interface and offset, cursor, keyset based strategy class. - add "PaginationTransformer" to do transfrom from koa ctx . - add data source inerface "IDataSource". - refactor "BaseRoute" for splting request and pagination transformer to implmented restful & graphql class, because of graphql get request from body. - update test cases of each sql clause method of "DataQueryBuilder".
7dabe61
to
f9cec9b
Compare
Description
This PR includes some components for two parts:
1. Support setting pagination parameter to provide mapping query parameter in API
Implementing pagination strategy abstract class and its sub-classes
Add
PaginationStragtegy
abstract class and supportOffsetBasedStragtegy
,CursorBasedStragtegy
,KeysetBasedStragtegy
sub-clasesses for transforming the koa ctx request to pagination format.2. Add data source driver interface
Add
IDataSource
interface for implementation in the future, the return format currently returns objects temporarily.How To Test / Expected Results
For the test result, please see the below test cases that passed the unit test:

Commit Message
APISchema
.PaginationStrategy
interface and offset, cursor, keyset based strategy class.PaginationTransformer
to do transfrom from koa ctx .IDataSource
.DataQueryBuilder
.