Handle missing baseVersion in entity upsert (create and update) case (#…

…1209)

* Different checking of baseVersion in entity upsert case

* Fix baseVersion parsing when create=true

* code fixes

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) => {

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 }) };

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);

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

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', () => {

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;
+        });
+      });
+    });
   });
 });