From 2aab492962ecf9615cc582bbe0dd854f4ebbb1ab Mon Sep 17 00:00:00 2001 From: Marcel Schwarz Date: Tue, 29 Apr 2025 16:35:31 +0200 Subject: [PATCH 01/10] Adjust error handling for `cds^9` --- lib/errorFormatter.js | 4 +-- lib/resolvers/error.js | 35 ++++++++++++++++++++++---- lib/utils/index.js | 2 +- test/tests/error-handling-dev.test.js | 26 +++++++++---------- test/tests/error-handling-prod.test.js | 24 +++++++++--------- 5 files changed, 58 insertions(+), 33 deletions(-) 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..2c834b44 100644 --- a/lib/resolvers/error.js +++ b/lib/resolvers/error.js @@ -1,12 +1,12 @@ 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 _reorderProperties = error => { // 'stack' and 'stacktrace' to cover both common cases that a custom error formatter might return @@ -17,7 +17,32 @@ const _reorderProperties = error => { const _cdsToGraphQLError = (context, err) => { const { req, errorFormatter } = context - let { error } = normalizeError(err, req, errorFormatter) + let error = normalizeError(err, req, false) + + // Make stack enumerable so it isn't stripped by toJSON function + if (error.stack) Object.defineProperty(error, 'stack', { value: error.stack, enumerable: true }) + // Apply toJSON function to apply i18n + error = error.toJSON() + + if (error.details) { + error.details = error.details.map(error => { + // Make stack enumerable so it isn't stripped by toJSON function + if (error.stack) Object.defineProperty(error, 'stack', { value: error.stack, enumerable: true }) + // Apply toJSON function to apply i18n + return error.toJSON() + }) + } + + // 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 +80,10 @@ const _log = error => { delete error2log.stack } - error2log = normalizeError(error2log, { locale: i18n.default_language }, error => error).error + error2log = normalizeError(error2log, undefined, 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..deb2e195 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: [ { @@ -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, diff --git a/test/tests/error-handling-prod.test.js b/test/tests/error-handling-prod.test.js index b57f35c9..0acc2cec 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') } From e20885b5c306c00932fe0dc4176ebc75408e6c6c Mon Sep 17 00:00:00 2001 From: Marcel Schwarz Date: Tue, 29 Apr 2025 17:20:20 +0200 Subject: [PATCH 02/10] Fix test with multiple errors, where one is a status 5xx error --- test/tests/error-handling-prod.test.js | 21 ++++----------------- 1 file changed, 4 insertions(+), 17 deletions(-) diff --git a/test/tests/error-handling-prod.test.js b/test/tests/error-handling-prod.test.js index 0acc2cec..a5663ea6 100644 --- a/test/tests/error-handling-prod.test.js +++ b/test/tests/error-handling-prod.test.js @@ -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: [ { From 73b41f8a63634ee6bd825ca9fc8c1b30083d668a Mon Sep 17 00:00:00 2001 From: Marcel Schwarz Date: Tue, 29 Apr 2025 17:26:55 +0200 Subject: [PATCH 03/10] Add changelog entry --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 38838e44..f9bc7bce 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ 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` ### Fixed From 4c6903509f799ea6ab67d2aa4cb2f1514ce38a66 Mon Sep 17 00:00:00 2001 From: Marcel Schwarz Date: Tue, 29 Apr 2025 17:27:22 +0200 Subject: [PATCH 04/10] Bump required `@sap/cds` version to `>=9` --- CHANGELOG.md | 1 + package.json | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f9bc7bce..205bd0b2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - 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/package.json b/package.json index 835f4c04..f7d15740 100644 --- a/package.json +++ b/package.json @@ -36,7 +36,7 @@ "graphql-http": "^1.18.0" }, "peerDependencies": { - "@sap/cds": ">=7.8" + "@sap/cds": ">=9" }, "devDependencies": { "@cap-js/graphql": "file:.", From b8ba01592c6e44b1e0708773866fabfcbf69e245 Mon Sep 17 00:00:00 2001 From: Marcel Schwarz Date: Wed, 30 Apr 2025 10:21:09 +0200 Subject: [PATCH 05/10] Improve comment wording --- lib/resolvers/error.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/resolvers/error.js b/lib/resolvers/error.js index 2c834b44..7a278648 100644 --- a/lib/resolvers/error.js +++ b/lib/resolvers/error.js @@ -21,14 +21,14 @@ const _cdsToGraphQLError = (context, err) => { // Make stack enumerable so it isn't stripped by toJSON function if (error.stack) Object.defineProperty(error, 'stack', { value: error.stack, enumerable: true }) - // Apply toJSON function to apply i18n + // Call toJSON function to apply i18n error = error.toJSON() if (error.details) { error.details = error.details.map(error => { // Make stack enumerable so it isn't stripped by toJSON function if (error.stack) Object.defineProperty(error, 'stack', { value: error.stack, enumerable: true }) - // Apply toJSON function to apply i18n + // Call toJSON function to apply i18n return error.toJSON() }) } From 6cbed1d5b26db77453a03052e48fe9a579066165 Mon Sep 17 00:00:00 2001 From: Marcel Schwarz Date: Wed, 30 Apr 2025 10:37:36 +0200 Subject: [PATCH 06/10] Remove outdated test --- test/tests/error-handling-prod.test.js | 27 -------------------------- 1 file changed, 27 deletions(-) diff --git a/test/tests/error-handling-prod.test.js b/test/tests/error-handling-prod.test.js index a5663ea6..45035dd0 100644 --- a/test/tests/error-handling-prod.test.js +++ b/test/tests/error-handling-prod.test.js @@ -429,32 +429,5 @@ describe('graphql - error handling in production', () => { 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 }) - expect(response.data).toMatchObject({ errors }) - expect(response.data.errors[0].extensions).not.toHaveProperty('stacktrace') // No stacktrace in production - }) }) }) From 356d666f038adab85b96975b7f9ff9f267bb9a6c Mon Sep 17 00:00:00 2001 From: Marcel Schwarz Date: Wed, 30 Apr 2025 10:48:44 +0200 Subject: [PATCH 07/10] Only call `toJSON` function if it exists --- lib/resolvers/error.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/resolvers/error.js b/lib/resolvers/error.js index 7a278648..532b659d 100644 --- a/lib/resolvers/error.js +++ b/lib/resolvers/error.js @@ -22,14 +22,14 @@ const _cdsToGraphQLError = (context, err) => { // 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 - error = error.toJSON() + if (error.toJSON) error = error.toJSON() if (error.details) { error.details = error.details.map(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() + return error.toJSON ? error.toJSON() : error }) } From 43db6f847c7b6a431344f45b2d5855d38e7d68fd Mon Sep 17 00:00:00 2001 From: Marcel Schwarz Date: Wed, 30 Apr 2025 10:49:34 +0200 Subject: [PATCH 08/10] Add locale to custom error code tests --- test/tests/error-handling-dev.test.js | 4 ++-- test/tests/error-handling-prod.test.js | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/test/tests/error-handling-dev.test.js b/test/tests/error-handling-dev.test.js index deb2e195..2a9493f5 100644 --- a/test/tests/error-handling-dev.test.js +++ b/test/tests/error-handling-dev.test.js @@ -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 }) @@ -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 45035dd0..2fecbdeb 100644 --- a/test/tests/error-handling-prod.test.js +++ b/test/tests/error-handling-prod.test.js @@ -398,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 }) @@ -425,7 +425,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 }) From 077c0cc6842e8a55d6a3779bdab59d4858d48748 Mon Sep 17 00:00:00 2001 From: Marcel Schwarz Date: Wed, 30 Apr 2025 10:58:24 +0200 Subject: [PATCH 09/10] Extract common code to helper function --- lib/resolvers/error.js | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/lib/resolvers/error.js b/lib/resolvers/error.js index 532b659d..46cf253e 100644 --- a/lib/resolvers/error.js +++ b/lib/resolvers/error.js @@ -8,6 +8,13 @@ const { IS_PRODUCTION } = require('../utils') // FIXME: importing internal modules from @sap/cds is discouraged and not recommended for external usage 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 let { code, message, details, stack, stacktrace } = error @@ -19,19 +26,8 @@ const _cdsToGraphQLError = (context, err) => { const { req, errorFormatter } = context let error = normalizeError(err, req, false) - // 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 - if (error.toJSON) error = error.toJSON() - - if (error.details) { - error.details = error.details.map(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 - }) - } + 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) From 66bb3afe10c616d7129978003e4d7c489b4a2e1c Mon Sep 17 00:00:00 2001 From: Marcel Schwarz Date: Wed, 30 Apr 2025 11:34:47 +0200 Subject: [PATCH 10/10] Clarify intension by passing empty `locale` --- lib/resolvers/error.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/resolvers/error.js b/lib/resolvers/error.js index 46cf253e..3871485a 100644 --- a/lib/resolvers/error.js +++ b/lib/resolvers/error.js @@ -76,7 +76,7 @@ const _log = error => { delete error2log.stack } - error2log = normalizeError(error2log, undefined, false) + error2log = normalizeError(error2log, { locale: '' }, false) // determine if the status code represents a client error (4xx range) if (error2log.status >= 400 && error2log.status < 500) {