From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id F10212909E for ; Fri, 1 Jun 2018 10:51:46 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id Tms6_N51lA9g for ; Fri, 1 Jun 2018 10:51:46 -0400 (EDT) Received: from smtp54.i.mail.ru (smtp54.i.mail.ru [217.69.128.34]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 543A227C28 for ; Fri, 1 Jun 2018 10:51:46 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH][vshard] Check self passed to object methods References: <20180601124912.9145-1-avkhatskevich@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Fri, 1 Jun 2018 17:51:42 +0300 MIME-Version: 1.0 In-Reply-To: <20180601124912.9145-1-avkhatskevich@tarantool.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org, AKhatskevich 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(... ' ?