Closed bartoszherba closed 1 year ago
@bartoszherba LGTM. One question though.
I know in the past the subject of this default error code has been coming back like a boomerang. It was 500 at first, then we've changed it to 200 and now back to 500 again. At the end of the day, I think 500 is the right choice but wanted to make sure: are you 100% aware of the history of the previous changes (i.e. have you read PRs such as this one or tickets such as this one?
@lsliwaradioluz I was also thinking about the 400 error-code. The main goal here is to mask the original error and return something generic to the front. In fact, I do not really know how to handle this if we are not able to get the actual code, eg. if the apollo client throws a network error, there is no status code even though the ecom-server
(CT for example) is using the code.
If you take a look at the task, there is a proposition on how it should be solved, and I am aligned with that proposition. I believe that status 200 in case of an error is not appropriate. We can always introduce our own 5xx code for that specific situation but that will require an explanation in docs.
Description
Previously, if the middleware was not able to recognize the error status it returned 200 which is not a proper code to mark an issue. Also, the message was not masked and could expose vulnerable data on the front end. Now the default status code fallback is set to 500, the error will be logged only on the server side and the message sent to the frontend will be masked.
Related Issue
IN-342
Types of changes
Checklist:
Changelog
Tests
Code standards
Docs