Skip to content
This repository has been archived by the owner on Apr 21, 2023. It is now read-only.

Expose Named Arguments in Error Responses #46

Merged
merged 26 commits into from
Mar 19, 2019

Conversation

zarebski-m
Copy link
Collaborator

@zarebski-m zarebski-m commented Mar 8, 2019

Hello. :) I hope this contribution will be as helpful as the previous one.

Currently response for each error exposes only code and message. Unfortunately, the exception arguments are not exposed explicitly, but only through interpolation of error message.

This feature provides enhancement, where all exception arguments can be exposed as a map. It is regulated by configuration property spring.errors.expose-arguments, which by default has value never. This is consistent with current bahavior.

When set to non_empty or always, rensponse payload includes map of named properties. With the following annotation:

@Size(min = 10, max = 20, message = "error.code")
private String password;

the following payload will be returned:

{
  "errors": [
    {
      "code": "error.code",
      "message": null,
      "arguments": {
        "min": 10,
        "max": 20
      }
    }
  ]
}

There is a problem with Spring Validation – there are no argument names exposed by ObjectError. For this problem I've decided to just assign those arguments names "arg0", "arg1", etc. If you have a better idea, please let me know.

Closes #44

@codecov-io
Copy link

codecov-io commented Mar 8, 2019

Codecov Report

Merging #46 into master will increase coverage by 0.19%.
The diff coverage is 89.44%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #46      +/-   ##
============================================
+ Coverage     91.18%   91.37%   +0.19%     
- Complexity      207      230      +23     
============================================
  Files            24       27       +3     
  Lines           465      545      +80     
  Branches         56       72      +16     
============================================
+ Hits            424      498      +74     
+ Misses           25       22       -3     
- Partials         16       25       +9
Impacted Files Coverage Δ Complexity Δ
...idg/errors/handlers/LastResortWebErrorHandler.java 100% <ø> (ø) 3 <0> (ø) ⬇️
...s/handlers/ConstraintViolationWebErrorHandler.java 100% <100%> (+2.7%) 12 <2> (-3) ⬇️
...andlers/ConstraintViolationArgumentsExtractor.java 100% <100%> (ø) 5 <5> (?)
.../me/alidg/errors/conf/ErrorsAutoConfiguration.java 86.95% <100%> (+0.59%) 11 <2> (+1) ⬆️
...rc/main/java/me/alidg/errors/WebErrorHandlers.java 100% <100%> (ø) 21 <3> (ø) ⬇️
...rc/main/java/me/alidg/errors/HandledException.java 100% <100%> (ø) 10 <2> (ø) ⬇️
...ors/adapter/DefaultHttpErrorAttributesAdapter.java 100% <100%> (ø) 6 <2> (ø) ⬇️
src/main/java/me/alidg/errors/HttpError.java 91.66% <100%> (+9.84%) 9 <0> (ø) ⬇️
...errors/handlers/ResponseStatusWebErrorHandler.java 78.78% <66.66%> (-3.76%) 20 <2> (+1)
.../alidg/errors/handlers/ServletWebErrorHandler.java 91.66% <78.57%> (-8.34%) 18 <1> (+1)
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f609080...ffc6f52. Read the comment docs.

@alimate
Copy link
Owner

alimate commented Mar 9, 2019

I like the idea of returning as much as data possible and letting the client to assemble the message.
I will look at the codes in the next couple of days. Thanks for your great PR.

@alimate
Copy link
Owner

alimate commented Mar 14, 2019

@zarebski-m The SpringValidationWebErrorHandler is responsible for handling all BindException and MethodArgumentNotValidException. Regarding that arg0, arg1 approach, here are my two cents:

  1. When the Spring's binding mechanism is the exception source, we have no other choice but those arg0,arg1,.. thingies.
  2. Otherwise, especially for MethodArgumentNotValidExceptions, we can still manage to extract the actual ConstraintViolationException. So in such cases, we can provide more meaningful argument names.

How can we access to the binding exception source?

  • The source field in ObjectError.
  • The unwrap(class) method in ObjectError.

Proposed Solution

If the underlying BindingResult contains:

  • Bean Validation errors, then the error handling should be compatible with the way we currently handle the ConstraintViolationExceptions.
  • Type mismatch exceptions, then we should return a dedicated error code along with arguments about the property name, required type and the invalid value.
  • Otherwise, there is no need to expose arguments at all.

I will push this solution on top of your commits!

@zarebski-m
Copy link
Collaborator Author

Extracting named arguments was moved to separate method ConstraintViolationArgumentsExtractor#extract(ConstraintViolation). This method is used by both ConstraintViolationWebErrorHandler and SpringValidationWebErrorHandler when ConstraintViolation is successfully unwrapped from ObjectError.

In addition two more arguments are exposed from constraint violations at the end of argument list:

  • invalidValue
  • propertyPath

@zarebski-m
Copy link
Collaborator Author

There seems to be some inconsistency with Java 9. I am looking into this now.

@alimate
Copy link
Owner

alimate commented Mar 15, 2019

@zarebski-m I had fun today, thanks 🙏
After a couple of more reviews and running a few more manual tests against production ready projects, I will merge the PR. Thanks again.

Marcin Zarebski and others added 18 commits March 19, 2019 08:03
Signed-off-by: Ali Dehghani <ali.dehghani.g@gmail.com>
Signed-off-by: Ali Dehghani <ali.dehghani.g@gmail.com>
Signed-off-by: Ali Dehghani <ali.dehghani.g@gmail.com>
Signed-off-by: Ali Dehghani <ali.dehghani.g@gmail.com>
Signed-off-by: Ali Dehghani <ali.dehghani.g@gmail.com>
Signed-off-by: Ali Dehghani <ali.dehghani.g@gmail.com>
According to the documentation, Class#getMethods() does not guarantee any order.
https://docs.oracle.com/javase/8/docs/api/java/lang/Class.html#getMethods--
Signed-off-by: Ali Dehghani <ali.dehghani.g@gmail.com>
Signed-off-by: Ali Dehghani <ali.dehghani.g@gmail.com>
Signed-off-by: Ali Dehghani <ali.dehghani.g@gmail.com>
Signed-off-by: Ali Dehghani <ali.dehghani.g@gmail.com>
Signed-off-by: Ali Dehghani <ali.dehghani.g@gmail.com>
Signed-off-by: Ali Dehghani <ali.dehghani.g@gmail.com>
Signed-off-by: Ali Dehghani <ali.dehghani.g@gmail.com>
Signed-off-by: Ali Dehghani <ali.dehghani.g@gmail.com>
@alimate alimate changed the title Expose named arguments in response payload Expose Named Arguments in Error Responses Mar 19, 2019
Signed-off-by: Ali Dehghani <ali.dehghani.g@gmail.com>
Signed-off-by: Ali Dehghani <ali.dehghani.g@gmail.com>
@alimate
Copy link
Owner

alimate commented Mar 19, 2019

@zarebski-m Thanks. Great Work.
Merging Now.

@alimate alimate merged commit e7226e7 into alimate:master Mar 19, 2019
@zarebski-m zarebski-m deleted the expose-arguments branch March 19, 2019 08:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants