Cleanup bad request exception type #4777
Conversation
There was a problem hiding this comment.
Left some comments but didn't annotate every situations since there were a lot of the same question/suggestion/comment.
- I think anywhere we do not return the original error, we should make sure to slog it prior to returning. We want to have the actual error somewhere.
- There's a few places we BadRequestException that possibly should be a different exception type
- Some questions on how we define our custom error types - I think the key here is to be consistent across all of them (assuming that's possible)
In hindsight, possibly just having single error param might have been best. We end up doing fmt.Sprintf() anyway in most cases so not much different than fmt.Errorf() lol.
| return exceptions.NewBadRequestException(pgErr.Message) | ||
| // We're intentionally hiding the details of the error | ||
| // for example: pgErr.Severity and pgErr.Code | ||
| return exceptions.NewBadRequestException(pgErr.Message, nil) |
There was a problem hiding this comment.
Should we slog here to capture full error?
| } | ||
| } | ||
|
|
||
| msg := err.Error() | ||
| if strings.Contains(msg, "failed to encode") || strings.Contains(msg, "failed to decode") { | ||
| return exceptions.NewBadRequestException(msg) | ||
| return exceptions.NewBadRequestException(msg, nil) |
There was a problem hiding this comment.
slog to capture full error since we're dropping it?
| @@ -143,10 +143,10 @@ func (c *Connection) Signup(signupMethod *meta.SignupMethod, payload map[string] | |||
| return c.callListenerBot(signupMethod.SignupBot, payload) | |||
| } | |||
| func (c *Connection) ResetPassword(payload map[string]interface{}, authenticated bool) (*meta.LoginMethod, error) { | |||
| return nil, exceptions.NewBadRequestException("Google login: unfortunately you cannot change the password") | |||
| return nil, exceptions.NewBadRequestException("Google login: unfortunately you cannot change the password", nil) | |||
There was a problem hiding this comment.
should this be a 403 Forbidden instead of 400?
| } | ||
| func (c *Connection) ConfirmResetPassword(payload map[string]interface{}) (*meta.User, error) { | ||
| return nil, exceptions.NewBadRequestException("Google login: unfortunately you cannot change the password") | ||
| return nil, exceptions.NewBadRequestException("Google login: unfortunately you cannot change the password", nil) |
There was a problem hiding this comment.
should this be a 403 Forbidden instead of 400?
| @@ -161,5 +161,5 @@ func (c *Connection) CreateLogin(signupMethod *meta.SignupMethod, payload map[st | |||
| }, c.connection, c.session) | |||
| } | |||
| func (c *Connection) ConfirmSignUp(signupMethod *meta.SignupMethod, payload map[string]interface{}) error { | |||
| return exceptions.NewBadRequestException("Google login: unfortunately you cannot change the password") | |||
| return exceptions.NewBadRequestException("Google login: unfortunately you cannot change the password", nil) | |||
There was a problem hiding this comment.
should this be a 403 Forbidden instead of 400?
| // TODO: Using [0] results in losing the original error in some cases which results in not always returning | ||
| // an appropriate HTTP status code. For example, the following url which has an invalid format for userfileid | ||
| // param encounters HTTP 500 when it should be HTTP 400 | ||
| // https://studio.uesio-dev.com:3000/site/userfiles/download?userfileid=my-bad-id&version=1734112100 |
There was a problem hiding this comment.
We should still keep this comment & TODO I think since we're still returning (*op.Errors)[0] which won't always have the original error
apps/platform/pkg/datasource/save.go
Outdated
| if op.HasErrors() { | ||
| return wire.NewGenericSaveException(exceptions.NewBadRequestException("error with field population")) | ||
| // TODO: This error message should be better and possibly include messages |
There was a problem hiding this comment.
Should likely slog all the errors in op here since we're dropping them
| type BadRequestException struct { | ||
| msg string | ||
| } | ||
| type BadRequestException BaseException |
There was a problem hiding this comment.
I don't think we gain anything (other than the private vars due to same memory layout) by the type declaration here since BadRequestException won't get BaseExceptions methods and we're customizing them anyway. I think the options here are:
- Embed BaseException - Not sure if this should be the type
BaseExceptionor pointer to type*BaseException, would need to test
type BadRequestException struct {
BaseException
- Just use the private package helpers w/out involving BaseException here.
I think there will be times that we have exceptions that just need the BaseException functionality so I'd lean towards option 1 so that we define custom exceptions all the same way, whether or not there is any custom Error function or just using the default provided by BaseException
| } | ||
|
|
||
| type LoadException struct { | ||
| Message string `json:"message"` | ||
| error error | ||
| err error |
There was a problem hiding this comment.
See comment for BadRequestException about embedding, vs not, vs type declaration vs just using privates. This one we expose Message so that it serializes - that requirement may change the general approach to having a baseexception. I think the key is that we want to try to have all our custom errors follow the same definition approach if possible.
| } | ||
| } | ||
|
|
||
| type SaveException struct { | ||
| RecordID string `json:"recordid"` | ||
| FieldID string `json:"fieldid"` | ||
| Message string `json:"message"` | ||
| error error | ||
| err error |
There was a problem hiding this comment.
See comments on BadRequestException and LoadException about how to define the custom errors.
What does this PR do?