adds method matching check to rewrite url captures policy

CHANGELOG.md

@@ -13,6 +13,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 
 ### Added
 
+- Add methods to transformations in rewrite url captures policy [PR #1253](https://github.com/3scale/APIcast/pull/1253) [THREESCALE-6270](https://issues.redhat.com/browse/THREESCALE-6270)
 - Add Access-Control-Max-Age [PR #1247](https://github.com/3scale/APIcast/pull/1247) [THREESCALE-6556](https://issues.redhat.com/browse/THREESCALE-6556)
 - Add HTTP codes policy [PR #1236](https://github.com/3scale/APIcast/pull/1236) [THREESCALE-6255](https://issues.redhat.com/browse/THREESCALE-6255)
 - Buffer access log on chunks [PR #1248](https://github.com/3scale/APIcast/pull/1248) [THREESCALE-6563](https://issues.redhat.com/browse/THREESCALE-6563)

gateway/src/apicast/policy/rewrite_url_captures/apicast-policy.json

@@ -11,6 +11,59 @@
      "'/123/456' will be transformed into '/sales/v2/123?account=456'"],
   "version": "builtin",
   "configuration": {
+    "definitions": {
+      "methods": {
+        "description": "Array of HTTP methods this rule must be applied to. If left blank it will be applied to all HTTP methods",
+        "type": "array",
+        "items": {
+          "type": "string",
+          "oneOf": [
+            {
+              "enum": [
+                "GET"
+              ],
+              "title": "GET"
+            },
+            {
+              "enum": [
+                "POST"
+              ],
+              "title": "POST"
+            },
+            {
+              "enum": [
+                "PUT"
+              ],
+              "title": "PUT"
+            },
+            {
+              "enum": [
+                "PATCH"
+              ],
+              "title": "PATCH"
+            },
+            {
+              "enum": [
+                "DELETE"
+              ],
+              "title": "DELETE"
+            },
+            {
+              "enum": [
+                "HEAD"
+              ],
+              "title": "HEAD"
+            },
+            {
+              "enum": [
+                "OPTIONS"
+              ],
+              "title": "OPTIONS"
+            }
+          ]
+        }
+      }
+    },
     "type": "object",
     "properties": {
       "transformations": {
@@ -25,6 +78,9 @@
             "template": {
               "type": "string",
               "description": "Template in which the matched args are replaced"
+            },
+            "methods": {
+              "$ref": "#/definitions/methods"
             }
           }
         }

gateway/src/apicast/policy/rewrite_url_captures/named_args_matcher.lua

@@ -99,16 +99,35 @@ end
 --   with "{}". For example: "/{var_1}/something/{var_2}".
 -- @tparam string template Template in which the named args matched will be
 --   replaced. For example: "/v2/something/{var_1}?my_arg={var_2}".
-function _M.new(match_rule, template)
+function _M.new(match_rule, template, methods)
   local self = setmetatable({}, mt)
 
   self.named_args = extract_named_args(match_rule)
   self.regex_rule = transform_rule_to_regex(match_rule)
   self.template = template
+  self.methods = methods
 
   return self
 end
 
+-- Returns true if the Method of the request is in the methods of the command meaning the rewrite rule should be applied
+-- Returns true if no Method is provided in the config for backwardscompatibility
+local function is_match_methods(methods)
+
+  if methods == nil or next(methods) == nil  then
+    return true
+  end
+
+  local request_method = ngx.req.get_method()
+
+  for _,v in pairs(methods) do
+    if v == request_method then
+      return true
+    end
+  end
+  return false
+end
+
 --- Match a path
 -- @tparam string path The path of the URL
 -- @treturn boolean True if there's a match, false otherwise
@@ -121,6 +140,11 @@ end
 -- this is so callers can iterate through them with pairs (jitted) instead of
 -- ipairs (non-jitted).
 function _M:match(path)
+
+  if not is_match_methods(self.methods) then
+    return false
+  end
+
   local matches = re_match(path, self.regex_rule, 'oj')
 
   if not matches or #self.named_args ~= #matches then

gateway/src/apicast/policy/rewrite_url_captures/rewrite_url_captures.lua

@@ -27,7 +27,8 @@ function _M.new(config)
   for _, transformation in ipairs(config.transformations or {}) do
     local matcher = NamedArgsMatcher.new(
       transformation.match_rule,
-      transformation.template
+      transformation.template,
+      transformation.methods
     )
 
     insert(self.matchers, matcher)

spec/policy/rewrite_url_captures/named_args_matcher_spec.lua

@@ -1,13 +1,35 @@
 local NamedArgsMatcher = require('apicast.policy.rewrite_url_captures.named_args_matcher')
 
+before_each(function()
+  stub(ngx.req, 'get_method', function() return 'GET' end)
+end)
+
 describe('named_args_matcher', function()
   describe('.match', function()
     describe('when there is a match', function()
-      describe('and there are query args in the template', function()
+      describe('and there are query args in the template and no methods', function()
         it('returns true, the new url and query args', function()
           local match_rule = "/{var_1}/blah/{var_2}/{var_3}"
           local template = "/{var_2}/something/{var_1}?my_arg={var_3}"
-          local matcher = NamedArgsMatcher.new(match_rule, template)
+          local methods = {}
+          local matcher = NamedArgsMatcher.new(match_rule, template, methods)
+          local request_path = "/abc/blah/def/ghi"
+
+          local matched, new_uri, args, arg_vals = matcher:match(request_path)
+
+          assert.is_true(matched)
+          assert.equals('/def/something/abc', new_uri)
+          assert.same({ 'my_arg' }, args)
+          assert.same({ 'ghi' }, arg_vals)
+        end)
+      end)
+
+      describe('and there are query args in the template and methods', function()
+        it('returns true, the new url and query args', function()
+          local match_rule = "/{var_1}/blah/{var_2}/{var_3}"
+          local template = "/{var_2}/something/{var_1}?my_arg={var_3}"
+          local methods = {"GET", "PUT"}
+          local matcher = NamedArgsMatcher.new(match_rule, template, methods)
           local request_path = "/abc/blah/def/ghi"
 
           local matched, new_uri, args, arg_vals = matcher:match(request_path)
@@ -19,11 +41,29 @@ describe('named_args_matcher', function()
         end)
       end)
 
-      describe('and there are no query args in the template', function()
+      describe('and there are no query args in the template and no methods', function()
         it('returns true, the new url and an empty list of args', function()
           local match_rule = "/{var_1}/blah/{var_2}"
           local template = "/{var_2}/something/{var_1}"
-          local matcher = NamedArgsMatcher.new(match_rule, template)
+          local methods = {}
+          local matcher = NamedArgsMatcher.new(match_rule, template, methods)
+          local request_path = "/abc/blah/def"
+
+          local matched, new_uri, args, arg_vals = matcher:match(request_path)
+
+          assert.is_true(matched)
+          assert.equals('/def/something/abc', new_uri)
+          assert.same({}, args)
+          assert.same({}, arg_vals)
+        end)
+      end)
+
+      describe('and there are methods in the template but no query args', function()
+        it('returns true, the new url and an empty list of args', function()
+          local match_rule = "/{var_1}/blah/{var_2}"
+          local template = "/{var_2}/something/{var_1}"
+          local methods = {"GET", "PUT"}
+          local matcher = NamedArgsMatcher.new(match_rule, template, methods)
           local request_path = "/abc/blah/def"
 
           local matched, new_uri, args, arg_vals = matcher:match(request_path)
@@ -39,7 +79,8 @@ describe('named_args_matcher', function()
         it('returns true, the new url and an empty list of args', function()
           local match_rule = "/v2/{var_1}"
           local template = "/?my_arg={var_1}"
-          local matcher = NamedArgsMatcher.new(match_rule, template)
+          local methods = {}
+          local matcher = NamedArgsMatcher.new(match_rule, template, methods)
           local request_path = "/v2/abc"
 
           local matched, new_uri, args, arg_vals = matcher:match(request_path)
@@ -50,14 +91,46 @@ describe('named_args_matcher', function()
           assert.same({ 'abc' }, arg_vals)
         end)
       end)
+
+      describe('and only the params part of the template has args and methods are defined', function()
+        it('returns true, the new url and an empty list of args', function()
+          local match_rule = "/v2/{var_1}"
+          local template = "/?my_arg={var_1}"
+          local methods = {"GET", "PUT"}
+          local matcher = NamedArgsMatcher.new(match_rule, template, methods)
+          local request_path = "/v2/abc"
+
+          local matched, new_uri, args, arg_vals = matcher:match(request_path)
+
+          assert.is_true(matched)
+          assert.equals('/', new_uri)
+          assert.same({ 'my_arg' }, args)
+          assert.same({ 'abc' }, arg_vals)
+        end)
+      end)
     end)
 
     describe('when there is not a match', function()
       describe('because no args matched', function()
         it('returns false', function()
           local match_rule = "/{var_1}/blah/{var_2}/{var_3}"
           local template = "/{var_2}/something/{var_1}?my_arg={var_3}"
-          local matcher = NamedArgsMatcher.new(match_rule, template)
+          local methods = {}
+          local matcher = NamedArgsMatcher.new(match_rule, template, methods)
+          local request_path = "/"
+
+          local matched = matcher:match(request_path)
+
+          assert.is_false(matched)
+        end)
+      end)
+
+      describe('because no args and no methods matched', function()
+        it('returns false', function()
+          local match_rule = "/{var_1}/blah/{var_2}/{var_3}"
+          local template = "/{var_2}/something/{var_1}?my_arg={var_3}"
+          local methods = {"PUT", "POST"}
+          local matcher = NamedArgsMatcher.new(match_rule, template, methods)
           local request_path = "/"
 
           local matched = matcher:match(request_path)
@@ -70,7 +143,22 @@ describe('named_args_matcher', function()
         it('returns false', function()
           local match_rule = "/{var_1}/blah/{var_2}/{var_3}"
           local template = "/{var_2}/something/{var_1}?my_arg={var_3}"
-          local matcher = NamedArgsMatcher.new(match_rule, template)
+          local methods = {}
+          local matcher = NamedArgsMatcher.new(match_rule, template, methods)
+          local request_path = "/abc/blah"
+
+          local matched = matcher:match(request_path)
+
+          assert.is_false(matched)
+        end)
+      end)
+
+      describe('because only some args matched and no methods matched', function()
+        it('returns false', function()
+          local match_rule = "/{var_1}/blah/{var_2}/{var_3}"
+          local template = "/{var_2}/something/{var_1}?my_arg={var_3}"
+          local methods = {"PUT", "POST"}
+          local matcher = NamedArgsMatcher.new(match_rule, template, methods)
           local request_path = "/abc/blah"
 
           local matched = matcher:match(request_path)

spec/policy/rewrite_url_captures/rewrite_url_captures_spec.lua

@@ -11,19 +11,27 @@ describe('Capture args policy', function()
     before_each(function()
       query_params = { set = stub.new() }
       stub(QueryParams, 'new').returns(query_params)
-
+      stub(ngx.req, 'get_method', function() return 'GET' end)
       set_uri_stub = stub(ngx.req, 'set_uri')
 
       ngx.var = { uri = '/abc/def' }
 
       matching_transformation = {
         match_rule = '/{var_1}/{var_2}',
-        template = '/{var_2}?my_arg={var_1}'
+        template = '/{var_2}?my_arg={var_1}',
+        methods = {'GET'}
       }
 
       non_matching_transformation = {
         match_rule = '/i_dont_match/{var_1}/{var_2}',
-        template = '/{var_2}?my_arg={var_1}'
+        template = '/{var_2}?my_arg={var_1}',
+        methods = {'GET'}
+      }
+
+      matching_transformation_no_method = {
+        match_rule = '/{var1}/{var2}',
+        template = '/{var2}?my_arg={var1}',
+        methods = {}
       }
     end)
 
@@ -33,7 +41,44 @@ describe('Capture args policy', function()
           non_matching_transformation,
           matching_transformation
         }
+        local rewrite_url_captures = RewriteUrlCapturesPolicy.new(
+          { transformations = transformations }
+        )
+
+        rewrite_url_captures:rewrite()
+
+        assert.stub(set_uri_stub).was_called_with('/def')
+        assert.stub(query_params.set).was_called_with(
+          query_params, 'my_arg', 'abc')
+      end)
+    end)
+
+    describe('when there is a match but method does not match', function()
+      it('does not modifiy the path nor the params', function()
+        local transformations = {
+          non_matching_transformation,
+          matching_transformation
+        }
+        ngx.req.get_method:revert()
+        stub(ngx.req, 'get_method', function() return 'PUT' end)
+        local rewrite_url_captures = RewriteUrlCapturesPolicy.new(
+          { transformations = transformations }
+        )
 
+        rewrite_url_captures:rewrite()
+
+        assert.stub(set_uri_stub).was_not_called()
+        assert.stub(query_params.set).was_not_called()
+      end)
+    end)
+
+    describe('when there is a match but no method is defined', function()
+      it('modifies the path and the params', function()
+        local transformations = {
+          non_matching_transformation,
+          matching_transformation,
+          matching_transformation_no_method
+        }
         local rewrite_url_captures = RewriteUrlCapturesPolicy.new(
           { transformations = transformations }
         )
@@ -52,7 +97,8 @@ describe('Capture args policy', function()
           match_rule = '/{var_1}/{var_2}',
 
           -- Swap var_1 and var_2 in the original one.
-          template = '/{var_1}?my_arg={var_2}'
+          template = '/{var_1}?my_arg={var_2}',
+          methods = {'POST'}
         }
 
         local transformations = {