fix(#799): batch contact IDs in move-contacts to avoid URI length errors#802
fix(#799): batch contact IDs in move-contacts to avoid URI length errors#802
Conversation
sugat009
left a comment
sugat009
left a comment
There was a problem hiding this comment.
The ID batching approach is sound for avoiding the URI length issue, but there are a few concerns with the current implementation.
| return res.rows.map(row => row.doc); | ||
| }; | ||
|
|
||
| const fetchReportsBatch = async (db, idsBatch, useNouveau, skip) => { |
There was a problem hiding this comment.
issue (bug): The skip parameter from the outer pagination loop in updateReports (index.js:78-92) is passed identically to every sub-batch here. This means on iteration 2 of the outer loop (skip=10000), each of the sub-batch queries independently skips 10,000 rows from its own small result set, likely returning 0 results. The outer loop then terminates early, silently missing reports.
The inner ID-batching loop and the outer report-pagination loop serve different purposes. The inner loop should always fetch all reports for its ID batch. The outer skip should not leak into it.
Also, allResults accumulates across all sub-batches, so the total can exceed BATCH_SIZE, which breaks the outer loop's termination condition (reportDocsBatch.length >= BATCH_SIZE).
|
|
||
| const fetchReportsBatch = async (db, idsBatch, useNouveau, skip) => { | ||
| if (useNouveau) { | ||
| return api().getReportsByCreatedByIds(idsBatch, BATCH_SIZE, skip); |
There was a problem hiding this comment.
suggestion (improvement): The Nouveau endpoint uses POST with the query in the request body (see cht-core/shared-libs/cht-datasource/src/local/libs/nouveau.ts), so there is no URI length constraint on this path. The URI-too-large issue from #799 only affects the CouchDB view path (CHT < 5.0.0).
Nouveau also uses bookmark-based pagination rather than skip, so the batching of contact IDs is unnecessary here. Consider only applying the ID batching to the CouchDB view branch.
| const createdByKeys = createdByIds.map(id => [`contact:${id}`]); | ||
| return await getFromDbView(db, 'medic-client/reports_by_freetext', createdByKeys, skip); | ||
| return allResults; | ||
| }; |
There was a problem hiding this comment.
question (consistency): fetchReportsBySubject (just below) passes all IDs directly to getFromDbView without batching. If the contact has many subject IDs, this has the same URI length vulnerability that this PR is fixing. Should this be batched as well for consistency?
…e POST request and update request body structure
2 participants
Description
Split IDs into batches of 100 per request
#799
Code review items
AI disclosure
This PR was developed with assistance from Claude Code. AI was used for:
fetchReportsByCreatorLicense
The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.