[tarantool-patches] [PATCH v2][vshard] Check self passed to object methods

[tarantool-patches] [PATCH v2][vshard] Check self passed to object methods

[AKhatskevich]

Changes:

1. `check_self_arg` function wrapper added to utils.
Its mechanics:
 * check that the first argument has expected metatable
 * return result of a real function

2. Check, that self argument is passed to all replicaset and replica
object methods.

Closes: #81
---
Branch: https://github.com/tarantool/vshard/tree/kh/gh-81-wo-mt-sort
Issue: https://github.com/tarantool/vshard/issues/81

changes:
 * object methods are tested in order which is guided by `next()` function,
   instead of being alphabetic
 * self arg is checked by first arg metatable (instead of first arg type)

 test/router/router.result   | 64 +++++++++++++++++++++++++++++++++++++++++++++
 test/router/router.test.lua | 28 ++++++++++++++++++++
 vshard/replicaset.lua       | 58 ++++++++++++++++++++++++----------------
 vshard/util.lua             | 28 ++++++++++++++++++++
 4 files changed, 155 insertions(+), 23 deletions(-)

diff --git a/test/router/router.result b/test/router/router.result
index 5fcabc6..101369a 100644
--- a/test/router/router.result
+++ b/test/router/router.result
@@ -977,6 +977,70 @@ util.check_error(vshard.router.sync, "xxx")
 vshard.router.sync(100500)
 ---
 ...
+--
+-- gh-81: Check that user passed self arg.
+-- This check ensures that in case a vshard user called an
+-- object method like this: object.method() instead of
+-- object:method(), an appropriate help-error returns.
+--
+_, replicaset = next(vshard.router.internal.replicasets)
+---
+...
+error_messages = {}
+---
+...
+test_run:cmd("setopt delimiter ';'")
+---
+- true
+...
+for _, func in pairs(getmetatable(replicaset).__index) do
+    local ok, msg = pcall(func, "arg_of_wrong_type")
+    table.insert(error_messages, msg:match("Use .*"))
+end;
+---
+...
+test_run:cmd("setopt delimiter ''");
+---
+- true
+...
+error_messages
+---
+- - Use replicaset:callro(...) instead of replicaset.callro(...)
+  - Use replicaset:connect_master(...) instead of replicaset.connect_master(...)
+  - Use replicaset:connect_replica(...) instead of replicaset.connect_replica(...)
+  - Use replicaset:rebind_connections(...) instead of replicaset.rebind_connections(...)
+  - Use replicaset:down_replica_priority(...) instead of replicaset.down_replica_priority(...)
+  - Use replicaset:call(...) instead of replicaset.call(...)
+  - Use replicaset:up_replica_priority(...) instead of replicaset.up_replica_priority(...)
+  - Use replicaset:connect(...) instead of replicaset.connect(...)
+  - Use replicaset:callrw(...) instead of replicaset.callrw(...)
+  - Use replicaset:connect_all(...) instead of replicaset.connect_all(...)
+...
+_, replica = next(replicaset.replicas)
+---
+...
+error_messages = {}
+---
+...
+test_run:cmd("setopt delimiter ';'")
+---
+- true
+...
+for _, func in pairs(getmetatable(replica).__index) do
+    local ok, msg = pcall(func, "arg_of_wrong_type")
+    table.insert(error_messages, msg:match("Use .*"))
+end;
+---
+...
+test_run:cmd("setopt delimiter ''");
+---
+- true
+...
+error_messages
+---
+- - Use replica:is_connected(...) instead of replica.is_connected(...)
+  - Use replica:safe_uri(...) instead of replica.safe_uri(...)
+...
 _ = test_run:cmd("switch default")
 ---
 ...
diff --git a/test/router/router.test.lua b/test/router/router.test.lua
index ebf0eb5..62d6730 100644
--- a/test/router/router.test.lua
+++ b/test/router/router.test.lua
@@ -361,6 +361,34 @@ vshard.router.sync()
 util.check_error(vshard.router.sync, "xxx")
 vshard.router.sync(100500)
 
+--
+-- gh-81: Check that user passed self arg.
+-- This check ensures that in case a vshard user called an
+-- object method like this: object.method() instead of
+-- object:method(), an appropriate help-error returns.
+--
+_, replicaset = next(vshard.router.internal.replicasets)
+error_messages = {}
+
+test_run:cmd("setopt delimiter ';'")
+for _, func in pairs(getmetatable(replicaset).__index) do
+    local ok, msg = pcall(func, "arg_of_wrong_type")
+    table.insert(error_messages, msg:match("Use .*"))
+end;
+test_run:cmd("setopt delimiter ''");
+error_messages
+
+_, replica = next(replicaset.replicas)
+error_messages = {}
+
+test_run:cmd("setopt delimiter ';'")
+for _, func in pairs(getmetatable(replica).__index) do
+    local ok, msg = pcall(func, "arg_of_wrong_type")
+    table.insert(error_messages, msg:match("Use .*"))
+end;
+test_run:cmd("setopt delimiter ''");
+error_messages
+
 _ = test_run:cmd("switch default")
 test_run:drop_cluster(REPLICASET_2)
 
diff --git a/vshard/replicaset.lua b/vshard/replicaset.lua
index d210973..ad2d164 100644
--- a/vshard/replicaset.lua
+++ b/vshard/replicaset.lua
@@ -48,6 +48,7 @@ local lerror = require('vshard.error')
 local fiber = require('fiber')
 local luri = require('uri')
 local ffi = require('ffi')
+local csa = require('vshard.util').check_self_arg
 
 --
 -- on_connect() trigger for net.box
@@ -363,32 +364,30 @@ end
 -- Meta-methods
 --
 local replicaset_mt = {
-    __index = {
-        connect = replicaset_connect_master;
-        connect_master = replicaset_connect_master;
-        connect_all = replicaset_connect_all;
-        connect_replica = replicaset_connect_to_replica;
-        rebind_connections = replicaset_rebind_connections;
-        down_replica_priority = replicaset_down_replica_priority;
-        up_replica_priority = replicaset_up_replica_priority;
-        call = replicaset_master_call;
-        callrw = replicaset_master_call;
-        callro = replicaset_nearest_call;
-    };
-    __tostring = replicaset_tostring;
+    __index = {}, -- filled in below
+    __tostring = replicaset_tostring,
 }
+-- 
+-- Fill metatable with functiona and wrap them with "check_self_arg" function.
+--
+local mt_index = replicaset_mt["__index"]
+for name, func in pairs({
+        connect = replicaset_connect_master,
+        connect_master = replicaset_connect_master,
+        connect_all = replicaset_connect_all,
+        connect_replica = replicaset_connect_to_replica,
+        rebind_connections = replicaset_rebind_connections,
+        down_replica_priority = replicaset_down_replica_priority,
+        up_replica_priority = replicaset_up_replica_priority,
+        call = replicaset_master_call,
+        callrw = replicaset_master_call,
+        callro = replicaset_nearest_call,
+    }) do
+    mt_index[name] = csa("replicaset", name, replicaset_mt, func)
+end
 
 local replica_mt = {
-    __index = {
-        is_connected = function(replica)
-            return replica.conn and replica.conn.state == 'active'
-        end,
-        safe_uri = function(replica)
-            local uri = luri.parse(replica.uri)
-            uri.password = nil
-            return luri.format(uri)
-        end,
-    },
+    __index = {}, -- filled in below
     __tostring = function(replica)
         if replica.name then
             return replica.name..'('..replica:safe_uri()..')'
@@ -397,6 +396,19 @@ local replica_mt = {
         end
     end,
 }
+local mt_index = replica_mt["__index"]
+for name, func in pairs({
+        is_connected = function(replica)
+            return replica.conn and replica.conn.state == 'active'
+        end,
+        safe_uri = function(replica)
+            local uri = luri.parse(replica.uri)
+            uri.password = nil
+            return luri.format(uri)
+        end,
+    }) do
+    mt_index[name] = csa("replica", name, replica_mt, func)
+end
 
 --
 -- Calculate for each replicaset its etalon bucket count.
diff --git a/vshard/util.lua b/vshard/util.lua
index 184aad5..4c44180 100644
--- a/vshard/util.lua
+++ b/vshard/util.lua
@@ -48,7 +48,35 @@ local function reloadable_fiber_f(M, func_name, worker_name)
     end
 end
 
+--
+-- Wrap a given function to check that self argument is passed.
+-- New function returns an error in case one called a method
+-- like object.func instead of object:func.
+-- Returning wrapped function to a user and using raw functions
+-- inside of a module improves speed.
+-- This function can be used only in case the second argument is
+-- not of the "table" type or has different metatable.
+-- @param obj_name Name of the called object. Used only for error
+--        message construction.
+-- @param func_name Name of the called function. Used only for an
+--        error message construction.
+-- @param mt Meta table of self argument.
+-- @param func A function which is about to be wrapped.
+--
+-- @retval Wrapped function.
+--
+local function check_self_arg(obj_name, func_name, mt, func)
+    return function (self, ...)
+        if type(self) ~= "table" or getmetatable(self) ~= mt then
+            local fmt = 'Use %s:%s(...) instead of %s.%s(...)'
+            error(string.format(fmt, obj_name, func_name, obj_name, func_name))
+        end
+        return func(self, ...)
+    end
+end
+
 return {
     tuple_extract_key = tuple_extract_key,
     reloadable_fiber_f = reloadable_fiber_f,
+    check_self_arg = check_self_arg,
 }
-- 
2.14.1

[tarantool-patches] Re: [PATCH v2][vshard] Check self passed to object methods

[Vladislav Shpilevoy]

Pushed with my minor fixes.

On 04/06/2018 14:12, AKhatskevich wrote:
> Changes:
> 
> 1. `check_self_arg` function wrapper added to utils.
> Its mechanics:
>   * check that the first argument has expected metatable
>   * return result of a real function
> 
> 2. Check, that self argument is passed to all replicaset and replica
> object methods.
> 
> Closes: #81
> ---
> Branch: https://github.com/tarantool/vshard/tree/kh/gh-81-wo-mt-sort
> Issue: https://github.com/tarantool/vshard/issues/81
>