fix: prevent undefined error codes in settle#7276
Conversation
…atus codes Replace array indexing logic with conditional check to ensure proper error code assignment for all HTTP status code ranges. Previously, the error code selection used: [ERR_BAD_REQUEST, ERR_BAD_RESPONSE][Math.floor(status / 100) - 4] This produced undefined for status codes outside 4xx/5xx ranges (1xx, 2xx, 3xx, 6xx+), which could break error handling when users customize validateStatus to reject non-standard status codes. Now uses: status >= 400 && status < 500 ? ERR_BAD_REQUEST : ERR_BAD_RESPONSE This ensures: - 4xx codes → ERR_BAD_REQUEST - All other error codes → ERR_BAD_RESPONSE
This comment was marked as spam.
This comment was marked as spam.
1 similar comment
This comment was marked as spam.
This comment was marked as spam.
|
Thanks @VeerShah41, the bug is real, and the diagnosis is right. The current One semantic thing worth flagging on the fix itself. Today The bigger gap is in tests. There's no settled spec today One last thing, please don't ping unrelated GitHub users on PRs here. They're not maintainers of this repo, and ad-hoc pings tend to get muted rather than acted on. The maintainers do see open PRs. |
Cover the boundary conditions that were previously untested: - 4xx status codes → ERR_BAD_REQUEST - 5xx status codes → ERR_BAD_RESPONSE - Non-4xx/5xx codes rejected via custom validateStatus → ERR_BAD_RESPONSE (defined, not undefined) These tests would have caught the original array-indexing bug where Math.floor(status/100) - 4 returned negative/out-of-range indices for 1xx, 2xx, 3xx, and 6xx+ status codes, resulting in undefined error.code.
|
Hey @jasonsaayman sorry for the unrelated pings and the spam, that was genuinely careless on my part. I didn't check if those people were actually maintainers before tagging them .Won't do that again. I've added the tests you mentioned . I'll be upfront ,I used an LLM to help me generate them since I'm still learning how to write good test cases though on my part, I did go through each one and understand what it's checking , just wanted to be transparent about that . The tests cover 4xx ➡️ ERR_BAD_REQUEST, 5xx ➡️ ERR_BAD_RESPONSE ,and the custom validateStatus edge cases where error.code was coming back as undefined before the fix . Thanks for the detailed feedback it actually helped me understand the codebase a lot better . |
…atus codes
Replace array indexing logic with conditional check to ensure proper error code assignment for all HTTP status code ranges.
Previously, the error code selection used:
[ERR_BAD_REQUEST, ERR_BAD_RESPONSE][Math.floor(status / 100) - 4]
This produced undefined for status codes outside 4xx/5xx ranges (1xx, 2xx, 3xx, 6xx+), which could break error handling when users customize validateStatus to reject non-standard status codes.
Now uses:
status >= 400 && status < 500 ? ERR_BAD_REQUEST : ERR_BAD_RESPONSE
This ensures:
Instructions
Please read and follow the instructions before creating and submitting a pull request:
Describe your pull request here.