Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Remove AllEncompassingFormHttpMessageConverter from WebMvcConfigurationSupport #32917

Closed
poutsma opened this issue May 28, 2024 · 3 comments
Closed
Assignees
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) status: invalid An issue that we don't feel is valid

Comments

@poutsma
Copy link
Contributor

poutsma commented May 28, 2024

WebMvcConfigurationSupport currently includes the AllEncompassingFormHttpMessageConverter, see

.

Including a form converter on the server side makes no sense, as the servlet environment already provides form support. Effectively, including this converter can result in controllers returning form data, which makes no sense. So, the AllEncompassingFormHttpMessageConverter should be removed from WebMvcConfigurationSupport.

@poutsma poutsma added in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement labels May 28, 2024
@poutsma poutsma added this to the 6.2.0-M4 milestone May 28, 2024
@poutsma poutsma self-assigned this May 28, 2024
@snicoll
Copy link
Member

snicoll commented Jun 4, 2024

Here is a project that attempts to demonstrate the problem of writing a Map: https://github.com/snicoll-scratches/spring-framework-32917

There are four tests: one local using MockMvc and three using a server with TestRestTemplate, RestTemplate and RestClient.

The MockMvc test fails with:

MockHttpServletRequest:
      HTTP Method = GET
      Request URI = /data
       Parameters = {}
          Headers = []
             Body = null
    Session Attrs = {}

Handler:
             Type = com.example.DemoController
           Method = com.example.DemoController#plain()

Async:
    Async started = false
     Async result = null

Resolved Exception:
             Type = null

ModelAndView:
        View name = null
             View = null
            Model = null

FlashMap:
       Attributes = null

MockHttpServletResponse:
           Status = 200
    Error message = null
          Headers = [Content-Type:"application/x-www-form-urlencoded", Content-Length:"20"]
     Content type = application/x-www-form-urlencoded
             Body = name=John+Doe&age=42
    Forwarded URL = null
   Redirected URL = null
          Cookies = []

MockHttpServletRequest:
      HTTP Method = GET
      Request URI = /data
       Parameters = {}
          Headers = []
             Body = null
    Session Attrs = {}

Handler:
             Type = com.example.DemoController
           Method = com.example.DemoController#plain()

Async:
    Async started = false
     Async result = null

Resolved Exception:
             Type = null

ModelAndView:
        View name = null
             View = null
            Model = null

FlashMap:
       Attributes = null

MockHttpServletResponse:
           Status = 200
    Error message = null
          Headers = [Content-Type:"application/x-www-form-urlencoded", Content-Length:"20"]
     Content type = application/x-www-form-urlencoded
             Body = name=John+Doe&age=42
    Forwarded URL = null
   Redirected URL = null
          Cookies = []

Content type [application/x-www-form-urlencoded] is not compatible with [application/json]
java.lang.AssertionError: Content type [application/x-www-form-urlencoded] is not compatible with [application/json]
	at org.springframework.test.util.AssertionErrors.fail(AssertionErrors.java:39)
	at org.springframework.test.util.AssertionErrors.assertTrue(AssertionErrors.java:73)
	at org.springframework.test.web.servlet.result.ContentResultMatchers.lambda$contentTypeCompatibleWith$1(ContentResultMatchers.java:107)
	at org.springframework.test.web.servlet.MockMvc$1.andExpect(MockMvc.java:214)
	at com.example.DemoControllerTests.getData(DemoControllerTests.java:29)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)

The test with the RestClient fails as follows:

Could not extract response: no suitable HttpMessageConverter found for response type [interface java.util.Map] and content type [application/x-www-form-urlencoded]
org.springframework.web.client.UnknownContentTypeException: Could not extract response: no suitable HttpMessageConverter found for response type [interface java.util.Map] and content type [application/x-www-form-urlencoded]
	at org.springframework.web.client.DefaultRestClient.readWithMessageConverters(DefaultRestClient.java:229)
	at org.springframework.web.client.DefaultRestClient$DefaultResponseSpec.readBody(DefaultRestClient.java:689)
	at org.springframework.web.client.DefaultRestClient$DefaultResponseSpec.toEntityInternal(DefaultRestClient.java:659)
	at org.springframework.web.client.DefaultRestClient$DefaultResponseSpec.toEntity(DefaultRestClient.java:648)
	at com.example.DemoControllerRestClientTests.getData(DemoControllerRestClientTests.java:46)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)

Switching to Spring Framework 6.2.0-M3 and they all work.

@rstoyanchev
Copy link
Contributor

rstoyanchev commented Jun 4, 2024

It's not uncommon to access form data through an @RequestBody parameter like in this test I think, so I'm not sure we can just exclude FormHttpMessageConverter from server configuration. On the response side, the converter enables writing multipart that I believe is also a valid use case.

@poutsma
Copy link
Contributor Author

poutsma commented Jun 6, 2024

It's not uncommon to access form data through an @RequestBody parameter like in this test I think, so I'm not sure we can just exclude FormHttpMessageConverter from server configuration. On the response side, the converter enables writing multipart that I believe is also a valid use case.

Understood. I guess we have no other way out that to revert #32826, as supporting maps will break too many applications. Fortunately there is still #32832, which we can keep.

@poutsma poutsma closed this as not planned Won't fix, can't repro, duplicate, stale Jun 6, 2024
@poutsma poutsma added status: invalid An issue that we don't feel is valid and removed type: enhancement A general enhancement labels Jun 6, 2024
@poutsma poutsma removed this from the 6.2.0-M4 milestone Jun 6, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) status: invalid An issue that we don't feel is valid
Projects
None yet
Development

No branches or pull requests

3 participants