-
Notifications
You must be signed in to change notification settings - Fork 37.7k
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
Fix MockHttpServletRequest.java #32718
Conversation
In MockHttpServletRequest 'application/json; Modified so that the 'charset=utf-8' format is no longer used.
Change variable order
@cnabro it's not clear how you get the problem. Using this: mockMvc.perform(post("/courses")
.contentType(MediaType.APPLICATION_JSON)
.content("{\"title\":\"Test Course\"}") results in:
|
@rstoyanchev Test Code@ExtendWith(SpringExtension.class)
@WebMvcTest(CourseController.class)
public class CourseControllerTest {
@Autowired
private MockMvc mockMvc;
@Test
public void testCreateCourse() throws Exception {
Course course = new Course();
course.setId(1L);
course.setTitle("Test Course");
given(courseService.saveCourse(any(Course.class))).willReturn(course);
mockMvc.perform(post("/courses")
.contentType(MediaType.APPLICATION_JSON)
.content("{\"title\":\"Test Course\"}"))
.andExpect(status().isOk())
.andExpect(jsonPath("$.id").value(1L))
.andExpect(jsonPath("$.title").value("Test Course"));
}
} Stacktrace
|
if (StringUtils.hasLength(this.characterEncoding) && !this.contentType.toLowerCase().contains(CHARSET_PREFIX)) { | ||
value += ';' + CHARSET_PREFIX + this.characterEncoding; | ||
} | ||
doAddHeaderValue(HttpHeaders.CONTENT_TYPE, value, true); | ||
} | ||
} | ||
|
||
private boolean isJsonContentType(String contentType) { | ||
return contentType.toLowerCase().startsWith("application/json"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to validate that the contentType is not null :)
An approach would be:
return contentType != null && contentType.toLowerCase().startsWith("application/json");
I had a go at reproducing this to the point of getting the charset part in the content-type header (when using After discussing it with the team, it looks like there is an opportunity to make a more general change: the (see the last part of https://datatracker.ietf.org/doc/html/rfc8259#section-11) @cnabro would you like to tackle that, as a new PR superseding this one? |
@simonbasle Do you mean to refer to |
@cnabro yeah sorry for the confusion I meant so yeah go for it, and don't forget to add tests to |
closing as declined, awaiting new PR targeting |
Description
application/json; charset=utf-8
is no longer supported, but MockHttpServletRequest still uses charset by default, so the mockMVC test fails.Sample Test