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

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

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,
>   }