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

[11.x] Fix chunked queries not honoring user-defined limits and offsets #52075

Closed
wants to merge 14 commits into from

Conversation

tonysm
Copy link
Contributor

@tonysm tonysm commented Jul 9, 2024

Changelog

  • ADDED: Adds new getter methods for offset and limit to the query builder (getOffset() and getLimit())
  • FIXED: Honor user-defined limits and offsets in chunk and chunkById queries instead of silently overriding it
  • FIXED: Any user-defined offset would cause the chunkById to silently skip the offset on every iteration (since it doesn't override it)

Currently, the chunk method ignores any user-defined limits and offsets. The chunkById method ignores user-defined limits, but gets tripped up by user-defined offsets (it skips the offset amount of entries on every batch iteration).

This PR ensures that user-defined limits and offsets are honored by chunked methods. For instance:

$survey->emails()
  ->orderBy('id', 'asc')
  ->offset(2)
  ->limit(3)
  ->chunk(2, function (Collection $users, $page) {
    // ...
  });

Given there are 7 entries in the database, this query would result in the following chunks distribution:

Entry ID Status Chunk Page
1 skipped --
2 skipped --
3 included 1
4 included 1
5 included 2
6 skipped --
7 skipped --

Warning

Anyone currently using chunk or chunkById with limits and offsets (and not testing the behavior, I guess) could be affected by this fix.

Context

I noticed chunked queries were ignoring user-defined limits while I was working on a legacy application that had a job doing something like this:

$survey->emails()
  ->orderBy('id', 'desc')
  ->limit($plan->remainingSurveysFor($survey))
  ->each(function ($email) {
    dispatch(new SendSurvey($email));
  });

As I was covering this with a test like this:

/** @test */
public function only_sends_plan_limit()
{
  Queue::fake();

  $survey = Survey::factory()
    ->has(Email::factory()->times(10))
    ->create();

  (new BroadcastSurveys($survey))->handle((new FakePlan)->withRemainingSurveys($limit = 5));

  Queue::assertPushed(SendSurvey::class, $limit);
}

I noticed the test was failing saying that 10 jobs were pushed instead of the expected 5.

I decided to dig in and found out the each method calls chunk behind the scenes (here), which calls the forPage() helper, which overrides offset and limits for the query.

After some searching, it looks like this was a known issue:

A fix hasn't been attempted in a while, so I figured it was about time for another one.

I've tested this patch in the application where I discovered it, and it works.

As I was looking for references in other frameworks, I found out that Rails' ActiveRecord also honors limits on their batched queries:

Limits are honored, and if present there is no requirement for the batch size: it can be less than, equal to, or greater than the limit.

@tonysm tonysm marked this pull request as draft July 9, 2024 21:08
@tonysm tonysm marked this pull request as ready for review July 9, 2024 21:58
@tonysm
Copy link
Contributor Author

tonysm commented Jul 9, 2024

An alternative approach could be to throw an exception if the user has limits and offsets configured instead of honoring them (if this PR doesn't get accepted, I mean.) This would limit the chunks API, but I think it's better than silently overriding it as it is doing now.

@taylorotwell
Copy link
Member

This would likely need to be sent to master branch since it could potentially break existing applications relying on current behavior (broken or not).

@tonysm tonysm changed the base branch from 11.x to master July 11, 2024 13:21
@tonysm tonysm marked this pull request as draft July 11, 2024 13:21
@tonysm tonysm changed the base branch from master to 11.x July 11, 2024 13:21
@tonysm
Copy link
Contributor Author

tonysm commented Jul 11, 2024

Closing this, sent another PR to master here: #52093

@tonysm tonysm closed this Jul 11, 2024
@tonysm tonysm deleted the tm/fix-chunked-limits branch July 11, 2024 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants