Issue1666 - Include specific parameters to success/error messages on all routes by aduques · Pull Request #1689 · OpenEnergyDashboard/OED

aduques · 2026-08-01T05:41:01Z

Description

This PR addresses inconsistent and vague error messages returned to the client across groups.js, units.js, users.js, meters.js, and conversions.js. The root causes fell into a few categories:

error.data vs error.data.message mismatch

Some client components read error.data while others read error.data.message, depending on whether the corresponding server route sent a plain string (via response.js success()/failure()) or a raw JSON object (via res.json()/res.send()).

users.js did not previously import response.js at all, so its client components (CreateUserModalComponent.tsx, EditUserModalComponent.tsx) required .message to extract the string. This PR converts users.js to use response.js consistently, so error.data can be used uniformly. This change for consistency is intended to set up a future issue where a standard function will be created to handle the messages and validations of these routes.

Generic "Invalid params" / hardcoded messages

users.js /create, /edit, and /delete routes previously discarded the actual validator output (validatorResult.errors) and always returned a generic "Invalid params" message. This has been fixed to display the specific validation error, matching the patterns already used in groups.js and conversions.js

response.js failure() discarding custom messages at 500+

failure() always overrides the provided comment with a generic "Internal Server Error" message whenever the status code is 500 or above. Several routes (groups.js, meters.js) were passing expected errors (duplicate names, invalid check-constraint values) through failure() at 500, silently hiding the actual message. These routes now catch known error cases (duplicate key violations, check constraint violations) and re-send them at 400 so the specific message reaches the client, while unexpected errors continue to fall through to the generic 500 response as intended.

Missing response bodies causing undefined in the client

groups.js /delete catch block previously called res.sendStatus() with no body, causing the client to display "undefined." This has been fixed to use failure() with a specific message.

All affected routes (conversions.js, groups.js, units.js, users.js, meters.js) and their corresponding client components have been manually tested for CREATE, EDIT, and DELETE success and failure paths to confirm specific, correct messages are now displayed.

Partially Addresses #1666

Type of change

(Check the ones that apply by placing an "x" instead of the space in the [ ] so it becomes [x])

Note merging this changes the database configuration.
This change requires a documentation update

Checklist

(Note what you have done by placing an "x" instead of the space in the [ ] so it becomes [x]. It is hoped you do all of them.)

I have followed the OED pull request ideas
I have removed text in ( ) from the issue request
You acknowledge that every person contributing to this work has signed the OED Contributing License Agreement and each author is listed in the Description section.

Limitations

meters.js /edit and /addMeter routes could not be converted to use response.js's success() on the success path, since the client depends on the DB-assigned data (e.g. id) being returned in the response body, and success() does not currently support returning structured data. These routes continue to use res.json() on success, per the pre-existing TODO comment in the file.
A broader, standardized validation and response wrapper function (to centralize validate(), logging, and message formatting across all routes) was discussed with the maintainer but has been left to be addressed in a new issue, since it depends on this issue and standardize server responses to use success/failure functions #1614 to be completed first.

…the global error handler Express 4 does not do this natively (Express 5 does); this closes that gap as a standalone fix ahead of the Express 5 migration tracked in OpenEnergyDashboard#1676. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

failure() call sites now pass new Error(message, { cause: originalErr }) to preserve descriptive context in the DB-persisted log message. This surfaces the original error's stack in console/file output too, not just the wrapper's -- console/file only, the DB-persisted message is unaffected either way.

obvius.js intentionally keeps its own separate implementation (different signature, hardware-specific 406 status and plaintext format that can't be safely changed) rather than being merged into response.js, per maintainer discussion on OpenEnergyDashboard#1614. Renaming makes that distinction visible at every call site instead of only in a comment.

…t-facing messages failure() now takes the raw error (log-only, never sent to the client) separately from an optional safe message (client-facing, only used under 500). Replaces the boolean skipLog parameter with a LogLevel severity (DEBUG/INFO/WARN/ERROR/SILENT) so routes can preserve their original logging severity instead of collapsing to ERROR -- log.js only e-mails admins for ERROR-priority events, so this avoids turning routine WARN-level validation failures into admin spam. Updates responseParamsTest.js for the new signature.

Routes every success/failure path through the shared success()/failure() functions instead of raw res.* calls, fixing several real bugs found along the way: multiple catch blocks that only logged and never responded (hung the request forever), two un-awaited Promise.all().then() chains that let errors escape their enclosing try/catch, and branches that bypassed response.js's sanitization guarantee entirely via raw res.send()/res.sendStatus(). Preserves the original descriptive log context via new Error(message, { cause: err }) so admin-visible logs stay as informative as before.

Routes every success/failure path through the shared success()/failure() functions instead of raw res.*/log.* calls. Preserves each original descriptive log message via new Error(message, { cause: err }), and restores the two validation branches (edit, delete) that originally logged at LogLevel.WARN back to that severity instead of silently escalating to the new default of LogLevel.ERROR -- addUnit's validation branch was already ERROR in the original, so it needed no severity change. Also fixes a pre-existing copy-paste bug where addUnit's log message said "end units" instead of "add units".

Routes every success/failure path through the shared success()/failure() functions instead of raw res.*/log.* calls. Preserves each original descriptive log message via new Error(message, { cause: err }), and restores the two validation branches (edit, simulate-delete) that originally logged at LogLevel.WARN back to that severity instead of silently escalating to the new default of LogLevel.ERROR. Fixes several real bugs found along the way: GET /'s catch block only logged and never responded (a hang bug, same family as groups.js's); addConversion's and simulate-delete's success paths bypassed success()/response.js entirely via raw res.sendStatus()/res.json(); and edit had a double-response bug where success(res) fired unconditionally after the try/catch, even when failure() had already responded inside the catch block. Also fixes a pre-existing wording bug where simulate-delete's client-facing validation message incorrectly said "delete conversions" (copied from the /delete endpoint above it) instead of "simulate deletion of conversions".

Routes every success/failure path through the shared success()/failure() functions instead of raw res.*/log.* calls. Preserves each original descriptive log message via new Error(message, { cause: err }), including restoring err['detail'] into both the log and (implicitly, via generic 500 text) the client-visible failure on /edit and /addMeter -- same pattern as groups.js's POST /create. Restores the two validation branches (edit, addMeter) that originally logged at LogLevel.WARN back to that severity instead of silently escalating to the new default of LogLevel.ERROR. Fixes a real hang bug: GET /'s catch block only logged and never responded, same family as groups.js's/conversions.js's GET /. GET /:meter_id's catch also previously bypassed failure() via raw res.sendStatus(500) despite already logging. Completes a pre-existing TODO on /edit and /addMeter: both success paths called res.json(...) directly instead of success(), per the original authors' own comment explaining success() couldn't return values -- confirmed via testing that success()'s res.send() JSON-encodes objects identically to res.json(), so both are now converted, removing the now-unused success import in the process. GET /:meter_id's two 400 branches (malformed meter_id, and "meter exists but isn't displayable") are deliberately indistinguishable from each other per the original comment; kept them symmetric with no safeMessage (empty body) rather than reproducing res.sendStatus(400)'s incidental "Bad Request" text, since the latter was never a deliberate message.

Routes every success/failure path through the shared success()/failure() functions instead of raw res.*/log.* calls, preserving each endpoint's original response body shape exactly (both plain-string and {message: ...}-object forms) since the frontend depends on that shape. Restores descriptive log context via new Error(message, { cause: err }) on all 5 catch blocks that had it (GET /, GET /:user_id, POST /create, POST /edit, POST /delete). Most of this file's non-500 branches (GET /token's 4 response points, and every endpoint's basic validation branch) never logged anything in the original code -- the migration preserves that by passing a null error or LogLevel.SILENT where appropriate, rather than defaulting to logging everything. Fixes a real hang bug: GET /'s catch block only logged and never responded at all. GET /:user_id's catch also previously bypassed failure() via raw res.sendStatus(500) despite already logging. The admin-lockout-prevention branch on POST /edit was already a deliberate log.error() in the original (not a WARN mismatch like other files) and is preserved at that severity.

Routes both success and failure paths through the shared success()/failure() functions instead of raw res.*/log.* calls, preserving the original {token, username, role}/{text: ...} response body shapes exactly. Restores the "Unable to check user password for <username>" log context (including the username, which was useful context in the original) on the genuine-error catch branch, lost in the mechanical migration. The bad-credentials branch (wrong password/nonexistent user) never logged in the original either, preserved via LogLevel.SILENT.

Routes every success/failure path through the shared success()/failure() functions instead of raw res.*/log.* calls. Preserves each original descriptive log message via new Error(message, { cause: err }) on the 4 genuine-error catch blocks (GET /, GET /:map_id, POST /create, POST /edit), and fixes a copy-paste bug where POST /delete's log message said "Error while deleting group" instead of "map". Fixes two hang bugs: GET /'s catch block only logged and never responded, same family as other files' GET /. POST /edit's validation branch called res.status(HTTP_CODES.BAD_REQUEST) with no .send()/.json()/ .end() -- that sets the status code but never actually completes the response, so the request hung indefinitely on invalid edit data. Both now respond correctly since failure() always calls res.send() internally. Validation branches on /create and /edit pass their descriptive message as failure()'s error param only (not safeMessage), matching each endpoint's original behavior of only ever logging these details, never showing them to the client -- consistent with the empty-body-over- sendStatus's-incidental-text precedent from meters.js/users.js/login.js.

Routes the validation-failure, JWT-verify-failure, and success paths through failure()/success() instead of raw res.*/log.* calls, preserving the original response body shapes exactly. Neither non-success branch logged in the original code -- preserved via omitting the error argument (validation branch) and LogLevel.SILENT (JWT-verify branch).

Routes success/failure paths through the shared success()/failure() functions instead of raw res.*/log.* calls. Converts both success paths from res.json(rows) to success(res, rows), completing the pattern (rows is always a single object here via Preferences.mapRow(), so res.send() JSON-encodes it identically to res.json()). Restores descriptive log context via new Error(message, { cause: err }) on both catch blocks. Fixes a real hang bug: GET /'s catch block only logged and never responded at all, same family as other files' GET /.

Routes success/failure paths through the shared success()/failure() functions instead of raw res.*/log.* calls. The client-logging endpoints (POST /info, /warn, /error) keep their level-specific log.info()/ log.warn()/log.error() calls on the success path untouched, since recording the client's message at the requested level is the actual purpose of those routes, not something to route through failure(). Their validation-failure branches (and GET /logsmsg's) pass the descriptive message as failure()'s error param only, matching the maps.js/logs-family precedent: logs at the correct default ERROR severity, client gets an empty body instead of raw sendStatus(400)'s incidental "Bad Request" text. Restores descriptive log context on GET /logsmsg's catch block via new Error(message, { cause: err }), and converts its success path from res.json(rows) to success(res, rows) -- rows is an array here, which Express's res.send() JSON-encodes identically to res.json().

Routes success/failure paths through the shared success()/failure() functions instead of a broken standalone log() call. baseline.js was importing ../log as const log = require('../log') (the whole module object, not a callable) and then calling log(message, 'error') directly -- that throws TypeError every time either catch block runs, meaning neither ever actually logged anything before this change; this was the only file in the codebase with that pattern. Converting to failure() removes the broken calls and fixes the bug as a side effect, restoring real admin-visible error logging for the first time. Also converts GET /'s success path from res.json(rawBaselines) to success(res, rawBaselines) (an array, which Express's res.send() JSON-encodes identically to res.json()), and restores descriptive log context via new Error(message, { cause: err }) on both catch blocks. Confirmed live that baselineParamsTest.js has no real coverage of this file due to a pre-existing URL typo (tests hit /api/baseline, singular; the route is mounted at /api/baselines) -- unrelated to this change, documented in OED NOTES.md, flagged as a possible follow-up issue.

Routes the success/failure path through the shared success()/failure() functions instead of a broken log.error()-only catch block that never sent a response at all -- the same hang-bug family found in every other file's GET /. Converts the success path from res.json(...) to success(res, ...) (an array, JSON-encoded identically either way), and restores descriptive log context via new Error(message, { cause: err }). Confirmed live via a genuine database outage (stopped the database container): the client gets a real 500 instead of hanging, and the log shows the fully restored context plus the original DNS-resolution error's stack via the "Caused by:" chain.

This file originally had no try/catch at all -- both endpoints relied entirely on the global error handler (and, before this branch, on nothing, since express-async-errors didn't exist yet either). Real try/catch blocks wrapping the data-fetch were already added earlier in this branch's work; this commit adds descriptive context to those catch blocks' failure() calls via new Error(message, { cause: err }) for consistency with every other file's catch blocks, distinguishing the meter and group endpoints' errors from each other. Converts both success paths from res.json(...) to success(res, ...) (both return plain objects keyed by meter/group id, JSON-encoded identically either way). Confirmed live via the known oversized-meter_id repro from earlier in this branch's work (meter_compare_readings_unit(...) does not exist): the client still gets the generic sanitized 500, and the log now shows the full restored context instead of the bare error.

Routes success/failure paths through the shared success()/failure() functions instead of raw res.*/log.* calls. Restores descriptive log context on both outer catch blocks via new Error(message, { cause: err }), and restores the WARN severity (with cause preservation) on both endpoints' timeInterval-parsing failure branches, which the mechanical migration had collapsed to the new default of ERROR. Left the two nosemgrep-suppressed res.send() calls on the success paths untouched, since converting them to success() would orphan the suppression comments from the exact line they're anchored to. Found and documented in OED NOTES.md (not fixed here, out of OpenEnergyDashboard#1614's scope as an input-validation issue): GET /line/count/meters/:meter_ids' meter_ids schema has no numeric pattern, letting non-numeric input reach the DB as NaN and escalate to an ERROR-level admin-emailed log entry instead of a clean 400. Also documented: GET /line/raw/meter/:meter_id's identical class of bug (a type/string mismatch making the endpoint permanently return 400) was already found and fixed upstream via issue noted as a real reconciliation point for the eventual development sync, since that PR's changes to this file (a file-size/role access-control feature, a schema fix, and a partial old-style response.js migration on one endpoint) don't overlap cleanly with what we did here.

This file originally had no try/catch at all across any of its 8 endpoints (same as compareReadings.js); real try/catch blocks wrapping each data-fetch were already added earlier in this branch's work. This commit adds descriptive context to all 8 catch blocks' failure() calls via new Error(message, { cause: err }), one per line/bar/radar/threeD x meters/groups combination, for consistency with every other file's catch blocks and to distinguish each endpoint's errors from one another in the admin logs. Converts all 8 success paths from res.json(forJson) to success(res, forJson) -- all return plain objects (via lodash mapValues, keyed by meter/group id) or similarly structured data, never raw strings, so JSON-encoding is identical either way.

Real try/catch (replacing a literal // TODO: Add try/catch error handling... comment) and the success()/failure() conversion were already added earlier in this branch's work. This commit adds a descriptive context wrapper to the catch block via new Error(message, { cause: err }), for consistency with every other file's catch blocks -- there was no original logging to restore here, same as compareReadings.js/ unitReadings.js, since the endpoint never had any error handling at all before. Confirmed live (validation failure, no-op success, and a real redoCik success). conversionArrayParamsTest.js has zero real coverage of this endpoint due to a pre-existing URL typo (tests hit /api/conversionArray/ refresh, camelCase; the route is mounted at /api/conversion-array, hyphenated) -- same bug class as baseline.js's test file, both from PR OpenEnergyDashboard#1528, documented in OED NOTES.md.

… DEBUGs + ran tests on users.js requests

…d TODO DEBUGs + ran tests on conversions.js requests

…O DEBUGs + ran tests on groups.js requests

… + ran tests on meters.js requests (NEEDS REVIEW/ SUCCESS NOT ADDED)

… DEBUGs + ran tests on units.js requests

aduques · 2026-08-01T05:45:19Z

I acknowledge that the there are still commented out TODO DEBUG lines of codes in the committed code. I will remove these in a future commit once the changes have been reviewed and the pull request is close to being wrapped up.

users.js

CREATE FAIL: Failed to create the user: (username: himeko) Got request to insert user with invalid user data. Error(s): instance is not allowed to have the additional property "username",instance is not allowed to have the additional property "role",instance is not allowed to have the additional property "password",instance is not allowed to have the additional property "note",instance requires property "user"

EDIT FAIL: Failed to edit user: (username: himeko) Got request to edit users with invalid user data, errors: instance.user.username does not meet minimum length of 3

DELETE FAIL: Failed to delete the user: (username: himeko) Got request to delete users with invalid user data. Error(s): instance is not allowed to have the additional property "id"

CREATE SUCCESS: Successfully created the user: (username: welt) (role: admin)

EDIT SUCCESS: Successfully edited user: (username: welt) (role: admin)

DELETE SUCCESS: Successfully deleted user: (username: welt)

conversions.js

CREATE FAIL: Failed to create a conversion. (Source: "7/26edit", Destination: "730unit") Got request to insert conversion with invalid conversion data. Error(s): instance.sourceId must be greater than or equal to 1,instance.destinationId must be greater than or equal to 1

EDIT FAIL: Failed to edit conversion. (Source: "730unit", Destination: "7/26unit") Got request to edit conversions with invalid conversion data, errors: instance.sourceId must be greater than or equal to 1,instance.destinationId must be greater than or equal to 1

DELETE FAIL: Failed to delete conversion. (Source: "730unit", Destination: "7/26edit") Got request to delete conversions with invalid conversion data. Error(s): instance.meterIds[0] must be greater than or equal to 1

CREATE SUCCESS: Successfully created a conversion. (Source: "7/26edit", Destination: "730unit")

EDIT SUCCESS: Successfully edited conversion. (Source: "7/26edit", Destination: "730unit")

DELETE SUCCESS: Successfully deleted conversion. (Source: "730unit", Destination: "7/26edit")

groups.js

CREATE FAIL: Failed to create a group with message: (name: "730group") Got request to create group with invalid data. Error(s): instance.name does not meet minimum length of 1

EDIT FAIL: Failed to edit group with message: (Name: "730group") Got request to edit group with invalid data. Error(s): instance.name does not meet minimum length of 1

DELETE FAIL: Failed to delete group.Got request to delete group with invalid data. Error(s): instance.id must be greater than or equal to 1

CREATE SUCCESS: Successfully created a group. (name: "730group")

EDIT SUCCESS: Successfully edited group. (Name: "730group")

DELETE SUCCESS: Successfully deleted group. (Name: "730group")

meters.js

CREATE FAIL:
Failed to create a meter with message: "731test" (identifier: 731test, type: egauge) invalid date/time format

Failed to create a meter with message: "730meter" (identifier: 730meter, type: egauge) Meter name "730meter" already exists

EDIT FAIL:
Failed to edit meter with message: "727meter" (identifier: 727meter, type: egauge) Invalid meter data: error: new row for relation "meters" violates check constraint "meters_identifier_check"

Failed to edit meter with message: "730meter" (identifier: 730meter, type: egauge) Meter name "730meter" already exists

CREATE SUCCESS: Successfully created a meter."731success" (identifier: 731success, type: egauge)

EDIT SUCCESS: Successfully edited meter."731success" (identifier: 731success, type: egauge)

NOTE: success() cannot be used currently in meters.js. There is an existing TODO note that states that /addMeter and /edit are expected to return a json object. Therefore, simply replacing it with success() does not work. For now, I have skipped over applying success() until I look into it more. If this is something that I can do within this pull request, I can apply the change, but if it requires more files to be changed, it may be better to open this specifically as a new issue to keep it separate from this one. Thoughts on this is appreciated.

NOTE: Every database error would fall through to the generic 500 branch in response.js's failure(). That function is designed to discard any custom message and always send back "Internal Server Error..." whenever the status code is 500 or higher, so no matter what specific comment the route passed in, the client would only ever see the generic text. By catching these particular error types here and re-sending them at 400 instead, failure() is allowed to pass the actual message through rather than suppressing it, so the user sees what's actually wrong instead of the message shown in failure().

units.js

CREATE FAIL:
Failed to create a unit. "731unit" (identifier: 731unit, type: unit) Got request to add units with invalid unit data, errors: instance.secInRate must be greater than or equal to 0

Failed to create a unit. "730unit" (identifier: 730unit, type: unit) Unit name "730unit" already exists

EDIT FAIL:
Failed to edit unit."730unit" (identifier: 730unit, type: unit) Got request to edit units with invalid unit data, errors: instance.secInRate must be greater than or equal to 0

Failed to edit unit."730unit" (identifier: 730unit, type: unit) Unit name "730unit" already exists

DELETE FAIL: Failed to deleted unit with error: Got request to delete a unit with invalid data, error(s): instance.id must be greater than or equal to 1

CREATE SUCCESS: Successfully created a unit. "731unit" (identifier: 731unit, type: unit)

EDIT SUCCESS: Successfully edited unit. "731unit" (identifier: 731unit, type: unit)

DELETE SUCCESS: Successfully deleted unit: 731unit

huss

@aduques Thank you for another contribution. I need some more time to look over all the proposed changes. I did compare this to another recent PR #1688 and put in some comments about that. I hope I correctly did all the ones that I should have. I wanted you to be able to see them now to think it over. Please let me know any thoughts.

huss · 2026-08-02T22:39:54Z

 				await newConversion.insert(t);
 			});
-			res.sendStatus(HTTP_CODES.OK);
+			//res.sendStatus(HTTP_CODES.OK);


I think this change overlaps a change in PR #1688 that came in at about the same time. My current thinking is their change should stay as all routes were aligned with this new methodology. Thought?

Yes, I definitely agree. The changes made in #1688 should take priority as extensive work has been done to have consistent implementations across all the routes. When merging the work from #1688, their change has overwritten this one.

huss · 2026-08-02T22:42:53Z

 				return t.batch(flatten([adoptGroupsQueries, disownGroupsQueries, adoptMetersQueries, disownMetersQueries]));
 			});
-			res.sendStatus(HTTP_CODES.OK);
+			//res.sendStatus(HTTP_CODES.OK);


See other comment. The changes here also overlap the other PR. Thoughts?

Noting that the merge has replaced this with the consistent implementation from #1688.

huss · 2026-08-02T22:45:09Z

 		} catch (err) {
 			log.error(`Error while editing a meter with detail "${err['detail']}"`, err);
-			failure(res, HTTP_CODES.INTERNAL_SERVER_ERROR, err.toString() + ' with detail ' + err['detail']);
+			if (err.toString().includes('duplicate key value violates unique constraint')) {


See other comment. The changes here also overlap the other PR but I think bring in other features. Do you think they could be merged? Thoughts?

This has been updated to consider the changes from the other PR.

} catch (err) { if (err.toString().includes('duplicate key value violates unique constraint')) { failure(res, HTTP_CODES.BAD_REQUEST, err, `Meter name "${req.body.name}" already exists`, LogLevel.SILENT); } else if (err.toString().includes('violates check constraint')) { failure(res, HTTP_CODES.BAD_REQUEST, err, `Invalid meter data: ${err.toString()}`, LogLevel.SILENT); } else { const detail = err['detail'] ? ` with detail "${err['detail']}"` : ''; failure(res, HTTP_CODES.INTERNAL_SERVER_ERROR, new Error(`Error while editing a meter${detail}: ${err.message}`, { cause: err })); } }

huss · 2026-08-02T22:46:26Z

 		} catch (err) {
 			log.error(`Failed to update unit: ${err}`, err);
-			failure(res, HTTP_CODES.INTERNAL_SERVER_ERROR, 'Unable to update unit');
+			//failure(res, HTTP_CODES.INTERNAL_SERVER_ERROR, 'Unable to update unit');


See other comment. The changes here also overlap the other PR but with new items. Thoughts?

This has been updated to consider the changes from the other PR.

} catch (err) { if (err.toString().includes('duplicate key value violates unique constraint')) { failure(res, HTTP_CODES.BAD_REQUEST, err, `Unit name "${req.body.name}" already exists`, LogLevel.SILENT); } else if (err.toString().includes('violates check constraint')) { failure(res, HTTP_CODES.BAD_REQUEST, err, `Invalid unit data: ${err.toString()}`, LogLevel.SILENT); } else { failure(res, HTTP_CODES.INTERNAL_SERVER_ERROR, new Error(`Failed to update unit: ${err.message}`, { cause: err })); } }

huss · 2026-08-02T22:46:59Z

 		} catch (err) {
 			log.error(`Error while inserting new unit: ${err}`, err);
-			failure(res, HTTP_CODES.INTERNAL_SERVER_ERROR, `Error while inserting new unit: ${err}`);
+			//failure(res, HTTP_CODES.INTERNAL_SERVER_ERROR, `Error while inserting new unit: ${err}`);


See comment above in this file.

This has been updated to consider the changes from the other PR.

} catch (err) { if (err.toString().includes('duplicate key value violates unique constraint')) { failure(res, HTTP_CODES.BAD_REQUEST, err, `Unit name "${req.body.name}" already exists`, LogLevel.SILENT); } else if (err.toString().includes('violates check constraint')) { failure(res, HTTP_CODES.BAD_REQUEST, err, `Invalid unit data: ${err.toString()}`, LogLevel.SILENT); } else { failure(res, HTTP_CODES.INTERNAL_SERVER_ERROR, new Error(`Error while inserting new unit: ${err.message}`, { cause: err })); } }

huss · 2026-08-02T22:48:14Z

 	// isEdit=false: id must not be present, since it's assigned by the DB on insert, and password is required.
-	if (!validateUsersParams(req.body, false).valid) {
-		res.status(HTTP_CODES.BAD_REQUEST).json({ message: 'Invalid params' });
+	const validatorResult = validateUsersParams(req.body, false);


See other comment. The changes here also overlap the other PR. Thoughts?

I have made the following changes to match with the other PR.

const validatorResult = validateUsersParams(req.body, false); if (!validatorResult.valid) { const message = `Got request to insert user with invalid user data. Error(s): ${validatorResult.errors}`; failure(res, HTTP_CODES.BAD_REQUEST, message, { message });

huss · 2026-08-02T22:49:22Z

 			const currentUser = await User.getByUsername(username, conn);
 			if (currentUser !== null) {
-				res.status(HTTP_CODES.BAD_REQUEST).send({ message: `user ${username} already exists so cannot create` });
+				//res.status(HTTP_CODES.BAD_REQUEST).send({ message: `user ${username} already exists so cannot create` });


See other comment. The changes here also overlap the other PR. Thoughts?

Also, commented out line may not be for debugging.

I have made the following changes to match with the other PR.

if (currentUser !== null) { failure(res, HTTP_CODES.BAD_REQUEST, null, { message: `user ${username} already exists so cannot create` });

The commented out line was handled.

huss · 2026-08-02T22:50:14Z

 				const user = new User(undefined, username, hashedPassword, role, note);
 				await user.insert(conn);
-				res.sendStatus(HTTP_CODES.OK);
+				//res.sendStatus(HTTP_CODES.OK);


Commented out line. See other comment. The changes here also overlap the other PR but seem the same.

I have addressed the commented out line.

huss · 2026-08-02T22:50:36Z

 			// Log the error internally and return a generic response
 			log.error(`Error while performing POST request to create user: ${error}`, error);
-			res.status(HTTP_CODES.INTERNAL_SERVER_ERROR).send({ message: 'Internal Server Error' });
+			failure(res, HTTP_CODES.INTERNAL_SERVER_ERROR, `Error while performing POST request to create user: ${error}`);


See other comment. The changes here also overlap the other PR. To simplify comments, this applies to all the remaining changes in this file. Thoughts?

I have merged the changes from the other PR and similar lines were also merged and replaced.

} catch (error) { failure(res, HTTP_CODES.INTERNAL_SERVER_ERROR, new Error(`Error while performing POST request to create user: ${error.message}`, { cause: error })); }

The difference was primarily that the error message was passed as a new Error() object rather than a raw string. I gave my thoughts on this change below, but overall, I did not find any issues with the changes made. I need to review the outputs of these messages again to ensure that the merged lines are working. The changes to the routes and parameter changes for failure()/success() affect the planning for the standard function.

aduques · 2026-08-03T20:56:29Z

@huss I am writing to update that I've seen the recent comments. I am anticipating that I will be spending some time comparing my changes with the changes made in #1688 as there are overlaps in the exact messages used in either the failure() or success() calls. From what I am seeing, the changes include detailed messages that may be generally similar to the ones that I have included in this PR. The reason for this is I believe we both took the existing error/success message and had it called in failure()/success(). The differences in our implementation appear to be that the implementations in #1688 had stored the messages into a message variable to clean up the actual failure()/success() calls. This alone can easily be merged with my changes without any issues. I will most likely merge the messaging style done in #1688 as it is very similar to what I included and it would be best for the entire codebase that there is a uniform messaging style for success() and failure() which #1688 has achieved.

There are some logic (such as the if-else statements in meters.js catch), that needs to be carefully looked at before merging. I believe that I will have no issue with merging the failure()/success() additions from #1688 to mine, but I need to test each message output again when I do so. I need to make sure that the merged changes from #1688 does not revert the fixes for undefined and generic error messages that this issue uniquely solves.

Therefore, I would appreciate guidance on how I should proceed with merging and testing the changes in #1688. Should I go through the changed files in #1688, copy paste the changes onto my branch, and test the changes. Or is the procedure more extensive, where I need to copy the working branch of #1688 onto mine and resolve the merge conflicts in that way. I would appreciate insight on the proper practice as this would be a great learning opportunity for me in the case I need to work with other active issues/pull requests.

huss · 2026-08-03T22:59:05Z

Thank you for your thoughts and looking at the other PR. First, your overall plan/idea seems good to me. I also liked the standardization of dealing with messages in the other PR along with taking care of doing the logging so it always happens. Second, I don't have a precise answer on how you can move forward because I'm not an expert and I think different people do it differently. I don't care how as long as it meets the objectives.

Having said that, the copy/paste method can lead to merge issues and/or wipe the git line history for those lines when all the work is combined, esp. if great care is not taken. If you want to accept all the changes in the other PR then you can fetch/merge that branch/file into your branch where you always use their version for those lines. If done correctly, I think, it will leave the git line history as the other person having done it along with your other changes in the file as yours. Let me add that if the entire file will be replaced with the PR one then you can revert your changes and leave it for when that PR is merged. Now, if you have overlapping changes where parts of each need to be merged then it gets trickier. Then, as far as I know, you need to manually decide what part of each is retained in the final version. This might cause merge issues for the other person (or you) depending on details but it might all be fine (others probably know better than I).

I'm not sure this is complete/perfect so I'm happy to have your thoughts/questions. Know I'm also here to help as you desire.

… than JSON object

…g rather than JSON object" This reverts commit 0392153.

aduques · 2026-08-05T21:13:46Z

@huss I have merged all of the changes made in #1688 and have resolved merge conflicts. As suspected, most of my changes have been replaced with the ones in #1688 to ensure that there is a consistent implementation in messaging across the codebase. I have tried to retain as much of my unique changes such as if-else conditions in the catch statements, sending specific param messages, and replacing generic "Invalid Params" messages.

While the primary overlap was the contents of the messages in the success() and failure() calls, there is a conflict in implementations that I would like to discuss. The implementations do not cause any errors, but it conflicts with the plan of wanting to create a standard function to handle message logs and validation across the routes. The reason for this is that there has been a change to have the success() and failure() take in strings with a message wrapper (and new Error() for error messages), rather than just a raw string parameter. The reason that this concerns me is because I have made the changes for failure to take in raw strings to handle the "error.data vs error.data.message" inconsistency problem.

For example, the messages in users.js don't work well with the changes. When using "error.data," the message will output [object Object] because it is not specifically getting the .message property of the data. Therefore, an immediate fix is to have error.data.message, but this would be one case where it is different from other routes. I plan to follow up with this message with updated outputs of each error message to visualize the current state of these messages.

If we were to proceed with the standard function, I would like an opinion on the design choice going forward. I would definitely like to prioritize #1688 as extensive work has been done to get a consistent implementation across the codebase.

My current TODO:

Using the current merged code, test the error messages on localhost
Take note of which routes are using .message and show [object Object]
Adapt any leftover messages that are raw strings to match the design of Standardize server responses to use success/failure functions #1688 (use message wrapper or new Error())

huss · 2026-08-07T13:47:58Z

@aduques It will be about a week before I can review this. Thank you for the update.

aduques and others added 27 commits July 23, 2026 13:43

initial reproducing error

837541c

Merge branch 'development' into issue1666

bc99aaf

imported response.js to users.js + added success/failure + added TODO…

874d390

… DEBUGs + ran tests on users.js requests

imported response.js to conversions.js + added success/failure + adde…

d1e90b8

…d TODO DEBUGs + ran tests on conversions.js requests

imported response.js to groups.js + added success/failure + added TOD…

a8602c5

…O DEBUGs + ran tests on groups.js requests

imported response.js to meters.js + added failure + added TODO DEBUGs…

0b954fb

… + ran tests on meters.js requests (NEEDS REVIEW/ SUCCESS NOT ADDED)

imported response.js to units.js + added success/failure + added TODO…

1356747

… DEBUGs + ran tests on units.js requests

huss reviewed Aug 2, 2026

View reviewed changes

aduques added 3 commits August 5, 2026 12:22

merged 1614 changes + removed TODO DEBUGs

3e94055

made changes to failure() in users.js to send a message string rather…

0392153

… than JSON object

Revert "made changes to failure() in users.js to send a message strin…

e33480a

…g rather than JSON object" This reverts commit 0392153.

Uh oh!

Conversation

aduques commented Aug 1, 2026

Description

Type of change

Checklist

Limitations

Uh oh!

aduques commented Aug 1, 2026

users.js

conversions.js

groups.js

meters.js

units.js

Uh oh!

huss 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!

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!

aduques commented Aug 3, 2026

Uh oh!

huss commented Aug 3, 2026

Uh oh!

aduques commented Aug 5, 2026

Uh oh!

huss commented Aug 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants