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

Broken link in the doc #344

Closed
kasir-barati opened this issue Dec 26, 2024 · 6 comments · Fixed by #346
Closed

Broken link in the doc #344

kasir-barati opened this issue Dec 26, 2024 · 6 comments · Fixed by #346
Labels
documentation Improvements or additions to documentation

Comments

@kasir-barati
Copy link
Contributor

📚 Documentation

Here: https://tripss.github.io/nestjs-query/docs/concepts/queries#paging

The link to "issue" is broken.

When using filters on relations with typeorm in combination with paging, performance can be degraded on large result sets. For more info see this issue

Dunno where should it be pointing to. Any comment?

Have you read the Contributing Guidelines on issues?

Yup.

@kasir-barati kasir-barati added the documentation Improvements or additions to documentation label Dec 26, 2024
@kasir-barati
Copy link
Contributor Author

kasir-barati commented Dec 26, 2024

Update 28.12.2024

I've figured out how pagination works in nestjs-query:

  1. When we write our DTO we are also kinda telling nestjs-query what kind of pagination strategy it should use, by default it is gonna use some sort of offset pagination with the Connection Spec compliant API (ref).

    const keySet = opts.disableKeySetPagination ? undefined : getKeySet(DTOClass)
    if (keySet && keySet.length) {
    return new CursorPager<DTO>(
    new KeysetPagerStrategy(DTOClass, keySet, opts.enableFetchAllWithNegative),
    opts.enableFetchAllWithNegative
    )
    }
    return new CursorPager<DTO>(new LimitOffsetPagerStrategy(opts.enableFetchAllWithNegative), opts.enableFetchAllWithNegative)

    Here if we've annotated our DTO with @KeySet decorator it will pick the keyset cursor-based pagination implementation, otherwise offset-based pagination implementation.

  2. Then it uses the options passed to it when we called NestjsQueryGraphQLModule.forFeature and pass something to it along this line: resolvers: [{ DTOClass: TodoItemDTO, EntityClass: TodoItemEntity }] it will uses QueryArgsType function to create a QueryArgs with the specified options passed to resolvers (e.g. DTOClass).

    In the createEntityAutoResolver for example we pass the options to CRUDResolver mixin.

    class AutoResolver extends CRUDResolver(DTOClass, resolverOpts) {

    Then inside CRUDResolver we call other mixins that we have. E.g. Readable.

    const readable = Readable(DTOClass, extractReadResolverOpts(opts))

    Then inside that we are calling QueryArgsType:

    const { QueryArgs = QueryArgsType(DTOClass, { connectionName: `${baseName}Connection`, ...opts }) } = opts

  3. QueryArgsType will decide based on pagination strategy to create the appropriate pagination class:

    if (mergedOpts.pagingStrategy === PagingStrategies.OFFSET) {
    return createOffsetQueryArgs(DTOClass, mergedOpts)
    }
    if (mergedOpts.pagingStrategy === PagingStrategies.NONE) {
    return createNonePagingQueryArgsType(DTOClass, mergedOpts)
    }
    return createCursorQueryArgsType(DTOClass, mergedOpts)

    JFYI the default pagination is being specified here inside the getMergedQueryOpts:

    pagingStrategy: PagingStrategies.CURSOR,

  4. And inside the create*QueryArgs* we're getting or creating the ConnectionType:

    const C = getOrCreateCursorConnectionType(DTOClass, opts)

  5. Now we are inside getOrCreateCursorConnectionType. In that function we are utilizing reflectors and rely on it to have a single instance of a connection for the DTOClass:

    return reflector.memoize(TItemClass, connectionName, () => {

  6. And inside its memoized callback function we're calling createPager helper function:

    Now we reached the point where we started 😂, I wrote this first and foremost for my future self but I think everybody should be able to see how this lib is working. At least from the pagination side of things.

Original post

BTW I am trying to figure out something else but I realized that nestjs-query is sorting by ID ascending when you do not specify any sorting rule.

Should not this be part of the doc? E.g. here maybe?

I do not get it quite right, but it is happening between the time that client's query reaches this resolver:

async queryMany(
@HookArgs() query: QA,
@AuthorizerFilter({
operationGroup: OperationGroup.READ,
many: true
})
authorizeFilter?: Filter<DTO>,
@GraphQLResultInfo(DTOClass)
resolveInfo?: GraphQLResolveInfoResult<DTO, DTO>
): Promise<InstanceType<typeof ConnectionType>> {
return ConnectionType.createFromPromise(
(q) =>
this.service.query(q, {
withDeleted: opts?.many?.withDeleted,
resolveInfo: resolveInfo?.info
}),
mergeQuery(query, { filter: authorizeFilter, relations: resolveInfo?.relations }),
(filter) =>
this.service.count(filter, {
withDeleted: opts?.many?.withDeleted
})
)
}
}

Somehow ConnectionType.createFromPromise is adding sorting: [{ field: "id", direction: "ASC" }] since when I debug it I could see that q is:

{
  sorting: [
    {
      field: "id",
      direction: "ASC",
    },
  ],
  paging: {
    limit: 3,
  },
  relations: [
  ],
  filter: {
  },
}

When my query is

query {
  posts(first: 3) {
    id
  } 
}

So I guess it worth noting it somewhere.

@kasir-barati
Copy link
Contributor Author

kasir-barati commented Dec 26, 2024

Update 28.12.2024

Here in the docs it says that the default sorting is determined by your persistence service layer: https://tripss.github.io/nestjs-query/docs/graphql/dtos#default-sort

But still I was able to pin point where it is being hard coded. Not yet at least 🥲.

Original Comment

If you know where it is doing the magic of adding this default sorting option please lemme know.

I did not manage to find any trace of it. I tried to debug it but it was like a maze and I could not find it 😅.

@TriPSs
Copy link
Owner

TriPSs commented Dec 27, 2024

The link of the "issue" should be doug-martin/nestjs-query#954, think is became broken when I replaced all references to that repo to this one.

@kasir-barati
Copy link
Contributor Author

kasir-barati commented Dec 27, 2024

Update 28.12.2024

So after a closer inspection of the docs I realized that since cursor is opaque (we do not know what is inside it) the default implementation is not using "Keyset Cursors" but we can of course change it if we want to (ref).

Will try to make sense of nestjs-query's codebase to see how the default approach is implemented + how we can change it to keyset cursor-based pagination 🙂.

Original Comment

Aha, OK. I will then take care of it in another PR. BTW could please gimme some pointers about pagination in nestjs-query? it is kinda like a mystery to me since I thought that I am using cursor based pagination (meaning I was expecting more or less a query like this: SELECT * FROM tbl WHERE id > cursor.id ORDER BY id ASC) but what I saw in terminal was this:

SELECT
    "Alert"."id" AS "Alert_id",
    "Alert"."title" AS "Alert_title",
    "Alert"."description" AS "Alert_description",
    "Alert"."userId" AS "Alert_userId",
    "Alert"."alertTypeId" AS "Alert_alertTypeId",
    "Alert"."createdAt" AS "Alert_createdAt",
    "Alert"."updatedAt" AS "Alert_updatedAt"
FROM "alert" "Alert"
LIMIT 7
OFFSET 6 # From where this 6 comes? I speculate that nestjs-query is either storing the keys and their offset in DB or just fetch it right when I send the query. But the problem is that I cannot see that in PostgreSQL logs. Or maybe it is part of the cursor???

When I send this query:

query {
  alerts(paging: {
  "paging": {
    "first": 6,
    "after": "YXJyYXljb25uZWN0aW9uOjU="
  }
}) {
    edges {
      cursor
      node {
        id
        title
        createdAt
      }
    }
    pageInfo {
      endCursor
      hasNextPage
      startCursor
      hasPreviousPage
    }
  }
}

Any comment would be much appreciated @TriPSs 🙏.

Also another comment, if I am understanding nestjs-query we're not using type-graphql. But I am confused why here it says:

The query-graphql package leverages most decorators from @nestjs/graphql and TypeGraphQL, with the exception of FilterableField.

@kasir-barati
Copy link
Contributor Author

kasir-barati commented Dec 28, 2024

The link of the "issue" should be doug-martin/nestjs-query#954, think is became broken when I replaced all references to that repo to this one.

Hmmm, it's interesting. Change log says that this is not a bug anymore: https://github.com/kasir-barati/nestjs-query/blob/master/CHANGELOG.md#bug-fixes-39

So I am gonna remove the note for it from the docs since it is not longer applicable. But please lemme know if I am getting something wrong :).

@kasir-barati
Copy link
Contributor Author

kasir-barati commented Dec 28, 2024

Another place that the docs seems a bit off is when it says:

By default if you do not provided an arguments you can query for all records.

But earlier it said:

By default all results will be limited to 10 records.

It just confused me and I tried it out in my local machine and it was returning only the first 10 records. So I suggest this wording: "By default if you do not provided an arguments it will uses the default value specified for the page size and for the sorting."

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants