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 11E4226A24 for ; Mon, 4 Jun 2018 07:13:08 -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 8QPXxsOkx27m for ; Mon, 4 Jun 2018 07:13:08 -0400 (EDT) Received: from smtp5.mail.ru (smtp5.mail.ru [94.100.179.24]) (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 269E526A21 for ; Mon, 4 Jun 2018 07:13:06 -0400 (EDT) From: AKhatskevich Subject: [tarantool-patches] [PATCH v2][vshard] Check self passed to object methods Date: Mon, 4 Jun 2018 14:12:26 +0300 Message-Id: <20180604111226.8446-1-avkhatskevich@tarantool.org> 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: v.shpilevoy@tarantool.org, tarantool-patches@freelists.org Changes: 1. `check_self_arg` function wrapper added to utils. Its mechanics: * check that the first argument has expected metatable * return result of a real function 2. Check, that self argument is passed to all replicaset and replica object methods. Closes: #81 --- Branch: https://github.com/tarantool/vshard/tree/kh/gh-81-wo-mt-sort Issue: https://github.com/tarantool/vshard/issues/81 changes: * object methods are tested in order which is guided by `next()` function, instead of being alphabetic * self arg is checked by first arg metatable (instead of first arg type) test/router/router.result | 64 +++++++++++++++++++++++++++++++++++++++++++++ test/router/router.test.lua | 28 ++++++++++++++++++++ vshard/replicaset.lua | 58 ++++++++++++++++++++++++---------------- vshard/util.lua | 28 ++++++++++++++++++++ 4 files changed, 155 insertions(+), 23 deletions(-) diff --git a/test/router/router.result b/test/router/router.result index 5fcabc6..101369a 100644 --- a/test/router/router.result +++ b/test/router/router.result @@ -977,6 +977,70 @@ 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 +... +for _, func in pairs(getmetatable(replicaset).__index) do + local ok, msg = pcall(func, "arg_of_wrong_type") + table.insert(error_messages, msg:match("Use .*")) +end; +--- +... +test_run:cmd("setopt delimiter ''"); +--- +- true +... +error_messages +--- +- - Use replicaset:callro(...) instead of replicaset.callro(...) + - Use replicaset:connect_master(...) instead of replicaset.connect_master(...) + - 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:call(...) instead of replicaset.call(...) + - Use replicaset:up_replica_priority(...) instead of replicaset.up_replica_priority(...) + - Use replicaset:connect(...) instead of replicaset.connect(...) + - Use replicaset:callrw(...) instead of replicaset.callrw(...) + - Use replicaset:connect_all(...) instead of replicaset.connect_all(...) +... +_, replica = next(replicaset.replicas) +--- +... +error_messages = {} +--- +... +test_run:cmd("setopt delimiter ';'") +--- +- true +... +for _, func in pairs(getmetatable(replica).__index) do + local ok, msg = pcall(func, "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..62d6730 100644 --- a/test/router/router.test.lua +++ b/test/router/router.test.lua @@ -361,6 +361,34 @@ 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 ';'") +for _, func in pairs(getmetatable(replicaset).__index) do + local ok, msg = pcall(func, "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 in pairs(getmetatable(replica).__index) do + local ok, msg = pcall(func, "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..ad2d164 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 @@ -363,32 +364,30 @@ end -- Meta-methods -- 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; - }; - __tostring = replicaset_tostring; + __index = {}, -- filled in below + __tostring = replicaset_tostring, } +-- +-- Fill metatable with functiona and wrap them with "check_self_arg" function. +-- +local mt_index = replicaset_mt["__index"] +for name, func in pairs({ + 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, + }) do + mt_index[name] = csa("replicaset", name, replicaset_mt, func) +end local replica_mt = { - __index = { - is_connected = function(replica) - return replica.conn and replica.conn.state == 'active' - end, - safe_uri = function(replica) - local uri = luri.parse(replica.uri) - uri.password = nil - return luri.format(uri) - end, - }, + __index = {}, -- filled in below __tostring = function(replica) if replica.name then return replica.name..'('..replica:safe_uri()..')' @@ -397,6 +396,19 @@ local replica_mt = { end end, } +local mt_index = replica_mt["__index"] +for name, func in pairs({ + is_connected = function(replica) + return replica.conn and replica.conn.state == 'active' + end, + safe_uri = function(replica) + local uri = luri.parse(replica.uri) + uri.password = nil + return luri.format(uri) + end, + }) do + mt_index[name] = csa("replica", name, replica_mt, func) +end -- -- Calculate for each replicaset its etalon bucket count. diff --git a/vshard/util.lua b/vshard/util.lua index 184aad5..4c44180 100644 --- a/vshard/util.lua +++ b/vshard/util.lua @@ -48,7 +48,35 @@ 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 or has different metatable. +-- @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 mt Meta table of self argument. +-- @param func A function which is about to be wrapped. +-- +-- @retval Wrapped function. +-- +local function check_self_arg(obj_name, func_name, mt, func) + return function (self, ...) + if type(self) ~= "table" or getmetatable(self) ~= mt 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, } -- 2.14.1