fix: Support handlers exported from nested modules #1348

akinboboye · 2022-03-08T14:15:37Z

Description

Added support for finding handlers that are further down the export tree or in nested objects. e.g imagine the code and handler path below:

in index.js

exports.object = {
   handler1: async (_event, _context) => {
      return { body: "Hello offline!" };
   }
};

For this case the handler would be: index.object.handler1.

The problem is that offline would read the handlerPath and handler as index.object1 and this handler1 respectively. However, that handlerPath doesn't exist and would subsequently lead to a MODULE_NOT_FOUND error.

On the flip side of things, AWS Lambda has not issues with finding the handler using the path above. it works well and it is supported!

Another use case is when a handler that is exported from a particular module is then re-exported in another module as shown below:

user-handlers.js file

module.exports = {
   async getUser () {
   },
   
   async addUser() {
   }
}

and in an index file, you could import and re-export the handler as below:
index.js file

exports.userHandlers = require('./get-user'); // you could also do a named import here

The handlers definition in serverless.yml for getUser and addUser would then be
index.userHandlers.getUser and index.userHandlers.addUser respectively. The problem here again is that serverless-offline would not interpret the above handler correctly while AWS would.

Motivation and Context

In the examples given above, you could argue that things could have been arranged differently in order to work with how serverless-offline understands it. But should serverless-offline or AWS and Node js be our standard ? If AWS gives me flexibility to annex the full power of NodeJs modules for my use cases, while face limitations from serverless-offline?

Also the test fixture file src/lambda/__tests__/fixtures/lambdaFunction.fixture.js was rearranged to src/lambda/__tests__/fixtures/lambdaFunction.js where fixture is now exported. The problem with the former is that, AWS doesn't understand the lambdaFunction.fixture.js to be a module. It thinks lambdaFunction is the module to look for and fixture is just one of the exports from the file in which the handler lives in.

How Has This Been Tested?

all of the above use cases and other simple use cases were tested directly in AWS
existing tests were left in place as the above should not break any current functionalities
additional tests were added to check against the new handler path interpretation capabilities of serverless-offline.

Screenshots (if appropriate):

akinboboye · 2022-03-08T15:05:57Z

src/lambda/__tests__/fixtures/lambdaFunction.js

+  callbackHandlerDeferred(event, context, callback) {
+    setTimeout(() => callback(null, 'foo'), 100)
+  },
+
+  promiseHandler() {
+    return Promise.resolve('foo')
+  },
+
+  promiseHandlerDeferred() {
+    return new Promise((resolve) => {
+      setTimeout(() => resolve('foo'), 100)
+    })
+  },
+
+  async asyncFunctionHandler() {
+    return 'foo'
+  },
+
+  async asyncFunctionHandlerObject() {
+    return {
+      foo: 'bar',
+    }
+  },
+
+  // we deliberately test the case where a 'callback' is defined
+  // in the handler, but a promise is being returned to protect from a
+  // potential naive implementation, e.g.
+  //
+  // const { promisify } = 'utils'
+  // const promisifiedHandler = handler.length === 3 ? promisify(handler) : handler
+  //
+  // if someone would return a promise, but also defines callback, without using it
+  // the handler would not be returning anything
+
+  promiseWithDefinedCallbackHandler(
+    event, // eslint-disable-line no-unused-vars
+    context, // eslint-disable-line no-unused-vars
+    callback, // eslint-disable-line no-unused-vars
+  ) {
+    return Promise.resolve('Hello Promise!')
+  },
+
+  contextSucceedWithContextDoneHandler(event, context) {
+    context.succeed('Hello Context.succeed!')
+
+    context.done(null, 'Hello Context.done!')
+  },
+
+  callbackWithContextDoneHandler(event, context, callback) {
+    callback(null, 'Hello Callback!')
+
+    context.done(null, 'Hello Context.done!')
+  },
+
+  callbackWithPromiseHandler(event, context, callback) {
+    callback(null, 'Hello Callback!')
+
+    return Promise.resolve('Hello Promise!')
+  },
+
+  callbackInsidePromiseHandler(event, context, callback) {
+    return new Promise((resolve) => {
+      callback(null, 'Hello Callback!')
+
+      resolve('Hello Promise!')
+    })
+  },
+
+  async requestIdHandler(event, context) {
+    return context.awsRequestId
+  },
+
+  async remainingExecutionTimeHandler(event, context) {
+    const first = context.getRemainingTimeInMillis()
+
+    await new Promise((resolve) => {
+      setTimeout(resolve, 100)
+    })
+
+    const second = context.getRemainingTimeInMillis()
+
+    await new Promise((resolve) => {
+      setTimeout(resolve, 200)
+    })
+
+    const third = context.getRemainingTimeInMillis()
+
+    return [first, second, third]
+  },
+
+  async defaultTimeoutHandler(event, context) {
+    return context.getRemainingTimeInMillis()
+  },
+
+  executionTimeInMillisHandler() {
+    return new Promise((resolve) => {
+      setTimeout(resolve, 100)
+    })
+  },
+}


this file replaces src/lambda/__tests__/fixtures/lambdaFunction.fixture.js. AWS lamda will throw errors when a file path includes dot ..

akinboboye · 2022-03-08T15:05:57Z

src/lambda/__tests__/fixtures/lambdaFunction.js

+  callbackHandlerDeferred(event, context, callback) {
+    setTimeout(() => callback(null, 'foo'), 100)
+  },
+
+  promiseHandler() {
+    return Promise.resolve('foo')
+  },
+
+  promiseHandlerDeferred() {
+    return new Promise((resolve) => {
+      setTimeout(() => resolve('foo'), 100)
+    })
+  },
+
+  async asyncFunctionHandler() {
+    return 'foo'
+  },
+
+  async asyncFunctionHandlerObject() {
+    return {
+      foo: 'bar',
+    }
+  },
+
+  // we deliberately test the case where a 'callback' is defined
+  // in the handler, but a promise is being returned to protect from a
+  // potential naive implementation, e.g.
+  //
+  // const { promisify } = 'utils'
+  // const promisifiedHandler = handler.length === 3 ? promisify(handler) : handler
+  //
+  // if someone would return a promise, but also defines callback, without using it
+  // the handler would not be returning anything
+
+  promiseWithDefinedCallbackHandler(
+    event, // eslint-disable-line no-unused-vars
+    context, // eslint-disable-line no-unused-vars
+    callback, // eslint-disable-line no-unused-vars
+  ) {
+    return Promise.resolve('Hello Promise!')
+  },
+
+  contextSucceedWithContextDoneHandler(event, context) {
+    context.succeed('Hello Context.succeed!')
+
+    context.done(null, 'Hello Context.done!')
+  },
+
+  callbackWithContextDoneHandler(event, context, callback) {
+    callback(null, 'Hello Callback!')
+
+    context.done(null, 'Hello Context.done!')
+  },
+
+  callbackWithPromiseHandler(event, context, callback) {
+    callback(null, 'Hello Callback!')
+
+    return Promise.resolve('Hello Promise!')
+  },
+
+  callbackInsidePromiseHandler(event, context, callback) {
+    return new Promise((resolve) => {
+      callback(null, 'Hello Callback!')
+
+      resolve('Hello Promise!')
+    })
+  },
+
+  async requestIdHandler(event, context) {
+    return context.awsRequestId
+  },
+
+  async remainingExecutionTimeHandler(event, context) {
+    const first = context.getRemainingTimeInMillis()
+
+    await new Promise((resolve) => {
+      setTimeout(resolve, 100)
+    })
+
+    const second = context.getRemainingTimeInMillis()
+
+    await new Promise((resolve) => {
+      setTimeout(resolve, 200)
+    })
+
+    const third = context.getRemainingTimeInMillis()
+
+    return [first, second, third]
+  },
+
+  async defaultTimeoutHandler(event, context) {
+    return context.getRemainingTimeInMillis()
+  },
+
+  executionTimeInMillisHandler() {
+    return new Promise((resolve) => {
+      setTimeout(resolve, 100)
+    })
+  },
+}


this file replaces src/lambda/__tests__/fixtures/lambdaFunction.fixture.js. AWS lamda will throw errors when a file path includes dot ..

AWS currently allows handler functions to be buried deep inside of a module or object but serverless-offline doesn't. The change updates serverless offline to align with how AWS Lambda attempts to find a handler function. It also changes the fixture file `src/lambda/__tests__/fixtures/lambdaFunction.fixture.js` to align with how AWS expects the module path of handler to be.

akinboboye · 2022-03-09T13:55:58Z

@medikoo @pgrzesik @dherault could you please give the above PR a look ? Thanks!

akinboboye · 2022-03-15T14:54:36Z

@pgrzesik can you help review this ? or tell me what I have to do to get it reviewed ?

pgrzesik · 2022-03-15T15:14:56Z

Hey @akinboboye - I'll try to review it sometime this week

akinboboye · 2022-03-15T15:24:20Z

Hey @akinboboye - I'll try to review it sometime this week

thank you for replying on this! Much appreciated

pgrzesik

Thanks @akinboboye - it looks good in general - I only have a few minor comments and we should be good to go

pgrzesik · 2022-03-17T11:02:12Z

src/utils/splitHandlerPathAndName.js

@@ -1,6 +1,10 @@
 // some-folder/src.index => some-folder/src
 export default function splitHandlerPathAndName(handler) {
  // Split handler into method name and path i.e. handler.run
+  const prepathDelimiter = handler.lastIndexOf('/')


Wouldn't it be easier to do a split on / here?

I played around with this. Doing a split would break the the ruby logic and would also some not too easy to understand flows for determining the handler path , name and moduleNestings.

Let me know what you think. thanks

pgrzesik · 2022-03-17T11:03:30Z

src/utils/splitHandlerPathAndName.js

-  const delimiter = handler.lastIndexOf('.')
-  const path = handler.substr(0, delimiter)
-  const name = handler.substr(delimiter + 1)
+  const postpathSplit = postpath.split('.')


would something like

const [filename, ...moduleNesting] = postpath.split('.')

work here?

i applied this fix. Quite simpler and easy to understand. Thanks!

pgrzesik

Thank you @akinboboye 🙇

akinboboye force-pushed the fix-nested-module-handlers branch 2 times, most recently from 0afefd9 to 50b3181 Compare March 8, 2022 14:22

akinboboye commented Mar 8, 2022

View reviewed changes

akinboboye force-pushed the fix-nested-module-handlers branch from 50b3181 to b9e3c7a Compare March 8, 2022 17:07

akinboboye force-pushed the fix-nested-module-handlers branch from b9e3c7a to 3e6dced Compare March 9, 2022 13:08

akinboboye mentioned this pull request Mar 15, 2022

feat: Support websockets authorizers #1350

Merged

pgrzesik reviewed Mar 17, 2022

View reviewed changes

pgrzesik added the feature label Mar 17, 2022

simplified logic for extracting handler details

d76f006

akinboboye requested a review from pgrzesik March 18, 2022 02:47

pgrzesik approved these changes Mar 18, 2022

View reviewed changes

pgrzesik changed the title ~~fix: support handlers exported from nested modules~~ fix: Support handlers exported from nested modules Mar 18, 2022

pgrzesik merged commit e7c72f4 into dherault:master Mar 18, 2022

Itchibon777 mentioned this pull request Apr 25, 2024

[Snyk] Security upgrade serverless-offline from 6.0.0 to 8.6.0 Itchibon777/serverless-offline#60

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Support handlers exported from nested modules #1348

fix: Support handlers exported from nested modules #1348

akinboboye commented Mar 8, 2022 •

edited

akinboboye Mar 8, 2022

akinboboye Mar 8, 2022

akinboboye commented Mar 9, 2022 •

edited

akinboboye commented Mar 15, 2022

pgrzesik commented Mar 15, 2022

akinboboye commented Mar 15, 2022

pgrzesik left a comment

pgrzesik Mar 17, 2022

akinboboye Mar 18, 2022

pgrzesik Mar 17, 2022

akinboboye Mar 18, 2022

pgrzesik left a comment

fix: Support handlers exported from nested modules #1348

fix: Support handlers exported from nested modules #1348

Conversation

akinboboye commented Mar 8, 2022 • edited

Description

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

akinboboye Mar 8, 2022

Choose a reason for hiding this comment

akinboboye Mar 8, 2022

Choose a reason for hiding this comment

akinboboye commented Mar 9, 2022 • edited

akinboboye commented Mar 15, 2022

pgrzesik commented Mar 15, 2022

akinboboye commented Mar 15, 2022

pgrzesik left a comment

Choose a reason for hiding this comment

pgrzesik Mar 17, 2022

Choose a reason for hiding this comment

akinboboye Mar 18, 2022

Choose a reason for hiding this comment

pgrzesik Mar 17, 2022

Choose a reason for hiding this comment

akinboboye Mar 18, 2022

Choose a reason for hiding this comment

pgrzesik left a comment

Choose a reason for hiding this comment

akinboboye commented Mar 8, 2022 •

edited

akinboboye commented Mar 9, 2022 •

edited