Skip to content

Expose a TestDispatcherServlet bean in the MockMvcAutoConfiguration #13241

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

Closed
mpryahin opened this issue May 23, 2018 · 15 comments
Closed

Expose a TestDispatcherServlet bean in the MockMvcAutoConfiguration #13241

mpryahin opened this issue May 23, 2018 · 15 comments
Assignees
Labels
theme: testing Issues related to testing type: enhancement A general enhancement
Milestone

Comments

@mpryahin
Copy link

mpryahin commented May 23, 2018

I came across an inconvenience while using @AutoConfigureMockMvc in my tests.
I have a controller that accepts multipart requests and delegates each distinct part into a Dispatcherservlet instance which is autowired into that controller - just a kind of batch endpoint implementation. Everything works great until I tried to test it via MockMvc (@AutoConfigureMockMvc).

As it turned out a MockMvc bean is created in a single isolated factory method with no interaction with the application context (a dispatcher servlet instance is created as a local variable and passed directly to MockMvc constructor instead of being looked up in the application context). This prevents my controller from being instantiated due to required been (DispatcherServlet) is not found in the application context. I've overcome this by implementing a Configuration class which creates a bean of the TestDispatcherServlet class and injects it into a MockMvc bean.
It would be great if such a behaviour is available out of the box.
Thanks in advance.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label May 23, 2018
@mpryahin mpryahin changed the title Expose TestDispatcherServlet in the MockMvcAutoConfiguration Expose a TestDispatcherServlet bean in the MockMvcAutoConfiguration May 23, 2018
@philwebb philwebb added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels May 23, 2018
@philwebb philwebb added this to the Backlog milestone May 23, 2018
@mbhave mbhave added the theme: testing Issues related to testing label May 25, 2018
@mbhave mbhave self-assigned this Jun 5, 2018
@mbhave
Copy link
Contributor

mbhave commented Jun 6, 2018

I've overcome this by implementing a Configuration class which creates a bean of the TestDispatcherServlet class and injects it into a MockMvc bean.

@mpryahin Does that mean you end up with two dispatcher servlets, one that's passed directly to the MockMvc constructor and one that's created as a bean?

Could you provide a code snippet to help us understand the use-case better?

@mbhave mbhave added the status: waiting-for-feedback We need additional information before we can continue label Jun 6, 2018
@mpryahin
Copy link
Author

mpryahin commented Jun 6, 2018

Does that mean you end up with two dispatcher servlets, one that's passed directly to the MockMvc constructor and one that's created as a bean?

No, it doesn't.

The inspiration for my use case I found here
I have a controller class that autowires a DispatcherServlet instance. The purpose of this controller is to accept batch requests each of which contains one or more self sufficient http requests (with its own headers, body, http method, url, etc...), then dispatch every sub-request via the autowired DispatcherSevlet instance, then receive responses, assemble them in a batch response and send it back to the client.

The main point to pay attention to is that my controller autowires a DispatcherServlet bean.

I tried to test my batch controller with the standard @AutoConfigureMockMvc, but failed due to no bean is exposed by the test configuration which is bootstrapped by @AutoConfigureMockMvc
You can see that MockMvc instance is created with preconfigured DispatcherServlet instance and this instance is not accessible from the spring context whatsoever.

I also saw that this servlet can be customized by customizers but it doesn't solve my main issue of not having a bean in the spring context.

After that I mimicked the test configuration class that is provided by Spring with the only one difference - I exposed the DispatcherServlet instance as a bean, something like that:

/**
 * A custom {@link MockMvc} configuration class exposes {@link DispatcherServlet} instance as a spring bean unlike the default
 * approach of creating a {@link MockMvc} instance via {@link MockMvcBuilderSupport#createMockMvc(Filter[], MockServletConfig, WebApplicationContext, RequestBuilder, List, List, List)}
 * method that inlines the {@link DispatcherServlet} instance as a method variable.
 *
 * @see org.springframework.boot.test.autoconfigure.web.servlet.MockMvcAutoConfiguration
 * @see org.springframework.test.web.servlet.MockMvcBuilderSupport
 */

@AutoConfigureWebMvc
public class MockMvcAutoConfiguration {

    @Autowired
    private WebApplicationContext webAppContext;

    @Bean
    public DispatcherServlet dispatcherServlet() throws Exception {
        WebApplicationContext wac = initWebAppContext();
        ServletContext servletContext = wac.getServletContext();
        MockServletConfig mockServletConfig = new MockServletConfig(servletContext);
        TestDispatcherServlet dispatcherServlet = new TestDispatcherServlet(wac);

        dispatcherServlet.init(mockServletConfig);
        return dispatcherServlet;
    }

    @Bean
    public MockMvc mockMvc(DispatcherServlet dispatcherServlet) {
        ServletContext servletContext = webAppContext.getServletContext();
        MockMvc mockMvc = new MockMvc((TestDispatcherServlet) dispatcherServlet, new Filter[0], servletContext);
        mockMvc.setDefaultRequest(MockMvcRequestBuilders.get("/"));
        mockMvc.setGlobalResultMatchers(Collections.emptyList());
        mockMvc.setGlobalResultHandlers(Collections.emptyList());
        return mockMvc;
    }

    private WebApplicationContext initWebAppContext() {
        ServletContext servletContext = webAppContext.getServletContext();
        ApplicationContext rootWac = WebApplicationContextUtils.getWebApplicationContext(servletContext);
        if (rootWac == null) {
            rootWac = webAppContext;
            ApplicationContext parent = webAppContext.getParent();
            while (parent != null) {
                if (parent instanceof WebApplicationContext && !(parent.getParent() instanceof WebApplicationContext)) {
                    rootWac = parent;
                    break;
                }
                parent = parent.getParent();
            }
            servletContext.setAttribute(WebApplicationContext.ROOT_WEB_APPLICATION_CONTEXT_ATTRIBUTE, rootWac);
        }
        return webAppContext;
    }
}

It would be great if Spring exposes a DispatcherServlet instance as a bean, so that a user doesn't need to implement his own configuration as I did to solve a simillar task.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jun 6, 2018
@philwebb
Copy link
Member

philwebb commented Jun 6, 2018

@mpryahin How are you able to call new MockMvc(...) when the constructor is package private? The TestDispatcherServlet class is also package private.

@mbhave mbhave added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Jun 6, 2018
@mpryahin
Copy link
Author

mpryahin commented Jun 6, 2018

@philwebb I had to put my custom configuration class into the right package to be able to instantiate it. This is another inconvenience I wanted to ask about but decided to postpone, hoping the initial request will eventually be fixed.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jun 6, 2018
@philwebb
Copy link
Member

philwebb commented Jun 6, 2018

@mpryahin Thanks! That's the information we were missing. We'll not be able to do that ourselves so we'll need a framework change to support this.

@philwebb
Copy link
Member

philwebb commented Jun 6, 2018

@rstoyanchev What are your thoughts on this. It would be nice if we could plug in our own dispatcher servlet to MockMvc but it would mean changes to the builder and the need to make TestDispatcherServlet public.

Another less invasive option might be to add a getter to MockMvc that returns the DispatcherServlet instance. That would allow us to register it as a bean.

@mbhave mbhave added the status: blocked An issue that's blocked on an external project change label Jun 6, 2018
@rstoyanchev
Copy link
Contributor

This is a very specific scenario for which MockMvc wasn't designed for. I'm not saying there is anything wrong with it, nor that it shouldn't be supported, but just that it hasn't been considered. I'm still missing a few details about how this works.

For example In MockMvc, the TestDispatcherServlet stores a single request attribute to store the MvcResult that is in turn used for expectations. How does that work in this scenario where logically there are multiple results and how are expectations declared against specific parts of the response?

Does each sub-dispatch (i.e. per batched request/part) have its own request and response, and how are the multiple responses aggregated back into a single multipart response?

@philwebb philwebb added the status: waiting-for-feedback We need additional information before we can continue label Jun 7, 2018
@mpryahin
Copy link
Author

mpryahin commented Jun 8, 2018

@rstoyanchev Let me clarify the implementation details first to make it all clear.

I have a controller which consumes a BatchRequest (more details later), HttpServletRequest and HttpServletResponse instances:

@PostMapping(path = "${cleverdata.dmpkit.spring.web.batch.endpoint:/batch}")
public BatchResponse batch(
    @RequestBody
    BatchRequest batchRequest, HttpServletRequest servletRequest,
    HttpServletResponse servletResponse) throws Exception {
    return batchRequestService.process(batchRequest, servletRequest, servletResponse);
}

BatchRequest - is a pojo which consists of sub-requests, which, as I sad before, are self-sufficient requests with the following fields:

HttpHeaders headers
byte[] body
String httpMethod
String url

The implementation iterates over the sub-requests objects, adapts each of them to the HttpServletRequest interface and invokes the DispatcherServlet.service(HttpServletRequest, HttpServletResponse) method. After that the the sub-response object is built out of the corresponding HttpServletResponse which was passed to the DispatcherServlet.service() method. All sub-response objects are aggregated into the BatchResponse pojo and returned back to a client. The simplified version looks like the following:


public BatchResponse process( 
	BatchRequest request, HttpServletRequest servletRequest,
	HttpServletResponse servletResponse) {

	BatchResponseBuilder batchResponseBuilder = BatchResponse.builder();
	for (BatchRequest.Part requestPart : parts) {
	    
	    BatchHttpServletRequest requestWrapper = new BatchHttpServletRequest(
	    	servletRequest, requestPart
	    );
	    BatchHttpServletResponse responseWrapper = new BatchHttpServletResponse(servletResponse);

	    servlet.service(requestWrapper, responseWrapper);

		BatchResponse.Part responsePart = BatchResponse.Part.builder()
	        .status(HttpStatus.valueOf(responseWrapper.getStatus()))
	        .body(responseWrapper.getContent())
	        .headers(responseWrapper.getHeaderObject())
	        .build();
	    batchResponseBuilder.part(responsePart);
	}
	return batchResponseBuilder.build();
}

The BatchRequest and BatchResponse objects are serialized/deserialized using a custom message converter that conforms rfc2046.

So from the MockMvc standpoint nothing has changed, it works the same way as before holding only one MvcResult instance which is evaluated against expectations.

It would be fantsatic if we had a chance to register aTestDispatcherSevlet instance as a spring bean, I believe my usecase is not so specific just a common way of leveraging spring abstractions (DispatcherServlet)

Thank you.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jun 8, 2018
@rstoyanchev
Copy link
Contributor

rstoyanchev commented Jun 8, 2018

Thanks @mpryahin for the extra detail ! The requestPart variable is not used in the sample, probably a typo, I'm guessing you meant to pass that into BatchHttpServletRequest?

Given the response is multipart content, probably the built-in ContentResultMatchers can't be used directly since they work on the full response content? Since the idea is for sub-requests to be unaware of the batching, have you considered testing them one by one, and directly, i.e. using the URL, HTTP method, and body of the would-be batch request? That would make it possible to use the built-in ContentResultMatchers to verify the response. Separately test the behavior of the batching layer, which is generic in any case, and the batch sub-responses could be mocked out.

From the proposed options, I would go with the least invasive one, i.e. expose a getter on MockMvc which complements the DispatcherServletCustomizer. We'll just have to explain that providing access to the DispatcherServlet instance that underlies MockMvc is only meant for cases where some component happens to delegate to DispatcherServlet at runtime and therefore needs be injected with it. Anything more would not be explicitly supported.

@mpryahin
Copy link
Author

mpryahin commented Jun 8, 2018

@rstoyanchev Thanks for your quick reply!

The requestPart variable is not used in the sample, probably a typo, I'm guessing you meant to pass that into BatchHttpServletRequest?

You're absolutely right, sorry for the typo, I've fixed it.

Given the response is multipart content, probably the built-in ContentResultMatchers can't be used directly since they work on the full response content?

All of them work great unless I have to match a request body, in this case, a custom matcher is used to match against MockHttpServletResponse object.

Since the idea is for sub-requests to be unaware of the batching, have you considered testing them one by one...

The idea of this Batch Controller is that it is implemented as a distinct library powered by Spring Boot autoconfiguration mechanism so that any RESTful service just includes this library to its dependencies and gains a batch endpoint for free! That's why I have no ability to test underlying controller methods one by one.

Separately test the behavior of the batching layer, ...

Absolutely agree, but I would like to have a kind of integration test which insures the implementation works as expected with Spring DesipatcherServlet

Thanks,
Best regards, Mike.

@rstoyanchev
Copy link
Contributor

All of them work great unless I have to match a request body

Did you mean response body? The response has multipart content, so I wouldn't expect any of the ContentResultMatchers to just work unless given only the content of a single part.

I've created https://jira.spring.io/browse/SPR-16924.

@mpryahin
Copy link
Author

mpryahin commented Jun 8, 2018

@rstoyanchev sure, I did mean response body. Thanks a lot for creating an issue.

@philwebb
Copy link
Member

SPR-16924 has been resolved.

@philwebb philwebb removed the status: blocked An issue that's blocked on an external project change label Jun 11, 2018
@mbhave mbhave removed the status: feedback-provided Feedback has been provided label Jul 10, 2018
@mbhave
Copy link
Contributor

mbhave commented Jul 12, 2018

This isn't as straightforward as I thought.
As things stand, adding the DispatcherServlet bean to MockMvcAutoConfiguration leads to a cycle between the DefaultMockMvcBuilder, MockMvc and DispatcherServlet beans. The builder has customizers which look for the DispatcherServlet (SpringBootMockMvcBuilderCustomizer has an addFilters method). To be more specific, SpringBootMockMvcBuilderCustomizer creates ServletContextInitializerBeans which looks for Servlet beans.

@snicoll and I had a chat about this and as the customizer does part of what WebMvcAutoConfiguration does (adding filters and servlets from the context, creating a registration bean for them if necessary), there is a difference between the real and mock setup.

@mbhave mbhave added the for: team-attention An issue we'd like other members of the team to review label Jul 12, 2018
@wilkinsona
Copy link
Member

wilkinsona commented Jul 13, 2018

As @mbhave says above, there's a cycle because ServletContextInitializerBeans looks for ServletRegistrationBean instances. Those beans aren't actually used as the customiser only cares about filters. I think we could break the cycle by allowing ServletContextInitializerBeans to be configured to only look for specific ServletContextInitializer subclasses, defaulting to ServletContextIntializer.class if none are specified. So, something like this:

public ServletContextInitializerBeans(ListableBeanFactory beanFactory,
		Class<? extends ServletContextInitializer>... initializerTypes) {

And then being used like this:

private void addServletContextInitializerBeans(ListableBeanFactory beanFactory) {
	for (Class<? extends ServletContextInitializer> initializerType : this.initializerTypes) {
		for (Entry<String, ? extends ServletContextInitializer> initializerBean : getOrderedBeansOfType(
				beanFactory, initializerType)) {
			addServletContextInitializerBean(initializerBean.getKey(),
					initializerBean.getValue(), beanFactory);
		}
	}
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme: testing Issues related to testing type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

6 participants