feat(elasticsearch-plugin): index and search per-currency variant prices#30
feat(elasticsearch-plugin): index and search per-currency variant prices#30tbouliere-datasolution wants to merge 9 commits into
Conversation
|
Hi! I just merged another PR that causes conflicts with this one - can you take a look at the merge conflicts? |
b6ca7ed to
e2e8e0b
Compare
grolmus
left a comment
There was a problem hiding this comment.
Thanks for putting this together — the diagnosis is right (single-currency-per-doc is broken for multi-currency channels) and the indexing/reading symmetry is exactly the shape of the fix. I pulled the branch, ran unit + e2e (all 99 green) and then probed a few scenarios the new tests don't cover. Found a handful of blockers I'd like to discuss before this lands.
I'm happy to take some/all of these on rather than ping-pong; let me know which you'd prefer.
Blockers
1. Zero-priced phantom documents
When a variant is assigned to a channel with availableCurrencyCodes = [GBP, EUR] but only has a ProductVariantPrice row in GBP, the EUR document is indexed with price = 0, priceWithTax = 0.
Reproduced locally with a probe e2e: T_1 assigned to a [GBP, EUR] channel, no EUR price added, reindex. Direct ES lookup of _doc/2_1_en_EUR returns price: 0, priceWithTax: 0, currencyCode: \"EUR\". The shop API then surfaces this in searchProductsShopDocument with currencyCode: EUR as a normal hit.
Root cause is in core, not this PR — but this PR exposes it:
DefaultProductVariantPriceSelectionStrategy.selectPrice(core:packages/core/src/config/catalog/default-product-variant-price-selection-strategy.ts:18-22) returnsundefinedwhen no matching currency exists.ProductPriceApplicator.applyChannelPriceAndTax(core:packages/core/src/service/helpers/product-price-applicator/product-price-applicator.ts:66-106) defaultsthrowIfNoPriceFound = false, falls through withinputPrice: channelPrice?.price ?? 0, and writesvariant.listPrice = 0,variant.currencyCode = ctx.currencyCode.- The indexer then unconditionally builds and ships a doc.
Sort impact: sort: { price: ASC } floats every phantom doc to the top of the result page. priceRange: { min: 1 } works around it but consumers shouldn't need to know that.
Two options, either acceptable:
- Skip: after
applyChannelPriceAndTax, ifvariant.listPrice === 0and noProductVariantPriceexists invariant.productVariantPricesforctx.currencyCode + ctx.channelId, do not push operations for that (variant, currency) tuple. Log a debug message. - Gate: make per-currency indexing opt-in via a new option (see #2). When the option is off, behaviour is unchanged from
main. When on, document that variants must have explicit prices in every available currency or rely on a customProductVariantPriceSelectionStrategyfallback.
2. No opt-in option (parity with core's DefaultSearchPlugin)
Vendure core's DefaultSearchPlugin indexer gates per-currency indexing behind this.options.indexCurrencyCode (packages/core/src/plugin/default-search-plugin/indexer/indexer.controller.ts:438-442). This PR has no equivalent, so:
- Every existing deployment — including single-currency channels — gets a forced behaviour change and a forced reindex on upgrade (the doc
_idschema changes). - Single-currency users gain nothing from the change.
Suggest mirroring core: add indexCurrencyCode?: boolean to ElasticsearchPluginOptions (default false). When false, iterate [channel.defaultCurrencyCode] and keep the _id shape backward-compatible (or write to both schemas for a migration window).
3. Deletion path leaves orphaned per-currency docs
The delete operations in deleteVariantsInternalOperations (packages/elasticsearch-plugin/src/indexing/indexer.controller.ts:804-852) and deleteProductOperations (:715-802) iterate the current channel.availableCurrencyCodes. If the channel's available currencies change between indexing and deletion, docs for the removed currencies are never deleted.
Reproduced locally:
- Channel
[GBP, EUR], variant T_1 indexed → ES contains_ids: ['2_1_en_GBP', '2_1_en_EUR']. - Update channel to
availableCurrencyCodes: [GBP]. - Delete variant T_1 → ES now contains
_ids: ['2_1_en_EUR']— the EUR doc is orphaned.
Suggest replacing per-currency delete ops with a single delete_by_query keyed on { channelId, productVariantId } (and for synthetic products { channelId, productId, productVariantId: -product.id }). Same network shape, idempotent, immune to channel-config drift.
4. createVariantIndexItem cross-currency contamination is one refactor away
applyChannelPriceAndTax mutates variant.listPrice, variant.listPriceIncludesTax, variant.currencyCode, variant.taxRateApplied in place on the same object reference across currency iterations (indexer.controller.ts:566-612). Today it works because the doc build is awaited inline and captures eagerly. Any future refactor that batches/defers createVariantIndexItem calls (e.g. parallel Promise.all across variants, deferred bulk-op composition) will silently produce cross-currency contamination — variant A's GBP doc shipped with variant B's EUR price.
Minimum: a comment near :569-570 pinning the invariant ("do not move this await outside the inner loop; the variant entity is mutated in place"). Better: snapshot the relevant price fields into local variables before pushing the bulk op, so the captured doc is immune to later mutation.
5. Mutated ctx state needs try/finally
setCurrencyCode and setChannel mutate ctx, and the same ctx is reused across products in updateProductsOperations (:666-672). An exception mid-channel-loop leaves mutatedCurrencyCode and ctx.channel in a wrong state for the next product. Wrap the channel loop body in try/finally and reset in finally. Pair the setCurrencyCode(undefined) (:652) and setChannel(originalChannel) (:654) into the same cleanup.
6. CurrencyAwareMutableRequestContext.deserialize via Object.setPrototypeOf
currency-aware-request-context.ts:22-25 retrofits the prototype after the parent factory returns. Works today, but:
- If core ever migrates
MutableRequestContextto private#fields, the swap can't reach them. - V8 deopts on prototype mutation of existing objects (not catastrophic, but worth avoiding in hot indexing paths).
The lighter currency-only swap is otherwise a nice improvement over core's new Channel({ ...channel, defaultCurrencyCode }) pattern — it sidesteps the side effects of mutating the channel reference (cache keys, tax-zone resolution memoised by channelId, etc.). Suggest keeping the approach but rewriting deserialize to construct an instance directly from ctxObject without prototype manipulation. Failing that, a comment noting the upstream-coupling risk.
7. CHANGELOG and README
The PR introduces:
- A forced full reindex on upgrade (doc
_idschema change). - A new requirement that variants have explicit
ProductVariantPricerows in every channel-available currency (or a customProductVariantPriceSelectionStrategywith fallback semantics).
Neither is documented yet. README's plugin section + a top-level MIGRATION.md note would catch most users; CHANGELOG will be generated by lerna at release time.
Non-blocking
- The
channel.availableCurrencyCodes?.length ? channel.availableCurrencyCodes : [channel.defaultCurrencyCode]fallback is duplicated 3x (:562,:754,:823). Worth extracting to agetChannelIndexCurrencies(channel)helper. build-elastic-body.ts:42'sif (ctx.currencyCode)guard exists only to keepbuild-elastic-body.spec.ts's test contexts (which build aRequestContextfromnew Channel({ id })with nodefaultCurrencyCode) passing. The conditional reads like an optional contract; fixing the tests to construct a proper ctx and dropping the guard would be cleaner.- The PR description acknowledges that custom
ProductVariantPriceSelectionStrategyimplementations that ignorectx.currencyCodewill produce duplicate prices. A startup-time warning log whenconfigService.catalogOptions.productVariantPriceSelectionStrategyis not the default and multi-currency indexing is on would catch misconfigurations early. - Synthetic product docs (
createSyntheticProductIndexItem) are nowchannels × languages × currenciesper variantless product. Fine functionally, slight index bloat for multi-currency stores with many empty products. - A regression e2e covering the missing-currency-price case (the scenario above) would prevent reintroduction.
Rolling-deploy note
UpdateProductMessageData / VariantChannelMessageData shapes are unchanged, so old workers won't crash on new messages. But during a rolling deploy, old workers will write docs with the 3-part _id schema while new workers write the 4-part schema, producing index inconsistency until everyone catches up + a reindex runs. Worth a one-line note in the migration doc.
Solid groundwork — the indexing/reading symmetry and the CurrencyAwareMutableRequestContext design are both better than core's pattern. Mainly the missing-price handling, the opt-in, and the deletion path need to be addressed before this is mergeable. Happy to pick up any subset of these on a follow-up PR if that's easier — let me know.
|
Hello @grolmus. |
|
«build-elastic-body.ts:42's if (ctx.currencyCode) guard exists only to keep build-elastic-body.spec.ts's test contexts (which build a RequestContext from new Channel({ id }) with no defaultCurrencyCode) passing. The conditional reads like an optional contract; fixing the tests to construct a proper ctx and dropping the guard would be cleaner.» This guard was implemented to avoid having to refactor the existing end-to-end test that simulates the request. |
e2e8e0b to
464561e
Compare
Context
Until now the Elasticsearch index used
${channelId}_${entityId}_${languageCode}as document ID and only stored the price for the channel's default currency. As a consequence, a channel withavailableCurrencyCodes: [GBP, EUR]would exposeGBP-priced variants only — querying it with
currencyCode = EURreturned the GBP price.Changes
Indexing
CurrencyAwareMutableRequestContext(extendsMutableRequestContext) that exposessetCurrencyCode()and overrides thecurrencyCodegetter so the indexing context can switch currency without mutating the activeChannel.ElasticsearchIndexerController.getId(...)now takes acurrencyCodeand produces${channelId}_${entityId}_${languageCode}_${currencyCode}. One document per (channel × language × currency) instead of one per (channel × language).updateProductsInternaliterates overchannel.availableCurrencyCodes(fallback to[channel.defaultCurrencyCode]). For each currency it callsctx.setCurrencyCode(currencyCode)thenapplyChannelPriceAndTax(variant, ctx). TheDefaultProductVariantPriceSelectionStrategyalready filters byctx.currencyCode, so the matchingProductVariantPriceis selected automatically — no patch of the price applicator was needed.deleteProductOperationsanddeleteVariantsInternalOperationsnow iterate over each channel's currencies as well, to keep deletions symmetrical with indexing. The channels query was extended to selectdefaultCurrencyCodeandavailableCurrencyCodes. The internal helper signature changed fromchannelIds: ID[]tochannels: Channel[](single internal caller updated).Reading
build-elastic-body.tsadds aterm: { currencyCode: ctx.currencyCode }filter whenctx.currencyCodeis defined. This mirrors the read-side behavior ofDefaultProductVariantPriceSelectionStrategyand avoids returning N duplicates pervariant when several currencies are indexed. In production
ctx.currencyCodeis always populated (RequestContext falls back tochannel.defaultCurrencyCodeat construction), so the filter is always applied; theifguard exists only for teststhat build a context without a channel default.
Out of scope / known caveats
productVariantPriceSelectionStrategyimplementations that ignorectx.currencyCodewill still produce identical prices across currencies during indexing. Users who override that strategy must keepctx.currencyCodeas a discriminatorif they want this feature to work.
currencyCodefield was already mapped askeywordinindexing-utils.ts, so no mapping change is required. A full reindex is required after merge because document IDs change shape.Tests
e2e/elasticsearch-plugin.e2e-spec.ts—multiple currency handlingdescribe:beforeAllfetches variants ofT_3andT_4, switches to the multi-currency channel and sets EUR prices viaupdateProductVariants(prices: [{ currencyCode: EUR, price: ... }]).return EUR when requestedqueries the shop API with thecurrencyCode: EURheader and asserts the returnedprice.min/maxmatches the EUR amounts that were set.return GBP by defaulttest still passes — the channel default currency is GBP.build-elastic-body.spec.tskeeps passing as-is: the testRequestContextis built without a currency, soctx.currencyCodeisundefinedand the new filter is not applied.Migration
After deploy, trigger a full
reindexmutation on each channel. Old documents (without currency in their_id) will be left dangling until they are explicitly cleaned up — alternatively, drop and recreate the index.