refactor!: support @sap/cds^9 error handling

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
 

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))
 

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

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, '_')
 

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,15 +440,15 @@ 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,
             stacktrace: expect.any(Array)
           }
         }
       ]
-      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
     })

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
     })