[tarantool-patches] Re: [PATCH][vshard] Check self passed to object methods
Alex Khatskevich
avkhatskevich at tarantool.org
Sat Jun 2 13:01:12 MSK 2018
> 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 <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