Add consistent JSON error responses with structured error codes#10
Closed
DominicBM wants to merge 1 commit into
Closed
Add consistent JSON error responses with structured error codes#10DominicBM wants to merge 1 commit into
DominicBM wants to merge 1 commit into
Conversation
All error responses now include an `error` machine-readable code alongside `message`. Consolidate FourHundredResponse/FiveHundredResponse behind a shared ErrorResponse base class, add UnauthorizedResponse, and fix a for...in/for...of bug in queryParams that silently dropped all query parameters. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
WalkthroughThis pull request refactors error response handling by introducing a base Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/aggregation/responses.ts (1)
1-17: Consider hardeningErrorResponseas an abstract immutable base.Small maintainability improvement: make this base
abstractand markmessage/errorasreadonlysince these values should not mutate after construction.♻️ Proposed refactor
-class ErrorResponse { +abstract class ErrorResponse { constructor( - message: string, + readonly message: string, readonly errorCode: number, - error: string, - ) { - this.message = message; - this.error = error; - } - - message: string; - error: string; + readonly error: string, + ) {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/aggregation/responses.ts` around lines 1 - 17, Make ErrorResponse an abstract, immutable base by declaring the class abstract and marking its instance properties message and error as readonly so they cannot be mutated after construction; update the constructor (the ErrorResponse constructor) to initialize these readonly properties (or use readonly parameter properties) and keep toJSON() unchanged to return { error, message }. Ensure references to ErrorResponse, message, error, constructor, and toJSON are updated accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/index.ts`:
- Around line 142-147: The loop over req.query is writing array entries into the
params Map<string,string> without checking array contents; update the
Array.isArray branch (where params.set(key, value[0] as string) is used) to
first ensure value.length > 0 and typeof value[0] === "string" (or coerce safely
with String(value[0]) if you intend to accept non-strings) before calling
params.set; otherwise skip the key (or log/handle the empty/non-string case) so
you never insert undefined or invalid types into params.
---
Nitpick comments:
In `@src/aggregation/responses.ts`:
- Around line 1-17: Make ErrorResponse an abstract, immutable base by declaring
the class abstract and marking its instance properties message and error as
readonly so they cannot be mutated after construction; update the constructor
(the ErrorResponse constructor) to initialize these readonly properties (or use
readonly parameter properties) and keep toJSON() unchanged to return { error,
message }. Ensure references to ErrorResponse, message, error, constructor, and
toJSON are updated accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f4b6e5bf-67f8-43ad-9edf-631976b2a43f
📒 Files selected for processing (2)
src/aggregation/responses.tssrc/index.ts
Comment on lines
+142
to
147
| for (const [key, value] of Object.entries(req.query)) { | ||
| if (typeof value === "string") { | ||
| params.set(key, value); | ||
| } else if (Array.isArray(value)) { | ||
| params.set(key, value[0] as string); | ||
| } |
There was a problem hiding this comment.
Guard array query values before writing to Map<string, string>.
On Line 146, value[0] as string can write undefined (empty array) or non-string values. Add a runtime type check before params.set.
🛠️ Proposed fix
for (const [key, value] of Object.entries(req.query)) {
if (typeof value === "string") {
params.set(key, value);
} else if (Array.isArray(value)) {
- params.set(key, value[0] as string);
+ const first = value[0];
+ if (typeof first === "string") {
+ params.set(key, first);
+ }
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for (const [key, value] of Object.entries(req.query)) { | |
| if (typeof value === "string") { | |
| params.set(key, value); | |
| } else if (Array.isArray(value)) { | |
| params.set(key, value[0] as string); | |
| } | |
| for (const [key, value] of Object.entries(req.query)) { | |
| if (typeof value === "string") { | |
| params.set(key, value); | |
| } else if (Array.isArray(value)) { | |
| const first = value[0]; | |
| if (typeof first === "string") { | |
| params.set(key, first); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/index.ts` around lines 142 - 147, The loop over req.query is writing
array entries into the params Map<string,string> without checking array
contents; update the Array.isArray branch (where params.set(key, value[0] as
string) is used) to first ensure value.length > 0 and typeof value[0] ===
"string" (or coerce safely with String(value[0]) if you intend to accept
non-strings) before calling params.set; otherwise skip the key (or log/handle
the empty/non-string case) so you never insert undefined or invalid types into
params.
Contributor
Author
|
Wrong repository — production API is dpla/api (Scala/Akka). Closing. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
errorfield alongsidemessage(e.g.{"error": "unauthorized", "message": "Unauthorized"})FourHundredResponseandFiveHundredResponsebehind a sharedErrorResponsebase classUnauthorizedResponseclass; authMiddleware now uses it instead of inline literalsfor...in/for...ofbug inqueryParamsthat silently dropped all query parameters (iteratingObject.entries()withfor...inyields index strings, not[key, value]pairs)Error format
Before:
{"message": "Unauthorized"}After:
{"error": "unauthorized", "message": "Unauthorized"}All existing subclasses (
InvalidEmail,UnrecognizedParameters,InvalidParameter,TooManyIdentifiers,InternalErrorResponse) carry their own error codes unchanged.Test plan
GET /itemswith no API key →401 {"error": "unauthorized", "message": "Unauthorized"}GET /itemswith invalid API key →401 {"error": "unauthorized", "message": "Unauthorized"}GET /items?q=dogswith valid API key →200with resultsGET /items?badparam=x→400 {"error": "unrecognized_parameters", "message": "Unrecognized parameters: badparam"}🤖 Generated with Claude Code
Summary
This PR adds structured, machine-readable error codes to all JSON error responses throughout the API. All error responses now include both an
errorfield with a machine-readable code (e.g.,"unauthorized","invalid_email","internal_error") and a human-readablemessagefield.Key Changes
Response Structure (Public API)
ErrorResponsebase class that adds a structurederrorfield to all error responses{"error": "unauthorized", "message": "Unauthorized"}(previously just{"message": "Unauthorized"})InvalidEmail,UnrecognizedParameters,InvalidParameter,TooManyIdentifiers,InternalErrorResponse) now include consistent error codesAuth Middleware
UnauthorizedResponseclass (401 status) for authentication failures{ message: "Unauthorized" }responses with structuredUnauthorizedResponseinstances in both API key validation pathsres.status(r.errorCode).json(r)pattern for consistent error handlingBug Fix
queryParamsextraction where incorrect loop syntax withObject.entries()was causing query parameter values to be silently droppedAPI Impact
errorfield alongsidemessage.No Deployment Changes Required