feat(#690): add individual attachment upload for forms with large attachments #724

sookwalinga · 2025-07-22T10:19:05Z

Closes #690

Is your feature request related to a problem? Please describe.
The CHT-Api has a hard limit of 32MB allowance per received JSON request bodies: https://github.com/medic/cht-core/blob/5c1f9ba4a149041dd2a16e58420546731cd27b25/api/src/routing.js#L91

This means that cht-conf form upload requests: https://github.com/medic/cht-conf/blob/main/src/lib/insert-or-replace.js will also be subjected to this limitation. Which blocks uploading any form that has a total size of attachments larger than 32MB.

Describe the solution you'd like
Add a fallback in the insert-or-replace function to check the response code from API. If the response code is that the request is too large, the code should attempt to upload the doc body and each individual attachment separately. When uploading attachments, the attachment real mime type should be used as content-type in order to overcome the limitation in API.

Describe alternatives you've considered
None.

Additional context

dianabarsan

Nice start! I left some recommendations below

dianabarsan · 2025-07-23T10:54:04Z

src/lib/insert-or-replace.js

-    .then(() => db.put(doc));
+const mime = require('mime-types');
+
+const upsertDoc = async (db, doc, maxRetries = 3, retryDelay = 100) => {


The retry model that I like the most is where there are no iterations inside the funciton:

const MAX_RETRY = 10; const retryable = (retry = MAX_RETRY) => { // do stuff if (failed && retry) { retryable(--retry); } }

Can you convert the function to follow this model?

dianabarsan · 2025-07-23T10:54:31Z

src/lib/insert-or-replace.js

+        const existingDoc = await db.get(doc._id);
+        // Creating a copy to avoid mutating the original doc
+        const docToSave = { ...doc, _rev: existingDoc._rev };
+        const result = await db.put(docToSave);
+        return result;
+      } catch (e) {
+        if (e.status === 404) {
+          // Document doesn't exist, try to create it
+          const docToSave = { ...doc };
+          delete docToSave._rev; // Remove _rev for new documents
+          const result = await db.put(docToSave);
+          return result;
+        } else {
+          throw e;
+        }


I think this block should be its own function.

dianabarsan · 2025-07-23T10:56:04Z

src/lib/insert-or-replace.js

+        const docToSave = { ...doc, _rev: existingDoc._rev };
+        const result = await db.put(docToSave);
+        return result;
+      } catch (e) {


This catch will end up catching errors from both the get and the put.
can you please separate them?

maybe create a function getDoc that handles the get an errors from the get and a function putDoc that handles conflicts?

dianabarsan · 2025-07-23T10:56:29Z

src/lib/insert-or-replace.js

+
+        console.log(`Conflict detected, retrying attempt ${attempts}/${maxRetries}`);
+        // Wait before retrying (exponential backoff)
+        await new Promise(resolve => setTimeout(resolve, retryDelay * Math.pow(2, attempts - 1)));


I don't think we should add a delay, just retry immediately.

dianabarsan · 2025-07-23T10:57:24Z

src/lib/insert-or-replace.js

+  }
+};
+
+const handleLargeDocument = async (db, doc, maxRetries = 5, retryDelay = 200) => {


Same as above, can you please rewrite this using the reducing retry parameter?

dianabarsan · 2025-07-23T11:02:21Z

src/lib/insert-or-replace.js

+
+      // Try to save the document without attachments
+      console.log(`Attempting to save ${doc._id} (attempt ${attempts + 1}/${maxRetries})`);
+      const res = await db.put(docWithoutAttachments);


You can reuse the putDoc function from the comment above.

dianabarsan · 2025-07-23T11:02:58Z

src/lib/insert-or-replace.js

+      console.log(`Successfully saved ${doc._id}, new rev: ${res.rev}`);
+
+      // Handle attachments one by one with individual error handling
+      if (attachments && Object.keys(attachments).length > 0) {


Please extract this into a separate function

dianabarsan · 2025-07-23T11:04:19Z

src/lib/insert-or-replace.js

+              const att = attachments[attachmentName];
+              const contentType = att.content_type || mime.lookup(attachmentName) || 'application/octet-stream';
+              const attachResult = await db.putAttachment(doc._id, attachmentName, currentDocRev, 
+                att.data, contentType);
+              currentDocRev = attachResult.rev;


I believe this should also be a retryable addDocAttachment function.

dianabarsan · 2025-07-23T11:04:33Z

src/lib/insert-or-replace.js

+
+        console.log(`Large document conflict detected for ${doc._id}, retrying attempt ${attempts}/${maxRetries}`);
+        // Increase delay progressively
+        const delay = retryDelay * Math.pow(2, attempts - 1) + Math.random() * 100; // Add jitter


I don't believe we should use a delay.

dianabarsan · 2025-07-23T11:06:26Z

src/lib/insert-or-replace.js

+  }
+};
+
+const forceSaveDocument = async (db, doc) => {


As far as I can tell this is not used. What is the purpose of this?

…and new documents without _rev

dianabarsan

Very very nice! Huge improvements! Thank you!

I had just reviewed Nitu's PR for the serverside and I realised that uploading a document without any attachments - and then uploading only the xml - will always force the server to regenerate the model.xml and form.html attachments. This adds extra load and will generate additional conflicts for your code.
So ... I want to ask you if you could try, instead of removing all attachments, only remove the media attachments and keep the functional ones.
I appreciate it!

dianabarsan · 2025-08-12T07:12:17Z

src/lib/insert-or-replace.js

+      if (!latestDoc) {
+        throw new Error(`Document ${docId} not found during attachment retry`);
      }


To reduce the bloat in this catch, you could move this outside of the try/catch, right after you check for retries.

Alright thanks, let me work on these

dianabarsan · 2025-08-12T07:16:24Z

src/lib/insert-or-replace.js

+  try {
+    const attachments = doc._attachments;
+    const docWithoutAttachments = { ...doc };
+    delete docWithoutAttachments._attachments;


To save roundabouts on generating attachments on the server, can you please try to preserve the non-media attachments here? These would be the form xml, model.xml and form.html.
Can you please try this and check how it works server-side? We would ideally not see forms getting regenerated when a new media attachment is added.

… update tests for uploading large attachments

dianabarsan · 2025-08-26T08:58:54Z

src/lib/insert-or-replace.js

+  try {
+    const attachments = doc._attachments || {};
+    // Regex for specific functional files: form.xml, model.xml, form.html
+    const functionalRegex = /^(form|model)\.xml$|^form\.html$/i;


I believe that the attachment that contains the main form xml would just be called xml. Is there a way that we can include that in this capture regex?

dianabarsan · 2025-08-26T09:03:10Z

src/lib/insert-or-replace.js

+    const docToSave = { ...doc, _attachments: functionalAttachments };
+    if (Object.keys(docToSave._attachments).length === 0) {
+      delete docToSave._attachments; 
+    }


This looks a little strange to me, where we add this property only to delete it in a validation later. I think it would make more sense to a quick read if we add it conditionally:

Suggested change

}

const docToSave = {

...doc,

_attachments: Object.keys(functionalAttachments).length ? functionalAttachments : undefined

};

dianabarsan · 2025-08-26T09:05:42Z

src/lib/insert-or-replace.js

+    }
+
+    const existingDoc = await getDoc(db, doc._id);
+    log.info(


I believe these logs will get very very noisy if the users upload all forms. I suggest we change most of these to be debug logs.

dianabarsan · 2025-08-26T09:24:36Z

src/lib/insert-or-replace.js

+      log.info(`Conflict detected for ${doc._id}, retrying`);
+      return upsertDoc(db, doc, retries - 1);
+    } else if (err.status === 413) {
+      return handleLargeDocument(db, doc);


I have a form with three video attachments, and every time I upload it, I am being told that I need to overwrite changes from the server, even though no changes have been done to the form in config.

However I'm seeing the same behavior when adding media to another form - media that would not trigger the custom code to handle large attachments, and I'm also seeing this on the main branch.

Would you mind creating an issue to track this bug? The warning should only show up when the config and the server are out of sync.

…ex, conditional _attachments assignment, and adjust log levels

dianabarsan

Your PR is not passing our Sonar linting rules. Could you please have a look and address these issues? https://sonarcloud.io/project/issues?id=medic_cht-conf&pullRequest=724&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true

Thanks!

…t uploads

…ling in attachment functions

…chments function

… reduce its cognitive complexity

dianabarsan

Looking good!!

mrjones-plip · 2025-10-01T18:36:58Z

awesome job - thanks for the review @dianabarsan and congrats to @sookwalinga on contributing the code which will now be in CHT Core \o/

Diana - are we ready to merge this then? Any reason to wait?

dianabarsan · 2025-10-02T10:22:20Z

Since there are a couple of connected PRs, this one and two in cht-core, I want to make sure than they work well together. It's a manual test.

I'll merge after testing the full interaction.

feat: add individual attachment upload for forms with large attachments

1d3cbb8

sookwalinga marked this pull request as ready for review July 22, 2025 10:19

dianabarsan self-requested a review July 23, 2025 05:32

dianabarsan requested changes Jul 23, 2025

View reviewed changes

sookwalinga added 2 commits August 7, 2025 20:17

Merge branch 'main' into 690-forms-with-large-attachments

552c0c2

refactor: implement recommendations and add unit test for creating br…

9c7638e

…and new documents without _rev

sookwalinga requested a review from dianabarsan August 12, 2025 03:39

dianabarsan reviewed Aug 12, 2025

View reviewed changes

sookwalinga added 2 commits August 13, 2025 16:08

Merge branch 'main' into 690-forms-with-large-attachments

ec724e2

refactor: preserve non-media attachments, optimize error handling and…

af912c5

… update tests for uploading large attachments

sookwalinga requested a review from dianabarsan August 20, 2025 12:32

dianabarsan reviewed Aug 26, 2025

View reviewed changes

dianabarsan mentioned this pull request Aug 27, 2025

fix(#10132): update how api handles form updates on change medic/cht-core#10171

Closed

refactor(insert-or-replace & unit tests): improve functional file reg…

25d126e

…ex, conditional _attachments assignment, and adjust log levels

sookwalinga requested a review from dianabarsan September 3, 2025 04:53

dianabarsan reviewed Sep 3, 2025

View reviewed changes

sookwalinga added 7 commits September 30, 2025 17:42

refactor: resolve sonarlint issues and update tests for large documen…

c840b37

…t uploads

refactor: remove redundant input validation and streamline error hand…

1e54bd5

…ling in attachment functions

refactor: reduce cognitive complexity of addDocAttachment function

b65f4fe

refactor: simplify addDocAttachment retry logic and enhance splitAtta…

ce42669

…chments function

refactor: reduce cognitive complexity

6cb405e

refactor: reduce cognitive complexity

155fabe

refactor: simplify addDocAttachment error handling and retry logic to…

abdc699

… reduce its cognitive complexity

sookwalinga requested a review from dianabarsan September 30, 2025 16:48

dianabarsan approved these changes Oct 1, 2025

View reviewed changes

feat(#690): add individual attachment upload for forms with large attachments #724

Are you sure you want to change the base?

feat(#690): add individual attachment upload for forms with large attachments #724

Uh oh!

Conversation

sookwalinga commented Jul 22, 2025 • edited by andrablaj Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dianabarsan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dianabarsan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dianabarsan left a comment

Choose a reason for hiding this comment

Uh oh!

dianabarsan left a comment

Choose a reason for hiding this comment

Uh oh!

mrjones-plip commented Oct 1, 2025

Uh oh!

dianabarsan commented Oct 2, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sookwalinga commented Jul 22, 2025 •

edited by andrablaj

Loading