A limit of zero returns the wrong results

[robhafner]

GORM Playground Link

go-gorm/playground#522

Description

Specifying a limit of 0 results in all rows being returned instead of none due to https://github.com/go-gorm/gorm/blob/master/clause/limit.go#L18.

For example:
gdb.Table(

).Limit(0).Find(&testDataRecords)

A pull request to fix this issue will be coming shortly.

[robhafner]

Here is the link to the pull request:

#5736

[github-actions]

The issue has been automatically marked as stale as it missing playground pull request link, which is important to help others understand your issue effectively and make sure the issue hasn't been fixed on latest master, checkout https://github.com/go-gorm/playground for details. it will be closed in 30 days if no further activity occurs. if you are asking question, please use the Question template, most likely your question already answered https://github.com/go-gorm/gorm/issues or described in the document https://gorm.io ✨ Search Before Asking ✨

[robhafner]

Adding playground pull request which illustrates the issue.

go-gorm/playground#522

[github-actions]

The issue has been automatically marked as stale as it missing playground pull request link, which is important to help others understand your issue effectively and make sure the issue hasn't been fixed on latest master, checkout https://github.com/go-gorm/playground for details. it will be closed in 30 days if no further activity occurs. if you are asking question, please use the Question template, most likely your question already answered https://github.com/go-gorm/gorm/issues or described in the document https://gorm.io ✨ Search Before Asking ✨

[a631807682]

What is the purpose of this usage? If the user doesn't need the data, it shouldn't execute the sql.

[robhafner]

Our Rest API supports returning additional pieces of information such as the number of resources in the collection. Clients retrieve this information by specifying a limit of zero when making a REST call to the service. We accomplish this by executing the query with a limit of zero followed by another query which retrieves the count.

For us to fix this without a change in gorm's behavior would require touching almost every service we deliver. Which is in the hundreds. Given this I'm hoping you might consider supporting the original behavior given it is valid to send a query to the database with a limit of zero. Also, as far as I can tell the change in behavior was not mentioned in the gorm v2 release notes and so it is unclear if the behavior change was intended or was just an oversight.

[robhafner]

Thanks for merging these PRs in! Can you share when the next release of these projects will be released so I can plan accordingly?

[robhafner]

I've upgraded our gorm dependencies to the following versions expecting this issue to be resolved now.

gorm.io/datatypes v1.0.7
gorm.io/driver/postgres v1.4.4
gorm.io/driver/sqlite v1.4.2
gorm.io/gorm v1.24.0

However, our unit tests which run against sqlite are still showing that the limit clause is not being added to the generated sql statement for a value of zero. Stepping through the debugger it appears the following line is missing the '=" sign in the condition and should look like this instead: "if lmt >= 0 || limit.Offset > 0 {"

https://github.com/go-gorm/sqlite/blob/1007940dc92e18c71bce8712a9931f050bef9c23/sqlite.go#L104

I believe the following change might of meant to include the ">" when the logic was refactored.
go-gorm/sqlite@ea59bcf

[robhafner]

The following PR has been created to fix the condition.

go-gorm/sqlite#118

[robhafner]

Thanks for merging this change @jinzhu! When will the next release of sqlite be tagged?

[roboslone]

@jinzhu please publish a release with this fix

[ghost]

This was a breaking API change and running go mod tidy automatically upgrades to the latest patch release. I 100% get the requirement for this fix, but our project has gorm as an indirect dependency and the dependency pulling in gorm is incompatible with this fix as they are constructing a clause.Limit object without going through the .Limit function.

I'm not sure the best way to address this while maintaining backwards compatibility, perhaps this should have been a major release? For now I'm going to have to exclude all future releases in my go.mod until the dependency updates their requirements which may not be for a while :/

[theStuRat]

This is absolutely not a change that should have been merged as part of a minor release. I understand @robhafner 's use case, but because it is breaking, making his use case work causes all use cases that depended on the existing zero value behavior to break. In my case, now I have to change all of the services that were assuming a zero value could be ignored now have to update to use *int

[a631807682]

I'm sorry that there is indeed a problem with our version release. We should plan the version number more reasonably.
https://go.dev/doc/modules/version-numbers

[frankli0324]

I believe a better/alternative fix for this issue is to keep the structure of struct Limit (Limit int) for API compatibility and change > 0 logic into >= 0, while documenting (in red alert) that the default value 0 would result in no records returned, if one wants to disable this clause without changing the code structure, use "-1" for Limit field. I believe 0 value checks should be performed at the API layer of an application.
by doing so gorm could block the unexpected case where LIMIT 0 returns all records, while NOT breaking a bunch of code built with clause.Limit
anyway this is already released...

[francisLee777]

好多个这种问题，字节血缘强制升级之后，线上业务直接爆炸了。 不兼容变更是怎么敢的？

[frankli0324]

好多个这种问题，字节血缘强制升级之后，线上业务直接爆炸了。 不兼容变更是怎么敢的？

血缘是让你谨慎升级不是让你强制升级。。。