-
Notifications
You must be signed in to change notification settings - Fork 169
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Usage: fix issues on encoding #1277
Conversation
14ea4f3
to
f7b11d4
Compare
f7b11d4
to
177616a
Compare
spec/usage_spec.lua
Outdated
|
||
it("with multiple metrics return the same", function() | ||
local usage = Usage.new() | ||
usage:add('a', 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be better to give different values to the metrics to test that they are assigned properly in the resulting string.
assert.are.same(usage:encoded_format(), "usage%5Ba%5D=0&usage%5Bb%5D=0&usage%5Bc%5D=0") | ||
end) | ||
|
||
it("integer metrics", function() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to test this? I don't think this is supported 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking the same, but didn't harm to have just in case.
03f7f39
to
c8b9a18
Compare
usage:add('a', 0) | ||
usage:add('c', 3) | ||
usage:add('b', 2) | ||
assert.are.same(usage:encoded_format(), "usage%5Ba%5D=1&usage%5Bb%5D=2&usage%5Bc%5D=3") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests are failing because of this. It expects a = 1, but above it's set to 0.
Usage keys are based on ngx.encode_args, BUT due to the Luajit update the data is not always the same, so the following usage metatable: ``` { metric_a=1, metric_b=1, } ``` can be `metric_a=1&metric_b=1` or `metric_b=1&metric_a=1`, so the cached key is not the same, and auth call will happen a few more times when it shouldn't. The way that I followed is the same as Openresty, and the escape_uri should always be a string due to the usage method. Fix THREESCALE-7122 Signed-off-by: Eloy Coto <[email protected]>
c8b9a18
to
adc4018
Compare
Usage keys are based on ngx.encode_args, BUT due to the Luajit update
the data is not always the same, so the following usage metatable:
can be
metric_a=1&metric_b=1
ormetric_b=1&metric_a=1
, so the cachedkey is not the same, and auth call will happen a few more times when it
shouldn't.
The way that I followed is the same as Openresty, and the escape_uri
should always be a string due to the usage method.
Fix THREESCALE-7122