Skip to content
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

@ExceptionHandler can't be override by @ControllerAdvice [DATAREST-421] #801

Closed
spring-projects-issues opened this issue Dec 4, 2014 · 8 comments
Assignees
Labels

Comments

@spring-projects-issues
Copy link

@spring-projects-issues spring-projects-issues commented Dec 4, 2014

Thibaud Lepretre opened DATAREST-421 and commented

AbstractRepositoryRestController defines a number of @ExceptionHandler however like clearly explain here http://spring.io/blog/2013/11/01/exception-handling-in-spring-mvc:

@ExceptionHandler methods on the Controller are always selected before those on any @ControllerAdvice instance

So as user I can't override the Spring DATA REST behavior without reimplement an HandlerExceptionResolver in order to inverse order choice between @ControllerAdvice and @ExceptionHandler methods

By switching the @ExceptionHandler methods to an instance of @ControllerAdvice user can override behavior by adding another @ControllerAdvice with better @Order inside applicationContext.

But we should be keep in mind this issue https://jira.spring.io/browse/SPR-11570. I don't know if the previous mechanism can works without the shared @ControllerAdvice, but I will try it as soon as possible.

If accepted I can send a pull request. I have already forked a working repository with @ControllerAdvice


Affects: 2.2 GA (Evans)

Referenced from: pull request #155

1 votes, 3 watchers

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Dec 4, 2014

Oliver Drotbohm commented

Feel free to submit a pull request :)

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Dec 4, 2014

Thibaud Lepretre commented

Done with Integration test. I hope it's will be ok else feel free to contact me I will be glad to help again

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Dec 5, 2014

Thibaud Lepretre commented

In order to do not force user to add @Order(Ordered.HIGHEST_PRECEDENCE) we can set Spring DATA REST @ControllerAdvice as @Order(Ordered.LOWEST_PRECEDENCE). What do you think about that @Olivier Gierke?

Off-topic: Should I mainly comment on Jira or in Github pull-request issue? (or both)

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Dec 5, 2014

Runar Botten commented

With this improvement in place (together with DATAREST-388) SDR will finally provide a minimum level of control with regards to error handling. Having some experience with Jersey projects, I will say SDR is about to catch up on this subject

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Dec 5, 2014

Oliver Drotbohm commented

I've used the code provided in the pull request but applied some general polishing (see this commit).

Unfortunately an @Order annotation will be required for the custom controller advice anyway as the default order an unannotated component gets is lowest priority to. So we'd basically depend on the component detection order. Not annotating the class is basically equivalent to @Order(Ordered.LOWEST_PRECENDENCE)

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Dec 5, 2014

Thibaud Lepretre commented

Yeah I've already check your commit, all seems good.

Ok for @Order(Ordered.LOWEST_PRECENDENCE), I have no problem with forcing usage of @Order(Ordered.HIGHEST_PRECEDENCE) because I always used the annotation to be sure that my custom :)

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Dec 5, 2014

Oliver Drotbohm commented

Runar Botten - We usually recommend to file tickets or vote for existing ones as that's how we prioritize workloads and differentiate nice-to-haves from this-seems-to-affect-a-lot-of-users

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Dec 5, 2014

Runar Botten commented

Sure, I already did Oliver - on both the mentioned issues. Appreciate the resolutions!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants