Repository navigation
feat(elasticsearch-plugin): avoid redundant reindex on stock movements - #51
Conversation
60ced9d to
e9e9d74
Compare
Adds two opt-in options that stop stock movements from needlessly re-indexing a product. Both default to the current behavior, so existing installs are unaffected. reindexOnStockMovement: 'always' | 'onStockStatusChange' (default 'always'). In 'onStockStatusChange' the StockMovementEvent subscriber only enqueues an update when the movement flips a variant's inStock or its product's productInStock, so a 50 to 49 change never creates a job. It inspects only the built-in stock booleans, so it is meant for setups without a stock-derived custom mapping. skipUnchangedIndexUpdates: boolean (default false). Before the delete-then-recreate, the indexer builds the product's documents, compares them against what is currently indexed, and skips the write when they are identical. It compares the whole document so it stays correct for any mapping configuration, and it removes the delete-then- recreate window during which a product drops out of search. A full reindex is never skipped. The document comparison reads through the existing SearchClientAdapter.search and the stock check uses ProductVariantService.getSaleableStockLevel, so both features behave the same on Elasticsearch and OpenSearch. No adapter interface or index mapping change. Refs vendurehq#50.
e9e9d74 to
0a3cd13
Compare
biggamesmallworld
left a comment
There was a problem hiding this comment.
Nice approach on skipUnchangedIndexUpdates. Diffing against the real builder output is the right call.
Three blockers:
- The refactor dropped bulk-operation chunking. Hits everyone, including full reindex, with both options off. See inline.
- Please drop
reindexOnStockMovement. Your description saysskipUnchangedIndexUpdatesis correct for any config and covers any trigger, which makes the other one a subset that's only correct sometimes. Add a stock-derived custom mapping later and you get a silently stale index. It also does an ES search plus a Product query plus per-variantgetSaleableStockLevelin the subscriber, so order placement pays for it. Dropping it takesproductStockStatusDiffersFromIndexand half the PR with it. stableStringifyreimplementsfast-deep-equal, which we already depend on, and it's buggy. See inline.
Also missing @since on both options, and elasticsearch-options.mdx needs regenerating.
| * path can build the target documents, compare them against what is currently indexed, and | ||
| * skip the write entirely when nothing changed (see `skipUnchangedIndexUpdates`). | ||
| */ | ||
| private async buildProductVariantOperations( |
There was a problem hiding this comment.
Both mid-build flushes are gone, so this buffers every operation for a product before returning. The description says writes are still chunked, which is true, but the build isn't anymore.
Count is channels × languages × variants × 2, each holding a full VariantIndexItem. 5000 variants, 3 channels, 4 languages = 120k buffered docs. reindex() at line 388 goes through here, so full reindex on a big catalog can now OOM. No opt-in needed.
The diff path needs them in memory, that's fine, it's opt-in. But updateProductsOperationsOnly has to keep streaming. A flush callback would do it.
| /** | ||
| * Deterministic JSON serialization (keys sorted, `undefined` omitted, array order preserved). | ||
| */ | ||
| export function stableStringify(value: any): string { |
There was a problem hiding this comment.
fast-deep-equal is already in package.json and imported in elasticsearch.service.ts:19.
Also: stableStringify(new Date()) returns {}. A custom mapping returning a Date never matches its own _source (an ISO string), so the skip never fires and the option quietly does nothing. Tests only cover primitives and plain objects.
equal(JSON.parse(JSON.stringify(doc)), hit._source) handles undefined, Dates, and key order, and compares exactly what gets stored.
| * Pairs the `{ update: { _id } }` and following `{ doc }` bulk operations produced for a product | ||
| * into a map of document id to document. | ||
| */ | ||
| export function targetDocumentsById( |
There was a problem hiding this comment.
buildProductVariantOperations has the id and the doc at construction time. Return { operations, documentsById } and this function plus its four tests go away. Array<{ operation: any }> also throws away the index field.
| * indexed. Returns `true` if any variant `inStock` or the product `productInStock` differs, or | ||
| * if the product is not indexed yet. | ||
| */ | ||
| private async productStockStatusDiffersFromIndex( |
There was a problem hiding this comment.
This recomputes stock status differently from createVariantIndexItem and they already disagree. ProductVariant.deletedAt is a plain @Column, not @DeleteDateColumn, so line 249 pulls in soft-deleted variants. The builder filters them and also force-disables everything when product.enabled is false, which changes productInStock.
Both differences fail safe today. The issue is that isProductIndexUnchanged gets this right by calling the real builder, and this one will go stale next time createVariantIndexItem changes with nothing to catch it.
| body: { | ||
| query: { term: { productId } }, | ||
| _source: ['channelId', 'productVariantId', 'inStock', 'productInStock'], | ||
| size: 10000, |
There was a problem hiding this comment.
10000 is the default index.max_result_window, so this truncates at the ceiling with no error. Same literal at 795.
Safe there (fewer hits = mismatch = write), not here: dropped hits mean dropped channel buckets, if (!channelDocs) continue skips them, guard says unchanged. Named constant plus handle the boundary. Both call sites are the same term: { productId } search too.
| // so every existing test already exercises the guard on real updates. These add explicit | ||
| // stock-movement cases: a non-boundary movement must leave search correct, and a movement | ||
| // that flips inStock must be reflected. | ||
| describe('skipUnchangedIndexUpdates', () => { |
There was a problem hiding this comment.
These pass with the flag off: they assert search is correct, which it already was. Nothing checks a write was skipped.
Sentinel makes it deterministic: write an extra field into the index doc, run the no-op update, assert it survived. Proves the delete-then-recreate didn't happen.
it('finds an in-stock variant to exercise') is a beforeAll in disguise, and the next two break with undefined if it fails. Fixture is fixed, so assert the exact variant. take: 200 will silently truncate once the fixture grows.
Flipping the whole suite also means these 96 tests stop covering the default path. The uuid spec still does, but that's 7 tests. Prefer a targeted describe.
Addresses the review on vendurehq#51. - Replace delete-then-recreate with an incremental writer: diff the freshly built documents against the index and issue only the changed upserts and the deletes for documents that no longer exist, writing nothing when nothing changed. Upserts use a full-document `index` replace so a field that is no longer present is cleared, and a still-current document is never removed, so a product no longer drops out of search during an update. Governed by `incrementalIndexUpdates` (default true); a product too large to diff safely falls back to the streaming delete-then-recreate path. - Restore streaming in the reindex and opt-out paths via a visitor builder, so a full reindex no longer buffers a whole large product before writing. - Drop the hand-rolled stableStringify in favour of the existing fast-deep-equal dependency, comparing the built document as the index stores it (Dates, undefined, key order). The diff logic lives in index-diff for unit testing. - Rename skipUnchangedIndexUpdates to incrementalIndexUpdates and default it on. - Make reindexOnStockMovement 'onStockStatusChange' self-disable when a custom mapping is configured, drop its extra index read's magic number and handle the result-window boundary, and share the inStock/productInStock computation with the document builder so the guard cannot drift from it. - Add @SInCE to both options and regenerate elasticsearch-options.mdx.
|
Community Plugins — View preview |
…pdates An admin variant update fires a ProductVariantEvent, which always enqueued a job, so a stock-only admin edit that did not flip inStock still created a job even under reindexOnStockMovement 'onStockStatusChange'. The event carries the update input, so the plugin can now tell a stock-only edit from an index-affecting one. - Add isStockOnlyVariantUpdate (pure, unit tested): true only when the update input touches stock-level fields alone. - Route ProductVariantEvent 'updated' through updateVariantsForVariantEvent, which reuses the existing stock guard and skips the job when the update is stock-only and the indexed stock status would not change. Any other update enqueues as before. - Broaden the reindexOnStockMovement docs to cover admin stock-only edits and regenerate elasticsearch-options.mdx. - Fix the incrementalIndexUpdates e2e to correlate the indexed document by sku (the index stores the raw id while the GraphQL id is encoded) and add a stock-guard e2e spec (no custom mappings) proving a non-flipping admin stock edit creates no write and a flipping or non-stock edit does. Full suite passes against OpenSearch.
…rds compatible Default incrementalIndexUpdates to false so the write path is byte-for-byte the historic delete-then-recreate unless a consumer opts in. The incremental path uses a full-document index replace (to clear removed fields without deleting first), while the reindex and default paths keep the original update + doc_as_upsert. - incrementalIndexUpdates now defaults to false; both new options are opt-in and reindexOnStockMovement already defaulted to 'always'. - Split the write op builders: documentToOperations (update + doc_as_upsert) for the reindex/default paths, documentToReplaceOperations (index) for the incremental path. - Move the incremental e2e into its own spec that enables the flag, so the default suite keeps covering the historic delete-then-recreate path. - Document both options and how to enable them in the plugin README (which drives the reference docs page) and regenerate the reference docs. Verified locally against Elasticsearch 9.3.4 and OpenSearch 3.7.0: full e2e suite green on both, plus unit tests.
|
Thanks for the thorough review. I reworked the PR around what turned out to be the real issue, the delete-then-recreate, and made every new behaviour opt-in, so with the defaults unchanged the plugin behaves exactly as before. Nothing changes for current installs until a flag is set. Two opt-in options.
Your specific points.
Magic number. Both index reads use a named INDEX_MAX_RESULT_WINDOW and fall back to a full write if a read reaches the window. @SInCE and docs. Added to both options. Documented both options and how to enable them on the plugin page and regenerated the reference docs. Heads up that the regen also picked up earlier drift from the adapter refactor, since the committed docs were stale. Tests. The default suite runs with both flags off, so it still covers the historic path. incrementalIndexUpdates has its own spec that reads the document directly rather than paging with take: 200, asserts the exact fixture variant, and proves a skip by checking the document _version stays put on a no-op and increments on a real change. A separate stock-guard spec with no custom mappings proves a non-flipping admin stock edit creates no write while a flipping or non-stock edit does. I ran the full suite locally against both Elasticsearch 9.3.4 and OpenSearch 3.7.0, all green. On point 2, keeping reindexOnStockMovement. With correctness owned by the whole-document paths, this is a pre-enqueue optimization. It self-disables when a custom mapping is configured, so the stock-booleans-only check runs only when those booleans are provably the only stock-derived fields, and adding a mapping later cannot cause a stale index. The recompute that could drift is gone, both paths share one helper, and it no longer does an index search or a heavy product load on the order path. It is not redundant with the write-path guard: that one runs inside the worker, so by the time it decides nothing changed, the job row is written, the poll waited, and the worker ran. On a database-backed queue that per-change job is the dominant cost, and stock changes are the highest-frequency event in a busy catalog, so skipping the job keeps the queue shallow and lets real updates reach the index sooner. Thanks again for the careful read. Everything is opt-in, so it is safe to merge and enable per deployment. |
|
Hi @biggamesmallworld please review the latest changes! Thank you. |
# Conflicts: # packages/elasticsearch-plugin/src/indexing/indexer.controller.ts
michaelbromley
left a comment
There was a problem hiding this comment.
Thanks for the rework. The incremental write path looks good to me: the historic behaviour is the default, it falls back safely, and the diff logic is unit tested. I'm OK with keeping reindexOnStockMovement, but the check it runs needs to scale with the stock movement, not with the size of the product.
The cost today
productStockStatusDiffersFromIndex runs on the server for every StockMovementEvent, and an order typically fires two or three of those (allocation, sale, and cancellation if any). For each product in the event it:
- searches the index for all of the product's documents,
- loads the product with all its variants and their channels,
- calls
getSaleableStockLevelfor every variant in every channel, then again for every enabled variant insidegetProductInStockValue. Each call is aStockLevelquery.
That is roughly 2 × variants × channels queries per product per event, run one after another:
| Product | Queries per event |
|---|---|
| 5 variants, 1 channel | ~13 |
| 20 variants, 1 channel | ~43 |
| 200 variants, 3 channels | ~1,200 |
The subscriber doesn't block the order request, but it shares the database pool with customer requests, so for large products at peak traffic it competes with checkout. (The Sep 13 reply says the guard no longer does an index search or a product load on the order path, but it still does both.)
Moving the check to the worker wouldn't help. The worker uses the same database, and the point of the option is to avoid creating the job.
Proposal: only check the variants that moved
- A variant's
inStockcan only flip if its own stock moved. productInStockcan only flip if some variant'sinStockflipped.
So if none of the moved variants flipped, nothing the guard cares about changed, and the rest of the product never needs loading. The check becomes:
- Load the moved variants in one query, for the
trackInventoryand threshold fields thatgetSaleableStockLevelneeds. - Run one search with a
termsquery onproductVariantIdfor those variants, with_source: ['productVariantId', 'channelId', 'inStock']. - For each (variant, channel) pair found, set the channel on the context and call
computeVariantInStockonce. Returntrueon the first mismatch, or if a moved variant has no indexed document.
That is one variant query, one search, and one stock query per moved variant per channel. A single order line goes from 13+ queries to about 3, and the cost no longer grows with the size of the product. The product load and getProductInStockValue drop out of the guard, and so does the hand-copied soft-delete and disabled-product logic, which also settles my earlier concern about it drifting from createVariantIndexItem.
One trade-off: the current version also notices a productInStock that is already wrong in the index for some unrelated reason, and the narrower check won't. I think that's fine, since repairing a stale index isn't this guard's job, but it's worth a line in the doc comment.
updateVariantsForVariantEvent uses the same guard, so it gets the same improvement.
Also before merge
- The PR description still describes
skipUnchangedIndexUpdates,stableStringifyandtargetDocumentsById. Please update it to match the current design. - Both options say
@since 2.2.0while the package is at2.0.0. Please confirm that's the intended release.
With the guard scoped to the moved variants, I'm happy to approve.
Addresses the review on vendurehq#51. The stock guard used to search every indexed document of each affected product, load the product with all variants and channels, and compute the saleable stock level for every variant in every channel (twice, once more inside the productInStock helper). A variant's inStock can only flip if its own stock moved, and productInStock can only flip if some variant's inStock flipped. So the guard now looks at the moved variants only: - Load the moved variants (with channels) in one query. - Read their indexed inStock values in one search, a terms query on productVariantId with a narrow _source. - Recompute inStock once per moved variant per indexed channel and return true on the first mismatch. It enqueues conservatively when it cannot judge: no variant ids, a missing or soft-deleted variant, a variant with no indexed document, a document for a channel the variant is no longer in, a non-boolean indexed value, or a result that may be truncated at the result window. The 'always' short circuit, the custom mapping fallback, the catch-all and the channel restore are kept. The admin stock-only update path uses the same guard and benefits as well. Trade-off: sibling variants are no longer recomputed, so the guard does not notice a productInStock that is already stale in the index for an unrelated reason. Repairing a stale index is not this guard's job. Tests: unit tests for the guard (flip, no flip, multi-channel, missing doc, truncation, errors, short circuits, and a regression guard that only the moved variant is loaded and evaluated once per channel), plus e2e cases that place real orders: a 50 to 49 allocation creates no job, and a sell-out allocation reindexes and flips inStock and productInStock.
Explain in the reindexOnStockMovement option docs and the README how the check is scoped to the moved variants, and its trade-off with an already stale productInStock. Regenerate the reference docs.
|
Thank you @michaelbromley, that makes sense. I've changed the guard to check only the moved variants, as you suggested:
A single order line in one channel is now three small SQL queries and one search, however many variants the product has. The product load, getProductInStockValue and the copied soft-delete and disabled-product logic are gone from the guard. It still enqueues when it can't decide (missing or soft-deleted variant, possibly truncated result, any error), and the custom mapping fallback is unchanged. The doc comment notes the stale productInStock trade-off. updateVariantsForVariantEvent uses the same method, so it gets this too. You were right about my Sep 13 reply, sorry about that. The guard still did both the search and the product load at that point. Now there's no product load, and the one search covers only the moved variants. Tests: there's a unit regression check that with 20 variants across 2 channels and one moved, only that variant is loaded and its stock is checked once per channel. The e2e spec now places real orders: a 50 to 49 allocation creates no job, and a sell-out reindexes and flips inStock and productInStock. The PR description is updated to match the current design. On @SInCE, I kept 2.2.0 because options.ts already has 2.1.7 tags from the core plugin's history, so 2.1.0 would look older than existing options. Happy to switch to 2.1.0 if you prefer. |
Closes #50.
Adds two opt-in options to reduce search index churn. Both default to the existing behaviour, so current installs see no change until they enable them.
Options
reindexOnStockMovement('always' | 'onStockStatusChange', default'always'): Controls whether a stock change enqueues an index update job. It covers order-drivenStockMovementEvents (allocations, sales, cancellations, releases, adjustments) and admin variant updates whose input touches only stock fields (stockOnHand,stockLevels,trackInventory,outOfStockThreshold,useGlobalOutOfStockThreshold). With'onStockStatusChange'the plugin checks before creating a job whether any moved variant'sinStockwould flip, and skips the job if not. A 50 to 49 sale that leaves the item in stock no longer costs a queue write, a poll and a worker cycle.incrementalIndexUpdates(boolean, defaultfalse): When enabled, a product update reads the product's indexed documents, compares them with the freshly built ones, and writes only the difference. Changed or new documents are written with a full-document index replace, documents that no longer exist are deleted, and nothing is written when nothing changed. A still-current document is never deleted first, so the product does not drop out of search during an update. A product with more documents than the result window, or a failed index read, falls back to the existing delete-then-recreate path. A full reindex is unchanged.How the stock check works
inStockcan only flip if its own stock moved, andproductInStockcan only flip if some variant'sinStockflipped. So the check only looks at the moved variants.termsquery onproductVariantIdwith a narrow_source) reads their indexedinStockvalues, and the saleable stock level is computed once per moved variant per indexed channel. For a single order line in one channel that is three small SQL queries and one search, regardless of product size.customProductMappingsorcustomProductVariantMappingsis configured, a custom field could derive from stock without aninStockflip, so the check turns itself off and every stock change enqueues as before.productInStockthat is already stale in the index for an unrelated reason. Fixing a stale index is left to a reindex or the next non-stock update.Backwards compatibility
reindexOnStockMovementdefaults to'always'andincrementalIndexUpdatestofalse, so with no config change the write path and the job behaviour are the same as today.doc_as_upsertoperations. Only the incremental path uses the index replace.Implementation notes
src/indexing/index-diff.ts. It compares the built document as the index stores it (JSON round trip, so undefined fields, Dates and key order are handled) using the existingfast-deep-equaldependency.src/indexing/stock-only-update.tsdecides whether aProductVariantEvent'updated'input touched only stock fields.computeVariantInStockhelper, so they cannot computeinStockdifferently.Tests
'always'and custom mapping short circuits, and a regression check that only the moved variant is loaded and evaluated once per channel. There are also tests for the diff helper and the stock-only update detection.inStockandproductInStockin search. It also covers admin stock edits and non-stock edits. An incremental spec covers in-place writes, no-op updates and field removal. The default suite still covers delete-then-recreate.