diff --git a/CHANGELOG.md b/CHANGELOG.md index 38838e44..205bd0b2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed - Bump `@graphiql/plugin-explorer` version to `^3` +- Make error handling compatible with `@sap/cds` version `>=9` +- Bump required `@sap/cds` version to `>=9` ### Fixed diff --git a/lib/errorFormatter.js b/lib/errorFormatter.js index 38f41cf4..866d9fa9 100644 --- a/lib/errorFormatter.js +++ b/lib/errorFormatter.js @@ -17,10 +17,10 @@ const _sanitizeProperty = (key, value, out) => { const errorFormatterFn = err => { const error = {} - const properties = Object.keys(err).concat('message') + let properties = Object.keys(err).concat('message', 'stack') // No stack for outer error of multiple errors, since the stack is not meaningful - if (!err.details) properties.push('stack') + if (err.details) properties = properties.filter(k => k !== 'stack') properties.forEach(k => _sanitizeProperty(k, err[k], error)) diff --git a/lib/resolvers/error.js b/lib/resolvers/error.js index 58ec3040..3871485a 100644 --- a/lib/resolvers/error.js +++ b/lib/resolvers/error.js @@ -1,12 +1,19 @@ const cds = require('@sap/cds') -const { i18n } = cds.env +const { i18n } = cds const LOG_CDS = cds.log() const LOG_GRAPHQL = cds.log('graphql') const { GraphQLError } = require('graphql') const { IS_PRODUCTION } = require('../utils') // FIXME: importing internal modules from @sap/cds is discouraged and not recommended for external usage -const { normalizeError, isClientError } = require('@sap/cds/libx/_runtime/common/error/frontend') +const { normalizeError } = require('@sap/cds/libx/odata/middleware/error') + +const _applyToJSON = error => { + // Make stack enumerable so it isn't stripped by toJSON function + if (error.stack) Object.defineProperty(error, 'stack', { value: error.stack, enumerable: true }) + // Call toJSON function to apply i18n + return error.toJSON ? error.toJSON() : error +} const _reorderProperties = error => { // 'stack' and 'stacktrace' to cover both common cases that a custom error formatter might return @@ -17,7 +24,21 @@ const _reorderProperties = error => { const _cdsToGraphQLError = (context, err) => { const { req, errorFormatter } = context - let { error } = normalizeError(err, req, errorFormatter) + let error = normalizeError(err, req, false) + + error = _applyToJSON(error) + if (error.details) error.details = error.details.map(_applyToJSON) + + // In case of 5xx errors in production don't reveal details to clients + if (IS_PRODUCTION && error.status >= 500 && error.$sanitize !== false) + error = { + message: i18n.messages.at(error.status, cds.context.locale) || 'Internal Server Error', + code: String(error.status) // toJSON is intentionally gone + } + + // Apply error formatter to error details first if they exist, then apply error formatter to outer error + if (error.details) error.details = error.details.map(errorFormatter) + error = errorFormatter(error) // Ensure error properties are ordered nicely for the client error = _reorderProperties(error) @@ -55,10 +76,10 @@ const _log = error => { delete error2log.stack } - error2log = normalizeError(error2log, { locale: i18n.default_language }, error => error).error + error2log = normalizeError(error2log, { locale: '' }, false) // determine if the status code represents a client error (4xx range) - if (isClientError(error2log)) { + if (error2log.status >= 400 && error2log.status < 500) { if (LOG_CDS._warn) LOG_CDS.warn(error2log) } else { // server error diff --git a/lib/utils/index.js b/lib/utils/index.js index ed982e92..c99076cf 100644 --- a/lib/utils/index.js +++ b/lib/utils/index.js @@ -1,4 +1,4 @@ -const IS_PRODUCTION = process.env.NODE_ENV === 'production' +const IS_PRODUCTION = process.env.NODE_ENV === 'production' || process.env.CDS_ENV === 'prod' const gqlName = cdsName => cdsName.replace(/\./g, '_') diff --git a/test/tests/error-handling-dev.test.js b/test/tests/error-handling-dev.test.js index 136bbf8e..2a9493f5 100644 --- a/test/tests/error-handling-dev.test.js +++ b/test/tests/error-handling-dev.test.js @@ -33,7 +33,7 @@ describe('graphql - error handling in development', () => { { message: 'Value is required', extensions: { - code: '400', + code: 'ASSERT_NOT_NULL', target: 'notEmptyI', stacktrace: expect.any(Array) } @@ -60,16 +60,16 @@ describe('graphql - error handling in development', () => { { message: 'Multiple errors occurred. Please see the details for more information.', extensions: { - code: '400', + code: 'MULTIPLE_ERRORS', details: [ { - code: '400', + code: 'ASSERT_NOT_NULL', message: 'Value is required', target: 'notEmptyI', stacktrace: expect.any(Array) }, { - code: '400', + code: 'ASSERT_NOT_NULL', message: 'Value is required', target: 'notEmptyS', stacktrace: expect.any(Array) @@ -101,7 +101,7 @@ describe('graphql - error handling in development', () => { { message: 'Value 10 is not in specified range [0, 3]', extensions: { - code: '400', + code: 'ASSERT_RANGE', target: 'inRange', args: [10, 0, 3], stacktrace: expect.any(Array) @@ -129,7 +129,7 @@ describe('graphql - error handling in development', () => { { message: 'Es sind mehrere Fehler aufgetreten.', extensions: { - code: '400', + code: 'MULTIPLE_ERRORS', details: [ { target: 'inRange', message: 'Wert ist erforderlich' }, { target: 'oneOfEnumValues', message: expect.stringContaining('Value "foo" is invalid') } @@ -145,7 +145,7 @@ describe('graphql - error handling in development', () => { const log = console.warn.mock.calls[0][1] expect(log).toMatchObject({ - code: '400', + code: 'MULTIPLE_ERRORS', details: [ { target: 'inRange', message: 'Value is required' }, { target: 'oneOfEnumValues', message: expect.stringContaining('Value "foo" is invalid') } @@ -172,7 +172,7 @@ describe('graphql - error handling in development', () => { { message: 'Error on READ A', extensions: { - code: '500', + code: '', myProperty: 'My value A1', $myProperty: 'My value A2', my: { nested: { property: 'My value A3' } }, @@ -202,7 +202,7 @@ describe('graphql - error handling in development', () => { { message: 'Error on READ B', extensions: { - code: '500', + code: '', stacktrace: expect.any(Array) } } @@ -228,7 +228,7 @@ describe('graphql - error handling in development', () => { { message: 'Error on READ C', extensions: { - code: '500', + code: '', stacktrace: expect.any(Array) } } @@ -311,7 +311,7 @@ describe('graphql - error handling in development', () => { { message: 'Multiple errors occurred. Please see the details for more information.', extensions: { - code: '500', + code: 'MULTIPLE_ERRORS', details: [ { code: 'Some-Custom-Code1', @@ -339,7 +339,7 @@ describe('graphql - error handling in development', () => { expect(response.data.errors[0].extensions.details[0].stacktrace[0]).not.toHaveLength(0) // Stacktrace exists and is not empty expect(response.data.errors[0].extensions.details[1].stacktrace[0]).not.toHaveLength(0) // Stacktrace exists and is not empty expect(console.error.mock.calls[0][1]).toMatchObject({ - code: '500', + code: 'MULTIPLE_ERRORS', message: 'Multiple errors occurred. Please see the details for more information.', details: [ { @@ -417,7 +417,7 @@ describe('graphql - error handling in development', () => { } } ] - const response = await POST('/graphql', { query }) + const response = await POST('/graphql', { query }, { headers: { 'Accept-Language': 'en' } }) expect(response.data).toMatchObject({ errors }) expect(response.data.errors[0].extensions.stacktrace).not.toHaveLength(0) // Stacktrace exists and is not empty }) @@ -440,7 +440,7 @@ describe('graphql - error handling in development', () => { { message: 'The order of 20 books exceeds the stock by 10', extensions: { - code: '500', + code: '', target: 'ORDER_EXCEEDS_STOCK', args: [20, 10], numericSeverity: 4, @@ -448,7 +448,7 @@ describe('graphql - error handling in development', () => { } } ] - const response = await POST('/graphql', { query }) + const response = await POST('/graphql', { query }, { headers: { 'Accept-Language': 'en' } }) expect(response.data).toMatchObject({ errors }) expect(response.data.errors[0].extensions.stacktrace).not.toHaveLength(0) // Stacktrace exists and is not empty }) diff --git a/test/tests/error-handling-prod.test.js b/test/tests/error-handling-prod.test.js index b57f35c9..2fecbdeb 100644 --- a/test/tests/error-handling-prod.test.js +++ b/test/tests/error-handling-prod.test.js @@ -34,7 +34,7 @@ describe('graphql - error handling in production', () => { { message: 'Value is required', extensions: { - code: '400', + code: 'ASSERT_NOT_NULL', target: 'notEmptyI' } } @@ -43,7 +43,7 @@ describe('graphql - error handling in production', () => { expect(response.data).toMatchObject({ errors }) expect(response.data.errors[0].extensions).not.toHaveProperty('stacktrace') // No stacktrace in production const log = console.warn.mock.calls[0][1] || JSON.parse(console.warn.mock.calls[0][0]) - expect(log).toMatchObject({ code: '400', target: 'notEmptyI', msg: 'Value is required' }) + expect(log).toMatchObject({ code: 'ASSERT_NOT_NULL', target: 'notEmptyI', msg: 'Value is required' }) }) test('Multiple @mandatory validation errors', async () => { @@ -62,15 +62,15 @@ describe('graphql - error handling in production', () => { { message: 'Multiple errors occurred. Please see the details for more information.', extensions: { - code: '400', + code: 'MULTIPLE_ERRORS', details: [ { - code: '400', + code: 'ASSERT_NOT_NULL', message: 'Value is required', target: 'notEmptyI' }, { - code: '400', + code: 'ASSERT_NOT_NULL', message: 'Value is required', target: 'notEmptyS' } @@ -101,7 +101,7 @@ describe('graphql - error handling in production', () => { { message: 'Value 10 is not in specified range [0, 3]', extensions: { - code: '400', + code: 'ASSERT_RANGE', target: 'inRange' } } @@ -127,15 +127,15 @@ describe('graphql - error handling in production', () => { { message: 'Es sind mehrere Fehler aufgetreten.', extensions: { - code: '400', + code: 'MULTIPLE_ERRORS', details: [ { - code: '400', + code: 'ASSERT_NOT_NULL', message: 'Wert ist erforderlich', target: 'inRange' }, { - code: '400', + code: 'ASSERT_ENUM', message: expect.stringContaining('Value "foo" is invalid'), target: 'oneOfEnumValues' } @@ -151,12 +151,12 @@ describe('graphql - error handling in production', () => { const log = console.warn.mock.calls[0][1] || JSON.parse(console.warn.mock.calls[0][0]) expect(log).toMatchObject({ - code: '400', + code: 'MULTIPLE_ERRORS', msg: 'Multiple errors occurred. Please see the details for more information.', details: [ - { code: '400', target: 'inRange', message: 'Value is required' }, + { code: 'ASSERT_NOT_NULL', target: 'inRange', message: 'Value is required' }, { - code: '400', + code: 'ASSERT_ENUM', target: 'oneOfEnumValues', message: expect.stringContaining('Value "foo" is invalid') } @@ -314,32 +314,19 @@ describe('graphql - error handling in production', () => { ` const errors = [ { - message: 'Multiple errors occurred. Please see the details for more information.', + message: 'Internal Server Error', extensions: { - code: '500', - details: [ - { - code: 'Some-Custom-Code1', - message: 'Some Custom Error Message 1', - target: 'some_field' - }, - { - code: 'Some-Custom-Code2', - message: 'Some Custom Error Message 2', - target: 'some_field' - } - ] + code: '500' } } ] const response = await POST('/graphql', { query }) expect(response.data).toMatchObject({ errors }) expect(response.data.errors[0].extensions).not.toHaveProperty('stacktrace') // No stacktrace outside of error details - expect(response.data.errors[0].extensions.details[0]).not.toHaveProperty('stacktrace') // No stacktrace in production - expect(response.data.errors[0].extensions.details[1]).not.toHaveProperty('stacktrace') // No stacktrace in production + expect(response.data.errors[0].extensions).not.toHaveProperty('details') // No details since one of the errors is 5xx const log = console.error.mock.calls[0][1] || JSON.parse(console.error.mock.calls[0][0]) expect(log).toMatchObject({ - code: '500', + code: 'MULTIPLE_ERRORS', msg: 'Multiple errors occurred. Please see the details for more information.', details: [ { @@ -411,7 +398,7 @@ describe('graphql - error handling in production', () => { } } ] - const response = await POST('/graphql', { query }) + const response = await POST('/graphql', { query }, { headers: { 'Accept-Language': 'en' } }) expect(response.data).toMatchObject({ errors }) expect(response.data.errors[0].extensions).not.toHaveProperty('stacktrace') // No stacktrace in production }) @@ -438,34 +425,7 @@ describe('graphql - error handling in production', () => { } } ] - const response = await POST('/graphql', { query }) - expect(response.data).toMatchObject({ errors }) - expect(response.data.errors[0].extensions).not.toHaveProperty('stacktrace') // No stacktrace in production - }) - - test('req.reject with custom code', async () => { - const query = gql` - mutation { - CustomHandlerErrorsService { - Orders { - create(input: { id: 3, quantity: 20, stock: 10 }) { - id - quantity - stock - } - } - } - } - ` - const errors = [ - { - message: 'Internal Server Error', - extensions: { - code: '500' - } - } - ] - const response = await POST('/graphql', { query }) + const response = await POST('/graphql', { query }, { headers: { 'Accept-Language': 'en' } }) expect(response.data).toMatchObject({ errors }) expect(response.data.errors[0].extensions).not.toHaveProperty('stacktrace') // No stacktrace in production })