Enhance: add entity version number and modify update entity API to wo…

…rk with version (#951)

* Enhance: add entity version number and modify update entity API to work with version

* rename migration file

* removed TODO comments

lib/data/entity.js

@@ -168,8 +168,8 @@ const streamEntityCsv = (inStream, properties) => {
   props.push(...properties.map(p => ['def', 'data', p.name]));
 
   // System properties
-  header.push(...[ '__createdAt', '__creatorId', '__creatorName', '__updates', '__updatedAt']);
-  props.push(...['createdAt', 'creatorId', 'aux.creator.displayName', 'aux.stats.updates', 'updatedAt'].map(e => e.split('.')));
+  header.push(...[ '__createdAt', '__creatorId', '__creatorName', '__updates', '__updatedAt', '__version']);
+  props.push(...['createdAt', 'creatorId', 'aux.creator.displayName', 'aux.stats.updates', 'updatedAt', 'def.version'].map(e => e.split('.')));
 
   const entityStream = _entityTransformer(header, props);
 
@@ -184,8 +184,8 @@ const streamEntityCsv = (inStream, properties) => {
 
 const streamEntityCsvAttachment = (inStream, properties) => {
   // Identifiers
-  const header = [ 'name', 'label' ];
-  const props = ['uuid', 'def.label'].map(e => e.split('.'));
+  const header = [ 'name', 'label', 'version' ];
+  const props = ['uuid', 'def.label', 'def.version'].map(e => e.split('.'));
 
   // User defined dataset properties
   header.push(...properties.map(p => p.name));
@@ -214,7 +214,8 @@ const selectFields = (entity, properties, selectedProperties) => {
     creatorId: entity.aux.creator.id.toString(),
     creatorName: entity.aux.creator.displayName,
     updates: entity.aux.stats.updates,
-    updatedAt: entity.updatedAt
+    updatedAt: entity.updatedAt,
+    version: entity.def.version
   };
 
   if (!selectedProperties || selectedProperties.has('__system')) {

lib/model/frames/entity.js

@@ -67,6 +67,7 @@ Entity.Def = Frame.define(
   'sourceId',                   'label', readable,
   'creatorId', readable,        'userAgent', readable,
   'data', readable,             'root',
+  'version', readable,
   embedded('creator'),
   embedded('source')
 );

lib/model/migrations/20230824-01-add-entity-version.js

@@ -0,0 +1,25 @@
+// Copyright 2023 ODK Central Developers
+// See the NOTICE file at the top-level directory of this distribution and at
+// https://github.com/getodk/central-backend/blob/master/NOTICE.
+// This file is part of ODK Central. It is subject to the license terms in
+// the LICENSE file found in the top-level directory of this distribution and at
+// https://www.apache.org/licenses/LICENSE-2.0. No part of ODK Central,
+// including this file, may be copied, modified, propagated, or distributed
+// except according to the terms contained in the LICENSE file.
+
+const up = async (db) => {
+  await db.raw('ALTER TABLE entity_defs ADD COLUMN version INT4 NOT NULL DEFAULT 1');
+
+  // Sets the correct version number for existing entities
+  await db.raw(`
+  UPDATE entity_defs SET "version" = vt.rownumber
+  FROM (
+    SELECT ROW_NUMBER() OVER(PARTITION BY "entityId" ORDER BY id ) rownumber, id FROM entity_defs
+  )
+  vt WHERE vt.id = entity_defs.id
+`);
+};
+
+const down = (db) => db.raw('ALTER TABLE entity_defs DROP COLUMN version');
+
+module.exports = { up, down };

lib/model/query/entities.js

@@ -22,9 +22,9 @@ const { runSequentially } = require('../../util/promise');
 ////////////////////////////////////////////////////////////////////////////////
 // ENTITY CREATE
 
-const _defInsert = (id, root, creatorId, userAgent, label, json, sourceId = null) => sql`
-  insert into entity_defs ("entityId", "root", "sourceId", "creatorId", "userAgent", "label", "data", "current", "createdAt")
-  values (${id}, ${root}, ${sourceId}, ${creatorId}, ${userAgent}, ${label}, ${json}, true, clock_timestamp())
+const _defInsert = (id, root, creatorId, userAgent, label, json, version, sourceId = null) => sql`
+  insert into entity_defs ("entityId", "root", "sourceId", "creatorId", "userAgent", "label", "data", "current", "createdAt", "version")
+  values (${id}, ${root}, ${sourceId}, ${creatorId}, ${userAgent}, ${label}, ${json}, true, clock_timestamp(), ${version})
   returning *`;
 const nextval = sql`nextval(pg_get_serial_sequence('entities', 'id'))`;
 
@@ -51,7 +51,7 @@ const createNew = (dataset, partial, subDef, sourceId, userAgentIn) => ({ one, c
   const json = JSON.stringify(partial.def.data);
 
   return one(sql`
-with def as (${_defInsert(nextval, true, creatorId, userAgent, partial.def.label, json, sourceId)}),
+with def as (${_defInsert(nextval, true, creatorId, userAgent, partial.def.label, json, 1, sourceId)}),
 ins as (insert into entities (id, "datasetId", "uuid", "createdAt", "creatorId")
   select def."entityId", ${dataset.id}, ${partial.uuid}, def."createdAt", ${creatorId} from def
   returning entities.*)
@@ -195,7 +195,7 @@ const createEntitiesFromPendingSubmissions = (submissionEvents, parentEvent) =>
 ////////////////////////////////////////////////////////////////////////////////
 // UPDATING ENTITIES
 
-const createVersion = (dataset, entity, data, label, sourceId, userAgentIn = null) => ({ context, one }) => {
+const createVersion = (dataset, entity, data, label, version, sourceId, userAgentIn = null) => ({ context, one }) => {
   // dataset is passed in so the audit log can use its actee id
   const creatorId = context.auth.actor.map((actor) => actor.id).orNull();
   const userAgent = blankStringToNull(userAgentIn);
@@ -204,7 +204,7 @@ const createVersion = (dataset, entity, data, label, sourceId, userAgentIn = nul
   const _unjoiner = unjoiner(Entity, Entity.Def.into('currentVersion'));
 
   return one(sql`
-  with def as (${_defInsert(entity.id, false, creatorId, userAgent, label, json, sourceId)}),
+  with def as (${_defInsert(entity.id, false, creatorId, userAgent, label, json, version, sourceId)}),
   upd as (update entity_defs set current=false where entity_defs."entityId" = ${entity.id}),
   entities as (update entities set "updatedAt"=clock_timestamp()
     where "uuid"=${entity.uuid}

lib/resources/entities.js

@@ -8,7 +8,7 @@
 // except according to the terms contained in the LICENSE file.
 
 const { getOrNotFound, reject } = require('../util/promise');
-const { isTrue, success } = require('../util/http');
+const { isTrue, success, withEtag } = require('../util/http');
 const { Entity } = require('../model/frames');
 const Problem = require('../util/problem');
 const { diffEntityData } = require('../data/entity');
@@ -30,7 +30,10 @@ module.exports = (service, endpoint) => {
 
     await auth.canOrReject('entity.read', dataset);
 
-    return Entities.getById(dataset.id, params.uuid, queryOptions).then(getOrNotFound);
+
+    const entity = await Entities.getById(dataset.id, params.uuid, queryOptions).then(getOrNotFound);
+
+    return withEtag(entity.aux.currentVersion.version, () => entity);
   }));
 
   service.get('/projects/:projectId/datasets/:name/entities/:uuid/versions', endpoint(async ({ Datasets, Entities }, { params, auth, queryOptions }) => {
@@ -91,21 +94,27 @@ module.exports = (service, endpoint) => {
     return Entities.getById(dataset.id, entity.uuid).then(getOrNotFound);
   }));
 
-  service.patch('/projects/:id/datasets/:name/entities/:uuid', endpoint(async ({ Datasets, Entities }, { auth, body, params, query, userAgent }) => {
+  service.patch('/projects/:id/datasets/:name/entities/:uuid', endpoint(async ({ Datasets, Entities }, { auth, body, params, query, userAgent, headers }) => {
 
     const dataset = await Datasets.get(params.id, params.name).then(getOrNotFound);
 
     await auth.canOrReject('entity.update', dataset);
 
     const entity = await Entities.getById(dataset.id, params.uuid).then(getOrNotFound);
 
-    if (!isTrue(query.force)) return reject(Problem.internal.notImplemented({ feature: '(updating an entity without ?force=true flag)' }));
+    const clientEntityVersion = headers['if-match'] && headers['if-match'].replaceAll('"', '');
+    const serverEntityVersion = entity.aux.currentVersion.version;
+
+    if (clientEntityVersion !== serverEntityVersion.toString() && !isTrue(query.force)) {
+      return reject(Problem.user.entityVersionConflict({ current: serverEntityVersion.toString(), provided: clientEntityVersion }));
+    }
 
     const properties = await Datasets.getProperties(dataset.id);
     const partial = Entity.fromJson(body, properties, dataset, entity);
 
     const sourceId = await Entities.createSource();
-    return Entities.createVersion(dataset, entity, partial.def.data, partial.def.label, sourceId, userAgent);
+
+    return Entities.createVersion(dataset, entity, partial.def.data, partial.def.label, serverEntityVersion + 1, sourceId, userAgent);
   }));
 
   service.delete('/projects/:projectId/datasets/:name/entities/:uuid', endpoint(async ({ Datasets, Entities }, { auth, params, queryOptions }) => {

lib/util/problem.js

@@ -189,9 +189,11 @@ const problems = {
 
     editingClosingOrClosed: problem(409.12, () => 'You are trying to edit a submission of a Form that is in Closing or Closed state. In this version of Central, you can only edit the submissions of Open Forms. We will fix this in a future version for Central.'),
 
-    enketoEditRateLimit: problem(409.13, () => 'You must wait one minute before trying to edit a submission a second time.')
+    enketoEditRateLimit: problem(409.13, () => 'You must wait one minute before trying to edit a submission a second time.'),
 
     // 409.14 (entityViolation) was used in the past, but has been deprecated
+
+    entityVersionConflict: problem(409.15, ({ current, provided }) => `Current version of the Entity is '${current}' and you provided '${provided}'. Please correct the version number or pass '?force=true' in the URL to forcefully update the Entity.`)
   },
   internal: {
     // no detail information, as this is only called when we don't know what happened.

test/integration/api/datasets.js

@@ -285,7 +285,7 @@ describe('datasets and entities', () => {
             .then(() => asAlice.get('/v1/projects/1/datasets/people/entities.csv')
               .expect(200)
               .then(({ text }) => {
-                text.should.equal('__id,label,first_name,age,__createdAt,__creatorId,__creatorName,__updates,__updatedAt\n');
+                text.should.equal('__id,label,first_name,age,__createdAt,__creatorId,__creatorName,__updates,__updatedAt,__version\n');
               })))));
 
       it('should return only published properties', testService(async (service) => {
@@ -306,7 +306,7 @@ describe('datasets and entities', () => {
         await asAlice.get('/v1/projects/1/datasets/people/entities.csv')
           .expect(200)
           .then(({ text }) => {
-            text.should.equal('__id,label,first_name,age,__createdAt,__creatorId,__creatorName,__updates,__updatedAt\n');
+            text.should.equal('__id,label,first_name,age,__createdAt,__creatorId,__creatorName,__updates,__updatedAt,__version\n');
           });
 
       }));
@@ -367,9 +367,9 @@ describe('datasets and entities', () => {
 
         const withOutTs = result.replace(isoRegex, '');
         withOutTs.should.be.eql(
-          '__id,label,first_name,age,__createdAt,__creatorId,__creatorName,__updates,__updatedAt\n' +
-            '12345678-1234-4123-8234-123456789aaa,Jane (30),Jane,30,,5,Alice,0,\n' +
-            '12345678-1234-4123-8234-123456789abc,Alice (88),Alice,88,,5,Alice,0,\n'
+          '__id,label,first_name,age,__createdAt,__creatorId,__creatorName,__updates,__updatedAt,__version\n' +
+            '12345678-1234-4123-8234-123456789aaa,Jane (30),Jane,30,,5,Alice,0,,1\n' +
+            '12345678-1234-4123-8234-123456789abc,Alice (88),Alice,88,,5,Alice,0,,1\n'
         );
 
       }));
@@ -399,8 +399,8 @@ describe('datasets and entities', () => {
 
         const withOutTs = result.replace(isoRegex, '');
         withOutTs.should.be.eql(
-          '__id,label,first_name,the.age,__createdAt,__creatorId,__creatorName,__updates,__updatedAt\n' +
-            '12345678-1234-4123-8234-123456789abc,Alice (88),Alice,88,,5,Alice,0,\n'
+          '__id,label,first_name,the.age,__createdAt,__creatorId,__creatorName,__updates,__updatedAt,__version\n' +
+            '12345678-1234-4123-8234-123456789abc,Alice (88),Alice,88,,5,Alice,0,,1\n'
         );
 
       }));
@@ -472,8 +472,8 @@ describe('datasets and entities', () => {
 
         const withOutTs = result.replace(isoRegex, '');
         withOutTs.should.be.eql(
-          '__id,label,first_name,age,__createdAt,__creatorId,__creatorName,__updates,__updatedAt\n' +
-            '12345678-1234-4123-8234-111111111aaa,Robert Doe (expired),Robert,,,5,Alice,1,\n'
+          '__id,label,first_name,age,__createdAt,__creatorId,__creatorName,__updates,__updatedAt,__version\n' +
+            '12345678-1234-4123-8234-111111111aaa,Robert Doe (expired),Robert,,,5,Alice,1,,2\n'
         );
 
       }));
@@ -502,8 +502,8 @@ describe('datasets and entities', () => {
 
         const withOutTs = result.text.replace(/\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}.\d{3}Z/g, '');
         withOutTs.should.be.eql(
-          '__id,label,first_name,age,__createdAt,__creatorId,__creatorName,__updates,__updatedAt\n' +
-          '12345678-1234-4123-8234-123456789abc,Alice (88),Alice,88,,5,Alice,0,\n'
+          '__id,label,first_name,age,__createdAt,__creatorId,__creatorName,__updates,__updatedAt,__version\n' +
+          '12345678-1234-4123-8234-123456789abc,Alice (88),Alice,88,,5,Alice,0,,1\n'
         );
 
         const etag = result.get('ETag');
@@ -1391,7 +1391,7 @@ describe('datasets and entities', () => {
               .then(({ headers, text }) => {
                 headers['content-disposition'].should.equal('attachment; filename="goodone.csv"; filename*=UTF-8\'\'goodone.csv');
                 headers['content-type'].should.equal('text/csv; charset=utf-8');
-                text.should.equal('name,label,first_name,age\n12345678-1234-4123-8234-123456789abc,Alice (88),Alice,88\n');
+                text.should.equal('name,label,version,first_name,age\n12345678-1234-4123-8234-123456789abc,Alice (88),1,Alice,88\n');
               })))));
 
       it('should return entities csv for testing', testService(async (service, container) => {
@@ -1412,7 +1412,7 @@ describe('datasets and entities', () => {
 
         await service.get(`/v1/test/${token}/projects/1/forms/withAttachments/draft/attachments/goodone.csv`)
           .expect(200)
-          .then(({ text }) => { text.should.equal('name,label,first_name,age\n12345678-1234-4123-8234-123456789abc,Alice (88),Alice,88\n'); });
+          .then(({ text }) => { text.should.equal('name,label,version,first_name,age\n12345678-1234-4123-8234-123456789abc,Alice (88),1,Alice,88\n'); });
 
       }));
 
@@ -1444,8 +1444,8 @@ describe('datasets and entities', () => {
         await asAlice.get('/v1/projects/1/forms/withAttachments/draft/attachments/goodone.csv')
           .expect(200)
           .then(({ text }) => {
-            text.should.equal('name,label,first_name,the.age\n' +
-          '12345678-1234-4123-8234-123456789abc,Alice (88),Alice,88\n');
+            text.should.equal('name,label,version,first_name,the.age\n' +
+          '12345678-1234-4123-8234-123456789abc,Alice (88),1,Alice,88\n');
           });
 
       }));
@@ -1506,8 +1506,8 @@ describe('datasets and entities', () => {
           .then(r => r.text);
 
         result.should.be.eql(
-          'name,label,first_name,age\n' +
-          '12345678-1234-4123-8234-111111111aaa,Robert Doe (expired),Robert,\n'
+          'name,label,version,first_name,age\n' +
+          '12345678-1234-4123-8234-111111111aaa,Robert Doe (expired),2,Robert,\n'
         );
 
       }));
@@ -1586,8 +1586,8 @@ describe('datasets and entities', () => {
           .expect(200);
 
         result.text.should.be.eql(
-          'name,label,first_name,age\n' +
-          '12345678-1234-4123-8234-123456789abc,Alice (88),Alice,88\n'
+          'name,label,version,first_name,age\n' +
+          '12345678-1234-4123-8234-123456789abc,Alice (88),1,Alice,88\n'
         );
 
         const etag = result.get('ETag');
@@ -1990,7 +1990,7 @@ describe('datasets and entities', () => {
         await asAlice.get('/v1/projects/1/datasets/people/entities.csv')
           .expect(200)
           .then(({ text }) => {
-            text.should.equal('__id,label,__createdAt,__creatorId,__creatorName,__updates,__updatedAt\n');
+            text.should.equal('__id,label,__createdAt,__creatorId,__creatorName,__updates,__updatedAt,__version\n');
           });
       }));
 

test/integration/api/entities.js

@@ -193,7 +193,9 @@ describe('Entities API', () => {
 
       await asAlice.get('/v1/projects/1/datasets/people/entities/12345678-1234-4123-8234-123456789abc')
         .expect(200)
-        .then(({ body: person }) => {
+        .then(({ body: person, headers }) => {
+          headers.etag.should.be.eql('"1"');
+
           person.should.be.an.Entity();
           person.should.have.property('currentVersion').which.is.an.EntityDef();
           person.currentVersion.should.have.property('data').which.is.eql({
@@ -219,7 +221,6 @@ describe('Entities API', () => {
         });
     }));
 
-
     it('should not mince the object properties', testEntities(async (service, container) => {
       const asAlice = await service.login('alice');
       const asBob = await service.login('bob');
@@ -279,6 +280,14 @@ describe('Entities API', () => {
           });
         });
     }));
+
+    it('should return 304 not changed ', testEntities(async (service) => {
+      const asAlice = await service.login('alice');
+
+      await asAlice.get('/v1/projects/1/datasets/people/entities/12345678-1234-4123-8234-123456789abc')
+        .set('If-None-Match', '"1"')
+        .expect(304);
+    }));
   });
 
   describe('GET /datasets/:name/entities/:uuid/versions', () => {
@@ -829,13 +838,22 @@ describe('Entities API', () => {
         .expect(403);
     }));
 
-    it('should reject force=true flag is not provided', testEntities(async (service) => {
-      // TODO: change logic around force flag and enforcing certain kinds of updates
+    it('should reject if version or force flag is not provided', testEntities(async (service) => {
       const asAlice = await service.login('alice');
       await asAlice.patch('/v1/projects/1/datasets/people/entities/12345678-1234-4123-8234-123456789abc')
-        .expect(501)
+        .expect(409)
         .then(({ body }) => {
-          body.code.should.equal(501.1);
+          body.code.should.equal(409.15);
+        });
+    }));
+
+    it('should reject if version does not match', testEntities(async (service) => {
+      const asAlice = await service.login('alice');
+      await asAlice.patch('/v1/projects/1/datasets/people/entities/12345678-1234-4123-8234-123456789abc')
+        .set('If-Match', '"0"')
+        .expect(409)
+        .then(({ body }) => {
+          body.code.should.equal(409.15);
         });
     }));
 
@@ -905,7 +923,8 @@ describe('Entities API', () => {
         const asAlice = await service.login('alice');
         const newData = { age: '77', first_name: 'Alan' };
 
-        await asAlice.patch('/v1/projects/1/datasets/people/entities/12345678-1234-4123-8234-123456789abc?force=true')
+        await asAlice.patch('/v1/projects/1/datasets/people/entities/12345678-1234-4123-8234-123456789abc')
+          .set('If-Match', '"1"')
           .send({
             data: { age: '77', first_name: 'Alan' }
           })