Skip to content
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

MockHttpServletRequestBuilder should support multiple locales [SPR-15116] #19683

Closed
spring-issuemaster opened this issue Jan 9, 2017 · 5 comments

Comments

@spring-issuemaster
Copy link
Collaborator

commented Jan 9, 2017

Louis-Rémi opened SPR-15116 and commented

It's possible to set multiple locales to MockHttpServletRequest with setPreferredLocales or addPreferredLocale.

However, MockHttpServletRequestBuilder lacks the method(s) to set it.

I thought of extending MockHttpServletRequestBuilder to work around this but MockMvc is final, so it's not possible to return anything else than a MockHttpServletRequestBuilder

I suggest that the MockHttpServletRequestBuilder should allow to set locales like it's possible in the underlying MockHttpServletRequest.


Affects: 4.3.1

Referenced from: commits spring-projects/spring-integration@c886b3b

0 votes, 5 watchers

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 9, 2017

Louis-Rémi commented

I've found a way, or workaround should I say, by adding a new RequestPostProcessor containing these locales to the builder.

I say workaround because it's not consistent with the way of setting the locales using the already existing MockHttpServletRequestBuilder#locale

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 12, 2017

Juergen Hoeller commented

I've added a corresponding locale(Locale...) method to MockHttpServletRequestBuilder, taking the opportunity to revise the builder's content type handling as well.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 13, 2017

Artem Bilan commented

Juergen Hoeller,

Is that really expected behavior do not parse Content-Type header any more?
02d727f#diff-ffd5e2cdd2795d57fc0a15da17156010L206

If that, the MockMvcClientHttpRequestFactory doesn't populate contentType():

MockHttpServletRequestBuilder requestBuilder = request(httpMethod, uri.toString());
requestBuilder.content(getBodyAsBytes());
requestBuilder.headers(getHeaders());
MvcResult mvcResult = MockMvcClientHttpRequestFactory.this.mockMvc.perform(requestBuilder).andReturn();

We have a test-case in Spring Integration which accidentally relies on the logic in the MockHttpServletRequestBuilder:

if (this.content != null && this.contentType != null) {
	MediaType mediaType = MediaType.parseMediaType(this.contentType);
	if (MediaType.APPLICATION_FORM_URLENCODED.includes(mediaType)) {
		addRequestParams(request, parseFormData(mediaType));
	}
}

But since this fix doesn't populate MockHttpServletRequestBuilder.contentType property from the headers(), our test fails.

Of course, we have to fix our test some how to really extract proper queryString, but that is already a different story.

Although I really would like to see queryString in the MockHttpServletRequest based on the provided parameters, because MockHttpServletRequestBuilder doesn't have queryString option...

/CC Gary Russell

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 13, 2017

Juergen Hoeller commented

Oops, that wasn't quite intended. I just meant to get rid of the double field vs header handling in both the builder and MockHttpServletRequest itself. That local content type check in the builder nevertheless needs to keep working; reopening this issue for it.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 16, 2017

Juergen Hoeller commented

We're checking the request's resolved content type there now instead of the builder's local contentType variable, restoring the previous detection of a form body.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.