Make it easy to extend MockMvc builders with additional syntax [SPR-11497]

[spring-projects-issues]

Rob Winch opened SPR-11497 and commented

Currently it is verbose to support extensions to MockMvc builders. For example, with Spring Security's new Spring MVC Test support, users would have to do something like:

@WithUser(roles="ADMIN")
public class WithUserClassLevelAuthenticationTests {

    @Before
    public void setup() {
        mvc = MockMvcBuilders
                .webAppContextSetup(context)
                .addFilters(springSecurityFilterChain)
                .defaultRequest(get("/").with(securityContext()))
                .build();
    }

Instead something like the following would be ideal:

@WithUser(roles="ADMIN")
public class WithUserClassLevelAuthenticationTests {

    @Before
    public void setup() {
        mvc = MockMvcBuilders
                .webAppContextSetup(context)
                .with(springSecurity(springSecurityFilterChain))
                .build();
    }

---

Issue Links:

• SEC-2587 Improvements in Spring Test ("is depended on by")
• SEC-2686 Add SecurityMockMvcConfigurer

• 16625 Further updates to MockMvcConfigurer SPI

[spring-projects-issues]

Rossen Stoyanchev commented

Indeed on the level of MockMvcRequestBuilders we support a .with(RequestPostProcessor) mechanism specifically for frameworks like Spring Security to provide additional syntax for initializing a request.

We need a similar mechanism for encapsulating MockMvc setup logic.

[spring-projects-issues]

Rossen Stoyanchev commented

robwinch, I've added something with commit c2b0fa. That should take care of it but it would be good to confirm some time before RC2.

[spring-projects-issues]

Rob Winch commented

Rossen Stoyanchev Thanks for the response. It does seem to work, but I had a few comments:

• It seems you changed your mind on calling it a PostProcessor which would be consistent with some of the other APIs. Is there a reason for that?
• Any thoughts about providing an adapter?
• It seems weird to me that the API uses a MockMvcBuilder and references implementation specific classes like RequestBuilder and AbstractMockMvcBuilder
• Very related..the casting doesn't seem all that ideal to me. Do you think it might make sense to use java generics?
• I'm wondering if there was any thought to naming AbstractMockMvcBuilder.add(MockMvcConfigurer) AbstractMockMvcBuilder.with(MockMvcConfigurer) instead? This would seem to better align with MockHttpServletRequestBuilder.with(RequestPostProcessor). Perhaps the add makes more sense with a configurer, but I think it reads quite a bit better using .with. For example, right now it is:

mvc = MockMvcBuilders
        .webAppContextSetup(context)
        .add(springSecurity())
        .build();

vs

mvc = MockMvcBuilders
        .webAppContextSetup(context)
        .with(springSecurity())
        .build();

[spring-projects-issues]

Rossen Stoyanchev commented

Yes I did change my mind on the name. The other example of a similar framework-oriented extension, i.e. RequestPostProcessor does indeed post-process the MockHttpServletRequest after it is created and built so it's name is fitting. A MockMvcPostProcessor may have been the expected outcome here but MockMvc was not designed to be configurable directly. It exposes only one public method to perform requests. So basically we are talking about hooking into the process of building a MockMvc with a MockMvcBuilder. And MockMvcBuilderPostProcessor doesn't really make sense. This is not post-processing the builder but rather it's hooking into the builder without actually adding methods to it, i.e. helping to pre-configure it in some way.

As for the access to RequestBuilder I fail to see how else do you manage to pre-configure the Principal for every performed request and do so on the default RequestBuilder configured by the application?

You're right on the point about having to cast MockMvcBuilder. I'll see about some changes there. Currently the builder only has the one build method. That's probably where part of the issue lies but so far it hasn't been an issue.

I did consider .with as the method name but at least initially didn't like it. Consider a more complete example with other methods used:

MockMvcBuilders.webAppContextSetup(this.wac)
    .addFilter(new CharacterEncodingFilter() )
    .alwaysExpect(status().isOk())
    .defaultRequest(get("/").contextPath("/mywebapp"))
    .build()

Somehow having .with next to all that doesn't read well. Either .add or .configure seems to fit better the overall MockMvcBuilder "language".

[spring-projects-issues]

Rob Winch commented

Any thoughts to my comment about providing an adapter (i.e. MockMvcConfigurerAdapter)? This seems to be fairly standard Spring practice and would mean implementations would only need to implement methods they are interested in.

And MockMvcBuilderPostProcessor doesn't really make sense. This is not post-processing the builder but rather it's hooking into the builder without actually adding methods to it, i.e. helping to pre-configure it in some way.

I don't think post processing needs to imply that the object type was changed. I was drawing the analogy of a BeanPostProcessor which may just change properties of a Bean. Admittedly, BeanPostProcessor does allow you to change the Object so perhaps your line of thinking makes more sense.

As for the access to RequestBuilder I fail to see how else do you manage to pre-configure the Principal for every performed request and do so on the default RequestBuilder configured by the application?

This is true. While not obvious, this comment was related to the casting of MockMvcBuilder. What I was trying to get at is...if the API already contains implementation specific APIs on it (i.e. RequestBuilder) it seems that perhaps changing the MockMvcBuilder to a more specific class. Overall, I was trying to help make the case for removing my need of casting the objects. It seems that you already agree that it would be nice to not have to perform casting.

You're right on the point about having to cast MockMvcBuilder. I'll see about some changes there.

You may have noticed this already, but just to be certain I am also talking about needing to cast the default RequestBuilder. Right now my code looks like this:

Filter springSecurityFilterChain = applicationContext.getBean(BeanIds.SPRING_SECURITY_FILTER_CHAIN, Filter.class);

AbstractMockMvcBuilder mockMvc = (AbstractMockMvcBuilder) mockMvcBuilder;
mockMvc.addFilters(springSecurityFilterChain);

MockHttpServletRequestBuilder requestBuilder = (MockHttpServletRequestBuilder) defaultRequestBuilder;
if(requestBuilder == null) {
    mockMvc.defaultRequest(get("/").with(testSecurityContext()));
} else {
    requestBuilder.with(testSecurityContext());
}

There isn't anything I can do with a RequestBuilder object without needing to perform a cast.

Somehow having .with next to all that doesn't read well. Either .add or .configure seems to fit better the overall MockMvcBuilder "language". Perhaps just me, but I guess I still feel like add(springSecurity()) reads very well:

MockMvcBuilders.webAppContextSetup(this.wac)
    .add(springSecurity())
    .addFilter(new CharacterEncodingFilter() )
    .alwaysExpect(status().isOk())
    .defaultRequest(get("/").contextPath("/mywebapp"))
    .build()

Perhaps you had something else in mind (i.e. maybe I should rename the springSecurity() static method)? This is quite opinionated, so feel free to disregard this feedback if you disagree.