puppetlabs / clj-http-client

HTTP client library wrapping Apache HttpAsyncClient
Apache License 2.0
15 stars 30 forks source link

Charset is overwritten with "; charset=UTF-8" when manually set. #55

Open eriktjacobsen opened 8 years ago

eriktjacobsen commented 8 years ago

For the Clojure Sync Client:

I'm trying to pass a header:

"content-type": "application/json; version=5"

Which is a requirement of an API I am using... this gets changed, silently, into:

"content-type": "application/json; charset=UTF-8"

Causing the call to fail. There doesn't seem to be an option for this behavior or easy workaround. At the very least, perhaps this should be indicated in the returned :opts for the response, as library currently indicates the header was sent correctly.

I'm currently looking through the patch for TK-145 which seems to touch similar code / issues, however in the mean time would appreciate any guidance or patch.

Thanks!

cprice404 commented 8 years ago

hmm, that sounds bad. Have you tried explicitly adding the charset in to your header, in addition to your version var?

eriktjacobsen commented 8 years ago

Thanks for the quick reply!

Adding a charset does stop the library from removing the extra content:

"Content-Type" "application/json;version=5;charset=UTF-8"

This passes through to the server correctly. Unfortunately, I'm working with a somewhat legacy API, and it is restrictive in what it accepts, and it doesn't like the appended ";charset=UTF-8".

Still, a way to turn off the behavior and pass the headers as manually set would be helpful.

cprice404 commented 8 years ago

Yeah, in that case, adding something to :opts that would disable that behavior would be the way to go; it's been a while since I looked at that part of the code, I can't recall if it's something we're doing manually (in which case it should be easy to wrap in a conditional), or whether the underlying Apache library is doing it (in which case it might be harder).

camlow325 commented 8 years ago

Seems like the library should at least preserve the portions of the Content-Type that don't relate to the charset. Like if the original Content-Type were set to "application/json; version=5; foo=bar", it may not be unreasonable to default in a charset but preserve the rest of the header content - e.g., "application/json; charset=UTF-8; version=5; foo=bar".

If it were additionally problematic to even have a charset at all, it might be even better to have a request option to avoid having it added to the header. We went back and forth a couple of times on that code. I think we decided to have a charset explicitly added to the header so the charset for the receiving server would be obvious.

In any event, I definitely agree that this code shouldn't be wiping out portions of the Content-Type as part of this process.

KevinCorcoran commented 8 years ago

Yeah, I think @camlow325 is on to the right solution. I don't think that we need to add something to :opts to disable this behavior - at least, not to fix this particular bug (no comment on the merit of that idea for Other Reasons).

The extent of this bug seems to be that we wipe out the rest of the content-type parameters (as defined here) when we explicitly set the charset if not defined. Seems like it'd be a pretty easy and isolated bug to fix.

eriktjacobsen commented 8 years ago

Thank you @cprice404, @camlow325, and @KevinCorcoran. I do think having this as an option would also be helpful, as some people have to deal with very restrictive API's that do exact string matching.

I think that in general, if you explicitly pass in a value, you should at least have the option to use that value without the library appending things to it.

Thanks for the library everyone, it's been very helpful for doing some client-side SSL cert work.

KevinCorcoran commented 8 years ago

Actually, I realize my previous comment is slightly off-the-mark.

I think there are two issues here, the first being that we should not clobber existing parameters when adding Content-type. That could be fixed in isolation, but for your use-case @eriktjacobsen, you'd still end up with application/json;version=5;charset=UTF-8. So I now understand and agree with the reasoning that it should be possible to disable this behavior of adding a charset via opts.