BCDA-7212: Make 500 response errors properly reflect the error they encounter #174
Conversation
Updates the API to have malformed requests error to BadRequest when requests are malformed, instead of erroring to a 500. Gives more feedback on error messages.
|
These look good, what are your thoughts on the others in the file that generally have an error of starting with "failed to" X and then have a BadRequest response? At least some of those appear to be transient / more appropriate as 5XX codes. |
|
@alex-dzeda that's a good question. Some of them are okay as 500, such as marshaling responses, but unmarshaling should probably be a BadRequest. |
Can you look at those and see if you'd change any others? For example, looking at bcda-ssas-app/ssas/service/admin/api.go Line 52 in 7ed74be |
Updated a handful of error states to correctly reflect what is happening with the service.
| @@ -272,7 +272,7 @@ func updateSystem(w http.ResponseWriter, r *http.Request) { | |||
| _, err = ssas.UpdateSystem(r.Context(), id, v) | |||
| if err != nil { | |||
| logger.Errorf("failed to update system; %s", err) | |||
| service.JSONError(w, http.StatusBadRequest, http.StatusText(http.StatusBadRequest), "failed to update system") | |||
| service.JSONError(w, http.StatusNotFound, http.StatusText(http.StatusNotFound), "failed to update system") | |||
There was a problem hiding this comment.
This error at least partially rolls up to the above image; I think it may be better to return the contextually-relevant http status, since this error appears to have multiple potential options for the correct status
There was a problem hiding this comment.
eg from the above method alone, I see a case where a 400 or a 404 is appropriate, depending on the error
There was a problem hiding this comment.
Had a conversation with Lauren. Rolling new conditional logic for custom response handling based on downstream errors have involved extra scope that is outside this specific ticket. Will create a new ticket for the custom error handling outside of malformed requests.
| @@ -308,7 +308,7 @@ func deleteGroup(w http.ResponseWriter, r *http.Request) { | |||
| err := ssas.DeleteGroup(r.Context(), id) | |||
| if err != nil { | |||
| logger.Errorf("failed to delete group; %s", err) | |||
| service.JSONError(w, http.StatusBadRequest, http.StatusText(http.StatusBadRequest), "failed to delete group") | |||
| service.JSONError(w, http.StatusNotFound, http.StatusText(http.StatusNotFound), "failed to delete group") | |||
There was a problem hiding this comment.
Comment above applies here, too
| @@ -592,7 +598,7 @@ func registerIP(w http.ResponseWriter, r *http.Request) { | |||
| system, err := ssas.GetSystemByID(r.Context(), systemID) | |||
| if err != nil { | |||
| logger.Errorf("failed to retrieve system; %s", err) | |||
| service.JSONError(w, http.StatusNotFound, http.StatusText(http.StatusBadRequest), "Invalid system ID") | |||
| service.JSONError(w, http.StatusNotFound, http.StatusText(http.StatusNotFound), "Invalid system ID") | |||
🎫 Ticket
https://jira.cms.gov/browse/BCDA-7212
🛠 Changes
Updated error status on certain endpoint failures to properly describe what went wrong.
ℹ️ Context for reviewers
Some 500 errors were being sent incorrectly for a Bad Requests.
✅ Acceptance Validation
(How were the changes verified? Did you fully test the acceptance criteria in the ticket? Provide reproducible testing instructions and screenshots if applicable.)
🔒 Security Implications
If any security implications apply, add Jason Ashbaugh (GitHub username: StewGoin) as a reviewer and do not merge this PR without his approval.