diff --git a/CHANGELOG.md b/CHANGELOG.md index 132f0985..824578d3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- Improved consistency of handling results of different types returned by custom handlers in CRUD resolvers: + + Wrap only objects (i.e. not primitive types or arrays) returned by custom handlers in arrays in create, read, and update resolvers + + Delete mutations return the length of an array that is returned by a `DELETE` custom handler or 1 if a single object is returned + ### Fixed ### Removed diff --git a/lib/resolvers/crud/create.js b/lib/resolvers/crud/create.js index 3b98a451..25557d96 100644 --- a/lib/resolvers/crud/create.js +++ b/lib/resolvers/crud/create.js @@ -2,6 +2,7 @@ const { INSERT } = require('@sap/cds/lib').ql const { ARGS } = require('../../constants') const formatResult = require('../parse/ast/result') const { getArgumentByName, astToEntries } = require('../parse/ast2cqn') +const { isPlainObject } = require('../utils') const { entriesStructureToEntityStructure } = require('./utils') module.exports = async (service, entity, selection) => { @@ -12,7 +13,7 @@ module.exports = async (service, entity, selection) => { query.entries(entries) const result = await service.run(query) - const resultInArray = Array.isArray(result) ? result : [result] + const resultInArray = isPlainObject(result) ? [result] : result return formatResult(selection, resultInArray, true) } diff --git a/lib/resolvers/crud/delete.js b/lib/resolvers/crud/delete.js index db235697..b24a80b7 100644 --- a/lib/resolvers/crud/delete.js +++ b/lib/resolvers/crud/delete.js @@ -1,6 +1,7 @@ const { DELETE } = require('@sap/cds/lib').ql const { ARGS } = require('../../constants') const { getArgumentByName, astToWhere } = require('../parse/ast2cqn') +const { isPlainObject } = require('../utils') module.exports = async (service, entity, selection) => { let query = DELETE.from(entity) @@ -16,5 +17,10 @@ module.exports = async (service, entity, selection) => { else throw e } + // The CDS delete query returns the number of deleted entries + // However, custom handlers can return non-numeric results for delete + if (isPlainObject(result)) return 1 + if (Array.isArray(result)) return result.length + return result } diff --git a/lib/resolvers/crud/read.js b/lib/resolvers/crud/read.js index 2a009fb1..a3c338df 100644 --- a/lib/resolvers/crud/read.js +++ b/lib/resolvers/crud/read.js @@ -2,6 +2,7 @@ const { SELECT } = require('@sap/cds/lib').ql const { ARGS, CONNECTION_FIELDS } = require('../../constants') const { getArgumentByName, astToColumns, astToWhere, astToOrderBy, astToLimit } = require('../parse/ast2cqn') const formatResult = require('../parse/ast/result') +const { isPlainObject } = require('../utils') module.exports = async (service, entity, selection) => { const selections = selection.selectionSet.selections @@ -27,5 +28,7 @@ module.exports = async (service, entity, selection) => { const result = await service.run(query) - return formatResult(selection, result) + const resultInArray = isPlainObject(result) ? [result] : result + + return formatResult(selection, resultInArray) } diff --git a/lib/resolvers/crud/update.js b/lib/resolvers/crud/update.js index c9ea4f15..90031aeb 100644 --- a/lib/resolvers/crud/update.js +++ b/lib/resolvers/crud/update.js @@ -2,6 +2,7 @@ const { SELECT, UPDATE } = require('@sap/cds/lib').ql const { ARGS } = require('../../constants') const formatResult = require('../parse/ast/result') const { getArgumentByName, astToColumns, astToWhere, astToEntries } = require('../parse/ast2cqn') +const { isPlainObject } = require('../utils') const { entriesStructureToEntityStructure } = require('./utils') module.exports = async (service, entity, selection) => { @@ -26,12 +27,17 @@ module.exports = async (service, entity, selection) => { const result = await service.tx(async tx => { // read needs to be done before the update, otherwise the where clause might become invalid (case that properties in where clause are updated by the mutation) resultBeforeUpdate = await tx.run(queryBeforeUpdate) - if (resultBeforeUpdate.length === 0) return [] + if (resultBeforeUpdate.length === 0) return {} return tx.run(query) }) - // Merge selected fields with updated data - const mergedResults = resultBeforeUpdate.map(original => ({ ...original, ...result })) + let mergedResults = result + if (Array.isArray(resultBeforeUpdate)) { + // Merge selected fields with updated data + mergedResults = resultBeforeUpdate.map(original => ({ ...original, ...result })) + } - return formatResult(selection, mergedResults, true) + const resultInArray = isPlainObject(mergedResults) ? [mergedResults] : mergedResults + + return formatResult(selection, resultInArray, true) } diff --git a/lib/resolvers/parse/ast/fromObject.js b/lib/resolvers/parse/ast/fromObject.js index fa35feca..02a8a809 100644 --- a/lib/resolvers/parse/ast/fromObject.js +++ b/lib/resolvers/parse/ast/fromObject.js @@ -1,5 +1,5 @@ const { Kind } = require('graphql') -const { isPlainObject } = require('./util') +const { isPlainObject } = require('../../utils') const _nullValue = { kind: Kind.NULL } diff --git a/lib/resolvers/parse/ast/result.js b/lib/resolvers/parse/ast/result.js index c134e09c..24647e7b 100644 --- a/lib/resolvers/parse/ast/result.js +++ b/lib/resolvers/parse/ast/result.js @@ -1,6 +1,6 @@ const { CONNECTION_FIELDS } = require('../../../constants') +const { isPlainObject } = require('../../utils') const { getPotentiallyNestedNodesSelections } = require('../util') -const { isPlainObject } = require('./util') const formatResult = (field, result, skipTopLevelConnection) => { const _formatObject = (selections, object) => { diff --git a/lib/resolvers/parse/ast/util/index.js b/lib/resolvers/parse/ast/util/index.js deleted file mode 100644 index d7970344..00000000 --- a/lib/resolvers/parse/ast/util/index.js +++ /dev/null @@ -1,3 +0,0 @@ -const isPlainObject = value => value !== null && typeof value === 'object' && !Buffer.isBuffer(value) - -module.exports = { isPlainObject } diff --git a/lib/resolvers/utils/index.js b/lib/resolvers/utils/index.js index 1ac9c375..5ea76237 100644 --- a/lib/resolvers/utils/index.js +++ b/lib/resolvers/utils/index.js @@ -1,3 +1,6 @@ +const isPlainObject = value => + value !== null && typeof value === 'object' && !Array.isArray(value) && !Buffer.isBuffer(value) + const _ensureError = error => (error instanceof Error ? error : new Error(error)) const setResponse = async (response, key, value) => { @@ -8,4 +11,4 @@ const setResponse = async (response, key, value) => { } } -module.exports = { setResponse } +module.exports = { isPlainObject, setResponse } diff --git a/test/resources/custom-handlers/server.js b/test/resources/custom-handlers/server.js new file mode 100644 index 00000000..6808b2bb --- /dev/null +++ b/test/resources/custom-handlers/server.js @@ -0,0 +1,6 @@ +const cds = require('@sap/cds') +const path = require('path') + +cds.env.protocols = { + graphql: { path: '/graphql', impl: path.join(__dirname, '../../../index.js') } +} \ No newline at end of file diff --git a/test/resources/custom-handlers/srv/return-types.cds b/test/resources/custom-handlers/srv/return-types.cds new file mode 100644 index 00000000..663cb32c --- /dev/null +++ b/test/resources/custom-handlers/srv/return-types.cds @@ -0,0 +1,21 @@ +service ReturnTypesService { + entity Integer { + key id : UUID; + string : cds.String; + } + + entity String { + key id : UUID; + string : cds.String; + } + + entity Object { + key id : UUID; + string : cds.String; + } + + entity Array { + key id : UUID; + string : cds.String; + } +} diff --git a/test/resources/custom-handlers/srv/return-types.js b/test/resources/custom-handlers/srv/return-types.js new file mode 100644 index 00000000..586c1f9d --- /dev/null +++ b/test/resources/custom-handlers/srv/return-types.js @@ -0,0 +1,9 @@ +module.exports = srv => { + const string = 'foo' + const id = '0557a188-326e-4dcb-999b-e1acf7979fa3' + + srv.on('*', 'Integer', () => 999) + srv.on('*', 'String', () => string) + srv.on('*', 'Object', () => ({ id, string })) + srv.on('*', 'Array', () => [{ id, string }, { id, string }]) +} \ No newline at end of file diff --git a/test/tests/concurrency.test.js b/test/tests/concurrency.test.js index fd14c010..93ccfc77 100644 --- a/test/tests/concurrency.test.js +++ b/test/tests/concurrency.test.js @@ -1,4 +1,4 @@ -describe('graphql resolver concurrency', () => { +describe('graphql - resolver concurrency', () => { const cds = require('@sap/cds/lib') const path = require('path') const { gql } = require('../util') diff --git a/test/tests/custom-handlers.test.js b/test/tests/custom-handlers.test.js new file mode 100644 index 00000000..75cb93d9 --- /dev/null +++ b/test/tests/custom-handlers.test.js @@ -0,0 +1,246 @@ +describe('graphql - custom handlers', () => { + const cds = require('@sap/cds/lib') + const path = require('path') + const { gql } = require('../util') + + const { axios, POST } = cds.test(path.join(__dirname, '../resources/custom-handlers')) + // Prevent axios from throwing errors for non 2xx status codes + axios.defaults.validateStatus = false + + describe('Handling of different return types returned by custom handlers', () => { + const id = '0557a188-326e-4dcb-999b-e1acf7979fa3' + const string = 'foo' + + test('Integer, String, Object, and Array returned for CREATE custom handler', async () => { + const query = gql` + mutation { + ReturnTypesService { + Integer { + create(input: {}) { + id + string + } + } + String { + create(input: {}) { + id + string + } + } + Object { + create(input: {}) { + id + string + } + } + Array { + create(input: {}) { + id + string + } + } + } + } + ` + const data = { + ReturnTypesService: { + Integer: { + create: null + }, + String: { + create: null + }, + Object: { + create: [{ id, string }] + }, + Array: { + create: [ + { id, string }, + { id, string } + ] + } + } + } + const errors = [ + { + locations: [{ column: 15, line: 5 }], + message: 'Expected Iterable, but did not find one for field "ReturnTypesService_Integer_input.create".', + path: ['ReturnTypesService', 'Integer', 'create'] + }, + { + locations: [{ column: 15, line: 11 }], + message: 'Expected Iterable, but did not find one for field "ReturnTypesService_String_input.create".', + path: ['ReturnTypesService', 'String', 'create'] + } + ] + const response = await POST('/graphql', { query }) + expect(response.data).toEqual({ data, errors }) + }) + + test('Integer, String, Object, and Array returned for READ custom handler', async () => { + const query = gql` + query { + ReturnTypesService { + Integer { + nodes { + id + string + } + } + String { + nodes { + id + string + } + } + Object { + nodes { + id + string + } + } + Array { + nodes { + id + string + } + } + } + } + ` + const data = { + ReturnTypesService: { + Integer: { + nodes: null + }, + String: { + nodes: null + }, + Object: { + nodes: [{ id, string }] + }, + Array: { + nodes: [ + { id, string }, + { id, string } + ] + } + } + } + const response = await POST('/graphql', { query }) + expect(response.data).toEqual({ data }) + }) + + test('Integer, String, Object, and Array returned for UPDATE custom handler', async () => { + const query = gql` + mutation { + ReturnTypesService { + Integer { + update(filter: [], input: {}) { + id + string + } + } + String { + update(filter: [], input: {}) { + id + string + } + } + Object { + update(filter: [], input: {}) { + id + string + } + } + Array { + update(filter: [], input: {}) { + id + string + } + } + } + } + ` + const data = { + ReturnTypesService: { + Integer: { + update: null + }, + String: { + update: null + }, + Object: { + update: [{ id, string }] + }, + Array: { + update: [ + { id, string }, + { id, string } + ] + } + } + } + const errors = [ + { + locations: [{ column: 15, line: 5 }], + message: 'Expected Iterable, but did not find one for field "ReturnTypesService_Integer_input.update".', + path: ['ReturnTypesService', 'Integer', 'update'] + }, + { + locations: [{ column: 15, line: 11 }], + message: 'Expected Iterable, but did not find one for field "ReturnTypesService_String_input.update".', + path: ['ReturnTypesService', 'String', 'update'] + } + ] + const response = await POST('/graphql', { query }) + expect(response.data).toEqual({ data, errors }) + }) + + test('Integer, String, Object, and Array returned for DELETE custom handler', async () => { + const query = gql` + mutation { + ReturnTypesService { + Integer { + delete(filter: []) + } + String { + delete(filter: []) + } + Object { + delete(filter: []) + } + Array { + delete(filter: []) + } + } + } + ` + const data = { + ReturnTypesService: { + Integer: { + delete: 999 + }, + String: { + delete: null + }, + Object: { + delete: 1 + }, + Array: { + delete: 2 + } + } + } + const errors = [ + { + locations: [{ column: 15, line: 8 }], + message: 'Int cannot represent non-integer value: "foo"', + path: ['ReturnTypesService', 'String', 'delete'] + } + ] + const response = await POST('/graphql', { query }) + expect(response.data).toEqual({ data, errors }) + }) + }) +})