-
Notifications
You must be signed in to change notification settings - Fork 1.5k
fix(core): Invalidate stock location channel id cache correctly #5087
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
ryandiginomad
wants to merge
2
commits into
vendurehq:master
Choose a base branch
from
ryandiginomad:fix/multi-channel-stock-location-cache-invalidation
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+369
−19
Open
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
174 changes: 174 additions & 0 deletions
174
packages/core/src/config/catalog/multi-channel-stock-location-strategy.spec.ts
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,174 @@ | ||
| import { Subject } from 'rxjs'; | ||
| import { filter } from 'rxjs/operators'; | ||
| import { beforeAll, beforeEach, describe, expect, it, vi } from 'vitest'; | ||
|
|
||
| import { RequestContext } from '../../api/common/request-context'; | ||
| import { Cache } from '../../cache/cache'; | ||
| import { Channel } from '../../entity/channel/channel.entity'; | ||
| import { Product } from '../../entity/product/product.entity'; | ||
| import { StockLevel } from '../../entity/stock-level/stock-level.entity'; | ||
| import { StockLocation } from '../../entity/stock-location/stock-location.entity'; | ||
| import { ChangeChannelEvent } from '../../event-bus/events/change-channel-event'; | ||
| import { StockLocationEvent } from '../../event-bus/events/stock-location-event'; | ||
| import { ensureConfigLoaded } from '../config-helpers'; | ||
|
|
||
| /** | ||
| * Unit tests for the MultiChannelStockLocationStrategy channelId cache invalidation. | ||
| * | ||
| * The real `Cache` wrapper is used (backed by an in-memory Map) so that the | ||
| * key-prefixing behaviour of `Cache.get()`/`Cache.delete()` matches production — | ||
| * the invalidation bugs under test live exactly in that interaction. | ||
| */ | ||
|
|
||
| const cacheStore = new Map<string, any>(); | ||
| const mockCacheService = { | ||
| createCache: (config: any) => | ||
| new Cache(config, { | ||
| get: (key: string) => Promise.resolve(cacheStore.get(key)), | ||
| set: (key: string, value: any) => { | ||
| cacheStore.set(key, value); | ||
| return Promise.resolve(); | ||
| }, | ||
| delete: (key: string) => { | ||
| cacheStore.delete(key); | ||
| return Promise.resolve(); | ||
| }, | ||
| } as any), | ||
| } as any; | ||
|
|
||
| const eventStream = new Subject<any>(); | ||
| const mockEventBus = { | ||
| ofType: (type: any) => eventStream.asObservable().pipe(filter((e: any) => e.constructor === type)), | ||
| } as any; | ||
|
|
||
| let channelsInDb: Channel[]; | ||
| const getEntityOrThrow = vi.fn(() => Promise.resolve(new StockLocation({ id: 1, channels: channelsInDb }))); | ||
|
|
||
| const mockInjector = { | ||
| get: (token: unknown) => { | ||
| const name = (token as { name?: string })?.name; | ||
| if (name === 'EventBus') return mockEventBus; | ||
| if (name === 'CacheService') return mockCacheService; | ||
| if (name === 'TransactionalConnection') return { getEntityOrThrow }; | ||
| return {}; | ||
| }, | ||
| } as any; | ||
|
|
||
| const channel1 = new Channel({ id: 1 }); | ||
| const channel2 = new Channel({ id: 2 }); | ||
|
|
||
| function contextForChannel(channel: Channel): RequestContext { | ||
| return new RequestContext({ | ||
| apiType: 'shop', | ||
| channel, | ||
| authorizedAsOwnerOnly: false, | ||
| isAuthorized: true, | ||
| session: {} as any, | ||
| } as any); | ||
| } | ||
|
|
||
| const ctxChannel1 = contextForChannel(channel1); | ||
| const ctxChannel2 = contextForChannel(channel2); | ||
|
|
||
| const stockLevel = new StockLevel({ | ||
| stockLocationId: 1, | ||
| stockOnHand: 100, | ||
| stockAllocated: 0, | ||
| productVariantId: 1, | ||
| }); | ||
|
|
||
| const stockLocation = new StockLocation({ id: 1 }); | ||
|
|
||
| /** Lets the async `subscribe` handlers (cache deletes) settle. */ | ||
| function flushEventHandlers() { | ||
| return new Promise(resolve => setTimeout(resolve, 0)); | ||
| } | ||
|
|
||
| describe('MultiChannelStockLocationStrategy', () => { | ||
| let strategy: import('./multi-channel-stock-location-strategy').MultiChannelStockLocationStrategy; | ||
|
|
||
| beforeAll(async () => { | ||
| await ensureConfigLoaded(); | ||
| }); | ||
|
|
||
| beforeEach(async () => { | ||
| cacheStore.clear(); | ||
| channelsInDb = [channel1]; | ||
| getEntityOrThrow.mockClear(); | ||
| // Dynamic import to avoid vitest circular dependency issue | ||
| const { MultiChannelStockLocationStrategy } = | ||
| await import('./multi-channel-stock-location-strategy.js'); | ||
| strategy = new MultiChannelStockLocationStrategy(); | ||
| await strategy.init(mockInjector); | ||
| }); | ||
|
|
||
| it('caches the channel ids of a StockLocation after the first lookup', async () => { | ||
| const first = await strategy.getAvailableStock(ctxChannel1, 1, [stockLevel]); | ||
| const second = await strategy.getAvailableStock(ctxChannel1, 1, [stockLevel]); | ||
|
|
||
| expect(first.stockOnHand).toBe(100); | ||
| expect(second.stockOnHand).toBe(100); | ||
| expect(getEntityOrThrow).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| it('invalidates the cache when a StockLocation is updated', async () => { | ||
| // Cache the channel ids while the location belongs to channel1 only | ||
| const stale = await strategy.getAvailableStock(ctxChannel2, 1, [stockLevel]); | ||
| expect(stale.stockOnHand).toBe(0); | ||
|
|
||
| channelsInDb = [channel1, channel2]; | ||
| eventStream.next(new StockLocationEvent(ctxChannel1, stockLocation, 'updated')); | ||
| await flushEventHandlers(); | ||
|
|
||
| const fresh = await strategy.getAvailableStock(ctxChannel2, 1, [stockLevel]); | ||
| expect(fresh.stockOnHand).toBe(100); | ||
| }); | ||
|
|
||
| it('does not invalidate the cache when a StockLocation is created', async () => { | ||
| await strategy.getAvailableStock(ctxChannel1, 1, [stockLevel]); | ||
|
|
||
| eventStream.next(new StockLocationEvent(ctxChannel1, stockLocation, 'created')); | ||
| await flushEventHandlers(); | ||
|
|
||
| await strategy.getAvailableStock(ctxChannel1, 1, [stockLevel]); | ||
| expect(getEntityOrThrow).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| it('invalidates the cache when a StockLocation is assigned to a Channel', async () => { | ||
| // Cache the channel ids before the location is assigned to channel2 | ||
| const stale = await strategy.getAvailableStock(ctxChannel2, 1, [stockLevel]); | ||
| expect(stale.stockOnHand).toBe(0); | ||
|
|
||
| channelsInDb = [channel1, channel2]; | ||
| eventStream.next(new ChangeChannelEvent(ctxChannel1, stockLocation, [2], 'assigned', StockLocation)); | ||
| await flushEventHandlers(); | ||
|
|
||
| const fresh = await strategy.getAvailableStock(ctxChannel2, 1, [stockLevel]); | ||
| expect(fresh.stockOnHand).toBe(100); | ||
| }); | ||
|
|
||
| it('invalidates the cache when a StockLocation is removed from a Channel', async () => { | ||
| channelsInDb = [channel1, channel2]; | ||
| const beforeRemoval = await strategy.getAvailableStock(ctxChannel2, 1, [stockLevel]); | ||
| expect(beforeRemoval.stockOnHand).toBe(100); | ||
|
|
||
| channelsInDb = [channel1]; | ||
| eventStream.next(new ChangeChannelEvent(ctxChannel1, stockLocation, [2], 'removed', StockLocation)); | ||
| await flushEventHandlers(); | ||
|
|
||
| const fresh = await strategy.getAvailableStock(ctxChannel2, 1, [stockLevel]); | ||
| expect(fresh.stockOnHand).toBe(0); | ||
| }); | ||
|
|
||
| it('does not invalidate the cache on ChangeChannelEvents for other entity types', async () => { | ||
| await strategy.getAvailableStock(ctxChannel1, 1, [stockLevel]); | ||
|
|
||
| eventStream.next( | ||
| new ChangeChannelEvent(ctxChannel1, new Product({ id: 1 }) as any, [2], 'assigned', Product), | ||
| ); | ||
| await flushEventHandlers(); | ||
|
|
||
| await strategy.getAvailableStock(ctxChannel1, 1, [stockLevel]); | ||
| expect(getEntityOrThrow).toHaveBeenCalledTimes(1); | ||
| }); | ||
| }); |
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This proves the assignment direction only. I think the removal direction deserves an equivalent e2e test, since stale positive membership can keep a removed location contributing stock in the channel it was removed from, which is the higher risk case. The unit spec constructs a removed event, but it does not drive the real removeStockLocationsFromChannel path through the Shop projection.
A sibling case could warm IN_STOCK in the channel, remove the location, assert the relation is gone while the source stock stays positive, then poll the Shop query with a deadline until it returns OUT_OF_STOCK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added in ad3e83c. The test warms IN_STOCK in a fresh channel, removes the location, checks the location's own stockOnHand is untouched, then polls until OUT_OF_STOCK through the real removeStockLocationsFromChannel path.
One ordering detail that turned out to matter: the StockLevel row is created only after the channel assignment. Any admin response that resolves variant stock in between (assignProductsToChannel does, via the ProductVariant.stockOnHand field) runs the strategy and seeds the cache with the pre-assignment list, and on master that makes the warm-positive state unreachable. With the ordering right, I verified the test against master's dist: it fails exactly at the stale-positive point (poll stuck at IN_STOCK after removal) and passes on this branch.
Thanks for running an independent reproduction and for pushing on this direction. You were right that it is the higher-risk case.