Repository navigation
perf: JIRA-17789 stop serialising discarded relations in exports - #115
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughChangesEloquent serialization
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The change narrows export row serialization to declared attributes while preserving custom model serialization and no-column behavior; no actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
filterQuery() built each row with toArray(), which serialises the model's loaded relations as well as its attributes. A processor's query normally eager-loads a graph of relations for additional() to read from, so that recursively converted every related model into nested arrays once per row. Accessors on those related models that reach for relations the query did not load then triggered lazy loads, so serialising cost queries too. The next line filtered the result down to the processor's declared columns, so the whole relation half was discarded. A processor that declares no columns discarded all of it. Use attributesToArray() instead, and skip it entirely when no columns are declared. Only attribute names can appear in the column map, so dropping the relations half cannot change any exported value. Measured on a model with a dozen eager-loaded relation paths: 2.842 ms per row before, 0.093 ms after, plus the lazy loads removed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
5029d16 to
ec738d6
Compare
There was a problem hiding this comment.
Pull request overview
Optimizes Eloquent exports by avoiding serialization of discarded relationships and unnecessary attributes.
Changes:
- Uses
attributesToArray()instead oftoArray(). - Skips attribute serialization when no columns are declared.
- Adds relation-heavy fixtures and performance regression tests.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/Processor/EloquentProcessor.php |
Avoids unnecessary model serialization. |
tests/TestCase.php |
Adds team-related test schema. |
tests/Feature/Processor/EloquentProcessorTest.php |
Adds serialization query-count tests. |
tests/Fake/Models/User.php |
Adds the team relationship. |
tests/Fake/Models/Team.php |
Adds a relation-dependent accessor fixture. |
tests/Fake/FakeProcessorWithRelationDriver.php |
Exercises exports with eager-loaded relations. |
tests/Fake/FakeProcessorWithoutColumnsDriver.php |
Exercises exports without declared columns. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The no-columns test used a root model with no appended attributes, so an unconditional attributesToArray() produced the same queries and the same output. The empty-columns short-circuit could have been deleted with the test still green. Point that fixture at a model whose appended accessor reads a relation the query did not load, so serialising its attributes is observable as a query. The test now fails both without the short-circuit and without the wider change. Reported by Copilot on the pull request. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Reading attributesToArray() directly bypassed any model that declares its own toArray(). A consumer redacting or reformatting a declared column there would have silently exported the raw attribute instead. Detect the override once per model class and fall back to toArray() for those, keeping the cheap path for models that do not declare one. The no-columns short-circuit is unaffected: with no declared columns only the additional() keys survive, and those are merged over the serialised attributes, so an override could never reach the output either way. Reported by Copilot on the pull request. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
zetxek
left a comment
zetxek
left a comment
There was a problem hiding this comment.
Approving — the analysis is thorough and the reflection fallback for custom toArray() is a nice, cheap save. One thing I'd like an answer on before this goes live, though, because it touches a client-facing artifact.
TimesheetProcessor's CSV column order changes
The $columns = [] short-circuit does change output for one real processor — not values, but column order.
The old path was array_merge($item->toArray(), $additional). When an additional() key collides with a model attribute name, array_merge takes the value from $additional but keeps the key at its attribute position. Dropping attributesToArray() removes those positions, so the order collapses to additional()'s declaration order.
Reproduced end-to-end through the real CsvDriver, same fixture, both branches:
main: team_id,name,email,is_admin,computed_a,computed_b
branch: team_id,computed_a,email,computed_b,name,is_admin
platform's TimesheetProcessor is exactly this shape — $columns = [], and 8 of its additional() keys are real timesheet_registrations columns (checked against database/schema/mysql-schema.sql): timesheet_id, external_identifier, type, start_time, end_time, duration, unit, plus bill_id from the raw select alias. So type moves from near-first to last, external_identifier from first to fifth.
Header and values stay consistent with each other (CsvDriver::toSpreadsheet() derives the header from the first row's keys), so there's no corruption — but the column order clients receive is different. Anything importing that CSV by position rather than by header breaks.
To be clear about what I actually verified: I proved the mechanism with a fixture and confirmed the collisions against the real schema, but I didn't run the platform timesheet export itself, so I can't hand you the exact before/after header.
What I'd like before merge: confirmation that no consumer of the timesheet export parses it positionally. If one does, the cheapest fix is reordering TimesheetProcessor::additional() in platform to match the currently emitted order — no change needed here. The old order was an accident of array_merge and declaration order is strictly better, so I'd rather sign this off consciously than discover it from a client.
The other three processors are clean, and I checked each rather than assuming: BillProcessor ($columns = [], but its keys are human labels like Bill ID — zero collisions, and it gets the biggest win), JobProcessor (all real yjobs columns, loaded relations don't overlap), ContractProcessor (real yjob_hires columns plus the raw join aliases).
Minor: the "no working processor" argument has a hole
Not blocking, but worth correcting in the description since it's the load-bearing justification. A belongsTo that is loaded and null serialises to null, not an array, so strval() never raises anything:
main: ["User ID" => "1", "Team" => ""] <- exported fine
branch: ["User ID" => "1"] <- column silently dropped
So a processor declaring an always-null relation column did produce a working export. None of ours do, so there's no impact — but the reasoning as written would mislead whoever reads it next.
Follow-up, separate ticket
CsvDriver::toSpreadsheet() takes the header from array_keys($data->first()) but writes every row in its own key order, so any processor whose rows differ in key set or order silently misaligns values against the header. Pre-existing, not yours — this PR is just what made me look at it.
Also confirmed while reviewing: Model::toArray() is declared directly on Illuminate\Database\Eloquent\Model in the installed Laravel, so the reflection check holds; traits and intermediate base classes correctly resolve to "custom". None of the four real models declare toArray(), so the fast path applies everywhere and the fallback is insurance. And your rebuttal to Copilot on isset() was right.
webhanspeter
left a comment
webhanspeter
left a comment
There was a problem hiding this comment.
Thanks — both points confirmed, and I reproduced each rather than taking the mechanism on trust.
Column order. Right, and right about the cause. array_merge keeps a colliding key at its first (attribute) position while taking the later value, and Arr::only() is array_intersect_key, which preserves source order — so the order carries through to the header:
array_merge(attrs, additional): team_id,name,email,is_admin,computed_a,computed_b
array_merge([], additional): computed_a,name,computed_b,is_admin
I also closed the gap you flagged: I ran the affected export end-to-end on both branches with the real processor, so there is now an exact before/after header rather than an inferred one. Same column set, same values, values consistent with the header — order only, and every column moves. I have put the two header strings and the per-column movement on the ticket rather than here, since this repo is public and that output is client-facing.
On the resolution I would rather not ship the reorder as a side effect of a performance patch: reordering additional() in the consumer to the currently emitted order makes the exported file byte-identical, needs no change in this package, and rides along in the same composer.lock bump. That way the order question gets decided deliberately later instead of being answered by a client. Details on the ticket.
The null-relation hole. Also correct, and my description was overstated. A loaded belongsTo that is null serialises to null, not an array, so strval() raises nothing:
main: ["User ID" => "1", "Team" => ""]
branch: ["User ID" => "1"]
So an always-null relation column was a working export. Description corrected — it now says non-null relations throw and calls out the null case explicitly, instead of claiming no working processor could have one.
Header/row alignment follow-up. Agreed and out of scope here. I already have a ticket open on CsvDriver for a different reason — it builds an entire PhpSpreadsheet workbook to emit a plain CSV, measured at ~0.52 ms and ~20 KB per row, so a large export needs gigabytes. I will fold the header-from-first-row fragility into that one so both are fixed in the same pass rather than opening a third.
Also noted your confirmation on the reflection check and the isset() point.
— Via Claude
webhanspeter
left a comment
webhanspeter
left a comment
There was a problem hiding this comment.
Correction to my previous comment: I said I already had a ticket open on the generator. I did not — I had a local task note, and I described it as a ticket, which was wrong. It exists now as JIRA-17824, covering both the memory problem and the header-from-first-row misalignment you spotted, so the follow-up is genuinely tracked rather than just promised.
Apologies for sending you looking for something that was not there.
— Via Claude
Summary
EloquentProcessor::filterQuery()built every exported row by calling$item->toArray(), then discarded almost all of the result.Model::toArray()isattributesToArray()plusrelationsToArray(). A processor's query normally eager-loads a graph of relations so thatadditional()can read from it without extra queries — buttoArray()then recursively serialises that whole graph into nested arrays, once per row. Accessors on the related models make it worse: any accessor that reaches for a relation the query did not load triggers a lazy load, so serialising costs database queries as well as CPU.The next line throws it away:
Only keys named in
$columnssurvive, and nothing produced byrelationsToArray()can ever be among them. A processor that declares$columns = []— building every column itself inadditional()— discards 100% of it.Change
Use
attributesToArray()instead, and skip it altogether when the processor declares no columns.Why no exported value changes
Only attribute names can usefully appear in
$columns. A relation key serialises to an array, and thestrval()a few lines further down then raisesArray to string conversion— which Laravel escalates to a thrownErrorException, becauseHandleExceptionssetserror_reporting(-1)and itshandleError()throws on any non-deprecation error in that mask. Verified onmain: a processor declaring a loaded, non-null relation in$columnsthrows atEloquentProcessor.php:88rather than exporting anything.One exception, raised in review: a loaded
belongsTothat is null serialises tonull, not an array, sostrval()raises nothing and the column exported as an empty string. Verified —mainreturns["User ID" => "1", "Team" => ""], this branch returns["User ID" => "1"]. So a processor declaring an always-null relation column did have a working export and now loses that column, though the column only ever held an empty string.Everything that shapes an exported value — casts, date serialisation,
$appends,$hidden,withCountresults and rawselectaliases — lives inattributesToArray()and is untouched.This was checked against real processors rather than argued from the source: for every column each one declares, reading those keys from
attributesToArray()produced values identical to reading them fromtoArray().One exception, raised in review: a model that declares its own
toArray()— to redact or reformat a declared column, say — would have been bypassed, silently exporting the raw attribute. The processor now detects that (once per model class, via reflection) and falls back totoArray()for those models, so their behaviour is preserved. Models that do not declare one take the cheap path.The no-columns short-circuit is unaffected by that: with no declared columns, only the
additional()keys surviveonly(), and those are merged over the serialised attributes, so an override could never have reached the output either way.Measured
Against a model with a dozen eager-loaded relation paths (roughly 15 related models per row), several of which have accessors that lazy-load:
toArray()attributesToArray()Roughly 30x less work per row, and the lazy loads disappear entirely. On a ~300k row export that worked out to about 90,000 fewer queries and ~15 minutes less wall time.
Test plan
main(4 queries instead of 2, and 3 instead of 1) and pass with the change.isEmpty()short-circuit is removed, so both halves of the change are covered.toArray(); it fails without the fallback (exporting the raw attribute instead of the reshaped one).Rector is not run: it already fails on a clean
mainhere becauseworksome/coding-style4.x registers the deprecatedArraySpreadInsteadOfArrayMergeRector, which hard-errors on any file containingarray_merge(20 errors on an untouched checkout). CI gates PHPStan and ECS.Consumer note
A processor declaring a relation name in
$columnschanges behaviour: that column is now omitted from the output instead of throwing. As above, such a processor could not have produced a working export before, so there is no functioning behaviour to preserve — adding a fallback for it would only restore a hard failure. Worth noting the diagnostics get quieter, though: a mistyped relation name used to fail loudly and now fails silently. A validation pass over$columnswould be a reasonable follow-up, but it is out of scope here.Models that declare their own
toArray()keep the previous behaviour and the previous cost — the fast path only applies wheretoArray()is Eloquent's own.Column order can change
This is the one output change that affects a working processor.
array_merge($item->toArray(), $additional)takes the value from$additionalwhen a key collides with a model attribute, but leaves the key at its attribute position. Removing the attributes removes those positions, so the surviving order collapses toadditional()'s declaration order:Values stay correct and consistent with the header, since
CsvDriverderives the header from the first row's keys. But a processor that declares$columns = []and hasadditional()keys colliding with real attribute names will emit those columns in a different order than before. Anything reading such an export by column position rather than by header name needs checking before upgrading. Reorderingadditional()to the previously emitted order restores it exactly, with no change needed in this package.🤖 Generated with Claude Code