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

fix: QueryBuilder limit(0) bug #8280

Merged
merged 14 commits into from
Dec 16, 2023
Merged

fix: QueryBuilder limit(0) bug #8280

merged 14 commits into from
Dec 16, 2023

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented Nov 30, 2023

Description
Fixes #8278

  • LIMIT can be used in MySQL, SQLite3, PostgreSQL, and they return 0 rows when we specify LIMIT 0.
  • limit() is to limit the result rows. So when we specify it, the result should be limited, not be all rows.

LIMIT 0 quickly returns an empty set. This can be useful for checking the validity of a query.
https://dev.mysql.com/doc/refman/5.7/en/limit-optimization.html

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@kenjis kenjis marked this pull request as draft November 30, 2023 10:56
@lonnieezell
Copy link
Member

Um. Don't you think this has the potential to alter behavior in a major way for thousands of sites? As you said before this has always been this way in v4. And I am pretty sure I did not make any changes to the way this works when I ported the database layer over from 3.

While I understand the reporter has a valid use case I think this is probably a situation where we should say sorry, we can't that. This is way too big of a breaking change.

@michalsn
Copy link
Member

While there is a difference in handling limit in CI3 vs CI4, this would be a risky change.

In CI4 we simply ignore the limit when it's 0, while in CI3 we include it in the query. IMO that train has left the station. While it may be a problem for users migrating their apps from CI3, we should care more about CI4 users in this case... we are already in 4.4 branch.

@kenjis kenjis changed the base branch from develop to 4.5 November 30, 2023 21:59
@kenjis kenjis added bug Verified issues on the current code behavior or pull requests that will fix them breaking change Pull requests that may break existing functionalities 4.5 labels Nov 30, 2023
@kenjis
Copy link
Member Author

kenjis commented Dec 1, 2023

Thank you for the comments.

I have modified the current implementation and checked to see if the tests passes, but the impact seems to be larger than expected, so I will be more careful in handling this issue.

However, I think this bug should be fixed.

  • LIMIT can be used in MySQL, SQLite3, PostgreSQL, and they return 0 rows when we specify LIMIT 0.
  • limit() is to limit the result rows. So when we specify it, the result should be limited, not be all rows.

@kenjis kenjis force-pushed the fix-limit-zero branch 2 times, most recently from c035ef1 to a1ed340 Compare December 1, 2023 01:37
@kenjis kenjis added the docs needed Pull requests needing documentation write-ups and/or revisions. label Dec 2, 2023
@kenjis kenjis marked this pull request as ready for review December 2, 2023 02:13
app/Config/Feature.php Outdated Show resolved Hide resolved
app/Config/Feature.php Outdated Show resolved Hide resolved
@MGatner MGatner requested a review from a team December 2, 2023 18:32
@kenjis kenjis force-pushed the fix-limit-zero branch 2 times, most recently from 24b2897 to ce350fc Compare December 2, 2023 21:06
@kenjis kenjis added enhancement PRs that improve existing functionalities and removed breaking change Pull requests that may break existing functionalities docs needed Pull requests needing documentation write-ups and/or revisions. labels Dec 2, 2023
@kenjis
Copy link
Member Author

kenjis commented Dec 2, 2023

Added docs.

@kenjis kenjis force-pushed the fix-limit-zero branch 3 times, most recently from 2280d82 to 23c17cf Compare December 3, 2023 00:57
@kenjis kenjis added the database Issues or pull requests that affect the database layer label Dec 4, 2023
@kenjis
Copy link
Member Author

kenjis commented Dec 4, 2023

@codeigniter4/database-team Review, please.

@michalsn
Copy link
Member

michalsn commented Dec 9, 2023

Just started wondering... will this change have any effect on pagination?

@kenjis
Copy link
Member Author

kenjis commented Dec 9, 2023

I want to fix this behavior because I don't think it is a good idea to have QueryBuilder behave so differently from the behavior of the original SQL. LIMIT 0 returning zero rows has been the case for a long time.

@michalsn
Copy link
Member

michalsn commented Dec 9, 2023

I agree with your reasons.

Well, the Pager class has tests and since they pass, I should not worry.

@kenjis
Copy link
Member Author

kenjis commented Dec 9, 2023

will this change have any effect on pagination?

Pager has nothing to do with Query Builder.

The $perPage is more than zero in normal use cases.

public function paginate(?int $perPage = null, string $group = 'default', ?int $page = null, int $segment = 0)

@kenjis
Copy link
Member Author

kenjis commented Dec 12, 2023

There were other $limit parameters. 20ea393

@michalsn
Copy link
Member

Do we reach a consensus here? The feature flag will be removed in v5, so this is a transitional solution.

@kenjis
Copy link
Member Author

kenjis commented Dec 15, 2023

It is true that the code is more complicated with the addition of feature flag.
However, there are only if statements regarding the feature flag,
and when they are removed, they are just removed.
We can easily search where the if statements are with the flag.

I do not see these as major maintenance obstacles.
In addition, there are few use cases where 0 is specified for limit.

@kenjis kenjis merged commit 8edaf13 into codeigniter4:4.5 Dec 16, 2023
46 checks passed
@kenjis kenjis deleted the fix-limit-zero branch December 16, 2023 20:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.5 bug Verified issues on the current code behavior or pull requests that will fix them database Issues or pull requests that affect the database layer enhancement PRs that improve existing functionalities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: [QueryBuilder] if set limit to 0, it will return all data. Perhaps it should return 0 data.
4 participants