fix(lightspeed): add return statement to address unhandled error in /v1/feedback #3070
fix(lightspeed): add return statement to address unhandled error in /v1/feedback #3070Jdubrick wants to merge 3 commits intoredhat-developer:mainfrom
Conversation
Jdubrick
requested review from
a team,
karthikjeeyar,
rohitkrai03 and
yangcao77
as code owners
Code Review by Qodo🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)
Great, no issues found!Qodo reviewed your code and found no material issues that require review |
Changed Packages
|
Review Summary by QodoFix unhandled error in lightspeed /v1/feedback handler
WalkthroughsDescription• Add missing return statement in /v1/feedback error handler • Prevents unhandled error fallthrough after 500 response • Add test case for upstream server error handling Diagramflowchart LR
A["POST /v1/feedback"] --> B["Upstream 500 Error"]
B --> C["Send 500 Response"]
C --> D["Add return statement"]
D --> E["Prevent Fallthrough"]
File Changes1. workspaces/lightspeed/plugins/lightspeed-backend/src/service/router.ts
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3070 +/- ##
=======================================
Coverage 57.28% 57.28%
=======================================
Files 2067 2067
Lines 63528 63529 +1
Branches 16583 16585 +2
=======================================
+ Hits 36390 36395 +5
+ Misses 26704 26700 -4
Partials 434 434
*This pull request uses carry forward flags. Click here to find out more. Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
gabemontero
left a comment
gabemontero
left a comment
There was a problem hiding this comment.
one question (answers might lead to an actual ask for change)
| error: errormsg, | ||
| }); | ||
|
|
||
| return; |
There was a problem hiding this comment.
you didn't touch setting of the status (to 500) above, nor the setting of the status to fetchResponse.status below, but these 2 bits make me ask
- is fetchResponse.ok false but fetchResponse.status a non error return code ?
- if fetchResponse.status is an error return code, why do you want to return 500 instead of the upstream error code
There was a problem hiding this comment.
I think in terms of any error coming from the Lightspeed Core container we are just treating it as 500. The question could be raised to tune this a bit better in the future. wdyt?
for fetchResponse.ok if it is in the 200s it returns true, so we are catching all errors from that check, we could use fetchResponse.status but then we'd need to check for all 2xx codes to say its good I think, .ok is a catch-all for them
There was a problem hiding this comment.
If you to convert the various 200s to just 200 I'm ok with that.
But traditionally converting all 300s 400s etc to 500 loses info.
And not sure why we need to wait until the future. What is the concern for addressing these basics now?
There was a problem hiding this comment.
ok @Jdubrick and I hoped on a quick call ... leaving router.ts change the same, but independent of the issue which drove him to fix things by adding the return, line 529 was non-optimal, and we'll fix that too, where instead of returning 500, we'll use fetchResponse.status
he'll then tweak the unit tests to adjust
There was a problem hiding this comment.
@gabemontero I updated it in this commit b3589db and as part of that fixed the other handlers that were hardcoding 500 as well.
…handled error Signed-off-by: Jordan Dubrick <jdubrick@redhat.com>
Signed-off-by: Jordan Dubrick <jdubrick@redhat.com>
Signed-off-by: Jordan Dubrick <jdubrick@redhat.com>
Jdubrick
force-pushed
the
fix-lightspeed-feedback-error-handling
branch
from
b3589db to
2d573c9
Compare
|
2 participants
Hey, I just made a Pull Request!
✔️ Checklist
Adds a
returnstatement to the/v1/feedbackhandler when it gets a 500 error, it was the only one missing this out of all the route handlers.Issue: https://redhat.atlassian.net/browse/RHIDP-13063