Skip to content

add some DAL method support relation add data consistency #280

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

Merged
iceiilin merged 1 commit into from
Jun 21, 2017
Merged

add some DAL method support relation add data consistency #280

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
225 changes: 216 additions & 9 deletions lib/common/model.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,13 @@ function ModelFactory (
}
},

deSerialize: function(obj){
if(obj && obj._id && !obj.id) {
obj.id = obj._id.toString();
}
return obj;
},

publishRecord: function (event, record, id) {
if (!id && this.attributes && this.attributes.instanceId) {
id = record && record.instanceId;
Expand Down Expand Up @@ -160,7 +167,8 @@ function ModelFactory (
});

return indexes;
}).then(function(indexes) {
})
.then(function(indexes) {
if (_.isEmpty(indexes)) {
return;
}
Expand Down Expand Up @@ -212,6 +220,7 @@ function ModelFactory (
},

findMongo: function(query, options) {
var self = this;
return this.runNativeMongo('find', [query])
.then(function(cursor) {
if (options && options.limit) {
Expand All @@ -220,12 +229,20 @@ function ModelFactory (
if(options && options.sort) {
cursor = cursor.sort(options.sort);
}
return cursor.toArray();

return Promise.resolve(cursor.toArray());
Copy link
Member

@pengz1 pengz1 Jun 6, 2017

Choose a reason for hiding this comment

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

return cursor.toArray() equals return Promise.resolve but shorter.

})
.then(function(docs){
return _.map(docs, self.deSerialize);
});
},

findOneMongo: function(args) {
return this.runNativeMongo('findOne', args);
var self = this;
return this.runNativeMongo('findOne', args)
.then(function(doc){
return self.deSerialize(doc);
});
},

findAndModifyMongo: function() {
Expand All @@ -248,7 +265,10 @@ function ModelFactory (
if (update.$set) {
this.addWaterlineUpdatedAtToObject(update, '$set');
}
return self.runNativeMongo('findAndModify', _arguments);
return self.runNativeMongo('findAndModify', _arguments)
.then(function(doc){
return self.deSerialize(doc);
});
}

// Don't validate if we're setting/unsetting individual properties
Expand All @@ -257,15 +277,21 @@ function ModelFactory (
update.$set = {};
}
this.addWaterlineUpdatedAtToObject(update, '$set');
return self.runNativeMongo('findAndModify', _arguments);
return self.runNativeMongo('findAndModify', _arguments)
.then(function(doc){
return self.deSerialize(doc);
});
}

if (update.$pullAll) {
if (update.$addToSet) {
if (!update.$set) {
update.$set = {};
}
this.addWaterlineUpdatedAtToObject(update, '$set');
return self.runNativeMongo('findAndModify', _arguments);
return self.runNativeMongo('findAndModify', _arguments)
.then(function(doc){
return self.deSerialize(doc);
});
}

this.addWaterlineUpdatedAtToObject(update);
Expand All @@ -274,8 +300,10 @@ function ModelFactory (
return Promise.fromNode(self.validate.bind(this, update))
.then(function() {
return self.runNativeMongo('findAndModify', _arguments);
})
.then(function(doc){
return self.deSerialize(doc);
});

},

needOneMongo: function () {
Expand All @@ -301,12 +329,28 @@ function ModelFactory (

updateMongo: function(filter, update, options) {
var self = this;

if (update.$set || update.$unset) {
this.addWaterlineUpdatedAtToObject(update, '$set');
this.addWaterlineUpdatedAtToObject(update, '$unset');
return self.runNativeMongo('update', [filter, update, options]);
}

if (update.$pullAll) {
if (!update.$set) {
update.$set = {};
}
this.addWaterlineUpdatedAtToObject(update, '$set');
return self.runNativeMongo('update', [filter, update, options]);
}

if (update.$addToSet) {
if (!update.$set) {
update.$set = {};
}
this.addWaterlineUpdatedAtToObject(update, '$set');
return self.runNativeMongo('update', [filter, update, options]);
}

this.addWaterlineUpdatedAtToObject(update);

return Promise.fromNode(self.validate.bind(this, update))
Expand All @@ -315,6 +359,20 @@ function ModelFactory (
});
},

updateAndFindMongo: function(filter, update, options){
var self = this;

return self.updateMongo(filter, update, options)
.then(function(){
if (options && options.multi === true){
return self.findMongo(filter);
}
// If filter point to multiple doc and multi !== true
// Update will be set on a random node.
Copy link
Member

@pengz1 pengz1 Jun 1, 2017

Choose a reason for hiding this comment

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

Update on a random node sounds dangerous, you should confirm if it is harmless, otherwise you should forbid this random operation.

Copy link
Member Author

@changev changev Jun 2, 2017

Choose a reason for hiding this comment

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

I hope to discuss this issue with you if necessary

return self.findOneMongo(filter);
Copy link
Member

@iceiilin iceiilin May 31, 2017

Choose a reason for hiding this comment

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

will findOne() and update({options:{multi:false}}) operate on the same random doc?

Copy link
Member Author

@changev changev Jun 2, 2017

Choose a reason for hiding this comment

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

They truly have a chance to operate on the same random doc.
So are you recommended here to throw error or some thing else to prevent possible mistake?

Copy link
Member

@iceiilin iceiilin Jun 7, 2017

Choose a reason for hiding this comment

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

Is there any use case that user would only update a random doc? If not, I would prefer to remove this option for the upper layer.

Copy link
Member Author

@changev changev Jun 13, 2017

Choose a reason for hiding this comment

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

Do you mean if filter points to multi docs and mult !== true, just throw error?

});
},

removeMongo: function(query) {
return this.runNativeMongo('remove', [query]);
},
Expand Down Expand Up @@ -437,6 +495,7 @@ function ModelFactory (
}
});
},

/**
* Using an identifier, find document and delete one or more items
* from a list of this document, the values provided specifies the list and items.
Expand All @@ -452,6 +511,42 @@ function ModelFactory (
return self.removeListItemsById(record.id, values);
});
},

/**
* Using an identifier, find document and add one or more items
* to a list of this document, the values provided specifies the list and items.
* The signs is an object list, each sign related to one or more object items in [value].
* The sign means primer key of object for determining if the object exist in list already.
* Returns a waterline (bluebird) promise with the document find and modified
*
* @param {string} identifier
* @param {Object} values {field: [value]}
* @Param {Array} signs
* @returns {Promise}
*/
addListItemsIfNotExistByIdentifier: function (identifier, values, signs) {
var self = this;
return this.needByIdentifier(identifier).then(function (record) {
return self.addListItemsIfNotExistById(record.id, values, signs);
});
},

/**
* Using an id, find document and update fields only if they don't exist.
* Returns a waterline (bluebird) promise with the document find and modify.
*
* @param {string} id
* @param {String} field
* @param {*} values
* @returns {Promise}
*/
addFieldIfNotExistByIdentifier: function (identifier, field, values) {
var self = this;
return this.needByIdentifier(identifier).then(function (record) {
return self.addFieldIfNotExistById(record.id, field, values);
});
},

/**
* Extend a criteria object with additional waterline query paramters to
* find the single most recently updated object that matches the criteria.
Expand Down Expand Up @@ -552,6 +647,118 @@ function ModelFactory (
var update = {
$pullAll: values
};
var options = {new: true};
return self.updateAndFindMongo(query, update, options);
},


/**
* Using an Id, find document and add one or more items into a list of this document,
* the values provided specifies the list and items, suppose relation is a obj list,
* targets is a list in obj item of relations, then
* values = {"relations.0.targets", ["some string", {obj}, 123]}
* The sign means primer key of object for determining if the object exist in list already.
* suppose before modifying, the document is:
* {
* ...,
* list: [
* {a:1}
* ],
* ...
* }
* If users would like to add "item1", {a:1, b:2} and {c:3} as items in the "list",
* , the function's parameter "value (field: [item])" can be set as
* value = {list: ['item1', {a:1, b:2}, {c:3}]}
* If you just want to modify the first item in the list, "list.0" can be used as field.
* To prevent from recreating the item {a:1, b:2} if {a:1} is already in the original
* field, "signs" can be
signs = [{a:1}]
* in this case,
* the document will remain the same as the original one without adding anything.
* Returns a waterline (bluebird) promise with the document find and modify
*
* @param {string} identifier
* @param {Object} values {field: [value]}
* @Param {Array} signs
* @returns {Promise}
*/
addListItemsIfNotExistById: function (id, values, signs) {
var self = this;

var transformedValues = _
.transform(values, function(result, array, key) {
var withSign = {};
if (signs && signs.length !== 0){
withSign = _.transform(array, function(result, item){
if (typeof item !== "object"){
return true;
}
_.forEach(signs, function(sign){
if (_.some([item], sign)){
var obj = {};
obj[key] = {$each: [item]};
obj.sign = sign;
obj.field = key;
(result.vaulesWithSign || (result.vaulesWithSign = []))
.push(obj);
(result.objsWithSign || (result.objsWithSign = []))
.push(item);
}
});
}, {});
}

result.vaulesWithSign =
(result.vaulesWithSign || (result.vaulesWithSign = []))
.concat(withSign.vaulesWithSign || []);

(result.vaulesWithoutSign || (result.vaulesWithoutSign = {}))[key] =
{$each: _.difference(array, withSign.objsWithSign)};
}, {});

if (transformedValues.vaulesWithSign){
transformedValues = transformedValues.vaulesWithSign
.concat(transformedValues.vaulesWithoutSign);
}

Copy link
Member

@iceiilin iceiilin Jun 12, 2017

Choose a reason for hiding this comment

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

It would be more readable if you could help to elaborate some comments above, even the sample value of these local variables will help future coders to make some code adjustment.

return Promise.map(transformedValues, function(transformedValue){
var query = { _id: waterline.nodes.mongo.objectId(id) };
var update = {
$addToSet: transformedValue
};
var options = {new: true};
if(transformedValue.sign){
query[transformedValue.field] = {$not: {$elemMatch: transformedValue.sign}};
transformedValue = _.pick(transformedValue, transformedValue.field);
update = {
$addToSet: transformedValue
};
return self.findAndModifyMongo(query, {}, update, options);
}
return self.updateAndFindMongo(query, update, options);
Copy link
Member

@iceiilin iceiilin Jun 12, 2017

Choose a reason for hiding this comment

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

Why using findAndModify and update separately here?

Copy link
Member Author

@changev changev Jun 13, 2017

Choose a reason for hiding this comment

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

The query in "add list items of value type" doesn't need to know the information of current list.
So it's no ploblem without doc lock.

But "add list items of obj type" need to check the sign in exists list items, it must know the information of current list.

So a doc lock must be applied when query, findAndModifyMongo can helo us to achieve this.

}).then(function(modifiedDocs){
return _.last(modifiedDocs);
});
},
Copy link
Member

@iceiilin iceiilin Jun 8, 2017

Choose a reason for hiding this comment

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

A general question, if no modification to the document is made after a function in this file, is there a common return value, I mean, a promise of [] or the original document?

Copy link
Member Author

@changev changev Jun 13, 2017

Choose a reason for hiding this comment

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

As long as the findAndModifyMongo is executed, though no modification the original doc will be return.

If query can't find any doc , a Promise.resolve() will be return.

If parameters is invalid, a Promise.reject() will be return.


/**
* Using an id, find document and update fields only if they don't exist.
* Returns a waterline (bluebird) promise with the document find and modify.
*
* @param {string} id
* @param {String} field
* @param {Object/String/Number etc.} values
* @returns {Promise}
*/
addFieldIfNotExistById: function (id, field, values) {
var self = this;

var query = { _id: waterline.nodes.mongo.objectId(id)};
query[field] = {$exists : false};

var update = { $set: {} };
update.$set[field] = values;

var options = {new: true};
Copy link
Member

@iceiilin iceiilin May 31, 2017

Choose a reason for hiding this comment

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

is "IfNotExist" necessary here? Can this function do the following things: 1) if the field doesn't exist, add it; 2) if it is already in the doc, return the original doc? In this case the callers don't need to check the field's existence by themselves in the upper layer.

Copy link
Member Author

@changev changev Jun 2, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

  1. Using this function the callers don't need to check the field's existence by themselves in the upper layer. If it doesn't exist, the function will create the field with vaule. If it already exist, the function will return nothing. This function can make sure the field indeed exist after it is called.

  2. "IfNotExist" must be contained in mongo query, otherwise the data consistency can be promised.

  3. About the two following things above: if the field doesn't exist, add it how can we judge it exist or not? FindbyID and then check the obj is not atomic. See 2

Copy link
Member

@iceiilin iceiilin Jun 8, 2017

Choose a reason for hiding this comment

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

I mean could we just remove "IfNotExist" in the function name for a shorter one, similar as https://github.com/RackHD/on-core/blob/master/lib/common/model.js#L549 to keep it consistent

Copy link
Member Author

@changev changev Jun 13, 2017

Choose a reason for hiding this comment

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

If user want to add {fieldA： valueA} to a doc but fieldA already exists, then the vaule of fieldA won't be changed to valueA.

Without "IfNotExist", the method looks more likely to change the value to valueA.

return self.findAndModifyMongo(query, {}, update, options);
},
Expand Down
16 changes: 6 additions & 10 deletions lib/models/work-item.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,19 +214,15 @@ function WorkItemModelFactory (
findPollers: function findPollers(criteria) {
var self = this;
return taskGraphStore.getPollers(criteria)
.then(function(pollers) {
if (!Array.isArray(pollers)) {
pollers = Array.prototype.slice.call(arguments, 1);
}
return _.map(pollers, self.deserialize, self);
});
.then(function(pollers) {
if (!Array.isArray(pollers)) {
pollers = Array.prototype.slice.call(arguments, 1);
}
return _.map(pollers, self.deserialize, self);
});
},

deserialize: function(obj) {
if(obj._id && !obj.id) {
obj.id = obj._id.toString();
delete obj._id;
}
if(obj.node && typeof obj.node === 'object') {
obj.node = obj.node.toString();
}
Expand Down
Loading