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

Can't use controller advices if annotation based event handling is used [DATAREST-388] #767

Closed
spring-projects-issues opened this issue Sep 26, 2014 · 11 comments

Comments

@spring-projects-issues
Copy link

@spring-projects-issues spring-projects-issues commented Sep 26, 2014

Saulo Medeiros de Araujo opened DATAREST-388 and commented

Spring Data REST is capturing any exceptions thrown by event handler methods and re-throwing them as IllegalStateException. By doing this, Spring Data REST makes it very hard to adopt any exception handling mechanism offered by Spring MVC (controller advices, for example) because we cannot pinpoint which exception we would like to handle in each exception handler of the controller advice (the only exception that we can handle is IllegalStateException).

The patch in the pull request corrects this behavior. If a event handler throws a unchecked exception, than it makes sure that this very exception is thrown. Otherwise (if it is a checked exception), it resorts to the old behavior, encapsulating the checked exception in a IllegalStateException. Without this patch, the controller advice shown below does not work.

@ControllerAdvice
public class ExceptionHandlingControllerAdvice {
  @ExceptionHandler(UnauthorizedException.class)
  public ResponseEntity handleUnauthorizedException(UnauthorizedException exception) {
    String stackTrace = ExceptionUtils.getStackTrace(exception);
    return new ResponseEntity(stackTrace, HttpStatus.FORBIDDEN); 
  }
}

By applying this patch, I believe Spring Data REST better fits the Spring MVC world


Affects: 2.2 GA (Evans)

Referenced from: pull request #151

Backported to: 2.2.2 (Evans SR2), 2.1.5 (Dijkstra SR5)

2 votes, 4 watchers

@spring-projects-issues
Copy link
Author

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

Saulo Medeiros de Araujo commented

Hi,

Any news about this pull request? Will it be accepted?

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Nov 29, 2014

Runar Botten commented

I'm having the same problem. Worked around it by adding an @ExceptionHandler on IllegalStateException which usually wraps an InvocationTargetException which again wraps the original exception thrown from the repository event handler. Quite ugly, but the only workaround I could think of.

I would really appreciate this bug to be solved as well

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Nov 29, 2014

Saulo Medeiros de Araujo commented

Hi Runar,

I am quite disappointed with the Spring Data REST maintainers. More than a month ago I opened two or three bugs with pull requests that fixes them but the maintainers neither gave me feedback nor accepted the pull requests. Not a good way to evolve a community...

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Nov 29, 2014

Oliver Drotbohm commented

I'm sorry for that experience Saulo. We're a small team only and autumn's pretty packed with conference travel, too. We're just about to wrap up the first milestone for 2.3 but I'll definitely look more closely into the PRs you've submitted. They usually require a bit of rework so that we're quite defensive in asking people to change stuff, especially if it's the first time they submit stuff. But still I understand that letting them stay untouched for such a long time is not a good alternative either.

So thanks for your contributions. I'll promise we'll get both tickets fixed in for 2.3 RC1 and the both look like good candidates for back-ports into Evans SR2

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Nov 29, 2014

Saulo Medeiros de Araujo commented

Nice to hear that Oliver!

Please let me know if there is anything else that I can do in order for the pull requests to be accepted

@spring-projects-issues
Copy link
Author

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

Thibaud Lepretre commented

+1 eagerly awaited feature. I must used patch until fix will be ported to the official release

@spring-projects-issues
Copy link
Author

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

Thibaud Lepretre commented

I will add other point:

Inside the org.springframework.data.rest.webmvc.AbstractRepositoryRestController there is some generic @ExceptionHandler, however the default Spring HandlerExceptionResolver will check @ExceptionHandler then @ControllerAdvice.

So if I want to override the behavior for one of these @ExceptionHandler I should implement my own ExceptionResolver. I think (must be tested) if you replace @ExceptionHandler with @ControllerAdvice it should be possible to override the Spring DATA REST @ControllerAdvice using @Order annotation.

@spring-projects-issues
Copy link
Author

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

Oliver Drotbohm commented

This fixed in master and the bugfix branches. For master I took the chance to clean up that rather dark area of the codebase by quite a bit which caused a few type renames and moves. Also I reduced the rather convoluted way to express interest in events for a certain type by rather expecting the first parameter of the annotated handler method. So no need to duplicate the type definition anymore for 2.3. The actual fix for the wrapped exception was solved by using Spring Framework's ReflectionUtils which already takes care of the necessary bits and pieces.

I've back-ported the fix manually by leaving away the more advanced changes and reduce it to the plain exception related bug fix. Feel free to give the snapshots a spin

@spring-projects-issues
Copy link
Author

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

Runar Botten commented

Just tried 2.2.2-BUILD-SNAPSHOT, and it works as expected. Thanks a lot!

My next take was actually exact what Thibaud Lepretre brought up. Was your last reply aimed at that issue as well, Oliver, or is this a separate (yet unregistered) issue?

@spring-projects-issues
Copy link
Author

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

Thibaud Lepretre commented

@Runar Botter I just created an issue https://jira.spring.io/browse/DATAREST-421 & pull request #155

@spring-projects-issues
Copy link
Author

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

Runar Botten commented

Ok, thanks, Thibaud!

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

Successfully merging a pull request may close this issue.

None yet
2 participants