-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Mutual exclusive: key, keys, start_key and end_key #4414
Conversation
I like the clarity of the new errors but do not think we should combine |
There is a related issue about how |
2fb9bf7
to
b6bfacc
Compare
869e4c2
to
a1344d4
Compare
key(s)
should incompatible with start_key
and end_key
d5d0f2c
to
465468f
Compare
6646728
to
a9ea220
Compare
a9ea220
to
64804cf
Compare
If we plan on toggling this on in 4.x we should add it to #4480 and make a note in docs about the new setting. Some users may want to enable it in 3.x already. |
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.
Looks good.
Great tests. Just had a few question and a suggestion to simplify the exclusivity check.
64804cf
to
15ff45f
Compare
aa877e8
to
0adbf34
Compare
70fb063
to
44a1028
Compare
- Key and Keys should be incompatible with start_key and end_key - Execute a reduced views, treat a single element keys as the key - Add flag `enable_key_exclusivity` to enable key incompatibility with start_key and end_key
44a1028
to
9c0d290
Compare
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'm -0 on the behaviour change behind the exclusivity flag. I'd rather not have it, but I am not vetoing.
My reasoning is 1) It's disabled by default because 2) it will convert successful requests to failures 3) it will be a long time, if ever, before CouchDB will flip the default 4) It only fixes a subset of the validation issues in the _view endpoint.
I'd rather see us document the actual behaviour, silly as it is.
I'm fine with the part of the PR that treats a single item keys
parameter as if you did key
, since this transforms a current failure into success and (importantly) yielding the correct answer.
I've made a few specific comments on the diff.
@@ -530,9 +551,20 @@ parse_param(Key, Val, Args, IsDecoded) -> | |||
JsonKey = ?JSON_DECODE(Val), | |||
Args#mrargs{start_key = JsonKey, end_key = JsonKey}; | |||
"keys" when IsDecoded -> | |||
Args#mrargs{keys = Val}; | |||
case Val of | |||
[Val1] -> |
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.
Key
would be a better name for Val1
here.
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.
Thanks for your comments, I tried to rewrite this PR here #4494
Changed [Val1]
to [SingleKeys]
because Key
is already used.
Args#mrargs{keys = ?JSON_DECODE(Val)}; | ||
Val1 = ?JSON_DECODE(Val), | ||
case Val1 of | ||
[Val2] -> |
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.
Key
would be a better name for Val2
here. I'd also inline the ?JSON_DECODE so we don't need a Val1
Close this PR as #4494 has been merged. |
Overview
key
behaves as if we setstartkey=$key&endkey=$key
. There is some confusion when we use them together. We should raise a 400 bad request when usingkey
,keys
,startkey
andendkey
together.- Previous behavior:
When setting
endkey
to a, it will return doc a. But if we addkey=c
, it will rewritestartkey
andendkey
to c, then only doc c will be returned.- After the change:
enable_key_exclusivity
to enable/disable mutual exclusivity for keyskey
and single elementkeys
, the response is different. We should treat one elementkeys
the same askey
and respond with the same result. Ifkeys
has multiple elements, it should behave as before.- Previous behavior:
- After the change:
Testing recommendations
make eunit apps=chttpd,couch_mrview
Related Issues or Pull Requests
Fix #3977, #2243
Checklist
rel/overlay/etc/default.ini
src/docs
folder