feat(nestjs-query): Add `enableFetchAllWithNegative` option to allow to return all the items with `-1` #87

ValentinVignal · 2023-01-29T14:25:44Z

Fixes #63

Querying the todo items from the example

  todoItems(paging: {first:-1}) {
    # ...
  }

returns the entire list.

ValentinVignal · 2023-01-29T14:27:41Z

Hello @TriPSs I'm putting this PR in draft as it is not finished (I would have to handle the offset connection type and the backward pagination)

I would like to check with you about what you were thinking about this implementation. Would you like me to create another field instead of reusing first?

TriPSs · 2023-01-30T12:50:22Z

I think doing it with the -1 is a good one but I do think we need to do this behind a many.enableFetchAllWithNegative (or something like that) so this behaviour will not become enabled by default.

The main reason I think we need to put this behind a flag is that if someone uses this lib for a external API and the users of that API know that they can do a -1 to get everything that could then add un expected load on the API/database, what do you think?

ValentinVignal · 2023-01-31T01:10:06Z

Yes, that is a good idea, in that way it would also prevent "breaking changes" (or changes in behavior) since the flag won't be on. I'll try to implement that !

ValentinVignal · 2023-02-18T07:24:46Z

@TriPSs This PR should be good to be reviewed. In the end, I didn't nest the option in many (many.enableFetchAllWithNegative), I put it in the same place as the other options (like pagingStrategy)

TriPSs · 2023-02-18T13:14:50Z

Nice! Can you check the tests?

codecov-commenter · 2023-02-20T14:26:28Z

Codecov Report

Attention: Patch coverage is 96.03960% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 86.36%. Comparing base (336a598) to head (ff0e245).
Report is 1 commits behind head on master.

Files	Patch %	Lines
...graphql/src/types/connection/cursor/pager/pager.ts	66.66%	0 Missing and 3 partials ⚠️
...or/pager/strategies/limit-offset.pager-strategy.ts	87.50%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #87      +/-   ##
==========================================
+ Coverage   86.22%   86.36%   +0.13%     
==========================================
  Files         688      697       +9     
  Lines        9765     9849      +84     
  Branches      864      876      +12     
==========================================
+ Hits         8420     8506      +86     
+ Misses        624      619       -5     
- Partials      721      724       +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ValentinVignal · 2023-02-20T14:46:34Z

@TriPSs Test / e2e-test (16.x, mysql) (pull_request) are failing with the error

Error: Access denied for user 'fetch_all_with_negative'@'172.18.0.1' (using password: NO)

It looks like I'm missing a set up somewhere, would you have an idea?

TriPSs · 2023-02-20T14:52:47Z

@TriPSs Test / e2e-test (16.x, mysql) (pull_request) are failing with the error
Error: Access denied for user 'fetch_all_with_negative'@'172.18.0.1' (using password: NO)
It looks like I'm missing a set up somewhere, would you have an idea?

You should probably create a file like this

ValentinVignal · 2023-02-20T14:57:14Z

I actually already created examples/init-scripts/mysql/init-fetch-all-with-negative.sql in fix: Fix isValidPaging

That's why I'm a bit confused

TriPSs · 2023-02-20T15:01:56Z

Where do you get that error? If i check the actions i see the following one:

RDBMS does not support OFFSET without LIMIT in SELECT statements. You must use limit in conjunction with offset function (or take in conjunction with skip function if you are using pagination).

ValentinVignal · 2023-03-14T09:19:03Z

@TriPSs Sorry for the late reply, I got busy, it seems like MySQL doesn't support OFFSET without LIMIT:

What do you think we could do about that?

We don't do anything and the user cannot use offset with with limit: -1 if he uses MySQL, and the error will be thrown by MySQL
There is a way to detect the db type and we could:
1. Send a pretty error message when offset is specified, the limit: -1 and MySQL is used
2. Change -1 to a very big number (as mentioned in the links) when offset is specified, the limit: -1 and MySQL is used

Or any other idea?

ValentinVignal · 2023-05-03T13:31:00Z

@TriPSs Sorry for the late reply, I got busy, it seems like MySQL doesn't support OFFSET without LIMIT:

https://dev.mysql.com/doc/refman/8.0/en/select.html

https://stackoverflow.com/questions/36197249/offset-without-limit-in-mysql

https://stackoverflow.com/questions/255517/mysql-offset-infinite-rows

What do you think we could do about that?

We don't do anything and the user cannot use offset with with limit: -1 if he uses MySQL, and the error will be thrown by MySQL

There is a way to detect the db type and we could:

Send a pretty error message when offset is specified, the limit: -1 and MySQL is used

Change -1 to a very big number (as mentioned in the links) when offset is specified, the limit: -1 and MySQL is used

Or any other idea?

@TriPSs Would you have any advice on this one?

TriPSs · 2023-05-04T06:44:01Z

We could to number 4 if the feature is enabled? Only not quite sure what "big number" we should use.

…urn-all-the-items-even-when-paginated # Conflicts: # packages/query-graphql/src/types/connection/cursor/pager/strategies/keyset.pager-strategy.ts

nx-cloud · 2024-04-04T15:13:11Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit ff0e245. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution

✅ Successfully ran 5 targets

Sent with 💌 from NxCloud.

ValentinVignal · 2024-04-04T15:16:01Z

@TriPSs I set the limit to Number.MAX_SAFE_INTEGER test: Fix the typeorm tests with negative fetch and my sql when using MySQL.

I had to implement it in query-typeorm and could not do it in query-graphql (which I don't really like) because in order to use anything like DriverUtils.isMySQLFamily(repo.manager.connection.driver), I need to import typeorm.

Tell me if you have a better design in mind or if you prefer to fall back to 1.:

We don't do anything and the user cannot use offset with with limit: -1 if he uses MySQL, and the error will be thrown by MySQL

TriPSs · 2024-04-04T15:20:56Z

We don't do anything and the user cannot use offset with with limit: -1 if he uses MySQL, and the error will be thrown by MySQL

Let's indeed do this, if we eventually get tickets that ppl also need it in MySQL we can also revisit it.

TriPSs · 2024-04-04T15:24:58Z

...es/query-graphql/src/types/connection/cursor/pager/strategies/limit-offset.pager-strategy.ts

@@ -70,11 +78,12 @@ export class LimitOffsetPagerStrategy<DTO> implements PagerStrategy<DTO> {
      }
      return last
    }
-    return cursor.first || 0
+    return cursor.first ?? 0


Think it's good to keep || here, or was there a specific reason?

It's just that cursor.first is a ?: number so the only values possible are undefined, 0, and any number above 0. The ?? operator is made for this use case (handle null and undefined only) while 0 will also "ignore" 0. So I felt it was "more correct" to use ?? rather than ||.

I don't mind reverting and using || if you prefer to

This reverts commit 9bfe290. # Conflicts: # packages/query-typeorm/src/services/typeorm-query.service.ts

ValentinVignal · 2024-04-05T02:38:09Z

@TriPSs I reverted the changes made in query-typeorm and updated the tests to behave differently on PostgreSQL and MySQL instead

ValentinVignal · 2024-04-09T01:06:02Z

My longest contribution ever 😄

ValentinVignal added 2 commits February 18, 2023 15:21

feat(query-graphql): Adds enableFetchAllWithNegative option

6bfc92f

test(nestjs-graphql): Add tests on enableFetchAllWithNegative option

a954f81

ValentinVignal force-pushed the query-graphql/return-all-the-items-even-when-paginated branch from 2a685ed to a954f81 Compare February 18, 2023 07:22

ValentinVignal marked this pull request as ready for review February 18, 2023 07:22

ValentinVignal changed the title ~~feat: Allow to return all the items even when paginated~~ feat(nestjs-query): Add enableFetchAllWithNegative option to allow to return all the items with -1 Feb 18, 2023

ValentinVignal force-pushed the query-graphql/return-all-the-items-even-when-paginated branch from 4867804 to 62eb9c3 Compare February 20, 2023 13:47

fix: Fix isValidPaging

7f8913f

ValentinVignal force-pushed the query-graphql/return-all-the-items-even-when-paginated branch from 62eb9c3 to 7f8913f Compare February 20, 2023 14:18

ValentinVignal added 2 commits April 4, 2024 22:10

Merge remote-tracking branch 'upstream/master' into query-graphql/ret…

fc5cbb0

…urn-all-the-items-even-when-paginated # Conflicts: # packages/query-graphql/src/types/connection/cursor/pager/strategies/keyset.pager-strategy.ts

test: Fix the typeorm tests with negative fetch and my sql

9bfe290

chore: Fix linter

f02c81d

TriPSs reviewed Apr 4, 2024

View reviewed changes

ValentinVignal added 2 commits April 5, 2024 09:57

Revert "test: Fix the typeorm tests with negative fetch and my sql"

066732f

This reverts commit 9bfe290. # Conflicts: # packages/query-typeorm/src/services/typeorm-query.service.ts

test: Update the tests for postgresql and mysql

ff0e245

ValentinVignal requested a review from TriPSs April 8, 2024 16:43

TriPSs approved these changes Apr 8, 2024

View reviewed changes

TriPSs merged commit 3a00c53 into TriPSs:master Apr 8, 2024
16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(nestjs-query): Add `enableFetchAllWithNegative` option to allow to return all the items with `-1` #87

feat(nestjs-query): Add `enableFetchAllWithNegative` option to allow to return all the items with `-1` #87

ValentinVignal commented Jan 29, 2023 •

edited

Loading

ValentinVignal commented Jan 29, 2023

TriPSs commented Jan 30, 2023

ValentinVignal commented Jan 31, 2023

ValentinVignal commented Feb 18, 2023

TriPSs commented Feb 18, 2023

codecov-commenter commented Feb 20, 2023 •

edited

Loading

ValentinVignal commented Feb 20, 2023

TriPSs commented Feb 20, 2023

ValentinVignal commented Feb 20, 2023

TriPSs commented Feb 20, 2023

ValentinVignal commented Mar 14, 2023 •

edited

Loading

ValentinVignal commented May 3, 2023

TriPSs commented May 4, 2023

nx-cloud bot commented Apr 4, 2024 •

edited

Loading

ValentinVignal commented Apr 4, 2024 •

edited

Loading

TriPSs commented Apr 4, 2024

TriPSs Apr 4, 2024

ValentinVignal Apr 4, 2024

ValentinVignal commented Apr 5, 2024

ValentinVignal commented Apr 9, 2024

feat(nestjs-query): Add enableFetchAllWithNegative option to allow to return all the items with -1 #87

feat(nestjs-query): Add enableFetchAllWithNegative option to allow to return all the items with -1 #87

Conversation

ValentinVignal commented Jan 29, 2023 • edited Loading

ValentinVignal commented Jan 29, 2023

TriPSs commented Jan 30, 2023

ValentinVignal commented Jan 31, 2023

ValentinVignal commented Feb 18, 2023

TriPSs commented Feb 18, 2023

codecov-commenter commented Feb 20, 2023 • edited Loading

Codecov Report

ValentinVignal commented Feb 20, 2023

TriPSs commented Feb 20, 2023

ValentinVignal commented Feb 20, 2023

TriPSs commented Feb 20, 2023

ValentinVignal commented Mar 14, 2023 • edited Loading

ValentinVignal commented May 3, 2023

TriPSs commented May 4, 2023

nx-cloud bot commented Apr 4, 2024 • edited Loading

☁️ Nx Cloud Report

ValentinVignal commented Apr 4, 2024 • edited Loading

TriPSs commented Apr 4, 2024

TriPSs Apr 4, 2024

Choose a reason for hiding this comment

ValentinVignal Apr 4, 2024

Choose a reason for hiding this comment

ValentinVignal commented Apr 5, 2024

ValentinVignal commented Apr 9, 2024

feat(nestjs-query): Add `enableFetchAllWithNegative` option to allow to return all the items with `-1` #87

feat(nestjs-query): Add `enableFetchAllWithNegative` option to allow to return all the items with `-1` #87

ValentinVignal commented Jan 29, 2023 •

edited

Loading

codecov-commenter commented Feb 20, 2023 •

edited

Loading

ValentinVignal commented Mar 14, 2023 •

edited

Loading

nx-cloud bot commented Apr 4, 2024 •

edited

Loading

ValentinVignal commented Apr 4, 2024 •

edited

Loading