Skip to content

fix(node): surface API errors from redirect() - #1900

Open
HarshMN2345 wants to merge 2 commits into
mainfrom
fix/node-redirect-error
Open

fix(node): surface API errors from redirect()#1900
HarshMN2345 wants to merge 2 commits into
mainfrom
fix/node-redirect-error

Conversation

@HarshMN2345

@HarshMN2345 HarshMN2345 commented Sep 11, 2026

Copy link
Copy Markdown
Member

Summary

Client.redirect() in the Node SDK threw Invalid redirect for any response other than a 301/302 and dropped the body, so a failing createOAuth2Token (invalid key, unknown project, missing scope) gave no hint of the cause. It now reads 4xx/5xx bodies the way call() does, so the exception carries the server's message, type and response. Other non-redirect statuses still throw Invalid redirect.

The dotnet, kotlin, python and php clients already surface the body on this path.

Part of appwrite/appwrite#8090.

Verification

createOAuth2Token against a local Appwrite with a real project and an invalid API key. The patched client is node-appwrite 29.0.0 with the same change applied to dist/client.js.

Client Server Exception
node-appwrite 29.0.0 main Invalid redirect, type ""
patched main message is the HTML error page
patched appwrite/appwrite#13625 The current user is not authorized to perform the requested action., type user_unauthorized

redirect() threw "Invalid redirect" for any response other than a 301 or
302 and dropped the body. Read 4xx and 5xx bodies the way call() does so
the exception carries the server's message, type and response.
@greptile-apps

greptile-apps Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 5/5

The implementation appears safe to merge, although the redirect error behavior still lacks the observable-behavior coverage requested in the existing review thread.

Fix All in Claude CodeFindings

  1. P2 Redirect errors lack coverage
Fix with agent prompt
### Issue 1
templates/node/src/client.ts.twig:621-641
The new redirect error behavior has no behavioral coverage. Existing redirect tests exercise only successful 301/302 responses, while existing 4xx/5xx tests use `call()`. Add observable-behavior tests proving that JSON and plain-text redirect failures preserve the expected message, type, status, and response; otherwise this fix can regress undetected.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Summary

  • Reads redirect error response bodies and propagates their message, type, status, and raw response.
  • Falls back to the raw body when a JSON-labeled error response is malformed.
  • Preserves existing handling for successful redirects and other non-redirect statuses.

Reviews (2) · Last reviewed commit: "fix(node): tolerate malformed JSON in re..."

Comment thread templates/node/src/client.ts.twig Outdated
Comment on lines +621 to +635
if (400 <= response.status) {
const responseText = await response.text();
let data: any = { message: responseText };
if (
response.headers.get('content-type')?.includes('application/json')
) {
data = JSONbig.parse(responseText);
}
throw new {{spec.info.title | caseUcfirst}}Exception(
data?.message,
response.status,
data?.type,
responseText,
);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 Redirect errors lack coverage

The new redirect error behavior has no behavioral coverage. Existing redirect tests exercise only successful 301/302 responses, while existing 4xx/5xx tests use call(). Add observable-behavior tests proving that JSON and plain-text redirect failures preserve the expected message, type, status, and response; otherwise this fix can regress undetected.

Prompt To Fix With AI
This is a comment left during a code review.
Path: templates/node/src/client.ts.twig
Line: 621-635

Comment:
**Redirect errors lack coverage**

The new redirect error behavior has no behavioral coverage. Existing redirect tests exercise only successful 301/302 responses, while existing 4xx/5xx tests use `call()`. Add observable-behavior tests proving that JSON and plain-text redirect failures preserve the expected message, type, status, and response; otherwise this fix can regress undetected.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Fix in Claude Code Fix in Codex

Fall back to the raw body as the exception message when a 4xx/5xx
response is labelled JSON but does not parse, and wrap the content-type
check to satisfy Prettier.
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.

1 participant