Skip to content

[12.x] Fix double query in model relation serialization #55547

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

Merged
merged 3 commits into from
Apr 25, 2025

Conversation

AndrewMast
Copy link
Contributor

Why

When a model has an eager-loaded relation through $with, the unserialization process ends up loading the relation twice (first through $with and then again when loading the relations that were serialized). Issue #55546 highlights what this PR tries to fix.

I created a repo that reproduces this error: https://github.com/AndrewMast/laravel-testing/tree/issue-55546 (see diff).

What

The restoreModel() method in SerializesAndRestoresModelIdentifiers.php uses the load method to fetch all the serialized relations. However, this leads to the double query for relations already loaded (i.e. in $with). Changing load to loadMissing resolves this issue.

@taylorotwell
Copy link
Member

Tests failing.

@taylorotwell taylorotwell marked this pull request as draft April 24, 2025 21:33
@AndrewMast
Copy link
Contributor Author

I will investigate the tests. I also will add a test to prevent double loading relations on unserialization.

@AndrewMast AndrewMast force-pushed the patch/fix-double-relation-query branch from b8ee15d to 04fdcde Compare April 25, 2025 00:02
@AndrewMast
Copy link
Contributor Author

@taylorotwell I found the reason for this failure. When running the tests in the failing file (ModelSerializationTest.php) by itself, no error was raised. Only when running all of the tests together is there a failure. I found that this was due to the status of Model::preventsLazyLoading() changing depending on whether ModelSerializationTest.php was run by itself or not.

This could be due to tests/Integration/Http/ThrottleRequestsTest.php, tests/Integration/Http/ResourceTest.php, or tests/Integration/Database/EloquentStrictLoadingTest.php running Model::preventLazyLoading() or Model::shouldBeStrict().

I was able to fix the tests by adding Model::preventLazyLoading(false); to the setUp method of ModelSerializationTest.php.

I am currently adding a test for the issue this PR resolves, will mark as ready when done.

@AndrewMast AndrewMast marked this pull request as ready for review April 25, 2025 00:25
@AndrewMast
Copy link
Contributor Author

Added a test utilizing expectsDatabaseQueryCount to ensure only 4 queries are called during unserialization (one for the model, three for the relations).

@taylorotwell taylorotwell merged commit 39fa9f4 into laravel:12.x Apr 25, 2025
58 checks passed
@AndrewMast AndrewMast deleted the patch/fix-double-relation-query branch April 25, 2025 16:15
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