validators: add validators #15
Conversation
CaioTeixeira95
requested review from
marcelosalloum,
marwen-abid and
daniel-burghardt
|
|
||
| func NewValidator() *validator.Validate { | ||
| validate := validator.New() | ||
| _ = validate.RegisterValidation("public_key", publicKeyValidation) |
There was a problem hiding this comment.
RegisterValidation returns an error, right? You should handle it accordingly here by wrapping it and sending the wrapped version to the caller.
Or at least log the error here.
There was a problem hiding this comment.
It only returns an error if the key is empty or the function is nil. That's why I'm not handling the error here, I don't think we should worry about it, wdyt?
internal/validators/validate.go
Outdated
| case "gt": | ||
| if fieldError.Kind() == reflect.Slice || fieldError.Kind() == reflect.Array { | ||
| return "Should have at least 1 element" | ||
| } | ||
| return fmt.Sprintf("Should be greater than %s", fieldError.Param()) | ||
| case "gte": | ||
| return fmt.Sprintf("Should be greater than or equal %s", fieldError.Param()) |
There was a problem hiding this comment.
I didn't know gt could be used with slices and arrays. In any case, the get and gt messages re not consistent in case of arrays.
There was a problem hiding this comment.
Also, is Should have at least 1 element what you expect to return when using gt with arrays? Isn't fieldError.Param() relevant for that case?
There was a problem hiding this comment.
Thank you for this! I'll change it so both will have the same behavior for slices. Also, I'm going to use the param here.
| func ParseValidationError(errors validator.ValidationErrors) map[string]interface{} { | ||
| fieldErrors := make(map[string]interface{}) | ||
| for _, err := range errors { | ||
| fieldErrors[getFieldName(err)] = msgForFieldError(err) | ||
| } | ||
| return fieldErrors | ||
| } |
There was a problem hiding this comment.
There will be cases for when more than one validation fails, like in validate:"required,public_key".
🤔 Do you think it's worth returning a slice of error messages, for each validation that fails in that field? Of would this library only return one error per field?
There was a problem hiding this comment.
It only returns one error per field, the order of the flags decides the validation order.
There was a problem hiding this comment.
It's fine to do incremental error handling in an API
| Status: http.StatusMethodNotAllowed, | ||
| Error: "The method is not allowed for resource at the url requested.", | ||
| } | ||
|
|
||
| func BadRequest(message string) errorResponse { | ||
| func BadRequest(message string, extras map[string]interface{}) *ErrorResponse { |
There was a problem hiding this comment.
Why are we changing the response to a pointer?
There was a problem hiding this comment.
That's because we are able to do things like:
httpErr := DecodeJSONAndValidate(req, &reqBody)
if httpErr != nil {
httpErr.Render(rw)
return
}Additionally, it's not possible to do something like this in go:
return &httperror.BadRequest(...)
internal/validators/validate_test.go
Outdated
| require.Len(t, vErrs, 2) | ||
|
|
||
| assert.Equal(t, "publicKey", getFieldName(vErrs[0])) | ||
| assert.Equal(t, "children[0].name", getFieldName(vErrs[1])) |
There was a problem hiding this comment.
I'm thinking whether in a case like this it could be useful to have the full path rather than only the two last elements.
Any particular reason why you chose to cut it?
There was a problem hiding this comment.
There is no particular reason, I can make it to return the entire path except for the Root struct (otherwise we'll leak an internal structure). I thought that only showing the last two would make it more clear.
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func TestParseValidationError(t *testing.T) { |
There was a problem hiding this comment.
I think it's amazing and super important to have these more complex test cases with dives and etc., but in addition to these, wdyt about adding separate sub tests for each validation error?
I think that way it might be easier to remember to add tests for new validations and we can better test each one individually. For instance, the Should be greater than or equal case of gt is not tested in this one.
What
This PR adds the validation layer to the project using the go-validator library.
Why
To make the request/object validation easier.
Known limitations
N/A
Issue that this PR addresses
N/A
Checklist
PR Structure
allif the changes are broad or impact many packages.Thoroughness
Release