From e2d6ff385de5c45b0f2ab87b79fade91ff8c0512 Mon Sep 17 00:00:00 2001 From: David Nowinsky Date: Mon, 19 Aug 2024 11:40:51 +0200 Subject: [PATCH] fix(ui): only result of the last request should be used TCTC-8992 (#2204) * fix(ui): only result of the last request should be used The other should be discarded * test(ui): cases for previews request that arrives not in order * fix(ui): format --- ui/src/store/actions.ts | 26 ++++++-- ui/src/store/state.ts | 2 + ui/tests/unit/store.spec.ts | 117 ++++++++++++++++++++++++++++++++++++ 3 files changed, 141 insertions(+), 4 deletions(-) diff --git a/ui/src/store/actions.ts b/ui/src/store/actions.ts index 306c1239e6..e3269fd16a 100644 --- a/ui/src/store/actions.ts +++ b/ui/src/store/actions.ts @@ -250,6 +250,7 @@ const actions: PiniaActionAdaptor = { }, async updateDataset(): Promise | undefined> { this.logBackendMessages({ backendMessages: [] }); + let requestPromise; try { this.setLoading({ type: 'dataset', isLoading: true }); this.toggleRequestOnGoing({ isRequestOnGoing: true }); @@ -264,13 +265,18 @@ const actions: PiniaActionAdaptor = { }); return; } - const response = await this.backendService.executePipeline( + requestPromise = this.ongoingRequestPromise = this.backendService.executePipeline( this.activePipeline, this.pipelines, this.pagesize, pageOffset(this.pagesize, this.pageNumber), this.previewSourceRowsSubset, ); + const response = await requestPromise; + if (requestPromise !== this.ongoingRequestPromise) { + // If another request has started in between, discard the result + return; + } const translator = response.translator ?? 'mongo50'; // mongo50 is not send by backend this.setTranslator({ translator }); const backendMessages = response.error || response.warning || []; @@ -280,8 +286,19 @@ const actions: PiniaActionAdaptor = { dataset: addLocalUniquesToDataset(response.data), }); } + + this.toggleRequestOnGoing({ isRequestOnGoing: false }); + this.setLoading({ type: 'dataset', isLoading: false }); + return response; } catch (error) { + if (requestPromise) { + if (requestPromise !== this.ongoingRequestPromise) { + // If another request has started in between, discard the result + return; + } + } + /* istanbul ignore next */ const response = { error: [formatError(error)] }; // Avoid spamming tests results with errors, but could be useful in production @@ -293,11 +310,12 @@ const actions: PiniaActionAdaptor = { this.logBackendMessages({ backendMessages: response.error, }); - /* istanbul ignore next */ - throw error; - } finally { + this.toggleRequestOnGoing({ isRequestOnGoing: false }); this.setLoading({ type: 'dataset', isLoading: false }); + + /* istanbul ignore next */ + throw error; } }, // COLUMNS diff --git a/ui/src/store/state.ts b/ui/src/store/state.ts index 04cfd8cdf0..5222e4c240 100644 --- a/ui/src/store/state.ts +++ b/ui/src/store/state.ts @@ -14,6 +14,7 @@ export interface VQBState { // necessary methods to preview pipeline results and fetch available data collections backendService: BackendService; isRequestOnGoing: boolean; + ongoingRequestPromise: Promise | undefined; isLoading: { dataset: boolean; @@ -99,6 +100,7 @@ export function emptyState(): VQBState { backendMessages: [], isLoading: { dataset: false, uniqueValues: false }, isRequestOnGoing: false, + ongoingRequestPromise: undefined, availableVariables: [], variables: {}, variableDelimiters: undefined, diff --git a/ui/tests/unit/store.spec.ts b/ui/tests/unit/store.spec.ts index 69ee125035..022103456b 100644 --- a/ui/tests/unit/store.spec.ts +++ b/ui/tests/unit/store.spec.ts @@ -9,6 +9,7 @@ import { formatError } from '@/store/actions'; import { currentPipeline, emptyState } from '@/store/state'; import { buildState, buildStateWithOnePipeline, setupMockStore } from './utils'; +import { VQB_MODULE_NAME } from '@/store'; describe('getter tests', () => { beforeEach(() => { @@ -1015,6 +1016,122 @@ describe('action tests', () => { }); }); + it('updateDataset before previous update is done', async () => { + const pipeline: Pipeline = [ + { name: 'domain', domain: 'GoT' }, + { name: 'replace', searchColumn: 'characters', toReplace: [['Snow', 'Targaryen']] }, + { name: 'sort', columns: [{ column: 'death', order: 'asc' }] }, + ]; + const executePipelineMock = vi.fn(); + const store = setupMockStore({ + ...buildStateWithOnePipeline(pipeline), + backendService: { + executePipeline: executePipelineMock, + }, + }); + + executePipelineMock.mockImplementation(() => { + let _res, _rej; + const promise = new Promise((res, rej) => { + _res = res; + _rej = rej; + }); + promise._resolve = _res; + promise._reject = _rej; + return promise; + }); + const setDatasetSpy = vi.spyOn(store, 'setDataset'); + + const updateDataset1 = store.updateDataset(); + expect(store.isLoading.dataset).toBe(true); + const executePipeline1 = executePipelineMock.mock.results[0].value; + + // Ask for a second preview + const updateDataset2 = store.updateDataset(); + const executePipeline2 = executePipelineMock.mock.results[1].value; + expect(store.isLoading.dataset).toBe(true); + + // The second preview arrives + const dummyDataset: DataSet = { + headers: [{ name: 'x' }, { name: 'y' }], + data: [ + [1, 2], + [3, 4], + ], + }; + executePipeline2._resolve({ data: dummyDataset }); + await updateDataset2; + expect(store.isLoading.dataset).toBe(false); + expect(store.dataset.data).toEqual(dummyDataset.data); + + // Then the first one, late + const outdatedDummyDataset: DataSet = { + headers: [{ name: 'x' }, { name: 'y' }], + data: [ + [5, 6], + [7, 8], + ], + }; + executePipeline1._resolve({ data: outdatedDummyDataset }); + await updateDataset1; + expect(store.isLoading.dataset).toBe(false); + expect(store.dataset.data).toEqual(dummyDataset.data); // outdated data should have been discarded + }); + + it('updateDataset with error before previous update is done', async () => { + const pipeline: Pipeline = [ + { name: 'domain', domain: 'GoT' }, + { name: 'replace', searchColumn: 'characters', toReplace: [['Snow', 'Targaryen']] }, + { name: 'sort', columns: [{ column: 'death', order: 'asc' }] }, + ]; + const executePipelineMock = vi.fn(); + const store = setupMockStore({ + ...buildStateWithOnePipeline(pipeline), + backendService: { + executePipeline: executePipelineMock, + }, + }); + + executePipelineMock.mockImplementation(() => { + let _res, _rej; + const promise = new Promise((res, rej) => { + _res = res; + _rej = rej; + }); + promise._resolve = _res; + promise._reject = _rej; + return promise; + }); + + const updateDataset1 = store.updateDataset(); + expect(store.isLoading.dataset).toBe(true); + const executePipeline1 = executePipelineMock.mock.results[0].value; + + // Ask for a second preview + const updateDataset2 = store.updateDataset(); + const executePipeline2 = executePipelineMock.mock.results[1].value; + expect(store.isLoading.dataset).toBe(true); + + // The second preview arrives + const dummyDataset: DataSet = { + headers: [{ name: 'x' }, { name: 'y' }], + data: [ + [1, 2], + [3, 4], + ], + }; + executePipeline2._resolve({ data: dummyDataset }); + await updateDataset2; + expect(store.isLoading.dataset).toBe(false); + expect(store.dataset.data).toEqual(dummyDataset.data); + + // Then the first one errors, late + executePipeline1._reject({ error: 'Something wrong!' }); + await updateDataset1; + expect(store.isLoading.dataset).toBe(false); + expect(store.dataset.data).toEqual(dummyDataset.data); // outdated error should have been discarded + }); + describe('loadColumnUniqueValues', () => { const dummyDataset: DataSet = { headers: [