Refactored code in controllers/bulk.js

[Mehulantony]

Included 3 helper functions to reduce duplications and improve maintainability:

1. setJsonHeaders - Standardizes JSON response headers by centralizing the "Content-Type" setting. Ensures consistent formatting in bulkCreate and bulkUpdate
2. requireNonEmptyArrayBody - Ensures the body is a non-empty array
3. isValidJsonObject - Ensures each item is a valid non-array JSON object.

[joeljoby02]

• The new commits are well structured.
• The changes introduced here improve correctness and robustness, especially around input validation.
• One area to consider for future improvement is reducing duplicated validation logic across bulk operations. While the current implementation is clear, extracting shared checks into a common helper would improve maintainability and reduce the risk of inconsistencies as additional bulk features are added.

[joeljoby02]

Could further enhance the error message by pointing to which object caused the error through indexing or any other valid method

[thehabes]

This looks ok. I update the jwt package and was able to confirm that /bulkCreate and /bulkUpdate are functional.

---

Static Review noted a couple items worth cleaning up before merging if you decide those changes are in scope. You can always make an issue and come back to them later.

Static Review Comments

Branch: refactor
Review Date: 2026-03-30
Reviewer: Pair Static Review - Claude & @thehabes

Claude and Bryan make mistakes. Verify all issues and suggestions. Avoid unnecessary scope creep.

Summary

Category	Issues Found
🔴 Critical	0
🟠 Major	1
🟡 Minor	1
:blue_circle: Suggestions	0

---

Critical Issues 🔴

None.

---

Major Issues 🟠

Issue 1: Inconsistent gatekeep filter in bulkCreate vs bulkUpdate — falsy value bug

File: controllers/bulk.js:56-63
Category: Logic Error

Problem:
The bulkUpdate gatekeep filter was properly refactored to return explicit booleans (return true / return false), but bulkCreate still uses the old pattern of return d with an implicit undefined fallthrough. This creates a subtle bug: if d is a falsy value (null, 0, "", false), return d evaluates as falsy, causing .filter() to exclude the invalid item from the gatekeep array. That item then silently passes validation.

Notably, the new isValidJsonObject now correctly catches null (which the original code didn't, since typeof null === "object"), but the return d on the very next line returns null — which is falsy — defeating the purpose of the check.

Current Code (bulkCreate):

const gatekeep = documents.filter(d=> {
    // Each item must be valid JSON, but can't be an array.
    if (!isValidJsonObject(d)) return d      // BUG: if d is null/0/""/false, returns falsy -> filter excludes it

    // Items must not have an @id, and in some cases same for id.
    const idcheck = _contextid(d["@context"]) ? (d.id ?? d["@id"]) : d["@id"]
    if(idcheck) return d
})                                           // implicit undefined return for valid items -- works, but fragile

Refactored Code (bulkUpdate — correct):

const gatekeep = documents.filter(d => {
    if (!isValidJsonObject(d)) return true
    const idcheck = _contextid(d["@context"]) ? (d.id ?? d["@id"]) : d["@id"]
    if (!idcheck) return true   // Reject items WITHOUT an id (updates need an id)
    return false
})

Suggested Fix — match the bulkUpdate pattern:

const gatekeep = documents.filter(d=> {
    // Each item must be valid JSON, but can't be an array.
    if (!isValidJsonObject(d)) return true

    // Items must not have an @id, and in some cases same for id.
    const idcheck = _contextid(d["@context"]) ? (d.id ?? d["@id"]) : d["@id"]
    if(idcheck) return true    // Reject items WITH an id (creates must not have one)
    return false
})

How to Verify:

1. Create a test request body containing null: [null, {"key": "value"}]
2. POST to /v1/api/bulkCreate — should return 400 with the "must be JSON" error
3. With the current code, null passes the gatekeep filter and crashes later at Object.keys(d) inside the for loop

---

Minor Issues 🟡

Issue 2: Redundant Content-Type header set after setJsonHeaders()

File: controllers/bulk.js:101 and controllers/bulk.js:193
Category: Dead Code / Redundancy

Problem:
Both bulkCreate (line 101) and bulkUpdate (line 193) call res.set("Content-Type", "application/json; charset=utf-8") again in their try blocks, even though setJsonHeaders(res) already set this header at the top of each function. The whole point of extracting setJsonHeaders was to centralize this — the original inline calls should have been removed.

Current Code (line 101 in bulkCreate):

setJsonHeaders(res)                                          // line 47
// ... validation logic ...
try {
    let dbResponse = await db.bulkWrite(bulkOps, {'ordered':false})
    res.set("Content-Type", "application/json; charset=utf-8")   // line 101 -- redundant

Suggested Fix: Remove the duplicate res.set(...) lines at 101 and 193.

---

If there are significant code changes in response to this review please test those changes. Run the application manually and test or perform internal application tests when applicable.

[thehabes]

Confirmed functional and the code is looking good. Static review found some lint things that do not block the pr.

Static Review Comments

Branch: refactor
Review Date: 2026-04-07
Reviewer: Pair Static Review - Claude & @thehabes

Claude and Bryan make mistakes. Verify all issues and suggestions. Avoid unnecessary scope creep.

Category	Issues Found
🔴 Critical	0
🟠 Major	0
🟡 Minor	2
🔵 Suggestions	0

---

Critical Issues 🔴

None.

---

Major Issues 🟠

None.

---

Minor Issues 🟡

Issue 1: Double blank lines between helper functions

File: controllers/bulk.js:20-22 and controllers/bulk.js:33-35
Category: Code Hygiene

Problem:
There are double blank lines between setJsonHeaders, requireNonEmptyArrayBody, and isValidJsonObject. The rest of the codebase uses single blank lines between function definitions.

How to Verify:
Remove the extra blank lines between the three helper functions.

---

Issue 2: Unrelated whitespace change in bin/rerum_v1.js

File: bin/rerum_v1.js:19
Category: Non-functional Change

Problem:
A blank line was removed between app.set('port', port) and the JSDoc comment block. This whitespace change is unrelated to the bulk.js refactoring and adds noise to the diff. The previous review flagged this same issue.

Suggested Fix:
Revert the whitespace change in bin/rerum_v1.js so it has no diff. Adding the trailing newline at EOF is fine and can stay.

---

Suggestions 🔵

None.

---

If there are significant code changes in response to this review please test those changes. Run the application manually and test or perform internal application tests when applicable.