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

Avoid setting default Content-Type in content methods without explicit MediaType #5964

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

JiwonKKang
Copy link
Contributor

@JiwonKKang JiwonKKang commented Oct 30, 2024

Motivation:

HttpResponse could generate duplicate Content-Type headers if a user manually added a Content-Type header after setting the content with content(String). This can cause client errors or unexpected behavior when parsing the response.

Modifications:

  • Updated the content methods without explicit MediaType in AbstractHttpMessageBuilder to avoid automatically setting the Content-Type header.
  • Added a new content(HttpData) method to HttpMessageSetters that allows setting content without adding any headers by default.
  • Adjusted test cases to ensure:
    • The Content-Type header is not automatically set when content(String) is used without MediaType.
    • No duplicate Content-Type headers are generated in response headers.

Result:

  • The HttpResponse now avoids setting a default Content-Type header when content(String) is used, thereby reducing the risk of duplicate headers.

@JiwonKKang JiwonKKang changed the title Avoid setting default Content-Type in content(String) Avoid setting default Content-Type in content methods without explicit MediaType Oct 30, 2024
@@ -50,6 +50,7 @@ protected void configure(ServerBuilder sb) {
return HttpResponse.builder()
.ok()
.content("OK\n")
.header(HttpHeaderNames.CONTENT_TYPE, "text/plain; charset=utf-8")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we can simply alter the behavior since it can result in breaking changes like seen in the tests.

Rather, I think a more reasonable approach would be:

  1. Remember if a String content has been set for a request/response
  2. At build time, if a content-type hasn't been set, set text/plain as the content-type

@jrhee17 jrhee17 added this to the 1.32.0 milestone Nov 5, 2024
@jrhee17 jrhee17 added the defect label Nov 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants