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

HttpResponse.mapHeaders doesn't work with FileService redirect #4056

Closed
tobias- opened this issue Jan 27, 2022 · 1 comment · Fixed by #4117
Closed

HttpResponse.mapHeaders doesn't work with FileService redirect #4056

tobias- opened this issue Jan 27, 2022 · 1 comment · Fixed by #4117
Labels

Comments

@tobias-
Copy link
Contributor

tobias- commented Jan 27, 2022

The problem is that when using FileService and using .mapHeaders on the resulting HttpResponse, the headers will NOT be mapped if the response is a directory redirect. .mapHeaders works as intended if a normal HttpResponse.ofRedirect() is returned.

I've added a small test to try to show the problem: https://github.com/tobias-/armeria/pull/4

My example adds the header "foo: bar" to the example. For "200 OK" request, the header is added. For 307 temporary redirect, it is not.

@ikhoon
Copy link
Contributor

ikhoon commented Jan 28, 2022

You cannot get HTTP headers of a failed HttpResponse with mapHeaders.
I recommend you use .recover to mutate headers in an HttpResponseException.
For example:

var sb = Server.builder();

sb.decorator((delegate, ctx, req) -> {
    final HttpResponse response = delegate.serve(ctx, req);
    return response.recover(ex -> {
        if (ex instanceof HttpResponseException) {
            final HttpResponse httpResponse = ((HttpResponseException) ex).httpResponse();
            return httpResponse.mapHeaders(
                    headers -> headers.withMutations(builder -> builder.add("foo", "bar")));
        }
        return HttpResponse.ofFailure(ex);
    });
});

ikhoon added a commit to ikhoon/armeria that referenced this issue Feb 25, 2022
Motivation:

An `HttpResponseException` has an `HttpResponse`. So the default
`ServerErrorHandler` extracts the `HttpResponse` from the exception.
As the response is wrapped by `HttpResponseException`,
decorators can not access the `HttpObject`s of the response in the
exception.

Users may find it hard to mutate the thrown `HttpResponse` because
recovery should be done first. So it would be better not to use
`HttpResponseException` or recover it before passing a response to
decorators.

Modifications:

- Add `HttpFile.ofRedirect(location)` to indicate a file in a different
  location.
- Recover an `Http{Response,Status}Exception thrown by
  `HealthCheckUpdateHandler`
- Update Javadoc in detail for `Http{Response,Status}Exception`
- Correctly propagate the cause of `Http{Response,Status}Exception` in
  `THttpService`

Result:

- You can now mutate a redirect response from `FileService` using
  `mapHeaders`
- You no longer see an `HttpResponseException` or
  an `HttpStatusException` from built-in services.
- Fixes line#4056
trustin pushed a commit that referenced this issue Mar 22, 2022
Motivation:

An `HttpResponseException` has an `HttpResponse`. The thrown response
is extracted by the default `ServerErrorHandler` and converted to 
a normal `HttpResponse`. There are downsides to `HttpResponseException`.
As the response is wrapped by `HttpResponseException`,
decorators can not access the `HttpObject`s of the response in the
exception using `mapXXX`.

Users may find it hard to mutate the thrown `HttpResponse` because 
recovery should be done first. Therefore, it would be better not to use 
`HttpResponseException` internally or recover it before passing
the response to decorators so that users use `HttpResponse.mapXXX` to
transform a returned response in decorators.

Modifications:

- Add `HttpFile.ofRedirect(location)` to indicate a file in a different
  location.
- Recover an `Http{Response,Status}Exception` thrown by
  `HealthCheckUpdateHandler`
- Update Javadoc in detail for `Http{Response,Status}Exception`
- Correctly propagate the cause of `Http{Response,Status}Exception` in
  `THttpService`

Result:

- You can now mutate a redirect response from `FileService` using
  `mapHeaders`
- You no longer see an `HttpResponseException` or
  an `HttpStatusException` from built-in services.
- Fixes #4056
heowc pushed a commit to heowc/armeria that referenced this issue Mar 25, 2022
Motivation:

An `HttpResponseException` has an `HttpResponse`. The thrown response
is extracted by the default `ServerErrorHandler` and converted to 
a normal `HttpResponse`. There are downsides to `HttpResponseException`.
As the response is wrapped by `HttpResponseException`,
decorators can not access the `HttpObject`s of the response in the
exception using `mapXXX`.

Users may find it hard to mutate the thrown `HttpResponse` because 
recovery should be done first. Therefore, it would be better not to use 
`HttpResponseException` internally or recover it before passing
the response to decorators so that users use `HttpResponse.mapXXX` to
transform a returned response in decorators.

Modifications:

- Add `HttpFile.ofRedirect(location)` to indicate a file in a different
  location.
- Recover an `Http{Response,Status}Exception` thrown by
  `HealthCheckUpdateHandler`
- Update Javadoc in detail for `Http{Response,Status}Exception`
- Correctly propagate the cause of `Http{Response,Status}Exception` in
  `THttpService`

Result:

- You can now mutate a redirect response from `FileService` using
  `mapHeaders`
- You no longer see an `HttpResponseException` or
  an `HttpStatusException` from built-in services.
- Fixes line#4056
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants