Navigation Menu

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

Standalone MockMvc ignores @RestControllerAdvice annotation attributes #25520

Closed
yevhenii-pazii opened this issue Aug 4, 2020 · 4 comments
Closed
Assignees
Labels
in: test Issues in the test module in: web Issues in web modules (web, webmvc, webflux, websocket) type: regression A bug that is also a regression
Milestone

Comments

@yevhenii-pazii
Copy link

yevhenii-pazii commented Aug 4, 2020

MockMvc created by MockMvcBuilders.standaloneSetup() ignores @RestControllerAdvice annotation attributes but works well for a @ControllerAdvice/ResponseBody pair.

  • Spring Boot: 2.3.2
  • Spring Framework: 5.2.8

Example: https://github.com/thecederick/MockMvc-ignores-RestControllerAdvice-annotation-fields

Failing Configuration

Controller:

@RestController
@RequestMapping
public class Api1Controller {

    @PostMapping(value = "/endpoint1")
    public String endpoint() {
        return "done";
    }
}

Controller advice:

@RestControllerAdvice(assignableTypes = Api1Controller.class)
@Order(Ordered.HIGHEST_PRECEDENCE)
public class Api1ControllerAdvice {

    @ExceptionHandler(Throwable.class)
    public String handleException(Throwable throwable) {
        return this.getClass() + " - " + throwable.getClass();
    }
}

Test:

    @BeforeEach
    public void setup() {
        this.mockMvc = MockMvcBuilders
                .standaloneSetup(controller)
                .addDispatcherServletCustomizer(
                        dispatcherServlet -> dispatcherServlet.setThrowExceptionIfNoHandlerFound(true))
                .setControllerAdvice(Api1ControllerAdvice.class, DefaultControllerAdvice.class)
                .build();
    }

    @Test
    void notFound() throws Exception {
        mockMvc
                .perform(
                        post("/test")
                                .contentType("application/json")
                                .content("{}"))
                .andExpect(content().string(
                        "class com.example.demo.root.DefaultControllerAdvice - class org.springframework.web.servlet.NoHandlerFoundException"));
    }

Working Configuration

@RestController
@RequestMapping
public class Api2Controller {

    @PostMapping(value = "/endpoint2")
    public String endpoint() {
        return "done";
    }
}

Controller advice:

@ControllerAdvice(assignableTypes = Api2Controller.class)
@ResponseBody
@Order(Ordered.HIGHEST_PRECEDENCE)
public class Api2ControllerAdvice {

    @ExceptionHandler(Throwable.class)
    public String handleException(Throwable throwable) {
        return this.getClass() + " - " + throwable.getClass();
    }
}

Test:

    @BeforeEach
    public void setup() {
        this.mockMvc = MockMvcBuilders
                .standaloneSetup(controller)
                .addDispatcherServletCustomizer(
                        dispatcherServlet -> dispatcherServlet.setThrowExceptionIfNoHandlerFound(true))
                .setControllerAdvice(
                        Api2ControllerAdvice.class,
                        DefaultControllerAdvice.class)
                .build();
    }

    @Test
    void notFound() throws Exception {
        mockMvc
                .perform(
                        post("/test")
                                .contentType("application/json")
                                .content("{}"))
                .andExpect(content().string(
                        "class com.example.demo.root.DefaultControllerAdvice - class org.springframework.web.servlet.NoHandlerFoundException"));
    }

Default advice

@RestControllerAdvice
@Order(Ordered.LOWEST_PRECEDENCE)
public class DefaultControllerAdvice {

    @ExceptionHandler(Throwable.class)
    public String handleException(Throwable throwable) {
        return this.getClass() + " - " + throwable.getClass();
    }
}

Summary

In the first configuration using the @RestControllerAdvice annotation the test fails; with the second one, it passes as expected.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Aug 4, 2020
@sbrannen sbrannen added in: test Issues in the test module in: web Issues in web modules (web, webmvc, webflux, websocket) labels Aug 4, 2020
@sbrannen sbrannen changed the title MockMvc ignores RestControllerAdvice annotation fields MockMvc ignores @RestControllerAdvice fields Aug 4, 2020
@sbrannen sbrannen self-assigned this Aug 4, 2020
@sbrannen sbrannen changed the title MockMvc ignores @RestControllerAdvice fields Stand-alone MockMvc ignores @RestControllerAdvice annotation attributes Aug 5, 2020
@sbrannen sbrannen added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Aug 5, 2020
@sbrannen sbrannen added this to the 5.2.9 milestone Aug 5, 2020
@sbrannen
Copy link
Member

sbrannen commented Aug 5, 2020

Thanks for raising the issue and providing the example project to debug. Much appreciated!

I've determined that there is a bug in the implementation of StaticListableBeanFactory.findAnnotationOnBean(). The implementation in DefaultListableBeanFactory (which is used in production and with MockMvc when providing a WebApplicationContext to the builder) looks up "merged annotations" with support for @AliasFor attribute overrides; whereas, the implementation in StaticListableBeanFactory (which is used when using the stand-alone MockMvc builder) currently only looks up raw annotations. That's why the annotation attributes in @RestControllerAdvice get ignored when using the stand-alone MockMvc support.

@sbrannen sbrannen changed the title Stand-alone MockMvc ignores @RestControllerAdvice annotation attributes Standalone MockMvc ignores @RestControllerAdvice annotation attributes Aug 5, 2020
@sbrannen sbrannen added type: regression A bug that is also a regression and removed type: bug A general bug labels Aug 5, 2020
@sbrannen
Copy link
Member

sbrannen commented Aug 5, 2020

@sbrannen
Copy link
Member

sbrannen commented Aug 5, 2020

The fix has been merged into 5.2.x and master.

Feel free to try it out in an upcoming 5.2.9 or 5.3 M2 snapshot.

Thanks again for raising the issue. 👍

sbrannen added a commit that referenced this issue Aug 5, 2020
This commit introduces regression tests for @RestControllerAdvice
support in standalone MockMvc configurations.

See gh-25520
sbrannen added a commit that referenced this issue Aug 5, 2020
This commit introduces regression tests for @RestControllerAdvice
support in standalone MockMvc configurations.

See gh-25520
sbrannen added a commit that referenced this issue Aug 5, 2020
This commit introduces regression tests for @RestControllerAdvice
support in standalone MockMvc configurations.

See gh-25520
@yevhenii-pazii
Copy link
Author

Thank you!

FelixFly pushed a commit to FelixFly/spring-framework that referenced this issue Aug 16, 2020
Since Spring Framework 5.2, @RestControllerAdvice registered with
MockMvc when using MockMvcBuilders.standaloneSetup() has no longer been
properly supported if annotation attributes were declared in the
@RestControllerAdvice annotation. Prior to 5.2, this was not an issue.

The cause for this regression is two-fold.

1. Commit 50c2577 refactored
   DefaultListableBeanFactory so that findAnnotationOnBean() supports
   merged annotations; however, that commit did not refactor
   StaticListableBeanFactory#findAnnotationOnBean() to support merged
   annotations.

2. Commit 978adbd refactored
   ControllerAdviceBean so that a merged @ControllerAdvice annotation
   is only looked up via ApplicationContext#findAnnotationOnBean().

The latter relies on the fact that findAnnotationOnBean() supports
merged annotations (e.g., @RestControllerAdvice as a merged instance of
@ControllerAdvice). Behind the scenes, MockMvcBuilders.standaloneSetup()
creates a StubWebApplicationContext which internally uses a
StubBeanFactory which extends StaticListableBeanFactory. Consequently,
since the implementation of findAnnotationOnBean() in
StaticListableBeanFactory was not updated to support merged annotations
like it was in DefaultListableBeanFactory, we only see this regression
with the standalone MockMvc support and not with MockMvc support for an
existing WebApplicationContext or with standard Spring applications
using an ApplicationContext that uses DefaultListableBeanFactory.

This commit fixes this regression by supporting merged annotations in
StaticListableBeanFactory#findAnnotationOnBean() as well.

Closes spring-projectsgh-25520
zx20110729 pushed a commit to zx20110729/spring-framework that referenced this issue Feb 18, 2022
This commit introduces regression tests for @RestControllerAdvice
support in standalone MockMvc configurations.

See spring-projectsgh-25520
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: test Issues in the test module in: web Issues in web modules (web, webmvc, webflux, websocket) type: regression A bug that is also a regression
Projects
None yet
Development

No branches or pull requests

3 participants