Merge branch 'feature/params_override_dsn' into develop

Author: pintsized

README.md

@@ -39,7 +39,7 @@ local rc = require("resty.redis.connector").new({
     connect_timeout = 50,
     read_timeout = 5000,
     keepalive_timeout = 30000,
-    
+
     host = "127.0.0.1",
     port = 6379,
     db = 2,
@@ -53,7 +53,7 @@ local redis, err = rc:connect()
 local ok, err = rc:set_keepalive(redis)
 ```
 
-`connect` can be used to override defaults given in `new`
+[connect](#connect) can be used to override some defaults given in [new](#new), which are pertinent to this connection only.
 
 
 ```lua
@@ -71,7 +71,9 @@ local redis, err = rc:connect({
 
 ## DSN format
 
-If the `params.url` field is present then it will be parsed, overriding values supplied in the parameters table.
+If the `params.url` field is present then it will be parsed to set the other params. Any manually specified params will override values given in the DSN.
+
+*Note: this is a behaviour change as of v0.06. Previously, the DSN values would take precedence.*
 
 ### Direct Redis connections
 
@@ -87,11 +89,7 @@ When connecting via Redis Sentinel, the format is as follows:
 
 `sentinel://PASSWORD@MASTER_NAME:ROLE/DB`
 
-Again, `PASSWORD` and `DB` are optional. `ROLE` must be any of `m`, `s` or `a`, meaning:
-
-* `m`: master
-* `s`: slave
-* `a`: any (first tries the master, but will failover to a slave if required)
+Again, `PASSWORD` and `DB` are optional. `ROLE` must be either `m` or `s` for master / slave respectively.
 
 A table of `sentinels` must also be supplied. e.g.
 
@@ -106,11 +104,11 @@ local redis, err = rc:connect{
 
 ## Proxy Mode
 
-Enable the `connection_is_proxied` parameter if connecting to Redis through a proxy service (e.g. Twemproxy).  
-These proxies generally only support a limited sub-set of Redis commands, those which do not require state and do not affect multiple keys.  
+Enable the `connection_is_proxied` parameter if connecting to Redis through a proxy service (e.g. Twemproxy).
+These proxies generally only support a limited sub-set of Redis commands, those which do not require state and do not affect multiple keys.
 Databases and transactions are also not supported.
 
-Proxy mode will disable switching to a DB on connect.  
+Proxy mode will disable switching to a DB on connect.
 Unsupported commands (defaults to those not supported by Twemproxy) will return `nil, err` immediately rather than being sent to the proxy, which can result in dropped connections.
 
 `discard` will not be sent when adding connections to the keepalive pool
@@ -138,7 +136,7 @@ If configured as a table of commands, the command methods will be replaced by a
     db = 0,
 
     master_name = "mymaster",
-    role = "master",  -- master | slave | any
+    role = "master",  -- master | slave
     sentinels = {},
 
     connection_is_proxied = false,
@@ -174,6 +172,13 @@ Creates the Redis Connector object, overring default params with the ones given.
 
 Attempts to create a connection, according to the [params](#parameters) supplied, falling back to defaults given in `new` or the predefined defaults. If a connection cannot be made, returns `nil` and a string describing the reason.
 
+Note that `params` given here do not change the connector's own configuration, and are only used to alter this particular connection operation. As such, the following parameters have no meaning when given in `connect`.
+
+* `keepalive_poolsize`
+* `keepalive_timeout`
+* `connection_is_proxied`
+* `disabled_commands`
+
 
 ### set_keepalive
 

lib/resty/redis/connector.lua

@@ -100,7 +100,7 @@ local DEFAULTS = setmetatable({
     url = "", -- DSN url
 
     master_name = "mymaster",
-    role = "master",  -- master | slave | any
+    role = "master",  -- master | slave
     sentinels = {},
 
     -- Redis proxies typically don't support full Redis capabilities
@@ -130,25 +130,6 @@ local _M = {
 local mt = { __index = _M }
 
 
-function _M.new(config)
-    local ok, config = pcall(tbl_copy_merge_defaults, config, DEFAULTS)
-    if not ok then
-        return nil, config  -- err
-    else
-        -- In proxied Redis mode disable default commands
-        if config.connection_is_proxied == true and
-            not next(config.disabled_commands) then
-
-            config.disabled_commands = default_disabled_commands
-        end
-
-        return setmetatable({
-            config = setmetatable(config, fixed_field_metatable)
-        }, mt)
-    end
-end
-
-
 local function parse_dsn(params)
     local url = params.url
     if url and url ~= "" then
@@ -170,29 +151,58 @@ local function parse_dsn(params)
         -- password may not be present
         if #m < 5 then tbl_remove(fields, 1) end
 
-        local roles = { m = "master", s = "slave", a = "any" }
+        local roles = { m = "master", s = "slave" }
+
+        local parsed_params = {}
 
         for i,v in ipairs(fields) do
-            params[v] = m[i + 1]
+            parsed_params[v] = m[i + 1]
             if v == "role" then
-                params[v] = roles[params[v]]
+                parsed_params[v] = roles[parsed_params[v]]
             end
         end
-    end
 
-    return true, nil
+        return tbl_copy_merge_defaults(params, parsed_params)
+    end
 end
 _M.parse_dsn = parse_dsn
 
 
-function _M.connect(self, params)
-    local params = tbl_copy_merge_defaults(params, self.config)
+function _M.new(config)
+    -- Fill out gaps in config with any dsn params
+    if config and config.url then
+        local err
+        config, err = parse_dsn(config)
+        if not ok then ngx_log(ngx_ERR, err) end
+    end
 
-    if params.url then
-        local ok, err = parse_dsn(params)
+    local ok, config = pcall(tbl_copy_merge_defaults, config, DEFAULTS)
+    if not ok then
+        return nil, config  -- err
+    else
+        -- In proxied Redis mode disable default commands
+        if config.connection_is_proxied == true and
+            not next(config.disabled_commands) then
+
+            config.disabled_commands = default_disabled_commands
+        end
+
+        return setmetatable({
+            config = setmetatable(config, fixed_field_metatable)
+        }, mt)
+    end
+end
+
+
+function _M.connect(self, params)
+    if params and params.url then
+        local err
+        params, err = parse_dsn(params)
         if not ok then ngx_log(ngx_ERR, err) end
     end
 
+    params = tbl_copy_merge_defaults(params, self.config)
+
     if #params.sentinels > 0 then
         return self:connect_via_sentinel(params)
     else
@@ -222,7 +232,7 @@ function _M.connect_via_sentinel(self, params)
         return nil, err, previous_errors
     end
 
-    if role == "master" or role == "any" then
+    if role == "master" then
         local master, err = get_master(sentnl, master_name)
         if master then
             master.db = db
@@ -238,31 +248,32 @@ function _M.connect_via_sentinel(self, params)
                 end
             end
         end
-    end
 
-    -- We either wanted a slave, or are failing over to a slave "any"
-    local slaves, err = get_slaves(sentnl, master_name)
-    sentnl:set_keepalive()
+    else
+        -- We want a slave
+        local slaves, err = get_slaves(sentnl, master_name)
+        sentnl:set_keepalive()
 
-    if not slaves then
-        return nil, err
-    end
+        if not slaves then
+            return nil, err
+        end
 
-    -- Put any slaves on 127.0.0.1 at the front
-    tbl_sort(slaves, sort_by_localhost)
+        -- Put any slaves on 127.0.0.1 at the front
+        tbl_sort(slaves, sort_by_localhost)
 
-    if db or password then
-        for i,slave in ipairs(slaves) do
-            slave.db = db
-            slave.password = password
+        if db or password then
+            for i,slave in ipairs(slaves) do
+                slave.db = db
+                slave.password = password
+            end
         end
-    end
 
-    local slave, err, previous_errors = self:try_hosts(slaves)
-    if not slave then
-        return nil, err, previous_errors
-    else
-        return slave
+        local slave, err, previous_errors = self:try_hosts(slaves)
+        if not slave then
+            return nil, err, previous_errors
+        else
+            return slave
+        end
     end
 end
 

t/connector.t

@@ -223,8 +223,8 @@ location /t {
             path = "unix://tmp/redis.sock",
         }):connect()
 
-		assert(not redis and err == "no such file or directory",
-			"bad domain socket should fail")
+        assert(not redis and err == "no such file or directory",
+            "bad domain socket should fail")
     }
 }
 --- request
@@ -241,13 +241,13 @@ location /t {
     content_by_lua_block {
         local rc = require("resty.redis.connector")
 
-		local params = {
-			url = "redis://foo@127.0.0.1:$TEST_NGINX_REDIS_PORT/4"
-		}
+        local user_params = {
+            url = "redis://foo@127.0.0.1:$TEST_NGINX_REDIS_PORT/4"
+        }
 
-		local ok, err = rc.parse_dsn(params)
-		assert(ok and not err,
-			"url should parse without error: " .. tostring(err))
+        local params, err = rc.parse_dsn(user_params)
+        assert(params and not err,
+            "url should parse without error: " .. tostring(err))
 
 		assert(params.host == "127.0.0.1", "host should be localhost")
 		assert(tonumber(params.port) == $TEST_NGINX_REDIS_PORT,
@@ -256,12 +256,12 @@ location /t {
 		assert(params.password == "foo", "password should be foo")
 
 
-		local params = {
+		local user_params = {
 			url = "sentinel://foo@foomaster:s/2"
 		}
 
-		local ok, err = rc.parse_dsn(params)
-		assert(ok and not err,
+		local params, err = rc.parse_dsn(user_params)
+		assert(params and not err,
 			"url should parse without error: " .. tostring(err))
 
 		assert(params.master_name == "foomaster", "master_name should be foomaster")
@@ -282,3 +282,81 @@ location /t {
 GET /t
 --- no_error_log
 [error]
+
+
+=== TEST 9: params override dsn components
+--- http_config eval: $::HttpConfig
+--- config
+location /t {
+    lua_socket_log_errors Off;
+    content_by_lua_block {
+        local rc = require("resty.redis.connector")
+
+        local user_params = {
+            url = "redis://foo@127.0.0.1:6381/4",
+            db = 2,
+            password = "bar",
+            host = "example.com",
+        }
+
+        local params, err = rc.parse_dsn(user_params)
+        assert(params and not err,
+            "url should parse without error: " .. tostring(err))
+
+        assert(tonumber(params.db) == 2, "db should be 2")
+        assert(params.password == "bar", "password should be bar")
+        assert(params.host == "example.com", "host should be example.com")
+
+        assert(tonumber(params.port) == 6381, "ort should still be 6381")
+
+    }
+}
+--- request
+GET /t
+--- no_error_log
+[error]
+
+
+=== TEST 9: Integration test for parse_dsn
+--- http_config eval: $::HttpConfig
+--- config
+location /t {
+    lua_socket_log_errors Off;
+    content_by_lua_block {
+        local user_params = {
+            url = "redis://foo.example:$TEST_NGINX_REDIS_PORT/4",
+            db = 2,
+            host = "127.0.0.1",
+        }
+
+        local rc, err = require("resty.redis.connector").new(user_params)
+        assert(rc and not err, "new should return positively")
+
+        local redis, err = rc:connect()
+        assert(redis and not err, "connect should return positively")
+        assert(redis:set("cat", "dog") and redis:get("cat") == "dog")
+
+        local redis, err = rc:connect({
+            url = "redis://foo.example:$TEST_NGINX_REDIS_PORT/4",
+            db = 2,
+            host = "127.0.0.1",
+        })
+        assert(redis and not err, "connect should return positively")
+        assert(redis:set("cat", "dog") and redis:get("cat") == "dog")
+
+
+        local rc2, err = require("resty.redis.connector").new()
+        local redis, err = rc2:connect({
+            url = "redis://foo.example:$TEST_NGINX_REDIS_PORT/4",
+            db = 2,
+            host = "127.0.0.1",
+        })
+        assert(redis and not err, "connect should return positively")
+        assert(redis:set("cat", "dog") and redis:get("cat") == "dog")
+
+    }
+}
+--- request
+GET /t
+--- no_error_log
+[error]