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

Adding response headers to error responses in interceptor #1065

Closed
anuraaga opened this issue Apr 18, 2024 · 1 comment
Closed

Adding response headers to error responses in interceptor #1065

anuraaga opened this issue Apr 18, 2024 · 1 comment
Labels
enhancement New feature or request

Comments

@anuraaga
Copy link

anuraaga commented Apr 18, 2024

Is your feature request related to a problem? Please describe.

I am using an interceptor and want to be able to add a response header to all requests (in this case, a trace ID).

Currently, AnyResponse is only available if no exception is thrown as the return value of next. It can be used to set a response header with res.header(..). If next throws an exception, there will not be any response object to use though and there doesn't seem to be a way to set a header then.

Describe the solution you'd like

Probably the least disruptive is to pass the underlying response object to contextValues in the adapter. For example here

https://github.com/connectrpc/connect-es/blob/main/packages/connect-node/src/connect-node-adapter.ts#L100

only req is passed but res is also available and could be passed as well. AFAIK this should be backwards compatible as functions that only accept the single req will still execute ignoring the second argument.

This provides access to the response for advanced usage in a somewhat tedious way, but thanks to that the Interceptor API continues to use the "connect lifecycle", where a response is returned, not filled.

Describe alternatives you've considered
If you've proposed a solution, are there any alternatives? Why are they worse
than your preferred approach?

Interceptors could be passed something along with AnyRequest that allows manipulating the response, ResponseManipulator or similar, as a second argument. It could potentially collide with the response returned by next.

Additional context
I am using NextJS. My interceptor is an OTel one, similar to #523 (comment). If I were to try to manipulate the response header at a NextJS layer such as middleware, there would be no way to access the span to be able to compute the trace ID as the span will have already been completed when returning from the interceptor.

@anuraaga anuraaga added the enhancement New feature or request label Apr 18, 2024
@srikrsna-buf
Copy link
Member

Hey! You can catch the connect error and set the headers on that:

(next) => {
  return async (req) => {
    try {
      const res = await next(req);
      res.header.append("key", "value");
      return res;
    } catch (err) {
      const cErr = ConnectError.from(err)
      cErr.metadata.append("key", "value");
      throw cErr;
    }
  }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants