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

Consider not wrapping errors thrown in an interceptor #1369

Closed
DustinJSilk opened this issue Dec 15, 2024 · 2 comments
Closed

Consider not wrapping errors thrown in an interceptor #1369

DustinJSilk opened this issue Dec 15, 2024 · 2 comments
Labels
enhancement New feature or request

Comments

@DustinJSilk
Copy link

Describe the bug

When I throw an error inside an interceptor, I expect to receive that error when I wrap my RPC method in a try/catch. Instead, I receive an [unknown] type. If I wrap my error in a ConnectError.from, I receive an ConnectError: [unknown].

My usecase:

The framework I'm using is waiting for a specific error instance, a ServerError, to correctly set the HTTP status code. It would be simple to have an interceptor catch ConnectErrors and throw ServerErrors. But since the ServerError is not surfaced on the other side, it requires every calling function to be wrapped in a try/catch and converted to the new error type.

This is my interceptor code:

// Simply used to map error codes
const ERRORS = {
  [Code.Canceled]: 499,
  // ...
}

// This would be inside the createConnectTransport interceptors list
(next) => async (req) => {
    try {
      return await next(req)
    } catch (err) {
      if (err instanceof ConnectError) {
        const httpStatus = ERRORS[err.code]
        if (httpStatus) {
          throw new ServerError(httpStatus, err)
        }
      }
      throw new ServerError(500, err)
    }
  }

To Reproduce

If you encountered an error message, please copy and paste it verbatim.
If the bug is specific to an RPC or payload, please provide a reduced
example.

Environment (please complete the following information):

  • @connectrpc/connect-web version: N/A
  • @connectrpc/connect-node version: 1.4.0
  • Frontend framework and version: [email protected]
  • Node.js version: 22.11.0
  • Browser and version: Chrome latest but not applicable

If your problem is specific to bundling, please also provide the following information:

  • Bundler and version: [email protected]
  • Bundler plugins and version:
  • Bundler configuration file:

Additional context
Add any other context about the problem here.

@DustinJSilk DustinJSilk added the bug Something isn't working label Dec 15, 2024
@timostamm
Copy link
Member

Hey Dustin. The API was designed to support clients with strong error types, and errors are wrapped to ConnectErrors eagerly.

Maybe it makes sense to revise this design. It looks like the change is simple (reject promise with reason instead of e in run-call.ts). But there may be side effects, and it's technically a breaking change. So this needs to be considered carefully, and it's not something we can change quickly.

There's a simple alternative though - instead of adding and interceptor, you can wrap the client:

import type {DescService} from "@bufbuild/protobuf";
import {type Client, createClient, makeAnyClient, type Transport} from "@connectrpc/connect";

export function createServerErrorClient<T extends DescService>(
  service: T,
  transport: Transport,
) {
  type UnaryFn = (...args: unknown[]) => Promise<unknown>;
  const client = createClient(service, transport) as unknown as Record<string, UnaryFn>;
  return makeAnyClient(service, (method) => {
    const fn = client[method.localName];
    if (method.methodKind !== "unary") {
      return fn;
    }
    return (...args: unknown[]) => fn(...args).catch(e => {
      throw new ServerError("server error "+ String(e));
    });
  }) as Client<T>;
}

class ServerError extends Error {}

The returned functions are the functions you call on the client. The thrown ServerError will not be caught and not wrapped by Connect.

@timostamm timostamm added enhancement New feature or request and removed bug Something isn't working labels Dec 19, 2024
@timostamm timostamm changed the title Error types are lost when thrown inside an interceptor Consider not wrapping errors thrown in an interceptor Dec 19, 2024
@chrispine
Copy link
Contributor

I'm going to go ahead and close this issue, but feel free to re-open if needed. Thanks!

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

3 participants