diff --git a/CHANGELOG.md b/CHANGELOG.md index 51200005..fb2ac342 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/lib/resolvers/error.js b/lib/resolvers/error.js index 5f190570..58ec3040 100644 --- a/lib/resolvers/error.js +++ b/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 } diff --git a/test/tests/localized.test.js b/test/tests/localized.test.js index 375d9044..d1f012e5 100644 --- a/test/tests/localized.test.js +++ b/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 () => { diff --git a/test/tests/logger-dev.test.js b/test/tests/logger-dev.test.js index d80b1b8e..4385a419 100644 --- a/test/tests/logger-dev.test.js +++ b/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) }) diff --git a/test/tests/logger-prod.test.js b/test/tests/logger-prod.test.js index 7df56054..707c28ed 100644 --- a/test/tests/logger-prod.test.js +++ b/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) }) diff --git a/test/tests/mutations/create.test.js b/test/tests/mutations/create.test.js index 72492d3d..ee5a04cb 100644 --- a/test/tests/mutations/create.test.js +++ b/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 () => { diff --git a/test/tests/mutations/delete.test.js b/test/tests/mutations/delete.test.js index e6f87d3d..919a19c0 100644 --- a/test/tests/mutations/delete.test.js +++ b/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 () => { diff --git a/test/tests/mutations/update.test.js b/test/tests/mutations/update.test.js index 6d24c176..00112db6 100644 --- a/test/tests/mutations/update.test.js +++ b/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 () => { diff --git a/test/tests/queries/filter.test.js b/test/tests/queries/filter.test.js index a3e98f84..e96272fd 100644 --- a/test/tests/queries/filter.test.js +++ b/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 diff --git a/test/tests/queries/queries.test.js b/test/tests/queries/queries.test.js index 135ff4f1..0f1de2ad 100644 --- a/test/tests/queries/queries.test.js +++ b/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 diff --git a/test/tests/types.test.js b/test/tests/types.test.js index 8ec770a5..1323e2e4 100644 --- a/test/tests/types.test.js +++ b/test/tests/types.test.js @@ -49,7 +49,7 @@ describe('graphql - types parsing and validation', () => { axios.defaults.validateStatus = false beforeEach(async () => { - await data.reset() + await data.reset() }) describe('cds.Binary', () => {