Skip to content

Commit

Permalink
feat: add respondWithErrors config
Browse files Browse the repository at this point in the history
* Changed from `logger.level === 'debug'`
* Now responds with stack trace instead of just message
* Added tests
  • Loading branch information
brettstack committed Jun 17, 2019
1 parent a69af79 commit 53cf974
Show file tree
Hide file tree
Showing 6 changed files with 83 additions and 22 deletions.
20 changes: 12 additions & 8 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -47,14 +47,18 @@ app.get('/', (req, res) => {

## 4.0.0 Goals

1. Improved API - Simpler for end user to use and configure; extensible without breaking backwards compatibility or hurting API
2. Node.js 8+ only - can upgrade dependencies to latest (Jest); can use latest syntax in source and tests; can use server.listening; future-proof for Node.js 10
3. Promise resolution mode by default? Requires benchmarking. Otherwise try callback with callbackWaitsForEventLoop=false (configurable by user); requires benchmarking. If context.succeed is still most performant, leave as default.
4. Additional event sources - currently only supports API Gateway Proxy; should also support Lambda@Edge (https://github.com/awslabs/aws-serverless-express/issues/152) and ALB; have had a customer request for DynamoDB; should make it easy to provide your own IO mapping function.
5. Multiple header values - can get rid of set-cookie hack
6. Configure logging - NONE, ERROR, INFO, DEBUG; also include option to respond to 500s with the stack trace instead of empty string currently
7. Improved documentation
8. Option to strip base path for custom domains (https://github.com/awslabs/aws-serverless-express/issues/86)
1. ✅ Improved API - Simpler for end user to use and configure; extensible without breaking backwards compatibility or hurting API
2. ✅ Node.js 8+ only - can upgrade dependencies to latest (Jest); can use latest syntax in source and tests; can use server.listening; future-proof for Node.js 10
3. 🗓 Promise resolution mode by default? Requires benchmarking. Otherwise try callback with callbackWaitsForEventLoop=false (configurable by user); requires benchmarking. If context.succeed is still most performant, leave as default.
4. 🗓 Additional event sources - currently only supports API Gateway Proxy; should also support Lambda@Edge (https://github.com/awslabs/aws-serverless-express/issues/152) and ALB; have had a customer request for DynamoDB; should make it easy to provide your own IO mapping function.
1. Added ALB; requires refactoring of common logic, additional tests, example refactor.
2. Need to add Lambda@Edge
3. Need to add example of doing custom resolver (DynamoDB)
5. ✅ Multiple header values - can get rid of set-cookie hack
6. ✅ Configure logging - default winston and allow customers to provide their own; include option to respond to 500s with the stack trace instead of empty string currently
7. 🗓Improved documentation
8. ✅ Option to strip base path for custom domains (https://github.com/awslabs/aws-serverless-express/issues/86).
9. 🗓 Update example to include optional parameter for setting up custom domain

### Is AWS serverless right for my app?

Expand Down
46 changes: 46 additions & 0 deletions __tests__/unit.js
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,29 @@ describe('forwardConnectionErrorResponseToApiGateway', () => {
multiValueHeaders: {}
}))
})
test('responds with 502 status and stack trace', () => {
return new Promise(
(resolve) => {
const context = new MockContext(resolve)
const contextResolver = {
succeed: (p) => context.succeed(p.response)
}
awsServerlessExpressTransport.forwardConnectionErrorResponseToApiGateway({
error: new Error('There was a connection error...'),
resolver: contextResolver,
logger,
respondWithErrors: true
})
}
).then(successResponse => {
expect(successResponse).toEqual({
statusCode: 502,
body: successResponse.body,
multiValueHeaders: {}
})
expect(successResponse.body).toContain('Error: There was a connection error...\n at ')
})
})
})

describe('forwardLibraryErrorResponseToApiGateway', () => {
Expand All @@ -180,6 +203,29 @@ describe('forwardLibraryErrorResponseToApiGateway', () => {
multiValueHeaders: {}
}))
})
test('responds with 500 status and stack trace', () => {
return new Promise(
(resolve) => {
const context = new MockContext(resolve)
const contextResolver = {
succeed: (p) => context.succeed(p.response)
}
awsServerlessExpressTransport.forwardLibraryErrorResponseToApiGateway({
error: new Error('There was an error...'),
resolver: contextResolver,
logger,
respondWithErrors: true
})
}
).then(successResponse => {
expect(successResponse).toEqual({
statusCode: 500,
body: successResponse.body,
multiValueHeaders: {}
})
expect(successResponse.body).toContain('Error: There was an error...\n at ')
})
})
})

function getContextResolver (resolve) {
Expand Down
3 changes: 2 additions & 1 deletion examples/basic-starter/lambda.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@ const binaryMimeTypes = [
]
const ase = awsServerlessExpress.configure({
app,
binaryMimeTypes
binaryMimeTypes,
respondWithErrors: true
// loggerConfig: {
// level: 'debug'
// }
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@
"scripts": {
"test": "jest",
"test:watch": "jest --watch",
"coverage": "jest --coverage",
"test:coverage": "jest --coverage",
"cz": "git-cz",
"release": "semantic-release",
"release-local": "node -r dotenv/config node_modules/semantic-release/bin/semantic-release --no-ci --dry-run",
Expand Down
16 changes: 11 additions & 5 deletions src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,8 @@ function proxy ({
resolutionMode = 'CONTEXT_SUCCEED',
eventSource = getEventSourceBasedOnEvent({ event }),
eventFns = getEventFnsBasedOnEventSource({ eventSource }),
logger
logger,
respondWithErrors
}) {
logger.debug('Calling proxy', { event, context, resolutionMode, eventSource })
setCurrentLambdaInvoke({ event, context })
Expand All @@ -99,7 +100,8 @@ function proxy ({
resolver,
eventSource,
eventFns,
logger
logger,
respondWithErrors
})
} else {
logger.debug('Server isn\'t listening... Starting server. This is likely a cold-start. If you see this message on every request, you may be calling `awsServerlessExpress.createServer` on every call inside the handler function. If this is the case, consider moving it outside of the handler function for imrpoved performance.')
Expand All @@ -113,7 +115,8 @@ function proxy ({
resolver,
eventSource,
eventFns,
logger
logger,
respondWithErrors
})
})
}
Expand Down Expand Up @@ -141,6 +144,7 @@ function configure ({
resolutionMode: configureResolutionMode = 'CONTEXT_SUCCEED',
eventSource: configureEventSource,
eventFns: configureEventFns,
respondWithErrors: configureRespondWithErrors = false,
loggerConfig: configureLoggerConfig = {},
logger: configureLogger = createLogger({
...DEFAULT_LOGGER_CONFIG,
Expand All @@ -164,7 +168,8 @@ function configure ({
callback,
eventSource = configureEventSource,
eventFns = configureEventFns,
logger = configureLogger
logger = configureLogger,
respondWithErrors = configureRespondWithErrors
} = {}) => (proxy({
server,
event,
Expand All @@ -173,7 +178,8 @@ function configure ({
callback,
eventSource,
eventFns,
logger
logger,
respondWithErrors
})),
handler: configureHandler = (event, context, callback) => configureProxy({
event,
Expand Down
18 changes: 11 additions & 7 deletions src/transport.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,9 +46,9 @@ function forwardResponse ({
})
}

function forwardConnectionErrorResponseToApiGateway ({ error, resolver, logger }) {
function forwardConnectionErrorResponseToApiGateway ({ error, resolver, logger, respondWithErrors }) {
logger.error('aws-serverless-express connection error: ', error)
const body = logger.level === 'debug' ? error.message : ''
const body = respondWithErrors ? error.stack : ''
const errorResponse = {
statusCode: 502, // "DNS resolution, TCP level errors, or actual HTTP parse errors" - https://nodejs.org/api/http.html#http_http_request_options_callback
body,
Expand All @@ -58,9 +58,10 @@ function forwardConnectionErrorResponseToApiGateway ({ error, resolver, logger }
resolver.succeed({ response: errorResponse })
}

function forwardLibraryErrorResponseToApiGateway ({ error, resolver, logger }) {
function forwardLibraryErrorResponseToApiGateway ({ error, resolver, logger, respondWithErrors }) {
logger.error('aws-serverless-express error: ', error)
const body = logger.level === 'debug' ? error.message : ''

const body = respondWithErrors ? error.stack : ''
const errorResponse = {
statusCode: 500,
body,
Expand All @@ -77,7 +78,8 @@ function forwardRequestToNodeServer ({
resolver,
eventSource,
eventFns = getEventFnsBasedOnEventSource({ eventSource }),
logger
logger,
respondWithErrors
}) {
logger.debug('Forwarding request to application...')
try {
Expand All @@ -104,14 +106,16 @@ function forwardRequestToNodeServer ({
.on('error', (error) => forwardConnectionErrorResponseToApiGateway({
error,
resolver,
logger
logger,
respondWithErrors
}))
.end()
} catch (error) {
forwardLibraryErrorResponseToApiGateway({
error,
resolver,
logger
logger,
respondWithErrors
})
}
}
Expand Down

0 comments on commit 53cf974

Please sign in to comment.