Conversation
…es and simplify deleteQueueEntries - Refactor generateSelectQuery to accept associative columnAliases array (alias => column) instead of positional selects, producing named SQL aliases - Refactor getValuesFromSqlResult to extract values by column alias name instead of fragile positional index access - Update all EnqueueService callers and type adapters to use new associative alias format - Simplify deleteQueueEntries to use WHERE id IN (...) leveraging the primary key - Remove unused quoteParameters method - Add unit tests for generateSelectQuery and enqueueBySelectQuery
The simplified WHERE id IN (...) introduced a race condition: if an element is re-queued (via ON DUPLICATE KEY UPDATE) while being processed, the operationTime on the existing row changes but the id stays the same. The id-only DELETE would accidentally remove the re-queued entry, losing the pending update. Restore the original WHERE (id, operationTime) IN (...) tuple matching. This acts as an optimistic-concurrency guard: if operationTime changed since the entry was fetched, the DELETE is a no-op and the re-queued entry survives for the next processing cycle. Add unit tests documenting this race condition protection.
There was a problem hiding this comment.
Pull request overview
Refactors IndexQueueRepository and related enqueue/query-building code to use named SQL column aliases (instead of positional extraction) when selecting items to enqueue into the index queue, and updates tests accordingly.
Changes:
- Refactored
generateSelectQuery()to accept an associative alias-to-expression map and updated call sites to produce named aliases. - Refactored result extraction in
enqueueBySelectQuery()to read values by alias names instead of positional indexes. - Added new unit tests for
generateSelectQuery(),enqueueBySelectQuery(), and documenteddeleteQueueEntries()behavior with operationTime guarding.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/Repository/IndexQueueRepository.php |
Implements alias-based select generation and alias-based result extraction; refactors delete query construction and removes unused quoting helper. |
src/Service/SearchIndex/IndexQueue/EnqueueService.php |
Updates enqueue queries to provide the new alias map to generateSelectQuery(). |
src/Service/SearchIndex/IndexService/ElementTypeAdapter/AssetTypeAdapter.php |
Adds explicit AS <alias> names to the select list for related-item enqueue queries. |
src/Service/SearchIndex/IndexService/ElementTypeAdapter/DocumentTypeAdapter.php |
Adds explicit AS <alias> names to the select list for related-item enqueue queries. |
src/Service/SearchIndex/IndexService/ElementTypeAdapter/DataObjectTypeAdapter.php |
Adds explicit AS <alias> names to the select list for related-item enqueue queries and derived select parameters. |
tests/Functional/SearchIndex/IndexQueueTest.php |
Updates functional test to call generateSelectQuery() with alias map. |
tests/Unit/Repository/IndexQueueRepositoryTest.php |
Adds unit tests covering named aliases, where parameters, empty results, and delete guarding behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Add InvalidArgumentException when generateSelectQuery() receives numeric keys in $columnAliases, catching misuse from callers migrating from the old positional array API. Update upgrade notes: mark generateSelectQuery() change as BC break, remove private getValuesFromSqlResult() entry (not relevant for upgrades).
|
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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
generateSelectQueryto accept an associativecolumnAliasesarray (alias => expression) instead of positional$selects, producing named SQL aliases (e.g.id AS elementId) for robust, position-independent result extractiongetValuesFromSqlResultto extract values by column alias name instead of fragile positional index accessEnqueueServicecallers and type adapters (Asset,Document,DataObject) to use the new associative alias formatdeleteQueueEntries: improved code structure and quoting while preserving the(id, operationTime)tuple matching that guards against race conditionsquoteParametersmethodgenerateSelectQuery,enqueueBySelectQuery, anddeleteQueueEntriesMotivation
The previous implementation used positional array indices (
$result[0],$result[1], etc.) to extract values from SQL query results. This was fragile — any change to column order in the SELECT statement would silently break the mapping. Named column aliases make the code self-documenting and resilient to column reordering.deleteQueueEntries — operationTime guard
The
deleteQueueEntriesmethod matches onWHERE (id, operationTime) IN (...)rather than justWHERE id IN (...). This is intentional — it acts as an optimistic-concurrency guard:(elementId, elementType), so only one queue entry per element can existON DUPLICATE KEY UPDATE, which updates the existing row in-place — sameid, but newoperationTimedeleteQueueEntriesmatched only onid, it would delete the re-queued entry, losing the pending update(id, operationTime), the DELETE becomes a no-op whenoperationTimehas changed, and the re-queued entry survives for the next processing cycleTwo dedicated unit tests document and verify this behavior.