fix(HMS-2933): Improve some HTTP error codes #752
Conversation
avitova
force-pushed
the
BugFix2933
branch
2 times, most recently
from
6a50bd2 to
a9850f9
Compare
|
Nice, this is what we probably should do with all the id params. 🧡 |
|
How to test this: |
Depends on how much is this in a rush. I do not mind doing a followup, but I can include it in here as well. |
No real rush ;) We are on target with SLO now, so we can do it properly and do follow-up :) |
internal/middleware/id_validation.go
Outdated
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
Do not assume that primary keys in databases are higher than 0, in fact, when a database reaches the maximum value it starts from -MAX back to zero. This is at least behavior of postgres. So I would drop this check only keeping ParseUint and checking for error. In addition, I would expect this function to actually error out when called with negative number anyway.
I would not bother too much about the error message, we have properly documented what is expected, just returning "invalid id" is enough.
There was a problem hiding this comment.
Now that I think about it, you should not use ParseUint because our primary keys are int64, just parse a regular 64 signed number and report errors.
There was a problem hiding this comment.
Yeah, sorry. There was even an invalid string, this regex does not check against 0, but numbers overall. The second check is against numbers that might overflow.
The second check would not fail for negative numbers. So I think we can keep both.
|
Any updates @avitova ? Thanks. |
| if resp.JSON404 != nil || resp.JSON400 != nil { | ||
| if resp.JSON404 != nil { | ||
| logger.Warn().Bytes("body", resp.Body).Int("status", resp.StatusCode()).Msg("Get authentication from sources returned 4xx") | ||
| return nil, fmt.Errorf("get source authentication call: %w", clients.ErrNotFound) | ||
| } | ||
| if resp.JSON400 != nil { | ||
| logger.Warn().Bytes("body", resp.Body).Int("status", resp.StatusCode()).Msg("Get authentication from sources returned 4xx") | ||
| return nil, fmt.Errorf("get source authentication call: %w", clients.ErrBadRequest) | ||
| } | ||
|
|
There was a problem hiding this comment.
These two would be the most common cases - sources return 404, 400 or 2xx payloads. I think any other case could be a larger failure that we might want to know about anyway.
Therefore, after calling GetAuthentication, I feel like we want to check for ErrBadRequest and ErrNotFound, and the rest, like ErrNoResponseData, ErrUnexpectedBackendResponse should be left as is. :)
|
/retest |
|
payloads.NewClientError takes care of the errors codes pretty well, as I have found out. |
| } | ||
| if resp.JSON400 != nil { | ||
| logger.Warn().Bytes("body", resp.Body).Int("status", resp.StatusCode()).Msg("Get authentication from sources returned 4xx") | ||
| return nil, fmt.Errorf("get source authentication call: %w", clients.ErrBadRequest) |
There was a problem hiding this comment.
The missing return at this resp.JSON404 != nil || resp.JSON400 != nil branch was the cause for the invalid returns.
The next if resp.JSON200 == nil returned ErrUnexpectedBackendResponse.
We are now validating id, so this would not happen for invalid ids at all. For 404 and 400 we are now able to return the correct status.
internal/payloads/validation/id.go
Outdated
|
|
||
| var ( | ||
| idValidation = regexp.MustCompile("^[0-9]+$") | ||
| ErrInvalidId = fmt.Errorf("invalid id") |
There was a problem hiding this comment.
| ErrInvalidId = fmt.Errorf("invalid id") | |
| ErrInvalidId = errors.New("invalid id") |
|
Yeah, and you have correctly reminded me that this has the exact handling, and we should return the error even for 404 and 400 at those two places as well. @ezr-ondrej |
Hopefully this should be okay.
I am not entirely sure, if ParseUint would prevent out of range error. Maybe sources use ParseInt, and I just added a bigger range for them.
Also, I think, that they validate with different regex: https://github.com/RedHatInsights/sources-api-go/blob/4a3475e59b8f515dbda70c423826b2a3e4d07cb6/middleware/id_validation.go#L14, and I do not think that this is correct tbh, so I did this my way.
This is still WIP, but I wanted you to be able to comment on this in advance.