Repository navigation
Conversation
postgrestRequest and the /spec resource parsed every response as JSON without checking the status, so a PostgREST error body such as an invalid API key was returned as a successful result and an empty or non-JSON error body threw a SyntaxError. Throw an error with the status and body when the response is not ok. Fixes supabase#540
mangeshraut712
left a comment
There was a problem hiding this comment.
Reading the body as text, checking response.ok first and parsing afterwards is the right shape, and the 204 → { result: null } case is a nice catch now that prefer: return=representation isn't always honoured. Two small follow-ups below.
| throw new Error(`PostgREST ${response.status}: ${text}`); | ||
| } | ||
|
|
||
| return { result: text ? JSON.parse(text) : null }; |
There was a problem hiding this comment.
The issue's second symptom ("a non-JSON body throws a SyntaxError that hides the status") is fixed for error statuses, but it can still happen on 2xx. A common example is a wrong apiUrl that points at something returning a 200 HTML page (a dashboard, a proxy splash page, a trailing /rest/v1 missing). JSON.parse(text) then throws Unexpected token '<' with no status or URL. Wrapping the parse would make that case as debuggable as the error-status one, e.g.:
let parsed: unknown;
try { parsed = JSON.parse(text); }
catch { throw new Error(`PostgREST ${response.status}: expected JSON, got ${response.headers.get('content-type') ?? 'unknown content-type'}: ${text.slice(0, 200)}`); }The same applies to JSON.parse(text) in the /spec read above (L64).
| return jsonResourceResponse(uri, result); | ||
| const text = await response.text(); | ||
| if (!response.ok) { | ||
| throw new Error(`PostgREST ${response.status}: ${text}`); |
There was a problem hiding this comment.
The new test file only exercises postgrestRequest; the /spec resource's error path isn't covered. The stub's fallback (404, empty body) would make an easy case via client.readResource({ uri: 'postgrest:///spec' }) (or whatever URI resources('postgrest', …) generates). That would also show what an empty error body looks like: currently it's PostgREST 404: with nothing after the colon, so appending response.statusText when text is empty might be friendlier.
… error bodies Wrap JSON.parse so a 200 response that is not JSON reports its status and content type, name the status when an error response has an empty body, and cover the /spec resource error path.
|
Thanks for the review. Both are in 8b13e06: a successful body that is not JSON now reports its status and content type, for the tool and for |
What kind of change does this PR introduce?
Bug fix
What is the current behavior?
Fixes #540
postgrestRequestand the/specresource parse every response as JSON without checking the status. A PostgREST error body is returned as a normal result withisErrorunset, and an empty or non-JSON body throws aSyntaxErrorthat hides the status.What is the new behavior?
Both go through a
readJsonBodyhelper. It throwsPostgREST <status>: <body>whenresponse.okis false, withstatusTextin place of an empty body and no suffix when both are empty. A successful body that is not JSON throwsPostgREST <status>: expected JSON, got <content-type>: <first 200 characters>.postgrestRequestreturns{ result: null }for an empty successful body (204).GET /no_such_table?select=*isError: false,{"result":{"code":"PGRST205",…}}isError: true,PostgREST 404: {"code":"PGRST205",…}GET /no_such_table?select=*&bad=isError: false,{"result":{"code":"PGRST100",…}}isError: true,PostgREST 400: {"code":"PGRST100",…}GET /with an invalid--apiKeyisError: false,{"result":{"message":"Invalid API key",…}}isError: true,PostgREST 401: {"message":"Invalid API key",…}{"result":…}No change to tool names, descriptions, or input and output schemas.
How to Review
Status check
packages/mcp-server-postgrest/src/server.ts/specresource read andpostgrestRequestboth callreadJsonBody, which holds the status check and the JSON parse.Tests
packages/mcp-server-postgrest/src/server.http-errors.test.tspostgrestRequestand reads the/specresource against a localhttpstub: a 400 PostgREST error, a 500 HTML body, a 200 JSON body, a 204, a 200 HTML body and an empty 404. It does not need the local Supabase stack thatserver.test.tsuses.Verification
npx vitest run src/server.http-errors.test.ts: 8 passed.npx tsc --noEmitandpnpm format:check: clean.isError: false. After, they returnisError: truewith 404, 400 and 401.