don't do * imports

since you are catching.. why not test this :)

Do you mean test this on correctness before trying to call create method?

I meant that the comment "MIME type may not contain reserved characters, so
try to parse content type" has an if statement. this code shouldn't exist
unless it is tested, so I'd make a test that exercises that if statement

On Sun, Sep 4, 2016 at 1:43 AM, Andrey Stolyarchuk <notifications@github.com

wrote:

In httpclient/src/main/java/feign/httpclient/ApacheHttpClient.java
#453 (comment):

@@ -153,13 +153,19 @@ HttpUriRequest toHttpUriRequest(Request request, Request.Options options) throws
private ContentType getContentType(Request request) {
ContentType contentType = ContentType.DEFAULT_TEXT;
for (Map.Entry<String, Collection> entry : request.headers().entrySet())

• if (entry.getKey().equalsIgnoreCase("Content-Type")) {

•  Collection values = entry.getValue();
  

•  if (values != null && !values.isEmpty()) {
  

•    contentType = ContentType.create(entry.getValue().iterator().next(), request.charset());
  

•    break;
  

•  if (entry.getKey().equalsIgnoreCase("Content-Type")) {
  

•    Collection<String> values = entry.getValue();
  

•    if (values != null && !values.isEmpty()) {
  

•      String mimeType = values.iterator().next();
  

•      try {
  

•        contentType = ContentType.create(mimeType, request.charset());
  

•      } catch (IllegalArgumentException e) {
  

•        // MIME type may not contain reserved characters, so try to parse content type
  

•        contentType = ContentType.parse(mimeType);
  

Do you mean test this on correctness before trying to call create method?

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/OpenFeign/feign/pull/453/files/e66d0bbe188548b8dbe4fa596ac13668cbdae5a0#r77440388,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAD61wOmXfy3tVZPqGESo_Sq7tVWWFRMks5qmbHWgaJpZM4Jz7yl
.

Test added

so the thing I'm curious about is that ContentType.create doesn't declare that it throws IllegalArgument, rather unsupported. https://hc.apache.org/httpcomponents-core-ga/httpcore/apidocs/org/apache/http/entity/ContentType.html#create(java.lang.String,%20java.lang.String)

Are you sure this branch of code is in use?

Yes, I'm sure it throws IllegalArgumentException in my use case and it broke our code, so currently i need to copy-paste this feign class in own code base.

can you please make a test case that causes the
IllegalArgumentException? copy/pasting stuff works until the
implementation changes invalidating the assumption.

On Wed, Sep 7, 2016 at 1:22 PM, Andrey Stolyarchuk
notifications@github.com wrote:

In httpclient/src/main/java/feign/httpclient/ApacheHttpClient.java:

@@ -153,13 +153,19 @@ HttpUriRequest toHttpUriRequest(Request request,
Request.Options options) throws
private ContentType getContentType(Request request) {
ContentType contentType = ContentType.DEFAULT_TEXT;
for (Map.Entry<String, Collection> entry :
request.headers().entrySet())

• if (entry.getKey().equalsIgnoreCase("Content-Type")) {

•  Collection values = entry.getValue();
  

•  if (values != null && !values.isEmpty()) {
  

•    contentType =
  

  ContentType.create(entry.getValue().iterator().next(), request.charset());

•    break;
  

•  if (entry.getKey().equalsIgnoreCase("Content-Type")) {
  

•    Collection<String> values = entry.getValue();
  

•    if (values != null && !values.isEmpty()) {
  

•      String mimeType = values.iterator().next();
  

•      try {
  

•        contentType = ContentType.create(mimeType,
  

  request.charset());

•      } catch (IllegalArgumentException e) {
  

Yes, I'm sure it throws IllegalArgumentException in my use case and it broke
our code, so currently i need to copy-paste this feign class in own code
base.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.

Test was written first, see testContentTypeWithCharset.
"copy/pasting stuff works until the
implementation changes invalidating the assumption." - That exactly why I requesting a pull

this was a bad recommendation of me. we shouldn't conflate the response charset with the request one.

Please simplify the code to the following vs pretending we know what the response charset is.. You'll notice all tests pass!

      if (values != null && !values.isEmpty()) {
        contentType = ContentType.parse(entry.getValue().iterator().next());
        break;
      }

Adrian, I removed using of request context here. Looks like it could be applied