Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Mark soft conflict if not contiguous with trunk #1187

Merged
merged 4 commits into from
Oct 28, 2024
Merged

Mark soft conflict if not contiguous with trunk #1187

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8b50747
Mark soft conflict if not contiguous with trunk
ktuite Sep 16, 2024
21a580d
Remove unneeded code and make other small changes
matthew-white Sep 19, 2024
f4d54fc
query to check for non-contiguous interrupted branches
ktuite Oct 21, 2024
9dd9db0
add more comments and explanation to tests
ktuite Oct 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 64 additions & 1 deletion lib/data/entity.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Comment on lines +442 to +446
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We saw a scenario in a test where this logic didn't quite work out

conflict false false
branches.get(v.branchId).lastContiguousWithTrunk 2
v baseversion 1
add: version, base version and last contiguous 3 1 2
conflict true false
branches.get(v.branchId).lastContiguousWithTrunk 2
v baseversion 1
add: version, base version and last contiguous 4 3 2
conflict true true
branches.get(v.branchId).lastContiguousWithTrunk 2
v baseversion 3

}

// Returns an array of properties which are different between
// `dataReceived` and `otherVersionData`
const getDiffProp = (dataReceived, otherVersionData) =>
Expand All @@ -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(),
{
Expand All @@ -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 });

Expand Down
31 changes: 30 additions & 1 deletion lib/model/query/entities.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,6 +308,12 @@ const _updateEntity = (dataset, entityData, submissionId, submissionDef, submiss
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.SOFT;
}
}

// merge data
Expand Down Expand Up @@ -541,6 +547,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")
Expand Down Expand Up @@ -780,7 +809,7 @@ module.exports = {
createSource,
createMany,
_createEntity, _updateEntity,
_computeBaseVersion,
_computeBaseVersion, _interruptedBranch,
_holdSubmission, _checkHeldSubmission,
_getNextHeldSubmissionInBranch, _deleteHeldSubmissionByEventId,
_getHeldSubmissionsAsEvents,
Expand Down
230 changes: 230 additions & 0 deletions test/integration/api/offline-entities.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1343,6 +1343,236 @@ 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)
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)
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>', '<age>26</age>')
)
.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')
.then(({ body: versions }) => {
versions.map(v => v.conflict).should.eql([null, null, 'soft', '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();

// 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('<status>arrived</status>', '<status>checked in</status>')
)
.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)
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);

// Send fourth update
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fourth update from the branch (3rd update is missing). this will get held in the backlog and when it gets force-processed, it will take the latest version as the base version and therefore not have a conflict due to version mismatch...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

link to issue here and mention the scenario is mentioned there

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('<status>arrived</status>', '<status>departed</status>')
)
.set('Content-Type', 'application/xml')
.expect(200);

await exhaust(container);
await container.Entities.processBacklog(true);

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

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

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)
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>', '<age>26</age>')
)
.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')
.then(({ body: versions }) => {
versions.map(v => v.conflict).should.eql([null, null, 'soft', '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('<status>new</status>', '<status>arrived</status>')
)
.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('<status>new</status>', '<status>arrived</status>')
.replace('<age>20</age>', '<age>27</age>')
)
.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('<status>new</status>', '<status>arrived</status>')
.replace('<age>20</age>', '<age>27</age>')
)
.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);

Expand Down