Standardize server responses to use success/failure functions

[CarlyAThomas]

Description

This standardizes how every server route responds, moving all 21 route files (63 endpoints) onto response.js's success()/failure() functions instead of the mix of hand-rolled res.* calls that had grown up across the codebase. The issue laid out four specific things to address, plus a follow-up question about the CSV pipeline and obvius.js, so I've gone through each of those in order below.

1. "The logging on failure should happen here rather than in each location where the error occurs... This means passing the sanitized message to this function and the error to log to the DB."

failure() now takes the raw error as its own parameter, separate from an optional safeMessage meant for the client. The raw error is only ever logged internally; it is never sent to the client under any circumstance, so a route can no longer accidentally leak a DB error or a stack trace just by passing the wrong thing into a response call. Routes no longer call log.warn()/log.error()/etc. themselves — that all happens inside failure() now.

2. "Add a parameter to disable logging with the default of false. That means logs are created unless the developer feels there is a good reason not to do that."

I went slightly beyond the literal ask here, with @huss's approval on the issue. Instead of a plain on/off boolean, failure() takes a LogLevel severity (DEBUG/INFO/WARN/ERROR/SILENT), so a route can keep its original log priority instead of every failure collapsing to one level. This turned out to matter more than I expected once I started migrating files: log.js's emailLevel only e-mails admins for ERROR-priority events, so if every failure had collapsed to a single severity, routine WARN-level validation failures would have started paging admins for things that were never meant to reach them. Logging is still on by default (LogLevel.ERROR); a route has to explicitly opt out with LogLevel.SILENT if it has good reason not to log something, which satisfies the "logs are created unless the developer feels there is a good reason not to" part of the ask.

3. "All routes should be checked so logging info is sent on for failures and not done in the route or any other uses."

I went through all 63 endpoints individually, by hand, rather than trusting a mechanical find-and-replace pass. That review is what turned up most of the real bugs described below — hang bugs where a route logged but never actually responded, catch blocks that responded but bypassed the shared functions entirely, and about 40 call sites where an earlier, more mechanical migration pass had silently dropped the descriptive part of the original log message. All of that is fixed and restored now, endpoint by endpoint.

4. "All routes should be changed to use the modified functions and stop sending back responses directly... Care should be taken not to send back internal error information but only have it logged."

Every one of the 21 route files (63 endpoints) has been migrated onto success()/failure(), with two deliberate, discussed exceptions explained under Limitations below. The client-facing sanitization is structural, not something each route has to remember to do correctly: failure() always substitutes a generic message for any 500-level response regardless of what's passed in, so internal detail can only ever reach the admin-visible logs, never the client.

On the CSV pipeline and obvius.js: both are addressed under Limitations below, since the outcome for each ended up being "leave it alone, for a specific reason" rather than a straightforward migration.

Fixes #1614

Real bugs found and fixed along the way

Going through every endpoint by hand (not just grepping for patterns) turned up a handful of real, pre-existing bugs:

• Multiple "hang bugs": catch blocks that only logged and never sent any response at all, leaving the client's request pending forever. Found in groups.js (5 endpoints: GET /, /idname, /deep/groups, /children/:group_id, /allChildren/), conversions.js's GET /, meters.js's GET /, users.js's GET /, maps.js's GET /, preferences.js's GET /, and ciks.js's GET / — all the same shape of bug. maps.js's /edit validation branch had a different flavor of the same problem: res.status(400) called with no .send()/.json()/.end(), which sets the status code but never completes the response.
• baseline.js had a completely broken logger: const log = require('../log') imports the log module (an object), but the file called it directly as log(message, 'error') — a TypeError on every single failure, meaning neither of this file's two endpoints ever actually logged anything, they just crashed silently. Fixed as a side effect of routing through failure().
• Several "centralized in name only" bugs: catch blocks that did log and respond, but via raw res.sendStatus()/res.send() calls that bypassed the shared response functions entirely, meaning any internal error detail passed to them went straight to the client unsanitized. Examples: groups.js's POST /delete, meters.js's GET /:meter_id, users.js's GET /:user_id, and maps.js's GET /:map_id all logged the error correctly but then responded with a raw res.sendStatus(500) instead of going through failure().
• A double-response bug in conversions.js's /edit: success(res) was called unconditionally after the try/catch, so it fired a second time even after failure() had already responded inside a caught error.
• Two un-awaited Promise.all(...).then(...) chains in groups.js (GET / and GET /deep/groups): neither awaited nor given a .catch(), so an error inside either callback fired after the enclosing try/catch had already exited — an unhandled rejection with nowhere to land, independent of the Express-4 async-forwarding gap below.
• Restored ~40 call sites' worth of lost log context: the initial mechanical migration pass replaced log.error("descriptive text: " + err, err)-style calls with bare failure(res, code, err), which loses everything except err.message. Every catch block across all 21 files was reviewed individually and had its original descriptive context restored via new Error(message, { cause: err }), which also required a small log.js fix so the console/file output continues to show the original error's stack trace (via .cause), not just the wrapper's — this only affects console/file output, never what gets persisted to the DB (which only ever stores message).
• Express 4 doesn't forward rejected async route handler promises to the global error handler on its own (Express 5 does this natively) — added express-async-errors as a standalone, one-line fix. This is temporary; tracked for removal in Upgrade Express from v4 to v5 #1676 (the Express 5 migration, split out of this issue's scope at @huss's request) via a TODO comment already in app.js pointing at that issue.

A few smaller things fixed in passing

While I was already touching these exact lines:

• units.js's /addUnit validation log said "edit units" instead of "add units" (copy-paste from the edit endpoint).
• maps.js's /delete catch block said "Error while deleting group" instead of "map" (copy-paste from groups.js).
• conversions.js's /simulate-delete client-facing validation message said "delete conversions" instead of "simulate deletion of conversions" (copy-paste from /delete, right above it).
• meters.js had a TODO left by the original authors: /edit and /addMeter's success paths called res.json(...) directly instead of success(), with a comment saying they weren't sure success() could return values. I checked — res.send() JSON-encodes objects identically to res.json(), so it works fine — and converted both. Once I'd confirmed that pattern was safe, I applied it consistently anywhere else a route's success path was bypassing success() for the same reason (preferences.js, logs.js, baseline.js, ciks.js, compareReadings.js, unitReadings.js, conversionArray.js).

Test coverage added

responseParamsTest.js already existed and covered success()/failure()'s response-sending mechanics (status codes, comment handling, edge cases), but every one of those tests passed error = null, so failure()'s actual logging behavior — the core of what this issue asked for — was never exercised. Added 5 tests, using a sinon stub on log.log() so nothing actually hits the real DB/e-mail during the test:

• Logs at the given severity — passing LogLevel.WARN actually results in log.log() being called with WARN, the error's message, and the error object itself.
• Defaults to LogLevel.ERROR when no severity is passed — this is the "logs are created unless the developer opts out" behavior from the issue.
• LogLevel.SILENT suppresses logging entirely — even with a real error present, log.log() is never called.
• No error, no log — if error is null/falsy, nothing gets logged regardless of severity.
• 500-level responses never leak the raw error to the client — constructs an Error with a fake secret in its message, sends it through failure() at a 500 code, and asserts the response body is only ever the generic message, never containing the secret — while confirming the error was still logged internally. This is the core safety guarantee of the whole redesign, and previously nothing verified it.

I also considered adding tests characterizing the "phantom success" delete bug noted above (units/conversions/users), but held off — whether deleting a nonexistent id should return 200 or something else is an API-design decision, not something to lock in via a test as a side effect of this PR. Flagging it here rather than guessing.

Catching this branch up with development

This took long enough that development moved a lot underneath it — 106 commits, 48 files, including the new session-invalidation/auth work (login.js became loginLogout.js) and changes overlapping 14 of the 18 route files this PR touches. I rebased onto current development and went through every conflict by hand rather than taking either side wholesale, so both upstream's new validation logic/features and this issue's response-standardization work survive together. Full test suite passes after the rebase — 942 passing, 1 pending (down from 4 pending before the rebase, since upstream's own date-validation fix let 3 previously-skipped compareReadingsParamsTest.js tests get re-enabled).

Type of change

• Note merging this changes the database configuration.
• This change requires a documentation update (left unchecked since none of the existing developer/admin docs are part of this repo to update directly — see the note under Limitations)

Checklist

• 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.

Author: @CarlyAThomas

Limitations

Two files stay outside the response.js consolidation, both discussed and agreed with @huss on the issue:

csvPipeline/success.js and csvPipeline/failure.js are left as-is. They were originally in scope, but PR #1591 (an in-flight, unmerged XSS fix using DOMPurify) is independently modifying the same two files for a different reason, and consolidating them now would conflict with that work and undermine its fix. Fusing them into one shared function with a mode parameter is still a real possibility, just as a separate follow-up once #1591 lands.

obvius.js keeps its own separate success/failure implementation — different signature, a hardware-specific 406 status, and a plaintext body format that shouldn't change without risking ~7-8 year old field hardware we can't test against. The only change there is renaming the local functions to successObvius/failureObvius, so the distinction from response.js's functions is visible at every call site instead of only in a comment.

A few pre-existing bugs and gaps turned up while reviewing every endpoint. These are input-validation/data-shaping issues, not response-sanitization ones, so I left them alone as out of scope for this issue, but wanted them on record:

• groups.js's GET /children/:group_id has no input validation on :group_id, unlike its siblings, so malformed input reaches the database directly instead of being rejected cleanly.
• readings.js's GET /line/count/meters/:meter_ids has no numeric pattern on meter_ids (its sibling endpoint and compareReadings.js's equivalent both do), so non-numeric input reaches the DB as NaN and turns into an ERROR-level, admin-emailed log entry instead of a clean 400.
• The same "phantom success" pattern shows up three times: POST /api/units/delete, POST /api/conversions/delete, and POST /api/users/delete all run a DELETE ... WHERE with no RETURNING clause through conn.none(), which only throws if rows are returned, not based on how many were affected — so deleting something that doesn't exist still comes back as a 200 success. Confirmed live for all three.
• GET /api/users/:user_id sends the raw DB row back to the client, including the bcrypt passwordHash field. Nothing this PR changes, but probably worth a look — there's no real reason an admin caller needs the hash itself.
• Some 400s leak raw jsonschema validator text (like instance.group_id does not match pattern "^\d+$") to the client — that's the library's own internal formatting, not something OED wrote on purpose, though it's not a server-internals leak either (nothing about DB structure or file paths). Left the wording exactly as it was, same as everywhere else in this PR.

Two of the route test files have a pre-existing bug that leaves them testing nothing. Both baselineParamsTest.js and conversionArrayParamsTest.js (from PR #1528) hit the wrong URL — /api/baseline and /api/conversionArray/refresh instead of the actual mounted paths, /api/baselines and /api/conversion-array. Every request in both files silently lands on the SPA's index.html catch-all instead of the real route, so ~30 tests per file are commented out with a note saying the route "isn't properly mounted" — it is, the URLs just have a typo. I verified both endpoints thoroughly by hand with curl instead. I haven't checked whether other files from #1528 have the same issue; the ones I actually ran this session all passed with real, meaningful counts.

Two flaky test failures showed up while I was running the full suite, and I chased both down before assuming they were mine. csvParamsTest.js's "should handle multiple file upload attempts" fails intermittently with an EPIPE (multer doesn't drain the request stream after a file-count error — nothing in csv.js/csvPipeline is touched by this branch at all). logsRouteTests.js's "should handle date range filtering" inserts a log, waits 1s, records a cutoff timestamp, waits another 1s, inserts a second log, then queries by date range — but it assumes each POST /api/logs/info has finished writing to the DB by the time it responds 200. It hasn't: log.js's Logger.log() fires its DB insert in an un-awaited async IIFE, so the HTTP response can return before the row is actually committed. That's invisible in an isolated run (the insert finishes in milliseconds either way), but under the full suite's DB connection-pool contention from hundreds of concurrent tests, that gap widens enough to occasionally break the test's own timing assumptions. Neither is something this branch touches or introduces. I reproduced both failing on the unmodified base commit on a repeat run, and both pass cleanly every time in isolation on either branch — they're pre-existing flakiness under load, not something this branch causes.

Heads up on overlap with #1666. @aduques is working on #1666 (making client-facing error messages show the specific validation details instead of generic text), and their investigation found the same users.js/groups.js raw-response issues this PR fixes. The mechanism change (routing through success()/failure(), consistent response shape) is what I did here; I deliberately left every endpoint's original message wording untouched, so their work on message content — like replacing users.js's generic "Invalid params" with the specific validation errors — is completely separate and still fully needed. @huss already weighed in on the issue that it's fine for both to proceed in parallel.

No documentation currently covers any of this, and I think it should exist. I checked the developer docs (they live in a separate repo, OpenEnergyDashboard.github.io, not this one) for anything on error handling, logging, log levels, or admin e-mail notifications. The closest existing page just says where log output ends up (log.txt, nohup.out) — nothing about how success()/failure() work, what LogLevel controls, or when an admin actually gets e-mailed. Since this affects how every future route is written, what the client sees on failure, and when admins get paged, I think it's worth a dedicated page rather than something I fold into this PR. I'd like to raise this with @huss directly — either as a documentation proposal in this PR thread, or as its own follow-up question — rather than guess at what should go where.

[huss]

@CarlyAThomas Thank you for this substantial contribution, esp. being your first one. I have reviewed your well documented changes and comments provided with this PR. I am very appreciative of the extra work you did outside the direct issue you were working on. At this point I wanted to give some early thoughts:

• It will take a little while to get through the many changes since they impact many files/areas. Please know I am trying to get the time to go through them all with the care they deserve. If I am delaying you then please let me know.
• You mention a number of items that are not done within the code changes of this PR but indicating they probably deserve consideration. In at least one case you talk about future work. These seem very useful and I will be commenting on them later. For now, I wanted to get a sense of what you might want for when I respond. I always prefer to give the person who located a concern the option to create the issue/documentation on the situation. Do you think you might want to do that for some/all of the items you discussed? If not, OED can create them as desired. This will help me know, if possible, what would be best to say/provide in my thoughts on each of these. Obviously, OED is not asking you to commit to definitely do any issues but it would remain an option if you want to after any discussion is done on them.

Again, thanks for your efforts and any information/thoughts/questions. I hope this is clear for you.

[CarlyAThomas]

Thanks! And no worries on timing—I know this is a large review, so I'd much rather you take the time needed than rush through it.

As for the additional items, I'd be happy to create the issues for the ones that make sense. My main goal is simply to make sure they're valid observations and that the scope is appropriate before they're tracked. Once we've discussed them, I'm happy to write them up if you think they'd be worthwhile additions to the issue tracker.

[huss]

Once we've discussed them, I'm happy to write them up if you think they'd be worthwhile additions to the issue tracker.

Thank you and this sounds great. It will be a little while but I'll get to this.