Skip to content

Commit

Permalink
fix(ui): only result of the last request should be used TCTC-8992 (#2204
Browse files Browse the repository at this point in the history
)

* 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
  • Loading branch information
davinov authored Aug 19, 2024
1 parent 3ad1e06 commit e2d6ff3
Show file tree
Hide file tree
Showing 3 changed files with 141 additions and 4 deletions.
26 changes: 22 additions & 4 deletions ui/src/store/actions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -250,6 +250,7 @@ const actions: PiniaActionAdaptor<VQBActions, VQBStore> = {
},
async updateDataset(): Promise<BackendResponse<DataSet> | undefined> {
this.logBackendMessages({ backendMessages: [] });
let requestPromise;
try {
this.setLoading({ type: 'dataset', isLoading: true });
this.toggleRequestOnGoing({ isRequestOnGoing: true });
Expand All @@ -264,13 +265,18 @@ const actions: PiniaActionAdaptor<VQBActions, VQBStore> = {
});
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 || [];
Expand All @@ -280,8 +286,19 @@ const actions: PiniaActionAdaptor<VQBActions, VQBStore> = {
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
Expand All @@ -293,11 +310,12 @@ const actions: PiniaActionAdaptor<VQBActions, VQBStore> = {
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
Expand Down
2 changes: 2 additions & 0 deletions ui/src/store/state.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ export interface VQBState {
// necessary methods to preview pipeline results and fetch available data collections
backendService: BackendService;
isRequestOnGoing: boolean;
ongoingRequestPromise: Promise<any> | undefined;

isLoading: {
dataset: boolean;
Expand Down Expand Up @@ -99,6 +100,7 @@ export function emptyState(): VQBState {
backendMessages: [],
isLoading: { dataset: false, uniqueValues: false },
isRequestOnGoing: false,
ongoingRequestPromise: undefined,
availableVariables: [],
variables: {},
variableDelimiters: undefined,
Expand Down
117 changes: 117 additions & 0 deletions ui/tests/unit/store.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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(() => {
Expand Down Expand Up @@ -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: [
Expand Down

0 comments on commit e2d6ff3

Please sign in to comment.