Early return doesn't work when response is undefined

[dreamorosi]

Describe the bug
A clear and concise description of what the bug is.

From the early return documentation:

Some middlewares might need to stop the whole execution flow and return a response immediately.

If you want to do this you can invoke return response in your middleware.

This suggests that whenever returning the response, Middy.js should skip the handler and subsequent Middy.js handlers (i.e. after, etc.).

When the response itself is undefined however, this is not the case and the request cycle, including the handler, run normally.

To Reproduce
How to reproduce the behaviour:

Use the same code as the one in the docs, just slightly modified in the handler and add types (although the issue happens also in vanilla JS):

import middy from '@middy/core';
import type { MiddlewareObj } from '@middy/core';
import type { Context } from 'aws-lambda';

const calculateCacheId = (event: { key: string }): string => {
  return event.key;
};
const storage = {};

// middleware
const cacheMiddleware = (options: {
  calculateCacheId: (event: { key: string }) => string;
  storage: Record<string, unknown>;
}) => {
  let cacheKey: string;

  const cacheMiddlewareBefore: MiddlewareObj<{ key: string }>['before'] =
    async (request) => {
      cacheKey = options.calculateCacheId(request.event);
      if (Object.hasOwn(options.storage, cacheKey)) {
        // exits early and returns the value from the cache if it's already there
        return options.storage[cacheKey];
      }
    };

  const cacheMiddlewareAfter: MiddlewareObj['after'] = async (request) => {
    // stores the calculated response in the cache
    options.storage[cacheKey] = request.response;
  };

  return {
    before: cacheMiddlewareBefore,
    after: cacheMiddlewareAfter,
  };
};

// sample usage
const lambdaHandler = (event: unknown, context: Context) => {
  console.log('stuff'); // add a log to confirm it's running
};

export const handler = middy()
  .use(
    cacheMiddleware({
      calculateCacheId,
      storage,
    })
  )
  .handler(lambdaHandler);

Call the function twice with the same payload (i.e. { "foo": "bar" }):

Observe that the log stuff appears twice, which indicates the handler is being run for each request.

Expected behaviour
A clear and concise description of what you expected to happen.

Early return/interrupt in the before handler should support undefined.

Alternatively, if this is not possible, middleware authors should be able to return early & skip the handler in other ways.

As a last resort, if this is not possible/in scope, I would suggest clearly articulating this limitation in the documentation of the feature.

Environment (please complete the following information):

• Node.js: 20.x
• Middy: 4.x & 5.x
• AWS SDK N/A

Additional context

Based on tests, it seems that other falsy values like null or false work as expected. This suggests that internally you are checking for anything but undefined.

If this is the case, I understand that practically speaking it's quite tricky to distinguish between void (aka no return) and return undefined.

Even on the example of the docs for this feature however, it's more than possible that the function handler might return undefined, thus breaking the caching use case entirely.

I realize that this is a somewhat edge case, so as I suggested in the previous section, it'd be useful if Middy.js allowed middleware authors to somehow still return undefined and return early in other ways.

[willfarrell]

@MahouShoujoMivutilde reported as malware. Not sure how long it will take GH to remove.

@dreamorosi
Thanks for reporting.

Based on tests, it seems that other falsy values like null or false work as expected. This suggests that internally you are checking for anything but undefined

That is correct, and expected.

practically speaking it's quite tricky to distinguish between void (aka no return) and return undefined.

yep, I'm open to solutions. Having a predefined string was considered, but also could lead to an edge case of it being a valid response edge case.

I can update the does in the mean time.

[MahouShoujoMivutilde]

Good!

You're a member, you should be able to hide comments like that, iirc, even before github deletes it.

[willfarrell]

Did some research, I don't think it's possible to address this.

https://262.ecma-international.org/5.1/#sec-13.2.1 or the newer version, https://262.ecma-international.org/#await (27.7.5.3.3.f)

[willfarrell]

Related: #1125 (comment)

[dreamorosi]

yep, I'm open to solutions. Having a predefined string was considered, but also could lead to an edge case of it being a valid response edge case.

Maybe it was not the originally intended scope/purpose, but we could leverage the request.internal (or any custom field on the request) to indicate that a before handler wants to circuit break.

For example, building on the example from OP:

// middleware
const cacheMiddleware = (options: {
  calculateCacheId: (event: { key: string }) => string;
  storage: Record<string, unknown>;
}) => {
  let cacheKey: string;

  const cacheMiddlewareBefore: MiddlewareObj<{ key: string }>['before'] =
    async (request) => {
      cacheKey = options.calculateCacheId(request.event);
      if (Object.hasOwn(options.storage, cacheKey)) {
        // exits early and returns the value from the cache if it's already there
        request.internal.shortCircuit = {
          value: options.storage[cacheKey]
        }
      }
    };

  const cacheMiddlewareAfter: MiddlewareObj['after'] = async (request) => {
    // stores the calculated response in the cache
    options.storage[cacheKey] = request.response;
  };

  return {
    before: cacheMiddlewareBefore,
    after: cacheMiddlewareAfter,
  };
};

Middy.js could internally check whether request.internal.shortCircuit is an object with value, and if so, act the same way as an early return behaves today, and return request.internal.shortCircuit.value.

I intentionally created an object in request.internal.shortCircuit to avoid bumping into the same issue we're trying to solve.

This is just an idea, probably there are better / different ways.

[willfarrell]

Interesting idea. It could be moved to the request, request.shortCircuit, and avoid possible conflicts with other middleware. Let me reflect on this, and look at the core code to see what's possible. I'm open to adding something for v6.

[dreamorosi]

Sounds good, thanks for considering this.

I'm also happy to change the denomination of this from bug to feature request, if that means anything for a project management standpoint.

[willfarrell]

I put together an update to address this (#1237). I wasn't able to support an early response inside of onError, but I don't see a use case that would require that. I plan to release an alpha soon.

[willfarrell]

Closing, 6.0.0-alpha.0 is now released.