From 8e21629c217b43d7a3c2e87d7072cd82f5232fab Mon Sep 17 00:00:00 2001 From: Sadiq Khoja Date: Mon, 12 Dec 2022 18:07:24 -0500 Subject: [PATCH] Enhance: complex type / groups can be '$select'ed (#717) * Enhance: complex type / groups can be '$select'ed * added a couple of unit tests --- lib/formats/odata.js | 100 +++++++++++++++++++----- test/data/xml.js | 140 +++++++++++++++++++++++++++++++++- test/integration/api/odata.js | 81 +++++++++++++++++++- test/unit/formats/odata.js | 53 ++++++++++++- 4 files changed, 350 insertions(+), 24 deletions(-) diff --git a/lib/formats/odata.js b/lib/formats/odata.js index bbafd68a3..8b5a3afa4 100644 --- a/lib/formats/odata.js +++ b/lib/formats/odata.js @@ -440,10 +440,66 @@ const singleRowToOData = (fields, row, domain, originalUrl, query) => { }); }; +// Create a tree of form fields +// Where each node has reference to its parent and children +// Returns an object with key=path and value=fieldNode +// so that we can quickly get the field node instead of traversing tree from top to bottom +// Assumption: formFields are in order +const getFieldTree = (formFields) => { + const tree = {}; + + for (let i = 0; i < formFields.length; i += 1) { + const node = { value: formFields[i], children: [], parent: null }; + tree[`${node.value.path.split('/').map(sanitizeOdataIdentifier).join('/')}`] = node; + } + + for (const i of Object.keys(tree)) { + const node = tree[i]; + const parentPath = node.value.path.match(/(^.*)\//)[1].split('/').map(sanitizeOdataIdentifier).join('/'); + + if (tree[parentPath]) { + node.parent = tree[parentPath]; + node.parent.children.push(node); + } + } + + return tree; +}; + +// Returns children recursively +const getChildren = (field) => { + const result = new Set(); + const stack = []; + stack.push(field); + + while (stack.length > 0) { + const node = stack.pop(); + node.children.forEach(c => { + if (c.value.type === 'structure') { + stack.push(c); + } + result.add(c.value); + }); + } + return result; +}; + // Validates $select query parameter including metadata properties and returns list of FormFields const filterFields = (formFields, select, table) => { const filteredFields = new Set(); - const fieldMap = formFields.reduce((map, field) => ({ ...map, [`${field.path.split('/').map(sanitizeOdataIdentifier).join('/')}`]: field }), {}); + const fieldTree = getFieldTree(formFields); + + let path = ''; + + // For subtables we have to include parents fields + if (table !== 'Submissions') { + for (const tableSegment of table.replace(/Submissions\./, '').split('.')) { + path += `/${tableSegment}`; + if (!fieldTree[path]) throw Problem.user.notFound(); + filteredFields.add(fieldTree[path].value); + } + } + for (const property of select.split(',').map(p => p.trim())) { // validate metadata properties. __system/.. properties are only valid for Submission table if (property.startsWith('__id') || property.startsWith('__system')) { @@ -451,27 +507,25 @@ const filterFields = (formFields, select, table) => { throw Problem.user.propertyNotFound({ property }); } else { - let path = ''; + const field = fieldTree[`${path}/${property}`]; + if (!field) throw Problem.user.propertyNotFound({ property }); - // For subtables we have to include parents fields - if (table !== 'Submissions') { - for (const tableSegment of table.replace(/Submissions\./, '').split('.')) { - path += `/${tableSegment}`; - if (!fieldMap[path]) throw Problem.user.notFound(); - filteredFields.add(fieldMap[path]); - } - } + filteredFields.add(field.value); // we have to include parents fields in the result to handle grouped fields - const propSegments = property.split('/'); - for (let i = 0; i < propSegments.length; i+=1) { - path += `/${propSegments[i]}`; - const field = fieldMap[path]; - if (!field) throw Problem.user.propertyNotFound({ property }); - // it's ok to include field with repeat type so that user can have navigation link - // but child field of a repeat field is not supported - if (field.type === 'repeat' && i < propSegments.length - 1) throw Problem.user.unsupportedODataSelectField({ property }); - filteredFields.add(field); + let node = field.parent; + while (node && !filteredFields.has(node.value)) { // filteredFields set already has the subtables + // Child field of a repeat field is not supported + if (node.value.type === 'repeat') throw Problem.user.unsupportedODataSelectField({ property }); + + filteredFields.add(node.value); + node = node.parent; + } + + // Include the children of structure/group + // Note: This doesn't expand 'repeat' fields + if (field.value.type === 'structure') { + getChildren(field).forEach(filteredFields.add, filteredFields); } } } @@ -489,5 +543,11 @@ const filterFields = (formFields, select, table) => { const selectFields = (query, table) => (fields) => (query.$select && query.$select !== '*' ? filterFields(fields, query.$select, table) : fields); -module.exports = { odataXmlError, serviceDocumentFor, edmxFor, rowStreamToOData, singleRowToOData, selectFields, getTableFromOriginalUrl }; +module.exports = { + odataXmlError, serviceDocumentFor, edmxFor, + rowStreamToOData, singleRowToOData, + selectFields, getTableFromOriginalUrl, + // exporting for unit tests + getFieldTree, getChildren +}; diff --git a/test/data/xml.js b/test/data/xml.js index e02d7063c..4df69c23a 100644 --- a/test/data/xml.js +++ b/test/data/xml.js @@ -354,7 +354,109 @@ module.exports = { -` +`, + + groupRepeat: ` + + + groupRepeat + + + + + + +
+ + +
+ + + +
+ + +
+ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + `, + + nestedGroup: ` + + + nestedGroup + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + ` }, instances: { simple: { @@ -464,6 +566,42 @@ module.exports = { John 40 ` + }, + groupRepeat: { + one: ` + xyz + + John +
+ Toronto + Canada +
+ + + Jane +
+ New York + US +
+ + + uuid:2be07915-2c9c-401a-93ea-1c8f3f8e68f6 + + ` + }, + nestedGroup: { + one: ` + xyz + + AKUH + + Yes + + + + uuid:f7908274-ef70-4169-90a0-e1389ab732ff + + ` } } }; diff --git a/test/integration/api/odata.js b/test/integration/api/odata.js index e244c7843..151b893a8 100644 --- a/test/integration/api/odata.js +++ b/test/integration/api/odata.js @@ -1,7 +1,7 @@ const { testService } = require('../setup'); const { sql } = require('slonik'); const testData = require('../../data/xml'); -const { dissocPath } = require('ramda'); +const { dissocPath, identity } = require('ramda'); // NOTE: for the data output tests, we do not attempt to extensively determine if every // internal case is covered; there are already two layers of tests below these, at @@ -1251,6 +1251,56 @@ describe('api: /forms/:id.svc', () => { }); })))); + it('should return toplevel rows with group properties', testService(async (service) => { + const asAlice = await withSubmissions(service, identity); + + await asAlice.get('/v1/projects/1/forms/withrepeat.svc/Submissions?$select=meta') + .expect(200) + .then(({ body }) => { + body.should.eql({ + '@odata.context': 'http://localhost:8989/v1/projects/1/forms/withrepeat.svc/$metadata#Submissions', + value: [ + { meta: { instanceID: 'rthree' } }, + { meta: { instanceID: 'rtwo' } }, + { meta: { instanceID: 'rone' } } + ] + }); + }); + })); + + it('should return toplevel row with nested group properties', testService(async (service) => { + const asAlice = await service.login('alice', identity); + + await asAlice.post('/v1/projects/1/forms?publish=true') + .send(testData.forms.nestedGroup) + .set('Content-Type', 'text/xml') + .expect(200); + + await asAlice.post('/v1/projects/1/forms/nestedGroup/submissions?deviceID=testid') + .send(testData.instances.nestedGroup.one) + .set('Content-Type', 'text/xml') + .expect(200); + + await asAlice.get('/v1/projects/1/forms/nestedGroup.svc/Submissions?$select=text,hospital') + .expect(200) + .then(({ body }) => { + body.should.eql({ + '@odata.context': 'http://localhost:8989/v1/projects/1/forms/nestedGroup.svc/$metadata#Submissions', + value: [ + { + text: 'xyz', + hospital: { + name: 'AKUH', + hiv_medication: { + have_hiv_medication: 'Yes', + }, + }, + }, + ] + }); + }); + })); + it('should return subtable results with selected properties', testService((service) => withSubmissions(service, (asAlice) => asAlice.get('/v1/projects/1/forms/withrepeat.svc/Submissions.children.child?$select=__id,name') @@ -1270,6 +1320,35 @@ describe('api: /forms/:id.svc', () => { }] }); })))); + + it('should return subtable results with group properties', testService(async (service) => { + const asAlice = await service.login('alice', identity); + + await asAlice.post('/v1/projects/1/forms?publish=true') + .send(testData.forms.groupRepeat) + .set('Content-Type', 'text/xml') + .expect(200); + + await asAlice.post('/v1/projects/1/forms/groupRepeat/submissions?deviceID=testid') + .send(testData.instances.groupRepeat.one) + .set('Content-Type', 'text/xml') + .expect(200); + + await asAlice.get('/v1/projects/1/forms/groupRepeat.svc/Submissions.child_repeat?$select=address') + .expect(200) + .then(({ body }) => { + body.should.eql({ + '@odata.context': 'http://localhost:8989/v1/projects/1/forms/groupRepeat.svc/$metadata#Submissions.child_repeat', + value: [ + { address: { city: 'Toronto', country: 'Canada' } }, + { address: { city: 'New York', country: 'US' } } + ] + }); + }); + })); + + + }); describe('/draft.svc', () => { diff --git a/test/unit/formats/odata.js b/test/unit/formats/odata.js index 8de63f3c1..6eb99fdf9 100644 --- a/test/unit/formats/odata.js +++ b/test/unit/formats/odata.js @@ -1,4 +1,5 @@ const appRoot = require('app-root-path'); +const { getFieldTree, getChildren } = require('../../../lib/formats/odata'); const streamTest = require('streamtest').v2; // eslint-disable-next-line import/no-dynamic-require const { serviceDocumentFor, edmxFor, rowStreamToOData, singleRowToOData, selectFields } = require(appRoot + '/lib/formats/odata'); @@ -6,6 +7,7 @@ const { serviceDocumentFor, edmxFor, rowStreamToOData, singleRowToOData, selectF const { fieldsFor, MockField } = require(appRoot + '/test/util/schema'); // eslint-disable-next-line import/no-dynamic-require const testData = require(appRoot + '/test/data/xml'); +const should = require('should'); // Helpers to deal with repeated system metadata generation. const submitter = { id: 5, displayName: 'Alice' }; @@ -1094,9 +1096,9 @@ describe('odata message composition', () => { (() => selectFields({ $select: 'address' }, 'Submissions')(fields)).should.throw('Could not find a property named \'address\''); })); - it('should not throw error if __id is requested for non-submissions table', () => fieldsFor(testData.forms.simple) + it('should not throw error if __id is requested for non-submissions table', () => fieldsFor(testData.forms.withrepeat) .then((fields) => { - (() => selectFields({ $select: '__id' }, 'Submissions.Children')(fields)).should.not.throw(); + (() => selectFields({ $select: '__id' }, 'Submissions.children')(fields)).should.not.throw(); })); it('should not throw error if system properties are requested for submissions table', () => fieldsFor(testData.forms.simple) @@ -1135,5 +1137,52 @@ describe('odata message composition', () => { filteredFields.length.should.equal(2); })); }); + + describe('getFieldTree', () => { + it('should make the tree', () => { + const fields = [ + new MockField({ path: '/home', name: 'home', type: 'structure' }), + new MockField({ path: '/home/address', name: 'address', type: 'structure' }), + new MockField({ path: '/home/address/country', name: 'country', type: 'text' }), + new MockField({ path: '/home/address/province', name: 'province', type: 'text' }), + new MockField({ path: '/home/address/city', name: 'city', type: 'text' }) + ]; + + const tree = getFieldTree(fields); + should(tree['/home'].parent).be.null(); + tree['/home'].children.map(c => c.value.name).should.be.eql(['address']); + + tree['/home/address'].parent.value.name.should.be.eql('home'); + tree['/home/address'].children.map(c => c.value.name).should.be.eql(['country', 'province', 'city']); + + tree['/home/address/country'].parent.value.name.should.be.eql('address'); + tree['/home/address/country'].children.should.be.eql([]); + + tree['/home/address/province'].parent.value.name.should.be.eql('address'); + tree['/home/address/province'].children.should.be.eql([]); + + tree['/home/address/city'].parent.value.name.should.be.eql('address'); + tree['/home/address/city'].children.should.be.eql([]); + }); + }); + + describe('getChildren', () => { + it('should return all children recursively', () => { + const fields = [ + new MockField({ path: '/home', name: 'home', type: 'structure' }), + new MockField({ path: '/home/address', name: 'address', type: 'structure' }), + new MockField({ path: '/home/address/country', name: 'country', type: 'text' }), + new MockField({ path: '/home/address/province', name: 'province', type: 'text' }), + new MockField({ path: '/home/address/city', name: 'city', type: 'text' }) + ]; + + const tree = getFieldTree(fields); + + const children = getChildren(tree['/home']); + + Array.from(children.values()).map(c => c.name).should.be.eql(['address', 'country', 'province', 'city']); + }); + }); + });