From 5296c9afc02a898cadc8c3476a74759285867aef Mon Sep 17 00:00:00 2001 From: Kathleen Tuite Date: Mon, 13 Nov 2023 17:12:35 -0800 Subject: [PATCH] Don't require label in entity update and allow other blank updates (#1035) * Updating tests in advance of code changes * Improve behavior with blank data in entity updates * Add entityVersionNotFound error * updated comment, added tests around submissionXmlToFieldStream * refactor validate entity from submission * Removed validateEntity entirely * Consolidated uuid code * More unit tests for entity parsing/validating * More test coverage --- lib/data/entity.js | 63 +++--- lib/data/submission.js | 18 +- lib/model/frames/entity.js | 20 +- lib/model/query/entities.js | 8 +- lib/util/problem.js | 5 +- test/integration/api/entities.js | 226 ++++++++++++++++++++-- test/integration/worker/entity.js | 1 - test/unit/data/entity.js | 309 +++++++++++++++++++----------- test/unit/data/submission.js | 67 ++++++- 9 files changed, 551 insertions(+), 166 deletions(-) diff --git a/lib/data/entity.js b/lib/data/entity.js index 75c06e689..0814b1b63 100644 --- a/lib/data/entity.js +++ b/lib/data/entity.js @@ -30,30 +30,31 @@ const odataToColumnMap = new Map([ //////////////////////////////////////////////////////////////////////////// // ENTITY PARSING -// After collecting the entity data from the request body or submission, -// we validate the data (properties must include label, and uuid) -// and assemble a valid entity data object. System properties like dataset -// label, uuid are in entity.system while the contents from the bound form -// fields are in enttiy.data. -const validateEntity = (entity) => { - const { label, id, update, baseVersion } = entity.system; - - if (!label || label.trim() === '') - throw Problem.user.missingParameter({ field: 'label' }); - +const normalizeUuid = (id) => { if (!id || id.trim() === '') throw Problem.user.missingParameter({ field: 'uuid' }); const uuidPattern = /^(uuid:)?([0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12})$/i; const matches = uuidPattern.exec(id); - if (matches == null) throw Problem.user.invalidDataTypeOfParameter({ field: 'uuid', expected: 'valid UUID' }); + return matches[2].toLowerCase(); +}; + +// Input: object representing entity, parsed from submission XML +const extractLabelFromSubmission = (entity) => { + const { label, create, update } = entity.system; + if ((create === '1' || create === 'true') && (!label || label.trim() === '')) + throw Problem.user.missingParameter({ field: 'label' }); - const uuid = matches[2].toLowerCase(); + if ((update === '1' || update === 'true') && (!label || label.trim() === '')) + return null; - const newEntity = { ...entity }; - newEntity.system.uuid = uuid; + return label; +}; +// Input: object representing entity, parsed from submission XML +const extractBaseVersionFromSubmission = (entity) => { + const { baseVersion, update } = entity.system; if ((update === '1' || update === 'true')) { if (!baseVersion) throw Problem.user.missingParameter({ field: 'baseVersion' }); @@ -61,15 +62,8 @@ const validateEntity = (entity) => { if (!/^\d+$/.test(baseVersion)) throw Problem.user.invalidDataTypeOfParameter({ field: 'baseVersion', expected: 'integer' }); - newEntity.system.baseVersion = parseInt(baseVersion, 10); - } else { - delete newEntity.system.baseVersion; + return parseInt(entity.system.baseVersion, 10); } - - delete newEntity.system.id; - delete newEntity.system.create; - delete newEntity.system.update; - return newEntity; }; // This works similarly to processing submissions for export, but also note: @@ -78,15 +72,15 @@ const validateEntity = (entity) => { // entity node attributes like dataset name. const parseSubmissionXml = (entityFields, xml) => new Promise((resolve, reject) => { const entity = { system: Object.create(null), data: Object.create(null) }; - const stream = submissionXmlToFieldStream(entityFields, xml, true); + const stream = submissionXmlToFieldStream(entityFields, xml, true, true); stream.on('error', reject); stream.on('data', ({ field, text }) => { if (field.name === 'entity') { entity.system.dataset = field.attrs.dataset; entity.system.id = field.attrs.id; entity.system.create = field.attrs.create; - entity.system.update = field.attrs.update; // TODO: add tests - entity.system.baseVersion = field.attrs.baseVersion; // TODO: add tests + entity.system.update = field.attrs.update; + entity.system.baseVersion = field.attrs.baseVersion; } else if (field.path.indexOf('/meta/entity') === 0) entity.system[field.name] = text; else if (field.propertyName != null) @@ -111,22 +105,26 @@ const extractEntity = (body, propertyNames, existingEntity) => { throw Problem.user.unexpectedAttributes({ expected: bodyKeys, actual: Object.keys(body) }); let entity; + if (existingEntity) { entity = clone(existingEntity); - entity.system.id = entity.system.uuid; - delete entity.system.uuid; } else { entity = { system: Object.create(null), data: Object.create(null) }; if (body.uuid && typeof body.uuid !== 'string') throw Problem.user.invalidDataTypeOfParameter({ field: 'uuid', expected: 'string' }); - entity.system.id = body.uuid; + + entity.system.uuid = normalizeUuid(body.uuid); } if (body.label != null) if (typeof body.label !== 'string') throw Problem.user.invalidDataTypeOfParameter({ field: 'label', expected: 'string' }); + else if (body.label.trim() === '') + throw Problem.user.unexpectedValue({ field: 'label', value: '(empty string)', reason: 'Label cannot be blank.' }); else entity.system.label = body.label; + else if (!existingEntity) + throw Problem.user.missingParameter({ field: 'label' }); if (body.data == null && body.label == null) throw Problem.user.invalidEntity({ reason: 'No entity data or label provided.' }); @@ -141,7 +139,7 @@ const extractEntity = (body, propertyNames, existingEntity) => { throw Problem.user.invalidEntity({ reason: `You specified the dataset property [${k}] which does not exist.` }); } - return validateEntity(entity); + return entity; }; //////////////////////////////////////////////////////////////////////////// @@ -421,8 +419,11 @@ const getWithConflictDetails = (defs, audits, relevantToConflict) => { }; module.exports = { - parseSubmissionXml, validateEntity, + parseSubmissionXml, extractEntity, + normalizeUuid, + extractLabelFromSubmission, + extractBaseVersionFromSubmission, streamEntityCsv, streamEntityCsvAttachment, streamEntityOdata, odataToColumnMap, extractSelectedProperties, selectFields, diff --git a/lib/data/submission.js b/lib/data/submission.js index a35d38148..3619880d1 100644 --- a/lib/data/submission.js +++ b/lib/data/submission.js @@ -21,13 +21,23 @@ const { union, last, pluck } = require('ramda'); // field in the submission. does no processing at all to localize repeat groups, // etc. it's just a stream of found data values. // -// !!! !!! WARNING WARNING: -// right now this facility is used only by generateExpectedAttachments, which wants +// the original place this facility is used is by generateExpectedAttachments, which wants // to navigate the schema stack ignoring gaps so it can just deal w binary fields. +// +// the second place this is used is by parseSubmissionXml for parsing entity data from +// submissions. this scenario is similar to the one above in that we want to navigate to +// specific entity-property fields and the SchemaStack allowEmptyNavigation is true. +// includeStructuralAttrs and includeEmptyNodes are two flags specifically for reaching and +// returning data needed for entities including the element with attributes +// and empty elements like `` meant to blank out certain entity properties. +// +// leaving this old comment here (the second use of this for entities didn't need to do this, +// but the 3rd use might!) --- +// !!! !!! WARNING WARNING: // if you are reading this thinking to use it elsewhere, you'll almost certainly // need to work out a sensible way to flag the SchemaStack allowEmptyNavigation boolean // to false for whatever you are doing. -const submissionXmlToFieldStream = (fields, xml, includeStructuralAttrs = false) => { +const submissionXmlToFieldStream = (fields, xml, includeStructuralAttrs = false, includeEmptyNodes = false) => { const outStream = new Readable({ objectMode: true, read: noop }); const stack = new SchemaStack(fields, true); @@ -50,7 +60,7 @@ const submissionXmlToFieldStream = (fields, xml, includeStructuralAttrs = false) onclosetag: () => { const field = stack.pop(); - if (textBuffer !== '') { + if (textBuffer !== '' || includeEmptyNodes) { if ((field != null) && !field.isStructural()) // don't output useless whitespace outStream.push({ field, text: textBuffer }); textBuffer = ''; diff --git a/lib/model/frames/entity.js b/lib/model/frames/entity.js index e7ae76c27..3963da60c 100644 --- a/lib/model/frames/entity.js +++ b/lib/model/frames/entity.js @@ -10,7 +10,7 @@ /* eslint-disable no-multi-spaces */ const { embedded, Frame, readable, table } = require('../frame'); -const { extractEntity, validateEntity } = require('../../data/entity'); +const { extractEntity, normalizeUuid, extractLabelFromSubmission, extractBaseVersionFromSubmission } = require('../../data/entity'); // These Frames don't interact with APIs directly, hence no readable/writable class Entity extends Frame.define( @@ -26,12 +26,20 @@ class Entity extends Frame.define( get def() { return this.aux.def; } static fromParseEntityData(entityData) { - const validatedEntity = validateEntity(entityData); - const { uuid, label, dataset, baseVersion } = validatedEntity.system; - const { data } = validatedEntity; - const dataReceived = { ...data, label }; + const { dataset } = entityData.system; + const { data } = entityData; + // validation for each field happens within each function + const uuid = normalizeUuid(entityData.system.id); + const label = extractLabelFromSubmission(entityData); + const baseVersion = extractBaseVersionFromSubmission(entityData); + const dataReceived = { ...data, ...(label && { label }) }; return new Entity.Partial({ uuid }, { - def: new Entity.Def({ data, label, dataReceived, ...(baseVersion && { baseVersion }) }), // add baseVersion only if it's there + def: new Entity.Def({ + data, + dataReceived, + ...(baseVersion && { baseVersion }), // add baseVersion only if it's there + ...(label && { label }) // add label only if it's there + }), dataset }); } diff --git a/lib/model/query/entities.js b/lib/model/query/entities.js index 8c64ca026..b1427de0a 100644 --- a/lib/model/query/entities.js +++ b/lib/model/query/entities.js @@ -170,7 +170,7 @@ const _createEntity = (dataset, entityData, submissionId, submissionDef, submiss }); }; -const _updateEntity = (dataset, entityData, submissionId, submissionDef, submissionDefId, event) => async ({ Audits, Entities, one }) => { +const _updateEntity = (dataset, entityData, submissionId, submissionDef, submissionDefId, event) => async ({ Audits, Entities, maybeOne }) => { if (!(event.action === 'submission.create')) // only update on submission.create return null; @@ -187,7 +187,9 @@ const _updateEntity = (dataset, entityData, submissionId, submissionDef, submiss if (clientEntity.def.baseVersion !== serverEntity.aux.currentVersion.version) { const condition = { datasetId: dataset.id, uuid: clientEntity.uuid, version: clientEntity.def.baseVersion }; - const baseEntityVersion = await _getDef(one, new QueryOptions({ condition })); // eslint-disable-line no-use-before-define + // eslint-disable-next-line no-use-before-define + const baseEntityVersion = (await _getDef(maybeOne, new QueryOptions({ condition }))) + .orThrow(Problem.user.entityVersionNotFound({ baseVersion: clientEntity.def.baseVersion, entityUuid: clientEntity.uuid, datasetName: dataset.name })); // we need to find what changed between baseVersion and lastVersion // it is not the data we received in lastVersion @@ -205,7 +207,7 @@ const _updateEntity = (dataset, entityData, submissionId, submissionDef, submiss // merge data const mergedData = mergeRight(serverEntity.aux.currentVersion.data, clientEntity.def.data); - const mergedLabel = clientEntity.def.label || serverEntity.aux.currentVersion.label; + const mergedLabel = clientEntity.def.label ?? serverEntity.aux.currentVersion.label; // make some kind of source object const sourceDetails = { diff --git a/lib/util/problem.js b/lib/util/problem.js index ba58ff140..b887ca0b0 100644 --- a/lib/util/problem.js +++ b/lib/util/problem.js @@ -157,9 +157,12 @@ const problems = { // dataset in submission does not exist datasetNotFound: problem(404.7, ({ datasetName }) => `The dataset (${datasetName}) specified in the submission does not exist.`), - // entity in submission does not exist + // entity specified in submission does not exist entityNotFound: problem(404.8, ({ entityUuid, datasetName }) => `The entity with UUID (${entityUuid}) specified in the submission does not exist in the dataset (${datasetName}).`), + // entity base version specified in submission does not exist + entityVersionNotFound: problem(404.9, ({ baseVersion, entityUuid, datasetName }) => `Base version (${baseVersion}) does not exist for entity UUID (${entityUuid}) in dataset (${datasetName}).`), + // { allowed: [ acceptable formats ], got } unacceptableFormat: problem(406.1, ({ allowed }) => `Requested format not acceptable; this resource allows: ${allowed.join()}.`), diff --git a/test/integration/api/entities.js b/test/integration/api/entities.js index 53add018a..b0b524650 100644 --- a/test/integration/api/entities.js +++ b/test/integration/api/entities.js @@ -1130,6 +1130,51 @@ describe('Entities API', () => { }); })); + it('should reject if uuid is not a valid uuid', testDataset(async (service) => { + const asAlice = await service.login('alice'); + + await asAlice.post('/v1/projects/1/datasets/people/entities') + .send({ + uuid: 'bad_uuidv4', + label: 'Johnny Doe', + data: { + first_name: 'Johnny', + age: '22' + } + }) + .expect(400); + })); + + it('should reject if label is blank', testEntities(async (service) => { + const asAlice = await service.login('alice'); + + await asAlice.post('/v1/projects/1/datasets/people/entities') + .send({ + uuid: '12345678-1234-4123-8234-123456789abc', + label: '', + data: { + first_name: 'Johnny', + age: '22' + } + }) + .expect(400); + })); + + + it('should reject if label is not provided', testEntities(async (service) => { + const asAlice = await service.login('alice'); + + await asAlice.post('/v1/projects/1/datasets/people/entities') + .send({ + uuid: '12345678-1234-4123-8234-123456789abc', + data: { + first_name: 'Johnny', + age: '22' + } + }) + .expect(400); + })); + it('should reject if uuid is not unique', testEntities(async (service) => { // Use testEntities here vs. testDataset to prepopulate with 2 entities const asAlice = await service.login('alice'); @@ -1317,6 +1362,7 @@ describe('Entities API', () => { }) .expect(200) .then(({ body: person }) => { + person.currentVersion.dataReceived.should.eql({ age: '77', first_name: 'Alan' }); person.currentVersion.should.have.property('data').which.is.eql(newData); // label hasn't been updated person.currentVersion.should.have.property('label').which.is.equal('Alice (88)'); @@ -1326,6 +1372,7 @@ describe('Entities API', () => { await asAlice.get('/v1/projects/1/datasets/people/entities/12345678-1234-4123-8234-123456789abc') .expect(200) .then(({ body: person }) => { + person.currentVersion.dataReceived.should.eql({ age: '77', first_name: 'Alan' }); person.currentVersion.should.have.property('data').which.is.eql(newData); person.currentVersion.should.have.property('label').which.is.equal('Alice (88)'); }); @@ -1360,6 +1407,7 @@ describe('Entities API', () => { .then(({ body: person }) => { person.currentVersion.should.have.property('data').which.is.eql(newData); person.currentVersion.should.have.property('label').which.is.equal('Arnold (66)'); + person.currentVersion.dataReceived.should.eql({ label: 'Arnold (66)' }); }); })); @@ -1381,6 +1429,7 @@ describe('Entities API', () => { .then(({ body: person }) => { person.currentVersion.should.have.property('label').which.is.eql('New Label'); person.currentVersion.dataReceived.should.have.property('label').which.is.eql('New Label'); + person.currentVersion.dataReceived.should.eql({ label: 'New Label' }); }); })); @@ -1420,6 +1469,7 @@ describe('Entities API', () => { .expect(200) .then(({ body: person }) => { person.currentVersion.should.have.property('data').which.is.eql(newData); + person.currentVersion.dataReceived.should.eql({ city: 'Toronto' }); }); })); @@ -1441,6 +1491,7 @@ describe('Entities API', () => { .expect(200) .then(({ body: person }) => { person.currentVersion.should.have.property('data').which.is.eql(newData); + person.currentVersion.dataReceived.should.eql({ first_name: '' }); }); })); @@ -1834,6 +1885,52 @@ describe('Entities API', () => { }); + describe('create new entities from submissions', () => { + // More success and error cases in test/integration/worker/entity.js + it('should create entity', testEntityUpdates(async (service, container) => { + const asAlice = await service.login('alice'); + + await asAlice.post('/v1/projects/1/forms/simpleEntity/submissions') + .send(testData.instances.simpleEntity.three) + .set('Content-Type', 'application/xml') + .expect(200); + + await exhaust(container); + + await asAlice.get('/v1/projects/1/datasets/people/entities/12345678-1234-4123-8234-123456789bbb') + .expect(200) + .then(({ body: person }) => { + person.currentVersion.label.should.equal('John (40)'); + person.currentVersion.data.should.eql({ age: '40', first_name: 'John' }); + person.currentVersion.version.should.equal(1); + }); + })); + + it('should not create entity if the label is missing in the submission', testEntityUpdates(async (service, container) => { + const asAlice = await service.login('alice'); + + await asAlice.post('/v1/projects/1/forms/simpleEntity/submissions') + .send(testData.instances.simpleEntity.three + .replace('John (40)', '')) + .set('Content-Type', 'application/xml') + .expect(200); + + await exhaust(container); + + await asAlice.get('/v1/projects/1/datasets/people/entities/12345678-1234-4123-8234-123456789bbb') + .expect(404); + + await asAlice.get('/v1/projects/1/forms/simpleEntity/submissions/three/audits') + .expect(200) + .then(({ body: logs }) => { + logs[0].should.be.an.Audit(); + logs[0].action.should.be.eql('entity.error'); + logs[0].details.problem.problemCode.should.equal(400.2); + logs[0].details.errorMessage.should.equal('Required parameter label missing.'); + }); + })); + }); + describe('entity updates from submissions', () => { it('should process multiple updates in a row', testEntityUpdates(async (service, container) => { const asAlice = await service.login('alice'); @@ -1865,12 +1962,11 @@ describe('Entities API', () => { }); })); - it('should update label', testEntityUpdates(async (service, container) => { + it('should update data and label', testEntityUpdates(async (service, container) => { const asAlice = await service.login('alice'); await asAlice.post('/v1/projects/1/forms/updateEntity/submissions') - .send(testData.instances.updateEntity.one - .replace('', '')) + .send(testData.instances.updateEntity.one) .expect(200); await exhaust(container); @@ -1878,18 +1974,19 @@ describe('Entities API', () => { await asAlice.get('/v1/projects/1/datasets/people/entities/12345678-1234-4123-8234-123456789abc') .expect(200) .then(({ body: person }) => { - person.currentVersion.label.should.equal('new label'); + person.currentVersion.dataReceived.should.eql({ age: '85', first_name: 'Alicia', label: 'Alicia (85)' }); + person.currentVersion.label.should.equal('Alicia (85)'); person.currentVersion.data.should.eql({ age: '85', first_name: 'Alicia' }); }); })); - it.skip('should set label to blank', testEntityUpdates(async (service, container) => { - // TODO: fix the entity label update logic to make this test pass. + it('should update only the label', testEntityUpdates(async (service, container) => { const asAlice = await service.login('alice'); await asAlice.post('/v1/projects/1/forms/updateEntity/submissions') .send(testData.instances.updateEntity.one - .replace('', '')) + .replace('Alicia', '') + .replace('85', '')) .expect(200); await exhaust(container); @@ -1897,7 +1994,9 @@ describe('Entities API', () => { await asAlice.get('/v1/projects/1/datasets/people/entities/12345678-1234-4123-8234-123456789abc') .expect(200) .then(({ body: person }) => { - person.currentVersion.label.should.equal(''); + person.currentVersion.dataReceived.should.eql({ label: 'Alicia (85)' }); + person.currentVersion.label.should.equal('Alicia (85)'); + person.currentVersion.data.should.eql({ age: '22', first_name: 'Johnny' }); }); })); @@ -1914,12 +2013,32 @@ describe('Entities API', () => { await asAlice.get('/v1/projects/1/datasets/people/entities/12345678-1234-4123-8234-123456789abc') .expect(200) .then(({ body: person }) => { + person.currentVersion.dataReceived.should.eql({ age: '85', first_name: 'Alicia' }); + person.currentVersion.data.should.eql({ age: '85', first_name: 'Alicia' }); + person.currentVersion.label.should.equal('Johnny Doe'); + }); + })); + + it('should not update label if label is blank', testEntityUpdates(async (service, container) => { + const asAlice = await service.login('alice'); + + await asAlice.post('/v1/projects/1/forms/updateEntity/submissions') + .send(testData.instances.updateEntity.one + .replace('', '')) + .expect(200); + + await exhaust(container); + + await asAlice.get('/v1/projects/1/datasets/people/entities/12345678-1234-4123-8234-123456789abc') + .expect(200) + .then(({ body: person }) => { + person.currentVersion.dataReceived.should.eql({ age: '85', first_name: 'Alicia' }); + person.currentVersion.data.should.eql({ age: '85', first_name: 'Alicia' }); person.currentVersion.label.should.equal('Johnny Doe'); }); })); - it.skip('should set field to blank', testEntityUpdates(async (service, container) => { - // TODO: fix update logic to make this test pass + it('should set field to blank', testEntityUpdates(async (service, container) => { const asAlice = await service.login('alice'); await asAlice.post('/v1/projects/1/forms/updateEntity/submissions') @@ -1932,6 +2051,8 @@ describe('Entities API', () => { await asAlice.get('/v1/projects/1/datasets/people/entities/12345678-1234-4123-8234-123456789abc') .expect(200) .then(({ body: person }) => { + person.currentVersion.dataReceived.should.eql({ label: 'Alicia (85)', first_name: 'Alicia', age: '' }); + person.currentVersion.data.first_name.should.eql('Alicia'); person.currentVersion.data.age.should.eql(''); }); })); @@ -1941,7 +2062,6 @@ describe('Entities API', () => { await asAlice.post('/v1/projects/1/forms/updateEntity/submissions') .send(testData.instances.updateEntity.one - .replace('85', '22') .replace('Alicia', '')) // original first_name in entity is Johnny .expect(200); @@ -1950,8 +2070,90 @@ describe('Entities API', () => { await asAlice.get('/v1/projects/1/datasets/people/entities/12345678-1234-4123-8234-123456789abc') .expect(200) .then(({ body: person }) => { - person.currentVersion.data.should.eql({ age: '22', first_name: 'Johnny' }); + person.currentVersion.dataReceived.should.eql({ label: 'Alicia (85)', age: '85' }); + person.currentVersion.data.first_name.should.equal('Johnny'); // original first name + person.currentVersion.data.should.eql({ age: '85', first_name: 'Johnny' }); + person.currentVersion.label.should.equal('Alicia (85)'); }); })); + + describe('update error cases', () => { + // more checks of errors are found in worker/entity tests + it('should log an error and not update if baseVersion is missing', testEntityUpdates(async (service, container) => { + const asAlice = await service.login('alice'); + + await asAlice.post('/v1/projects/1/forms/updateEntity/submissions') + .send(testData.instances.updateEntity.one + .replace('baseVersion="1"', '')) + .expect(200); + + await exhaust(container); + + await asAlice.get('/v1/projects/1/datasets/people/entities/12345678-1234-4123-8234-123456789abc') + .expect(200) + .then(({ body: person }) => { + should(person.currentVersion.baseVersion).be.null(); + }); + + await asAlice.get('/v1/projects/1/forms/updateEntity/submissions/one/audits') + .expect(200) + .then(({ body: logs }) => { + logs[0].should.be.an.Audit(); + logs[0].action.should.be.eql('entity.error'); + }); + })); + + it('should log an error and not update if baseVersion in submission does not exist', testEntityUpdates(async (service, container) => { + const asAlice = await service.login('alice'); + + await asAlice.post('/v1/projects/1/forms/updateEntity/submissions') + .send(testData.instances.updateEntity.one + .replace('baseVersion="1"', 'baseVersion="2"')) + .expect(200); + + await exhaust(container); + + await asAlice.get('/v1/projects/1/datasets/people/entities/12345678-1234-4123-8234-123456789abc') + .expect(200) + .then(({ body: person }) => { + should(person.currentVersion.baseVersion).be.null(); + }); + + await asAlice.get('/v1/projects/1/forms/updateEntity/submissions/one/audits') + .expect(200) + .then(({ body: logs }) => { + logs[0].should.be.an.Audit(); + logs[0].action.should.be.eql('entity.error'); + logs[0].details.problem.problemCode.should.equal(404.9); + logs[0].details.errorMessage.should.equal('Base version (2) does not exist for entity UUID (12345678-1234-4123-8234-123456789abc) in dataset (people).'); + }); + })); + + it('should log an error and if baseVersion is not an integer', testEntityUpdates(async (service, container) => { + const asAlice = await service.login('alice'); + + await asAlice.post('/v1/projects/1/forms/updateEntity/submissions') + .send(testData.instances.updateEntity.one + .replace('baseVersion="1"', 'baseVersion="not_an_integer"')) + .expect(200); + + await exhaust(container); + + await asAlice.get('/v1/projects/1/datasets/people/entities/12345678-1234-4123-8234-123456789abc') + .expect(200) + .then(({ body: person }) => { + person.currentVersion.version.should.equal(1); + }); + + await asAlice.get('/v1/projects/1/forms/updateEntity/submissions/one/audits') + .expect(200) + .then(({ body: logs }) => { + logs[0].should.be.an.Audit(); + logs[0].action.should.be.eql('entity.error'); + logs[0].details.problem.problemCode.should.equal(400.11); + logs[0].details.errorMessage.should.equal('Invalid input data type: expected (baseVersion) to be (integer)'); + }); + })); + }); }); }); diff --git a/test/integration/worker/entity.js b/test/integration/worker/entity.js index 2be29e270..e556c86e4 100644 --- a/test/integration/worker/entity.js +++ b/test/integration/worker/entity.js @@ -454,7 +454,6 @@ describe('worker: entity', () => { }); describe('should catch problems updating entity', () => { - // TODO: these errors are getting logged as entity.error audit events describe('validation errors', () => { it('should fail because UUID is invalid', testService(async (service, container) => { const asAlice = await service.login('alice'); diff --git a/test/unit/data/entity.js b/test/unit/data/entity.js index 9f5a1385a..b83c22477 100644 --- a/test/unit/data/entity.js +++ b/test/unit/data/entity.js @@ -3,100 +3,145 @@ const appRoot = require('app-root-path'); const assert = require('assert'); const { ConflictType } = require('../../../lib/data/entity'); const { Entity } = require('../../../lib/model/frames'); -const { parseSubmissionXml, extractEntity, validateEntity, extractSelectedProperties, selectFields, diffEntityData, getDiffProp, getWithConflictDetails } = require(appRoot + '/lib/data/entity'); +const { normalizeUuid, extractLabelFromSubmission, extractBaseVersionFromSubmission, parseSubmissionXml, extractEntity, extractSelectedProperties, selectFields, diffEntityData, getDiffProp, getWithConflictDetails } = require(appRoot + '/lib/data/entity'); const { fieldsFor } = require(appRoot + '/test/util/schema'); const testData = require(appRoot + '/test/data/xml'); describe('extracting and validating entities', () => { - describe('validateEntity', () => { - it('should throw errors on when label is missing', () => { - const entity = { - system: { - id: '12345678-1234-4123-8234-123456789abc', - dataset: 'foo', - }, - data: {} - }; - assert.throws(() => { validateEntity(entity); }, (err) => { - err.problemCode.should.equal(400.2); - err.message.should.equal('Required parameter label missing.'); - return true; - }); - }); + describe('helper functions', () => { + describe('normalizeUuid', () => { + it('should return uuid in standard v4 format', () => + normalizeUuid('12345678-1234-4123-8234-123456789abc').should.equal('12345678-1234-4123-8234-123456789abc')); - it('should throw errors when id is missing', () => { - (() => validateEntity({ - system: { - label: 'foo', - id: ' ', - dataset: 'foo', - }, - data: {} - })).should.throw(/Required parameter uuid missing/); - }); + it('should return lowercase uuid', () => + normalizeUuid('12345678-1234-4123-8234-123456789ABC').should.equal('12345678-1234-4123-8234-123456789abc')); - it('should throw errors when id is not a valid uuid', () => { - (() => validateEntity({ - system: { - label: 'foo', - id: 'uuid:12123123', - dataset: 'foo', - }, - data: {} - })).should.throw(/Invalid input data type: expected \(uuid\) to be \(valid UUID\)/); - }); + it('should return uuid with uuid: prefix stripped', () => + normalizeUuid('uuid:12345678-1234-4123-8234-123456789abc').should.equal('12345678-1234-4123-8234-123456789abc')); - it('should remove create property from system', () => { - const entity = { - system: { - create: '1', - id: '12345678-1234-4123-8234-123456789abc', - label: 'foo', - dataset: 'foo', - }, - data: {} - }; - validateEntity(entity).system.should.not.have.property('create'); + it('should return uuid with uupercase UUID: prefix stripped', () => + normalizeUuid('UUID:12345678-1234-4123-8234-123456789abc').should.equal('12345678-1234-4123-8234-123456789abc')); + + it('should return problem if null passed in as arg', () => + assert.throws(() => { normalizeUuid(null); }, (err) => { + err.problemCode.should.equal(400.2); + err.message.should.equal('Required parameter uuid missing.'); + return true; + })); + + it('should return problem if undefined passed in as arg', () => + assert.throws(() => { normalizeUuid(undefined); }, (err) => { + err.problemCode.should.equal(400.2); + err.message.should.equal('Required parameter uuid missing.'); + return true; + })); + + it('should return problem if invalid uuid passed in', () => + assert.throws(() => { normalizeUuid('this_is_not_a_valid_uuid'); }, (err) => { + err.problemCode.should.equal(400.11); + err.message.should.equal('Invalid input data type: expected (uuid) to be (valid UUID)'); + return true; + })); }); - it('should id property with uuid and remove uuid: prefix from the value', () => { - const entity = { - system: { - id: '12345678-1234-4123-8234-123456789abc', - label: 'foo', - dataset: 'foo', - }, - data: {} - }; - const validatedEntity = validateEntity(entity); + describe('extractLabelFromSubmission', () => { + it('should return label when creating new entity (create = 1)', () => { + const entity = { system: { create: '1', label: 'the_label' } }; + extractLabelFromSubmission(entity).should.equal('the_label'); + }); + + it('should return label when creating new entity (create = true)', () => { + const entity = { system: { create: 'true', label: 'the_label' } }; + extractLabelFromSubmission(entity).should.equal('the_label'); + }); + + it('should complain if label is missing when creating entity', () => { + const entity = { system: { create: '1' } }; + assert.throws(() => { extractLabelFromSubmission(entity); }, (err) => { + err.problemCode.should.equal(400.2); + err.message.should.equal('Required parameter label missing.'); + return true; + }); + }); + + it('should complain if label is empty when creating entity', () => { + const entity = { system: { create: '1', label: '' } }; + assert.throws(() => { extractLabelFromSubmission(entity); }, (err) => { + err.problemCode.should.equal(400.2); + err.message.should.equal('Required parameter label missing.'); + return true; + }); + }); + + it('should return null for label if updating and label not provided', () => { + const entity = { system: { update: '1', } }; + should.not.exist(extractLabelFromSubmission(entity)); + }); - validatedEntity.system.should.not.have.property('id'); - validatedEntity.system.should.have.property('uuid', '12345678-1234-4123-8234-123456789abc'); + it('should return null for label if updating and label is empty', () => { + const entity = { system: { update: '1', label: '' } }; + should.not.exist(extractLabelFromSubmission(entity)); + }); + + // The 3 following cases shouldn't come up + it('should return label when neither create nor update is specified', () => { + const entity = { system: { unknown_action: 'true', label: 'the_label' } }; + extractLabelFromSubmission(entity).should.equal('the_label'); + }); + + it('should return empty label when neither create nor update is specified', () => { + const entity = { system: { unknown_action: 'true', label: '' } }; + extractLabelFromSubmission(entity).should.equal(''); + }); + + it('should return null when label is null label when neither create nor update is specified', () => { + const entity = { system: { unknown_action: 'true' } }; + should.not.exist(extractLabelFromSubmission(entity)); + }); }); - it('should throw error when baseVersion for update is missing', () => { - (() => validateEntity({ - system: { - id: '12345678-1234-4123-8234-123456789abc', - label: 'foo', - dataset: 'foo', - update: '1' - }, - data: {} - })).should.throw(/Required parameter baseVersion missing/); - }); - - it('should throw error when baseVersion is not an integer', () => { - (() => validateEntity({ - system: { - id: '12345678-1234-4123-8234-123456789abc', - label: 'foo', - dataset: 'foo', - update: '1', - baseVersion: 'a' - }, - data: {} - })).should.throw('Invalid input data type: expected (baseVersion) to be (integer)'); + describe('extractBaseVersionFromSubmission', () => { + it('should extract integer base version when update is true', () => { + const entity = { system: { update: '1', baseVersion: '99' } }; + extractBaseVersionFromSubmission(entity).should.equal(99); + }); + + it('not return base version if create is true because it is not relevant', () => { + const entity = { system: { create: '1', baseVersion: '99' } }; + should.not.exist(extractBaseVersionFromSubmission(entity)); + }); + + it('not return base version if neither create nor update are provided', () => { + const entity = { system: { baseVersion: '99' } }; + should.not.exist(extractBaseVersionFromSubmission(entity)); + }); + + it('should complain if baseVersion is missing when update is true (update = 1)', () => { + const entity = { system: { update: '1' } }; + assert.throws(() => { extractBaseVersionFromSubmission(entity); }, (err) => { + err.problemCode.should.equal(400.2); + err.message.should.equal('Required parameter baseVersion missing.'); + return true; + }); + }); + + it('should complain if baseVersion is missing when update is true (update = true)', () => { + const entity = { system: { update: 'true' } }; + assert.throws(() => { extractBaseVersionFromSubmission(entity); }, (err) => { + err.problemCode.should.equal(400.2); + err.message.should.equal('Required parameter baseVersion missing.'); + return true; + }); + }); + + it('should complain if baseVersion not an integer', () => { + const entity = { system: { update: '1', baseVersion: 'ten' } }; + assert.throws(() => { extractBaseVersionFromSubmission(entity); }, (err) => { + err.problemCode.should.equal(400.11); + err.message.should.equal('Invalid input data type: expected (baseVersion) to be (integer)'); + return true; + }); + }); }); }); @@ -274,7 +319,34 @@ describe('extracting and validating entities', () => { }); }); - it('should reject if required part of the request is missing or not a string', () => { + it('should reject if label is blank', () => { + const body = { + uuid: '12345678-1234-4123-8234-123456789abc', + label: '', + data: { age: '88' } + }; + const propertyNames = ['age']; + assert.throws(() => { extractEntity(body, propertyNames); }, (err) => { + err.problemCode.should.equal(400.8); + err.message.should.equal('Unexpected label value (empty string); Label cannot be blank.'); + return true; + }); + }); + + it('should reject if label is missing AND in create case (no existingEntity)', () => { + const body = { + uuid: '12345678-1234-4123-8234-123456789abc', + data: { age: '88' } + }; + const propertyNames = ['age']; + assert.throws(() => { extractEntity(body, propertyNames); }, (err) => { + err.problemCode.should.equal(400.2); + err.message.should.equal('Required parameter label missing.'); + return true; + }); + }); + + it('should reject if required part of the request is null or not a string in create', () => { // These are JSON entity validation errors so they use a newer 400 bad request problem const requests = [ [ @@ -282,11 +354,6 @@ describe('extracting and validating entities', () => { 400.11, 'Invalid input data type: expected (label) to be (string)' ], - [ - { uuid: '12345678-1234-4123-8234-123456789abc' }, - 400.28, - 'The entity is invalid. No entity data or label provided.' - ], [ { uuid: '12345678-1234-4123-8234-123456789abc', label: 'Label', data: { first_name: 'Alice', age: 99 } }, 400.11, @@ -338,6 +405,28 @@ describe('extracting and validating entities', () => { }); }); + it('should allow updating properties not included in earlier version of entity', () => { + const existingEntity = { + system: { + uuid: '12345678-1234-4123-8234-123456789abc', + label: 'Label', + }, + data: { first_name: 'Alice' } + }; + const newData = { + data: { age: '99' } + }; + const propertyNames = ['age', 'first_name']; + const entity = extractEntity(newData, propertyNames, existingEntity); + should(entity).eql({ + system: { + label: 'Label', + uuid: '12345678-1234-4123-8234-123456789abc' + }, + data: { age: '99', first_name: 'Alice' } + }); + }); + it('should allow only label to be updated without changing data', () => { const existingEntity = { system: { @@ -360,29 +449,39 @@ describe('extracting and validating entities', () => { }); }); - it('should allow updating properties not included in earlier version of entity', () => { - const existingEntity = { - system: { - uuid: '12345678-1234-4123-8234-123456789abc', - label: 'Label', - }, - data: { first_name: 'Alice' } - }; - const newData = { - data: { age: '99' } + it('should allow label to be missing and use label of existing entity', () => { + const existingEntity = { system: { uuid: '12345678-1234-4123-8234-123456789abc', label: 'previous_label' }, data: {} }; + const body = { + uuid: '12345678-1234-4123-8234-123456789abc', + data: { age: '88' } }; - const propertyNames = ['age', 'first_name']; - const entity = extractEntity(newData, propertyNames, existingEntity); + const propertyNames = ['age']; + const entity = extractEntity(body, propertyNames, existingEntity); should(entity).eql({ system: { - label: 'Label', + label: 'previous_label', uuid: '12345678-1234-4123-8234-123456789abc' }, - data: { age: '99', first_name: 'Alice' } + data: { age: '88' } + }); + }); + + it('should reject if blank label provided in update', () => { + const existingEntity = { system: { uuid: '12345678-1234-4123-8234-123456789abc', label: 'previous_label' }, data: {} }; + const body = { + uuid: '12345678-1234-4123-8234-123456789abc', + label: '', + data: { age: '88' } + }; + const propertyNames = ['age']; + assert.throws(() => { extractEntity(body, propertyNames, existingEntity); }, (err) => { + err.problemCode.should.equal(400.8); + err.message.should.match('Unexpected label value (empty string); Label cannot be blank.'); + return true; }); }); - it('should reject if required part of the request is missing or not a string', () => { + it('should reject if required part of the request is missing or not a string in update', () => { const requests = [ [ {}, @@ -392,10 +491,6 @@ describe('extracting and validating entities', () => { { label: null }, 400.28, 'The entity is invalid. No entity data or label provided.' ], - [ - { label: '' }, - 400.2, 'Required parameter label missing.' - ], [ { data: { first_name: 'Alice', age: 99 } }, 400.11, 'Invalid input data type: expected (age) to be (string)' diff --git a/test/unit/data/submission.js b/test/unit/data/submission.js index 210297407..449894508 100644 --- a/test/unit/data/submission.js +++ b/test/unit/data/submission.js @@ -1,4 +1,4 @@ -require('should'); +const should = require('should'); const appRoot = require('app-root-path'); const { filter } = require('ramda'); const { toObjects } = require('streamtest').v2; @@ -57,6 +57,71 @@ describe('submission field streamer', () => { stream.on('end', done); // not hanging/timing out is the assertion here }); }); + + describe('entity flags includeStructuralAttrs and includeEmptyNodes', () => { + beforeEach(() => { + should.config.checkProtoEql = false; + }); + afterEach(() => { + should.config.checkProtoEql = true; + }); + + it('should include structural fields', (done) => { + fieldsFor(testData.forms.simpleEntity).then((fields) => + submissionXmlToFieldStream(fields, testData.instances.simpleEntity.one, true).pipe(toObjects((error, result) => { + result.should.eql([ + { field: new MockField({ order: 4, name: 'entity', path: '/meta/entity', type: 'structure', attrs: { + create: '1', + dataset: 'people', + id: 'uuid:12345678-1234-4123-8234-123456789abc' + } }), text: null }, + { field: new MockField({ order: 5, name: 'label', path: '/meta/entity/label', type: 'unknown' }), text: 'Alice (88)' }, + { field: new MockField({ order: 0, name: 'name', path: '/name', type: 'string', propertyName: 'first_name' }), text: 'Alice' }, + { field: new MockField({ order: 1, name: 'age', path: '/age', type: 'int', propertyName: 'age' }), text: '88' }, + { field: new MockField({ order: 2, name: 'hometown', path: '/hometown', type: 'string' }), text: 'Chicago' } + ]); + done(); + }))); + }); + + it('should not include structural fields', (done) => { + fieldsFor(testData.forms.simpleEntity).then((fields) => + submissionXmlToFieldStream(fields, testData.instances.simpleEntity.one, false).pipe(toObjects((error, result) => { + result.should.eql([ + { field: new MockField({ order: 5, name: 'label', path: '/meta/entity/label', type: 'unknown' }), text: 'Alice (88)' }, + { field: new MockField({ order: 0, name: 'name', path: '/name', type: 'string', propertyName: 'first_name' }), text: 'Alice' }, + { field: new MockField({ order: 1, name: 'age', path: '/age', type: 'int', propertyName: 'age' }), text: '88' }, + { field: new MockField({ order: 2, name: 'hometown', path: '/hometown', type: 'string' }), text: 'Chicago' } + ]); + done(); + }))); + }); + + it('should include empty nodes if specified in fields', (done) => { + fieldsFor(testData.forms.simpleEntity).then((fields) => + submissionXmlToFieldStream(fields, testData.instances.simpleEntity.one.replace('88', ''), false, true).pipe(toObjects((error, result) => { + result.should.eql([ + { field: new MockField({ order: 5, name: 'label', path: '/meta/entity/label', type: 'unknown' }), text: 'Alice (88)' }, + { field: new MockField({ order: 0, name: 'name', path: '/name', type: 'string', propertyName: 'first_name' }), text: 'Alice' }, + { field: new MockField({ order: 1, name: 'age', path: '/age', type: 'int', propertyName: 'age' }), text: '' }, + { field: new MockField({ order: 2, name: 'hometown', path: '/hometown', type: 'string' }), text: 'Chicago' } + ]); + done(); + }))); + }); + + it('should not include empty nodes', (done) => { + fieldsFor(testData.forms.simpleEntity).then((fields) => + submissionXmlToFieldStream(fields, testData.instances.simpleEntity.one.replace('88', ''), false, false).pipe(toObjects((error, result) => { + result.should.eql([ + { field: new MockField({ order: 5, name: 'label', path: '/meta/entity/label', type: 'unknown' }), text: 'Alice (88)' }, + { field: new MockField({ order: 0, name: 'name', path: '/name', type: 'string', propertyName: 'first_name' }), text: 'Alice' }, + { field: new MockField({ order: 2, name: 'hometown', path: '/hometown', type: 'string' }), text: 'Chicago' } + ]); + done(); + }))); + }); + }); }); describe('getSelectMultipleResponses', () => {