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.
Closes #50.
Adds two opt-in options that stop stock movements from needlessly re-indexing a product, including the delete-then-recreate window during which the product drops out of search results. Both default to the current behavior, so existing installs are unaffected.
Changes
reindexOnStockMovement: 'always' | 'onStockStatusChange'(default'always')Pre-enqueue guard in the
StockMovementEventsubscriber. In'onStockStatusChange'it only enqueues an update when the movement flips a variant'sinStockor its product'sproductInStock, 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. Others should stay on'always'and use the option below.skipUnchangedIndexUpdates: boolean(defaultfalse)Write-path guard. Before the delete-then-recreate,
updateProductsOperationsbuilds the product's target 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 including stock-derived custom mappings, and it removes the flicker for any no-op update regardless of trigger. A full reindex is never skipped.The two are complementary: the first avoids creating a job for stock movements that cannot change search results, the second is a general safety net against a redundant delete-then-recreate from any trigger.
Backwards compatibility
SearchClientAdapterinterface (the comparison uses the existingsearch), and no index mapping or schema change.ProductVariantService.getSaleableStockLeveland the comparison reads through the adapter, so both features behave identically on Elasticsearch and OpenSearch.Implementation notes
updateProductsOperationsOnlywas split intobuildProductVariantOperations(build) and a thin execute wrapper, so the incremental path can build the target documents, diff them, and skip the write. Output is unchanged and writes are still chunked viaexecuteBulkOperationsByChunks.src/indexing/index-diff.ts(stableStringify,indexedDocumentsMatch) so it can be unit tested in isolation.Tests
Unit (
src/indexing/index-diff.spec.ts):stableStringify,targetDocumentsById(pairing the update and doc bulk operations back into a document map), andindexedDocumentsMatch, including that a stock-derived custom field difference still counts as a change, which is the correctness invariant behindonStockStatusChange.E2E (
e2e/elasticsearch-plugin.e2e-spec.ts): the existing suite now runs withskipUnchangedIndexUpdatesenabled, so every existing test exercises the guard on real updates. A newskipUnchangedIndexUpdatesdescribe adds explicit stock-movement cases: a non-boundary movement leaves search correct, and movements that flipinStockin and out of stock are reflected. Runs under both backends viaSEARCH_BACKEND.I validated
onStockStatusChange(the pre-enqueue guard) manually against a running OpenSearch instance: a non-boundary movement created no reindex job, a boundary movement flippedinStock/productInStock, and a full reindex rebuilt everything. I did not enableonStockStatusChangein the e2e config because the existing spec defines a stock-derived custom mapping, and that mode is not intended for such setups. Happy to add dedicated coverage for it if you would like, and to adjust the option names or defaults.