[GitHub] [apisix] Firstsawyou commented on a change in pull request #4244: feat: support uri encoding in redirect

[GitBox <gi...@apache.org>]

Firstsawyou commented on a change in pull request #4244:
URL: https://github.com/apache/apisix/pull/4244#discussion_r631858010



##########
File path: t/plugin/redirect.t
##########
@@ -813,3 +813,100 @@ GET /hello
 hello world
 --- no_error_log
 [error]
+
+
+
+=== TEST 33: add plugin with new regex_uri: encode_uri = true
+--- config
+    location /t {
+        content_by_lua_block {
+            local t = require("lib.test_admin").test
+            local code, body = t('/apisix/admin/routes/1',
+                ngx.HTTP_PUT,
+                [[{
+                    "plugins": {
+                        "redirect": {
+                            "regex_uri": ["^/test/(.*)", "http://test.com/${1}"],
+                            "ret_code": 301,
+                            "encode_uri": true
+                        }
+                    },
+                    "uri": "/test/*",
+                    "upstream": {
+                        "nodes": {
+                            "127.0.0.1:1980": 1
+                        },
+                        "type": "roundrobin"
+                    }
+                }]]
+                )
+
+            if code >= 300 then
+                ngx.status = code
+            end
+            ngx.say(body)
+        }
+    }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 34: regex_uri redirect with special characters
+--- request
+GET /test/with%20space
+--- error_code: 200
+--- response_headers
+Location: http://test.com/with%20space
+--- error_code: 301
+--- no_error_log
+[error]
+
+
+
+=== TEST 35: add plugin with new uri: encode_uri = true
+--- config
+    location /t {
+        content_by_lua_block {
+            local t = require("lib.test_admin").test
+            local code, body = t('/apisix/admin/routes/1',
+                ngx.HTTP_PUT,
+                [[{
+                    "plugins": {
+                        "redirect": {
+                            "uri": "$uri",
+                            "ret_code": 301,
+                            "encode_uri": true
+                        }
+                    },
+                    "uri": "/hello*"
+                }]]
+                )
+
+            if code >= 300 then
+                ngx.status = code
+            end
+            ngx.say(body)
+        }
+    }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 36: redirect with special characters
+--- request
+GET /hello/with%20space
+--- response_headers
+Location: /hello/with%20space
+--- error_code: 301
+--- no_error_log
+[error]

Review comment:
       A blank line needs to be added at the end of the file.

##########
File path: apisix/plugins/redirect.lua
##########
@@ -160,17 +163,17 @@ function _M.rewrite(conf, ctx)
     end
 
     if ret_code then
+        local new_uri
         if uri then
-            local new_uri, err = concat_new_uri(uri, ctx)
+            local err
+            new_uri, err = concat_new_uri(uri, ctx)
             if not new_uri then
                 core.log.error("failed to generate new uri by: " .. uri .. err)
                 return 500
             end
-
-            core.response.set_header("Location", new_uri)
-            return ret_code
         elseif regex_uri then
-            local new_uri, n, err = re_sub(ctx.var.uri, regex_uri[1],
+            local n, err
+            new_uri, n, err = re_sub(ctx.var.uri, regex_uri[1],
                                            regex_uri[2], "jo")

Review comment:
       ```suggestion
               new_uri, n, err = re_sub(ctx.var.uri, regex_uri[1],
                                        regex_uri[2], "jo")
   ```
   This style is better.

##########
File path: apisix/plugins/redirect.lua
##########
@@ -22,6 +22,8 @@ local re_gmatch = ngx.re.gmatch
 local re_sub = ngx.re.sub
 local ipairs = ipairs
 local ngx = ngx
+local str_find    = core.string.find
+local sub_str     = string.sub

Review comment:
       ```suggestion
   local str_find = core.string.find
   local str_sub  = string.sub
   ```
   I think this would be better.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org