diff --git a/lib/data/entity.js b/lib/data/entity.js index 3be7d9932..4acdf0525 100644 --- a/lib/data/entity.js +++ b/lib/data/entity.js @@ -19,7 +19,7 @@ const { PartialPipe } = require('../util/stream'); const Problem = require('../util/problem'); const { submissionXmlToFieldStream } = require('./submission'); const { nextUrlFor, getServiceRoot, jsonDataFooter, extractPaging } = require('../util/odata'); -const { sanitizeOdataIdentifier, blankStringToNull } = require('../util/util'); +const { sanitizeOdataIdentifier, blankStringToNull, isBlank } = require('../util/util'); const odataToColumnMap = new Map([ ['__system/createdAt', 'entities.createdAt'], @@ -55,17 +55,19 @@ const extractLabelFromSubmission = (entity, options = { create: true }) => { }; // Input: object representing entity, parsed from submission XML -const extractBaseVersionFromSubmission = (entity) => { - const { baseVersion, update } = entity.system; - if ((update === '1' || update === 'true')) { - if (!baseVersion) +const extractBaseVersionFromSubmission = (entity, options = { update: true }) => { + const { update = false } = options; + const { baseVersion } = entity.system; + if (isBlank(baseVersion)) { + if (update) throw Problem.user.missingParameter({ field: 'baseVersion' }); + return null; + } - if (!/^\d+$/.test(baseVersion)) - throw Problem.user.invalidDataTypeOfParameter({ field: 'baseVersion', expected: 'integer' }); + if (!/^\d+$/.test(baseVersion)) + throw Problem.user.invalidDataTypeOfParameter({ field: 'baseVersion', expected: 'integer' }); - return parseInt(entity.system.baseVersion, 10); - } + return parseInt(entity.system.baseVersion, 10); }; const extractBranchIdFromSubmission = (entity) => { diff --git a/lib/model/frames/entity.js b/lib/model/frames/entity.js index 7522193d7..42359f47d 100644 --- a/lib/model/frames/entity.js +++ b/lib/model/frames/entity.js @@ -39,7 +39,7 @@ class Entity extends Frame.define( // validation for each field happens within each function const uuid = normalizeUuid(entityData.system.id); const label = extractLabelFromSubmission(entityData, options); - const baseVersion = extractBaseVersionFromSubmission(entityData); + const baseVersion = extractBaseVersionFromSubmission(entityData, options); const branchId = extractBranchIdFromSubmission(entityData); const trunkVersion = extractTrunkVersionFromSubmission(entityData); const dataReceived = { ...data, ...(label && { label }) }; diff --git a/lib/model/query/entities.js b/lib/model/query/entities.js index 7ea5326ae..59c1a37bb 100644 --- a/lib/model/query/entities.js +++ b/lib/model/query/entities.js @@ -486,7 +486,11 @@ const _processSubmissionEvent = (event, parentEvent) => async ({ Audits, Dataset maybeEntity = await Entities._updateEntity(dataset, entityData, submissionId, submissionDef, submissionDefId, event, forceOutOfOrderProcessing); } catch (err) { const attemptCreate = (entityData.system.create === '1' || entityData.system.create === 'true') || forceOutOfOrderProcessing; - if ((err.problemCode === 404.8) && attemptCreate) { + // The two types of errors for which we attempt to create after a failed update: + // 1. entity not found + // 2. baseVersion is empty and failed to parse in the update case + // This second one is a special case related to issue c#727 + if ((err.problemCode === 404.8 || (err.problemCode === 400.2 && err.problemDetails.field === 'baseVersion')) && attemptCreate) { maybeEntity = await Entities._createEntity(dataset, entityData, submissionId, submissionDef, submissionDefId, event, parentEvent, forceOutOfOrderProcessing); } else { throw (err); diff --git a/test/integration/api/entities.js b/test/integration/api/entities.js index 91c4268de..59779a387 100644 --- a/test/integration/api/entities.js +++ b/test/integration/api/entities.js @@ -3238,6 +3238,53 @@ describe('Entities API', () => { logs[1].action.should.be.eql('submission.create'); }); })); + + it('should create entity when both create and update are true but baseVersion is empty', 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('id="12345678-1234-4123-8234-123456789abc"', 'id="12345678-1234-4123-8234-123456789ddd"') + .replace('update="1"', 'create="1" update="1"') + .replace('baseVersion="1"', 'baseVersion=""')) + .expect(200); + + await exhaust(container); + await asAlice.get('/v1/projects/1/datasets/people/entities') + .expect(200) + .then(({ body: people }) => { + // existing entity + new entity + people.length.should.be.eql(2); + }); + })); + + it('should log error when both create and update are true, baseVersion is empty but entity uuid exists', testEntityUpdates(async (service, container) => { + const asAlice = await service.login('alice'); + + // This submission uses an existing entity uuid + // has create and update both set to true + // and is missing a baseVersion. + // This will attempt to update but fail because it finds no + // baseVersion to parse in the update case + // and will then attempt to create but fail because + // the entity uuid does actually exist. + await asAlice.post('/v1/projects/1/forms/updateEntity/submissions') + .send(testData.instances.updateEntity.one + .replace('update="1"', 'create="1" update="1"') + .replace('baseVersion="1"', 'baseVersion=""')) + .expect(200); + + await exhaust(container); + + await asAlice.get('/v1/projects/1/forms/updateEntity/submissions/one/audits') + .expect(200) + .then(({ body: logs }) => { + logs[0].action.should.be.eql('entity.error'); + logs[0].details.problem.problemCode.should.be.eql(409.3); + logs[0].details.errorMessage.should.be.eql('A resource already exists with uuid value(s) of 12345678-1234-4123-8234-123456789abc.'); + logs[1].action.should.be.eql('submission.create'); + }); + })); }); // 10 examples described in central issue #517 diff --git a/test/unit/data/entity.js b/test/unit/data/entity.js index 252dc6105..47ab73d84 100644 --- a/test/unit/data/entity.js +++ b/test/unit/data/entity.js @@ -131,36 +131,30 @@ describe('extracting and validating entities', () => { 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)); + const entity = { system: { baseVersion: '99' } }; + extractBaseVersionFromSubmission(entity, { update: true }).should.equal(99); }); - it('not return base version if neither create nor update are provided', () => { + it('should extract base version even if create is true and update is not', () => { const entity = { system: { baseVersion: '99' } }; - should.not.exist(extractBaseVersionFromSubmission(entity)); + extractBaseVersionFromSubmission(entity, { create: true }).should.equal(99); }); - it('should complain if baseVersion is missing when update is true (update = 1)', () => { + it('should complain if baseVersion is missing when update is true', () => { const entity = { system: { update: '1' } }; - assert.throws(() => { extractBaseVersionFromSubmission(entity); }, (err) => { + assert.throws(() => { extractBaseVersionFromSubmission(entity, { update: true }); }, (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('not complain if baseVersion is missing when update is false and create is true', () => { + const entity = { system: { } }; + // the create/update values do not get pulled from the entity system data here + // but rather from the branch in the code that decides whether a create + // or update is currently being attempted. + should.not.exist(extractBaseVersionFromSubmission(entity, { create: true })); }); it('should complain if baseVersion not an integer', () => { diff --git a/test/unit/model/frames/entity.js b/test/unit/model/frames/entity.js index 5a16f5f1f..b452d0f47 100644 --- a/test/unit/model/frames/entity.js +++ b/test/unit/model/frames/entity.js @@ -1,5 +1,7 @@ const appRoot = require('app-root-path'); const { Entity } = require(appRoot + '/lib/model/frames'); +const assert = require('assert'); + describe('entity', () => { describe('fromParseEntityData', () => { @@ -23,6 +25,77 @@ describe('entity', () => { })); partial.aux.should.have.property('dataset', 'people'); }); + + it('should throw 400.2 for other problems like missing branchId when trunkVersion is present', () => { + const entity = { + system: { + label: 'label', + id: 'uuid:12345678-1234-4123-8234-abcd56789abc', + update: '1', + trunkVersion: '1', + baseVersion: '3', + dataset: 'people' + }, + data: { field: 'value' } + }; + + assert.throws(() => { Entity.fromParseEntityData(entity, { update: true }); }, (err) => { + err.problemCode.should.equal(400.2); + err.message.should.equal('Required parameter branchId missing.'); + return true; + }); + }); + + describe('baseVersion', () => { + it('should parse successfully for empty baseVersion, create: true', () => { + const partial = Entity.fromParseEntityData({ + system: { + label: 'label', + id: 'uuid:12345678-1234-4123-8234-abcd56789abc', + create: '1', + baseVersion: '', + dataset: 'people' + }, + data: { field: 'value' } + }, + { create: true }); + partial.aux.def.should.not.have.property('baseVersion'); + }); + + it('should return baseVersion even when create: true', () => { + const partial = Entity.fromParseEntityData({ + system: { + label: 'label', + id: 'uuid:12345678-1234-4123-8234-abcd56789abc', + create: '1', + baseVersion: '73', + dataset: 'people' + }, + data: { field: 'value' } + }, + { create: true }); + partial.aux.def.baseVersion.should.equal(73); + }); + + it('should complain about missing baseVersion when update: true', () => { + const entity = { + system: { + label: 'label', + id: 'uuid:12345678-1234-4123-8234-abcd56789abc', + update: '1', + baseVersion: '', + dataset: 'people' + }, + data: { field: 'value' } + }; + + assert.throws(() => { Entity.fromParseEntityData(entity, { update: true }); }, (err) => { + err.problemCode.should.equal(400.2); + err.message.should.equal('Required parameter baseVersion missing.'); + return true; + }); + }); + }); }); });