add some DAL method support relation add data consistency #280

changev · 2017-05-26T11:22:37Z

Modfied findMongo, findAndModifyMongo, findOneMongo to return docs in normal format.
Add updateAndFindMongo to return modified doc(s) after update.
Add

addFieldIfNotExistById,
addFieldIfNotExistByIdentifier, 
addListItemsIfNotExistById,
addListItemsIfNotExistByIdentifier,

For support some data consistency needs.

iceiilin · 2017-05-31T07:55:11Z

lib/common/model.js

+
+                return Promise.resolve(cursor.toArray()).then(function(docs){
+                    return _.map(docs, self.deSerialize);
+                });


Could it only be "return _.map(cursor.toArray, self.deSerialize)"? I assume "then" will always return a promise, and if toArray() returns an empty array, _.map() will return as it is.

Agree with Lyne, and also if Promise required, return Promise.map(cursor.toArray, self.deSerialize) should be more clean.

cursor.toArray() may be null
Promise.map(null, ....) will be rejected, but here when find nothing we just want to return [].
And also cursor.toArray() only can be executed only once and then cursor will be exhausted.
So also can't use

if(cursor.toArray()){ Promise.map(cursor.toArray, self.deSerialize) }

iceiilin · 2017-05-31T08:28:21Z

lib/common/model.js

-                return self.runNativeMongo('findAndModify', _arguments);
+                return self.runNativeMongo('findAndModify', _arguments).then(function(doc){
+                    return doc ? self.deSerialize(doc) : doc;
+                });


Could it be possible to change doc null validation to deSerialize()? that would save the efforts every time when calling it.

!Excellent idea, thanks!

iceiilin · 2017-05-31T08:30:37Z

lib/common/model.js

+                if (!update.$set) {
+                    update.$set = {};
+                }
+                //this.addWaterlineUpdatedAtToObject(update, '$set');


Why is this line unnecessary here?

Sry, I forgot why I add // before this line.

iceiilin · 2017-05-31T08:39:17Z

lib/common/model.js

+                }
+                // If filter point to multiple doc and multi !== true
+                // Update will be set on a random node.
+                return self.findOneMongo(filter);


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

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?

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.

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

iceiilin · 2017-05-31T08:48:42Z

lib/common/model.js

+         */
+        addListItemsIfNotExistById: function (id, values, signs) {
+            var self = this;
+


lodash has already been required using DI at the beginning of this file

Sry this is just for local debug. I'll remove it

iceiilin · 2017-05-31T08:49:10Z

lib/common/model.js

+            var transformedValues = require("lodash")
+            .transform(values, function(result, array, key) {
+                var withSign = {};
+                if (signs && signs.length !== 0){


same as above

iceiilin · 2017-05-31T08:57:23Z

lib/common/model.js

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


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.

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.

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

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

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

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.

iceiilin · 2017-05-31T09:03:53Z

lib/common/model.js

+         * from 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 modify


Could you please elaborate more on this description? or using a simple example? It is a little difficult to understand.

pengz1 · 2017-06-01T06:05:07Z

lib/common/model.js

+
+                return Promise.resolve(cursor.toArray()).then(function(docs){
+                    return _.map(docs, self.deSerialize);
+                });


Agree with Lyne, and also if Promise required, return Promise.map(cursor.toArray, self.deSerialize) should be more clean.

pengz1 · 2017-06-01T06:09:58Z

lib/common/model.js

                }
-                return cursor.toArray();
+
+                return Promise.resolve(cursor.toArray()).then(function(docs){


Just a style suggestion, change a new line before .then and all the other same lines below.

pengz1 · 2017-06-01T06:31:15Z

lib/common/model.js

            return Promise.fromNode(self.validate.bind(this, update))
            .then(function() {
-                return self.runNativeMongo('findAndModify', _arguments);
+                return self.runNativeMongo('findAndModify', _arguments).then(function(doc){


If you want, you can move all the then() to be after above then() not inside it.

pengz1 · 2017-06-01T06:56:19Z

lib/common/model.js

+                if (!update.$set) {
+                    update.$set = {};
+                }
+                //this.addWaterlineUpdatedAtToObject(update, '$set');


Same question, I expect to use this $addToSet in Warnado story.

pengz1 · 2017-06-01T07:31:57Z

lib/common/model.js

+                    return self.findMongo(filter);
+                }
+                // If filter point to multiple doc and multi !== true
+                // Update will be set on a random node.


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

I hope to discuss this issue with you if necessary

pengz1 · 2017-06-01T07:48:39Z

lib/common/model.js

+         * @param {Object} values {field: [value]}
+         * @param {Object} signs [{sign}]
+         * @returns {Promise}
+         */


This method looks too complicated, why just use $addToSet or $push?

pengz1 · 2017-06-01T07:50:11Z

lib/common/model.js

+         *
+         * @param {string} identifier
+         * @param {Object} values {field: [value]}
+         * @param {Object} signs [{sign}]


@param {Array} signs

pengz1 · 2017-06-01T07:50:20Z

lib/common/model.js

+         * Returns a waterline (bluebird) promise with the document find and modify
+         *
+         * @param {string} identifier
+         * @param {Object} values {field: [value]}


@param {Array} signs

pengz1 · 2017-06-01T07:51:10Z

lib/common/model.js

+         * Using an Id, find document and add one or more items 
+         * from 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.


found and modified

pengz1 · 2017-06-01T07:54:28Z

lib/common/model.js

+         * Returns a waterline (bluebird) promise with the document find and modify
+         *
+         * @param {string} identifier
+         * @param {Object} values {field: [value]}


Without example, I can't understand how sign works and what it looks like. If we heavily depends on sign, suggest to create a new function dedicated for it.

iceiilin

Complicated work here. Some personal thoughts. No hurry to adjust the code, I will need more time to think about addListItemsIfNotExistById.

iceiilin · 2017-06-08T06:46:21Z

lib/common/model.js

+         * we don't want to add another obj of type A,
+         * how can we know the item of type: A is already exist? the para sign help to do this，
+         * set sign =  [{type:A}] and it can prevent to add another obj of type A. 
+         * Instead you can use value = {list[0].value: 2} to add value 2 into type A obj.


I think using the following format might be more readable for users than literal description. Actually the example in your test case is a great example.
"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.
"
you could modify the whole example to be more general like stated in the original description, but please make sure it is clear and concrete enough for users.

iceiilin · 2017-06-08T06:59:04Z

spec/lib/common/model-spec.js

+        });
+
+
+        describe('by Id with addListItemsIfNotExistById()', function () {


suggest to modify the description as a complete sentence for easier understanding

I found the text of nested descript will be joined together in the unit test log.
So this describe + parent describe is a complete sentence.

Otherwise the sentence in log will be duplicated

iceiilin · 2017-06-08T09:06:16Z

spec/lib/common/model-spec.js


-        describe('remove list items by bad identifier with removeListItemsByIdentifier()',
+        describe('by bad value with addListItemsIfNotExistById()',
+            function () {


this line can be moved one line above

iceiilin · 2017-06-08T09:30:06Z

lib/common/model.js

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


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

iceiilin · 2017-06-08T10:06:40Z

lib/common/model.js

+                return self.updateAndFindMongo(query, update, options);
+            }).then(function(modifiedDocs){
+                return _.last(modifiedDocs);
+            });


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?

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.

iceiilin · 2017-06-12T06:30:04Z

lib/common/model.js

+                        $addToSet: transformedValue
+                    };
+                    return self.findAndModifyMongo(query, {}, update, options);
+                }


Why using findAndModify and update separately here?

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.

iceiilin · 2017-06-12T06:32:46Z

lib/common/model.js

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


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.

iceiilin · 2017-06-16T06:51:30Z

lib/common/model.js

+         * 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 signs is an object list, each sign related to one or more object items in [value].


The above description is a little duplicate with examples below.

iceiilin · 2017-06-16T06:51:56Z

lib/common/model.js

+               signs = [{a:1}]
+         *   in this case, 
+         *   the document will remain the same as the original one without adding anything.
+         * 


pls. help to remove extra empty line or space in this paragraph.

pengz1 · 2017-06-06T06:41:48Z

lib/common/model.js

                }
-                return cursor.toArray();
+
+                return Promise.resolve(cursor.toArray());


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

changev force-pushed the bugfix/enclosure-update-data-consistency branch 2 times, most recently from 0824a77 to 1604615 Compare May 26, 2017 12:02

changev mentioned this pull request May 26, 2017

keep data consistency when add realtions RackHD/on-http#658

Merged

iceiilin reviewed May 31, 2017

View reviewed changes

pengz1 reviewed Jun 1, 2017

View reviewed changes

changev mentioned this pull request Jun 2, 2017

keep date consistency when generate enclosure RackHD/on-tasks#455

Merged

changev force-pushed the bugfix/enclosure-update-data-consistency branch 9 times, most recently from 040463a to bac49f3 Compare June 7, 2017 10:24

iceiilin reviewed Jun 8, 2017

View reviewed changes

iceiilin reviewed Jun 12, 2017

View reviewed changes

changev force-pushed the bugfix/enclosure-update-data-consistency branch 2 times, most recently from c52e62f to bb32b50 Compare June 14, 2017 10:48

RackHD deleted a comment from changev Jun 15, 2017

changev force-pushed the bugfix/enclosure-update-data-consistency branch 4 times, most recently from 642f4ad to f39e32e Compare June 15, 2017 15:43

iceiilin reviewed Jun 19, 2017

View reviewed changes

add some DAL method support relation add data consistency

6c28ec6

RackHD deleted a comment from changev Jun 20, 2017

RackHD deleted a comment from houndci-bot Jun 20, 2017

RackHD deleted a comment from changev Jun 20, 2017

changev force-pushed the bugfix/enclosure-update-data-consistency branch from f39e32e to 6c28ec6 Compare June 20, 2017 06:04

pengz1 approved these changes Jun 21, 2017

View reviewed changes

iceiilin approved these changes Jun 21, 2017

View reviewed changes

iceiilin merged commit 77328d1 into RackHD:master Jun 21, 2017

		});


		describe('by Id with addListItemsIfNotExistById()', function () {

add some DAL method support relation add data consistency #280

add some DAL method support relation add data consistency #280

Uh oh!

Conversation

changev commented May 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

changev Jun 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

changev Jun 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iceiilin left a comment

Choose a reason for hiding this comment

Uh oh!

iceiilin Jun 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

changev commented May 26, 2017 •

edited

Loading

changev Jun 2, 2017 •

edited

Loading

changev Jun 2, 2017 •

edited

Loading

iceiilin Jun 8, 2017 •

edited

Loading

changev Jun 13, 2017 •

edited

Loading