Add key range check for _all_docs

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
apache · Jun 19, 2023 · 3fc6fe2 · 3fc6fe2
1 parent 290ea87
commit 3fc6fe2
Show file tree

Hide file tree

Showing 3 changed files with 59 additions and 37 deletions.
diff --git a/src/chttpd/test/eunit/chttpd_view_test.erl b/src/chttpd/test/eunit/chttpd_view_test.erl
@@ -42,6 +42,12 @@
     <<"error">> := <<"query_parse_error">>,
     <<"reason">> := <<"`keys` is incompatible with `key`, `start_key` and `end_key`">>
 }).
+-define(ERROR_KEY_RANGE, #{
+    <<"error">> := <<"query_parse_error">>,
+    <<"reason">> :=
+        <<"No rows can match your key range, reverse your ",
+            "start_key and end_key or set descending=true">>
+}).
 
 % seconds
 -define(TIMEOUT, 60).
@@ -163,26 +169,17 @@ t_view_with_multiple_queries(Db) ->
     ?assertEqual(200, Code).
 
 t_view_with_key_and_start_key(Db) ->
-    {Code1, Res1} = req(get, url(Db, "_design/ddoc/_view/map", "key=\"a\"&startkey=\"b\"")),
-    {Code2, Res2} = req(get, url(Db, "_design/ddoc/_view/map", "startkey=\"b\"&key=\"a\"")),
-    ?assertMatch(
-        #{
-            <<"error">> := <<"query_parse_error">>,
-            <<"reason">> :=
-                <<"No rows can match your key range, reverse your start_key and end_key or set descending=true">>
-        },
-        Res1
-    ),
-    ?assertMatch(#{<<"rows">> := [#{<<"id">> := <<"a">>}]}, Res2),
-    ?assertEqual(400, Code1),
-    ?assertEqual(200, Code2).
+    test_helper_key_and_start_key(Db, "_design/ddoc/_view/map").
 
 t_all_docs_with_key_and_start_key(Db) ->
-    {Code1, Res1} = req(get, url(Db, "_all_docs", "key=\"a\"&startkey=\"b\"")),
-    {Code2, Res2} = req(get, url(Db, "_all_docs", "startkey=\"b\"&key=\"a\"")),
-    ?assertMatch(#{<<"rows">> := []}, Res1),
+    test_helper_key_and_start_key(Db, "_all_docs").
+
+test_helper_key_and_start_key(Db, Path) ->
+    {Code1, Res1} = req(get, url(Db, Path, "key=\"a\"&startkey=\"b\"")),
+    {Code2, Res2} = req(get, url(Db, Path, "startkey=\"b\"&key=\"a\"")),
+    ?assertMatch(?ERROR_KEY_RANGE, Res1),
     ?assertMatch(#{<<"rows">> := [#{<<"id">> := <<"a">>}]}, Res2),
-    ?assertEqual(200, Code1),
+    ?assertEqual(400, Code1),
     ?assertEqual(200, Code2).
 
 t_view_with_key_and_end_key(Db) ->
@@ -200,31 +197,35 @@ test_helper_key_and_end_key(Db, Path) ->
     ?assertEqual(200, Code2).
 
 t_view_with_single_keys_and_start_key(Db) ->
-    {Code, Res} = req(get, url(Db, "_design/ddoc/_view/map?keys=[\"a\"]&startkey=\"b\"")),
-    ?assertMatch(
-        #{
-            <<"error">> := <<"query_parse_error">>,
-            <<"reason">> :=
-                <<"No rows can match your key range, reverse your start_key and end_key or set descending=true">>
-        },
-        Res
-    ),
-    ?assertEqual(400, Code).
+    test_helper_single_keys_and_start_key(Db, "_design/ddoc/_view/map").
 
 t_all_docs_with_single_keys_and_start_key(Db) ->
-    {Code, Res} = req(get, url(Db, "_all_docs?keys=[\"a\"]&startkey=\"b\"")),
-    ?assertMatch(?ERROR_KEYS_INCOMPATIBLE, Res),
-    ?assertEqual(400, Code).
+    test_helper_single_keys_and_start_key(Db, "_all_docs").
+
+test_helper_single_keys_and_start_key(Db, Path) ->
+    {Code1, Res1} = req(get, url(Db, Path, "keys=[\"a\"]&startkey=\"b\"")),
+    {Code2, Res2} = req(get, url(Db, Path, "startkey=\"b\"&keys=[\"a\"]")),
+    case Path of
+        "_all_docs" -> ?assertMatch(?ERROR_KEYS_INCOMPATIBLE, Res1);
+        _ -> ?assertMatch(?ERROR_KEY_RANGE, Res1)
+    end,
+    ?assertMatch(#{<<"rows">> := [#{<<"id">> := <<"a">>}]}, Res2),
+    ?assertEqual(400, Code1),
+    ?assertEqual(200, Code2).
 
 t_view_with_keys_and_start_key(Db) ->
-    {Code, Res} = req(get, url(Db, "_design/ddoc/_view/map", "keys=[\"a\",\"b\"]&start_key=\"b\"")),
-    ?assertMatch(?ERROR_KEYS_INCOMPATIBLE, Res),
-    ?assertEqual(400, Code).
+    test_helper_keys_and_start_key(Db, "_design/ddoc/_view/map").
 
 t_all_docs_with_keys_and_start_key(Db) ->
-    {Code, Res} = req(get, url(Db, "_all_docs", "keys=[\"a\",\"b\"]&start_key=\"b\"")),
-    ?assertMatch(?ERROR_KEYS_INCOMPATIBLE, Res),
-    ?assertEqual(400, Code).
+    test_helper_keys_and_start_key(Db, "_all_docs").
+
+test_helper_keys_and_start_key(Db, Path) ->
+    {Code1, Res1} = req(get, url(Db, Path, "keys=[\"a\",\"b\"]&start_key=\"b\"")),
+    {Code2, Res2} = req(get, url(Db, Path, "startkey=\"b\"&keys=[\"a\",\"b\"]")),
+    ?assertMatch(?ERROR_KEYS_INCOMPATIBLE, Res1),
+    ?assertMatch(?ERROR_KEYS_INCOMPATIBLE, Res2),
+    ?assertEqual(400, Code1),
+    ?assertEqual(400, Code2).
 
 t_view_with_key_non_existent_docs(Db) ->
     {Code, Res} = req(get, url(Db, "_design/ddoc/_view/map", "key=\"not_exist\"")),

diff --git a/src/couch_mrview/src/couch_mrview_util.erl b/src/couch_mrview/src/couch_mrview_util.erl
@@ -533,6 +533,7 @@ apply_limit(ViewPartitioned, Args) ->
 
 validate_all_docs_args(Db, Args0) ->
     Args = validate_args(Args0),
+    check_range_all_docs(Args),
 
     DbPartitioned = couch_db:is_partitioned(Db),
     Partition = get_extra(Args, partition),
@@ -782,6 +783,26 @@ apply_all_docs_partition(#mrargs{} = Args, Partition) ->
         end_key = EK1
     }.
 
+check_range_all_docs(#mrargs{direction = Dir, start_key = SK, end_key = EK}) ->
+    case {Dir, SK, EK} of
+        {_, undefined, _} ->
+            ok;
+        {_, _, undefined} ->
+            ok;
+        {fwd, SK, EK} when SK > EK ->
+            mrverror(
+                <<"No rows can match your key range, reverse your ",
+                    "start_key and end_key or set descending=true">>
+            );
+        {rev, SK, EK} when SK < EK ->
+            mrverror(
+                <<"No rows can match your key range, reverse your ",
+                    "start_key and end_key or set descending=false">>
+            );
+        _ ->
+            ok
+    end.
+
 check_range(#mrargs{start_key = undefined}, _Cmp) ->
     ok;
 check_range(#mrargs{end_key = undefined}, _Cmp) ->

diff --git a/src/docs/src/api/ddoc/views.rst b/src/docs/src/api/ddoc/views.rst
@@ -651,7 +651,7 @@ multi-element ``keys``. For single-element ``keys``, treat it as a ``key``.
     $ curl http://adm:[email protected]:5984/db/_design/ddoc/_view/reduce'?key="a"'
     {"rows":[{"key":null,"value":1}]}
 
-    $ curl http://adm:[email protected]:5984/db/_design/ddoc/_view/reduce'?keys="[\"a\"]"'
+    $ curl http://adm:[email protected]:5984/db/_design/ddoc/_view/reduce'?keys=\["a"\]'
     {"rows":[{"key":null,"value":1}]}
 
     $ curl http://adm:[email protected]:5984/db/_design/ddoc/_view/reduce'?keys=\["a","b"\]'