diff --git a/lib/data/entity.js b/lib/data/entity.js index 4acdf0525..160c64e83 100644 --- a/lib/data/entity.js +++ b/lib/data/entity.js @@ -397,6 +397,55 @@ const diffEntityData = (defData) => { return diffs; }; +// Copied from frontend +// Offline branch +class Branch { + // firstUpdate is the first offline update (not create) to be processed from + // the branch. + constructor(firstUpdate) { + if (firstUpdate.trunkVersion != null) { + const { trunkVersion } = firstUpdate; + /* this.lastContiguousWithTrunk is the version number of the last version + from the branch that is contiguous with the trunk version. In other words, + it is the version number of the last version where there has been no + update from outside the branch between the version and the trunk version. + this.lastContiguousWithTrunk is not related to branch order: as long as + there hasn't been an update from outside the branch, the branch is + contiguous, regardless of the order of the updates within it. */ + this.lastContiguousWithTrunk = firstUpdate.version === trunkVersion + 1 + ? firstUpdate.version + : 0; + } else { + /* + If the entity was both created and updated offline before being sent to + the server, then there technically isn't a trunk version. On the flip + side, there also isn't a contiguity problem -- except in one special case. + If the submission for the entity creation is sent and processed, but the + submission for the update is not sent at the same time for some reason, + then it's possible for another update to be made between the two. Once the + original update is sent and processed, it will no longer be contiguous + with the entity creation. + + Another special case is if the submission for the entity creation was sent + late and processed out of order. In that case, firstUpdate.version === 1. + There's again no contiguity problem (just an order problem), so + lastContiguousWithTrunk should equal 1. + + The normal case is if firstUpdate.version === 2. + */ + this.lastContiguousWithTrunk = firstUpdate.version === 2 ? 2 : 1; + } + + this.id = firstUpdate.branchId; + } + + add(version) { + if (version.baseVersion === this.lastContiguousWithTrunk && + version.version === version.baseVersion + 1) + this.lastContiguousWithTrunk = version.version; + } +} + // Returns an array of properties which are different between // `dataReceived` and `otherVersionData` const getDiffProp = (dataReceived, otherVersionData) => @@ -419,7 +468,18 @@ const getWithConflictDetails = (defs, audits, relevantToConflict) => { const relevantBaseVersions = new Set(); + const branches = new Map(); + for (const def of defs) { + // build up branches + const { branchId } = def; + if (branchId != null) { + const existingBranch = branches.get(branchId); + if (existingBranch == null) + branches.set(branchId, new Branch(def)); + else + existingBranch.add(def); + } const v = mergeLeft(def.forApi(), { @@ -438,7 +498,10 @@ const getWithConflictDetails = (defs, audits, relevantToConflict) => { v.source = event.source; if (v.version > 1) { // v.root is false here - can use either - const conflict = v.version !== (v.baseVersion + 1); + const baseNotContiguousWithTrunk = v.branchId != null && + branches.get(v.branchId).lastContiguousWithTrunk < v.baseVersion; + const conflict = v.version !== (v.baseVersion + 1) || + baseNotContiguousWithTrunk; v.baseDiff = getDiffProp(v.dataReceived, { ...defs[v.baseVersion - 1].data, label: defs[v.baseVersion - 1].label }); diff --git a/lib/model/query/entities.js b/lib/model/query/entities.js index 59c1a37bb..edf7a3ffe 100644 --- a/lib/model/query/entities.js +++ b/lib/model/query/entities.js @@ -304,10 +304,15 @@ const _updateEntity = (dataset, entityData, submissionId, submissionDef, submiss serverDiffData.label = serverEntity.aux.currentVersion.label; conflictingProperties = Object.keys(clientEntity.def.dataReceived).filter(key => key in serverDiffData && clientEntity.def.dataReceived[key] !== serverDiffData[key]); - if (conflict !== ConflictType.HARD) { // We don't want to downgrade conflict here conflict = conflictingProperties.length > 0 ? ConflictType.HARD : ConflictType.SOFT; } + } else { + // This may still be a soft conflict if the new version is not contiguous with this branch's trunk version + const interrupted = await Entities._interruptedBranch(serverEntity.id, clientEntity); + if (interrupted && conflict !== ConflictType.HARD) { + conflict = ConflictType.SOFT; + } } // merge data @@ -541,6 +546,29 @@ const processSubmissionEvent = (event, parentEvent) => (container) => //////////////////////////////////////////////////////////////////////////////// // Submission processing helper functions +// Used by _updateEntity to determine if a new offline update is contiguous with its trunk version +// by searching for an interrupting version with a different or null branchId that has a higher +// version than the trunk version of the given branch. +const _interruptedBranch = (entityId, clientEntity) => async ({ maybeOne }) => { + // If there is no branchId, the branch cannot be interrupted + if (clientEntity.def.branchId == null) + return false; + + // look for a version of a different branch that has a version + // higher than the trunkVersion, which indicates an interrupting version. + // if trunkVersion is null (becuase it is part of a branch beginning with + // an offline create), look for a version higher than 1 because version + // 1 is implicitly the create action of that offline branch. + const interruptingVersion = await maybeOne(sql` + SELECT version + FROM entity_defs + WHERE "branchId" IS DISTINCT FROM ${clientEntity.def.branchId} + AND version > ${clientEntity.def.trunkVersion || 1} + AND "entityId" = ${entityId} + LIMIT 1`); + return interruptingVersion.isDefined(); +}; + // Used by _computeBaseVersion to hold submissions that are not yet ready to be processed const _holdSubmission = (eventId, submissionId, submissionDefId, entityUuid, branchId, branchBaseVersion) => async ({ run }) => run(sql` INSERT INTO entity_submission_backlog ("auditId", "submissionId", "submissionDefId", "entityUuid", "branchId", "branchBaseVersion", "loggedAt") @@ -780,7 +808,7 @@ module.exports = { createSource, createMany, _createEntity, _updateEntity, - _computeBaseVersion, + _computeBaseVersion, _interruptedBranch, _holdSubmission, _checkHeldSubmission, _getNextHeldSubmissionInBranch, _deleteHeldSubmissionByEventId, _getHeldSubmissionsAsEvents, diff --git a/test/integration/api/offline-entities.js b/test/integration/api/offline-entities.js index f13ec188c..1a0124175 100644 --- a/test/integration/api/offline-entities.js +++ b/test/integration/api/offline-entities.js @@ -1343,6 +1343,282 @@ describe('Offline Entities', () => { }); }); + describe('conflict cases', () => { + it('should mark an update that is not contiguous with its trunk version as a soft conflict', testOfflineEntities(async (service, container) => { + const asAlice = await service.login('alice'); + const branchId = uuid(); + + // Update existing entity on server (change age from 22 to 24) + await asAlice.patch('/v1/projects/1/datasets/people/entities/12345678-1234-4123-8234-123456789abc?baseVersion=1') + .send({ data: { age: '24' } }) + .expect(200); + + // Send update (change status from null to arrived, no other properties included/changed) + // Introduces a soft conflict because baseVersion is 1 + // But no hard conflict + await asAlice.post('/v1/projects/1/forms/offlineEntity/submissions') + .send(testData.instances.offlineEntity.one + .replace('branchId=""', `branchId="${branchId}"`) + ) + .set('Content-Type', 'application/xml') + .expect(200); + + // Send second update (change age from 22 to 26, instead of changing status) + // Doesn't conflict with previous version (from the same offline branch) + // But it should be marked as a soft conflict because the branch was + // interrupted by the first API update. + 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('arrived', '26') + ) + .set('Content-Type', 'application/xml') + .expect(200); + + await exhaust(container); + + // Final version should have soft conflict + await asAlice.get('/v1/projects/1/datasets/people/entities/12345678-1234-4123-8234-123456789abc/versions') + .then(({ body: versions }) => { + versions.map(v => v.conflict).should.eql([null, null, 'soft', 'soft']); + }); + + // Overall entity should have soft conflict + // (A test below shows this is set explicitily and not just carried over + // from the previous conflict state.) + await asAlice.get('/v1/projects/1/datasets/people/entities/12345678-1234-4123-8234-123456789abc') + .then(({ body: entity }) => { + should(entity.conflict).equal('soft'); + }); + })); + + it('should mark an update that is not contiguous (due to force processing) as a soft conflict', testOfflineEntities(async (service, container) => { + const asAlice = await service.login('alice'); + const branchId = uuid(); + + // Scenario described in issue: c#698 + // Send second update first + 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('arrived', 'checked in') + ) + .set('Content-Type', 'application/xml') + .expect(200); + + await exhaust(container); + await container.Entities.processBacklog(true); + + // Send first update now (it will be applied right away) + // Introduces a hard conflict because it will find baseVersion v1 + // and the force-processed update above also branched of v1 + // and both update 'status'. + await asAlice.post('/v1/projects/1/forms/offlineEntity/submissions') + .send(testData.instances.offlineEntity.one + .replace('branchId=""', `branchId="${branchId}"`) + ) + .set('Content-Type', 'application/xml') + .expect(200); + + await exhaust(container); + + // Entity conflict should be hard at this point + await asAlice.get('/v1/projects/1/datasets/people/entities/12345678-1234-4123-8234-123456789abc') + .then(({ body: entity }) => { + should(entity.conflict).equal('hard'); + }); + + // Send fourth update (skipping a 3rd update so this must be force-applied) + await asAlice.post('/v1/projects/1/forms/offlineEntity/submissions') + .send(testData.instances.offlineEntity.one + .replace('branchId=""', `branchId="${branchId}"`) + .replace('one', 'one-update4') + .replace('baseVersion="1"', 'baseVersion="4"') + .replace('arrived', 'departed') + ) + .set('Content-Type', 'application/xml') + .expect(200); + + await exhaust(container); + await container.Entities.processBacklog(true); + + // All updates are from the same branch, but 1 expected version is missing + // and updates came in out of order. Unclear if final 'soft' conflict is what we + // want or if it should possibly be null. + await asAlice.get('/v1/projects/1/datasets/people/entities/12345678-1234-4123-8234-123456789abc/versions') + .then(({ body: versions }) => { + versions.map(v => v.conflict).should.eql([null, null, 'hard', 'soft']); + }); + + // Hard conflict is carried forward + await asAlice.get('/v1/projects/1/datasets/people/entities/12345678-1234-4123-8234-123456789abc') + .then(({ body: entity }) => { + should(entity.conflict).equal('hard'); + }); + })); + + it('should mark an update that is not contiguous with its trunk version as a soft conflict on entity despite earlier conflict resolution', testOfflineEntities(async (service, container) => { + const asAlice = await service.login('alice'); + const branchId = uuid(); + + // Update existing entity on server (change age from 22 to 24) + await asAlice.patch('/v1/projects/1/datasets/people/entities/12345678-1234-4123-8234-123456789abc?baseVersion=1') + .send({ data: { age: '24' } }) + .expect(200); + + // Send update (change status from null to arrived, don't change age) + // Has soft conflict with parallel update but doesn't have hard conflict + // because different properties were changed. + await asAlice.post('/v1/projects/1/forms/offlineEntity/submissions') + .send(testData.instances.offlineEntity.one + .replace('branchId=""', `branchId="${branchId}"`) + ) + .set('Content-Type', 'application/xml') + .expect(200); + await exhaust(container); + + // Entity has soft conflict at this point + await asAlice.get('/v1/projects/1/datasets/people/entities/12345678-1234-4123-8234-123456789abc') + .then(({ body: entity }) => { + should(entity.conflict).equal('soft'); + }); + + await asAlice.patch('/v1/projects/1/datasets/people/entities/12345678-1234-4123-8234-123456789abc?resolve=true&baseVersion=3') + .expect(200); + + // Send second update (change age from 22 to 26) + // Doesn't conflict with previous version (from the same offline branch) + // But it should be marked as a soft conflict because the branch was + // interrupted by the first API update. + 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('arrived', '26') + ) + .set('Content-Type', 'application/xml') + .expect(200); + await exhaust(container); + + // Final version conflict is soft because of interrupted branch + await asAlice.get('/v1/projects/1/datasets/people/entities/12345678-1234-4123-8234-123456789abc/versions') + .then(({ body: versions }) => { + versions.map(v => v.conflict).should.eql([null, null, 'soft', 'soft']); + }); + + // Entity version is soft + await asAlice.get('/v1/projects/1/datasets/people/entities/12345678-1234-4123-8234-123456789abc') + .then(({ body: entity }) => { + should(entity.conflict).equal('soft'); + }); + })); + + it('should mark an update that is not contiguous (from an offline create branch) as a soft conflict on entity despite earlier conflict resolution', testOfflineEntities(async (service, container) => { + const asAlice = await service.login('alice'); + const branchId = uuid(); + + // Send initial submission to create entity + await asAlice.post('/v1/projects/1/forms/offlineEntity/submissions') + .send(testData.instances.offlineEntity.two) + .set('Content-Type', 'application/xml') + .expect(200); + await exhaust(container); + + // Update existing entity on server before getting the rest of the branch + await asAlice.patch('/v1/projects/1/datasets/people/entities/12345678-1234-4123-8234-123456789ddd?baseVersion=1') + .send({ data: { age: '24' } }) + .expect(200); + + // Send update (change status from new to arrived) + await asAlice.post('/v1/projects/1/forms/offlineEntity/submissions') + .send(testData.instances.offlineEntity.two + .replace('two', 'two-update1') + .replace('branchId=""', `branchId="${branchId}"`) + .replace('create="1"', 'update="1"') + .replace('baseVersion=""', 'baseVersion="1"') + .replace('new', 'arrived') + ) + .set('Content-Type', 'application/xml') + .expect(200); + await exhaust(container); + + // Conflict is hard here + await asAlice.get('/v1/projects/1/datasets/people/entities/12345678-1234-4123-8234-123456789ddd') + .then(({ body: entity }) => { + should(entity.conflict).equal('hard'); + }); + + // resolve the conflict + await asAlice.patch('/v1/projects/1/datasets/people/entities/12345678-1234-4123-8234-123456789ddd?resolve=true&baseVersion=3') + .expect(200); + + // Send second update in offline create-update-update chain (change age from 22 to 26) + await asAlice.post('/v1/projects/1/forms/offlineEntity/submissions') + .send(testData.instances.offlineEntity.two + .replace('two', 'two-update2') + .replace('branchId=""', `branchId="${branchId}"`) + .replace('create="1"', 'update="1"') + .replace('baseVersion=""', 'baseVersion="2"') + .replace('new', 'arrived') + .replace('20', '27') + ) + .set('Content-Type', 'application/xml') + .expect(200); + await exhaust(container); + + await asAlice.get('/v1/projects/1/datasets/people/entities/12345678-1234-4123-8234-123456789ddd/versions') + .then(({ body: versions }) => { + versions.map(v => v.conflict).should.eql([null, null, 'hard', 'soft']); + }); + + await asAlice.get('/v1/projects/1/datasets/people/entities/12345678-1234-4123-8234-123456789ddd') + .then(({ body: entity }) => { + should(entity.conflict).equal('soft'); + }); + })); + + it('should check that interrupting version logic is doesnt flag non-conflicts as conflicts', testOfflineEntities(async (service, container) => { + const asAlice = await service.login('alice'); + const branchId = uuid(); + + // Send initial submission to create entity + await asAlice.post('/v1/projects/1/forms/offlineEntity/submissions') + .send(testData.instances.offlineEntity.two) + .set('Content-Type', 'application/xml') + .expect(200); + await exhaust(container); + + // Send second update in offline create-update-update chain (change age from 22 to 26) + await asAlice.post('/v1/projects/1/forms/offlineEntity/submissions') + .send(testData.instances.offlineEntity.two + .replace('two', 'two-update') + .replace('branchId=""', `branchId="${branchId}"`) + .replace('create="1"', 'update="1"') + .replace('baseVersion=""', 'baseVersion="1"') + .replace('new', 'arrived') + .replace('20', '27') + ) + .set('Content-Type', 'application/xml') + .expect(200); + await exhaust(container); + + await asAlice.get('/v1/projects/1/datasets/people/entities/12345678-1234-4123-8234-123456789ddd/versions') + .then(({ body: versions }) => { + versions.map(v => v.conflict).should.eql([null, null]); + }); + + await asAlice.get('/v1/projects/1/datasets/people/entities/12345678-1234-4123-8234-123456789ddd') + .then(({ body: entity }) => { + should(entity.conflict).equal(null); + }); + })); + }); + describe('locking an entity while processing a related submission', function() { this.timeout(8000);