-
Notifications
You must be signed in to change notification settings - Fork 18
feat(elasticsearch-plugin): avoid redundant reindex on stock movements #51
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,124 @@ | ||
| import { describe, expect, it } from 'vitest'; | ||
|
|
||
| import { indexedDocumentsMatch, stableStringify, targetDocumentsById } from './index-diff'; | ||
|
|
||
| describe('stableStringify', () => { | ||
| it('is independent of object key order', () => { | ||
| expect(stableStringify({ a: 1, b: 2 })).toBe(stableStringify({ b: 2, a: 1 })); | ||
| }); | ||
|
|
||
| it('is independent of key order in nested objects', () => { | ||
| const x = { outer: { a: 1, b: { c: 3, d: 4 } } }; | ||
| const y = { outer: { b: { d: 4, c: 3 }, a: 1 } }; | ||
| expect(stableStringify(x)).toBe(stableStringify(y)); | ||
| }); | ||
|
|
||
| it('preserves array order (arrays are position-sensitive)', () => { | ||
| expect(stableStringify([1, 2, 3])).not.toBe(stableStringify([3, 2, 1])); | ||
| expect(stableStringify(['a', 'b'])).toBe(stableStringify(['a', 'b'])); | ||
| }); | ||
|
|
||
| it('omits undefined values so they equal an absent key', () => { | ||
| expect(stableStringify({ a: 1, b: undefined })).toBe(stableStringify({ a: 1 })); | ||
| }); | ||
|
|
||
| it('distinguishes null from absent/undefined', () => { | ||
| expect(stableStringify({ a: null })).not.toBe(stableStringify({})); | ||
| expect(stableStringify(null)).toBe('null'); | ||
| }); | ||
|
|
||
| it('serializes primitives', () => { | ||
| expect(stableStringify('x')).toBe('"x"'); | ||
| expect(stableStringify(42)).toBe('42'); | ||
| expect(stableStringify(true)).toBe('true'); | ||
| }); | ||
|
|
||
| it('distinguishes number from numeric string', () => { | ||
| expect(stableStringify({ n: 2 })).not.toBe(stableStringify({ n: '2' })); | ||
| }); | ||
| }); | ||
|
|
||
| describe('targetDocumentsById', () => { | ||
| const update = (id: string) => ({ operation: { update: { _id: id } } }); | ||
| const doc = (d: unknown) => ({ operation: { doc: d, doc_as_upsert: true } }); | ||
|
|
||
| it('pairs each update op with the following doc op', () => { | ||
| const ops = [update('1_10_en'), doc({ inStock: true }), update('1_11_en'), doc({ inStock: false })]; | ||
| const result = targetDocumentsById(ops); | ||
| expect(result.size).toBe(2); | ||
| expect(result.get('1_10_en')).toEqual({ inStock: true }); | ||
| expect(result.get('1_11_en')).toEqual({ inStock: false }); | ||
| }); | ||
|
|
||
| it('returns an empty map for no operations', () => { | ||
| expect(targetDocumentsById([]).size).toBe(0); | ||
| }); | ||
|
|
||
| it('ignores a trailing update op with no following doc', () => { | ||
| const ops = [update('1_10_en'), doc({ inStock: true }), update('1_11_en')]; | ||
| const result = targetDocumentsById(ops); | ||
| expect(result.size).toBe(1); | ||
| expect(result.has('1_11_en')).toBe(false); | ||
| }); | ||
|
|
||
| it('coerces numeric ids to strings', () => { | ||
| const ops = [{ operation: { update: { _id: 5 } } }, doc({ inStock: true })]; | ||
| expect(targetDocumentsById(ops).has('5')).toBe(true); | ||
| }); | ||
| }); | ||
|
|
||
| describe('indexedDocumentsMatch', () => { | ||
| const target = new Map<string, unknown>([ | ||
| ['1_10_en', { inStock: true, productInStock: true, sku: 'A', facetIds: ['1', '2'] }], | ||
| ['1_11_en', { inStock: false, productInStock: true, sku: 'B', facetIds: [] }], | ||
| ]); | ||
|
|
||
| it('returns true when ids and content match (ignoring key order)', () => { | ||
| const hits = [ | ||
| // deliberately reordered keys in _source | ||
| { _id: '1_11_en', _source: { facetIds: [], sku: 'B', productInStock: true, inStock: false } }, | ||
| { _id: '1_10_en', _source: { facetIds: ['1', '2'], sku: 'A', inStock: true, productInStock: true } }, | ||
| ]; | ||
| expect(indexedDocumentsMatch(target, hits)).toBe(true); | ||
| }); | ||
|
|
||
| it('returns false when a document field differs (e.g. inStock flipped)', () => { | ||
| const hits = [ | ||
| { _id: '1_10_en', _source: { inStock: false, productInStock: true, sku: 'A', facetIds: ['1', '2'] } }, | ||
| { _id: '1_11_en', _source: { inStock: false, productInStock: true, sku: 'B', facetIds: [] } }, | ||
| ]; | ||
| expect(indexedDocumentsMatch(target, hits)).toBe(false); | ||
| }); | ||
|
|
||
| it('returns false when a stock-derived custom field differs (guards the onStockStatusChange caveat)', () => { | ||
| const targetWithCustom = new Map<string, unknown>([ | ||
| ['1_10_en', { inStock: true, 'product-stockCount': 5 }], | ||
| ]); | ||
| const hits = [{ _id: '1_10_en', _source: { inStock: true, 'product-stockCount': 4 } }]; | ||
| expect(indexedDocumentsMatch(targetWithCustom, hits)).toBe(false); | ||
| }); | ||
|
|
||
| it('returns false when the index has an extra document (removal needed)', () => { | ||
| const hits = [ | ||
| { _id: '1_10_en', _source: { inStock: true, productInStock: true, sku: 'A', facetIds: ['1', '2'] } }, | ||
| { _id: '1_11_en', _source: { inStock: false, productInStock: true, sku: 'B', facetIds: [] } }, | ||
| { _id: '1_12_en', _source: { inStock: true, productInStock: true, sku: 'C', facetIds: [] } }, | ||
| ]; | ||
| expect(indexedDocumentsMatch(target, hits)).toBe(false); | ||
| }); | ||
|
|
||
| it('returns false when a target document is missing from the index (needs creating)', () => { | ||
| const hits = [ | ||
| { _id: '1_10_en', _source: { inStock: true, productInStock: true, sku: 'A', facetIds: ['1', '2'] } }, | ||
| ]; | ||
| expect(indexedDocumentsMatch(target, hits)).toBe(false); | ||
| }); | ||
|
|
||
| it('returns false when an indexed id is not in the target set', () => { | ||
| const hits = [ | ||
| { _id: '1_10_en', _source: { inStock: true, productInStock: true, sku: 'A', facetIds: ['1', '2'] } }, | ||
| { _id: '9_99_en', _source: { inStock: false, productInStock: true, sku: 'B', facetIds: [] } }, | ||
| ]; | ||
| expect(indexedDocumentsMatch(target, hits)).toBe(false); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,56 @@ | ||
| /** | ||
| * Deterministic JSON serialization (keys sorted, `undefined` omitted, array order preserved). | ||
| */ | ||
| export function stableStringify(value: any): string { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Also:
|
||
| if (value === null || typeof value !== 'object') { | ||
| return JSON.stringify(value) ?? 'null'; | ||
| } | ||
| if (Array.isArray(value)) { | ||
| return `[${value.map(v => stableStringify(v)).join(',')}]`; | ||
| } | ||
| const keys = Object.keys(value) | ||
| .filter(k => value[k] !== undefined) | ||
| .sort(); | ||
| return `{${keys.map(k => `${JSON.stringify(k)}:${stableStringify(value[k])}`).join(',')}}`; | ||
| } | ||
|
|
||
| /** | ||
| * Pairs the `{ update: { _id } }` and following `{ doc }` bulk operations produced for a product | ||
| * into a map of document id to document. | ||
| */ | ||
| export function targetDocumentsById( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| operations: Array<{ operation: any }>, | ||
| ): Map<string, unknown> { | ||
| const byId = new Map<string, unknown>(); | ||
| for (let i = 0; i < operations.length - 1; i++) { | ||
| const meta = operations[i].operation; | ||
| const body = operations[i + 1].operation; | ||
| if (meta?.update?._id != null && body?.doc) { | ||
| byId.set(String(meta.update._id), body.doc); | ||
| i++; | ||
| } | ||
| } | ||
| return byId; | ||
| } | ||
|
|
||
| /** | ||
| * True when the freshly-built documents (keyed by `_id`) match what is currently indexed, both in | ||
| * the set of ids and in content. | ||
| */ | ||
| export function indexedDocumentsMatch( | ||
| targetById: Map<string, unknown>, | ||
| currentHits: Array<{ _id: string; _source: unknown }>, | ||
| ): boolean { | ||
| if (currentHits.length !== targetById.size) { | ||
| return false; | ||
| } | ||
| for (const hit of currentHits) { | ||
| if (!targetById.has(hit._id)) { | ||
| return false; | ||
| } | ||
| if (stableStringify(targetById.get(hit._id)) !== stableStringify(hit._source)) { | ||
| return false; | ||
| } | ||
| } | ||
| return true; | ||
| } | ||
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.
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 abeforeAllin disguise, and the next two break withundefinedif it fails. Fixture is fixed, so assert the exact variant.take: 200will 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.