Altinn / Altinn/app-frontend-react

Investigate data locking when uploading attachments

Open
#3,854 0 comments 0 reactions 0 assignees View on GitHub
status/triage
Dominant language
TypeScript
Stars
21
Forks
33
Avg merge
6d 23h
Merged PRs (30d)
3

Description

In https://github.com/Altinn/altinn-studio/pull/16966#discussion_r2524408553, coderabbit noticed something that worries me. We should investigate this and perhaps add a cypress test for verifying that locks are always released when uploading fails:

If anything between acquiring the lock and the final `unlock(updatedData)` throws (for example `setAttachmentsInDataModel` or `uploadFinished`), the function exits early and the FD lock stays engaged. Once that happens every subsequent form-data mutation will queue behind an unlock that never comes, effectively bricking uploads for the rest of the session. Please wrap the body in a `try/finally` so we always release the lock.

```diff
- const { unlock } = await lock();
- const results: AttachmentUploadResult[] = [];
-
- const updatedData: FDActionResult = { updatedDataModels: {}, updatedValidationIssues: {} };
-
- for (const { file, temporaryId } of fullAction.files) {
- const { baseComponentId } = splitDashedKey(action.nodeId);
- 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),
- action.dataModelBindings,
- );
- uploadFinished(fullAction, results);
- unlock(updatedData);
+ const { unlock } = await lock();
+ const results: AttachmentUploadResult[] = [];
+
+ const updatedData: FDActionResult = { updatedDataModels: {}, updatedValidationIssues: {} };
+
+ try {
+ for (const { file, temporaryId } of fullAction.files) {
+ const { baseComponentId } = splitDashedKey(action.nodeId);
+ 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),
+ action.dataModelBindings,
+ );
+ uploadFinished(fullAction, results);
+ } finally {
+ unlock(updatedData);
+ }
```

Contributor guide

Open the contributing guide

Research direction

Start from the attachment-upload path and the lock(), setAttachmentsInDataModel(), uploadFinished(), and unlock() calls discussed in PR #16966. Verify failure paths release the lock, then add a Cypress test if the existing test structure supports it; done means later form-data mutations are not blocked after an upload failure.

Written by the indexing model from the issue text.

Assessment

Tech stack
cypress, typescript
Domain
frontend, testing
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
52/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.