"Rewrite URL captures" policy #827

davidor · 2018-07-30T15:49:32Z

Closes #711 (first use case)

After the discussion in #711, I think that implementing the first use case of that issue with a new policy seems reasonable. We need something for the next release, although in the future, we might be able to do something better as we add more features to the conditional policy.

Regarding the name of the policy, I'm open to suggestions. I can't think of a good name for it 🤔

davidor · 2018-07-30T18:31:11Z

t/apicast-policy-rewrite-url-captures.t

+          { "pattern": "/", "http_method": "GET", "metric_system_name": "hits", "delta": 2 }
+        ],
+        "policy_chain": [
+          { "name": "apicast.policy.apicast" },


I'll change these tests so they use the Echo policy instead. I think that will simplify the code.

I'll keep just one with the APIcast policy just to make sure that there are not any problems when combining it with the new policy.

I'd love that 👍

I just realized that we can't do that because the Echo policy uses ngx.say(ngx.var.request), so it just works with the original request.

So the upstream policy pointing to the echo endpoint? That is still simpler than APIcast + dependencies.

Using the echo policy requires more work because we are only interested in checking the path and the query args, but the echo returns more data.

In the end, I opted for deleting the APIcast policy from the chain and adding an upstream policy that redirects all the request to the upstream defining in the config. I think this is the simplest solution.

I kept one of the tests I had with the APIcast policy just to make sure that both policies can be combined without any problems.

mikz · 2018-07-31T13:04:08Z

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

+  local replaced_template = replace_in_template(
+    self.named_args, matches, self.template)
+
+  local uri, raw_params = unpack(re_split(replaced_template, '\\?'))


unpack can't be JITed so this should not be in the hot path.

And instead of '\\?' you can use [[\?]], but that might be just my preference.

Fixed the unpack.
I don't have a strong preference on \\? vs [[\?]] so I changed it.

mikz · 2018-07-31T13:05:54Z

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

+  local res = template
+
+  for i, arg in ipairs(args) do
+    res = re_gsub(res, "{" .. arg .. "}", vals[i], 'oj')


This is allocating 3 strings in a loop. Better to use string.format and even better to do this during policy initialization.

Right. It turns out that we always use the result of this wrapped between "{" and "}" so I concatenate them in extract_named_args which is only called from .new().

mikz · 2018-07-31T13:06:25Z

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

+  local params_split = re_split(string_params, '&')
+
+  for _, param in ipairs(params_split) do
+    local name, val = unpack(re_split(param, '='))


Another unpack in a loop. This will prevent JIT.

mikz · 2018-07-31T13:07:14Z

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

+local function string_params_to_table(string_params)
+  local res = {}
+
+  local params_split = re_split(string_params, '&')


Maybe good to extract re_split into own function split that will always pass oj flags.

Right, that way we won't forget to add oj. Fixed 👍

So it can be used from other policies, and not just from the URL rewriting one.

mikz · 2018-07-31T16:13:56Z

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

+  local params_split = split(string_params, '&')
+
+  for _, param in ipairs(params_split) do
+    local parts = split(param, '=') -- avoid unpack, not jitted.


You might want to limit the number of parts you want by setting the max results parameter.

mikz · 2018-07-31T16:15:16Z

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

+    local parts = split(param, '=') -- avoid unpack, not jitted.
+    local name = parts[1]
+    local val = parts[2]
+    res[name] = val


LuaJIT performance guide says it is better not to create short lived temporary variables like these and just use the tuple:

res[parts[1]] = parts[2]

I think that defining those vars helps readability. I doubt this will have a noticeable impact.

mikz · 2018-07-31T16:16:42Z

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

+local function set_query_params(new_params)
+  local query_params = QueryParams.new()
+
+  for arg, value in pairs(new_params) do


pairs is not JITed. Does it need to be key valued table? Integer indexed could be JITed.

mikz · 2018-07-31T16:18:53Z

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

+end
+
+local function uri_and_params_from_template(template)
+  local parts = split(template, [[\?]]) -- avoid unpack, not jitted.


Here it might need to limit the results too.

davidor · 2018-08-01T08:34:12Z

@mikz Regarding the name of the policy, do you have any suggestions?

davidor · 2018-08-01T10:17:04Z

I addressed all your comments @mikz

…ing to URL captures

mikz · 2018-08-03T09:10:41Z

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

+end
+
+-- Returns a list of named args extracted from a match_rule.
+-- For example, for the rule /{abc}/{def}?{ghi}=1, it returns this list:


So, do you think it makes sense to use just {} instead of {{ }} to match with the liquid syntax?

I think {} is better in this case because it's the same format that we are using for the mapping rules.

Ok, thats fair point. And for the replacement ? Shouldn't we just pass it through liquid?

Not sure. That would only replace the code we have in replace_in_template which is not problematic, in my opinion.

Yep. I'm thinking about consistency. If push people everywhere to use liquid, then it would make sense to stick to it. I have no hard feelings one way or the other, just want to avoid confused people that can't use what they do everywhere else.

But it will not be hard to migrate. Just adding the template_type. So if you have no feelings one way or the other then lets merge this 👍

I'd rather leave this one as it is, at least for now 👍

mikz · 2018-08-03T09:12:40Z

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

+function _M.new(config)
+  local self = new(config)
+
+  self.matchers = {}


Just for the future. Using tab_new(#config.transformations, 0) would save table expansion because it would preallocate slots in the table for each matcher.

mikz

I think this is great! 👍

My only opened question is if we should make this look like liquid.

And regarding the name, I personally think this is fine, but @3scale/product should have say in this.

davidor requested a review from a team as a code owner July 30, 2018 15:49

davidor commented Jul 30, 2018

View reviewed changes

mikz reviewed Jul 31, 2018

View reviewed changes

davidor added 2 commits July 31, 2018 17:12

Move QueryParams to apicast dir

5fb1567

So it can be used from other policies, and not just from the URL rewriting one.

policy: add schema for new Rewrite URL captures policy

7b25ec3

davidor force-pushed the named-args-policy branch from abf0448 to b47a619 Compare July 31, 2018 15:50

mikz reviewed Jul 31, 2018

View reviewed changes

davidor force-pushed the named-args-policy branch 2 times, most recently from 1d797ad to d9c7b97 Compare August 1, 2018 10:15

davidor force-pushed the named-args-policy branch from d9c7b97 to 70f81b8 Compare August 1, 2018 10:17

davidor added 6 commits August 1, 2018 12:26

policy/rewrite_url_captures: add module that generates new URL accord…

e3a682c

…ing to URL captures

spec/policy/rewrite_url_captures: add specs for NamedArgsMatcher

960ec07

policy: add RewriteURLCaptures policy

185c29c

spec/policy: add specs for RewriteURLCaptures policy

1dd7398

t: add tests for rewrite URL captures policy

83b90c7

CHANGELOG: add RewriteURLCaptures policy

306619a

davidor force-pushed the named-args-policy branch from 70f81b8 to 306619a Compare August 1, 2018 10:27

davidor requested a review from mikz August 3, 2018 08:53

mikz reviewed Aug 3, 2018

View reviewed changes

mikz approved these changes Aug 3, 2018

View reviewed changes

mikz requested a review from a team August 3, 2018 09:15

davidor merged commit 6324e2b into master Aug 3, 2018

davidor deleted the named-args-policy branch August 3, 2018 09:46

mikz mentioned this pull request Sep 19, 2018

Query string policy: Manipulate request URI query string #711

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

"Rewrite URL captures" policy #827

"Rewrite URL captures" policy #827

davidor commented Jul 30, 2018

davidor Jul 30, 2018

mikz Jul 31, 2018

davidor Jul 31, 2018

mikz Jul 31, 2018

davidor Aug 1, 2018

mikz Jul 31, 2018

mikz Jul 31, 2018

davidor Jul 31, 2018

mikz Jul 31, 2018

davidor Jul 31, 2018

mikz Jul 31, 2018

davidor Jul 31, 2018

mikz Jul 31, 2018

davidor Jul 31, 2018

mikz Jul 31, 2018

davidor Aug 1, 2018

mikz Jul 31, 2018

davidor Aug 1, 2018

mikz Jul 31, 2018

mikz Jul 31, 2018

davidor commented Aug 1, 2018

davidor commented Aug 1, 2018

mikz Aug 3, 2018

davidor Aug 3, 2018 •

edited

Loading

mikz Aug 3, 2018

davidor Aug 3, 2018

mikz Aug 3, 2018

davidor Aug 3, 2018

mikz Aug 3, 2018 •

edited

Loading

mikz left a comment

"Rewrite URL captures" policy #827

"Rewrite URL captures" policy #827

Conversation

davidor commented Jul 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidor commented Aug 1, 2018

davidor commented Aug 1, 2018

Choose a reason for hiding this comment

davidor Aug 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikz Aug 3, 2018 • edited Loading

Choose a reason for hiding this comment

mikz left a comment

Choose a reason for hiding this comment

davidor Aug 3, 2018 •

edited

Loading

mikz Aug 3, 2018 •

edited

Loading