Wrp validation update #318
Conversation
primaryHandler.go
Outdated
| err = multierr.Append(err, wrp.SourceValidator(*msg)) | ||
| err = multierr.Append(err, wrp.DestinationValidator(*msg)) | ||
| if err != nil { | ||
| logger.Warn("errors returned during WRP message validation", zap.Error(err)) |
There was a problem hiding this comment.
| logger.Warn("errors returned during WRP message validation", zap.Error(err)) | |
| logger.Warn("Found WRP message validation failures", zap.Error(err)) |
primaryHandler.go
Outdated
| for _, v := range validators { | ||
| err = multierr.Append(err, v.Validate(*msg)) | ||
| } |
There was a problem hiding this comment.
i say keep the for loop pattern you had and add assert.ErrorIserrors.Is
it'll help manage and make clear which error we'll like to return 400s for
There was a problem hiding this comment.
is there a different assert package? i thought assert was just for testing.
There was a problem hiding this comment.
nevermind i think i found the function i can use.
There was a problem hiding this comment.
oh sorry, I meant to say errors.Is and not assert.ErrorIs
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #318 +/- ##
==========================================
- Coverage 20.95% 20.64% -0.32%
==========================================
Files 7 7
Lines 730 741 +11
==========================================
Hits 153 153
- Misses 575 586 +11
Partials 2 2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
primaryHandler.go
Outdated
| for _, v := range validators { | ||
| err = multierr.Append(err, v.Validate(*msg)) | ||
| } | ||
|
|
||
| if err != nil { |
There was a problem hiding this comment.
I don't think we need this nil check since errors.Is should handle this
| if err != nil { | |
| if err != nil { |
primaryHandler.go
Outdated
| if err != nil { | ||
|
|
||
| if warningErrors != nil { | ||
| logger.Warn("WRP message validation failures found", zap.Error(err)) |
There was a problem hiding this comment.
| logger.Warn("WRP message validation failures found", zap.Error(err)) | |
| logger.Warn("WRP message validation warnings found", zap.Error(warningErrors)) |
| w.Header().Set("Content-Type", "application/json") | ||
| w.WriteHeader(http.StatusBadRequest) | ||
| fmt.Fprintf( | ||
| w, | ||
| `{"code": %d, "message": "%s"}`, | ||
| http.StatusBadRequest, | ||
| fmt.Sprintf("failed to validate WRP message: %s", err), | ||
| ) | ||
| fmt.Sprintf("failed to validate WRP message: %s", err)) |
There was a problem hiding this comment.
| fmt.Sprintf("failed to validate WRP message: %s", err)) | |
| fmt.Sprintf("failed to validate WRP message: %s", failureError)) |
What's Included:
UTF8ValidatorandMessageTypeValidatorusage #315SourceValidatorandDestinationValidatorusage #314