Updates from final code review

lib/data/entity.js

@@ -69,14 +69,12 @@ const extractBaseVersionFromSubmission = (entity) => {
 
 const extractBranchIdFromSubmission = (entity) => {
   const { branchId } = entity.system;
-  if (branchId === '')
+  if (branchId === '' || branchId == null)
     return null;
 
-  if (branchId) {
-    const matches = _uuidPattern.exec(branchId);
-    if (matches == null) throw Problem.user.invalidDataTypeOfParameter({ field: 'branchId', expected: 'valid version 4 UUID' });
-    return matches[2].toLowerCase();
-  }
+  const matches = _uuidPattern.exec(branchId);
+  if (matches == null) throw Problem.user.invalidDataTypeOfParameter({ field: 'branchId', expected: 'valid version 4 UUID' });
+  return matches[2].toLowerCase();
 };
 
 const extractTrunkVersionFromSubmission = (entity) => {

lib/model/migrations/20240607-02-add-submission-backlog.js

@@ -9,11 +9,11 @@
 
 const up = async (db) => {
   await db.raw(`CREATE TABLE entity_submission_backlog (
-    "submissionId" INT4,
-    "submissionDefId" INT4,
-    "branchId" UUID,
-    "branchBaseVersion" INT4,
-    "loggedAt" TIMESTAMPTZ(3),
+    "submissionId" INT4 NOT NULL,
+    "submissionDefId" INT4 NOT NULL,
+    "branchId" UUID NOT NULL,
+    "branchBaseVersion" INT4 NOT NULL,
+    "loggedAt" TIMESTAMPTZ(3) NOT NULL,
     CONSTRAINT fk_submission_defs
       FOREIGN KEY("submissionDefId") 
       REFERENCES submission_defs(id)

lib/model/query/entities.js

@@ -276,6 +276,11 @@ const _updateEntity = (dataset, entityData, submissionId, submissionDef, submiss
     serverEntity = serverEntity.get();
   }
 
+  // If the trunk version exists but is higher than current server version,
+  // that is a weird case that should not be processed OR held, and should log an error.
+  if (clientEntity.def.trunkVersion && clientEntity.def.trunkVersion > serverEntity.aux.currentVersion.version) {
+    throw Problem.user.entityVersionNotFound({ baseVersion: `trunkVersion=${clientEntity.def.trunkVersion}`, entityUuid: clientEntity.uuid, datasetName: dataset.name });
+  }
 
   let { conflict } = serverEntity;
   let conflictingProperties; // Maybe we don't need to persist this??? just compute at the read time
@@ -287,7 +292,7 @@ const _updateEntity = (dataset, entityData, submissionId, submissionDef, submiss
 
   // If baseVersion is null, we held a submission and will stop processing now.
   if (baseVersion == null)
-    return;
+    return null;
 
   if (baseVersion !== serverEntity.aux.currentVersion.version) {
 
@@ -416,14 +421,11 @@ const _processSubmissionEvent = (event, parentEvent) => async ({ Audits, Dataset
     maybeEntity = await Entities._createEntity(dataset, entityData, submissionId, submissionDef, submissionDefId, event, parentEvent);
 
   // Check for held submissions that follow this one in the same branch
-  // TODO: consider changing checkHeldSub query to check for existing version of entity
-  // so that return value of create/update above doesn't matter as much.
-  // Also consider actually checking entity uuid, in query.
   if (maybeEntity != null && maybeEntity.aux.currentVersion.branchId != null) {
-    const { branchId } = maybeEntity.aux.currentVersion;
-    // baseVersion could be '', meaning its an offline create
-    const currentBaseVersion = entityData.system.baseVersion === '' ? 0 : parseInt(entityData.system.baseVersion, 10);
-    const nextSub = await _checkHeldSubmission(maybeOne, branchId, currentBaseVersion + 1);
+    const { branchId, branchBaseVersion } = maybeEntity.aux.currentVersion;
+    // branchBaseVersion could be undefined if handling an offline create
+    const currentBranchBaseVersion = branchBaseVersion ?? 0;
+    const nextSub = await _checkHeldSubmission(maybeOne, branchId, currentBranchBaseVersion + 1);
     if (nextSub.isDefined()) {
       const { submissionId: nextSubmissionId, submissionDefId: nextSubmissionDefId } = nextSub.get();
       await Audits.log({ id: event.actorId }, 'submission.reprocess', { acteeId: event.acteeId },

test/integration/api/offline-entities.js

@@ -53,6 +53,7 @@ describe('Offline Entities', () => {
           // This is the first version of the entity so there should be no base or trunk versions
           should.not.exist(body.currentVersion.trunkVersion);
           should.not.exist(body.currentVersion.baseVersion);
+          should.not.exist(body.currentVersion.branchBaseVersion);
           body.currentVersion.branchId.should.equal(branchId);
         });
     }));
@@ -354,6 +355,7 @@ describe('Offline Entities', () => {
       await asAlice.get('/v1/projects/1/datasets/people/entities/12345678-1234-4123-8234-123456789abc')
         .expect(200)
         .then(({ body }) => {
+          body.currentVersion.version.should.equal(1);
           // no status property in data because out of order update did not get applied
           body.currentVersion.data.should.eql({ age: '22', first_name: 'Johnny' });
         });
@@ -404,23 +406,32 @@ describe('Offline Entities', () => {
         });
     }));
 
-    it('should not apply later trunkVersion when run has not even started', testOfflineEntities(async (service, container) => {
+    it('should not apply later trunkVersion (past existing server version)', testOfflineEntities(async (service, container) => {
       const asAlice = await service.login('alice');
       const branchId = uuid();
 
-      // much later run index
+      // trunkVersion past existing server version
       await asAlice.post('/v1/projects/1/forms/offlineEntity/submissions')
         .send(testData.instances.offlineEntity.one
           .replace('branchId=""', `branchId="${branchId}"`)
-          .replace('one', 'one-update2')
-          .replace('baseVersion="1"', 'baseVersion="2"')
-          .replace('<status>arrived</status>', '<status>departed</status>')
+          .replace('trunkVersion="1"', 'trunkVersion="2"')
+          .replace('<status>arrived</status>', '<status>weird case</status>')
         )
         .set('Content-Type', 'application/xml')
         .expect(200);
 
       await exhaust(container);
 
+      const backlogCount = await container.oneFirst(sql`select count(*) from entity_submission_backlog`);
+      backlogCount.should.equal(0);
+
+      await asAlice.get('/v1/projects/1/forms/offlineEntity/submissions/one/audits')
+        .expect(200)
+        .then(({ body }) => {
+          body[0].details.errorMessage.should.eql('Base version (trunkVersion=2) does not exist for entity UUID (12345678-1234-4123-8234-123456789abc) in dataset (people).');
+        });
+
+
       await asAlice.get('/v1/projects/1/datasets/people/entities/12345678-1234-4123-8234-123456789abc')
         .expect(200)
         .then(({ body }) => {
@@ -475,6 +486,12 @@ describe('Offline Entities', () => {
 
       await exhaust(container);
 
+      await asAlice.get('/v1/projects/1/forms/offlineEntity/submissions/one-update2/audits')
+        .expect(200)
+        .then(({ body }) => {
+          body[1].action.should.equal('submission.reprocess');
+        });
+
       await asAlice.get('/v1/projects/1/datasets/people/entities/12345678-1234-4123-8234-123456789abc')
         .expect(200)
         .then(({ body }) => {
@@ -552,7 +569,7 @@ describe('Offline Entities', () => {
       let backlogCount = await container.oneFirst(sql`select count(*) from entity_submission_backlog`);
       backlogCount.should.equal(1);
 
-      // Second submission contains update after create (middle of branhc)
+      // Second submission contains update after create (middle of branch)
       await asAlice.post('/v1/projects/1/forms/offlineEntity/submissions')
         .send(testData.instances.offlineEntity.two
           .replace('create="1"', 'update="1"')