Skip to content

Preserve delegate content type for multipart parameters#3382

Open
seonwooj0810 wants to merge 2 commits into
OpenFeign:masterfrom
seonwooj0810:fix/issue-2813-multipart-part-content-type
Open

Preserve delegate content type for multipart parameters#3382
seonwooj0810 wants to merge 2 commits into
OpenFeign:masterfrom
seonwooj0810:fix/issue-2813-multipart-part-content-type

Conversation

@seonwooj0810

Copy link
Copy Markdown
Contributor

Fixes #2813

Problem

When a multipart request contains a POJO/JSON part, DelegateWriter encodes the value through the delegate Encoder (e.g. the Jackson encoder), which correctly sets Content-Type: application/json on the throwaway RequestTemplate. That header was then discarded, and SingleParameterWriter hard-coded Content-Type: text/plain. As a result, JSON parts were emitted as text/plain instead of application/json (see the report in #2813).

Change

  • DelegateWriter now reads the Content-Type produced by the delegate and forwards it to a new SingleParameterWriter.writeWithContentType(output, key, value, contentType) method.
  • When the delegate sets no content type, the writer falls back to the previous text/plain; charset=<charset> behaviour, so plain single parameters are unaffected.
  • The existing write(output, key, value) template method is preserved and simply delegates with a null content type.

Tests

Added feign.form.multipart.DelegateWriterTest:

  • usesContentTypeFromDelegate — asserts a delegate that sets application/json produces Content-Type: application/json (and not text/plain).
  • fallsBackToTextPlainWhenDelegateSetsNoContentType — asserts the text/plain; charset=UTF-8 fallback when the delegate sets no content type.

Test evidence

Ran the form module test suite (and its upstream dependencies) locally:

Tests run: 2, Failures: 0, Errors: 0, Skipped: 0 -- feign.form.multipart.DelegateWriterTest
... full feign-core + feign-form suites green ...
BUILD SUCCESS

Verification done: confirmed the hard-coded text/plain still present on master in SingleParameterWriter#write; confirmed no in-flight PR referencing the issue and no active self-claim (maintainer invited a PR with tests on 2026-05-27); fix touches only .java files; full form module test suite passes after the change.

@velo velo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the focused fix and for adding coverage around the delegated part content type. I cannot approve this while the required CI check is failing.\n\nGate results:\n1) Test coverage: PASS.\n2) Backwards compatibility: PASS.\n3) Security: PASS.\n\nBlocking issue:\n- Required check is currently failing for this PR. Please get the build green before this can be approved or merged.

@seonwooj0810 seonwooj0810 force-pushed the fix/issue-2813-multipart-part-content-type branch from 38d942f to 789f3ef Compare June 6, 2026 03:49
@seonwooj0810

Copy link
Copy Markdown
Contributor Author

Thanks for the review @velo. The failing pr-build was feign.vertx.ConnectionsLeakTests.http2NoConnectionLeak in feign-vertx5-test — unrelated to this change (this PR only touches feign-form multipart). That flakiness was fixed on master by 6769d9a ("Fix flaky vertx http2NoConnectionLeak test by using h2c prior knowledge"), so I rebased onto the latest master to pick it up and retrigger CI.

@seonwooj0810

Copy link
Copy Markdown
Contributor Author

Quick follow-up: CI is now fully green on the rebased branch — ci/circleci: pr-build, ci/circleci: setup-environment, and security/snyk all pass. The earlier failure was the unrelated flaky feign-vertx5-test http2NoConnectionLeak test, which was fixed on master and is no longer present after the rebase. This PR only touches feign-form multipart handling. Happy to make any further adjustments if needed.

@velo velo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the update. The new regression tests cover both delegated content-type propagation and the existing fallback; the change preserves the protected write path and public behavior, and introduces no security issue. Required checks are green. The branch still conflicts with the base branch, so it cannot be merged until the conflict is resolved.

`DelegateWriter` encodes a parameter through the delegate `Encoder` (e.g.
the Jackson encoder), which sets the correct `Content-Type` (such as
`application/json`) on the throwaway `RequestTemplate`. That header was
discarded and `SingleParameterWriter` hard-coded `text/plain`, so JSON
parts of a multipart request were emitted as `text/plain`.

`DelegateWriter` now reads the `Content-Type` produced by the delegate and
passes it through to `SingleParameterWriter.writeWithContentType`, falling
back to `text/plain; charset=<charset>` when the delegate sets no content
type. The previous behaviour for plain single parameters is unchanged.

Fixes OpenFeign#2813

Signed-off-by: seonwoo_jung <79202163+seonwooj0810@users.noreply.github.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@seonwooj0810 seonwooj0810 force-pushed the fix/issue-2813-multipart-part-content-type branch from 789f3ef to ba1cf15 Compare June 9, 2026 14:53
@seonwooj0810

Copy link
Copy Markdown
Contributor Author

Thanks for the approval @velo. I've rebased the branch onto the latest master to resolve the conflict in DelegateWritermaster had refactored body extraction to use RequestTemplate#requestBody() / Request.Body#writeToString, so I adapted the content-type propagation to route the delegate's Content-Type through SingleParameterWriter#writeWithContentType on top of that new API. The fix and its regression tests are otherwise unchanged. The branch now reports mergeable (head ba1cf15a).

@seonwooj0810

Copy link
Copy Markdown
Contributor Author

The current pr-build failure (CircleCI 12024) is a test-compilation error in feign-core that is pre-existing on master, not introduced by this PR: JavaLoggerTest and LoggerRebufferTest still call the Request.create(..., byte[], Charset) overload that was removed in f9159cf ("Charset cannot be converted to RequestTemplate"). Master's own snapshot builds fail with the identical error (CircleCI 11938 and 11953), and those two test files are byte-identical to master on this branch — the build never reaches feign-form. cc @velo in case the master fix isn't already in flight.

While reproducing this locally I noticed the rebased DelegateWriterTest also still used the removed RequestTemplate#body(String), which would have failed compilation once feign-core is fixed. I've pushed d9d46e9 updating it to template.body(Request.Body.of(...)) to match the new streaming body API. With that change all feign-form tests pass locally (31/31, including DelegateWriterTest). Once master compiles again, a CI re-run here should go green.

@velo velo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for adapting the regression test to the current Request.Body API. The test coverage, backwards-compatibility, and security gates pass, but the required ci/circleci: pr-build check is failing, so this cannot be approved or merged until CI is green.

@seonwooj0810

Copy link
Copy Markdown
Contributor Author

To unblock the CI gate here, I've opened #3406 fixing the feign-core test-compilation error on master (JavaLoggerTest/LoggerRebufferTest still using the Request.create overload removed in f9159cf). Once that lands, a re-run of pr-build on this branch should go green — the failure never reaches feign-form, and all 31 feign-form tests pass locally on this branch.

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.

Wrong content-type for multipart requests on JSON parts (text/plain instead of application/json)

2 participants