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

Support @ControllerAdvice registrations in the MockMvcBuilders standalone setup [SPR-12751] #17348

Closed
spring-issuemaster opened this issue Feb 24, 2015 · 14 comments

Comments

@spring-issuemaster
Copy link
Collaborator

commented Feb 24, 2015

Matt Byrne opened SPR-12751 and commented

We are using standaloneSetup for our MockMvc test and want to create a HandlerExceptionResolver so that we can instruct Spring to use an @ControllerAdvice class much like this post; however, if we set handler exception resolvers the builder will overwrite any defaults and only use our resolver.

Our setup is like so (I didn't bother including code for how our resolver is created).

MockMvcBuilders.standaloneSetup(buildController())
        .setHandlerExceptionResolvers(createExceptionResolver())
        .build();

What we'd really like to do is add our resolver to the default list. We've had to do some nasty reflection to get a handle on the WebMvcConfigurationSupport#addDefaultHandlerExceptionResolvers(java.util.List) method so we can get the default resolvers first before creating our own, and that works as a workaround.

Can a suitable method be added to the builder to allow this?


Issue Links:

  • #22074 Fix support of @ControllerAdvice registrations in the MockMvcBuilders standalone setup

0 votes, 5 watchers

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 25, 2015

Rossen Stoyanchev commented

The MockMvcBuilders standaloneSetup let's you (1) set up one controller at a time, (2) gives you basic Spring MVC configuration (roughly the same as the MVC Java config), and (3) provides basic methods to customize the configuration manually. By contrast the MockMvcBuilders webContextSetup loads Spring configuration and discovers controllers and Spring MVC configuration in it, just like Spring MVC does at runtime.

When it comes to @ControllerAdvice beans, those are discovered by scanning the Spring configuration so naturally they don't work with the standaloneSetup. Rather than having to register a HandlerExceptionResolver, what we could do is allow registration of @ControllerAdvice beans in the standalone setup, much like you register controllers (rather than having the discovered). I think that's an obvious gap and would make a nice improvement.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 25, 2015

Rossen Stoyanchev commented

Updated title (was: "Introduce support for adding HandlerExceptionResolver to defaults in MockMvc")

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 25, 2015

Matt Byrne commented

That's an even better solution @Rossen! I assume the registered controller advices would use the already-configured MessageResolvers that are configured in the builder? (I only ask because you had to set them again for each HandlerExceptionResolver). Just a heads-up that a test with a rest response would be great (e.g. a 400 with JSON containing the message as to why).

Thanks for speedy feedback.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 25, 2015

Rossen Stoyanchev commented

Yes it should work fine. In other words you'll get the default exception resolvers configured as usual. The key difference is that ExceptionHandlerExceptionResolver should now be aware of any @ControllerAdvice instances registered with the standalone builder.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 25, 2015

Matt Byrne commented

Perfect. Happy to try it out and provide feedback whenever a snapshot is available.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 25, 2015

Rossen Stoyanchev commented

The latest 4.2.0.BUILD-SNAPSHOT should have the changes. This is the relevant commit.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 25, 2015

Matt Byrne commented

Just tested with latest SNAPSHOT and all passed in our build using setControllerAdvice() - thanks!

Just a naming convention thing ... since you accept a varargs should the method name be setControllerAdvices()? (i.e. pluralize). This would also be consistent with the other methods like setMessageResolvers() and setHandlerExceptionResolvers() even if "advices" sounds a bit odd.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 26, 2015

Juergen Hoeller commented

Good point... setControllerAdvices may sound a bit odd but it would be more consistent. Unless we document setControllerAdvice to intentionally drop the 's' for grammatical reasons ;-)

Juergen

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 26, 2015

Rossen Stoyanchev commented

My reason for sticking to setControllerAdvice is that it is a vararg so I don't expect ever a method taking a single ControllerAdvice. In that sense we can remain consistent by claiming "advice", given that it's also grammatically correct, for the plural form.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 26, 2015

Sam Brannen commented

I agree with Rossen. If you are using standaloneSetup() then you are testing a single controller class in which case you typically would not have more than one @ControllerAdvice class anyway. Plus "advices" is not a word, so adding the "s" would look/sound strange to most people. IMHO, documenting the var-args parameter in the Javadoc should suffice. ;)

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 26, 2015

Rossen Stoyanchev commented

Well you could have more than one @ControllerAdvice. It's a bit like having one or more interceptors. That said I've updated the Javadoc a bit!

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 26, 2015

Matt Byrne commented

Fair points - thanks. Just wanted to check before the API was released and because it wasn't immediately obvious you could specify multiple before I looked at the param def. that being said it's pretty quick to find out anyway and Javadoc clarification will more than suffice.

Thanks again for introducing the feature so quickly.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 22, 2015

Ryan Gustafson commented

@ControllerAdvice is also used to setup a ResponseBodyAdvice implementation. Will this work too?

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 22, 2015

Rossen Stoyanchev commented

Yes it should work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.