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

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

[AKhatskevich]

Branch: https://github.com/tarantool/vshard/tree/kh/gh-81-check-self
Issue: https://github.com/tarantool/vshard/issues/81

------------ commit message ------------------

Changes:

1. `check_self_arg` function wrapper added to utils.
Its mechanics:
 * check that the first argument is of table type
 * return result of a real function

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

Closes: #81
---
 test/router/router.result   | 63 +++++++++++++++++++++++++++++++++++++++++++++
 test/router/router.test.lua | 27 +++++++++++++++++++
 vshard/replicaset.lua       | 38 +++++++++++++++++----------
 vshard/util.lua             | 33 ++++++++++++++++++++++++
 4 files changed, 147 insertions(+), 14 deletions(-)

diff --git a/test/router/router.result b/test/router/router.result
index 5fcabc6..ea99a6a 100644
--- a/test/router/router.result
+++ b/test/router/router.result
@@ -977,6 +977,69 @@ util.check_error(vshard.router.sync, "xxx")
 vshard.router.sync(100500)
 ---
 ...
+-- Check that user passed self arg
+_, replicaset = next(vshard.router.internal.replicasets)
+---
+...
+error_messages = {}
+---
+...
+test_run:cmd("setopt delimiter ';'")
+---
+- true
+...
+for _, func_name in pairs({
+        "connect", "connect_master", "connect_all", "connect_replica",
+        "rebind_connections", "down_replica_priority", "up_replica_priority",
+        "call", "callrw", "callro"}) do
+    local ok, msg = pcall(replicaset[func_name], "arg_of_wrong_type")
+    table.insert(error_messages, msg:match("Use .*"))
+end;
+---
+...
+test_run:cmd("setopt delimiter ''");
+---
+- true
+...
+error_messages
+---
+- - Use replicaset:connect(...) instead of replicaset.connect(...)
+  - Use replicaset:connect_master(...) instead of replicaset.connect_master(...)
+  - Use replicaset:connect_all(...) instead of replicaset.connect_all(...)
+  - 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:up_replica_priority(...) instead of replicaset.up_replica_priority(...)
+  - Use replicaset:call(...) instead of replicaset.call(...)
+  - Use replicaset:callrw(...) instead of replicaset.callrw(...)
+  - Use replicaset:callro(...) instead of replicaset.callro(...)
+...
+_, replica = next(replicaset.replicas)
+---
+...
+error_messages = {}
+---
+...
+test_run:cmd("setopt delimiter ';'")
+---
+- true
+...
+for _, func_name in pairs({
+        "is_connected", "safe_uri"}) do
+    local ok, msg = pcall(replica[func_name], "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..c0e66e4 100644
--- a/test/router/router.test.lua
+++ b/test/router/router.test.lua
@@ -361,6 +361,33 @@ vshard.router.sync()
 util.check_error(vshard.router.sync, "xxx")
 vshard.router.sync(100500)
 
+-- Check that user passed self arg
+_, replicaset = next(vshard.router.internal.replicasets)
+error_messages = {}
+
+test_run:cmd("setopt delimiter ';'")
+for _, func_name in pairs({
+        "connect", "connect_master", "connect_all", "connect_replica",
+        "rebind_connections", "down_replica_priority", "up_replica_priority",
+        "call", "callrw", "callro"}) do
+    local ok, msg = pcall(replicaset[func_name], "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_name in pairs({
+        "is_connected", "safe_uri"}) do
+    local ok, msg = pcall(replica[func_name], "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..0fc5fb1 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
@@ -364,30 +365,39 @@ end
 --
 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;
+        connect = csa("replicaset", "connect", replicaset_connect_master);
+        connect_master = csa("replicaset", "connect_master",
+                             replicaset_connect_master);
+        connect_all = csa("replicaset", "connect_all", replicaset_connect_all);
+        connect_replica = function(rs, r)
+            assert(type(r) == "table",
+                   'Use replicaset:connect_replica(...) instead of ' ..
+                   'replicaset.connect_replica(...)')
+            return replicaset_connect_to_replica(rs, r)
+        end;
+        rebind_connections = csa("replicaset", "rebind_connections",
+                                 replicaset_rebind_connections);
+        down_replica_priority = csa("replicaset", "down_replica_priority",
+                                    replicaset_down_replica_priority);
+        up_replica_priority = csa("replicaset", "up_replica_priority",
+                                  replicaset_up_replica_priority);
+        call = csa("replicaset", "call", replicaset_master_call);
+        callrw = csa("replicaset", "callrw", replicaset_master_call);
+        callro = csa("replicaset", "callro", replicaset_nearest_call);
     };
     __tostring = replicaset_tostring;
 }
 
 local replica_mt = {
     __index = {
-        is_connected = function(replica)
+        is_connected = csa("replica", "is_connected", function(replica)
             return replica.conn and replica.conn.state == 'active'
-        end,
-        safe_uri = function(replica)
+        end),
+        safe_uri = csa("replica", "safe_uri", function(replica)
             local uri = luri.parse(replica.uri)
             uri.password = nil
             return luri.format(uri)
-        end,
+        end),
     },
     __tostring = function(replica)
         if replica.name then
diff --git a/vshard/util.lua b/vshard/util.lua
index 184aad5..95d538d 100644
--- a/vshard/util.lua
+++ b/vshard/util.lua
@@ -48,7 +48,40 @@ local function reloadable_fiber_f(M, func_name, worker_name)
     end
 end
 
+--
+-- This function is used to return an error in case one called a method
+-- like object.func instead of object:func.
+-- This wrapper takes a function and returns another function, which checks
+-- anguments and calls wrapped function.
+-- Returning wrapped values to the user and using raw functions inside of a
+-- modele improves speed.
+--
+-- This function can be used only in case the second argument is not of the
+-- "table" type.
+--
+-- @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 func A function which is about to be wrapped
+--
+-- @retval Wrapped function.
+--
+local function check_self_arg(obj_name, func_name, func)
+    local function wrapped_func(self, ...)
+        if type(self) ~= "table" 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
+    return wrapped_func
+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][vshard] Check self passed to object methods

[Vladislav Shpilevoy]

Hello. Thanks for the patch! See 10 comments below.

On 01/06/2018 15:49, AKhatskevich wrote:
> Branch: https://github.com/tarantool/vshard/tree/kh/gh-81-check-self
> Issue: https://github.com/tarantool/vshard/issues/81

1. Please, put links after --- below the commit message. Not before and
not above your own ---.

> 
> ------------ commit message ------------------
> 
> Changes:
> 
> 1. `check_self_arg` function wrapper added to utils.
> Its mechanics:
>   * check that the first argument is of table type
>   * return result of a real function
> 
> 2. Check, that self argument is passed to all replicaset and replica
> object methods.
> 
> Closes: #81
> ---

Put the links here.

>   test/router/router.result   | 63 +++++++++++++++++++++++++++++++++++++++++++++
>   test/router/router.test.lua | 27 +++++++++++++++++++
>   vshard/replicaset.lua       | 38 +++++++++++++++++----------
>   vshard/util.lua             | 33 ++++++++++++++++++++++++
>   4 files changed, 147 insertions(+), 14 deletions(-)
> 
> diff --git a/test/router/router.result b/test/router/router.result
> index 5fcabc6..ea99a6a 100644
> --- a/test/router/router.result
> +++ b/test/router/router.result
> @@ -977,6 +977,69 @@ util.check_error(vshard.router.sync, "xxx")
>   vshard.router.sync(100500)
>   ---
>   ...
> +-- Check that user passed self arg
> +_, replicaset = next(vshard.router.internal.replicasets)
> +---
> +...
> +error_messages = {}
> +---
> +...
> +test_run:cmd("setopt delimiter ';'")
> +---
> +- true
> +...
> +for _, func_name in pairs({
> +        "connect", "connect_master", "connect_all", "connect_replica",
> +        "rebind_connections", "down_replica_priority", "up_replica_priority",
> +        "call", "callrw", "callro"}) do
> +    local ok, msg = pcall(replicaset[func_name], "arg_of_wrong_type")
> +    table.insert(error_messages, msg:match("Use .*"))
> +end;

2. Why can not you just iterate over replicaset metatable? Your way
requires to update this test each time a new method is added.

> +---
> +...
> +test_run:cmd("setopt delimiter ''");
> +---
> +- true
> +...
> +error_messages
> +---
> +- - Use replicaset:connect(...) instead of replicaset.connect(...)
> +  - Use replicaset:connect_master(...) instead of replicaset.connect_master(...)
> +  - Use replicaset:connect_all(...) instead of replicaset.connect_all(...)
> +  - 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:up_replica_priority(...) instead of replicaset.up_replica_priority(...)
> +  - Use replicaset:call(...) instead of replicaset.call(...)
> +  - Use replicaset:callrw(...) instead of replicaset.callrw(...)
> +  - Use replicaset:callro(...) instead of replicaset.callro(...)
> +...
> +_, replica = next(replicaset.replicas)
> +---
> +...
> +error_messages = {}
> +---
> +...
> +test_run:cmd("setopt delimiter ';'")
> +---
> +- true
> +...
> +for _, func_name in pairs({
> +        "is_connected", "safe_uri"}) do
> +    local ok, msg = pcall(replica[func_name], "arg_of_wrong_type")
> +    table.insert(error_messages, msg:match("Use .*"))

3. Same.

> +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/vshard/replicaset.lua b/vshard/replicaset.lua
> index d210973..0fc5fb1 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
> @@ -364,30 +365,39 @@ end
>   --
>   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;
> +        connect = csa("replicaset", "connect", replicaset_connect_master);
> +        connect_master = csa("replicaset", "connect_master",
> +                             replicaset_connect_master);
> +        connect_all = csa("replicaset", "connect_all", replicaset_connect_all);
> +        connect_replica = function(rs, r)
> +            assert(type(r) == "table",
> +                   'Use replicaset:connect_replica(...) instead of ' ..
> +                   'replicaset.connect_replica(...)')> +            return replicaset_connect_to_replica(rs, r)

4. You have no checked rs. This check_self_arg is broken - you must have two functions:
check and generate checker. Now you have only generate checker that you have named
check_self_arg.

In this function you do manual check_self_arg for 'r' and 'rs'. For other
functions you do the same as now - generate checker.

> +        end;
> +        rebind_connections = csa("replicaset", "rebind_connections",
> +                                 replicaset_rebind_connections);
> +        down_replica_priority = csa("replicaset", "down_replica_priority",
> +                                    replicaset_down_replica_priority);
> +        up_replica_priority = csa("replicaset", "up_replica_priority",
> +                                  replicaset_up_replica_priority);
> +        call = csa("replicaset", "call", replicaset_master_call);
> +        callrw = csa("replicaset", "callrw", replicaset_master_call);
> +        callro = csa("replicaset", "callro", replicaset_nearest_call);
>       };
>       __tostring = replicaset_tostring;
>   }
> diff --git a/vshard/util.lua b/vshard/util.lua
> index 184aad5..95d538d 100644
> --- a/vshard/util.lua
> +++ b/vshard/util.lua
> @@ -48,7 +48,40 @@ local function reloadable_fiber_f(M, func_name, worker_name)
>       end
>   end
>   
> +--
> +-- This function is used to return an error in case one called a method

5. Describing a function you may omit 'this function does', 'this function
is needed to' etc. Function comment must look like a commit title:
imperative text describing what this function shall do.

> +-- like object.func instead of object:func.
> +-- This wrapper takes a function and returns another function, which checks
> +-- anguments and calls wrapped function.

6. anguments?

7. 'This function takes' must be moved to the corresponding @param.

> +-- Returning wrapped values to the user and using raw functions inside of a
> +-- modele improves speed.

8. modele?

> +--
> +-- This function can be used only in case the second argument is not of the
> +-- "table" type.
> +--
> +-- @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 func A function which is about to be wrapped
> +--

9. You do not need extra new line after each @param.

> +-- @retval Wrapped function.
> +--
> +local function check_self_arg(obj_name, func_name, func)
> +    local function wrapped_func(self, ...)
> +        if type(self) ~= "table" 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
> +    return wrapped_func

10. Why no just 'return function wrapped_func(... ' ?

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

[Alex Khatskevich]

Hi.

> 1. Please, put links after --- below the commit message. Not before and
> not above your own ---.
>
ok
>
> 2. Why can not you just iterate over replicaset metatable? Your way
> requires to update this test each time a new method is added.
I thought, that it can create a lot of pain in case someone adds method 
which do not take
self attribute.
Anyway, fixed adding this helper:
-----------------------
get_object_method_names = function(object)
     local res = {}
     for name, _ in pairs(getmetatable(object).__index) do
         table.insert(res, name)
     end
     table.sort(res)
     return res
end;
-------------------------
>> +        connect_replica = function(rs, r)
>> +            assert(type(r) == "table",
>> +                   'Use replicaset:connect_replica(...) instead of ' ..
>> +                   'replicaset.connect_replica(...)')> +            
>> return replicaset_connect_to_replica(rs, r)
>
> 4. You have no checked rs. This check_self_arg is broken - you must 
> have two functions:
> check and generate checker. Now you have only generate checker that 
> you have named
> check_self_arg.
>
> In this function you do manual check_self_arg for 'r' and 'rs'. For other
> functions you do the same as now - generate checker.
Fixed this way:
             assert(type(rs) == "table" and type(r) == "table",
>
> 5. Describing a function you may omit 'this function does', 'this 
> function
> is needed to' etc. Function comment must look like a commit title:
> imperative text describing what this function shall do.
> 7. 'This function takes' must be moved to the corresponding @param.
Rewritten a little.

> 6. anguments?
> 8. modele?
> 9. You do not need extra new line after each @param.
Fixed

Here is a new version
--
-- 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.
--
-- @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 func A function which is about to be wrapped
--
-- @retval Wrapped function.
--

> 10. Why no just 'return function wrapped_func(... ' ?
Fixed

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

[Vladislav Shpilevoy]

Thanks for the fixes!

Please, after fixes put the complete patch at the end of a letter.

See 6 comments below.

> diff --git a/test/router/router.result b/test/router/router.result
> index 5fcabc6..b00cd12 100644
> --- a/test/router/router.result
> +++ b/test/router/router.result
> @@ -977,6 +977,75 @@ util.check_error(vshard.router.sync, "xxx")
>  vshard.router.sync(100500)
>  ---
>  ...
> +-- Check that user passed self arg

1. Please, beforehand describe the issue:
--
-- gh-####: issue description.
--

> +get_object_method_names = function(object)
> +    local res = {}
> +    for name, _ in pairs(getmetatable(object).__index) do
> +        table.insert(res, name)
> +    end
> +    table.sort(res)
> +    return res
> +end;

2. Why do you need table.sort here?

> +for _, func_name in pairs(get_object_method_names(replicaset)) do
> +    local ok, msg = pcall(replicaset[func_name], "arg_of_wrong_type")
> +    table.insert(error_messages, msg:match("Use .*"))
> +end;

3. Why does not work just iteration over getmetatable(object).__index ?

> diff --git a/vshard/replicaset.lua b/vshard/replicaset.lua
> index d210973..ede53c7 100644
> --- a/vshard/replicaset.lua
> +++ b/vshard/replicaset.lua
> @@ -364,30 +365,39 @@ end
>  --
>  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;
> +        connect = csa("replicaset", "connect", replicaset_connect_master);
> +        connect_master = csa("replicaset", "connect_master",
> +                             replicaset_connect_master);
> +        connect_all = csa("replicaset", "connect_all", replicaset_connect_all);
> +        connect_replica = function(rs, r)
> +            assert(type(rs) == "table" and type(r) == "table",
> +                   'Use replicaset:connect_replica(...) instead of ' ..
> +                   'replicaset.connect_replica(...)')
> +            return replicaset_connect_to_replica(rs, r)
> +        end;

4. Looks like you did not that I asked for. Please, do not use assert, and do not
inline part of check_self_arg. Make two functions: check_self_arg, generate_checker
or something.

> +--
> +-- 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.
> +--

5. Still extra line.

> +-- @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 func A function which is about to be wrapped
> +--
> +-- @retval Wrapped function.
> +--

6. Please, fit the comment in 66 symbols.

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

[Alex Khatskevich]

> 1. Please, beforehand describe the issue:
> -- 
> -- gh-####: issue description.
> -- 
Fixed
> 2. Why do you need table.sort here?
> 3. Why does not work just iteration over getmetatable(object).__index ?
Because as far as I know, the order of iteration over a dictionary is not
determined. However we need to keep order the same, so that test.result
file do not change.
> 4. Looks like you did not that I asked for. Please, do not use assert, 
> and do not
> inline part of check_self_arg. Make two functions: check_self_arg, 
> generate_checker
> or something.
You want the `generate_checker` to be a universal wrapper-function, 
which checks
any number of arguments (not only the first one). Is that right?
> 5. Still extra line.
Fixed
> 6. Please, fit the comment in 66 symbols.
Fixed


The whole diff:

commit c0b7847cb76802f0bbe839537a00d17dfc995eb5
Author: AKhatskevich <avkhatskevich@tarantool.org>
Date:   Fri Jun 1 14:58:51 2018 +0300

     Check self passed to object methods

     Changes:

     1. `check_self_arg` function wrapper added to utils.
     Its mechanics:
      * check that the first argument is of table type
      * return result of a real function

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

     Closes: #81

diff --git a/test/router/router.result b/test/router/router.result
index 5fcabc6..635b63c 100644
--- a/test/router/router.result
+++ b/test/router/router.result
@@ -977,6 +977,80 @@ 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
+...
+get_object_method_names = function(object)
+    local res = {}
+    for name, _ in pairs(getmetatable(object).__index) do
+        table.insert(res, name)
+    end
+    table.sort(res)
+    return res
+end;
+---
+...
+for _, func_name in pairs(get_object_method_names(replicaset)) do
+    local ok, msg = pcall(replicaset[func_name], "arg_of_wrong_type")
+    table.insert(error_messages, msg:match("Use .*"))
+end;
+---
+...
+test_run:cmd("setopt delimiter ''");
+---
+- true
+...
+error_messages
+---
+- - Use replicaset:call(...) instead of replicaset.call(...)
+  - Use replicaset:callro(...) instead of replicaset.callro(...)
+  - Use replicaset:callrw(...) instead of replicaset.callrw(...)
+  - Use replicaset:connect(...) instead of replicaset.connect(...)
+  - Use replicaset:connect_all(...) instead of replicaset.connect_all(...)
+  - Use replicaset:connect_master(...) instead of 
replicaset.connect_master(...)
+  - Use replicaset:connect_replica(...) instead of 
replicaset.connect_replica(...)
+  - Use replicaset:down_replica_priority(...) instead of 
replicaset.down_replica_priority(...)
+  - Use replicaset:rebind_connections(...) instead of 
replicaset.rebind_connections(...)
+  - Use replicaset:up_replica_priority(...) instead of 
replicaset.up_replica_priority(...)
+...
+_, replica = next(replicaset.replicas)
+---
+...
+error_messages = {}
+---
+...
+test_run:cmd("setopt delimiter ';'")
+---
+- true
+...
+for _, func_name in pairs(get_object_method_names(replica)) do
+    local ok, msg = pcall(replica[func_name], "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..ff77176 100644
--- a/test/router/router.test.lua
+++ b/test/router/router.test.lua
@@ -361,6 +361,43 @@ 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 ';'")
+get_object_method_names = function(object)
+    local res = {}
+    for name, _ in pairs(getmetatable(object).__index) do
+        table.insert(res, name)
+    end
+    table.sort(res)
+    return res
+end;
+
+for _, func_name in pairs(get_object_method_names(replicaset)) do
+    local ok, msg = pcall(replicaset[func_name], "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_name in pairs(get_object_method_names(replica)) do
+    local ok, msg = pcall(replica[func_name], "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..ede53c7 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
@@ -364,30 +365,39 @@ end
  --
  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;
+        connect = csa("replicaset", "connect", replicaset_connect_master);
+        connect_master = csa("replicaset", "connect_master",
+                             replicaset_connect_master);
+        connect_all = csa("replicaset", "connect_all", 
replicaset_connect_all);
+        connect_replica = function(rs, r)
+            assert(type(rs) == "table" and type(r) == "table",
+                   'Use replicaset:connect_replica(...) instead of ' ..
+                   'replicaset.connect_replica(...)')
+            return replicaset_connect_to_replica(rs, r)
+        end;
+        rebind_connections = csa("replicaset", "rebind_connections",
+ replicaset_rebind_connections);
+        down_replica_priority = csa("replicaset", "down_replica_priority",
+ replicaset_down_replica_priority);
+        up_replica_priority = csa("replicaset", "up_replica_priority",
+ replicaset_up_replica_priority);
+        call = csa("replicaset", "call", replicaset_master_call);
+        callrw = csa("replicaset", "callrw", replicaset_master_call);
+        callro = csa("replicaset", "callro", replicaset_nearest_call);
      };
      __tostring = replicaset_tostring;
  }

  local replica_mt = {
      __index = {
-        is_connected = function(replica)
+        is_connected = csa("replica", "is_connected", function(replica)
              return replica.conn and replica.conn.state == 'active'
-        end,
-        safe_uri = function(replica)
+        end),
+        safe_uri = csa("replica", "safe_uri", function(replica)
              local uri = luri.parse(replica.uri)
              uri.password = nil
              return luri.format(uri)
-        end,
+        end),
      },
      __tostring = function(replica)
          if replica.name then
diff --git a/vshard/util.lua b/vshard/util.lua
index 184aad5..5b83260 100644
--- a/vshard/util.lua
+++ b/vshard/util.lua
@@ -48,7 +48,34 @@ 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.
+-- @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 func A function which is about to be wrapped.
+--
+-- @retval Wrapped function.
+--
+local function check_self_arg(obj_name, func_name, func)
+    return function (self, ...)
+        if type(self) ~= "table" 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,
  }

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

[Vladislav Shpilevoy]

On 02/06/2018 13:01, Alex Khatskevich wrote:
> 
>> 1. Please, beforehand describe the issue:
>> -- 
>> -- gh-####: issue description.
>> -- 
> Fixed
>> 2. Why do you need table.sort here?
>> 3. Why does not work just iteration over getmetatable(object).__index ?
> Because as far as I know, the order of iteration over a dictionary is not
> determined. However we need to keep order the same, so that test.result
> file do not change.

It is not completely random. Depends on order of insertion, and on keys.
Please, remove this helper and just iterate over metatable __index.

>> 4. Looks like you did not that I asked for. Please, do not use assert, and do not
>> inline part of check_self_arg. Make two functions: check_self_arg, generate_checker
>> or something.
> You want the `generate_checker` to be a universal wrapper-function, which checks
> any number of arguments (not only the first one). Is that right?

No, it is not right. I want separate generate_checker() for first argument table
checking, and separate check_self_arg().

>> 5. Still extra line.
> Fixed
>> 6. Please, fit the comment in 66 symbols.
> Fixed
> 
> 
> The whole diff:
> 
> commit c0b7847cb76802f0bbe839537a00d17dfc995eb5
> Author: AKhatskevich <avkhatskevich@tarantool.org>
> Date:   Fri Jun 1 14:58:51 2018 +0300
> 
>      Check self passed to object methods
> 
>      Changes:
> 
>      1. `check_self_arg` function wrapper added to utils.
>      Its mechanics:
>       * check that the first argument is of table type
>       * return result of a real function
> 
>      2. Check, that self argument is passed to all replicaset and replica
>      object methods.
> 
>      Closes: #81
> 
> diff --git a/test/router/router.result b/test/router/router.result
> index 5fcabc6..635b63c 100644
> --- a/test/router/router.result
> +++ b/test/router/router.result
> @@ -977,6 +977,80 @@ 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
> +...
> +get_object_method_names = function(object)
> +    local res = {}
> +    for name, _ in pairs(getmetatable(object).__index) do
> +        table.insert(res, name)
> +    end
> +    table.sort(res)
> +    return res
> +end;
> +---
> +...
> +for _, func_name in pairs(get_object_method_names(replicaset)) do
> +    local ok, msg = pcall(replicaset[func_name], "arg_of_wrong_type")
> +    table.insert(error_messages, msg:match("Use .*"))
> +end;
> +---
> +...
> +test_run:cmd("setopt delimiter ''");
> +---
> +- true
> +...
> +error_messages
> +---
> +- - Use replicaset:call(...) instead of replicaset.call(...)
> +  - Use replicaset:callro(...) instead of replicaset.callro(...)
> +  - Use replicaset:callrw(...) instead of replicaset.callrw(...)
> +  - Use replicaset:connect(...) instead of replicaset.connect(...)
> +  - Use replicaset:connect_all(...) instead of replicaset.connect_all(...)
> +  - Use replicaset:connect_master(...) instead of replicaset.connect_master(...)
> +  - Use replicaset:connect_replica(...) instead of replicaset.connect_replica(...)
> +  - Use replicaset:down_replica_priority(...) instead of replicaset.down_replica_priority(...)
> +  - Use replicaset:rebind_connections(...) instead of replicaset.rebind_connections(...)
> +  - Use replicaset:up_replica_priority(...) instead of replicaset.up_replica_priority(...)
> +...
> +_, replica = next(replicaset.replicas)
> +---
> +...
> +error_messages = {}
> +---
> +...
> +test_run:cmd("setopt delimiter ';'")
> +---
> +- true
> +...
> +for _, func_name in pairs(get_object_method_names(replica)) do
> +    local ok, msg = pcall(replica[func_name], "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..ff77176 100644
> --- a/test/router/router.test.lua
> +++ b/test/router/router.test.lua
> @@ -361,6 +361,43 @@ 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 ';'")
> +get_object_method_names = function(object)
> +    local res = {}
> +    for name, _ in pairs(getmetatable(object).__index) do
> +        table.insert(res, name)
> +    end
> +    table.sort(res)
> +    return res
> +end;
> +
> +for _, func_name in pairs(get_object_method_names(replicaset)) do
> +    local ok, msg = pcall(replicaset[func_name], "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_name in pairs(get_object_method_names(replica)) do
> +    local ok, msg = pcall(replica[func_name], "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..ede53c7 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
> @@ -364,30 +365,39 @@ end
>   --
>   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;
> +        connect = csa("replicaset", "connect", replicaset_connect_master);
> +        connect_master = csa("replicaset", "connect_master",
> +                             replicaset_connect_master);
> +        connect_all = csa("replicaset", "connect_all", replicaset_connect_all);
> +        connect_replica = function(rs, r)
> +            assert(type(rs) == "table" and type(r) == "table",
> +                   'Use replicaset:connect_replica(...) instead of ' ..
> +                   'replicaset.connect_replica(...)')
> +            return replicaset_connect_to_replica(rs, r)
> +        end;
> +        rebind_connections = csa("replicaset", "rebind_connections",
> + replicaset_rebind_connections);
> +        down_replica_priority = csa("replicaset", "down_replica_priority",
> + replicaset_down_replica_priority);
> +        up_replica_priority = csa("replicaset", "up_replica_priority",
> + replicaset_up_replica_priority);
> +        call = csa("replicaset", "call", replicaset_master_call);
> +        callrw = csa("replicaset", "callrw", replicaset_master_call);
> +        callro = csa("replicaset", "callro", replicaset_nearest_call);
>       };
>       __tostring = replicaset_tostring;
>   }
> 
>   local replica_mt = {
>       __index = {
> -        is_connected = function(replica)
> +        is_connected = csa("replica", "is_connected", function(replica)
>               return replica.conn and replica.conn.state == 'active'
> -        end,
> -        safe_uri = function(replica)
> +        end),
> +        safe_uri = csa("replica", "safe_uri", function(replica)
>               local uri = luri.parse(replica.uri)
>               uri.password = nil
>               return luri.format(uri)
> -        end,
> +        end),
>       },
>       __tostring = function(replica)
>           if replica.name then
> diff --git a/vshard/util.lua b/vshard/util.lua
> index 184aad5..5b83260 100644
> --- a/vshard/util.lua
> +++ b/vshard/util.lua
> @@ -48,7 +48,34 @@ 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.
> +-- @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 func A function which is about to be wrapped.
> +--
> +-- @retval Wrapped function.
> +--
> +local function check_self_arg(obj_name, func_name, func)
> +    return function (self, ...)
> +        if type(self) ~= "table" 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,
>   }