Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature Request: add support for custom request Content-Encoding (such as gzip) #52

Closed
davidmc24 opened this issue Aug 22, 2013 · 8 comments

Comments

@davidmc24
Copy link
Contributor

I have an API that I'd like to use Feign to connect to that requires that the POST request body use gzip content encoding.

This feels to me like something that should be possible using a RequestInterceptor, but the current structure of RequestTemplate (storing body as String, and converting to bytes in the client) doesn't really seem like it supports it. I know I could add support for this using a custom Client implementation. Is there a better approach?

@codefromthecrypt
Copy link
Contributor

ack. Yeah I saw this coming :)

ps do you mind reviewing #53? I want to ensure we have time to address features like this by trimming edge-case code.

@codefromthecrypt
Copy link
Contributor

I suspect this would need to be a part of the Encoder or possibly a revision to RequestInterceptor

you only need encoding?

@davidmc24
Copy link
Contributor Author

I've reviewed the ticket you mentioned.

For my current use case, I only need the ability to gzip-encode the request body; no need to do any special decoding.

If RequestTemplate were structured to allow for treating the body as a byte[] rather than a String, I could accomplish this via a RequestInterceptor that added a Content-Encoding header and replaced the pre-existing body with the gzip-ed version.

The API I'm integrating with takes a POST of either an XML or JSON document as the body, but requires that the body is gzip-encoded. I'm currently approaching it using JSON, encoded via the GSON module. The Encoder interface doesn't currently allow for meeting this need, as Encoders currently always need to return a String, and this encoding would return a byte[]. If it returned a byte[](or similar representation of binary data), then this approach could work. I'd likely extend the GSON encoder to also GZIP compress.

After having thought this through, I think that it may be better to first work on a new feature of "support binary bodies". This seems like a sensible, general-use feature by itself (for example, file/image uploads to a REST endpoint), and seems likely to provide the majority of the structural changes needed to make this particular use case workable.

@codefromthecrypt
Copy link
Contributor

I was thinking similarly. That said, clients like ok http are handling
gzip encoding automatically. Should we try with a gzip client decorator
for now? If you want I could paste an example.

@davidmc24
Copy link
Contributor Author

A decorator sounds good for now. I'll put one together Monday. No need for an example. Thanks.

@davidmc24
Copy link
Contributor Author

On second thought, given the way that the Client and Request are currently structured, I don't think a decorator is workable. For that to work, I'd need to take the original request, perform the gzip encoding modifications to it, and then pass it to the delegate's execute method. That isn't possible for exactly the same reason as it isn't possible to handle this as a RequestInterceptor: the Request is treating the body as a String, rather than some binary form.

In the following gist, I've included an example client that does what I need it to for now. It contains mostly the same code as the default client, with the following additions:

  • Detects whether GZIP content encoding should be used by the presence of a request header
  • If GZIP content encoding should be used, ignore any specified content length header as it will be wrong.
  • If GZIP content encoding should be used, wrap the connection output stream with a GZIPOutputStream

https://gist.github.com/davidmc24/6345889

@codefromthecrypt
Copy link
Contributor

If you don't mind, move this code into the default client in a Pull Request. As there's no binary body supported as yet, there's no chance this approach would conflict with expectations.

@davidmc24
Copy link
Contributor Author

Sure. I'll work on making this into a pull request.

codefromthecrypt pushed a commit that referenced this issue Aug 29, 2013
default client: add support for gzip-encoded request bodies (#52)
codefromthecrypt pushed a commit that referenced this issue Aug 29, 2013
Enhances the default client to GZIP-encode request bodies when the appropriate
content-encoding header is set in the interface's method definition.

#52
velo pushed a commit to velo/feign that referenced this issue Apr 20, 2019
* Skip filename in Content-Disposition if it's null.

When making an upload from a byte[] there isn't a filename and the Content-Disposition header ends up with a value ending in filename="null" which can result in consuming services thinking null is should be used as a filename. Instead when it's a byte[] upload we shouldn't output the filename.

The filename part of the Content-Disposition header is optional(see RFC-6266).

Fixes OpenFeign#52

* Fixed indentation.
velo pushed a commit that referenced this issue Oct 7, 2024
* Skip filename in Content-Disposition if it's null.

When making an upload from a byte[] there isn't a filename and the Content-Disposition header ends up with a value ending in filename="null" which can result in consuming services thinking null is should be used as a filename. Instead when it's a byte[] upload we shouldn't output the filename.

The filename part of the Content-Disposition header is optional(see RFC-6266).

Fixes #52

* Fixed indentation.
velo pushed a commit that referenced this issue Oct 8, 2024
Enhances the default client to GZIP-encode request bodies when the appropriate
content-encoding header is set in the interface's method definition.

#52
velo pushed a commit that referenced this issue Oct 8, 2024
default client: add support for gzip-encoded request bodies (#52)
velo pushed a commit that referenced this issue Oct 8, 2024
* Skip filename in Content-Disposition if it's null.

When making an upload from a byte[] there isn't a filename and the Content-Disposition header ends up with a value ending in filename="null" which can result in consuming services thinking null is should be used as a filename. Instead when it's a byte[] upload we shouldn't output the filename.

The filename part of the Content-Disposition header is optional(see RFC-6266).

Fixes #52

* Fixed indentation.
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

No branches or pull requests

2 participants