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 36073293B6 for ; Sat, 2 Jun 2018 06:01:20 -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 MKoFm96nyeY8 for ; Sat, 2 Jun 2018 06:01:20 -0400 (EDT) Received: from smtp63.i.mail.ru (smtp63.i.mail.ru [217.69.128.43]) (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 63559293B2 for ; Sat, 2 Jun 2018 06:01:19 -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: Alex Khatskevich Message-ID: Date: Sat, 2 Jun 2018 13:01:12 +0300 MIME-Version: 1.0 In-Reply-To: <2404a222-e13e-6bd8-16ee-4f654ff452ea@tarantool.org> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: en-US 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, Vladislav Shpilevoy > 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. > 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? > 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,  }