URL rewriting policy: Allow to modify query parameters #724
Conversation
| "oneOf": [ | ||
| { | ||
| "enum": ["add"], | ||
| "title": "Set an argument if it does not exist" |
There was a problem hiding this comment.
This is different from what we have in the headers policy. There it is "append".
Query string arguments are quite similar and we have to have some "append" operation.
There was a problem hiding this comment.
You're right. I forgot about values with several arguments.
I've adapted the code to take this case into account. The big changes are in query_params.lua. The other changes that I needed to make were basically adapting the rest of modules to use the correct names of the operations.
| assert.stub(ngx.req.set_uri_args).was_called_with({ an_arg = 'a_val' }) | ||
| end) | ||
|
|
||
| it('can replace arguments from the query', function() |
There was a problem hiding this comment.
These tests duplicate tests of QueryParams.
I think we are over testing this a bit. We have 3 tests for the same code.
Not sure what is the best path forward though.
There was a problem hiding this comment.
I see your point. In particular, some of these tests check that ngx.req.set_uri_args was called with the proper arguments, but that shouldn't be a concern of url_rewriting. It should be of query_params, and it's being already tested on its specs.
However, it's a bit tricky to test this properly as QueryParams.new() is called from rewrite() and it does not make sense to inject an instance of QueryParams there.
There was a problem hiding this comment.
Check the latest commit. It mocks QueryParams to avoid testing its internals.
davidor
force-pushed
the
querystring-ops-url-rewriting-policy
branch
2 times, most recently
from
3bee4a7 to
d25202e
Compare
| } | ||
| } | ||
| local url_rewriting = URLRewriting.new(config_to_add_args) | ||
|
|
||
| url_rewriting:rewrite() | ||
|
|
||
| assert.stub(ngx.req.set_uri_args).was_called_with({ an_arg = { '1', '2' } }) | ||
| assert.spy(spy_mocked_query_params_push).was_called_with( |
There was a problem hiding this comment.
Looks like all these tests are sharing one spy. Is that safe?
There was a problem hiding this comment.
Changed. They need to be instantiated in the before_each to reset their calls.
davidor
force-pushed
the
querystring-ops-url-rewriting-policy
branch
from
d25202e to
f83af88
Compare
davidor
force-pushed
the
querystring-ops-url-rewriting-policy
branch
from
f83af88 to
73745e9
Compare
| -- 2) When the arg is set, replaces its value with the given one. | ||
| -- 3) Deletes an arg when the value is "". | ||
| function _M:set(arg, value) | ||
| if value == '' then |
There was a problem hiding this comment.
Hmmm. This might not be right. HTTP headers are not sent at all by nginx when the they are empty, but Query string args can be binary like ?foo&bar&search. We might need to have "delete" operation after all.
There was a problem hiding this comment.
Right. I forgot about this corner case.
There was a problem hiding this comment.
Fixed. Check the latest commit.
Codecov Report
@@ Coverage Diff @@
## master #724 +/- ##
==========================================
+ Coverage 88.06% 88.13% +0.07%
==========================================
Files 177 179 +2
Lines 8460 8662 +202
==========================================
+ Hits 7450 7634 +184
- Misses 1010 1028 +18
Continue to review full report at Codecov.
|
…in combination with upstream policy
We can't rely on deleting a query arg by setting it to empty string because queries like /?a&b&c are valid.
davidor
force-pushed
the
querystring-ops-url-rewriting-policy
branch
from
73745e9 to
bf094e5
Compare
This PR implements part of #711
It adds the ability to modify query parameters in the URL rewriting policy.