Bind attach relation keys as strings to use the attachment_id index#233
Bind attach relation keys as strings to use the attachment_id index#233austinderrick wants to merge 1 commit into
Conversation
The files table stores attachment_id as a string, but addConstraints() and addEagerConstraints() bound the parent key(s) using their native integer type. Comparing a string column against an integer forces the database to coerce the column value for every candidate row, which prevents the attachment_id index from being used - on MySQL/MariaDB an EXPLAIN shows the query falling back to the low-cardinality attachment_type index and examining every row of that morph type. Casting the keys to strings restores index usage for both lazy and eager attachment loads. getRelationExistenceQuery() already handles this for has() queries by casting the parent column to TEXT, so this brings the regular constraint paths in line with that approach.
WalkthroughThe PR modifies the Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/Database/Relations/Concerns/AttachOneOrMany.php (1)
45-50: 💤 Low valueConsider simplifying the null-preserving ternary.
The ternary
is_null($parentKey) ? $parentKey : (string) $parentKeycorrectly preserves null values while casting non-null keys to strings. For clarity, consider:$this->query->where($this->foreignKey, '=', $parentKey !== null ? (string) $parentKey : null);This makes the intent slightly more explicit: "if not null, cast to string; otherwise, use null."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Database/Relations/Concerns/AttachOneOrMany.php` around lines 45 - 50, Replace the null-preserving ternary used when binding the parent key to the query so the intent is clearer: in the block that calls $this->getParentKey() and then $this->query->where($this->foreignKey, '=', ...), change the expression is_null($parentKey) ? $parentKey : (string) $parentKey to the clearer form $parentKey !== null ? (string) $parentKey : null so non-null keys are cast to string and nulls remain null; keep references to $parentKey, $this->foreignKey, $this->query->where and getParentKey() unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/Database/Relations/Concerns/AttachOneOrMany.php`:
- Around line 45-50: Replace the null-preserving ternary used when binding the
parent key to the query so the intent is clearer: in the block that calls
$this->getParentKey() and then $this->query->where($this->foreignKey, '=', ...),
change the expression is_null($parentKey) ? $parentKey : (string) $parentKey to
the clearer form $parentKey !== null ? (string) $parentKey : null so non-null
keys are cast to string and nulls remain null; keep references to $parentKey,
$this->foreignKey, $this->query->where and getParentKey() unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 68cf2ae7-c5c6-47a3-96b3-64a896b50610
📒 Files selected for processing (2)
src/Database/Relations/Concerns/AttachOneOrMany.phptests/Database/Relations/AttachOneTest.php
1 participant
Problem
The
filestable storesattachment_idas an indexed string column, butAttachOneOrMany::addConstraints()andaddEagerConstraints()bind the parent key(s) using their native integer type. Comparing a string column against an integer forces the database to coerce the column value for every candidate row, which makes theattachment_idindex unusable.On MySQL/MariaDB this is easy to demonstrate with
EXPLAIN: with an integer binding the planner can only use the low-cardinalityattachment_typeindex, so every attachment query scans every file row of that morph type. With a string binding it uses both indexes (ref|filterwith rowid intersection) and examines only the matching rows. On installations with largesystem_filestables this is the difference between a handful of rows examined and tens of thousands — per attachment query, multiplied by however manyattachOne/attachManyrelations a page touches.The eager path is affected the same way: for integer parent keys Laravel selects
whereIntegerInRaw(), which inlines raw integers into the SQL.Fix
addConstraints()casts the parent key to a string (preservingnull, so the= null→whereNullconversion is unchanged).addEagerConstraints()builds thewhereInwith string-cast keys instead of deferring to the parent implementation. Null keys are preserved as-is so they match nothing, rather than being cast to''(which could match orphaned rows). The morph type and field constraints are applied exactly as before.getRelationExistenceQuery()already handles this same mismatch forhas()queries by casting the parent column to TEXT viaDbDongle::cast, so this brings the regular constraint paths in line with the approach already used there.Result matching is unaffected: PHP normalizes numeric-string array keys, so the eager-load dictionary lookup behaves identically (covered by a new end-to-end test).
Testing
Four new tests in
AttachOneTest: string bindings on lazy constraints, string bindings on eager constraints, null-key preservation in eager constraints, and an end-to-end eager load asserting attachments still match their parents. Full test suite passes.Summary by CodeRabbit
Bug Fixes
Tests