From e795dfc3dbfc7998ce6af394d974ef06b6f9985d Mon Sep 17 00:00:00 2001 From: Huy Pham <11185878+hdp617@users.noreply.github.com> Date: Wed, 6 May 2020 22:50:11 -0700 Subject: [PATCH 1/4] refactor: Move the logic to load user's function to loader.ts This separates the logic to load user's function and one to start the webserver. Moves the function signature type definition to function.ts. Also, adds more testcases for the function loading logic. See also: #134 --- src/functions.ts | 98 ++++++++++++++++ src/index.ts | 9 +- src/invoker.ts | 177 ++--------------------------- src/loader.ts | 101 ++++++++++++++++ test/data/with_main/foo.js | 24 ++++ test/data/with_main/package.json | 3 + test/data/without_main/function.js | 24 ++++ test/function.js | 18 --- test/invoker.ts | 38 +++---- test/loader.ts | 49 ++++++++ 10 files changed, 327 insertions(+), 214 deletions(-) create mode 100644 src/functions.ts create mode 100644 src/loader.ts create mode 100644 test/data/with_main/foo.js create mode 100644 test/data/with_main/package.json create mode 100644 test/data/without_main/function.js delete mode 100644 test/function.js create mode 100644 test/loader.ts diff --git a/src/functions.ts b/src/functions.ts new file mode 100644 index 00000000..2494f019 --- /dev/null +++ b/src/functions.ts @@ -0,0 +1,98 @@ +// Copyright 2019 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +import * as express from 'express'; + +export interface HttpFunction { + // tslint:disable-next-line:no-any express interface. + (req: express.Request, res: express.Response): any; +} +export interface EventFunction { + // tslint:disable-next-line:no-any + (data: {}, context: Context): any; +} +export interface EventFunctionWithCallback { + // tslint:disable-next-line:no-any + (data: {}, context: Context, callback: Function): any; +} +export type HandlerFunction = + | HttpFunction + | EventFunction + | EventFunctionWithCallback; + +/** + * The Cloud Functions context object for the event. + * + * @link https://cloud.google.com/functions/docs/writing/background#function_parameters + */ +export interface CloudFunctionsContext { + /** + * A unique ID for the event. For example: "70172329041928". + */ + eventId?: string; + /** + * The date/time this event was created. For example: "2018-04-09T07:56:12.975Z" + * This will be formatted as ISO 8601. + */ + timestamp?: string; + /** + * The type of the event. For example: "google.pubsub.topic.publish". + */ + eventType?: string; + /** + * The resource that emitted the event. + */ + resource?: string; +} + +/** + * The CloudEvents v0.2 context object for the event. + * + * @link https://github.com/cloudevents/spec/blob/v0.2/spec.md#context-attributes + */ +export interface CloudEventsContext { + /** + * Type of occurrence which has happened. + */ + type?: string; + /** + * The version of the CloudEvents specification which the event uses. + */ + specversion?: string; + /** + * The event producer. + */ + source?: string; + /** + * ID of the event. + */ + id?: string; + /** + * Timestamp of when the event happened. + */ + time?: string; + /** + * A link to the schema that the event data adheres to. + */ + schemaurl?: string; + /** + * Content type of the event data. + */ + contenttype?: string; + + // tslint:disable-next-line:no-any CloudEvents extension attributes. + [key: string]: any; +} + +export type Context = CloudFunctionsContext | CloudEventsContext; diff --git a/src/index.ts b/src/index.ts index ed6ef53c..9c357efd 100644 --- a/src/index.ts +++ b/src/index.ts @@ -30,12 +30,9 @@ import * as minimist from 'minimist'; import { resolve } from 'path'; -import { - ErrorHandler, - SignatureType, - getServer, - getUserFunction, -} from './invoker'; +import { getUserFunction } from './loader'; + +import { ErrorHandler, SignatureType, getServer } from './invoker'; // Supported command-line flags const FLAG = { diff --git a/src/invoker.ts b/src/invoker.ts index 74f800b6..59f16dbf 100644 --- a/src/invoker.ts +++ b/src/invoker.ts @@ -29,89 +29,14 @@ import * as onFinished from 'on-finished'; import { FUNCTION_STATUS_HEADER_FIELD } from './types'; import { logAndSendError } from './logger'; import { isBinaryCloudEvent } from './cloudevents'; - -/** - * The Cloud Functions context object for the event. - * - * @link https://cloud.google.com/functions/docs/writing/background#function_parameters - */ -export interface CloudFunctionsContext { - /** - * A unique ID for the event. For example: "70172329041928". - */ - eventId?: string; - /** - * The date/time this event was created. For example: "2018-04-09T07:56:12.975Z" - * This will be formatted as ISO 8601. - */ - timestamp?: string; - /** - * The type of the event. For example: "google.pubsub.topic.publish". - */ - eventType?: string; - /** - * The resource that emitted the event. - */ - resource?: string; -} - -/** - * The CloudEvents v0.2 context object for the event. - * - * @link https://github.com/cloudevents/spec/blob/v0.2/spec.md#context-attributes - */ -export interface CloudEventsContext { - /** - * Type of occurrence which has happened. - */ - type?: string; - /** - * The version of the CloudEvents specification which the event uses. - */ - specversion?: string; - /** - * The event producer. - */ - source?: string; - /** - * ID of the event. - */ - id?: string; - /** - * Timestamp of when the event happened. - */ - time?: string; - /** - * A link to the schema that the event data adheres to. - */ - schemaurl?: string; - /** - * Content type of the event data. - */ - contenttype?: string; - - // tslint:disable-next-line:no-any CloudEvents extension attributes. - [key: string]: any; -} - -export type Context = CloudFunctionsContext | CloudEventsContext; - -export interface HttpFunction { - // tslint:disable-next-line:no-any express interface. - (req: express.Request, res: express.Response): any; -} -export interface EventFunctionWithCallback { - // tslint:disable-next-line:no-any - (data: {}, context: Context, callback: Function): any; -} -export interface EventFunction { - // tslint:disable-next-line:no-any - (data: {}, context: Context): any; -} -export type HandlerFunction = - | HttpFunction - | EventFunction - | EventFunctionWithCallback; +import { + HttpFunction, + EventFunction, + EventFunctionWithCallback, + HandlerFunction, + CloudFunctionsContext, + CloudEventsContext, +} from './functions'; // We optionally annotate the express Request with a rawBody field. // Express leaves the Express namespace open to allow merging of new fields. @@ -141,95 +66,9 @@ function isHttpFunction( return functionSignatureType === SignatureType.HTTP; } -/** - * Returns user's function from function file. - * Returns null if function can't be retrieved. - * @return User's function or null. - */ -export function getUserFunction( - codeLocation: string, - functionTarget: string -): HandlerFunction | null { - try { - const functionModulePath = getFunctionModulePath(codeLocation); - if (functionModulePath === null) { - console.error('Provided code is not a loadable module.'); - return null; - } - - const functionModule = require(functionModulePath); - let userFunction = functionTarget - .split('.') - .reduce((code, functionTargetPart) => { - if (typeof code === 'undefined') { - return undefined; - } else { - return code[functionTargetPart]; - } - }, functionModule); - - // TODO: do we want 'function' fallback? - if (typeof userFunction === 'undefined') { - if (functionModule.hasOwnProperty('function')) { - userFunction = functionModule['function']; - } else { - console.error( - `Function '${functionTarget}' is not defined in the provided ` + - 'module.\nDid you specify the correct target function to execute?' - ); - return null; - } - } - - if (typeof userFunction !== 'function') { - console.error( - `'${functionTarget}' needs to be of type function. Got: ` + - `${typeof userFunction}` - ); - return null; - } - - return userFunction as HandlerFunction; - } catch (ex) { - let additionalHint: string; - // TODO: this should be done based on ex.code rather than string matching. - if (ex.stack && ex.stack.includes('Cannot find module')) { - additionalHint = - 'Did you list all required modules in the package.json ' + - 'dependencies?\n'; - } else { - additionalHint = 'Is there a syntax error in your code?\n'; - } - console.error( - `Provided module can't be loaded.\n${additionalHint}` + - `Detailed stack trace: ${ex.stack}` - ); - return null; - } -} - // Response object for the most recent request. let latestRes: express.Response | null = null; -/** - * Returns resolved path to the module containing the user function. - * Returns null if the module can not be identified. - * @param codeLocation Directory with user's code. - * @return Resolved path or null. - */ -function getFunctionModulePath(codeLocation: string): string | null { - let path: string | null = null; - try { - path = require.resolve(codeLocation); - } catch (ex) { - try { - // TODO: Decide if we want to keep this fallback. - path = require.resolve(codeLocation + '/function.js'); - } catch (ex) {} - } - return path; -} - /** * Sends back a response to the incoming request. * @param result Output from function execution. diff --git a/src/loader.ts b/src/loader.ts new file mode 100644 index 00000000..a255a5ed --- /dev/null +++ b/src/loader.ts @@ -0,0 +1,101 @@ +// Copyright 2019 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +import { HandlerFunction } from './functions'; + +/** + * Returns user's function from function file. + * Returns null if function can't be retrieved. + * @return User's function or null. + */ +export function getUserFunction( + codeLocation: string, + functionTarget: string +): HandlerFunction | null { + try { + const functionModulePath = getFunctionModulePath(codeLocation); + if (functionModulePath === null) { + console.error('Provided code is not a loadable module.'); + return null; + } + + const functionModule = require(functionModulePath); + let userFunction = functionTarget + .split('.') + .reduce((code, functionTargetPart) => { + if (typeof code === 'undefined') { + return undefined; + } else { + return code[functionTargetPart]; + } + }, functionModule); + + // TODO: do we want 'function' fallback? + if (typeof userFunction === 'undefined') { + if (functionModule.hasOwnProperty('function')) { + userFunction = functionModule['function']; + } else { + console.error( + `Function '${functionTarget}' is not defined in the provided ` + + 'module.\nDid you specify the correct target function to execute?' + ); + return null; + } + } + + if (typeof userFunction !== 'function') { + console.error( + `'${functionTarget}' needs to be of type function. Got: ` + + `${typeof userFunction}` + ); + return null; + } + + return userFunction as HandlerFunction; + } catch (ex) { + let additionalHint: string; + // TODO: this should be done based on ex.code rather than string matching. + if (ex.stack && ex.stack.includes('Cannot find module')) { + additionalHint = + 'Did you list all required modules in the package.json ' + + 'dependencies?\n'; + } else { + additionalHint = 'Is there a syntax error in your code?\n'; + } + console.error( + `Provided module can't be loaded.\n${additionalHint}` + + `Detailed stack trace: ${ex.stack}` + ); + return null; + } +} + +/** + * Returns resolved path to the module containing the user function. + * Returns null if the module can not be identified. + * @param codeLocation Directory with user's code. + * @return Resolved path or null. + */ +function getFunctionModulePath(codeLocation: string): string | null { + let path: string | null = null; + try { + path = require.resolve(codeLocation); + } catch (ex) { + try { + // TODO: Decide if we want to keep this fallback. + path = require.resolve(codeLocation + '/function.js'); + } catch (ex) {} + } + return path; +} diff --git a/test/data/with_main/foo.js b/test/data/with_main/foo.js new file mode 100644 index 00000000..42bc07f9 --- /dev/null +++ b/test/data/with_main/foo.js @@ -0,0 +1,24 @@ +/** + * + * + * @param {!Object} req request context. + * @param {!Object} res response context. + */ +function testHttpFunction (res, req) { + return 'PASS' +}; + +/** + * + * + * @param {!Object} data event payload. + * @param {!Object} context event metadata. + */ +function testEventFunction (data, context) { + return 'PASS'; +}; + +module.exports = { + testHttpFunction, + testEventFunction, +} diff --git a/test/data/with_main/package.json b/test/data/with_main/package.json new file mode 100644 index 00000000..438495d6 --- /dev/null +++ b/test/data/with_main/package.json @@ -0,0 +1,3 @@ +{ + "main": "foo.js" +} \ No newline at end of file diff --git a/test/data/without_main/function.js b/test/data/without_main/function.js new file mode 100644 index 00000000..42bc07f9 --- /dev/null +++ b/test/data/without_main/function.js @@ -0,0 +1,24 @@ +/** + * + * + * @param {!Object} req request context. + * @param {!Object} res response context. + */ +function testHttpFunction (res, req) { + return 'PASS' +}; + +/** + * + * + * @param {!Object} data event payload. + * @param {!Object} context event metadata. + */ +function testEventFunction (data, context) { + return 'PASS'; +}; + +module.exports = { + testHttpFunction, + testEventFunction, +} diff --git a/test/function.js b/test/function.js deleted file mode 100644 index e1c222d0..00000000 --- a/test/function.js +++ /dev/null @@ -1,18 +0,0 @@ -// Copyright 2019 Google LLC -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -exports.foo = (data, context) => { - // Message used for function load verification in test. - return 'Hello from foo'; -}; diff --git a/test/invoker.ts b/test/invoker.ts index e8659b56..e9546e80 100644 --- a/test/invoker.ts +++ b/test/invoker.ts @@ -14,20 +14,10 @@ import * as assert from 'assert'; import * as express from 'express'; +import * as functions from '../src/functions'; import * as invoker from '../src/invoker'; import * as supertest from 'supertest'; -describe('loading function', () => { - it('should load the function', () => { - const loadedFunction = invoker.getUserFunction( - process.cwd() + '/test/function', - 'foo' - ) as invoker.EventFunction; - const returned = loadedFunction({}, {}); - assert.strictEqual(returned, 'Hello from foo'); - }); -}); - describe('request to HTTP function', () => { interface TestData { name: string; @@ -137,11 +127,14 @@ describe('GCF event request to event function', () => { testData.forEach(test => { it(`should receive data and context from ${test.name}`, async () => { let receivedData: {} | null = null; - let receivedContext: invoker.CloudFunctionsContext | null = null; - const server = invoker.getServer((data: {}, context: invoker.Context) => { - receivedData = data; - receivedContext = context as invoker.CloudFunctionsContext; - }, invoker.SignatureType.EVENT); + let receivedContext: functions.CloudFunctionsContext | null = null; + const server = invoker.getServer( + (data: {}, context: functions.Context) => { + receivedData = data; + receivedContext = context as functions.CloudFunctionsContext; + }, + invoker.SignatureType.EVENT + ); await supertest(server) .post('/') .send(test.body) @@ -200,11 +193,14 @@ describe('CloudEvents request to event function', () => { testData.forEach(test => { it(`should receive data and context from ${test.name}`, async () => { let receivedData: {} | null = null; - let receivedContext: invoker.CloudEventsContext | null = null; - const server = invoker.getServer((data: {}, context: invoker.Context) => { - receivedData = data; - receivedContext = context as invoker.CloudEventsContext; - }, invoker.SignatureType.EVENT); + let receivedContext: functions.CloudEventsContext | null = null; + const server = invoker.getServer( + (data: {}, context: functions.Context) => { + receivedData = data; + receivedContext = context as functions.CloudEventsContext; + }, + invoker.SignatureType.EVENT + ); await supertest(server) .post('/') .set(test.headers) diff --git a/test/loader.ts b/test/loader.ts new file mode 100644 index 00000000..13388f35 --- /dev/null +++ b/test/loader.ts @@ -0,0 +1,49 @@ +// Copyright 2019 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +import * as assert from 'assert'; +import * as functions from '../src/functions'; +import * as loader from '../src/loader'; + +describe('loading HTTP function', () => { + interface TestData { + name: string; + codeLocation: string; + target: string; + } + + const testData: TestData[] = [ + { + name: 'function without "main" in package.json', + codeLocation: '/test/data/without_main', + target: 'testEventFunction', + }, + { + name: 'function with "main" in package.json', + codeLocation: '/test/data/with_main', + target: 'testEventFunction', + }, + ]; + + testData.forEach(test => { + it(`should load ${test.name}`, () => { + const loadedFunction = loader.getUserFunction( + process.cwd() + test.codeLocation, + test.target + ) as functions.EventFunction; + const returned = loadedFunction({}, {}); + assert.strictEqual(returned, 'PASS'); + }); + }); +}); From ebfd229cdd9afadd6c59a11326c8d4fc278d00d2 Mon Sep 17 00:00:00 2001 From: Huy Pham <11185878+hdp617@users.noreply.github.com> Date: Thu, 7 May 2020 16:27:07 -0700 Subject: [PATCH 2/4] fix: Add comments to test functions --- test/data/with_main/foo.js | 4 ++-- test/data/without_main/function.js | 4 ++-- test/loader.ts | 15 ++------------- 3 files changed, 6 insertions(+), 17 deletions(-) diff --git a/test/data/with_main/foo.js b/test/data/with_main/foo.js index 42bc07f9..e3699a83 100644 --- a/test/data/with_main/foo.js +++ b/test/data/with_main/foo.js @@ -1,5 +1,5 @@ /** - * + * Test HTTP function to test function loading. * * @param {!Object} req request context. * @param {!Object} res response context. @@ -9,7 +9,7 @@ function testHttpFunction (res, req) { }; /** - * + * Test event function to test function loading. * * @param {!Object} data event payload. * @param {!Object} context event metadata. diff --git a/test/data/without_main/function.js b/test/data/without_main/function.js index 42bc07f9..e3699a83 100644 --- a/test/data/without_main/function.js +++ b/test/data/without_main/function.js @@ -1,5 +1,5 @@ /** - * + * Test HTTP function to test function loading. * * @param {!Object} req request context. * @param {!Object} res response context. @@ -9,7 +9,7 @@ function testHttpFunction (res, req) { }; /** - * + * Test event function to test function loading. * * @param {!Object} data event payload. * @param {!Object} context event metadata. diff --git a/test/loader.ts b/test/loader.ts index 13388f35..25e18b9e 100644 --- a/test/loader.ts +++ b/test/loader.ts @@ -13,10 +13,11 @@ // limitations under the License. import * as assert from 'assert'; +import * as express from 'express'; import * as functions from '../src/functions'; import * as loader from '../src/loader'; -describe('loading HTTP function', () => { +describe('loading function', () => { interface TestData { name: string; codeLocation: string; @@ -35,15 +36,3 @@ describe('loading HTTP function', () => { target: 'testEventFunction', }, ]; - - testData.forEach(test => { - it(`should load ${test.name}`, () => { - const loadedFunction = loader.getUserFunction( - process.cwd() + test.codeLocation, - test.target - ) as functions.EventFunction; - const returned = loadedFunction({}, {}); - assert.strictEqual(returned, 'PASS'); - }); - }); -}); From c48e216f8bf4590981cf3eef6d3b266fb83f87b7 Mon Sep 17 00:00:00 2001 From: Huy Pham <11185878+hdp617@users.noreply.github.com> Date: Thu, 7 May 2020 18:48:35 -0700 Subject: [PATCH 3/4] fix: Fix loader.ts tests --- src/loader.ts | 9 +++++++++ test/loader.ts | 12 ++++++++++++ 2 files changed, 21 insertions(+) diff --git a/src/loader.ts b/src/loader.ts index a255a5ed..cda65891 100644 --- a/src/loader.ts +++ b/src/loader.ts @@ -12,6 +12,15 @@ // See the License for the specific language governing permissions and // limitations under the License. +// loader.ts +/** + * This package contains the logic to load user's function. + * @packageDocumentation + */ + +/** + * Import function signature type's definition. + */ import { HandlerFunction } from './functions'; /** diff --git a/test/loader.ts b/test/loader.ts index 25e18b9e..f6434517 100644 --- a/test/loader.ts +++ b/test/loader.ts @@ -36,3 +36,15 @@ describe('loading function', () => { target: 'testEventFunction', }, ]; + + testData.forEach(test => { + it(`should load ${test.name}`, () => { + const loadedFunction = loader.getUserFunction( + process.cwd() + test.codeLocation, + test.target + ) as functions.HttpFunction; + const returned = loadedFunction(express.request, express.response); + assert.strictEqual(returned, 'PASS'); + }); + }); +}); From 23645d8604246f2285c32c9a5f418d608a4c14f1 Mon Sep 17 00:00:00 2001 From: Huy Pham <11185878+hdp617@users.noreply.github.com> Date: Mon, 11 May 2020 17:58:01 -0700 Subject: [PATCH 4/4] fix: Remove unnecessary closure --- test/loader.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/loader.ts b/test/loader.ts index f6434517..38b611b1 100644 --- a/test/loader.ts +++ b/test/loader.ts @@ -37,7 +37,7 @@ describe('loading function', () => { }, ]; - testData.forEach(test => { + for (const test of testData) { it(`should load ${test.name}`, () => { const loadedFunction = loader.getUserFunction( process.cwd() + test.codeLocation, @@ -46,5 +46,5 @@ describe('loading function', () => { const returned = loadedFunction(express.request, express.response); assert.strictEqual(returned, 'PASS'); }); - }); + } });