From 9f81a591e1a37806ac93ce5866814b27b875811a Mon Sep 17 00:00:00 2001
From: Nick O'Leary <nick.oleary@gmail.com>
Date: Mon, 23 Jul 2018 13:27:43 +0100
Subject: [PATCH] Move multiple-get/set logic into individual context stores

---
 red/runtime/nodes/context/index.js            | 130 ++++++------------
 red/runtime/nodes/context/localfilesystem.js  |  57 +++++---
 red/runtime/nodes/context/memory.js           |  77 ++++++++---
 test/red/runtime/nodes/context/index_spec.js  |  20 ++-
 .../nodes/context/localfilesystem_spec.js     |  59 +++++++-
 test/red/runtime/nodes/context/memory_spec.js |  70 +++++++++-
 6 files changed, 262 insertions(+), 151 deletions(-)

diff --git a/red/runtime/nodes/context/index.js b/red/runtime/nodes/context/index.js
index cb61ca187..91cbf60c4 100644
--- a/red/runtime/nodes/context/index.js
+++ b/red/runtime/nodes/context/index.js
@@ -195,9 +195,24 @@ function createContext(id,seed) {
     var scope = id;
     var obj = seed || {};
     var seedKeys;
+    var insertSeedValues;
     if (seed) {
         seedKeys = Object.keys(seed);
+        insertSeedValues = function(keys,values) {
+            if (!Array.isArray(keys)) {
+                if (values[0] === undefined) {
+                    values[0] = seed[keys];
+                }
+            } else {
+                for (var i=0;i<keys.length;i++) {
+                    if (values[i] === undefined) {
+                        values[i] = seed[keys[i]];
+                    }
+                }
+            }
+        }
     }
+
     obj.get = function(key, storage, callback) {
         var context;
         if (!storage && !callback) {
@@ -212,71 +227,33 @@ function createContext(id,seed) {
             }
             context = getContextStorage(storage);
         }
-        if (seed) {
-            // Get the value from the underlying store. If it is undefined,
-            // check the seed for a default value.
-            if (callback) {
-                if (!Array.isArray(key)) {
-                    context.get(scope,key,function(err, v) {
-                        if (v === undefined) {
-                            callback(err, seed[key]);
-                        } else {
-                            callback(err, v);
-                        }
-                    });
-                } else {
-                    // If key is an array, get the value of each key.
-                    var storeValues = [];
-                    var keys = key.slice();
-                    var _key = keys.shift();
-                    var cb = function(err, v) {
-                        if (err) {
-                            callback(err);
-                        } else {
-                            if (v === undefined) {
-                                storeValues.push(seed[_key]);
-                            } else {
-                                storeValues.push(v);
-                            }
-                            if (keys.length === 0) {
-                                callback.apply(null, [null].concat(storeValues));
-                            } else {
-                                _key = keys.shift();
-                                context.get(scope, _key, cb);
-                            }
-                        }
-                    };
-                    context.get(scope, _key, cb);
-                }
+        if (callback) {
+            if (!seed) {
+                context.get(scope,key,callback);
             } else {
-                // No callback, attempt to do this synchronously
-                var storeValue = context.get(scope,key);
-                if (storeValue === undefined) {
-                    return seed[key];
-                } else {
-                    return storeValue;
-                }
+                context.get(scope,key,function() {
+                    if (arguments[0]) {
+                        callback(arguments[0]);
+                        return;
+                    }
+                    var results = Array.prototype.slice.call(arguments,[1]);
+                    insertSeedValues(key,results);
+                    // Put the err arg back
+                    results.unshift(undefined);
+                    callback.apply(null,results);
+                });
             }
         } else {
-            if (!Array.isArray(key)) {
-                return context.get(scope, key, callback);
-            } else {
-                var storeValues = [];
-                var keys = key.slice();
-                var cb = function(err, v) {
-                    if (err) {
-                        callback(err);
-                    } else {
-                        storeValues.push(v);
-                        if (keys.length === 0) {
-                            callback.apply(null, [null].concat(storeValues));
-                        } else {
-                            context.get(scope, keys.shift(), cb);
-                        }
-                    }
-                };
-                context.get(scope, keys.shift(), cb);
+            // No callback, attempt to do this synchronously
+            var results = context.get(scope,key);
+            if (seed) {
+                if (Array.isArray(key)) {
+                    insertSeedValues(key,results);
+                } else if (results === undefined){
+                    results = seed[key];
+                }
             }
+            return results;
         }
     };
     obj.set = function(key, value, storage, callback) {
@@ -293,36 +270,7 @@ function createContext(id,seed) {
             }
             context = getContextStorage(storage);
         }
-        if (!Array.isArray(key)) {
-            context.set(scope, key, value, callback);
-        } else {
-            // If key is an array, set each key-value pair.
-            // If the value array is longer than the key array, then the extra values are ignored.
-            var index = 0;
-            var values = [].concat(value); // Convert the value to an array
-            var cb = function(err) {
-                if (err) {
-                    if (callback) {
-                        callback(err);
-                    }
-                } else {
-                    index++;
-                    if (index === key.length) {
-                        if (callback) {
-                            callback(null);
-                        }
-                    } else {
-                        if(index < values.length) {
-                            context.set(scope, key[index], values[index], cb);
-                        } else {
-                            // If the value array is shorter than the key array, use null for missing values.
-                            context.set(scope, key[index], null, cb);
-                        }
-                    }
-                }
-            };
-            context.set(scope, key[index], values[index], cb);
-        }
+        context.set(scope, key, value, callback);
     };
     obj.keys = function(storage, callback) {
         var context;
diff --git a/red/runtime/nodes/context/localfilesystem.js b/red/runtime/nodes/context/localfilesystem.js
index 74e1f30c0..135cbae77 100644
--- a/red/runtime/nodes/context/localfilesystem.js
+++ b/red/runtime/nodes/context/localfilesystem.js
@@ -169,7 +169,16 @@ LocalFileSystem.prototype.get = function(scope, key, callback) {
     var storagePath = getStoragePath(this.storageBaseDir ,scope);
     loadFile(storagePath + ".json").then(function(data){
         if(data){
-            callback(null, util.getMessageProperty(JSON.parse(data),key));
+            data = JSON.parse(data);
+            if (!Array.isArray(key)) {
+                callback(null, util.getMessageProperty(data,key));
+            } else {
+                var results = [undefined];
+                for (var i=0;i<key.length;i++) {
+                    results.push(util.getMessageProperty(data,key[i]))
+                }
+                callback.apply(null,results);
+            }
         }else{
             callback(null, undefined);
         }
@@ -178,35 +187,43 @@ LocalFileSystem.prototype.get = function(scope, key, callback) {
     });
 };
 
-LocalFileSystem.prototype._set = function(scope, key, value, callback) {
-    var storagePath = getStoragePath(this.storageBaseDir ,scope);
-    loadFile(storagePath + ".json").then(function(data){
-        var obj = data ? JSON.parse(data) : {}
-        util.setMessageProperty(obj,key,value);
-        return fs.outputFile(storagePath + ".json", JSON.stringify(obj, undefined, 4), "utf8");
-    }).then(function(){
-        if(typeof callback === "function"){
-            callback(null);
-        }
-    }).catch(function(err){
-        if(typeof callback === "function"){
-            callback(err);
-        }
-    });
-}
-
 LocalFileSystem.prototype.set = function(scope, key, value, callback) {
+    var storagePath = getStoragePath(this.storageBaseDir ,scope);
     if (this.cache) {
         this.cache.set(scope,key,value,callback);
         // With cache enabled, no need to re-read the file prior to writing.
         var newContext = this.cache._export()[scope];
-        var storagePath = getStoragePath(this.storageBaseDir ,scope);
         fs.outputFile(storagePath + ".json", JSON.stringify(newContext, undefined, 4), "utf8").catch(function(err) {
         });
     } else if (callback && typeof callback !== 'function') {
         throw new Error("Callback must be a function");
     } else {
-        this._set(scope,key,value,callback);
+        loadFile(storagePath + ".json").then(function(data){
+            var obj = data ? JSON.parse(data) : {}
+            if (!Array.isArray(key)) {
+                key = [key];
+                value = [value];
+            } else if (!Array.isArray(value)) {
+                // key is an array, but value is not - wrap it as an array
+                value = [value];
+            }
+            for (var i=0;i<key.length;i++) {
+                var v = null;
+                if (i<value.length) {
+                    v = value[i];
+                }
+                util.setMessageProperty(obj,key[i],v);
+            }
+            return fs.outputFile(storagePath + ".json", JSON.stringify(obj, undefined, 4), "utf8");
+        }).then(function(){
+            if(typeof callback === "function"){
+                callback(null);
+            }
+        }).catch(function(err){
+            if(typeof callback === "function"){
+                callback(err);
+            }
+        });
     }
 };
 
diff --git a/red/runtime/nodes/context/memory.js b/red/runtime/nodes/context/memory.js
index 1f76a1520..1100d323c 100644
--- a/red/runtime/nodes/context/memory.js
+++ b/red/runtime/nodes/context/memory.js
@@ -28,47 +28,84 @@ Memory.prototype.close = function(){
     return Promise.resolve();
 };
 
+Memory.prototype._getOne = function(scope, key) {
+    var value;
+    var error;
+    if(this.data[scope]){
+        value = util.getMessageProperty(this.data[scope], key);
+    }
+    return value;
+}
+
 Memory.prototype.get = function(scope, key, callback) {
     var value;
     var error;
-    try{
-        if(this.data[scope]){
-            value = util.getMessageProperty(this.data[scope], key);
-        }
-    }catch(err){
-        if(callback){
+    if (!Array.isArray(key)) {
+        try {
+            value = this._getOne(scope,key);
+        } catch(err) {
+            if (!callback) {
+                throw err;
+            }
             error = err;
-        }else{
-            throw err;
+        }
+        if (callback) {
+            callback(error,value);
+            return;
+        } else {
+            return value;
         }
     }
-    if(callback){
-        if(error){
-            callback(error);
-        } else {
-            callback(null, value);
+
+    value = [];
+    for (var i=0; i<key.length; i++) {
+        try {
+            value.push(this._getOne(scope,key[i]));
+        } catch(err) {
+            if (!callback) {
+                throw err;
+            } else {
+                callback(err);
+                return;
+            }
         }
+    }
+    if (callback) {
+        callback.apply(null, [undefined].concat(value));
     } else {
         return value;
     }
 };
 
-Memory.prototype.set =function(scope, key, value, callback) {
+Memory.prototype.set = function(scope, key, value, callback) {
     if(!this.data[scope]){
         this.data[scope] = {};
     }
     var error;
-    try{
-        util.setMessageProperty(this.data[scope],key,value);
-    }catch(err){
-        if(callback){
+    if (!Array.isArray(key)) {
+        key = [key];
+        value = [value];
+    } else if (!Array.isArray(value)) {
+        // key is an array, but value is not - wrap it as an array
+        value = [value];
+    }
+    try {
+        for (var i=0; i<key.length; i++) {
+            var v = null;
+            if (i < value.length) {
+                v = value[i];
+            }
+            util.setMessageProperty(this.data[scope],key[i],v);
+        }
+    } catch(err) {
+        if (callback) {
             error = err;
-        }else{
+        } else {
             throw err;
         }
     }
     if(callback){
-        callback(error || null);
+        callback(error);
     }
 };
 
diff --git a/test/red/runtime/nodes/context/index_spec.js b/test/red/runtime/nodes/context/index_spec.js
index 693b9be16..68e6b3d9e 100644
--- a/test/red/runtime/nodes/context/index_spec.js
+++ b/test/red/runtime/nodes/context/index_spec.js
@@ -386,7 +386,7 @@ describe('context', function() {
                     context.get("foo","test",cb);
                     context.keys("test",cb);
                     stubSet.calledWithExactly("1:flow","foo","bar",cb).should.be.true();
-                    stubGet.calledWithExactly("1:flow","foo",cb).should.be.true();
+                    stubGet.calledWith("1:flow","foo").should.be.true();
                     stubKeys.calledWithExactly("1:flow",cb).should.be.true();
                     done();
                 }).catch(done);
@@ -400,7 +400,7 @@ describe('context', function() {
                     context.flow.get("foo","test",cb);
                     context.flow.keys("test",cb);
                     stubSet.calledWithExactly("flow","foo","bar",cb).should.be.true();
-                    stubGet.calledWithExactly("flow","foo",cb).should.be.true();
+                    stubGet.calledWith("flow","foo").should.be.true();
                     stubKeys.calledWithExactly("flow",cb).should.be.true();
                     done();
                 }).catch(done);
@@ -431,7 +431,7 @@ describe('context', function() {
                     stubSet.called.should.be.false();
                     stubKeys.called.should.be.false();
                     stubSet2.calledWithExactly("1:flow","foo","bar",cb).should.be.true();
-                    stubGet2.calledWithExactly("1:flow","foo",cb).should.be.true();
+                    stubGet2.calledWith("1:flow","foo").should.be.true();
                     stubKeys2.calledWithExactly("1:flow",cb).should.be.true();
                     done();
                 }).catch(done);
@@ -448,7 +448,7 @@ describe('context', function() {
                     stubSet.called.should.be.false();
                     stubKeys.called.should.be.false();
                     stubSet2.calledWithExactly("1:flow","foo","bar",cb).should.be.true();
-                    stubGet2.calledWithExactly("1:flow","foo",cb).should.be.true();
+                    stubGet2.calledWith("1:flow","foo").should.be.true();
                     stubKeys2.calledWithExactly("1:flow",cb).should.be.true();
                     done();
                 }).catch(done);
@@ -465,7 +465,7 @@ describe('context', function() {
                     stubSet.called.should.be.false();
                     stubKeys.called.should.be.false();
                     stubSet2.calledWithExactly("1:flow","foo","alias",cb).should.be.true();
-                    stubGet2.calledWithExactly("1:flow","foo",cb).should.be.true();
+                    stubGet2.calledWith("1:flow","foo").should.be.true();
                     stubKeys2.calledWithExactly("1:flow",cb).should.be.true();
                     done();
                 }).catch(done);
@@ -479,7 +479,7 @@ describe('context', function() {
                     context.get("foo",cb);
                     context.keys(cb);
                     stubSet.calledWithExactly("1:flow","foo","alias",cb).should.be.true();
-                    stubGet.calledWithExactly("1:flow","foo",cb).should.be.true();
+                    stubGet.calledWith("1:flow","foo").should.be.true();
                     stubKeys.calledWithExactly("1:flow",cb).should.be.true();
                     done();
                 }).catch(done);
@@ -595,9 +595,7 @@ describe('context', function() {
 
             it('should return an error if an error occurs in getting multiple store values', function(done) {
                 Context.init({contextStorage:contextStorage});
-                stubGet.onFirstCall().callsArgWith(2, null, "bar1");
-                stubGet.onSecondCall().callsArgWith(2, "error2");
-                stubGet.onThirdCall().callsArgWith(2, null, "bar3");
+                stubGet.onFirstCall().callsArgWith(2, "error2", "bar1");
                 Context.load().then(function(){
                     var context =  Context.get("1","flow");
                     context.global.get(["foo1","foo2","foo3"], "memory", function(err,foo1,foo2,foo3){
@@ -771,9 +769,7 @@ describe('context', function() {
 
             it('should return an error if an error occurs in storing multiple values', function(done) {
                 Context.init({contextStorage:contextStorage});
-                stubSet.onFirstCall().callsArgWith(3, null);
-                stubSet.onSecondCall().callsArgWith(3, "error2");
-                stubSet.onThirdCall().callsArgWith(3, null);
+                stubSet.onFirstCall().callsArgWith(3, "error2");
                 Context.load().then(function(){
                     var context =  Context.get("1","flow");
                     context.set(["foo1","foo2","foo3"], ["bar1","bar2","bar3"], "memory", function(err){
diff --git a/test/red/runtime/nodes/context/localfilesystem_spec.js b/test/red/runtime/nodes/context/localfilesystem_spec.js
index f461dace6..0d5e7062e 100644
--- a/test/red/runtime/nodes/context/localfilesystem_spec.js
+++ b/test/red/runtime/nodes/context/localfilesystem_spec.js
@@ -106,7 +106,7 @@ describe('localfilesystem',function() {
                     context.set("nodeX","foo","testX",function(err){
                         context.set("nodeY","foo","testY",function(err){
                             context.get("nodeX","foo",function(err, value){
-                                value.should.be.equal("testX"),
+                                value.should.be.equal("testX");
                                 context.get("nodeY","foo",function(err, value){
                                     value.should.be.equal("testY");
                                     done();
@@ -250,6 +250,51 @@ describe('localfilesystem',function() {
             });
         });
 
+        it('should set/get multiple values', function(done) {
+            context.set("nodeX",["one","two","three"],["test1","test2","test3"], function(err) {
+                context.get("nodeX",["one","two"], function() {
+                    Array.prototype.slice.apply(arguments).should.eql([undefined,"test1","test2"])
+                    done();
+                });
+            });
+        })
+        it('should set/get multiple values - get unknown', function(done) {
+            context.set("nodeX",["one","two","three"],["test1","test2","test3"], function(err) {
+                context.get("nodeX",["one","two","unknown"], function() {
+                    Array.prototype.slice.apply(arguments).should.eql([undefined,"test1","test2",undefined])
+                    done();
+                });
+            });
+        })
+        it('should set/get multiple values - single value providd', function(done) {
+            context.set("nodeX",["one","two","three"],"test1", function(err) {
+                context.get("nodeX",["one","two"], function() {
+                    Array.prototype.slice.apply(arguments).should.eql([undefined,"test1",null])
+                    done();
+                });
+            });
+        })
+
+        it('should throw error if bad key included in multiple keys - get', function(done) {
+            context.set("nodeX",["one","two","three"],["test1","test2","test3"], function(err) {
+                context.get("nodeX",["one",".foo","three"], function(err) {
+                    should.exist(err);
+                    done();
+                });
+            });
+        })
+
+        it('should throw error if bad key included in multiple keys - set', function(done) {
+            context.set("nodeX",["one",".foo","three"],["test1","test2","test3"], function(err) {
+                should.exist(err);
+                // Check 'one' didn't get set as a result
+                context.get("nodeX","one",function(err,one) {
+                    should.not.exist(one);
+                    done();
+                })
+            });
+        })
+
         it('should throw an error when getting a value with invalid key', function (done) {
             context.set("nodeX","foo","bar",function(err) {
                 context.get("nodeX"," ",function(err,value) {
@@ -272,7 +317,7 @@ describe('localfilesystem',function() {
                 done("should throw an error.");
             } catch (err) {
                 done();
-            };
+            }
         });
 
         it('should throw an error when callback of get() is not specified',function (done) {
@@ -281,7 +326,7 @@ describe('localfilesystem',function() {
                 done("should throw an error.");
             } catch (err) {
                 done();
-            };
+            }
         });
 
         it('should throw an error when callback of set() is not a function',function (done) {
@@ -290,7 +335,7 @@ describe('localfilesystem',function() {
                 done("should throw an error.");
             } catch (err) {
                 done();
-            };
+            }
         });
 
         it('should not throw an error when callback of set() is not specified', function (done) {
@@ -299,7 +344,7 @@ describe('localfilesystem',function() {
                 done();
             } catch (err) {
                 done("should not throw an error.");
-            };
+            }
         });
 
         it('should handle empty context file', function (done) {
@@ -377,7 +422,7 @@ describe('localfilesystem',function() {
                 done("should throw an error.");
             } catch (err) {
                 done();
-            };
+            }
         });
 
         it('should throw an error when callback of keys() is not specified', function (done) {
@@ -386,7 +431,7 @@ describe('localfilesystem',function() {
                 done("should throw an error.");
             } catch (err) {
                 done();
-            };
+            }
         });
     });
 
diff --git a/test/red/runtime/nodes/context/memory_spec.js b/test/red/runtime/nodes/context/memory_spec.js
index 95bb0674f..b353fd209 100644
--- a/test/red/runtime/nodes/context/memory_spec.js
+++ b/test/red/runtime/nodes/context/memory_spec.js
@@ -66,7 +66,7 @@ describe('memory',function() {
                 context.get("nodeY","foo").should.equal("testY");
             });
 
-            it('should thorw the error if the error occurs', function() {
+            it('should throw the error if the error occurs', function() {
                 try{
                     context.set("nodeX",".foo","test");
                     should.fail("Error was not thrown");
@@ -80,6 +80,40 @@ describe('memory',function() {
                     }
                 }
             });
+
+            it('should get multiple values - all known', function() {
+                context.set("nodeX","one","test1");
+                context.set("nodeX","two","test2");
+                context.set("nodeX","three","test3");
+                context.set("nodeX","four","test4");
+
+                var values = context.get("nodeX",["one","two","four"]);
+                values.should.eql(["test1","test2","test4"])
+            })
+            it('should get multiple values - include unknown', function() {
+                context.set("nodeX","one","test1");
+                context.set("nodeX","two","test2");
+                context.set("nodeX","three","test3");
+                context.set("nodeX","four","test4");
+
+                var values = context.get("nodeX",["one","unknown"]);
+                values.should.eql(["test1",undefined])
+            })
+            it('should throw error if bad key included in multiple keys', function() {
+                context.set("nodeX","one","test1");
+                context.set("nodeX","two","test2");
+                context.set("nodeX","three","test3");
+                context.set("nodeX","four","test4");
+
+                try{
+                    var values = context.get("nodeX",["one",".foo","three"]);
+                    should.fail("Error was not thrown");
+                }catch(err){
+                    should.exist(err);
+                }
+            })
+
+
         });
 
         describe('async',function() {
@@ -104,6 +138,40 @@ describe('memory',function() {
                     });
                 });
             });
+
+            it('should get multiple values - all known', function(done) {
+                context.set("nodeX","one","test1");
+                context.set("nodeX","two","test2");
+                context.set("nodeX","three","test3");
+                context.set("nodeX","four","test4");
+
+                context.get("nodeX",["one","two","four"],function() {
+                    Array.prototype.slice.apply(arguments).should.eql([undefined,"test1","test2","test4"])
+                    done();
+                });
+            })
+            it('should get multiple values - include unknown', function(done) {
+                context.set("nodeX","one","test1");
+                context.set("nodeX","two","test2");
+                context.set("nodeX","three","test3");
+                context.set("nodeX","four","test4");
+
+                context.get("nodeX",["one","unknown"],function() {
+                    Array.prototype.slice.apply(arguments).should.eql([undefined,"test1",undefined])
+                    done();
+                });
+            })
+            it('should throw error if bad key included in multiple keys', function(done) {
+                context.set("nodeX","one","test1");
+                context.set("nodeX","two","test2");
+                context.set("nodeX","three","test3");
+                context.set("nodeX","four","test4");
+
+                context.get("nodeX",["one",".foo","three"], function(err) {
+                    should.exist(err);
+                    done();
+                });
+            })
         });
     });