Skip to content

Commit

Permalink
Fixing form data locking race condition (#3031)
Browse files Browse the repository at this point in the history 
Co-authored-by: Ole Martin Handeland <git@olemartin.org>
  • Loading branch information
@olemartinorg
olemartinorg and Ole Martin Handeland authored Feb 25, 2025
1 parent 324aece commit 293eb92
Show file tree
Hide file tree
Showing 9 changed files with 261 additions and 119 deletions.
42 changes: 20 additions & 22 deletions src/features/attachments/AttachmentsStorePlugin.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -289,7 +289,7 @@ export class AttachmentsStorePlugin extends NodeDataPlugin<AttachmentsStorePlugi
const supportsNewAttachmentAPI = appSupportsNewAttachmentAPI(applicationMetadata);

const setAttachmentsInDataModel = useSetAttachmentInDataModel();
const { lock, unlock } = FD.useLocking('__attachment__upload__');
const lock = FD.useLocking('__attachment__upload__');

return useCallback(
async (action) => {
Expand All @@ -300,33 +300,32 @@ export class AttachmentsStorePlugin extends NodeDataPlugin<AttachmentsStorePlugi
upload(fullAction);

if (supportsNewAttachmentAPI) {
await lock();
const { unlock } = await lock();
const results: AttachmentUploadResult[] = [];

const updatedData: FDActionResult = { updatedDataModels: {}, updatedValidationIssues: {} };

for (const { file, temporaryId } of fullAction.files) {
const { baseComponentId } = splitDashedKey(action.nodeId);
await uploadAttachment({
dataTypeId: baseComponentId,
file,
})
.then((reply) => {
results.push({ temporaryId, newDataElementId: reply.newDataElementId });

updatedData.instance = reply.instance;
updatedData.updatedDataModels = {
...updatedData.updatedDataModels,
...dataModelPairsToObject(reply.newDataModels),
};
updatedData.updatedValidationIssues = {
...updatedData.updatedValidationIssues,
...backendValidationIssueGroupListToObject(reply.validationIssues),
};
})
.catch((error) => {
results.push({ temporaryId, error });
try {
const reply = await uploadAttachment({
dataTypeId: baseComponentId,
file,
});
results.push({ temporaryId, newDataElementId: reply.newDataElementId });

updatedData.instance = reply.instance;
updatedData.updatedDataModels = {
...updatedData.updatedDataModels,
...dataModelPairsToObject(reply.newDataModels),
};
updatedData.updatedValidationIssues = {
...updatedData.updatedValidationIssues,
...backendValidationIssueGroupListToObject(reply.validationIssues),
};
} catch (error) {
results.push({ temporaryId, error });
}
}
setAttachmentsInDataModel(
results.filter(isAttachmentUploadSuccess).map(({ newDataElementId }) => newDataElementId),
Expand Down Expand Up @@ -365,7 +364,6 @@ export class AttachmentsStorePlugin extends NodeDataPlugin<AttachmentsStorePlugi
lock,
setAttachmentsInDataModel,
uploadFinished,
unlock,
uploadAttachment,
appendDataElements,
uploadAttachmentOld,
Expand Down
8 changes: 4 additions & 4 deletions src/features/devtools/components/DownloadXMLButton/DownloadXMLButton.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ const InnerDownloadXMLButton = () => {
const [selectedDataType, setSelectedDataType] = useState(writableDataTypes?.at(0));
const disabled = !selectedDataType;

const { lock, unlock } = FD.useLocking('__dev_tools__');
const lock = FD.useLocking('__dev_tools__');

const downloadXML = async () => {
const dataElementId = selectedDataType ? getDataElementIdForDataType(selectedDataType) : undefined;
Expand All @@ -51,8 +51,8 @@ const InnerDownloadXMLButton = () => {
const dataElementId = selectedDataType ? getDataElementIdForDataType(selectedDataType) : undefined;
const dataUrl = dataElementId && instanceId ? getStatefulDataModelUrl(instanceId, dataElementId, true) : undefined;
if (dataUrl && acceptedFiles.length) {
const currentLock = await lock();
try {
lock();
const dataToUpload = await acceptedFiles[0].text();
await axios.put(dataUrl, dataToUpload, { headers: { 'Content-Type': 'application/xml' } }).catch((error) => {
// 303 is expected when using ProcessDataWrite and can be ignored
Expand All @@ -63,9 +63,9 @@ const InnerDownloadXMLButton = () => {
const { data: updatedDataModel } = await axios.get(dataUrl, {
headers: { Accept: 'application/json' },
});
unlock({ updatedDataModels: { [dataElementId!]: updatedDataModel }, updatedValidationIssues: {} });
currentLock.unlock({ updatedDataModels: { [dataElementId!]: updatedDataModel }, updatedValidationIssues: {} });
} catch {
unlock();
currentLock.unlock();
}
}
};
Expand Down
142 changes: 121 additions & 21 deletions src/features/formData/FormData.test.tsx
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React from 'react';
import React, { useState } from 'react';
import { MemoryRouter, Route, Routes } from 'react-router-dom';
import type { PropsWithChildren } from 'react';

Expand Down Expand Up @@ -333,27 +333,49 @@ describe('FormData', () => {
}

describe('Locking', () => {
function LockActionButton() {
const { lock, unlock, isLocked } = FD.useLocking('myLockId');
beforeEach(() => {
jest
.spyOn(window, 'logWarn')
.mockImplementation(() => {})
.mockName(`window.logWarn`);
jest
.spyOn(window, 'logError')
.mockImplementation(() => {})
.mockName(`window.logError`);
});

afterEach(() => {
jest.clearAllMocks();
});

function LockActionButton({ lockId, renderInfo }: { lockId: string; renderInfo: boolean }) {
const lock = FD.useLocking(lockId);
const { isLocked, lockedBy } = FD.useLockStatus();
const [currentLock, setCurrentLock] = useState<Awaited<ReturnType<typeof lock>> | undefined>();

return (
<>
<div data-testid='isLocked'>{isLocked ? 'true' : 'false'}</div>
<button
onClick={async () => {
if (isLocked) {
{renderInfo && (
<>
<div data-testid='isLocked'>{isLocked ? 'true' : 'false'}</div>
<div data-testid='lockedBy'>{lockedBy === undefined ? 'undefined' : lockedBy}</div>
</>
)}
<button onClick={async () => setCurrentLock(await lock())}>Lock {lockId}</button>
{currentLock && (
<button
onClick={() => {
// Unlock with some pretend updated form data
unlock({
currentLock?.unlock({
updatedDataModels: { [defaultMockDataElementId]: { obj1: { prop1: 'new value' } } },
updatedValidationIssues: { obj1: [] },
});
} else {
await lock();
}
}}
>
{isLocked ? 'Unlock' : 'Lock'} form data
</button>
setCurrentLock(undefined);
}}
>
Unlock {lockId}
</button>
)}
</>
);
}
Expand All @@ -374,7 +396,14 @@ describe('FormData', () => {
path='obj2.prop1'
dataType={defaultDataTypeMock}
/>
<LockActionButton />
<LockActionButton
lockId='myLockId'
renderInfo={true}
/>
<LockActionButton
lockId='myOtherLockId'
renderInfo={false}
/>
<HasUnsavedChanges />
</>
),
Expand Down Expand Up @@ -405,7 +434,7 @@ describe('FormData', () => {
expect(screen.getByTestId('obj1.prop1')).toHaveValue('value1');

// Lock the form
await user.click(screen.getByRole('button', { name: 'Lock form data' }));
await user.click(screen.getByRole('button', { name: 'Lock myLockId' }));
await waitFor(() => expect(screen.getByTestId('isLocked')).toHaveTextContent('true'));

// Change another value (this will be preserved)
Expand All @@ -418,7 +447,7 @@ describe('FormData', () => {
expect(mutations.doPatchFormData.mock).toHaveBeenCalledTimes(0);

// Unlock the form
await user.click(screen.getByRole('button', { name: 'Unlock form data' }));
await user.click(screen.getByRole('button', { name: 'Unlock myLockId' }));
await waitFor(() => expect(screen.getByTestId('isLocked')).toHaveTextContent('false'));
await waitFor(() => expect(screen.getByTestId('obj1.prop1')).toHaveValue('new value'));
expect(screen.getByTestId('obj1.prop2')).toHaveValue('b');
Expand All @@ -431,21 +460,23 @@ describe('FormData', () => {

const patchReq = (mutations.doPatchFormData.mock as jest.Mock).mock.calls[0][1] as IDataModelPatchRequest;
expect(patchReq.patch).toEqual([{ op: 'add', path: '/obj1/prop2', value: 'b' }]);
expect(window.logError).toHaveBeenCalledTimes(0);
expect(window.logWarn).toHaveBeenCalledTimes(0);
});

it('Locking will not trigger a save if no values have changed', async () => {
const user = userEvent.setup({ delay: null });
const { mutations } = await render();
expect(screen.getAllByTestId(/^obj\d+.prop\d+$/).length).toBe(3);

await user.click(screen.getByRole('button', { name: 'Lock form data' }));
await user.click(screen.getByRole('button', { name: 'Lock myLockId' }));
await waitFor(() => expect(screen.getByTestId('isLocked')).toHaveTextContent('true'));

expect(mutations.doPatchFormData.mock).toHaveBeenCalledTimes(0);
act(() => jest.advanceTimersByTime(5000));
expect(mutations.doPatchFormData.mock).toHaveBeenCalledTimes(0);

await user.click(screen.getByRole('button', { name: 'Unlock form data' }));
await user.click(await screen.findByRole('button', { name: 'Unlock myLockId' }));
await waitFor(() => expect(screen.getByTestId('isLocked')).toHaveTextContent('false'));
await waitFor(() => expect(screen.getByTestId('obj1.prop1')).toHaveValue('new value'));
expect(screen.getByTestId('obj1.prop2')).toHaveValue('');
Expand All @@ -454,6 +485,8 @@ describe('FormData', () => {
act(() => jest.advanceTimersByTime(5000));
await waitFor(() => expect(screen.getByTestId('hasUnsavedChanges')).toHaveTextContent('false'));
expect(mutations.doPatchFormData.mock).toHaveBeenCalledTimes(0);
expect(window.logError).toHaveBeenCalledTimes(0);
expect(window.logWarn).toHaveBeenCalledTimes(0);
});

it('Unsaved changes should be saved before locking', async () => {
Expand All @@ -466,7 +499,7 @@ describe('FormData', () => {
expect(screen.getByTestId('hasUnsavedChanges')).toHaveTextContent('true');

expect(mutations.doPatchFormData.mock).toHaveBeenCalledTimes(0);
await user.click(screen.getByRole('button', { name: 'Lock form data' }));
await user.click(screen.getByRole('button', { name: 'Lock myLockId' }));
await waitFor(() => expect(mutations.doPatchFormData.mock).toHaveBeenCalledTimes(1));
expect(screen.getByTestId('isLocked')).toHaveTextContent('false'); // The save has not finished yet

Expand All @@ -481,6 +514,73 @@ describe('FormData', () => {
};
mutations.doPatchFormData.resolve(response);
await waitFor(() => expect(screen.getByTestId('isLocked')).toHaveTextContent('true'));
expect(window.logError).toHaveBeenCalledTimes(0);
expect(window.logWarn).toHaveBeenCalledTimes(0);
});

it('Locking should queue up when requested multiple times', async () => {
const user = userEvent.setup({ delay: null });
const { mutations } = await render();

expect(mutations.doPatchFormData.mock).toHaveBeenCalledTimes(0);
await user.click(screen.getByRole('button', { name: 'Lock myLockId' }));
await waitFor(() => expect(screen.getByTestId('isLocked')).toHaveTextContent('true'));
expect(screen.getByTestId('lockedBy')).toHaveTextContent('myLockId');
expect(mutations.doPatchFormData.mock).toHaveBeenCalledTimes(0);
expect(window.logError).toHaveBeenCalledTimes(0);
expect(window.logWarn).toHaveBeenCalledTimes(0);

// Try to lock another lock id (will wait for the first lock to finish)
await user.click(screen.getByRole('button', { name: 'Lock myOtherLockId' }));
await waitFor(() => expect(screen.getByTestId('isLocked')).toHaveTextContent('true'));
await screen.findByRole('button', { name: 'Unlock myLockId' });
expect(screen.getByTestId('lockedBy')).toHaveTextContent('myLockId');

// The other lock id will be locked after the first one is unlocked, so it is still not acquired
act(() => jest.advanceTimersByTime(5000));
expect(screen.queryByRole('button', { name: 'Unlock myOtherLockId' })).not.toBeInTheDocument();

await user.click(screen.getByRole('button', { name: 'Unlock myLockId' }));
await waitFor(() => expect(screen.getByTestId('lockedBy')).toHaveTextContent('myOtherLockId'));

await screen.findByRole('button', { name: 'Unlock myOtherLockId' });
await user.click(screen.getByRole('button', { name: 'Unlock myOtherLockId' }));
await waitFor(() => expect(screen.getByTestId('lockedBy')).toHaveTextContent('undefined'));
expect(screen.getByTestId('isLocked')).toHaveTextContent('false');
});

it('When multiple locks are scheduled, form data should be saved in between locks', async () => {
const user = userEvent.setup({ delay: null });
const { mutations } = await render();

await user.click(screen.getByRole('button', { name: 'Lock myLockId' }));
await waitFor(() => expect(screen.getByTestId('isLocked')).toHaveTextContent('true'));
expect(mutations.doPatchFormData.mock).toHaveBeenCalledTimes(0);

// Schedule another lock that will be scheduled after the first one
await user.click(screen.getByRole('button', { name: 'Lock myOtherLockId' }));
expect(screen.getByTestId('lockedBy')).toHaveTextContent('myLockId');

// Change a value
await user.type(screen.getByTestId('obj2.prop1'), 'a');

// Unlock the first lock
await user.click(screen.getByRole('button', { name: 'Unlock myLockId' }));
await waitFor(() => expect(mutations.doPatchFormData.mock).toHaveBeenCalledTimes(1));

const response: IDataModelPatchResponse = {
newDataModel: {
obj2: { prop1: 'a' },
},
validationIssues: {},
};
mutations.doPatchFormData.resolve(response);

await waitFor(() => expect(screen.getByTestId('lockedBy')).toHaveTextContent('myOtherLockId'));
await waitFor(() => expect(screen.getByTestId('isLocked')).toHaveTextContent('true'));
await user.click(screen.getByRole('button', { name: 'Unlock myOtherLockId' }));
await waitFor(() => expect(screen.getByTestId('lockedBy')).toHaveTextContent('undefined'));
expect(mutations.doPatchFormData.mock).toHaveBeenCalledTimes(1);
});
});

Expand Down
Loading

0 comments on commit 293eb92

Please sign in to comment.