Skip to content

Commit

Permalink
fix edge case in versioning
Browse files Browse the repository at this point in the history 
handle case:
client one uses push/addToSet/pushAll on an array while
client two attempts a $set of the array.
  • Loading branch information
@aheckmann
aheckmann committed Jun 27, 2012
1 parent f287ce4 commit fb5fe46
Show file tree
Hide file tree
Showing 2 changed files with 46 additions and 13 deletions.
24 changes: 17 additions & 7 deletions lib/model.js
View file
Expand Up @@ -17,7 +17,8 @@ var Document = require('./document')
, tick = utils.tick

var VERSION_WHERE = 1
, VERSION_INC = 2;
, VERSION_INC = 2
, VERSION_ALL = VERSION_WHERE | VERSION_INC;

/**
* Model constructor
Expand Down Expand Up @@ -385,14 +386,17 @@ function operand (self, where, delta, data, val, op) {
if (false === self.schema.options.versionKey) return;

// already marked for versioning?
if (VERSION_INC === self.__version) return;
if (VERSION_ALL === (VERSION_ALL & self.__version)) return;

switch (op) {
case '$set':
case '$unset':
case '$pop':
case '$pull':
case '$pullAll':
case '$push':
case '$pushAll':
case '$addToSet':
break;
default:
// nothing to do
Expand All @@ -404,7 +408,10 @@ function operand (self, where, delta, data, val, op) {
// only increment the version if an array position changes.
// modifying elements of an array is ok if position does not change.

if (/^\$p/.test(op)) {
if ('$push' == op || '$pushAll' == op || '$addToSet' == op) {
self.__version = VERSION_INC;
}
else if (/^\$p/.test(op)) {
// potentially changing array positions
self.increment();
}
Expand Down Expand Up @@ -549,14 +556,17 @@ Model.prototype._version = function _version (where, delta) {
// there is no way to select the correct version. we could fail
// fast here and force them to include the versionKey but
// thats a bit intrusive. can we do this automatically?
// make that an option to fail fast.
// TODO fail fast option?
if (!this.isSelected(key)) {
return;
}

where[key] = this.getValue(key);
// $push $addToSet don't need the where clause set
if (VERSION_WHERE === (VERSION_WHERE & this.__version)) {
where[key] = this.getValue(key);
}

if (VERSION_INC === this.__version) {
if (VERSION_INC === (VERSION_INC & this.__version)) {
delta.$inc || (delta.$inc = {});
delta.$inc[key] = 1;
}
Expand All @@ -572,7 +582,7 @@ Model.prototype._version = function _version (where, delta) {
*/

Model.prototype.increment = function increment () {
this.__version = VERSION_INC;
this.__version = VERSION_ALL;
return this;
}

Expand Down
35 changes: 29 additions & 6 deletions test/versioning.test.js
View file
Expand Up @@ -103,6 +103,7 @@ describe('versioning', function(){
function test2 (err, a, b) {
assert.ifError(err);
assert.equal(a.meta.numbers.length, 5);
assert.equal(a._doc.__v, 2)
a.meta.numbers.pull(10);
b.meta.numbers.push(20);
save(a, b, test3);
Expand All @@ -114,6 +115,7 @@ describe('versioning', function(){
assert.equal(b.meta.numbers.length, 5);
assert.equal(-1, a.meta.numbers.indexOf(10));
assert.ok(~a.meta.numbers.indexOf(20));
assert.equal(a._doc.__v, 4)

a.numbers.pull(3, 20);

Expand All @@ -124,15 +126,18 @@ describe('versioning', function(){

function test4 (err, a, b) {
assert.ok(/No matching document/.test(err), err);
assert.equal(a._doc.__v, 5)
a.set('arr.0.0', 'updated');
var d = a._delta()[0];
assert.equal(a._doc.__v, d.__v, 'version should be added to where clause')
var d = a._delta();
assert.equal(a._doc.__v, d[0].__v, 'version should be added to where clause')
assert.ok(!('$inc' in d[1]));
save(a,b,test5);
}

function test5 (err, a, b) {
assert.ifError(err);
assert.equal('updated', a.arr[0][0]);
assert.equal(a._doc.__v, 5);
a.set('arr.0', 'not an array');
// should overwrite a's changes, last write wins
b.arr.pull(10);
Expand All @@ -145,6 +150,7 @@ describe('versioning', function(){
assert.equal(a.arr.length, 2);
assert.equal('updated', a.arr[0][0]);
assert.equal('using set', a.arr[1]);
assert.equal(a._doc.__v, 6)
b.set('arr.0', 'not an array');
// should overwrite b's changes, last write wins
// force a $set
Expand All @@ -159,6 +165,7 @@ describe('versioning', function(){
assert.equal(a.arr.length, 2);
assert.equal('updated', a.arr[0][0]);
assert.equal('woot', a.arr[1]);
assert.equal(a._doc.__v, 7)
a.meta.nested.$pop();
b.meta.nested.$pop();
save(a, b, test8);
Expand All @@ -167,6 +174,7 @@ describe('versioning', function(){
function test8 (err, a, b) {
assert.ok(/No matching document/.test(err), 'changes to b should not be applied');
assert.equal(a.meta.nested.length, 3);
assert.equal(a._doc.__v, 8)
a.meta.nested.push({ title: 'the' });
a.meta.nested.push({ title: 'killing' });
b.meta.nested.push({ title: 'biutiful' });
Expand All @@ -176,6 +184,7 @@ describe('versioning', function(){
function test9 (err, a, b) {
assert.ifError(err);
assert.equal(6, a.meta.nested.length);
assert.equal(a._doc.__v, 10)
// nested subdoc property changes should not trigger version increments
a.meta.nested[2].title = 'two';
b.meta.nested[0].title = 'zero';
Expand All @@ -188,7 +197,7 @@ describe('versioning', function(){
assert.equal('two', b.meta.nested[2].title);
assert.equal('zero', b.meta.nested[0].title);
assert.equal('sub one', b.meta.nested[1].comments[0].title);

assert.equal(a._doc.__v, 10)
assert.equal(3, a.mixed.arr.length);
a.mixed.arr.push([10],{x:1},'woot');
a.markModified('mixed.arr');
Expand All @@ -197,23 +206,37 @@ describe('versioning', function(){

function test11 (err, a, b) {
assert.ifError(err);
assert.equal(a._doc.__v, 6)
assert.equal(a._doc.__v, 11)
assert.equal(6, a.mixed.arr.length);
assert.equal(1, a.mixed.arr[4].x)
assert.equal('woot', a.mixed.arr[5])
assert.equal(10, a.mixed.arr[3][0])

a.comments.addToSet({ title: 'monkey' });
b.markModified('comments');

var d = b._delta();
assert.ok(d[1].$inc, 'a $set of an array should trigger versioning');

save(a, b, test12);
}

function test12 (err, a, b) {
assert.ok(/No matching document/.test(err), 'changes to b should not be applied');
assert.equal(5, a.comments.length);

a.comments.addToSet({ title: 'aven' });
a.comments.addToSet({ title: 'avengers' });
var d = a._delta();
assert.equal(undefined, d[0].__v, 'version should not be included');

assert.equal(undefined, d[0].__v, 'version should not be included in where clause');
assert.ok(!d[1].$set);
assert.ok(d[1].$addToSet);
assert.ok(d[1].$addToSet.comments);

a.comments.$shift();
var d = a._delta();
assert.equal(6, d[0].__v, 'version should be included in where clause');
assert.equal(12, d[0].__v, 'version should be included in where clause');
assert.ok(d[1].$set, 'two differing atomic ops on same path should create a $set');
assert.ok(d[1].$inc, 'a $set of an array should trigger versioning');
assert.ok(!d[1].$addToSet);
Expand Down

0 comments on commit fb5fe46

Please sign in to comment.