Fix inconsistent mapping rules matching in POST requests #437
Conversation
apicast/src/configuration.lua
Outdated
| -- This will overwrite uri params if they appear in the request body. It's | ||
| -- an arbitrary decision. Overwriting the other way around would be equally | ||
| -- valid. | ||
| for k, v in pairs(body_params) do params[k] = v end |
There was a problem hiding this comment.
I'd suggest you do the reverse: iterate params and overwrite body_params[k] only if body_params[k] is nil and return the body_params table.
I am assuming that POST requests with URI parameters are not the norm (and then probably a few parameters only). IOW, I'd expect most cases to have the size of params smaller or way smaller than the size of body_params. So as is you start with a table, body_params, that is likely to be very similar to what you are going to return, but you are filling up another table that is likely to be empty or mostly empty. You will need O(2*Body+Uri) space (and potential reallocation) to fill in params, whereas you will need O(2*Uri+Body) space to fill in body_params. You will also need O(Body) lookups into params vs O(Uri) lookups in body_params. If the assumption Body >> Uri holds, it would make sense to do the reverse.
There was a problem hiding this comment.
That is a good idea @unleashed 👍
There was a problem hiding this comment.
That's a good point @unleashed . I'll change this.
| } | ||
| --- request | ||
| POST /foo?user_key=somekey | ||
| bar=baz |
There was a problem hiding this comment.
Might be good to test the parameter priority between uri/body.
davidor
force-pushed
the
fix-mapping-rules-matching-post
branch
3 times, most recently
from
4bdd0b3 to
9f0b704
Compare
…ams in uri & body
davidor
force-pushed
the
fix-mapping-rules-matching-post
branch
from
9f0b704 to
b724ffd
Compare
apicast/src/configuration.lua
Outdated
| -- Adds to body_params URI params that are not included in the body. Doing | ||
| -- the reverse would be more expensive, because in general, we expect the | ||
| -- size of body_params to be larger than the size of params. | ||
| for k, v in pairs(params) do |
There was a problem hiding this comment.
This is not JITable. http://wiki.luajit.org/NYI#libraries_base-library
What about using metatable?
setmetatable(body_params, { __index = params })That will just allocate one extra table, but no need for the iteration.
…g rules
Suppose we have this mapping rule:
POST /test?key={value}
Before this change, `curl -XPOST "http://localhost:8080/test?key=hello"
-d "another=parameter"` did not match, but `curl -XPOST
"http://localhost:8080/test" -d "key=hello&another=parameter"` did.
That's because in POST requests, body params were taken into account,
but URI params were not. This commit changes that inconsistent
behavior, now both body & URI params are checked.
davidor
force-pushed
the
fix-mapping-rules-matching-post
branch
from
b724ffd to
5ff88db
Compare
Fixes #163
Suppose we have this mapping rule:
POST /test?key={value}.Before this change,
curl -XPOST "http://localhost:8080/test?key=hello" -d "another=parameter"did not match, butcurl -XPOST "http://localhost:8080/test" -d "key=hello&another=parameter"did.That's because in POST requests, body params were taken into account, but URI params were not. This PR changes that inconsistent behavior. Now both body & URI params are checked.
Please take a look at my comment here : https://github.com/3scale/apicast/compare/fix-mapping-rules-matching-post?expand=1#diff-5b5add30019fd642b1fd7ef108ef978eR75 and let me know if you would do it differently.