Framework: chunkById misbuilds query between where/orWhere

[iBotPeaches]

• Laravel Version: 5.8.32
• PHP Version: 7.3.8
• Database Driver & Version: MariaDB 10.2.23

Description / Repro Steps:

1. Build a query that leverages where/orWhere

        User::query()
            ->where(function (Builder $query) {
                // basic where
            })
            ->orWhere(function (Builder $query) {
                // basic where
            })
            ->chunkById(50, function (Collection $users) {
                // logic
            });

This generates

SELECT * FROM … WHERE a OR b AND id > X

Which when grouped becomes

SELECT * FROM … WHERE a OR (b AND id > X)

While I think it should be

SELECT * FROM … WHERE (a OR b) AND id > X

This led to a situation where our queue system ran out of bounds and just queued millions of millions of jobs until redis died out.

Workaround

        User::query()
            ->where(function (Builder $query) {
                $query->where(function (Builder $query) {
                    // basic where
                })
                ->orWhere(function (Builder $query) {
                    // basic where
                });
            })
            ->chunkById(50, function (Collection $users) {
                    // logic
            });

We basically just shove the logic into a where closure to force the grouping. I think this is a bug or at least unexpected behavior.

[staudenmeir]

I agree that Laravel should support this kind of query.

[driesvints]

Closing this since the proposed PR was rejected. This will probably be a no-fix.

[halaei]

@driesvints I think it may be fixed. The mentioned PR was changing code in some other functions forPageBeforeId and forPageAfterId. But maybe a PR that fixes inside chunkById would be fine, as already suggested by @rgehan ?

[driesvints]

@halaei you can always attempt a new pr. But I'm keeping the issue as a no-fix because Taylor rejected the first attempt and gave his reasons why.

[TimWolla]

@driesvints I consider that a straw man. Does that mean that if I start submitting half baked PRs, claiming to fix real issues, that are being rejected that all those issues will be marked “Wont Fix, because the first attempt was rejected”? That does not seem to be right.

The argumentation given by Taylor in #29721 might be valid for forPage(Before|After)Id, because other query builder methods can be chained. But you cannot chain anything after chunkById, because it calls get internally:

https://github.com/laravel/framework/blob/438b78d20e888eaa8217144a330aca7850f51912/src/Illuminate/Database/Concerns/BuildsQueries.php#L92

The fact that it uses forPage(Before|After)Id is an implementation detail that the API user should not need to know. I can't comment on whether #29721 is technically sound, but if changing forPage(Before|After)Id is the easiest way to fix this issue then it is a net-positive, even if forPage(Before|After)Id still misbuilds queries when not using chunkById.

I propose re-opening this issue. It's definitely non-intuitive and potentionally leads to infinite loops if you don't make sure to examine the generated queries. I am specifically using a query builder, because I don't want to do raw SQL.

[halaei]

About the infinite loop, it wasted a couple of hours of mine 🥴