Skip to content

Return JSON when IA->OL unlink request fails#11994

Open
jimchamp wants to merge 2 commits intointernetarchive:masterfrom
jimchamp:unlink-handler-return-type
Open

Return JSON when IA->OL unlink request fails#11994
jimchamp wants to merge 2 commits intointernetarchive:masterfrom
jimchamp:unlink-handler-return-type

Conversation

@jimchamp
Copy link
Collaborator

@jimchamp jimchamp commented Mar 3, 2026

Follows #11230

Updates record unlink handler such that it returns JSON when an error occurs.

Technical

Testing

Screenshot

Stakeholders

Copilot AI review requested due to automatic review settings March 3, 2026 04:06
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Updates the IA→OL unlink internal API handler to return JSON responses when errors occur, aligning error handling with API consumers expecting JSON payloads.

Changes:

  • Adds Content-Type: application/json headers to several web.HTTPError responses in /api/unlink.
  • Ensures some error cases (400/409/500) include JSON-encoded error payloads.

HMACToken.verify(digest, msg, "ia_sync_secret")
except (ValueError, ExpiredTokenError):
raise web.HTTPError("401 Unauthorized")
raise web.HTTPError("401 Unauthorized", {"Content-Type": "application/json"})
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

401 Unauthorized sets Content-Type: application/json but doesn't provide a JSON body (data=...). This can produce a response that advertises JSON but isn't valid JSON (and may still be the default error payload). To ensure clients can always parse errors, include a JSON data payload (consistent with other handlers in this file, e.g. the 404s that return data="{}").

Suggested change
raise web.HTTPError("401 Unauthorized", {"Content-Type": "application/json"})
raise web.HTTPError(
"401 Unauthorized",
{"Content-Type": "application/json"},
data=json.dumps({"error": "Unauthorized"}),
)

Copilot uses AI. Check for mistakes.
Comment on lines +991 to 994
raise web.HTTPError("401 Unauthorized", {"Content-Type": "application/json"})

ocaid, ts = msg.split("|")

Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

msg.split("|") will raise ValueError if the message contains more than one | (which HMACToken.verify permits, since it uses rsplit(..., maxsplit=1)). That would lead to an unhandled exception (likely a 500 / non-JSON response), undermining the goal of returning JSON on failures. Parse with rsplit('|', 1) (or otherwise validate the split) and convert any parsing errors into a JSON 400 Bad Request.

Copilot uses AI. Check for mistakes.

editions = [web.ctx.site.get(key) for key in edition_keys]
if len(editions) > 1:
raise web.HTTPError(
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

404 Not Found sets Content-Type: application/json but doesn't provide a JSON body (data=...). This can result in clients receiving a non-JSON body while attempting to parse JSON. Consider returning at least {} or an {"error": ...} payload (similar to other JSON 404s in this file).

Copilot uses AI. Check for mistakes.
Copy link
Collaborator

@RayBB RayBB left a comment

Choose a reason for hiding this comment

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

I was looking at this code today actually and wondering how we decide to return JSON or not. And I'm glad you're updating it to return JSON. This makes a lot of sense to me and will make it easier to do the migration for FastAPI.

@mekarpeles mekarpeles self-assigned this Mar 6, 2026
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.

4 participants