Avoid misconfiguration of controller overrides [DATAREST-1591]

[spring-projects-issues]

Karsten Ludwig Hauser opened DATAREST-1591 and commented

Calling

mvc.perform(MockMvcRequestBuilders.patch("/person/" + EXPECTED_PERSON_ID).contentType(JSON_PATCH_JSON)
                .content(patchPayload)).andDo(print()).andExpect(status().isNoContent());

results in

MockHttpServletRequest:
      HTTP Method = PATCH
      Request URI = /person/1
       Parameters = {}
          Headers = [Content-Type:"application/json-patch+json;charset=UTF-8", Content-Length:"52"]
             Body = [{"op":"replace","path":"/firstName","value":"Max"}]
    Session Attrs = {}Handler:
             Type = nullAsync:
    Async started = false
     Async result = nullResolved Exception:
             Type = org.springframework.web.HttpRequestMethodNotSupportedExceptionModelAndView:
        View name = null
             View = null
            Model = nullFlashMap:
       Attributes = nullMockHttpServletResponse:
           Status = 405
    Error message = Request method 'PATCH' not supported
          Headers = [Allow:"GET"]
     Content type = null
             Body = 
    Forwarded URL = null
   Redirected URL = null
          Cookies = []

when a custom RestController having the same mapping path as the repository.

Please check with the project behind the reference url.

 

---

Affects: 3.3.6 (Neumann SR6)

Reference URL: https://github.com/khauser/JsonPatchShowcase

[spring-projects-issues]

Oliver Drotbohm commented

This fundamentally works as designed. You need to make sure that your controller is not picked up by Spring MVC by accident. Your current controller setup unfortunately does so.

The crucial bits to get this to work are:

• Remove @RestController as well as the type level mapping @RequestMapping(value = "/person"). This is a known limitation of Spring MVC.

This makes all requests up until the PATCH one work. For the one accessing the the item resources work, too, remove the produces clause from the implemented method's mapping. application/json and application/hal+json are not compatible on the media type level be cause media types don't know "inheritance". Beyond that, the produces clause is simply not necessary.

Finally @EnableHypermediaSupport is unnecessary as that's automatically enabled through Spring Data REST.

[spring-projects-issues]

Karsten Ludwig Hauser commented

Thanks

[spring-projects-issues]

Oliver Drotbohm commented

After thinking about this for a while, let me reopen the ticket and take the chance to improve a bit to avoid others have to waste time on this and make the time you invested a bit more worthwhile.

We can detect this problem at bootstrap time and we can reject those misconfigurations. A controller looking like this is never a valid one and we can tell you that.

BasePathAwareHandlerMapping.isHandler(…) can inspect the given bean type for the annotations causing the issues and throw an exception explaining the situation to give advice on how to fix it.

I'll schedule that for inclusion in the next milestone.

 

[spring-projects-issues]

Karsten Ludwig Hauser commented

I adjusted the project and all is working fine now.  @RestController is still needed to make findByFirstName available.

But now I also have another project where it only works with @RestController. So weird..

It also would be great if classes like ReplaceOperation, AddOperation, etc. would be public. I would like to use them on client side.

Plus RestMediaTypes.JSON_PATCH_JSON_VALUE to easly add it to a FeignCLient.

[spring-projects-issues]

Oliver Drotbohm commented

I adjusted the project and all is working fine now.  @RestController is still needed to make findByFirstName available.

But now I also have another project where it only works with @RestController. So weird..

That doesn't seem to be the case. The test case works just fine with the annotation removed.

It also would be great if classes like ReplaceOperation, AddOperation, etc. would be public. I would like to use them on client side.

Plus RestMediaTypes.JSON_PATCH_JSON_VALUE to easly add it to a FeignCLient.

For one, this doesn't have to do anything with the issue discussed here and we have to perform a bit of ticket hygiene to not get lost in "Oh, and this… oh, and that".
Second – and I think that's been discussed in another ticket already – that's unlikely to happen for two reasons: first, the implementation was never designed to be usable outside of what Spring Data REST does. I.e. there are dependencies to all sorts of things that should not make it into your clients. Also, in general, why would you want to to put a server library on your client's classpath? There are a couple of well-usable options available out there.

[odrotbohm]

Duplicates #1342. See the my latest comment over there.