{{ message }}
fix: Scope orWhere clauses in Item::exists() and Item::get_item_id() to fix items performance regression#4520
Merged
Merged
Conversation
In PR #4250 (commit 29c3c55), orWhere was added to match items by either item_id or item_number, but the OR condition was not wrapped in groupStart()/groupEnd(). This causes: 1. Wrong SQL semantics: generates WHERE item_id = ? OR item_number = ? AND deleted = 0 instead of WHERE (item_id = ? OR item_number = ?) AND deleted = 0 Due to AND binding tighter than OR, the deleted filter only applies to the item_number branch, allowing deleted items to match via item_id. 2. Performance: the unscoped OR causes MySQL to bypass the item_id primary key index and fall back to full table scans when item_number is a string column compared against a numeric parameter. Both exists() and get_item_id() are fixed by wrapping the OR conditions in groupStart()/groupEnd() for proper parenthesization.
Contributor
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/Models/Item.php (1)
393-402: Grouping fix is correct; consider also dropping the unusedsuppliersJOIN.The
groupStart()/groupEnd()wrap around the OR conditions is correct and matches the fix inexists().However, per the PR description,
get_item_id()also incurs "an unnecessary JOIN on suppliers when only checking item existence". The query selects nothing fromsuppliersand the WHERE clause doesn't reference it, so the left join at line 394 is pure overhead on what's meant to be a hot lookup path (e.g., called viaSales.php→sale_lib->get_item_id()). Removing it would complete the performance-regression fix hinted at in the PR description.♻️ Proposed follow-up
public function get_item_id(string $item_number, bool $ignore_deleted = false, bool $deleted = false): bool|int { $builder = $this->db->table('items'); - $builder->join('suppliers', 'suppliers.person_id = items.supplier_id', 'left'); $builder->groupStart(); $builder->where('item_number', $item_number); $builder->orWhere('item_id', $item_number); $builder->groupEnd();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/Item.php` around lines 393 - 402, The left JOIN on suppliers is unnecessary in get_item_id() and should be removed to avoid the extra overhead; locate the get_item_id method (the code creating $builder = $this->db->table('items')), delete or skip the $builder->join('suppliers', 'suppliers.person_id = items.supplier_id', 'left') call, confirm no columns from suppliers are referenced elsewhere in that method (the groupStart()/groupEnd() and where/orWhere logic should remain), and run tests or the hot lookup path (sale_lib->get_item_id) to verify behavior is unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/Models/Item.php`:
- Around line 393-402: The left JOIN on suppliers is unnecessary in
get_item_id() and should be removed to avoid the extra overhead; locate the
get_item_id method (the code creating $builder = $this->db->table('items')),
delete or skip the $builder->join('suppliers', 'suppliers.person_id =
items.supplier_id', 'left') call, confirm no columns from suppliers are
referenced elsewhere in that method (the groupStart()/groupEnd() and
where/orWhere logic should remain), and run tests or the hot lookup path
(sale_lib->get_item_id) to verify behavior is unchanged.
3 tasks
objecttothis
approved these changes
Apr 20, 2026
This was referenced Apr 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
orWhereclauses inItem::exists()andItem::get_id()introduced in PR Fix item number lookup in sales/receivings (#4212) #4250 (commit 29c3c55) that cause a performance regression and incorrect query semantics in the items vieworWhereconditions ingroupStart()/groupEnd()to generate correct SQL parenthesizationProblem
PR #4250 added
orWhere('item_number', $item_id)toexists()andorWhere('item_id', $item_number)toget_item_id()to support barcode lookup. However, theseORconditions were not scoped with parentheses, causing two issues:1. Wrong SQL Semantics
Before fix (broken):
Due to SQL operator precedence (
ANDbinds tighter thanOR), thedeleted = 0filter only applies to theitem_numberbranch. This means deleted items can match viaitem_id, returning extra rows and causinggetNumRows() === 1checks to fail for legitimate items.After fix (correct):
2. Performance Regression
The unscoped
ORcauses MySQL's optimizer to bypass theitem_idprimary key index and fall back to less efficient query plans, particularly when comparing a stringitem_numbercolumn against a numeric parameter. This results in full table scans or index scans on large tables.Additionally,
get_item_id()unnecessarily JOINs thesupplierstable even when only checking item existence, adding further overhead.Testing
groupStart()/groupEnd()produces correct parenthesized queriesexists()andget_item_id()are fixedRelated
Summary by CodeRabbit
Bug Fixes