fix(error): internal server error when formatting non CDS and GraphQLError errors (#132)

* Test status code of requests with undefined queries

* Fix trying to set stacktrace in extensions when error isn't GraphQLError

* Reorder function declarations

* Prettier format

* Add changelog entry

* Format changelog

* Improve comment

* Destructure response in tests

* Fix expects for GET requests

* Improve expects for POST requests

* Add comments about GraphiQL to expected HTML

Author: schwma

CHANGELOG.md

@@ -16,12 +16,12 @@ 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 logged error message in multiple error scenarios, as the inner stack trace already
-contained the precise initial segment of the outer stack trace.
+- 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
 
 - Load custom `errorFormatter` relative to CDS project root
+- Fix internal server error when formatting errors that aren't CDS errors (thrown by CDS or in custom handlers) or instances of GraphQLError, such as the error caused by requests with undefined `query` property 
 
 ### Removed
 

lib/resolvers/error.js

@@ -33,15 +33,8 @@ const _cdsToGraphQLError = (context, err) => {
   return Object.defineProperty(graphQLError, '_cdsError', { value: true, writable: false, enumerable: false })
 }
 
-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)
-  _log(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 abstract/generic
 // protocol adapter is designed and implemented.
@@ -73,13 +66,24 @@ const _log = error => {
   }
 }
 
+const _ensureError = error => (error instanceof Error ? error : new Error(error))
+
+const handleCDSError = (context, error) => {
+  error = _ensureError(error)
+  _log(error)
+  return _cdsToGraphQLError(context, error)
+}
+
 const formatError = error => {
+  // Note: error is not always an instance of GraphQLError
+
   // CDS errors have already been logged and already have a stacktrace in extensions
   if (error.originalError?._cdsError) return error
 
   if (LOG_GRAPHQL._error) LOG_GRAPHQL.error(error)
 
-  if (!IS_PRODUCTION) error.extensions.stacktrace = error.stack.split('\n')
+  // error does not have an extensions property when it is not an instance of GraphQLError
+  if (!IS_PRODUCTION && error.extensions) error.extensions.stacktrace = error.stack.split('\n')
 
   return error
 }

test/tests/localized.test.js

@@ -8,7 +8,7 @@ describe('graphql - queries with localized data', () => {
   axios.defaults.validateStatus = false
 
   beforeEach(async () => {
-    await data.reset() 
+    await data.reset()
   })
 
   test('query with default locale', async () => {

test/tests/logger-dev.test.js

@@ -13,7 +13,7 @@ describe('graphql - query logging in development', () => {
   let _log
 
   beforeEach(async () => {
-    await data.reset() 
+    await data.reset()
     _log = jest.spyOn(console, 'info')
   })
 
@@ -23,7 +23,9 @@ describe('graphql - query logging in development', () => {
 
   describe('POST requests', () => {
     test('Do not log requests with undefined queries', async () => {
-      await POST('/graphql')
+      const response = await POST('/graphql')
+      expect(response.status).toEqual(400)
+      expect(response.data.errors[0]).toEqual({ message: 'Missing query' })
       expect(_log.mock.calls.length).toEqual(0)
     })
 
@@ -138,7 +140,9 @@ describe('graphql - query logging in development', () => {
 
   describe('GET requests', () => {
     test('Do not log requests with undefined queries', async () => {
-      await GET('/graphql')
+      const response = await GET('/graphql')
+      expect(response.status).toEqual(200)
+      expect(response.data).toMatch(/html/i) // GraphiQL is returned
       expect(_log.mock.calls.length).toEqual(0)
     })
 

test/tests/logger-prod.test.js

@@ -20,7 +20,7 @@ describe('graphql - query logging with sanitization in production', () => {
   })
 
   beforeEach(async () => {
-    await data.reset() 
+    await data.reset()
     _log = jest.spyOn(console, 'info')
   })
 
@@ -30,7 +30,9 @@ describe('graphql - query logging with sanitization in production', () => {
 
   describe('POST requests', () => {
     test('Do not log requests with undefined queries', async () => {
-      await POST('/graphql')
+      const response = await POST('/graphql')
+      expect(response.status).toEqual(400)
+      expect(response.data.errors[0]).toEqual({ message: 'Missing query' })
       expect(_log.mock.calls.length).toEqual(0)
     })
 
@@ -147,7 +149,9 @@ describe('graphql - query logging with sanitization in production', () => {
 
   describe('GET requests', () => {
     test('Do not log requests with undefined queries', async () => {
-      await GET('/graphql')
+      const response = await GET('/graphql')
+      expect(response.status).toEqual(200)
+      expect(response.data).toMatch(/html/i) // GraphiQL is returned
       expect(_log.mock.calls.length).toEqual(0)
     })
 

test/tests/mutations/create.test.js

@@ -8,7 +8,7 @@ describe('graphql - create mutations', () => {
   axios.defaults.validateStatus = false
 
   beforeEach(async () => {
-    await data.reset() 
+    await data.reset()
   })
 
   test('create empty entry', async () => {

test/tests/mutations/delete.test.js

@@ -8,7 +8,7 @@ describe('graphql - delete mutations', () => {
   axios.defaults.validateStatus = false
 
   beforeEach(async () => {
-    await data.reset() 
+    await data.reset()
   })
 
   test('delete no entries', async () => {

test/tests/mutations/update.test.js

@@ -8,7 +8,7 @@ describe('graphql - update mutations', () => {
   axios.defaults.validateStatus = false
 
   beforeEach(async () => {
-    await data.reset() 
+    await data.reset()
   })
 
   test('update no entries', async () => {

test/tests/queries/filter.test.js

@@ -8,7 +8,7 @@ describe('graphql - filter', () => {
   axios.defaults.validateStatus = false
 
   beforeEach(async () => {
-    await data.reset() 
+    await data.reset()
   })
 
   // REVISIT: unskip for support of configurable schema flavors

test/tests/queries/queries.test.js

@@ -8,7 +8,7 @@ describe('graphql - queries', () => {
   axios.defaults.validateStatus = false
 
   beforeEach(async () => {
-    await data.reset() 
+    await data.reset()
   })
 
   // REVISIT: unskip for support of configurable schema flavors