Return structured JSON objects for all error responses

[DominicBM]

Summary

All error responses previously returned a bare JSON string:

"Invalid or inactive API key."

They now return a structured object with a machine-readable error code alongside the human-readable message:

{"error": "invalid_api_key", "message": "Invalid or inactive API key."}

This makes error handling programmatic for API consumers and aligns with the WAF rate-limit error body already in production ({"error": "rate_limit_exceeded", ...}).

Error codes

Response	HTTP status	error code
Invalid/inactive API key	403	invalid_api_key
Record not found	404	not_found
Validation failure	400	bad_request
Key already exists	409	existing_key
Key disabled	409	disabled_key
Unexpected internal error	500	internal_error

Changes

Single change in Routes.scala: jsonEntity(message) → errorEntity(error, message). All response message text is unchanged.

Test plan

• GET /items with no API key → 403 {"error": "invalid_api_key", "message": "Invalid or inactive API key."}
• GET /items?page=abc with valid API key → 400 {"error": "bad_request", "message": "..."}
• GET /items/nonexistent-id → 404 {"error": "not_found", "message": "The record you are searching for could not be found."}
• Force an internal error → 500 {"error": "internal_error", "message": "..."}

🤖 Generated with Claude Code

This PR modifies the error response format for the DPLA API by replacing plain JSON string responses with structured JSON objects containing a machine-readable "error" code, a human-readable "message", and a "documentation" URL.

Key Changes

• File changed: src/main/scala/dpla/api/Routes.scala
• Replaced previous single-string JSON error payloads with errorEntity(error, message) which produces:
  {"error":"","message":"","documentation":"https://pro.dp.la/developers/responses#errors"}
• Error codes introduced/mapped:
  • 403 — invalid_api_key — "Invalid or inactive API key."
  • 404 — not_found — "The record you are searching for could not be found."
  • 400 — bad_request — validation failure messages (passed through)
  • 409 — existing_key — "There is already an API key for ..."
  • 409 — disabled_key — "The API key associated with email address has been disabled..."
  • 500 — internal_error — "There was an unexpected internal error. Please try again later."
• Implementation is localized to Routes.scala (private helper replaced and private HttpResponse values updated). Message text preserved; JSON shape extended and documentation URL added.
• Tests: plan includes verifying 403, 400, 404, and 500 responses return the new structured JSON bodies.

Impact Assessment

• Public-facing API response shape change: Yes. All error responses now return a structured JSON object with an "error" code, "message", and "documentation" URL. Clients that parse error responses must update parsing/handling accordingly.
• No endpoints added/removed.
• No changes to environment variables, AWS Secrets Manager keys, or database migrations.
• No shared infrastructure (CodePipeline/CodeBuild/ECS/IAM) modifications.
• No manual pipeline trigger or special ECS redeployment required beyond the normal release/deploy process.
• Security implications: None in authentication/credential handling; change is purely response formatting (but clients depending on previous string-only error bodies must update to avoid parsing issues).

Commits

• Single commit: "Add documentation field to all error responses" (adds documentation URL and structured error format; co-authored).

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 34b90e67-1645-43b5-9052-b4aecfa70128

📥 Commits

Reviewing files that changed from the base of the PR and between 4cd9b3f and c63b342.

📒 Files selected for processing (1)

• src/main/scala/dpla/api/Routes.scala

🚧 Files skipped from review as they are similar to previous changes (1)

• src/main/scala/dpla/api/Routes.scala

---

Walkthrough

Replaced the private jsonEntity(message: String) helper with errorEntity(error: String, message: String) in Routes.scala. Updated predefined HttpResponse values for 500, 404, 403, 400, and 409 to emit JSON objects with error, message, and a fixed documentation URL.

Changes

Cohort / File(s)	Summary
HTTP Error Response Refactor src/main/scala/dpla/api/Routes.scala	Replaced jsonEntity with errorEntity(error, message) and updated predefined HttpResponse instances: 500->internal_error, 404->not_found, 403->invalid_api_key, 400->bad_request, 409->existing_key/disabled_key. Responses now include error, message, and a documentation URL.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Possibly related PRs

• Reject malformed API keys before spawning actors or ES queries #98 — Adds early-return ForbiddenFailure for malformed API keys and relies on the updated 403 invalid_api_key response format.

🚥 Pre-merge checks | ✅ 2

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Return structured JSON objects for all error responses' directly and accurately summarizes the main change: replacing bare JSON strings with structured JSON error objects containing error codes and messages across all API error endpoints.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch json-error-responses

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

src/main/scala/dpla/api/Routes.scala (1)

557-560: Add assertions for the new error-body contract in end-to-end tests.

This helper defines the new API contract (error + message), but current related tests only check status/content-type (see PostgresUnauthorizedTest.scala Line 36-70, InvalidParamsTest.scala Line 46-77, ElasticSearchErrorTest.scala Line 29-47). Please assert both fields to prevent silent regressions.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/scala/dpla/api/Routes.scala` around lines 557 - 560, Tests only
assert status/content-type but not the new error-body contract produced by
errorEntity(error: String, message: String); update the end-to-end tests (e.g.,
PostgresUnauthorizedTest.scala, InvalidParamsTest.scala,
ElasticSearchErrorTest.scala) to parse the response JSON and assert that it
contains both "error" and "message" keys and that their values match the
expected error string and human-readable message produced by the API for each
case. Locate where the test currently checks status/content-type and add JSON
parsing + assertions for the two fields to prevent regressions against the
errorEntity contract.

🤖 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/main/scala/dpla/api/Routes.scala`:
- Around line 598-613: Both branches expose distinct machine-readable codes
("existing_key" vs "disabled_key") which enable account-state enumeration;
update the error responses so both conflict cases return the same code and
message. Replace the errorEntity call that currently uses "existing_key" and the
one inside disabledKeyResponse that uses "disabled_key" with a unified code
(e.g., "key_conflict") and a single generic message (e.g., "An API key already
exists for that address; please contact support.") so callers cannot distinguish
disabled vs existing keys; adjust the errorEntity invocations where these
strings are passed to enforce the uniform response.

---

Nitpick comments:
In `@src/main/scala/dpla/api/Routes.scala`:
- Around line 557-560: Tests only assert status/content-type but not the new
error-body contract produced by errorEntity(error: String, message: String);
update the end-to-end tests (e.g., PostgresUnauthorizedTest.scala,
InvalidParamsTest.scala, ElasticSearchErrorTest.scala) to parse the response
JSON and assert that it contains both "error" and "message" keys and that their
values match the expected error string and human-readable message produced by
the API for each case. Locate where the test currently checks
status/content-type and add JSON parsing + assertions for the two fields to
prevent regressions against the errorEntity contract.

🪄 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: 246f38e8-7558-4de7-a2e8-225ff86cb84a

📥 Commits

Reviewing files that changed from the base of the PR and between bd2bc45 and b791bbb.

📒 Files selected for processing (1)

• src/main/scala/dpla/api/Routes.scala

[coderabbitai]

♻️ Duplicate comments (1)

src/main/scala/dpla/api/Routes.scala (1)

599-617: ⚠️ Potential issue | 🟠 Major

Unify 409 conflict responses to avoid account-state enumeration.

Line 603 (existing_key) and Line 613 (disabled_key) expose distinguishable account states via both code and message. Returning a single conflict code/message for both branches reduces enumeration risk.

🔐 Suggested mitigation

   private def existingKeyResponse(email: String): HttpResponse =
     HttpResponse(
       Conflict,
       entity = errorEntity(
-        "existing_key",
-        s"There is already an API key for $email" +
-          ". We have sent a reminder message to that address."
+        "api_key_conflict",
+        "Unable to create a new API key for this email. Please check your inbox or contact DPLA."
       )
     )

   private def disabledKeyResponse(email: String): HttpResponse =
     HttpResponse(
       Conflict,
       entity = errorEntity(
-        "disabled_key",
-        s"The API key associated with email address $email" +
-          " has been disabled. If you would like to reactivate it, " +
-          "please contact DPLA."
+        "api_key_conflict",
+        "Unable to create a new API key for this email. Please check your inbox or contact DPLA."
       )
     )

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/scala/dpla/api/Routes.scala` around lines 599 - 617, Both
existingKeyResponse and disabledKeyResponse leak distinct account state via
differing Conflict messages; change them to return the same HTTP status and
identical errorEntity payload to avoid account-state enumeration. Locate the
methods existingKeyResponse and disabledKeyResponse and replace their specific
messages with a single generic Conflict response (e.g., same error code and
neutral message like "An API key for this address already exists or has been
disabled; please contact DPLA.") so both branches produce the exact same status
and entity.

🧹 Nitpick comments (1)

src/main/scala/dpla/api/Routes.scala (1)

557-564: Add contract assertions for the new error JSON shape.

This introduces a new response contract (error, message, documentation), but existing E2E coverage (e.g., src/test/scala/dpla/api/v2/endToEnd/PostgresUnauthorizedTest.scala around Line 36 onward) only verifies status/content type. Please add body assertions so schema regressions are caught.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/scala/dpla/api/Routes.scala` around lines 557 - 564, The new error
JSON shape returned by errorEntity(error: String, message: String) adds fields
("error","message","documentation") but E2E tests (e.g.,
PostgresUnauthorizedTest.scala) only assert status and content-type; update
those tests to parse the response body as JSON and assert the presence and
values of these fields (check that "error" equals the expected error
code/string, "message" contains the expected message, and "documentation" equals
"https://pro.dp.la/developers/responses#errors"), and apply the same body
assertions to any other end-to-end tests that exercise error responses so schema
regressions are caught.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@src/main/scala/dpla/api/Routes.scala`:
- Around line 599-617: Both existingKeyResponse and disabledKeyResponse leak
distinct account state via differing Conflict messages; change them to return
the same HTTP status and identical errorEntity payload to avoid account-state
enumeration. Locate the methods existingKeyResponse and disabledKeyResponse and
replace their specific messages with a single generic Conflict response (e.g.,
same error code and neutral message like "An API key for this address already
exists or has been disabled; please contact DPLA.") so both branches produce the
exact same status and entity.

---

Nitpick comments:
In `@src/main/scala/dpla/api/Routes.scala`:
- Around line 557-564: The new error JSON shape returned by errorEntity(error:
String, message: String) adds fields ("error","message","documentation") but E2E
tests (e.g., PostgresUnauthorizedTest.scala) only assert status and
content-type; update those tests to parse the response body as JSON and assert
the presence and values of these fields (check that "error" equals the expected
error code/string, "message" contains the expected message, and "documentation"
equals "https://pro.dp.la/developers/responses#errors"), and apply the same body
assertions to any other end-to-end tests that exercise error responses so schema
regressions are caught.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 33e54027-d8c0-42ef-bbbd-81e0ac5e4ce4

📥 Commits

Reviewing files that changed from the base of the PR and between b791bbb and 4cd9b3f.

📒 Files selected for processing (1)

• src/main/scala/dpla/api/Routes.scala