[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(... ' ?