[tarantool-patches] Re: [PATCH][vshard] Check self passed to object methods
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Sun Jun 3 19:56:47 MSK 2018
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 at 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,
> }
More information about the Tarantool-patches
mailing list