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

[5.8] Fixed queue jobs using SerializesModels losing order of passed in collections #29136

Merged

Conversation

@itsgoingd
Copy link
Contributor

commented Jul 10, 2019

SerializesModels is a trait for queue jobs that converts passed in models and collections to keys on serialization and restores them back from database when unserialized.

The current implementation loses the intended order for passed in collections.

Eg. let's say we have a queue job exporting users sorted by email address.

User::create([ 'email' => 'taylor@laravel.com' ]);
User::create([ 'email' => 'mohamed@laravel.com' ]);

$users = User::orderBy('name')->get();
// SELECT * FROM users ORDER BY name
// [ [ 'id' => 2, 'email' => 'mohamed@laravel.com ], [ 'id' => 1, 'email' => 'taylor@laravel.com' ] ]

$serialized = serialize(new Export($users));

Now on unserialization, Laravel doesn't know about the order clause and runs a simple WHERE IN query, which returns the records in the original order as inserted into db.

$unserialized = unserialize($serialized);
// SELECT * FROM users WHERE id IN (2, 1)
// [ [ 'id' => 1, 'email' => 'taylor@laravel.com' ], [ 'id' => 2, 'email' => 'mohamed@laravel.com ] ]

The proposed fix adds an extra step where the restored collection is re-mapped based on the serialized collection of keys which is in the correct order, thus restoring the intended order.

Included an integration test demonstrating the issue.

@devcircus

This comment has been minimized.

Copy link
Contributor

commented Jul 10, 2019

Are there performance implications on large collections with this? I'd rather default to the current implementation and order, if necessary, in the job itself.

@devcircus

This comment has been minimized.

Copy link
Contributor

commented Jul 10, 2019

Serialization tests are failing.

@itsgoingd

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2019

Fixed the failing tests by skipping the order recovery step for pivot records.

Kinda share the performance concern, but with collections of tens or hundreds of thousands of items you'd run into memory limitations before cpu imo. The memory footprint shouldn't be too bad since both original and sorted collections are referencing the same objects. But I'm just guessing. 🙃

The current fix makes for some awkward code, maybe someone else has a better idea?

Alternatively we could document this as a known limitation as it's definitively an unexpected behaviour atm.

@driesvints

This comment has been minimized.

Copy link
Member

commented Jul 15, 2019

StyleCI is failing.

@itsgoingd

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2019

Sorry, thought these get merged automatically, fixed now.

@taylorotwell taylorotwell merged commit d6b93c9 into laravel:5.8 Jul 16, 2019

2 checks passed

continuous-integration/styleci/pr The analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.