-
-
Notifications
You must be signed in to change notification settings - Fork 144
feat(server): implement automatic error map merging for middleware #924
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| import type { Meta } from '@orpc/contract' | ||
| import type { ErrorMap, Meta } from '@orpc/contract' | ||
| import type { IntersectPick } from '@orpc/shared' | ||
| import type { Context, MergedCurrentContext, MergedInitialContext } from './context' | ||
| import type { ORPCErrorConstructorMap } from './error' | ||
|
|
@@ -12,6 +12,11 @@ export interface DecoratedMiddleware< | |
| TErrorConstructorMap extends ORPCErrorConstructorMap<any>, | ||
| TMeta extends Meta, | ||
| > extends Middleware<TInContext, TOutContext, TInput, TOutput, TErrorConstructorMap, TMeta> { | ||
| /** | ||
| * Error map associated with this middleware (if any) | ||
| * @internal | ||
| */ | ||
| errorMap?: ErrorMap | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Your current implementation seem not "typesafe" at all, it only cover at runtime level while ignore the type level. To fully support this, we require procedure inherit middleware's errors on both runtime and type levels when And you should change the type of
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For typesafe, do you mean we should be able to infer the errors type in handler? I do understand my implementation is not fulfilling it. But I want to fix the "defined for error thrown from middleware is false" thing first without affecting current behavior. Screen.Recording.2025-08-24.at.17.mp4If we want it work as a whole, that is, both "defined=true" and "typesafe", I agree change
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You must make sure runtime For reference here my implementation in the past: https://github.com/unnoq/orpc/pull/224 - but it faces some issue with contract-first |
||
| /** | ||
| * Change the expected input type by providing a map function. | ||
| */ | ||
|
|
@@ -78,6 +83,8 @@ export interface DecoratedMiddleware< | |
| > | ||
| } | ||
|
|
||
| export type AnyDecoratedMiddleware = DecoratedMiddleware<any, any, any, any, any, any> | ||
|
|
||
| export function decorateMiddleware< | ||
| TInContext extends Context, | ||
| TOutContext extends Context, | ||
|
|
@@ -86,23 +93,39 @@ export function decorateMiddleware< | |
| TErrorConstructorMap extends ORPCErrorConstructorMap<any>, | ||
| TMeta extends Meta, | ||
| >( | ||
| middleware: Middleware<TInContext, TOutContext, TInput, TOutput, TErrorConstructorMap, TMeta>, | ||
| middleware: Middleware<TInContext, TOutContext, TInput, TOutput, TErrorConstructorMap, TMeta> | ||
| | DecoratedMiddleware<TInContext, TOutContext, TInput, TOutput, TErrorConstructorMap, TMeta>, | ||
| errorMap?: ErrorMap, | ||
| ): DecoratedMiddleware<TInContext, TOutContext, TInput, TOutput, TErrorConstructorMap, TMeta> { | ||
| const decorated = ((...args) => middleware(...args)) as DecoratedMiddleware<TInContext, TOutContext, TInput, TOutput, TErrorConstructorMap, TMeta> | ||
|
|
||
| // Attach error map if provided | ||
| if (errorMap) { | ||
| decorated.errorMap = errorMap | ||
| } | ||
|
Comment on lines
+102
to
+105
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The // Attach error map if provided, preserving existing one if present
if (errorMap) {
decorated.errorMap = errorMap
} else if ('errorMap' in middleware && (middleware as AnyDecoratedMiddleware).errorMap) {
decorated.errorMap = (middleware as AnyDecoratedMiddleware).errorMap
}
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about this? I want it typed. export function decorateMiddleware<
TInContext extends Context,
TOutContext extends Context,
TInput,
TOutput,
TErrorConstructorMap extends ORPCErrorConstructorMap<any>,
TMeta extends Meta,
>(
middleware: Middleware<TInContext, TOutContext, TInput, TOutput, TErrorConstructorMap, TMeta>
| DecoratedMiddleware<TInContext, TOutContext, TInput, TOutput, TErrorConstructorMap, TMeta>,
errorMap?: ErrorMap,
): DecoratedMiddleware<TInContext, TOutContext, TInput, TOutput, TErrorConstructorMap, TMeta> {
const decorated = ((...args) => middleware(...args)) as DecoratedMiddleware<TInContext, TOutContext, TInput, TOutput, TErrorConstructorMap, TMeta>
// Attach error map if provided
if (errorMap) {
decorated.errorMap = errorMap
}
if ('errorMap' in middleware) {
decorated.errorMap = middleware.errorMap
} |
||
| if ('errorMap' in middleware) { | ||
| decorated.errorMap = middleware.errorMap | ||
| } | ||
|
|
||
| decorated.mapInput = (mapInput) => { | ||
| const mapped = decorateMiddleware( | ||
| (options, input, ...rest) => middleware(options as any, mapInput(input as any), ...rest as [any]), | ||
| decorated.errorMap, // Preserve error map | ||
| ) | ||
|
|
||
| return mapped as any | ||
| } | ||
|
|
||
| decorated.concat = (concatMiddleware: AnyMiddleware, mapInput?: MapInputMiddleware<any, any>) => { | ||
| decorated.concat = (concatMiddleware: AnyMiddleware | AnyDecoratedMiddleware, mapInput?: MapInputMiddleware<any, any>) => { | ||
| const mapped = mapInput | ||
| ? decorateMiddleware(concatMiddleware).mapInput(mapInput) | ||
| : concatMiddleware | ||
|
|
||
| const combinedErrorMap = { | ||
| ...decorated.errorMap, | ||
| ...('errorMap' in concatMiddleware ? concatMiddleware.errorMap : undefined), | ||
| } | ||
|
|
||
| const concatted = decorateMiddleware((options, input, output, ...rest) => { | ||
| const merged = middleware({ | ||
| ...options, | ||
|
|
@@ -114,7 +137,7 @@ export function decorateMiddleware< | |
| } as any, input as any, output as any, ...rest) | ||
|
|
||
| return merged | ||
| }) | ||
| }, combinedErrorMap) | ||
|
|
||
| return concatted as any | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,6 +12,7 @@ import type { IntersectPick, MaybeOptionalOptions } from '@orpc/shared' | |
| import type { Context, MergedCurrentContext, MergedInitialContext } from './context' | ||
| import type { ORPCErrorConstructorMap } from './error' | ||
| import type { AnyMiddleware, MapInputMiddleware, Middleware } from './middleware' | ||
| import type { AnyDecoratedMiddleware } from './middleware-decorated' | ||
| import type { ProcedureActionableClient } from './procedure-action' | ||
| import type { CreateProcedureClientOptions, ProcedureClient } from './procedure-client' | ||
| import { mergeErrorMap, mergeMeta, mergeRoute } from '@orpc/contract' | ||
|
|
@@ -136,14 +137,22 @@ export class DecoratedProcedure< | |
| TMeta | ||
| > | ||
|
|
||
| use(middleware: AnyMiddleware, mapInput?: MapInputMiddleware<any, any>): DecoratedProcedure<any, any, any, any, any, any> { | ||
| use(middleware: AnyMiddleware | AnyDecoratedMiddleware, mapInput?: MapInputMiddleware<any, any>): DecoratedProcedure<any, any, any, any, any, any> { | ||
| const mapped = mapInput | ||
| ? decorateMiddleware(middleware).mapInput(mapInput) | ||
| : middleware | ||
|
|
||
| if (!('errorMap' in middleware) || !middleware.errorMap) { | ||
| return new DecoratedProcedure({ | ||
| ...this['~orpc'], | ||
| middlewares: addMiddleware(this['~orpc'].middlewares, mapped), | ||
| }) | ||
| } | ||
|
|
||
| return new DecoratedProcedure({ | ||
| ...this['~orpc'], | ||
| middlewares: addMiddleware(this['~orpc'].middlewares, mapped), | ||
| errorMap: mergeErrorMap(this['~orpc'].errorMap, middleware.errorMap), | ||
| }) | ||
|
Comment on lines
+145
to
156
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar to the const errorMap = ('errorMap' in middleware && middleware.errorMap)
? mergeErrorMap(this['~orpc'].errorMap, middleware.errorMap)
: this['~orpc'].errorMap
return new DecoratedProcedure({
...this['~orpc'],
middlewares: addMiddleware(this['~orpc'].middlewares, mapped),
errorMap,
}) |
||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,80 @@ | ||
| import { createSafeClient } from '@orpc/client' | ||
| import { createRouterClient, os } from '../src' | ||
|
|
||
| describe('error middleware patterns', () => { | ||
| it('should have defined=true when using original supported pattern (same base for middleware and procedure)', async () => { | ||
| // ✅ CORRECT: Define errors on base, use same base for both middleware and procedure | ||
| const base = os.errors({ | ||
| UNAUTHORIZED: {}, | ||
| }) | ||
|
|
||
| const middleware = base.middleware(async ({ next, context, errors }) => { | ||
| throw errors.UNAUTHORIZED() | ||
| }) | ||
|
|
||
| const router = base.use(middleware).handler(async () => {}) | ||
|
|
||
| const client = createSafeClient(createRouterClient(router)) | ||
| const [error,, isDefined] = await client() | ||
|
|
||
| // Should have defined=true because error was thrown from defined error map | ||
| expect((error as any).defined).toBe(true) | ||
| expect(isDefined).toBe(true) | ||
| expect((error as any).code).toBe('UNAUTHORIZED') | ||
| }) | ||
|
|
||
| it('should have defined=true with automatic error map merging (new behavior)', async () => { | ||
| // ✅ NEW BEHAVIOR: Middleware error maps are automatically merged | ||
| const middleware = os.errors({ | ||
| UNAUTHORIZED: {}, | ||
| }).middleware(async ({ next, context, errors }) => { | ||
| throw errors.UNAUTHORIZED() | ||
| }) | ||
|
|
||
| // Using base os (no errors) but middleware error map should be automatically merged | ||
| const router = os.use(middleware).handler(async () => {}) | ||
|
|
||
| const client = createSafeClient(createRouterClient(router)) | ||
| const [error,, isDefined] = await client() | ||
|
|
||
| // Should now have defined=true because middleware error map gets merged automatically | ||
| expect((error as any).defined).toBe(true) | ||
| expect(isDefined).toBe(true) | ||
| expect((error as any).code).toBe('UNAUTHORIZED') | ||
| // Verify the error map was merged | ||
| expect(router['~orpc'].errorMap).toHaveProperty('UNAUTHORIZED') | ||
| }) | ||
|
|
||
| it('should merge errors from different sources correctly', async () => { | ||
| const middleware = os.errors({ | ||
| UNAUTHORIZED: {}, | ||
| }).middleware(async ({ next, context, errors }) => { | ||
| // Don't throw, just continue to test error map merging | ||
| return next({ context }) | ||
| }) | ||
| const router = os | ||
| .use(middleware) | ||
| .errors({ NOT_FOUND: {} }) | ||
| .handler(async ({ errors }) => { | ||
| // Should have access to both UNAUTHORIZED and NOT_FOUND | ||
| expect('UNAUTHORIZED' in errors).toBe(true) | ||
| expect('NOT_FOUND' in errors).toBe(true) | ||
|
|
||
| // @ts-expect-error TODO: Currently, errors defined in middleware is not inferred into the procedure | ||
| const unauthorizedError = errors.UNAUTHORIZED() | ||
| const notFoundError = errors.NOT_FOUND() | ||
|
|
||
| expect(unauthorizedError.defined).toBe(true) | ||
| expect(notFoundError.defined).toBe(true) | ||
|
|
||
| throw notFoundError | ||
| }) | ||
|
|
||
| const client = createSafeClient(createRouterClient(router)) | ||
| const [error,, isDefined] = await client() | ||
|
|
||
| expect((error as any).defined).toBe(true) | ||
| expect(isDefined).toBe(true) | ||
| expect((error as any).code).toBe('NOT_FOUND') | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic inside the
usemethod has some code duplication between theifand theelsepath. You can refactor it to be more concise and maintainable by creating the new builder definition object and conditionally adding theerrorMap.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's about whether the
"errorMap"exist or not. The given solution is conciser but would make the object{errorMap: undefined}instead of omit the"errorMap"property. I don't know if it would affect anytime so I better make it safer.