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 E1F9828825 for ; Sun, 3 Jun 2018 12:56:52 -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 dIRZEuqvZn5d for ; Sun, 3 Jun 2018 12:56:52 -0400 (EDT) Received: from smtp55.i.mail.ru (smtp55.i.mail.ru [217.69.128.35]) (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 179C32881E for ; Sun, 3 Jun 2018 12:56:51 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH][vshard] Check self passed to object methods References: <20180601124912.9145-1-avkhatskevich@tarantool.org> <2404a222-e13e-6bd8-16ee-4f654ff452ea@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Sun, 3 Jun 2018 19:56:47 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Language: en-US Content-Transfer-Encoding: 8bit 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: Alex Khatskevich , tarantool-patches@freelists.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 > 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, >  }