From 5b386ee60458bf6314e338933b6a18f7e3ccf06f Mon Sep 17 00:00:00 2001 From: Sadiq Khoja Date: Mon, 30 Oct 2023 21:42:40 -0400 Subject: [PATCH] Features/entities/relevant to conflict flag (#1028) * Added relevantToConflict flag in entity diff API * incorporated PR feedback * Use array indexing instead of Maps + change the definition of lastGoodKnow version * added more tests --- lib/data/entity.js | 55 +++++++++++--- lib/resources/entities.js | 6 +- test/integration/api/entities.js | 125 ++++++++++++++++++++++++++++++- test/unit/data/entity.js | 37 +++++++-- 4 files changed, 198 insertions(+), 25 deletions(-) diff --git a/lib/data/entity.js b/lib/data/entity.js index 936c00feb..2c18c4fec 100644 --- a/lib/data/entity.js +++ b/lib/data/entity.js @@ -354,39 +354,70 @@ const ConflictType = { SOFT: 'soft', // discrepancy in version number but no overlaping properties HARD: 'hard' // discrepancy in version number and one/more properties updated simultaneously }; -const getWithConflictDetails = (defs) => { - const defMap = new Map(defs.map(d => [d.version, d])); - +const getWithConflictDetails = (defs, audits, relevantToConflict) => { const result = []; + const lastResolveEvent = audits.findLast(a => a.action === 'entity.update.resolve'); + + const auditMap = new Map(audits + .filter(a => a.action === 'entity.create' || a.action === 'entity.update.version') + .map(a => [a.details.entityDefId, a.details])); + + let lastGoodVersion = 0; // Entity Version is never 0 + + const relevantBaseVersions = new Set(); + for (const def of defs) { - const v = mergeLeft(def, + const { sourceEvent, submissionCreate, submission } = auditMap.get(def.id); + + const v = mergeLeft(def.forApi(), { conflict: null, resolved: false, baseDiff: [], serverDiff: [], - conflictingProp: [], - source: {} // TODO + source: { event: sourceEvent, submissionCreate, submission }, + lastGoodVersion: false }); if (v.version > 1) { // v.root is false here - can use either const conflict = v.version !== (v.baseVersion + 1); - v.baseDiff = getDiffProp(v.dataReceived, defMap.get(v.baseVersion).data); - if ('label' in v.dataReceived && v.dataReceived.label !== defMap.get(v.baseVersion).label) v.baseDiff.push('label'); + v.baseDiff = getDiffProp(v.dataReceived, defs[v.baseVersion - 1].data); + if ('label' in v.dataReceived && v.dataReceived.label !== defs[v.baseVersion - 1].label) v.baseDiff.push('label'); - v.serverDiff = getDiffProp(v.dataReceived, defMap.get(v.version - 1).data); - if ('label' in v.dataReceived && v.dataReceived.label !== defMap.get(v.version - 1).label) v.serverDiff.push('label'); + v.serverDiff = getDiffProp(v.dataReceived, defs[v.version - 2].data); + if ('label' in v.dataReceived && v.dataReceived.label !== defs[v.version - 2].label) v.serverDiff.push('label'); if (conflict) { - v.conflict = v.conflictingProp && v.conflictingProp.length > 0 ? ConflictType.HARD : ConflictType.SOFT; + v.conflict = v.conflictingProperties && v.conflictingProperties.length > 0 ? ConflictType.HARD : ConflictType.SOFT; + + v.resolved = lastResolveEvent && lastResolveEvent.details.entityDefId >= def.id; + + if (!v.resolved && lastGoodVersion === 0) { + lastGoodVersion = v.version - 1; + result[v.version - 2].lastGoodVersion = true; + } + } - v.resolve = false; // TOOD + // We have some unresolved conflicts + if (lastGoodVersion > 0) { + relevantBaseVersions.add(v.baseVersion); } } + result.push(v); } + + // We return all the versions after last good version and the base versions of those. + if (relevantToConflict) { + return lastGoodVersion > 0 ? result.filter(v => v.version >= lastGoodVersion || relevantBaseVersions.has(v.version)) : []; + } + + if (lastGoodVersion === 0) { + result[result.length - 1].lastGoodVersion = true; + } + return result; }; diff --git a/lib/resources/entities.js b/lib/resources/entities.js index 8aeaf2470..05cbf29c1 100644 --- a/lib/resources/entities.js +++ b/lib/resources/entities.js @@ -36,7 +36,7 @@ module.exports = (service, endpoint) => { return withEtag(entity.aux.currentVersion.version, () => entity); })); - service.get('/projects/:projectId/datasets/:name/entities/:uuid/versions', endpoint(async ({ Datasets, Entities, Audits }, { params, auth, queryOptions }) => { + service.get('/projects/:projectId/datasets/:name/entities/:uuid/versions', endpoint(async ({ Datasets, Entities, Audits }, { params, auth, queryOptions, query }) => { const dataset = await Datasets.get(params.projectId, params.name, true).then(getOrNotFound); @@ -47,9 +47,9 @@ module.exports = (service, endpoint) => { // it means there's no entity with the provided UUID if (defs.length === 0) return reject(Problem.user.notFound()); - const audits = Audits.getByEntityId(defs[0].id, queryOptions); + const audits = await Audits.getByEntityId(defs[0].entityId, queryOptions); - return getWithConflictDetails(defs.map(d => d.forApi()), audits); + return getWithConflictDetails(defs, audits, isTrue(query.relevantToConflict)); })); service.get('/projects/:projectId/datasets/:name/entities/:uuid/diffs', endpoint(async ({ Datasets, Entities }, { params, auth, queryOptions }) => { diff --git a/test/integration/api/entities.js b/test/integration/api/entities.js index 4d6226e5f..d8cdf7c96 100644 --- a/test/integration/api/entities.js +++ b/test/integration/api/entities.js @@ -472,6 +472,7 @@ describe('Entities API', () => { }); versions[1].data.should.be.eql({ age: '12', first_name: 'John' }); + versions[1].lastGoodVersion.should.be.true(); }); })); @@ -549,10 +550,130 @@ describe('Entities API', () => { v.should.have.property('data'); }); - versions[2].conflictingProperties.should.be.eql([]); - versions[3].conflictingProperties.should.be.eql(['age', 'label']); + const thirdVersion = versions[2]; + thirdVersion.conflict.should.be.eql('soft'); + thirdVersion.conflictingProperties.should.be.eql([]); + thirdVersion.source.event.action.should.be.eql('submission.create'); + thirdVersion.source.submission.instanceId.should.be.eql('two'); + + const fourthVersion = versions[3]; + fourthVersion.conflict.should.be.eql('hard'); + fourthVersion.conflictingProperties.should.be.eql(['age', 'label']); + fourthVersion.source.event.action.should.be.eql('submission.create'); + fourthVersion.source.submission.instanceId.should.be.eql('one'); + }); })); + + describe('relevantToConflict', () => { + + const createConflictOnV2 = async (user, container) => { + await user.patch('/v1/projects/1/datasets/people/entities/12345678-1234-4123-8234-123456789abc') + .send({ data: { age: '12' } }) + .set('If-Match', '"1"') + .expect(200); + + await user.patch('/v1/projects/1/datasets/people/entities/12345678-1234-4123-8234-123456789abc') + .send({ data: { age: '18' } }) + .set('If-Match', '"2"') + .expect(200); + + await user.post('/v1/projects/1/forms?publish=true') + .send(testData.forms.updateEntity) + .set('Content-Type', 'application/xml') + .expect(200); + + // Hard conflict - all properties are changed + await user.post('/v1/projects/1/forms/updateEntity/submissions') + .send(testData.instances.updateEntity.one.replace('baseVersion="1"', 'baseVersion="2"')) + .set('Content-Type', 'application/xml') + .expect(200); + + await exhaust(container); + }; + + it('should return only relevent versions needed for conflict resolution', testEntities(async (service, container) => { + const asAlice = await service.login('alice'); + + await createConflictOnV2(asAlice, container); + + await asAlice.get('/v1/projects/1/datasets/people/entities/12345678-1234-4123-8234-123456789abc/versions?relevantToConflict=true') + .expect(200) + .then(({ body: versions }) => { + // Doesn't return first version + versions.map(v => v.version).should.eql([2, 3, 4]); + + versions[1].lastGoodVersion.should.be.true(); + versions[2].conflictingProperties.should.be.eql(['age']); + }); + })); + + it('should return empty array when all conflicts are resolved', testEntities(async (service, container) => { + const asAlice = await service.login('alice'); + + await createConflictOnV2(asAlice, container); + + await asAlice.patch('/v1/projects/1/datasets/people/entities/12345678-1234-4123-8234-123456789abc?resolve=true') + .expect(200); + + await asAlice.get('/v1/projects/1/datasets/people/entities/12345678-1234-4123-8234-123456789abc/versions?relevantToConflict=true') + .expect(200) + .then(({ body: versions }) => { + versions.length.should.be.eql(0); + }); + })); + + it('should return only relevent versions after conflict resolution', testEntities(async (service, container) => { + const asAlice = await service.login('alice'); + + await createConflictOnV2(asAlice, container); + + await asAlice.patch('/v1/projects/1/datasets/people/entities/12345678-1234-4123-8234-123456789abc?resolve=true') + .expect(200); + + await asAlice.post('/v1/projects/1/forms/updateEntity/submissions') + .send(testData.instances.updateEntity.two + .replace('baseVersion="1"', 'baseVersion="3"')) + .set('Content-Type', 'application/xml') + .expect(200); + + await exhaust(container); + + await asAlice.get('/v1/projects/1/datasets/people/entities/12345678-1234-4123-8234-123456789abc/versions?relevantToConflict=true') + .expect(200) + .then(({ body: versions }) => { + // Doesn't return old versions + versions.map(v => v.version).should.eql([3, 4, 5]); + + versions[1].lastGoodVersion.should.be.true(); + versions[2].conflictingProperties.should.be.eql(['label']); + }); + })); + + it('should correctly set `resolved` flag for the versions', testEntities(async (service, container) => { + const asAlice = await service.login('alice'); + + await createConflictOnV2(asAlice, container); + + await asAlice.patch('/v1/projects/1/datasets/people/entities/12345678-1234-4123-8234-123456789abc?resolve=true') + .expect(200); + + await asAlice.post('/v1/projects/1/forms/updateEntity/submissions') + .send(testData.instances.updateEntity.two + .replace('baseVersion="1"', 'baseVersion="2"')) + .set('Content-Type', 'application/xml') + .expect(200); + + await exhaust(container); + + await asAlice.get('/v1/projects/1/datasets/people/entities/12345678-1234-4123-8234-123456789abc/versions') + .expect(200) + .then(({ body: versions }) => { + // resolved flag is true only for the old conflict + versions.map(v => v.resolved).should.eql([false, false, false, true, false]); + }); + })); + }); }); describe('GET /datasets/:name/entities/:uuid/diffs', () => { diff --git a/test/unit/data/entity.js b/test/unit/data/entity.js index 63ecd8da5..9f5a1385a 100644 --- a/test/unit/data/entity.js +++ b/test/unit/data/entity.js @@ -2,6 +2,7 @@ const should = require('should'); 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 { fieldsFor } = require(appRoot + '/test/util/schema'); const testData = require(appRoot + '/test/data/xml'); @@ -687,12 +688,14 @@ describe('extracting and validating entities', () => { it('should fill in correct information for SOFT conflict', () => { const defs = [ - { version: 1, label: 'John', data: { name: 'John', age: '88' }, dataReceived: { name: 'John', age: '88' }, conflictingProp: null, baseVersion: null }, - { version: 2, label: 'Jane', data: { name: 'Jane', age: '88' }, dataReceived: { name: 'Jane' }, conflictingProp: [], baseVersion: 1 }, - { version: 3, label: 'Jane', data: { name: 'Jane', age: '99' }, dataReceived: { age: '99' }, conflictingProp: [], baseVersion: 1 } + new Entity.Def({ id: 0, version: 1, label: 'John', data: { name: 'John', age: '88' }, dataReceived: { name: 'John', age: '88' }, conflictingProp: null, baseVersion: null }), + new Entity.Def({ id: 0, version: 2, label: 'Jane', data: { name: 'Jane', age: '88' }, dataReceived: { label: 'Jane', name: 'Jane' }, conflictingProp: [], baseVersion: 1 }), + new Entity.Def({ id: 0, version: 3, label: 'Jane', data: { name: 'Jane', age: '99' }, dataReceived: { age: '99' }, conflictingProp: [], baseVersion: 1 }) ]; - const result = getWithConflictDetails(defs); + const audits = [{ action: 'entity.create', details: { entityDefId: 0 } }]; + + const result = getWithConflictDetails(defs, audits, false); result[2].conflict.should.be.eql(ConflictType.SOFT); result[2].baseDiff.should.be.eql(['age']); @@ -701,16 +704,34 @@ describe('extracting and validating entities', () => { it('should fill in correct information for HARD conflict', () => { const defs = [ - { version: 1, label: 'John', data: { name: 'John', age: '88' }, dataReceived: { name: 'John', age: '88' }, conflictingProp: null, baseVersion: null }, - { version: 2, label: 'Jane', data: { name: 'Jane', age: '77' }, dataReceived: { age: '77' }, conflictingProp: [], baseVersion: 1 }, - { version: 3, label: 'Jane', data: { name: 'Jane', age: '99' }, dataReceived: { age: '99' }, conflictingProp: ['age'], baseVersion: 1 } + new Entity.Def({ id: 0, version: 1, label: 'John', data: { name: 'John', age: '88' }, dataReceived: { name: 'John', age: '88' }, conflictingProperties: null, baseVersion: null }), + new Entity.Def({ id: 0, version: 2, label: 'Jane', data: { name: 'Jane', age: '77' }, dataReceived: { label: 'Jane', name: 'Jane', age: '77' }, conflictingProperties: [], baseVersion: 1 }), + new Entity.Def({ id: 0, version: 3, label: 'Jane', data: { name: 'Jane', age: '99' }, dataReceived: { age: '99' }, conflictingProperties: ['age'], baseVersion: 1 }) ]; - const result = getWithConflictDetails(defs); + const audits = [{ action: 'entity.create', details: { entityDefId: 0 } }]; + + const result = getWithConflictDetails(defs, audits, false); result[2].conflict.should.be.eql(ConflictType.HARD); result[2].baseDiff.should.be.eql(['age']); result[2].serverDiff.should.be.eql(['age']); }); + + it('should return only relevant versions', () => { + const defs = [ + new Entity.Def({ id: 0, version: 1, label: 'John', data: { name: 'John', age: '88' }, dataReceived: { name: 'John', age: '88' }, conflictingProp: null, baseVersion: null }), + new Entity.Def({ id: 0, version: 2, label: 'Robert', data: { name: 'Robert', age: '20' }, dataReceived: { label: 'Robert', name: 'Robert', age: '20' }, conflictingProp: null, baseVersion: 1 }), + new Entity.Def({ id: 0, version: 3, label: 'Jane', data: { name: 'Jane', age: '20' }, dataReceived: { label: 'Jane', name: 'Jane' }, conflictingProp: [], baseVersion: 2 }), + new Entity.Def({ id: 0, version: 4, label: 'Jane', data: { name: 'Jane', age: '99' }, dataReceived: { age: '99' }, conflictingProp: [], baseVersion: 2 }), + new Entity.Def({ id: 0, version: 5, label: 'Jane', data: { name: 'Jane', age: '10' }, dataReceived: { age: '10' }, conflictingProp: [], baseVersion: 3 }), + ]; + + const audits = [{ action: 'entity.create', details: { entityDefId: 0 } }]; + + const result = getWithConflictDetails(defs, audits, true); + + result.map(v => v.version).should.eql([2, 3, 4, 5]); + }); }); });