-
-
Notifications
You must be signed in to change notification settings - Fork 599
fix: Returned error for 5xx server response has incorrect format #2755
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Returned error for 5xx server response has incorrect format #2755
Conversation
🚀 Thanks for opening this pull request! |
📝 WalkthroughWalkthroughUpdates RESTController’s retry-failure handling to parse and reject with JSON error objects when retries exhaust on 5xx responses. Tests updated: one test now validates error shape; a new test covers consecutive 5xx errors and verifies code/message propagation. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant RC as RESTController
participant F as fetch()
participant S as Server
C->>RC: request(options)
loop Retry on failure
RC->>F: fetch(url, opts)
F->>S: HTTP request
S-->>F: Response (5xx or error)
alt Connection error or 5xx
RC->>RC: increment retries
RC-->>C: retry (if below limit)
else Success (2xx)
F-->>RC: Response OK
RC-->>C: Resolve(data)
end
end
rect rgba(255, 235, 205, 0.35)
note over RC: Retries exhausted
RC->>F: last response available
alt Non-connection 5xx response
RC->>F: response.json()
F-->>RC: { code, message, ... }
RC-->>C: Reject(parsed error object)
else Connection error
RC-->>C: Reject(connection error)
end
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## alpha #2755 +/- ##
=======================================
Coverage 99.88% 99.88%
=======================================
Files 64 64
Lines 6219 6220 +1
Branches 1472 1488 +16
=======================================
+ Hits 6212 6213 +1
Misses 7 7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/RESTController.ts (1)
217-218
: LGTM! Improved error handling for exhausted 5xx retries.Parsing the response as JSON before rejection makes 5xx error handling consistent with 4xx errors (lines 196-198) and ensures the error object has the expected shape (
code
,error
/message
properties) forhandleError
to transform it into a ParseError.Optional: Consider handling malformed JSON responses.
If the server returns a 5xx error with invalid JSON,
response.json()
will throw and be caught by the outer catch block (line 223), but the resulting error may not have the expected shape. While this is acceptable (the application won't crash), you could optionally add explicit handling:} else { // After the retry limit is reached, fail - const error = await response.json(); - promise.reject(error); + try { + const error = await response.json(); + promise.reject(error); + } catch (parseError) { + promise.reject({ + code: ParseError.INTERNAL_SERVER_ERROR, + error: `Server error (status ${status})`, + }); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/RESTController.ts
(1 hunks)src/__tests__/RESTController-test.js
(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/__tests__/RESTController-test.js (1)
src/CoreManager.ts (1)
RESTController
(135-145)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build (Node 20, 20.15.1)
- GitHub Check: build (Node 22, 22.4.1)
- GitHub Check: build (Node 18, 18.20.4)
🔇 Additional comments (2)
src/__tests__/RESTController-test.js (2)
87-101
: LGTM! Test properly validates error object shape.The updated test now correctly:
- Expects 3 assertions (line 87)
- Mocks all 5xx responses with proper JSON bodies containing
code
anderror
fields (lines 89-93)- Validates that
fetchError.code
andfetchError.error
are present and correct (lines 100-101)This ensures the error object returned by
ajax()
has the expected structure when retries are exhausted.
183-198
: LGTM! Good test coverage for 5xx error propagation through the request pipeline.This test verifies that 5xx errors, after exhausting retries, properly propagate through
request()
→handleError()
→ParseError
. The test correctly validates:
error.code
is preserved (line 195)error.message
contains the server error message (line 196)Note: The test uses
error.message
(noterror.error
) becausehandleError
transforms the raw error object into aParseError
instance, which stores theerror
field as itsmessage
property.
@coderabbitai is this a breaking change? Suggest a an angular commit message as PR title. |
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed:
Length of output: 583 @mtrezza: Yes, this is a breaking change. Breaking Change AnalysisThe PR changes the error rejection format for 5xx errors (after retry limit is reached):
This breaks any existing error handling code that expects a Example of breaking code: try {
await Parse.Cloud.run("someFunction");
} catch (error) {
// This will now fail - error is no longer a Response object
const status = error.status;
const body = await error.json();
} Suggested Angular Commit MessagesWith breaking change indicator:
Alternative (shorter):
Note: The |
@Moumouls What's the story behind this issue here? Why do we need this fix? |
## [7.0.1-alpha.1](7.0.0...7.0.1-alpha.1) (2025-10-15) ### Bug Fixes * Returned error for 5xx server response has incorrect format ([#2755](#2755)) ([16a34d9](16a34d9))
🎉 This change has been released in version 7.0.1-alpha.1 |
## [7.0.1](7.0.0...7.0.1) (2025-10-15) ### Bug Fixes * Returned error for 5xx server response has incorrect format ([#2755](#2755)) ([16a34d9](16a34d9))
🎉 This change has been released in version 7.0.1 |
Pull Request
Issue
Parse SDK V7 introduce a regression about 500 errors. The issue was introduced in #2503.
Approach
Fix to so that server 500 responses are returned in same format as pre v7 release. Even though this is technically a breaking change compared to Parse JS SDK 7.0.0, we don't release it as such, as it is a correction of the accidental breaking change released in 7.0.0.