Skip to content
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

Add key range check for _all_docs #4518

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jiahuili430
Copy link
Contributor

@jiahuili430 jiahuili430 commented Apr 7, 2023

Overview

Using key and start/end_key in a different order will produce different responses when querying _all_docs.
To reduce confusion, add a key range check for _all_docs.

  • If direction=fwd, start_key > end_key throws an error
  • If direction=rev, start_key < end_key throws an error
  • Otherwise, return relevant responses

Previous behavior:

$ curl -XPUT $DB/db
$ curl -XPOST $DB/db/_bulk_docs -H 'Content-Type: application/json' -d '{"docs": [{"_id": "a"},{"_id": "b"},{"_id": "c"}]}'

$ curl $DB/db/_all_docs'?key="c"&endkey="a"'
{"total_rows":3,"offset":2,"rows":[ ]}

$ curl $DB/db/_all_docs'?key="c"&endkey="a"&descending=true'
{"total_rows":3,"offset":0,"rows":[
  {"id":"c","key":"c","value":{"rev":"1-967a00dff5e02add41819138abb3284d"}},
  {"id":"b","key":"b","value":{"rev":"1-967a00dff5e02add41819138abb3284d"}},
  {"id":"a","key":"a","value":{"rev":"1-967a00dff5e02add41819138abb3284d"}}
]}

After the change:

$ curl $DB/db/_all_docs'?key="c"&endkey="a"'
{"error":"query_parse_error","reason":"No rows can match your key range, reverse your start_key and end_key or set descending=true"}

$ curl $DB/db/_all_docs'?key="c"&endkey="a"&descending=true'
{"total_rows":3,"offset":0,"rows":[
  {"id":"c","key":"c","value":{"rev":"1-967a00dff5e02add41819138abb3284d"}},
  {"id":"b","key":"b","value":{"rev":"1-967a00dff5e02add41819138abb3284d"}},
  {"id":"a","key":"a","value":{"rev":"1-967a00dff5e02add41819138abb3284d"}}
]}

Testing recommendations

make eunit apps=chttpd,couch_mrview

Related Issues or Pull Requests

#3977

Checklist

  • Code is written and works correctly
  • Changes are covered by tests
  • Any new configurable parameters are documented in rel/overlay/etc/default.ini
  • Documentation changes were made in the src/docs folder
  • Documentation changes were backported (separated PR) to affected branches

@jiahuili430 jiahuili430 marked this pull request as draft April 8, 2023 01:31
@jiahuili430 jiahuili430 changed the title Add key range check for _all_docs md5: testing only Apr 26, 2023
@jiahuili430 jiahuili430 force-pushed the key-range-check branch 5 times, most recently from 89790d1 to f42b978 Compare April 26, 2023 20:07
@jiahuili430 jiahuili430 changed the title md5: testing only Add key range check for _all_docs Apr 26, 2023
Using key and start/end_key in a different order will produce
different responses when querying `_all_docs`. To reduce confusion,
add a key range check for `_all_docs`.

- If direction=fwd, start_key > end_key throws an error
- If direction=rev, start_key < end_key throws an error
- Otherwise, return relevant responses
@jiahuili430 jiahuili430 marked this pull request as ready for review June 19, 2023 21:35
Copy link
Member

@rnewson rnewson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the change itself looks fine (though I have a question on why this extra argument validation is not happening inside validate_args/1) but the backward compatibility issue needs discussing. This change means a user will now get an error when previously they got success (with no rows).

@@ -533,6 +533,7 @@ apply_limit(ViewPartitioned, Args) ->

validate_all_docs_args(Db, Args0) ->
Args = validate_args(Args0),
check_range_all_docs(Args),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not put this inside validate_args?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried that before but it fails in mango tests related to _find endpoint.
https://ci-couchdb.apache.org/blue/organizations/jenkins/jenkins-cm1%2FPullRequests/detail/PR-4518/9/pipeline/

ERROR: test_gte_respsects_unicode_collation (03-operator-test.OperatorJSONTests)
----------------------------------------------------------------------
Traceback (most recent call last):
 File "/home/jenkins/workspace/jenkins-cm1_PullRequests_PR-4518/25.3/build/apache-couchdb-3.3.2-04ead65/src/mango/test/03-operator-test.py", line 187, in test_gte_respsects_unicode_collation
   docs = self.db.find({"ordered": {"$gte": "a"}})
 File "/home/jenkins/workspace/jenkins-cm1_PullRequests_PR-4518/25.3/build/apache-couchdb-3.3.2-04ead65/src/mango/test/mango.py", line 280, in find
   r.raise_for_status()
 File "/home/jenkins/workspace/jenkins-cm1_PullRequests_PR-4518/25.3/build/apache-couchdb-3.3.2-04ead65/src/mango/.venv/lib/python3.9/site-packages/requests/models.py", line 960, in raise_for_status
   raise HTTPError(http_error_msg, response=self)
requests.exceptions.HTTPError: 400 Client Error: Bad Request for url: http://127.0.0.1:15984/mango_test_cad95f52e95341e097ce2c1d861c1457/_find
======================================================================
ERROR: test_old_selector_with_no_selector_still_supported (16-index-selectors-test.IndexSelectorJson)
... ...

@janl
Copy link
Member

janl commented Jun 22, 2023

This change means a user will now get an error when previously they got success (with no rows).

With more of these cleanups coming in (thanks @jiahuili430!) we may have to bite the bullet and think about API versioning. We have discussed several approaches in the past but nothing has ever stuck.

It might be worth bringing this up in the next developer meeting for a high-level discussion. Until we have a better idea, the best idea I’d have is to put this (and other improvements) behind a [chttpd] strict_api = false | true or some such config flag, but it might be better to hold this PR until we have a general solution for this.

@rnewson
Copy link
Member

rnewson commented Jun 22, 2023

I like the idea of a flag but wouldn't we need a roadmap for the full set of changes first? (in a python 2/3 way)

@janl
Copy link
Member

janl commented Jun 22, 2023

the flag would be a stopgap until we have proper versioning, it’d be a toggle between version 0 and 1, and later we allow versions 2, 3, 4 etc. once we know what these look like. — I wouldn’t mind parking this PR until we have a proper system set up and skip the binary flag for now, but if we would want the PR to land quickly, that’d be the fasted way there, but yeah, we probably want a complete scrub before we introduce even the toggle.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants