-
Notifications
You must be signed in to change notification settings - Fork 17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Validation error of user request returns HTTP 500 #263
Conversation
Codecov Report
@@ Coverage Diff @@
## master #263 +/- ##
=======================================
Coverage 91.87% 91.87%
=======================================
Files 50 50
Lines 2510 2512 +2
=======================================
+ Hits 2306 2308 +2
- Misses 110 111 +1
+ Partials 94 93 -1
Continue to review full report at Codecov.
|
@@ -60,6 +60,9 @@ func (o *ResolveHandler) doResolve(id string) (*document.ResolutionResult, error | |||
|
|||
doc, err := o.resolver.ResolveDocument(id) | |||
if err != nil { | |||
if strings.Contains(err.Error(), "bad request") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we instead define a 'BadRequest' error and then use errors.Is(...) to determine that it's the cause? I find that matching a string within the error is not reliable. I was already thinking about refactoring the code that searches for "not found".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I provide details about error? We usually use this pattern if error is constant e.g.
var ErrNotFound = errors.New("did not found under given key")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that a new 'BadRequestError' type should be defined which has a code. The code contains a specific value that's defined in an enumeration. This is what's done for the cas REST endpoint: https://github.com/trustbloc/sidetree-fabric/blob/master/pkg/rest/dcashandler/retrieveerror.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have 5 status codes there. If I would capture all validations it would probably be more than 30 status codes plus they are all over the place because of the nature of resolution with initial state (parse did, parse initial state parameter, validate initial document, parse operation which involves suffix, delta patches (keys, services), reveal values, commitment values, hashes, recovery key etc). I can create new issue for that and we can address it later on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, this can be done later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created #264
Fixed resolution and resolution with initial state to return bad request in case that user supplies invalid input Operation processing has been fixed with issue trustbloc#201 (operation validation on server side) Closes trustbloc#149 Signed-off-by: Sandra Vrtikapa <[email protected]>
Fixed resolution and resolution with initial state to return bad request in case that user supplies invalid input
Operation processing has been fixed with issue #201 (operation validation on server side)
Closes #149
Signed-off-by: Sandra Vrtikapa [email protected]