Skip to content

[#3850] Fix the problem when setting content-type by ResponseEntity.header() does not take effect#3852

Merged
liubao68 merged 1 commit into
apache:masterfrom
Sayaka617:master
Jul 17, 2023
Merged

[#3850] Fix the problem when setting content-type by ResponseEntity.header() does not take effect#3852
liubao68 merged 1 commit into
apache:masterfrom
Sayaka617:master

Conversation

@Sayaka617

@Sayaka617 Sayaka617 commented Jul 9, 2023

Copy link
Copy Markdown
Contributor

Follow this checklist to help us incorporate your contribution quickly and easily:

  • Make sure there is a JIRA issue filed for the change (usually before you start working on it). Trivial changes like typos do not require a JIRA issue. Your pull request should address just this issue, without pulling in other changes.
  • Each commit in the pull request should have a meaningful subject line and body.
  • Format the pull request title like [SCB-XXX] Fixes bug in ApproximateQuantiles, where you replace SCB-XXX with the appropriate JIRA issue.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Run mvn clean install -Pit to make sure basic checks pass. A more thorough check will be performed on your pull request automatically.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

@Sayaka617 Sayaka617 changed the title [#3850] Fix the problem when set content-type by ResponseEntity.header() [#3850] Fix the problem when setting content-type by ResponseEntity.header() does not take effect Jul 9, 2023
Comment on lines +47 to +51
if (responseEx.getContentType() != null) {
responseEx.setHeader(HttpHeaders.CONTENT_TYPE.toString(), responseEx.getContentType());
} else {
responseEx.setHeader(HttpHeaders.CONTENT_TYPE.toString(), part.getContentType());
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do you find this problem in spring boot embedded tomcat or standalone tomcat? And would you mind to add a test case in demo folders (there are many test cases and you can run each demo from your IDE).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

it's in spring boot embedded tomcat .
Of course , I'll add some test cases .

@liubao68

liubao68 commented Jul 12, 2023

Copy link
Copy Markdown
Contributor

@Sayaka617 Do not use force-push if possible, so that we can see what's changed after last review. Do don't worry about the commits, we can use squash merge when merge the PR.
I triggered the CI again as this is your first PR.

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

Merging #3852 (d8d31f7) into master (008a3a4) will increase coverage by 0.00%.
The diff coverage is 72.72%.

❗ Current head d8d31f7 differs from pull request most recent head c57f5cf. Consider uploading reports for the commit c57f5cf to get more accurate results

@@            Coverage Diff            @@
##             master    #3852   +/-   ##
=========================================
  Coverage     43.97%   43.97%           
+ Complexity     6087     6085    -2     
=========================================
  Files          1557     1557           
  Lines         38278    38288   +10     
  Branches       3512     3513    +1     
=========================================
+ Hits          16833    16839    +6     
- Misses        20134    20139    +5     
+ Partials       1311     1310    -1     
Impacted Files Coverage Δ
...rvicecomb/foundation/vertx/http/DownloadUtils.java 9.52% <0.00%> (-1.01%) ⬇️
...comb/demo/springmvc/client/TestDownloadSchema.java 100.00% <100.00%> (ø)

... and 7 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@Sayaka617

Copy link
Copy Markdown
Contributor Author

@Sayaka617 Do not use force-push if possible, so that we can see what's changed after last review. Do don't worry about the commits, we can use squash merge when merge the PR. I triggered the CI again as this is your first PR.

okay,i got it . I'll avoid this next time . thank you

@liubao68

Copy link
Copy Markdown
Contributor

@Sayaka617 This PR is OK now. Do you mind to cherry-pick this PR to 2.8.x branch?

@Sayaka617

Copy link
Copy Markdown
Contributor Author

@Sayaka617 This PR is OK now. Do you mind to cherry-pick this PR to 2.8.x branch?

Of course, I've just create a backport pr to 2.8.x.

@liubao68
liubao68 merged commit 9bfb896 into apache:master Jul 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants