couchdb icon indicating copy to clipboard operation
couchdb copied to clipboard

Add key range check for _all_docs

Open jiahuili430 opened this issue 2 years ago • 3 comments

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

https://github.com/apache/couchdb/issues/3977

Checklist

  • [ ] Code is written and works correctly
  • [x] 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 avatar Apr 07 '23 21:04 jiahuili430

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.

janl avatar Jun 22 '23 12:06 janl

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)

rnewson avatar Jun 22 '23 13:06 rnewson

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.

janl avatar Jun 22 '23 13:06 janl