From 524520dc46a7520610c6837ffee7e4bb77f4950b Mon Sep 17 00:00:00 2001 From: Arley Triana Morin Date: Wed, 8 Nov 2023 09:49:09 +0100 Subject: [PATCH 01/22] feat(logging): selective error message logging by error type Previously, all errors were logged as generic errors. This enhancement introduces selective logging of warnings and errors based on their error type. Additionally, message interpolation has been introduced to resolve error message placeholders into human-readable text. --- lib/resolvers/error.js | 40 +++++++++++++++++++++++++++++++++++++-- lib/resolvers/response.js | 4 ++-- 2 files changed, 40 insertions(+), 4 deletions(-) diff --git a/lib/resolvers/error.js b/lib/resolvers/error.js index 308a2cce..0598639a 100644 --- a/lib/resolvers/error.js +++ b/lib/resolvers/error.js @@ -2,6 +2,7 @@ const cds = require('@sap/cds') const LOG_CDS = cds.log() const LOG_GRAPHQL = cds.log('graphql') const { normalizeError } = require('@sap/cds/libx/_runtime/common/error/frontend') +const { getErrorMessage } = require('@sap/cds/libx/_runtime/common/error/utils') const { GraphQLError } = require('graphql') const { IS_PRODUCTION } = require('../utils') @@ -31,14 +32,49 @@ const _cdsToGraphQLError = (context, err) => { } const _ensureError = error => (error instanceof Error ? error : new Error(error)) +const _clone = obj => Object.create(Object.getPrototypeOf(obj), Object.getOwnPropertyDescriptors(obj)) const handleCDSError = (context, error) => { error = _ensureError(error) - // TODO: choose log level depending on type of error analogous to OData adapter - if (LOG_CDS._error) LOG_CDS.error(error) + _log(context.req, error) return _cdsToGraphQLError(context, error) } +const _log = (req, error) => { + // do not log standard errors, e.g. TypeError + if (error.code === undefined && error.details === undefined) return + + // log errors and warnings only + if (LOG_CDS.level <= cds.log.levels.WARN) return + + // Clone of the original error object to prevent mutation and unintended side-effects. + // Notice that the cloned error is logged to standard output in its default language, + // whether the original error message is locale-dependent as it is usually sent in the + // HTTP response to HTTP Clients to be displayed in the user interface. + const error2log = _clone(error) + + // interpolate message placeholder to default language + error2log.message = getErrorMessage(error) + if (error.details) { + error2log.details = error.details.map(error => { + const clone = _clone(error) + clone.message = getErrorMessage(error) + return clone + }) + } + + const reqClone = _clone(req) + const statusCode = normalizeError(error2log, reqClone).statusCode + + if (statusCode >= 400 && statusCode < 500) { + // client error + if (LOG_CDS._warn) LOG_CDS.warn(error2log) + } else { + // server error + if (LOG_CDS._error) LOG_CDS.error(error2log) + } +} + const formatError = error => { // CDS errors have already been logged and already have a stacktrace in extensions if (error.originalError?._cdsError) return error diff --git a/lib/resolvers/response.js b/lib/resolvers/response.js index 2453c903..3cb62c0a 100644 --- a/lib/resolvers/response.js +++ b/lib/resolvers/response.js @@ -3,8 +3,8 @@ const { handleCDSError } = require('./error') const setResponse = async (context, response, key, value) => { try { response[key] = await value - } catch (e) { - response[key] = handleCDSError(context, e) + } catch (error) { + response[key] = handleCDSError(context, error) } } From 89add68a9ffd98eb71673ec941c5bfb537354979 Mon Sep 17 00:00:00 2001 From: Arley Triana Morin Date: Wed, 8 Nov 2023 12:20:08 +0100 Subject: [PATCH 02/22] Update lib/resolvers/error.js Co-authored-by: Marcel Schwarz --- 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 0598639a..576f84ad 100644 --- a/lib/resolvers/error.js +++ b/lib/resolvers/error.js @@ -49,7 +49,7 @@ const _log = (req, error) => { // Clone of the original error object to prevent mutation and unintended side-effects. // Notice that the cloned error is logged to standard output in its default language, - // whether the original error message is locale-dependent as it is usually sent in the + // whereas the original error message is locale-dependent as it is usually sent in the // HTTP response to HTTP Clients to be displayed in the user interface. const error2log = _clone(error) From a1364aae32260a8c79a3d92c2c2ddcf2f05b300e Mon Sep 17 00:00:00 2001 From: Arley Triana Morin Date: Wed, 8 Nov 2023 15:57:26 +0100 Subject: [PATCH 03/22] hand over interpolation of message placeholders to CAP land --- lib/resolvers/error.js | 21 ++++++--------------- 1 file changed, 6 insertions(+), 15 deletions(-) diff --git a/lib/resolvers/error.js b/lib/resolvers/error.js index 576f84ad..059d03d4 100644 --- a/lib/resolvers/error.js +++ b/lib/resolvers/error.js @@ -2,7 +2,6 @@ const cds = require('@sap/cds') const LOG_CDS = cds.log() const LOG_GRAPHQL = cds.log('graphql') const { normalizeError } = require('@sap/cds/libx/_runtime/common/error/frontend') -const { getErrorMessage } = require('@sap/cds/libx/_runtime/common/error/utils') const { GraphQLError } = require('graphql') const { IS_PRODUCTION } = require('../utils') @@ -15,7 +14,8 @@ const _reorderProperties = error => { const _cdsToGraphQLError = (context, err) => { const { req, errorFormatter } = context - let { error } = normalizeError(err, req, errorFormatter) + const options = { filterFn: errorFormatter, locale: req.locale } + let { error } = normalizeError(err, req, options) // Ensure error properties are ordered nicely for the client error = _reorderProperties(error) @@ -52,22 +52,13 @@ const _log = (req, error) => { // whereas the original error message is locale-dependent as it is usually sent in the // HTTP response to HTTP Clients to be displayed in the user interface. const error2log = _clone(error) + if (error.details) error2log.details = error.details.map(error => _clone(error)) - // interpolate message placeholder to default language - error2log.message = getErrorMessage(error) - if (error.details) { - error2log.details = error.details.map(error => { - const clone = _clone(error) - clone.message = getErrorMessage(error) - return clone - }) - } - - const reqClone = _clone(req) - const statusCode = normalizeError(error2log, reqClone).statusCode + const errorOptions = { locale: 'en', filterFn: null } + const statusCode = normalizeError(error2log, _clone(req), errorOptions).statusCode + // determine if the status code represents a client error (4xx range) if (statusCode >= 400 && statusCode < 500) { - // client error if (LOG_CDS._warn) LOG_CDS.warn(error2log) } else { // server error From e23b3283deef82b77ce98cf3cc9f615eb90068ee Mon Sep 17 00:00:00 2001 From: Arley Triana Morin Date: Thu, 9 Nov 2023 11:42:40 +0100 Subject: [PATCH 04/22] WIP --- lib/resolvers/error.js | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/lib/resolvers/error.js b/lib/resolvers/error.js index 059d03d4..0a0706b4 100644 --- a/lib/resolvers/error.js +++ b/lib/resolvers/error.js @@ -2,6 +2,7 @@ const cds = require('@sap/cds') const LOG_CDS = cds.log() const LOG_GRAPHQL = cds.log('graphql') const { normalizeError } = require('@sap/cds/libx/_runtime/common/error/frontend') +const { i18n } = require('@sap/cds').env const { GraphQLError } = require('graphql') const { IS_PRODUCTION } = require('../utils') @@ -14,8 +15,7 @@ const _reorderProperties = error => { const _cdsToGraphQLError = (context, err) => { const { req, errorFormatter } = context - const options = { filterFn: errorFormatter, locale: req.locale } - let { error } = normalizeError(err, req, options) + let { error } = normalizeError(err, req, errorFormatter) // Ensure error properties are ordered nicely for the client error = _reorderProperties(error) @@ -36,11 +36,11 @@ const _clone = obj => Object.create(Object.getPrototypeOf(obj), Object.getOwnPro const handleCDSError = (context, error) => { error = _ensureError(error) - _log(context.req, error) + _log(error) return _cdsToGraphQLError(context, error) } -const _log = (req, error) => { +const _log = error => { // do not log standard errors, e.g. TypeError if (error.code === undefined && error.details === undefined) return @@ -54,8 +54,7 @@ const _log = (req, error) => { const error2log = _clone(error) if (error.details) error2log.details = error.details.map(error => _clone(error)) - const errorOptions = { locale: 'en', filterFn: null } - const statusCode = normalizeError(error2log, _clone(req), errorOptions).statusCode + const statusCode = normalizeError(error2log, { locale: i18n.default_language }, error => error).statusCode // determine if the status code represents a client error (4xx range) if (statusCode >= 400 && statusCode < 500) { From f5eeef400da605ac7092113bed78803a846d277c Mon Sep 17 00:00:00 2001 From: Arley Triana Morin Date: Thu, 9 Nov 2023 12:04:27 +0100 Subject: [PATCH 05/22] add TODO comment --- lib/resolvers/error.js | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/lib/resolvers/error.js b/lib/resolvers/error.js index 0a0706b4..ad3ca442 100644 --- a/lib/resolvers/error.js +++ b/lib/resolvers/error.js @@ -1,10 +1,10 @@ const cds = require('@sap/cds') +const { i18n } = cds.env const LOG_CDS = cds.log() const LOG_GRAPHQL = cds.log('graphql') -const { normalizeError } = require('@sap/cds/libx/_runtime/common/error/frontend') -const { i18n } = require('@sap/cds').env const { GraphQLError } = require('graphql') const { IS_PRODUCTION } = require('../utils') +const { normalizeError } = require('@sap/cds/libx/_runtime/common/error/frontend') const _reorderProperties = error => { // 'stack' and 'stacktrace' to cover both common cases that a custom error formatter might return @@ -40,6 +40,9 @@ const handleCDSError = (context, error) => { return _cdsToGraphQLError(context, error) } +// TODO: Revise this logging functionality, as it's not specific to protocol adapters. +// This function should be relocated and/or cleaned up when the new generic protocol adapter +// is designed and implemented. const _log = error => { // do not log standard errors, e.g. TypeError if (error.code === undefined && error.details === undefined) return From 6f2928055ab4041a463601a6c242b2aaecacdc97 Mon Sep 17 00:00:00 2001 From: Arley Triana Morin Date: Thu, 9 Nov 2023 12:05:36 +0100 Subject: [PATCH 06/22] abstract --- 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 ad3ca442..c855c93e 100644 --- a/lib/resolvers/error.js +++ b/lib/resolvers/error.js @@ -41,7 +41,7 @@ const handleCDSError = (context, error) => { } // TODO: Revise this logging functionality, as it's not specific to protocol adapters. -// This function should be relocated and/or cleaned up when the new generic protocol adapter +// This function should be relocated and/or cleaned up when the new abstract protocol adapter // is designed and implemented. const _log = error => { // do not log standard errors, e.g. TypeError From 7ab215c05b5d78d044cd6aca9f824ee48a77732c Mon Sep 17 00:00:00 2001 From: Arley Triana Morin Date: Thu, 9 Nov 2023 12:24:03 +0100 Subject: [PATCH 07/22] add FIXME --- lib/resolvers/error.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/resolvers/error.js b/lib/resolvers/error.js index c855c93e..1db7bc1b 100644 --- a/lib/resolvers/error.js +++ b/lib/resolvers/error.js @@ -4,6 +4,8 @@ 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 } = require('@sap/cds/libx/_runtime/common/error/frontend') const _reorderProperties = error => { From ea63b644795cce6a891e73ed66c71a85f4473c2f Mon Sep 17 00:00:00 2001 From: Arley Triana Morin Date: Thu, 9 Nov 2023 12:25:58 +0100 Subject: [PATCH 08/22] WIP --- 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 1db7bc1b..29e0eb9b 100644 --- a/lib/resolvers/error.js +++ b/lib/resolvers/error.js @@ -43,8 +43,8 @@ const handleCDSError = (context, error) => { } // TODO: Revise this logging functionality, as it's not specific to protocol adapters. -// This function should be relocated and/or cleaned up when the new abstract protocol adapter -// is designed and implemented. +// This function should be relocated and/or cleaned up when the new abstract/generic +// protocol adapter is designed and implemented. const _log = error => { // do not log standard errors, e.g. TypeError if (error.code === undefined && error.details === undefined) return From 66b84e15b3722440cabba8021310f9c8423fff05 Mon Sep 17 00:00:00 2001 From: Arley Triana Morin Date: Thu, 9 Nov 2023 15:42:38 +0100 Subject: [PATCH 09/22] use `isClientError` function --- lib/resolvers/error.js | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/lib/resolvers/error.js b/lib/resolvers/error.js index 29e0eb9b..3beb54d7 100644 --- a/lib/resolvers/error.js +++ b/lib/resolvers/error.js @@ -6,7 +6,7 @@ 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 } = require('@sap/cds/libx/_runtime/common/error/frontend') +const { normalizeError, isClientError } = require('@sap/cds/libx/_runtime/common/error/frontend') const _reorderProperties = error => { // 'stack' and 'stacktrace' to cover both common cases that a custom error formatter might return @@ -44,7 +44,7 @@ const handleCDSError = (context, error) => { // TODO: Revise this logging functionality, as it's not specific to protocol adapters. // This function should be relocated and/or cleaned up when the new abstract/generic -// protocol adapter is designed and implemented. +// protocol adapter is designed and implemented. const _log = error => { // do not log standard errors, e.g. TypeError if (error.code === undefined && error.details === undefined) return @@ -56,13 +56,12 @@ const _log = error => { // Notice that the cloned error is logged to standard output in its default language, // whereas the original error message is locale-dependent as it is usually sent in the // HTTP response to HTTP Clients to be displayed in the user interface. - const error2log = _clone(error) + let error2log = _clone(error) if (error.details) error2log.details = error.details.map(error => _clone(error)) - - const statusCode = normalizeError(error2log, { locale: i18n.default_language }, error => error).statusCode + error2log = normalizeError(error2log, { locale: i18n.default_language }, error => error).error // determine if the status code represents a client error (4xx range) - if (statusCode >= 400 && statusCode < 500) { + if (isClientError(error2log)) { if (LOG_CDS._warn) LOG_CDS.warn(error2log) } else { // server error From d843c37249ea8311847567013862152711e349b8 Mon Sep 17 00:00:00 2001 From: Arley Triana Morin Date: Thu, 9 Nov 2023 15:44:38 +0100 Subject: [PATCH 10/22] add tests --- test/tests/error-handling-prod.test.js | 67 ++++++++++++++++++++++++++ 1 file changed, 67 insertions(+) diff --git a/test/tests/error-handling-prod.test.js b/test/tests/error-handling-prod.test.js index 9852c0b4..963a7185 100644 --- a/test/tests/error-handling-prod.test.js +++ b/test/tests/error-handling-prod.test.js @@ -8,6 +8,15 @@ describe('graphql - error handling in production', () => { // Prevent axios from throwing errors for non 2xx status codes axios.defaults.validateStatus = false + beforeEach(() => { + jest.spyOn(console, 'warn') + jest.spyOn(console, 'error') + }) + + afterEach(() => { + jest.clearAllMocks() + }) + describe('Errors thrown by CDS', () => { test('Single @mandatory validation error', async () => { const query = gql` @@ -33,6 +42,16 @@ 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 + expect(console.warn.mock.calls[0][1]).toMatchObject({ + code: '400', + element: 'notEmptyI', + entity: 'ValidationErrorsService.A', + message: 'Value is required', + numericSeverity: 4, + target: 'notEmptyI', + type: 'cds.Integer', + value: undefined + }) }) test('Multiple @mandatory validation errors', async () => { @@ -137,6 +156,35 @@ describe('graphql - error handling in production', () => { 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(console.warn.mock.calls[0][1]).toMatchObject({ + code: '400', + message: 'Multiple errors occurred. Please see the details for more information.', + details: [ + { + args: ['inRange'], + code: '400', + element: 'inRange', + entity: 'ValidationErrorsService.C', + message: 'Value is required', + numericSeverity: 4, + target: 'inRange', + type: 'cds.Integer', + value: undefined + }, + { + args: ['"foo"', '"high", "medium", "low"'], + code: '400', + element: 'oneOfEnumValues', + entity: 'ValidationErrorsService.C', + enum: ['@assert.range', 'type', 'enum'], + message: 'Value "foo" is invalid according to enum declaration {"high", "medium", "low"}', + numericSeverity: 4, + target: 'oneOfEnumValues', + type: 'cds.String', + value: 'foo' + } + ] + }) }) }) @@ -310,6 +358,25 @@ describe('graphql - error handling in production', () => { 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(console.error.mock.calls[0][1]).toMatchObject({ + code: '500', + details: [ + { + code: 'Some-Custom-Code1', + message: 'Some Custom Error Message 1', + numericSeverity: 4, + status: 418, + target: 'some_field' + }, + { + code: 'Some-Custom-Code2', + message: 'Some Custom Error Message 2', + numericSeverity: 4, + status: 500, + target: 'some_field' + } + ] + }) }) test('Thrown error is modified in srv.on(error) handler', async () => { From d4e7e0ffbefe0c9ee235790e11bd0ef005adf66d Mon Sep 17 00:00:00 2001 From: Arley Triana Morin Date: Thu, 9 Nov 2023 21:52:05 +0100 Subject: [PATCH 11/22] update CHANGELOG.md --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 569b20bd..5856bde8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,10 +9,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added + - Message interpolation, transforming error message placeholders into human-readable text (default locale) + ### Changed - Moved registration of `cds.compile.to.gql` and `cds.compile.to.graphql` targets from `@sap/cds` to `@cap-js/graphql` - Improve merging of custom `graphql` protocol configuration with plugin default configuration +- Errors representing client errors (`4xx` range) are now logged as warnings instead of errors ### Fixed From d1a0f1ec9fd2c8f6f5ba67e9ce1d1896046888e7 Mon Sep 17 00:00:00 2001 From: Arley Triana Morin Date: Mon, 13 Nov 2023 11:13:59 +0100 Subject: [PATCH 12/22] add expected stack property to test --- test/tests/error-handling-prod.test.js | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/test/tests/error-handling-prod.test.js b/test/tests/error-handling-prod.test.js index 963a7185..9d8093bc 100644 --- a/test/tests/error-handling-prod.test.js +++ b/test/tests/error-handling-prod.test.js @@ -50,7 +50,8 @@ describe('graphql - error handling in production', () => { numericSeverity: 4, target: 'notEmptyI', type: 'cds.Integer', - value: undefined + value: undefined, + stack: expect.any(String) }) }) @@ -169,7 +170,8 @@ describe('graphql - error handling in production', () => { numericSeverity: 4, target: 'inRange', type: 'cds.Integer', - value: undefined + value: undefined, + stack: expect.any(String) }, { args: ['"foo"', '"high", "medium", "low"'], @@ -181,7 +183,8 @@ describe('graphql - error handling in production', () => { numericSeverity: 4, target: 'oneOfEnumValues', type: 'cds.String', - value: 'foo' + value: 'foo', + stack: expect.any(String) } ] }) @@ -366,14 +369,16 @@ describe('graphql - error handling in production', () => { message: 'Some Custom Error Message 1', numericSeverity: 4, status: 418, - target: 'some_field' + target: 'some_field', + stack: expect.any(String) }, { code: 'Some-Custom-Code2', message: 'Some Custom Error Message 2', numericSeverity: 4, status: 500, - target: 'some_field' + target: 'some_field', + stack: expect.any(String) } ] }) From 4b577e3a9bc1456b31d3c9fa085133e5854bd2e7 Mon Sep 17 00:00:00 2001 From: Arley Triana Morin Date: Mon, 13 Nov 2023 17:16:20 +0100 Subject: [PATCH 13/22] Update CHANGELOG.md Co-authored-by: Marcel Schwarz --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5856bde8..2f417cfe 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added - - Message interpolation, transforming error message placeholders into human-readable text (default locale) + - Message interpolation of logged errors, transforming error message placeholders into human-readable text (default locale) ### Changed From a28f82429be66e4f78606a2b7d1159e3f5d1d322 Mon Sep 17 00:00:00 2001 From: Arley Triana Morin Date: Mon, 13 Nov 2023 18:11:00 +0100 Subject: [PATCH 14/22] log errors thrown by `cds.error(...)` --- 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 3beb54d7..b2199aed 100644 --- a/lib/resolvers/error.js +++ b/lib/resolvers/error.js @@ -47,7 +47,7 @@ const handleCDSError = (context, error) => { // protocol adapter is designed and implemented. const _log = error => { // do not log standard errors, e.g. TypeError - if (error.code === undefined && error.details === undefined) return + if (error.message === undefined) return // log errors and warnings only if (LOG_CDS.level <= cds.log.levels.WARN) return From 68238cb552a7d57c1b0b0f56ebda3ba1fa412e54 Mon Sep 17 00:00:00 2001 From: Arley Triana Morin Date: Mon, 13 Nov 2023 18:49:09 +0100 Subject: [PATCH 15/22] Update lib/resolvers/error.js Co-authored-by: Marcel Schwarz --- lib/resolvers/error.js | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/lib/resolvers/error.js b/lib/resolvers/error.js index b2199aed..31b90af4 100644 --- a/lib/resolvers/error.js +++ b/lib/resolvers/error.js @@ -58,7 +58,12 @@ const _log = error => { // HTTP response to HTTP Clients to be displayed in the user interface. let error2log = _clone(error) if (error.details) error2log.details = error.details.map(error => _clone(error)) - error2log = normalizeError(error2log, { locale: i18n.default_language }, error => error).error + const formatterFn = error => { + // No stack for outer error of multiple errors, since the stacktrace is not meaningful + if (error.details) delete error.stack + return error + } + error2log = normalizeError(error2log, { locale: i18n.default_language }, formatterFn).error // determine if the status code represents a client error (4xx range) if (isClientError(error2log)) { From 004411336151f8e2f3260650f1e4690773330756 Mon Sep 17 00:00:00 2001 From: Arley Triana Morin Date: Mon, 13 Nov 2023 18:54:11 +0100 Subject: [PATCH 16/22] Update test/tests/error-handling-prod.test.js Co-authored-by: Marcel Schwarz --- test/tests/error-handling-prod.test.js | 1 + 1 file changed, 1 insertion(+) diff --git a/test/tests/error-handling-prod.test.js b/test/tests/error-handling-prod.test.js index 9d8093bc..ad15c955 100644 --- a/test/tests/error-handling-prod.test.js +++ b/test/tests/error-handling-prod.test.js @@ -188,6 +188,7 @@ describe('graphql - error handling in production', () => { } ] }) + expect(console.warn.mock.calls[0][1]).not.toHaveProperty('stacktrace') // No stacktrace outside of error details }) }) From ae76880ffda0864cdcff6d514cb90d509c558c38 Mon Sep 17 00:00:00 2001 From: Arley Triana Morin Date: Mon, 13 Nov 2023 18:55:24 +0100 Subject: [PATCH 17/22] Update test/tests/error-handling-prod.test.js Co-authored-by: Marcel Schwarz --- test/tests/error-handling-prod.test.js | 1 + 1 file changed, 1 insertion(+) diff --git a/test/tests/error-handling-prod.test.js b/test/tests/error-handling-prod.test.js index ad15c955..36103c1f 100644 --- a/test/tests/error-handling-prod.test.js +++ b/test/tests/error-handling-prod.test.js @@ -364,6 +364,7 @@ describe('graphql - error handling in production', () => { expect(response.data.errors[0].extensions.details[1]).not.toHaveProperty('stacktrace') // No stacktrace in production expect(console.error.mock.calls[0][1]).toMatchObject({ code: '500', + message: 'Multiple errors occurred. Please see the details for more information.', details: [ { code: 'Some-Custom-Code1', From 028f288d94702f694c4b05e9c8951637ab4858a4 Mon Sep 17 00:00:00 2001 From: Arley Triana Morin Date: Mon, 13 Nov 2023 18:56:16 +0100 Subject: [PATCH 18/22] Update test/tests/error-handling-prod.test.js Co-authored-by: Marcel Schwarz --- test/tests/error-handling-prod.test.js | 1 + 1 file changed, 1 insertion(+) diff --git a/test/tests/error-handling-prod.test.js b/test/tests/error-handling-prod.test.js index 36103c1f..e7089e31 100644 --- a/test/tests/error-handling-prod.test.js +++ b/test/tests/error-handling-prod.test.js @@ -384,6 +384,7 @@ describe('graphql - error handling in production', () => { } ] }) + expect(console.error.mock.calls[0][1]).not.toHaveProperty('stacktrace') // No stacktrace outside of error details }) test('Thrown error is modified in srv.on(error) handler', async () => { From abc81d425f196d1422acbb7479888a933dbdd5a8 Mon Sep 17 00:00:00 2001 From: Arley Triana Morin Date: Tue, 14 Nov 2023 09:28:34 +0100 Subject: [PATCH 19/22] refactor: remove stack of outer error --- lib/resolvers/error.js | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/lib/resolvers/error.js b/lib/resolvers/error.js index 31b90af4..77fc445f 100644 --- a/lib/resolvers/error.js +++ b/lib/resolvers/error.js @@ -57,13 +57,15 @@ const _log = error => { // whereas the original error message is locale-dependent as it is usually sent in the // HTTP response to HTTP Clients to be displayed in the user interface. let error2log = _clone(error) - if (error.details) error2log.details = error.details.map(error => _clone(error)) - const formatterFn = error => { - // No stack for outer error of multiple errors, since the stacktrace is not meaningful - if (error.details) delete error.stack - return error + if (error.details) { + error2log.details = error.details.map(error => _clone(error)) + + // Excluding the stack trace for the outer error as the inner stack trace already + // contains the initial segment of the outer stack trace. + delete error2log.stack } - error2log = normalizeError(error2log, { locale: i18n.default_language }, formatterFn).error + + error2log = normalizeError(error2log, { locale: i18n.default_language }, error => error).error // determine if the status code represents a client error (4xx range) if (isClientError(error2log)) { From 7df7b93bbb145319c85cb573458d66e49de59ef8 Mon Sep 17 00:00:00 2001 From: Arley Triana Morin Date: Tue, 14 Nov 2023 14:41:28 +0100 Subject: [PATCH 20/22] update CHANGELOG.md --- CHANGELOG.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2f417cfe..f40d51c4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,13 +9,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added - - Message interpolation of logged errors, transforming error message placeholders into human-readable text (default locale) +- Message interpolation of logged errors, transforming error message placeholders into human-readable text (default locale) ### Changed - Moved registration of `cds.compile.to.gql` and `cds.compile.to.graphql` targets from `@sap/cds` to `@cap-js/graphql` - Improve merging of custom `graphql` protocol configuration with plugin default configuration - Errors representing client errors (`4xx` range) are now logged as warnings instead of errors +- Exclude the stack trace of the outer error message in multiple error scenarios, as the inner stack trace already +contained the precise initial segment of the outer stack trace. ### Fixed From cc3101a0413554ebd6d0d896b3792163734f79e6 Mon Sep 17 00:00:00 2001 From: Arley Triana Morin Date: Tue, 14 Nov 2023 17:26:43 +0100 Subject: [PATCH 21/22] Update CHANGELOG.md Co-authored-by: Marcel Schwarz --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f40d51c4..51200005 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,7 +16,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Moved registration of `cds.compile.to.gql` and `cds.compile.to.graphql` targets from `@sap/cds` to `@cap-js/graphql` - Improve merging of custom `graphql` protocol configuration with plugin default configuration - Errors representing client errors (`4xx` range) are now logged as warnings instead of errors -- Exclude the stack trace of the outer error message in multiple error scenarios, as the inner stack trace already +- Exclude the stack trace of the outer logged error message in multiple error scenarios, as the inner stack trace already contained the precise initial segment of the outer stack trace. ### Fixed From 93c73ad7453542c7fa80c2f855d743a0a162ddde Mon Sep 17 00:00:00 2001 From: Arley Triana Morin Date: Tue, 14 Nov 2023 18:00:30 +0100 Subject: [PATCH 22/22] Update lib/resolvers/error.js Co-authored-by: Marcel Schwarz --- lib/resolvers/error.js | 3 --- 1 file changed, 3 deletions(-) diff --git a/lib/resolvers/error.js b/lib/resolvers/error.js index 77fc445f..5f190570 100644 --- a/lib/resolvers/error.js +++ b/lib/resolvers/error.js @@ -46,9 +46,6 @@ const handleCDSError = (context, error) => { // This function should be relocated and/or cleaned up when the new abstract/generic // protocol adapter is designed and implemented. const _log = error => { - // do not log standard errors, e.g. TypeError - if (error.message === undefined) return - // log errors and warnings only if (LOG_CDS.level <= cds.log.levels.WARN) return