Skip to content

Support binary request/response bodies (#57)#83

Merged
codefromthecrypt merged 1 commit intoOpenFeign:masterfrom
davidmc24:feature/binary-bodies
Oct 22, 2013
Merged

Support binary request/response bodies (#57)#83
codefromthecrypt merged 1 commit intoOpenFeign:masterfrom
davidmc24:feature/binary-bodies

Conversation

@davidmc24
Copy link
Copy Markdown
Contributor

Request/Response/RequestTemplate are now fundamentally based on a bodyData field, which is a byte[].
For Request/RequestTemplate, if a charset is provided, it can be treated as text.
If you are only using text bodies, usage of the library should feel almost exactly the same.
There were some non-backwards-compatible signature changes that require a
major version bump, however.

@cloudbees-pull-request-builder
Copy link
Copy Markdown

feign-pull-requests #113 SUCCESS
This pull request looks good

@codefromthecrypt codefromthecrypt mentioned this pull request Oct 15, 2013
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.

I'd prefer to javadoc this saying that when present, you can use new String(req.body(), req.charset()) vs adding isBodyText and having 2 accessors for body

@codefromthecrypt
Copy link
Copy Markdown
Contributor

looks close. I'd like to remove the suffixes of body*(). Basically have another look and see if the interface could be more narrow and still useful.

@davidmc24
Copy link
Copy Markdown
Contributor Author

Sounds good. I'll take another pass at this tomorrow.

Request/Response/RequestTemplate are now fundamentally based on a byte[] body field.
For Request/RequestTemplate, if a charset is provided, it can be treated as text.
For many users of the library, the change should barely be noticeable, as the methods that
were changed were mostly used internally.
There were some non-backwards-compatible signature changes that require a
major version bump, however.
@davidmc24
Copy link
Copy Markdown
Contributor Author

Updated based on feedback. The following changes have been made.

  • Rebased, and added minor tweaks to Jackson module (FYI: @mhurne)
  • Renamed Request.bodyData() to Request.body()
  • Removed Request.isBodyText() and Request.bodyText(), replacing them with javadoc on charset() and body()
  • Added RequestTemplate.charset()
  • Removed RequestTemplate.bodyData(byte[]); this was only used in the default encoder as a convenience to avoid passing a null Charset; now that it's the only reference in the API to a "body data" concept, it felt out of place
    • we can't provide RequestTemplate.body(byte[]), or it would product ambiguous signature errors with null values due to RequestTemplate.body(String)
  • Removed RequestTemplate.isBodyText() and RequestTemplate.bodyText(), replacing them with javadoc on charset() and body()
  • Renamed RequestTemplate.bodyData() to RequestTemplate.body()
  • Tweaked various files to account for the API changes

@cloudbees-pull-request-builder
Copy link
Copy Markdown

feign-pull-requests #116 SUCCESS
This pull request looks good

@codefromthecrypt
Copy link
Copy Markdown
Contributor

Sorry I half reviewed the update then got distracted. Lgtm and thanks for the new members of FeignTest

codefromthecrypt pushed a commit that referenced this pull request Oct 22, 2013
Support binary request/response bodies (#57)
@codefromthecrypt codefromthecrypt merged commit 1907903 into OpenFeign:master Oct 22, 2013
@davidmc24 davidmc24 deleted the feature/binary-bodies branch December 27, 2013 15:56
velo pushed a commit that referenced this pull request Oct 8, 2024
Support binary request/response bodies (#57)
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