Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions packages/event-handler/src/appsync-events/Router.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
import type { GenericLogger } from '@aws-lambda-powertools/commons/types';
import { isRecord } from '@aws-lambda-powertools/commons/typeutils';
import { getStringFromEnv, isDevMode } from '@aws-lambda-powertools/commons/utils/env';
import {
getStringFromEnv,
isDevMode,
} from '@aws-lambda-powertools/commons/utils/env';
import type {
OnPublishHandler,
OnSubscribeHandler,
Expand Down Expand Up @@ -31,7 +34,7 @@ class Router {
* Whether the router is running in development mode.
*/
protected readonly isDev: boolean = false;

public constructor(options?: RouterOptions) {
const alcLogLevel = getStringFromEnv({
key: 'AWS_LAMBDA_LOG_LEVEL',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -152,11 +152,7 @@ class AppSyncGraphQLResolver extends Router {
* @param context - The AWS Lambda context object.
* @param options - Optional parameters for the resolver, such as the scope of the handler.
*/
public async resolve(
event: unknown,
context: Context,
options?: ResolveOptions
): Promise<unknown> {
public resolve(event: unknown, context: Context, options?: ResolveOptions) {
if (Array.isArray(event)) {
if (event.some((e) => !isAppSyncGraphQLEvent(e))) {
this.logger.warn(
Expand All @@ -178,7 +174,7 @@ class AppSyncGraphQLResolver extends Router {
}

return this.#withErrorHandling(
() => this.#executeSingleResolver(event, context, options),
async () => await this.#executeSingleResolver(event, context, options),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just pass the unresolved promise here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, because as this is with this change, it slightly changes the behaviour from what it was.

event,
options
);
Expand All @@ -197,7 +193,7 @@ class AppSyncGraphQLResolver extends Router {
fn: () => Promise<unknown>,
event: AppSyncResolverEvent<Record<string, unknown>>,
options?: ResolveOptions
): Promise<unknown> {
) {
try {
return await fn();
} catch (error) {
Expand Down Expand Up @@ -377,11 +373,11 @@ class AppSyncGraphQLResolver extends Router {
* @param options - Optional parameters for the resolver, such as the scope of the handler.
* @throws {ResolverNotFoundException} If no resolver is registered for the given field and type.
*/
async #executeSingleResolver(
#executeSingleResolver(
event: AppSyncResolverEvent<Record<string, unknown>>,
context: Context,
options?: ResolveOptions
): Promise<unknown> {
) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function can still return a promise, the type signature should reflect that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are quite a few uses of unknown in the AppSyncGraphQLResolver. Do you have some suggestions on how we could we narrow down the return types for this?

const { fieldName, parentTypeName: typeName } = event.info;

const resolverHandlerOptions = this.resolverRegistry.resolve(
Expand Down
2 changes: 1 addition & 1 deletion packages/event-handler/src/appsync-graphql/index.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
export { AppSyncGraphQLResolver } from './AppSyncGraphQLResolver.js';
export {
ResolverNotFoundException,
InvalidBatchResponseException,
ResolverNotFoundException,
} from './errors.js';
export {
awsDate,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ class BedrockAgentFunctionResolver {
public tool<TParams extends Record<string, ParameterValue>>(
fn: ToolFunction<TParams>,
config: Configuration
): undefined {
) {
const { name } = config;
if (this.#tools.has(name)) {
this.#logger.warn(
Expand Down
1 change: 1 addition & 0 deletions packages/event-handler/src/rest/middleware/cors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import {
* }));
* ```
*
* @param options - Configuration options for CORS
* @param options.origin - The origin to allow requests from
* @param options.allowMethods - The HTTP methods to allow
* @param options.allowHeaders - The headers to allow
Expand Down
14 changes: 11 additions & 3 deletions packages/event-handler/src/rest/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@ import type {
HandlerResponse,
HttpMethod,
Middleware,
NextFunction,
Path,
RequestContext,
ValidationResult,
} from '../types/rest.js';
import {
Expand Down Expand Up @@ -149,8 +151,14 @@ export const isAPIGatewayProxyResult = (
* // -> middleware1 end
* ```
*/
export const composeMiddleware = (middleware: Middleware[]): Middleware => {
return async ({ reqCtx, next }): Promise<HandlerResponse | void> => {
export const composeMiddleware = (middleware: Middleware[]) => {
return async ({
reqCtx,
next,
}: {
reqCtx: RequestContext;
next: NextFunction;
}) => {
let index = -1;
let result: HandlerResponse | undefined;

Expand Down Expand Up @@ -193,6 +201,6 @@ export const composeMiddleware = (middleware: Middleware[]): Middleware => {
};

await dispatch(0);
return result;
if (result !== undefined) return result;
};
};
7 changes: 5 additions & 2 deletions packages/event-handler/src/types/bedrock-agent.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,10 @@ type ToolFunction<TParams = Record<string, ParameterValue>> = (
event: BedrockAgentFunctionEvent;
context: Context;
}
) => Promise<JSONValue | BedrockFunctionResponse>;
) =>
| Promise<JSONValue | BedrockFunctionResponse>
| JSONValue
| BedrockFunctionResponse;

/**
* Tool in the Bedrock Agent Function Resolver.
Expand Down Expand Up @@ -159,7 +162,7 @@ type ResolverOptions = {
*
* When no logger is provided, we'll only log warnings and errors using the global `console` object.
*/
logger?: GenericLogger;
logger?: Pick<GenericLogger, 'debug' | 'warn' | 'error'>;
};

export type {
Expand Down
13 changes: 9 additions & 4 deletions packages/event-handler/src/types/rest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ type ErrorHandler<T extends Error = Error> = (
) => Promise<HandlerResponse>;

interface ErrorConstructor<T extends Error = Error> {
new (...args: any[]): T;
new (...args: unknown[]): T;
prototype: T;
}

Expand Down Expand Up @@ -57,7 +57,7 @@ type HandlerResponse = Response | JSONObject;

type RouteHandler<TReturn = HandlerResponse> = (
reqCtx: RequestContext
) => Promise<TReturn>;
) => Promise<TReturn> | TReturn;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice


type HttpMethod = keyof typeof HttpVerbs;

Expand All @@ -78,12 +78,16 @@ type RestRouteOptions = {
middleware?: Middleware[];
};

type NextFunction = () => Promise<HandlerResponse | void>;
type NextFunction = () =>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm worried this change is going to encourage people not to await the next function because they think it being async is optional. Failing to await next causes the an error to be thrown and the whole requst fails.

Copy link
Contributor

@dreamorosi dreamorosi Sep 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got an idea (haven't tested it tho):

What if whenever someone calls app.use we wrap their middleware arrow function in a function we control, before putting the wrapped middleware in the array.

Our function becomes the next and doesn't care if it's called with await or not, but it will always await the function it's wrapping before returning its result.

This way (assuming I'm not just saying something dumb), it doesn't matter if customers forget to call await next or even if the middleware is asynchronous or synchronous, we'll always treat it as async.

Copy link
Contributor

@svozza svozza Sep 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the issue is not with awaiting the middleware, it's the step after that, which is actually happening inside the the middlware handler. Not calling await returns immediately and messes up the execution order. Even when I tested capturing the Promise and forcing an await inside the composeMiddleware function, the problem was still there.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok then - @sdangol I think this discussion is relevant to your question below.

| Promise<void>
| Promise<HandlerResponse>
| HandlerResponse
| void;

type Middleware = (args: {
reqCtx: RequestContext;
next: NextFunction;
}) => Promise<void | HandlerResponse>;
}) => Promise<void> | Promise<HandlerResponse> | HandlerResponse | void;

type RouteRegistryOptions = {
/**
Expand Down Expand Up @@ -176,4 +180,5 @@ export type {
RouteRegistryOptions,
ValidationResult,
CompressionOptions,
NextFunction,
};
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ describe('Class: AppSyncEventsResolver', () => {
public scope = 'scoped';

@app.onPublish('/foo', { aggregate })
public async handleFoo(payloads: OnPublishAggregatePayload) {
public handleFoo(payloads: OnPublishAggregatePayload) {
return payloads.map((payload) => {
return {
id: payload.id,
Expand All @@ -97,15 +97,15 @@ describe('Class: AppSyncEventsResolver', () => {
}

@app.onPublish('/bar')
public async handleBar(payload: string) {
public handleBar(payload: string) {
return `${this.scope} ${payload}`;
}

public async handler(event: unknown, context: Context) {
public handler(event: unknown, context: Context) {
return this.stuff(event, context);
}

async stuff(event: unknown, context: Context) {
stuff(event: unknown, context: Context) {
return app.resolve(event, context, { scope: this });
}
}
Expand Down Expand Up @@ -146,15 +146,15 @@ describe('Class: AppSyncEventsResolver', () => {
public scope = 'scoped';

@app.onSubscribe('/foo')
public async handleFoo(payload: AppSyncEventsSubscribeEvent) {
public handleFoo(payload: AppSyncEventsSubscribeEvent) {
console.debug(`${this.scope} ${payload.info.channel.path}`);
}

public async handler(event: unknown, context: Context) {
public handler(event: unknown, context: Context) {
return this.stuff(event, context);
}

async stuff(event: unknown, context: Context) {
stuff(event: unknown, context: Context) {
return app.resolve(event, context, { scope: this });
}
}
Expand Down Expand Up @@ -222,7 +222,7 @@ describe('Class: AppSyncEventsResolver', () => {
async ({ error, message }) => {
// Prepare
const app = new AppSyncEventsResolver({ logger: console });
app.onSubscribe('/foo', async () => {
app.onSubscribe('/foo', () => {
throw error;
});

Expand All @@ -242,7 +242,7 @@ describe('Class: AppSyncEventsResolver', () => {
it('throws an UnauthorizedException when thrown by the handler', async () => {
// Prepare
const app = new AppSyncEventsResolver({ logger: console });
app.onSubscribe('/foo', async () => {
app.onSubscribe('/foo', () => {
throw new UnauthorizedException('nah');
});

Expand All @@ -258,7 +258,7 @@ describe('Class: AppSyncEventsResolver', () => {
it('returns the response of the onPublish handler', async () => {
// Prepare
const app = new AppSyncEventsResolver({ logger: console });
app.onPublish('/foo', async (payload) => {
app.onPublish('/foo', (payload) => {
if (payload === 'foo') {
return true;
}
Expand Down Expand Up @@ -303,7 +303,7 @@ describe('Class: AppSyncEventsResolver', () => {
const app = new AppSyncEventsResolver({ logger: console });
app.onPublish(
'/foo',
async (payloads) => {
(payloads) => {
return payloads.map((payload) => ({
id: payload.id,
payload: true,
Expand Down Expand Up @@ -353,7 +353,7 @@ describe('Class: AppSyncEventsResolver', () => {
const app = new AppSyncEventsResolver({ logger: console });
app.onPublish(
'/foo',
async () => {
() => {
throw new Error('Error in handler');
},
{ aggregate: true }
Expand All @@ -379,7 +379,7 @@ describe('Class: AppSyncEventsResolver', () => {
const app = new AppSyncEventsResolver({ logger: console });
app.onPublish(
'/foo',
async () => {
() => {
throw new UnauthorizedException('nah');
},
{ aggregate: true }
Expand All @@ -400,7 +400,7 @@ describe('Class: AppSyncEventsResolver', () => {
const app = new AppSyncEventsResolver();
app.onPublish(
'/foo',
async () => {
() => {
throw new Error('Error in handler');
},
{ aggregate: true }
Expand Down
Loading
Loading