Skip to content

Commit 5cbce4e

Browse files
schwmajohannes-vogel
schwma
and
johannes-vogel
authored
refactor!: support @sap/cds^9 error handling (#194)
Co-authored-by: Johannes Vogel <[email protected]>
1 parent 65811ec commit 5cbce4e

File tree

6 files changed

+64
-81
lines changed

6 files changed

+64
-81
lines changed

‎CHANGELOG.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
1212
### Changed
1313

1414
- Bump `@graphiql/plugin-explorer` version to `^3`
15+
- Make error handling compatible with `@sap/cds` version `>=9`
16+
- Bump required `@sap/cds` version to `>=9`
1517

1618
### Fixed
1719

‎lib/errorFormatter.js‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,10 +17,10 @@ const _sanitizeProperty = (key, value, out) => {
1717
const errorFormatterFn = err => {
1818
const error = {}
1919

20-
const properties = Object.keys(err).concat('message')
20+
let properties = Object.keys(err).concat('message', 'stack')
2121

2222
// No stack for outer error of multiple errors, since the stack is not meaningful
23-
if (!err.details) properties.push('stack')
23+
if (err.details) properties = properties.filter(k => k !== 'stack')
2424

2525
properties.forEach(k => _sanitizeProperty(k, err[k], error))
2626

‎lib/resolvers/error.js‎

Lines changed: 26 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,19 @@
11
const cds = require('@sap/cds')
2-
const { i18n } = cds.env
2+
const { i18n } = cds
33
const LOG_CDS = cds.log()
44
const LOG_GRAPHQL = cds.log('graphql')
55
const { GraphQLError } = require('graphql')
66
const { IS_PRODUCTION } = require('../utils')
77

88
// FIXME: importing internal modules from @sap/cds is discouraged and not recommended for external usage
9-
const { normalizeError, isClientError } = require('@sap/cds/libx/_runtime/common/error/frontend')
9+
const { normalizeError } = require('@sap/cds/libx/odata/middleware/error')
10+
11+
const _applyToJSON = error => {
12+
// Make stack enumerable so it isn't stripped by toJSON function
13+
if (error.stack) Object.defineProperty(error, 'stack', { value: error.stack, enumerable: true })
14+
// Call toJSON function to apply i18n
15+
return error.toJSON ? error.toJSON() : error
16+
}
1017

1118
const _reorderProperties = error => {
1219
// 'stack' and 'stacktrace' to cover both common cases that a custom error formatter might return
@@ -17,7 +24,21 @@ const _reorderProperties = error => {
1724

1825
const _cdsToGraphQLError = (context, err) => {
1926
const { req, errorFormatter } = context
20-
let { error } = normalizeError(err, req, errorFormatter)
27+
let error = normalizeError(err, req, false)
28+
29+
error = _applyToJSON(error)
30+
if (error.details) error.details = error.details.map(_applyToJSON)
31+
32+
// In case of 5xx errors in production don't reveal details to clients
33+
if (IS_PRODUCTION && error.status >= 500 && error.$sanitize !== false)
34+
error = {
35+
message: i18n.messages.at(error.status, cds.context.locale) || 'Internal Server Error',
36+
code: String(error.status) // toJSON is intentionally gone
37+
}
38+
39+
// Apply error formatter to error details first if they exist, then apply error formatter to outer error
40+
if (error.details) error.details = error.details.map(errorFormatter)
41+
error = errorFormatter(error)
2142

2243
// Ensure error properties are ordered nicely for the client
2344
error = _reorderProperties(error)
@@ -55,10 +76,10 @@ const _log = error => {
5576
delete error2log.stack
5677
}
5778

58-
error2log = normalizeError(error2log, { locale: i18n.default_language }, error => error).error
79+
error2log = normalizeError(error2log, { locale: '' }, false)
5980

6081
// determine if the status code represents a client error (4xx range)
61-
if (isClientError(error2log)) {
82+
if (error2log.status >= 400 && error2log.status < 500) {
6283
if (LOG_CDS._warn) LOG_CDS.warn(error2log)
6384
} else {
6485
// server error

‎lib/utils/index.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
const IS_PRODUCTION = process.env.NODE_ENV === 'production'
1+
const IS_PRODUCTION = process.env.NODE_ENV === 'production' || process.env.CDS_ENV === 'prod'
22

33
const gqlName = cdsName => cdsName.replace(/\./g, '_')
44

‎test/tests/error-handling-dev.test.js‎

Lines changed: 15 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ describe('graphql - error handling in development', () => {
3333
{
3434
message: 'Value is required',
3535
extensions: {
36-
code: '400',
36+
code: 'ASSERT_NOT_NULL',
3737
target: 'notEmptyI',
3838
stacktrace: expect.any(Array)
3939
}
@@ -60,16 +60,16 @@ describe('graphql - error handling in development', () => {
6060
{
6161
message: 'Multiple errors occurred. Please see the details for more information.',
6262
extensions: {
63-
code: '400',
63+
code: 'MULTIPLE_ERRORS',
6464
details: [
6565
{
66-
code: '400',
66+
code: 'ASSERT_NOT_NULL',
6767
message: 'Value is required',
6868
target: 'notEmptyI',
6969
stacktrace: expect.any(Array)
7070
},
7171
{
72-
code: '400',
72+
code: 'ASSERT_NOT_NULL',
7373
message: 'Value is required',
7474
target: 'notEmptyS',
7575
stacktrace: expect.any(Array)
@@ -101,7 +101,7 @@ describe('graphql - error handling in development', () => {
101101
{
102102
message: 'Value 10 is not in specified range [0, 3]',
103103
extensions: {
104-
code: '400',
104+
code: 'ASSERT_RANGE',
105105
target: 'inRange',
106106
args: [10, 0, 3],
107107
stacktrace: expect.any(Array)
@@ -129,7 +129,7 @@ describe('graphql - error handling in development', () => {
129129
{
130130
message: 'Es sind mehrere Fehler aufgetreten.',
131131
extensions: {
132-
code: '400',
132+
code: 'MULTIPLE_ERRORS',
133133
details: [
134134
{ target: 'inRange', message: 'Wert ist erforderlich' },
135135
{ target: 'oneOfEnumValues', message: expect.stringContaining('Value "foo" is invalid') }
@@ -145,7 +145,7 @@ describe('graphql - error handling in development', () => {
145145

146146
const log = console.warn.mock.calls[0][1]
147147
expect(log).toMatchObject({
148-
code: '400',
148+
code: 'MULTIPLE_ERRORS',
149149
details: [
150150
{ target: 'inRange', message: 'Value is required' },
151151
{ target: 'oneOfEnumValues', message: expect.stringContaining('Value "foo" is invalid') }
@@ -172,7 +172,7 @@ describe('graphql - error handling in development', () => {
172172
{
173173
message: 'Error on READ A',
174174
extensions: {
175-
code: '500',
175+
code: '',
176176
myProperty: 'My value A1',
177177
$myProperty: 'My value A2',
178178
my: { nested: { property: 'My value A3' } },
@@ -202,7 +202,7 @@ describe('graphql - error handling in development', () => {
202202
{
203203
message: 'Error on READ B',
204204
extensions: {
205-
code: '500',
205+
code: '',
206206
stacktrace: expect.any(Array)
207207
}
208208
}
@@ -228,7 +228,7 @@ describe('graphql - error handling in development', () => {
228228
{
229229
message: 'Error on READ C',
230230
extensions: {
231-
code: '500',
231+
code: '',
232232
stacktrace: expect.any(Array)
233233
}
234234
}
@@ -311,7 +311,7 @@ describe('graphql - error handling in development', () => {
311311
{
312312
message: 'Multiple errors occurred. Please see the details for more information.',
313313
extensions: {
314-
code: '500',
314+
code: 'MULTIPLE_ERRORS',
315315
details: [
316316
{
317317
code: 'Some-Custom-Code1',
@@ -339,7 +339,7 @@ describe('graphql - error handling in development', () => {
339339
expect(response.data.errors[0].extensions.details[0].stacktrace[0]).not.toHaveLength(0) // Stacktrace exists and is not empty
340340
expect(response.data.errors[0].extensions.details[1].stacktrace[0]).not.toHaveLength(0) // Stacktrace exists and is not empty
341341
expect(console.error.mock.calls[0][1]).toMatchObject({
342-
code: '500',
342+
code: 'MULTIPLE_ERRORS',
343343
message: 'Multiple errors occurred. Please see the details for more information.',
344344
details: [
345345
{
@@ -417,7 +417,7 @@ describe('graphql - error handling in development', () => {
417417
}
418418
}
419419
]
420-
const response = await POST('/graphql', { query })
420+
const response = await POST('/graphql', { query }, { headers: { 'Accept-Language': 'en' } })
421421
expect(response.data).toMatchObject({ errors })
422422
expect(response.data.errors[0].extensions.stacktrace).not.toHaveLength(0) // Stacktrace exists and is not empty
423423
})
@@ -440,15 +440,15 @@ describe('graphql - error handling in development', () => {
440440
{
441441
message: 'The order of 20 books exceeds the stock by 10',
442442
extensions: {
443-
code: '500',
443+
code: '',
444444
target: 'ORDER_EXCEEDS_STOCK',
445445
args: [20, 10],
446446
numericSeverity: 4,
447447
stacktrace: expect.any(Array)
448448
}
449449
}
450450
]
451-
const response = await POST('/graphql', { query })
451+
const response = await POST('/graphql', { query }, { headers: { 'Accept-Language': 'en' } })
452452
expect(response.data).toMatchObject({ errors })
453453
expect(response.data.errors[0].extensions.stacktrace).not.toHaveLength(0) // Stacktrace exists and is not empty
454454
})

‎test/tests/error-handling-prod.test.js‎

Lines changed: 18 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ describe('graphql - error handling in production', () => {
3434
{
3535
message: 'Value is required',
3636
extensions: {
37-
code: '400',
37+
code: 'ASSERT_NOT_NULL',
3838
target: 'notEmptyI'
3939
}
4040
}
@@ -43,7 +43,7 @@ describe('graphql - error handling in production', () => {
4343
expect(response.data).toMatchObject({ errors })
4444
expect(response.data.errors[0].extensions).not.toHaveProperty('stacktrace') // No stacktrace in production
4545
const log = console.warn.mock.calls[0][1] || JSON.parse(console.warn.mock.calls[0][0])
46-
expect(log).toMatchObject({ code: '400', target: 'notEmptyI', msg: 'Value is required' })
46+
expect(log).toMatchObject({ code: 'ASSERT_NOT_NULL', target: 'notEmptyI', msg: 'Value is required' })
4747
})
4848

4949
test('Multiple @mandatory validation errors', async () => {
@@ -62,15 +62,15 @@ describe('graphql - error handling in production', () => {
6262
{
6363
message: 'Multiple errors occurred. Please see the details for more information.',
6464
extensions: {
65-
code: '400',
65+
code: 'MULTIPLE_ERRORS',
6666
details: [
6767
{
68-
code: '400',
68+
code: 'ASSERT_NOT_NULL',
6969
message: 'Value is required',
7070
target: 'notEmptyI'
7171
},
7272
{
73-
code: '400',
73+
code: 'ASSERT_NOT_NULL',
7474
message: 'Value is required',
7575
target: 'notEmptyS'
7676
}
@@ -101,7 +101,7 @@ describe('graphql - error handling in production', () => {
101101
{
102102
message: 'Value 10 is not in specified range [0, 3]',
103103
extensions: {
104-
code: '400',
104+
code: 'ASSERT_RANGE',
105105
target: 'inRange'
106106
}
107107
}
@@ -127,15 +127,15 @@ describe('graphql - error handling in production', () => {
127127
{
128128
message: 'Es sind mehrere Fehler aufgetreten.',
129129
extensions: {
130-
code: '400',
130+
code: 'MULTIPLE_ERRORS',
131131
details: [
132132
{
133-
code: '400',
133+
code: 'ASSERT_NOT_NULL',
134134
message: 'Wert ist erforderlich',
135135
target: 'inRange'
136136
},
137137
{
138-
code: '400',
138+
code: 'ASSERT_ENUM',
139139
message: expect.stringContaining('Value "foo" is invalid'),
140140
target: 'oneOfEnumValues'
141141
}
@@ -151,12 +151,12 @@ describe('graphql - error handling in production', () => {
151151
const log = console.warn.mock.calls[0][1] || JSON.parse(console.warn.mock.calls[0][0])
152152

153153
expect(log).toMatchObject({
154-
code: '400',
154+
code: 'MULTIPLE_ERRORS',
155155
msg: 'Multiple errors occurred. Please see the details for more information.',
156156
details: [
157-
{ code: '400', target: 'inRange', message: 'Value is required' },
157+
{ code: 'ASSERT_NOT_NULL', target: 'inRange', message: 'Value is required' },
158158
{
159-
code: '400',
159+
code: 'ASSERT_ENUM',
160160
target: 'oneOfEnumValues',
161161
message: expect.stringContaining('Value "foo" is invalid')
162162
}
@@ -314,32 +314,19 @@ describe('graphql - error handling in production', () => {
314314
`
315315
const errors = [
316316
{
317-
message: 'Multiple errors occurred. Please see the details for more information.',
317+
message: 'Internal Server Error',
318318
extensions: {
319-
code: '500',
320-
details: [
321-
{
322-
code: 'Some-Custom-Code1',
323-
message: 'Some Custom Error Message 1',
324-
target: 'some_field'
325-
},
326-
{
327-
code: 'Some-Custom-Code2',
328-
message: 'Some Custom Error Message 2',
329-
target: 'some_field'
330-
}
331-
]
319+
code: '500'
332320
}
333321
}
334322
]
335323
const response = await POST('/graphql', { query })
336324
expect(response.data).toMatchObject({ errors })
337325
expect(response.data.errors[0].extensions).not.toHaveProperty('stacktrace') // No stacktrace outside of error details
338-
expect(response.data.errors[0].extensions.details[0]).not.toHaveProperty('stacktrace') // No stacktrace in production
339-
expect(response.data.errors[0].extensions.details[1]).not.toHaveProperty('stacktrace') // No stacktrace in production
326+
expect(response.data.errors[0].extensions).not.toHaveProperty('details') // No details since one of the errors is 5xx
340327
const log = console.error.mock.calls[0][1] || JSON.parse(console.error.mock.calls[0][0])
341328
expect(log).toMatchObject({
342-
code: '500',
329+
code: 'MULTIPLE_ERRORS',
343330
msg: 'Multiple errors occurred. Please see the details for more information.',
344331
details: [
345332
{
@@ -411,7 +398,7 @@ describe('graphql - error handling in production', () => {
411398
}
412399
}
413400
]
414-
const response = await POST('/graphql', { query })
401+
const response = await POST('/graphql', { query }, { headers: { 'Accept-Language': 'en' } })
415402
expect(response.data).toMatchObject({ errors })
416403
expect(response.data.errors[0].extensions).not.toHaveProperty('stacktrace') // No stacktrace in production
417404
})
@@ -438,34 +425,7 @@ describe('graphql - error handling in production', () => {
438425
}
439426
}
440427
]
441-
const response = await POST('/graphql', { query })
442-
expect(response.data).toMatchObject({ errors })
443-
expect(response.data.errors[0].extensions).not.toHaveProperty('stacktrace') // No stacktrace in production
444-
})
445-
446-
test('req.reject with custom code', async () => {
447-
const query = gql`
448-
mutation {
449-
CustomHandlerErrorsService {
450-
Orders {
451-
create(input: { id: 3, quantity: 20, stock: 10 }) {
452-
id
453-
quantity
454-
stock
455-
}
456-
}
457-
}
458-
}
459-
`
460-
const errors = [
461-
{
462-
message: 'Internal Server Error',
463-
extensions: {
464-
code: '500'
465-
}
466-
}
467-
]
468-
const response = await POST('/graphql', { query })
428+
const response = await POST('/graphql', { query }, { headers: { 'Accept-Language': 'en' } })
469429
expect(response.data).toMatchObject({ errors })
470430
expect(response.data.errors[0].extensions).not.toHaveProperty('stacktrace') // No stacktrace in production
471431
})

0 commit comments

Comments
 (0)