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

Give more options to DecodingClient #3348

Closed
ikhoon opened this issue Feb 15, 2021 · 0 comments · Fixed by #3372
Closed

Give more options to DecodingClient #3348

ikhoon opened this issue Feb 15, 2021 · 0 comments · Fixed by #3372
Milestone

Comments

@ikhoon
Copy link
Contributor

ikhoon commented Feb 15, 2021

  • A DecodingClient does nothing if an Accept-Encoding header is provided. The DecodingClient only works if an Accept-Encoding header is absent. It will send all possible accept encodings.

    if (req.headers().contains(HttpHeaderNames.ACCEPT_ENCODING)) {
    // Client specified encoding, so we don't do anything automatically.
    return unwrap().execute(ctx, req);
    }

    Sometimes users may want to dynamically choose accept-encodings using an Accept-Encoding header.

  • When unsupported Content-Encoding is received, a DecodingClient silently skip decoding. It would good that users decide whether to ignore it or returns failed response.

    // If the server returned an encoding we don't support (shouldn't happen since we set
    // Accept-Encoding), decoding will be skipped which is ok.
    if (decoderFactory != null) {
    responseDecoder = decoderFactory.newDecoder(alloc);
    }

We might give options by introducing DecodingClientBuilder. For example:

DecodingClient.buider()
              .respectAcceptEncoding(true) // Needs better names
              .ignoreUnsupportedEncoding(false)
ikhoon added a commit to ikhoon/armeria that referenced this issue Mar 6, 2021
Motivation:

Currently, `DecodingClient` does nothing if an accept-encoding is specified.
https://github.com/line/armeria/blob/6da4e23b198cb769bc97dfe23f4e2eaed73254bf/core/src/main/java/com/linecorp/armeria/client/encoding/DecodingClient.java#L87-L90
However, some users might want to specify accept-encoding header and decode the response content using `DecodingClient`.

Modifications:

- Add `DecodingClientBuilder` in order to fluently build a `DecodingClient` with various options.
- Add `autoFillAcceptEncoding(boolean)` to `DecodingClientBuilder`
  - If this option is disabled, `DecodingClient` will respect user-defined accept-encoding header
- Add `strictContentEncoding(boolean)` to `DecodingClientBuilder`
  - If this option is enabled and unsupported encoding is received from an abnormal server,
    a response is failed with `UnsupportedEncodingException`

Result:

- You can now fluently build `DecodingClient` using `DecodingClientBuilder`.
  ```java
  DecodingClient.builder()
                .autoFillAcceptEncoding(false)
                .strictContentEncoding(true)
                .newDecorator();
  ```
- Fixes line#3348
ikhoon added a commit to ikhoon/armeria that referenced this issue Mar 6, 2021
Motivation:

Currently, `DecodingClient` does nothing if an accept-encoding is specified.
https://github.com/line/armeria/blob/6da4e23b198cb769bc97dfe23f4e2eaed73254bf/core/src/main/java/com/linecorp/armeria/client/encoding/DecodingClient.java#L87-L90
However, some users might want to specify accept-encoding header and decode the response content using `DecodingClient`.

Modifications:

- Add `DecodingClientBuilder` in order to fluently build a `DecodingClient` with various options.
- Add `autoFillAcceptEncoding(boolean)` to `DecodingClientBuilder`
  - If this option is disabled, `DecodingClient` will respect user-defined accept-encoding header
- Add `strictContentEncoding(boolean)` to `DecodingClientBuilder`
  - If this option is enabled and unsupported encoding is received from an abnormal server,
    a response is failed with `UnsupportedEncodingException`

Result:

- You can now fluently build `DecodingClient` using `DecodingClientBuilder`.
  ```java
  DecodingClient.builder()
                .autoFillAcceptEncoding(false)
                .strictContentEncoding(true)
                .newDecorator();
  ```
- Fixes line#3348
trustin pushed a commit that referenced this issue Mar 24, 2021
Motivation:

Currently, `DecodingClient` does nothing if an accept-encoding is specified.
https://github.com/line/armeria/blob/6da4e23b198cb769bc97dfe23f4e2eaed73254bf/core/src/main/java/com/linecorp/armeria/client/encoding/DecodingClient.java#L87-L90
However, some users might want to specify accept-encoding header and decode the response content using `DecodingClient`.

Modifications:

- Add `DecodingClientBuilder` in order to fluently build a `DecodingClient` with various options.
- Add `autoFillAcceptEncoding(boolean)` to `DecodingClientBuilder`
  - If this option is disabled, `DecodingClient` will respect user-defined accept-encoding header
- Add `strictContentEncoding(boolean)` to `DecodingClientBuilder`
  - If this option is enabled and unsupported encoding is received from an abnormal server,
    a response is failed with `UnsupportedEncodingException`

Result:

- You can now fluently build `DecodingClient` using `DecodingClientBuilder`.
  ```java
  DecodingClient.builder()
                .autoFillAcceptEncoding(false)
                .strictContentEncoding(true)
                .newDecorator();
  ```
- Fixes #3348
@trustin trustin added this to the 1.6.0 milestone Mar 24, 2021
ikhoon added a commit to ikhoon/sttp that referenced this issue May 7, 2021
Motivation:

line/armeria#3348 has been fixed.
`HttpDecordingClient` which could be replaced with Armeria's built-in `DecodingClient`.

Modifications:

- Remove `HttpDecodingClient` in favor of Armeria's `DecodingClient`
- Use `StreamMessage.of(Path)` instead of additional parameters

Result:

Clean up workarounds.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants