Nested CursorConnection query paging limit error
Closed this issue · 7 comments
Describe the bug
When there is nested Cursor Connection paging of DTOs in a query, the SQL generated by the query will contain incorrect LIMIT n limits.
Have you read the Contributing Guidelines?
Yes.
To Reproduce
Steps to reproduce the behavior:
- Define the following resources.
@ObjectType('Test1')
@CursorConnection('test2', () => Test2DTO)
export class Test1DTO {
@IDField(() => String, { allowedComparisons: ['eq', 'in'] })
id: string;
@Field()
createdAt: Date;
@Field()
updatedAt: Date;
}@ObjectType('Test2')
export class Test2DTO {
@IDField(() => String, { allowedComparisons: ['eq', 'in'] })
id: string;
@Field()
createdAt: Date;
@Field()
updatedAt: Date;
}Using automatically generated Services and Resolvers
- Prepare data
Create three Test1 resources and associate a Test2 resource with each of them. i.e.
- test1-1
- test2-1
- test1-2
- test2-2
- test1-3
- test2-3
- Use the following query
query Test {
test1 {
edges {
node {
id
test2(paging: { first: 1 }) {
edges {
node {
id
}
}
}
}
}
}
}- View Results
{
"data": {
"test1": {
"edges": [
{
"node": {
"id": "test1-1",
"test2": {
"edges": [
{
"node": {
"id": "test2-1"
}
}
]
}
}
},
{
"node": {
"id": "test1-2",
"test2": {
"edges": [
{
"node": {
"id": "test2-2"
}
}
]
}
}
},
{
"node": {
"id": "test1-3",
"test2": {
"edges": []
}
}
}
]
}
}
}One of the three pieces of data in the output will not contain edges data.
- Debug
First look at the SQL printed by typeorm, which generates this SQL in order to query test2.
SELECT id FROM test2 WHERE "test1_id" IN ( 'test1-1', 'test1-2', 'test1-3' ) LIMIT 2I note that it contains LIMIT 2, so it's normal for test2 to output only two, so I'm concluding that the problem doesn't arise with the typeorm itself and the relational mapping in this project.
Checking with the debugger, I noticed the following code: https://github.com/TriPSs/nestjs-query/blob/master/packages/query-graphql/src/loader/query-relations.loader.ts#L31-L34
[...queryRelationsMap.values()].map(async (args) => {
const { query } = args[0]
const dtos = args.map((a) => a.dto)
const relationResults = await service.queryRelations(this.RelationDTO, this.relationName, dtos, query)
const dtoRelations = dtos.map((dto) => relationResults.get(dto) ?? [])
dtoRelations.forEach((relations, index) => {
results[args[index].index] = relations
})
})where the query is obtained from args[0] (that is, get the conditions used for all subqueries from the query object of the first subquery, limit 2 comes from there, completely disregarding how many pieces of data might be queried at the first level), it contains the requirements for paging:
{
"paging": {
"limit": 2
}
}The code behind only passes query to typesorm to generate SQL. According to this paging.limit requirement, it is correct that the generated SQL contains LIMIT 2, but this does not fit our general understanding.
For nested correlation queries, we need to consider the number of queries to the first level, for example, if the get 3 records, for its subqueries, then we should use 3 * limit to generate SQL, i.e. LIMIT 6 (3 * 2)
This way we can get the correct data.
- Validate of conclusion
Use yarn patch @ptc-org/nestjs-query-graphql to patch the release package. Make the following changes:
diff --git a/src/loader/query-relations.loader.js b/src/loader/query-relations.loader.js
index d7b86cb9d527539259dffadde7806b64f84de5f9..86f82c506d9b8cd6f6042026cf31014411fc4466 100644
--- a/src/loader/query-relations.loader.js
+++ b/src/loader/query-relations.loader.js
@@ -18,6 +18,9 @@ class QueryRelationsLoader {
await Promise.all([...queryRelationsMap.values()].map(async (args) => {
const { query } = args[0];
const dtos = args.map((a) => a.dto);
+ if (query && query.paging && query.paging.limit && dtos.length > 1) {
+ query.paging.limit = query.paging.limit * dtos.length;
+ }
const relationResults = await service.queryRelations(this.RelationDTO, this.relationName, dtos, query);
const dtoRelations = dtos.map((dto) => relationResults.get(dto) ?? []);
dtoRelations.forEach((relations, index) => {That is, if the first level contains more than one entry, multiply it by the limit requirement.
query.paging.limit = query.paging.limit * dtos.length;It has been tested and it works fine.
Expected behavior
For nested paging queries, generate the correct SQL query.
Screenshots
If applicable, add screenshots to help explain your problem.
Desktop (please complete the following information):
- Node Version [e.g. 14.14.0] 18.16.1
- Nestjs-query Version [e.g. v0.21.0] 4.0.0
Additional context
I also have a related issue where I'm using the query
{
"paging": {
"first": 10
}
}But it will include LIMIT 11 in the SQL instead of LIMIT 10, and I'm wondering what the purpose of this is.
For the example of the problem report above, it generates LIMIT 2 instead of LIMIT 1 (Although LIMIT 1 isn't right either, it should be LIMIT 3), which I think is odd. I hope to get your answer as well.
Thanks for reporting, I will still need to read it through completely but to answer atleast you last question:
We do LIMIT 11 instead of LIMIT 10 so we know if there is a next page or not.
Massive thanks for this detailed ticket! Would you be open on re-creating this case in a test (examples directory, copy examples/offset-paging and make one specific for cursor paging) with this in it? That way I can also immediately see the issue locally and we will have test to make sure it keeps working in the future :)
Is someone still working on this? I am running into this issue and am very motivated to get this resolved. How can I help?
Is someone still working on this? I am running into this issue and am very motivated to get this resolved. How can I help?
I guess the first step would be to do what TriPSs asked in his comment above: #168 (comment)
Closing this, if still an issue please add a test case and re-open.