Skip to content

Refactored utils.js#53

Open
joeljoby02 wants to merge 2 commits intomainfrom
refactor_utils
Open

Refactored utils.js#53
joeljoby02 wants to merge 2 commits intomainfrom
refactor_utils

Conversation

@joeljoby02
Copy link
Collaborator

Split utils.js into three focused modules:

  • versioning.jsconfigureRerumOptions() and history/release logic.
  • predicates.js – boolean checks (isDeleted, isReleased, etc.).
  • headers.js – response header constructors.

utils.js now re‑exports the contents of the new files, preserving existing
imports.

Updated a couple of controllers (crud.js, delete.js) to import only the
functions they need.

Copy link
Collaborator

@thehabes thehabes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like even I have to go look at my rerum, geez.

Please fix the bug you inherited as noted by Critical Issue 1, and I will do the same upstream. Not your fault, but this change is required.

I found the Minor issues and Suggestions to be strong this time. It is worth listening to them, and I strongly suggest making some of those changes now. Those utilities are used in more places, so you may have a bigger refactor on your hands than you realize. Decide how much is in scope.

Static Review Comments

Branch: refactor_utils
Review Date: 2026-03-09
Reviewer: Pair Static Review - Claude & @thehabes

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

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

Critical Issues 🔴

Issue 1: Operator precedence bug in configureLastModifiedHeaderisOverwritten is never used

File: headers.js:60
Category: Logic Error (pre-existing, carried forward)

Problem:
The ! (logical NOT) operator has higher precedence than ===. So !obj.__rerum.isOverwritten === "" evaluates as (!obj.__rerum.isOverwritten) === "", which compares a boolean to a string — this is always false. As a result, the isOverwritten date is never used and the function always falls through to createdAt.

This bug existed in the original utils.js, but since this code is being extracted into a fresh file, now is the ideal time to fix it. Notifying @thehabes to check for this in the upstream RERUM as well.

Current Code:

if(!obj.__rerum.isOverwritten === ""){
    date = obj.__rerum.isOverwritten
}

Suggested Fix:

if(obj.__rerum.isOverwritten !== ""){
    date = obj.__rerum.isOverwritten
}

How to Verify:

  1. Create an object, then overwrite it (POST /v1/api/overwrite) — the object's __rerum.isOverwritten will be set to a date string
  2. GET /v1/id/{id} for the overwritten object
  3. Inspect the Last-Modified response header — it should reflect the overwrite timestamp, not the original createdAt

Major Issues 🟠

None.


Minor Issues 🟡

Issue 2: Unused import in controllers/utils.js

File: controllers/utils.js:8
Category: Dead Code

Problem:
This import is dead code. It was likely used historically but is no longer needed.

Suggested Fix:
Remove the line entirely.

Issue 3: Missing newline at end of file in all new modules

Files: headers.js:88, predicates.js:85, versioning.js:102, utils.js:21
Category: Code Hygiene

Problem:
All four files end without a trailing newline. Git flags this with \ No newline at end of file. POSIX standard expects text files to end with a newline, and most editors/linters flag this.

Suggested Fix:
Add a trailing newline to each file.


Suggestions 🔵

Suggestion 1: Consider named exports only (drop redundant default exports)

Files: headers.js, predicates.js, versioning.js

Each new module exports both named exports and a default export object containing the same functions. The utils.js re-export shim uses the default exports (via spread), and direct consumers (crud.js, delete.js) use named exports.

You could simplify by using only named exports and adjusting utils.js to import named:

// utils.js
import { configureRerumOptions } from './versioning.js'
import { configureWebAnnoHeadersFor, configureLDHeadersFor, configureLastModifiedHeader } from './headers.js'
import { isDeleted, isReleased, isGenerator, isContainerType, isLD } from './predicates.js'

export default {
    configureRerumOptions,
    configureWebAnnoHeadersFor,
    configureLDHeadersFor,
    configureLastModifiedHeader,
    isDeleted,
    isReleased,
    isGenerator,
    isContainerType,
    isLD
}

This removes the dual-export pattern and makes each module's public API explicit.


Suggestion 2: utils.js should also provide named re-exports

File: utils.js

Currently utils.js only has a default export. If any future consumer wants to do import { isDeleted } from '../utils.js', it won't work. Adding named re-exports would make the shim more complete:

export { configureRerumOptions } from './versioning.js'
export { configureWebAnnoHeadersFor, configureLDHeadersFor, configureLastModifiedHeader } from './headers.js'
export { isDeleted, isReleased, isGenerator, isContainerType, isLD } from './predicates.js'

Suggestion 3: Document the migration plan for remaining controllers

Files: 10 controllers still using import utils from '../utils.js'

The PR updates crud.js and delete.js to import directly from the new modules, but 10 other controllers (search.js, release.js, putUpdate.js, history.js, bulk.js, patchSet.js, gog.js, patchUpdate.js, overwrite.js, patchUnset.js) still use the re-export shim. This is fine for now — the shim ensures backward compatibility — but consider adding a note in the PR description about whether migrating the remaining controllers is planned as follow-up work or intentionally left as-is.


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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants