Deduplicate indexing filters via a data-provider membership check#179
Open
loevgaard wants to merge 1 commit into
Open
Deduplicate indexing filters via a data-provider membership check#179loevgaard wants to merge 1 commit into
loevgaard wants to merge 1 commit into
Conversation
"What should be indexed" was expressed twice: query-builder (SQL) filters that shape a full reindex, and mirror object filters applied per entity on both the batch and incremental paths. Keeping the two in sync was manual, and a divergence could delete a document a reindex would keep. Route the incremental (single-entity) path through the same data provider the batch path uses. IndexableDataProviderInterface gains containsId(), implemented via a query builder shared with getIds() (same QueryBuilderForDataProvisionCreated event, constrained to one id), so the SQL filters are the single source of truth. IndexEntityHandler asks containsId() whether a changed entity still qualifies: index it if so, remove its document if not. The two true mirrors (EnabledEntityFilter, StockAvailableEntityFilter) are removed along with their object-filter registrations. ChannelsAwareEntityFilter and FilterableEntityFilter stay, since they carry per-scope / SQL-inexpressible logic; EntityFilterInterface remains the escape hatch for such rules. BC breaks (pre-1.0): new interface method on IndexableDataProviderInterface, and removal of the two filter classes. Closes #178
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #179 +/- ##
============================================
- Coverage 49.08% 49.03% -0.05%
+ Complexity 805 798 -7
============================================
Files 139 137 -2
Lines 2557 2541 -16
============================================
- Hits 1255 1246 -9
+ Misses 1302 1295 -7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
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.
Closes #178.
Problem
"What should be indexed" lived in two parallel mechanisms that had to be kept in sync by hand:
EventSubscriber/IndexableDataFilter/*) — shape a full reindex, applied insideDefaultIndexableDataProvider::getIds().Filter/Entity/*) — applied per entity inDefaultIndexer::indexEntities(), on both the batch and the incremental path.EnabledEntityFilter/StockAvailableEntityFilter/ChannelsAwareEntityFiltermirrored the SQL filters so the same rules also applied on the incremental (Doctrine-event) path, where the query builder never runs. If a SQL/PHP pair diverged, batch and incremental disagreed about what qualifies — and since the incremental path issuesdeleteDocumentson filter-out, a divergence could delete a document a reindex would keep.Solution
Route the incremental path through the same data provider the batch path uses, making the query-builder filters the single source of truth.
IndexableDataProviderInterfacegainscontainsId(string $entity, Index $index, int|string $id): bool.DefaultIndexableDataProviderbuilds its query once in a privatecreateQueryBuilder()shared bygetIds()andcontainsId(). The id constraint is added after theQueryBuilderForDataProvisionCreateddispatch, so every filter subscriber sees the identical query on both paths — that's what keeps them consistent.IndexEntityHandleraskscontainsId()whether a changed entity still qualifies for each index: index it if so,removeEntity()(delete its document) if not.Only the two true mirrors are removed (
EnabledEntityFilter,StockAvailableEntityFilter) along with their object-filter registrations in the extension.ChannelsAwareEntityFilter(per-scope channel membership — the SQL twin only checksSIZE(channels) > 0) andFilterableEntityFilter(arbitrary userFilterableInterface) stay.EntityFilterInterfaceremains the documented escape hatch for rules that can't be expressed in SQL or that depend on the channel/locale/currency scope.BC breaks (pre-1.0)
IndexableDataProviderInterface— custom data providers must implementcontainsId().Filter\Entity\EnabledEntityFilterandFilter\Entity\StockAvailableEntityFilterare removed.Notes
WHERE id = ?query per change per index instead of a purely in-memory check.Tests / QA
IndexEntityHandlerTest(index-when-contained, remove-when-not, not-found, non-scalar id).DefaultIndexableDataProviderTest(functional): provesgetIds()/containsId()agree and that disabling a product flipscontainsId()tofalse.composer validate --strict,composer normalize --dry-run,lint:container, Unit (111) and Functional (17) suites.