From e21d0e983001d09cdeb7e4208be6c25cda695733 Mon Sep 17 00:00:00 2001 From: Gabor Pali Date: Thu, 5 Oct 2023 16:53:34 +0200 Subject: [PATCH 1/3] mango: add `allow_fallback` for user-specified indexes on `_find` It is not always beneficial for the performance if the Mango query planner tries to assign an index to the selector. User-specified indexes may save the day, but since they are only hints for the planner, fallbacks may still happen. Introduce the `allow_fallback` flag which can be used to tell if falling back to other indexes is acceptable when an index is explicitly specified by the user. When set to `false`, give up on planning and return an HTTP 400 response right away. This way the user has the chance to learn about the requested but missing index, optionally create it and try again. By default, fallbacks are allowed to maintain backwards compatibility. It is possible to set `allow_fallback` to `true` but currently it coincides with the default behavior hence becomes a no-op in practice. Fixes #4511 --- src/docs/src/api/database/find.rst | 9 +- src/mango/src/mango_cursor.erl | 104 ++++++++++++++++++++-- src/mango/src/mango_error.erl | 22 +++++ src/mango/src/mango_opts.erl | 6 ++ src/mango/test/02-basic-find-test.py | 1 + src/mango/test/05-index-selection-test.py | 25 ++++++ src/mango/test/mango.py | 3 + 7 files changed, 160 insertions(+), 10 deletions(-) diff --git a/src/docs/src/api/database/find.rst b/src/docs/src/api/database/find.rst index dc322538438..884f2c431b0 100644 --- a/src/docs/src/api/database/find.rst +++ b/src/docs/src/api/database/find.rst @@ -50,6 +50,12 @@ is attempted. Therefore that is more like a hint. When fallback occurs, the details are given in the ``warning`` field of the response. *Optional* + : {UsableIndexes, Trace} = mango_idx:get_usable_indexes(Db, Selector, Opts, Kind), case maybe_filter_indexes_by_ddoc(UsableIndexes, Opts) of [] -> - % use_index doesn't match a valid index - fall back to a valid one - create_cursor(Db, {UsableIndexes, Trace}, Selector, Opts); + % use_index doesn't match a valid index - determine how + % this shall be handled by the further settings + case allow_fallback(Opts) of + true -> + % fall back to a valid index + create_cursor(Db, {UsableIndexes, Trace}, Selector, Opts); + false -> + % return an error + Details = + case use_index(Opts) of + [] -> []; + [DesignId] -> [ddoc_name(DesignId)]; + [DesignId, ViewName] -> [ddoc_name(DesignId), ViewName] + end, + ?MANGO_ERROR({invalid_index, Details}) + end; UserSpecifiedIndex -> create_cursor(Db, {UserSpecifiedIndex, Trace}, Selector, Opts) end. @@ -366,13 +380,21 @@ execute(#cursor{index = Idx} = Cursor, UserFun, UserAcc) -> Mod = mango_idx:cursor_mod(Idx), Mod:execute(Cursor, UserFun, UserAcc). +use_index(Opts) -> + {use_index, UseIndex} = lists:keyfind(use_index, 1, Opts), + UseIndex. + +allow_fallback(Opts) -> + {allow_fallback, AllowFallback} = lists:keyfind(allow_fallback, 1, Opts), + AllowFallback. + maybe_filter_indexes_by_ddoc(Indexes, Opts) -> - case lists:keyfind(use_index, 1, Opts) of - {use_index, []} -> + case use_index(Opts) of + [] -> []; - {use_index, [DesignId]} -> + [DesignId] -> filter_indexes(Indexes, DesignId); - {use_index, [DesignId, ViewName]} -> + [DesignId, ViewName] -> filter_indexes(Indexes, DesignId, ViewName) end. @@ -575,7 +597,9 @@ create_test_() -> [ ?TDEF_FE(t_create_regular, 10), ?TDEF_FE(t_create_user_specified_index, 10), - ?TDEF_FE(t_create_invalid_user_specified_index, 10) + ?TDEF_FE(t_create_invalid_user_specified_index, 10), + ?TDEF_FE(t_create_invalid_user_specified_index_no_fallback_1, 10), + ?TDEF_FE(t_create_invalid_user_specified_index_no_fallback_2, 10) ] }. @@ -591,7 +615,7 @@ t_create_regular(_) -> filtered_indexes => sets:from_list(FilteredIndexes), indexes_of_type => sets:from_list(IndexesOfType) }, - Options = [{use_index, []}], + Options = [{use_index, []}, {allow_fallback, true}], meck:expect(mango_selector, normalize, [selector], meck:val(normalized_selector)), meck:expect( mango_idx, @@ -650,7 +674,7 @@ t_create_invalid_user_specified_index(_) -> filtered_indexes => sets:from_list(UsableIndexes), indexes_of_type => sets:from_list(IndexesOfType) }, - Options = [{use_index, [<<"foobar">>]}], + Options = [{use_index, [<<"foobar">>]}, {allow_fallback, true}], meck:expect(mango_selector, normalize, [selector], meck:val(normalized_selector)), meck:expect( mango_idx, @@ -666,6 +690,68 @@ t_create_invalid_user_specified_index(_) -> ), ?assertEqual(view_cursor, create(db, selector, Options, target)). +t_create_invalid_user_specified_index_no_fallback_1(_) -> + IndexSpecial = #idx{type = <<"special">>, def = all_docs}, + IndexView1 = #idx{type = <<"json">>, ddoc = <<"_design/view_idx1">>}, + IndexView2 = #idx{type = <<"json">>, ddoc = <<"_design/view_idx2">>}, + IndexView3 = #idx{type = <<"json">>, ddoc = <<"_design/view_idx3">>}, + UsableIndexes = [IndexSpecial, IndexView1, IndexView2, IndexView3], + IndexesOfType = [IndexView1, IndexView2, IndexView3], + Trace1 = #{}, + Trace2 = + #{ + filtered_indexes => sets:from_list(UsableIndexes), + indexes_of_type => sets:from_list(IndexesOfType) + }, + UseIndex = [<<"design">>, <<"foobar">>], + Options = [{use_index, UseIndex}, {allow_fallback, false}], + meck:expect(mango_selector, normalize, [selector], meck:val(normalized_selector)), + meck:expect( + mango_idx, + get_usable_indexes, + [db, normalized_selector, Options, target], + meck:val({UsableIndexes, Trace1}) + ), + meck:expect( + mango_cursor_view, + create, + [db, {IndexesOfType, Trace2}, normalized_selector, Options], + meck:val(view_cursor) + ), + Exception = {mango_error, mango_cursor, {invalid_index, UseIndex}}, + ?assertThrow(Exception, create(db, selector, Options, target)). + +t_create_invalid_user_specified_index_no_fallback_2(_) -> + IndexSpecial = #idx{type = <<"special">>, def = all_docs}, + IndexView1 = #idx{type = <<"json">>, ddoc = <<"_design/view_idx1">>}, + IndexView2 = #idx{type = <<"json">>, ddoc = <<"_design/view_idx2">>}, + IndexView3 = #idx{type = <<"json">>, ddoc = <<"_design/view_idx3">>}, + UsableIndexes = [IndexSpecial, IndexView1, IndexView2, IndexView3], + IndexesOfType = [IndexView1, IndexView2, IndexView3], + Trace1 = #{}, + Trace2 = + #{ + filtered_indexes => sets:from_list(UsableIndexes), + indexes_of_type => sets:from_list(IndexesOfType) + }, + UseIndex = [], + Options = [{use_index, UseIndex}, {allow_fallback, false}], + meck:expect(mango_selector, normalize, [selector], meck:val(normalized_selector)), + meck:expect( + mango_idx, + get_usable_indexes, + [db, normalized_selector, Options, target], + meck:val({UsableIndexes, Trace1}) + ), + meck:expect( + mango_cursor_view, + create, + [db, {IndexesOfType, Trace2}, normalized_selector, Options], + meck:val(view_cursor) + ), + Exception = {mango_error, mango_cursor, {invalid_index, UseIndex}}, + ?assertThrow(Exception, create(db, selector, Options, target)). + enhance_candidates_test() -> Candidates1 = #{index => #{reason => [], usable => true}}, Candidates2 = #{index => #{reason => [reason1], usable => true}}, diff --git a/src/mango/src/mango_error.erl b/src/mango/src/mango_error.erl index f1c343854f3..989e9fa1245 100644 --- a/src/mango/src/mango_error.erl +++ b/src/mango/src/mango_error.erl @@ -48,6 +48,28 @@ info(mango_json_bookmark, {invalid_bookmark, BadBookmark}) -> <<"invalid_bookmark">>, fmt("Invalid bookmark value: ~s", [?JSON_ENCODE(BadBookmark)]) }; +info(mango_cursor, {invalid_index, []}) -> + { + 400, + <<"invalid_index">>, + <<"You must specify an index with the `use_index` parameter.">> + }; +info(mango_cursor, {invalid_index, [DDocName]}) -> + { + 400, + <<"invalid_index">>, + fmt("_design/~s specified by `use_index` could not be found or it is not suitable.", [ + DDocName + ]) + }; +info(mango_cursor, {invalid_index, [DDocName, ViewName]}) -> + { + 400, + <<"invalid_index">>, + fmt("_design/~s, ~s specified by `use_index` could not be found or it is not suitable.", [ + DDocName, ViewName + ]) + }; info(mango_cursor_text, {invalid_bookmark, BadBookmark}) -> { 400, diff --git a/src/mango/src/mango_opts.erl b/src/mango/src/mango_opts.erl index 96aa0eb42ef..1d3ded1701f 100644 --- a/src/mango/src/mango_opts.erl +++ b/src/mango/src/mango_opts.erl @@ -162,6 +162,12 @@ validate_find({Props}) -> {optional, true}, {default, false}, {validator, fun mango_opts:is_boolean/1} + ]}, + {<<"allow_fallback">>, [ + {tag, allow_fallback}, + {optional, true}, + {default, true}, + {validator, fun mango_opts:is_boolean/1} ]} ], validate(Props, Opts). diff --git a/src/mango/test/02-basic-find-test.py b/src/mango/test/02-basic-find-test.py index 6544df8489f..9a701b06db8 100644 --- a/src/mango/test/02-basic-find-test.py +++ b/src/mango/test/02-basic-find-test.py @@ -311,6 +311,7 @@ def test_explain_options(self): assert opts["stale"] == False assert opts["update"] == True assert opts["use_index"] == [] + assert opts["allow_fallback"] == True def test_sort_with_all_docs(self): explain = self.db.find( diff --git a/src/mango/test/05-index-selection-test.py b/src/mango/test/05-index-selection-test.py index a4c15608a09..c4f245bdb27 100644 --- a/src/mango/test/05-index-selection-test.py +++ b/src/mango/test/05-index-selection-test.py @@ -211,6 +211,31 @@ def test_explain_sort_reverse(self): ) self.assertEqual(resp_explain["index"]["type"], "json") + def test_use_index_without_fallback(self): + with self.subTest(use_index="valid"): + docs = self.db.find( + {"manager": True}, use_index="manager", allow_fallback=False + ) + assert len(docs) > 0 + + with self.subTest(use_index="invalid"): + try: + self.db.find( + {"manager": True}, use_index="invalid", allow_fallback=False + ) + except Exception as e: + self.assertEqual(e.response.status_code, 400) + else: + raise AssertionError("did not fail on invalid index") + + with self.subTest(use_index="empty"): + try: + self.db.find({"manager": True}, use_index=[], allow_fallback=False) + except Exception as e: + self.assertEqual(e.response.status_code, 400) + else: + raise AssertionError("did not fail due to missing use_index") + class JSONIndexSelectionTests(mango.UserDocsTests, IndexSelectionTests): @classmethod diff --git a/src/mango/test/mango.py b/src/mango/test/mango.py index 3ead0655cce..a23ba314714 100644 --- a/src/mango/test/mango.py +++ b/src/mango/test/mango.py @@ -282,6 +282,7 @@ def find( update=True, executionStats=False, partition=None, + allow_fallback=None, ): body = { "selector": selector, @@ -301,6 +302,8 @@ def find( body["update"] = False if executionStats == True: body["execution_stats"] = True + if allow_fallback is not None: + body["allow_fallback"] = allow_fallback body = json.dumps(body) if partition: ppath = "_partition/{}/".format(partition) From 21d6e7b7228240d6483c9b9cd6118c576d0e1dad Mon Sep 17 00:00:00 2001 From: Gabor Pali Date: Thu, 5 Oct 2023 16:54:39 +0200 Subject: [PATCH 2/3] mango: fix validation of `use_index` The `binary:split/2` function will split the input only at the first occurrence of the pattern which is not suitable for identifying each part of the index name, therefore validating it. Add the `global` option to fully break the index name into parts and let the related validation logic work as expected. --- src/mango/src/mango_error.erl | 6 ++++++ src/mango/src/mango_opts.erl | 2 +- src/mango/test/05-index-selection-test.py | 10 ++++++++++ 3 files changed, 17 insertions(+), 1 deletion(-) diff --git a/src/mango/src/mango_error.erl b/src/mango/src/mango_error.erl index 989e9fa1245..830b36dd1fa 100644 --- a/src/mango/src/mango_error.erl +++ b/src/mango/src/mango_error.erl @@ -253,6 +253,12 @@ info(mango_opts, {invalid_bulk_docs, Val}) -> [Val] ) }; +info(mango_opts, {invalid_index_name, Val}) -> + { + 400, + <<"invalid_index_name">>, + fmt("Invalid index name: ~w", [Val]) + }; info(mango_opts, {invalid_ejson, Val}) -> { 400, diff --git a/src/mango/src/mango_opts.erl b/src/mango/src/mango_opts.erl index 1d3ded1701f..c846bce14b0 100644 --- a/src/mango/src/mango_opts.erl +++ b/src/mango/src/mango_opts.erl @@ -262,7 +262,7 @@ validate_bulk_docs(Else) -> ?MANGO_ERROR({invalid_bulk_docs, Else}). validate_use_index(IndexName) when is_binary(IndexName) -> - case binary:split(IndexName, <<"/">>) of + case binary:split(IndexName, <<"/">>, [global]) of [DesignId] -> {ok, [DesignId]}; [<<"_design">>, DesignId] -> diff --git a/src/mango/test/05-index-selection-test.py b/src/mango/test/05-index-selection-test.py index c4f245bdb27..0cbbd0b8013 100644 --- a/src/mango/test/05-index-selection-test.py +++ b/src/mango/test/05-index-selection-test.py @@ -211,6 +211,16 @@ def test_explain_sort_reverse(self): ) self.assertEqual(resp_explain["index"]["type"], "json") + def test_use_index_with_invalid_name(self): + for index in ["foo/bar/baz", ["foo", "bar", "baz"]]: + with self.subTest(index=index): + try: + self.db.find({"manager": True}, use_index=index) + except Exception as e: + self.assertEqual(e.response.status_code, 400) + else: + raise AssertionError("did not fail on invalid index name") + def test_use_index_without_fallback(self): with self.subTest(use_index="valid"): docs = self.db.find( From c92c389f2a05f8ae73146f91aef7c56636abe74b Mon Sep 17 00:00:00 2001 From: Gabor Pali Date: Tue, 8 Oct 2024 00:27:31 +0200 Subject: [PATCH 3/3] fixup: extend `allow_fallback` to always exclude `all_docs` --- src/docs/src/api/database/find.rst | 22 +++++----- src/mango/src/mango_cursor.erl | 27 ++++++------ src/mango/test/05-index-selection-test.py | 50 ++++++++++++++++++++--- 3 files changed, 68 insertions(+), 31 deletions(-) diff --git a/src/docs/src/api/database/find.rst b/src/docs/src/api/database/find.rst index 884f2c431b0..462d841b51c 100644 --- a/src/docs/src/api/database/find.rst +++ b/src/docs/src/api/database/find.rst @@ -19,11 +19,11 @@ .. http:post:: /{db}/_find :synopsis: Find documents within a given database. - Find documents using a declarative JSON querying syntax. - Queries will use custom indexes, specified using the :ref:`_index ` - endpoint, if available. - Otherwise, they use the built-in :ref:`_all_docs ` index, which - can be arbitrarily slow. + Find documents using a declarative JSON querying syntax. Queries + will use custom indexes, specified using the :ref:`_index + ` endpoint, if available. Otherwise, when + allowed, they use the built-in :ref:`_all_docs ` + index, which can be arbitrarily slow. :param db: Database name @@ -51,11 +51,13 @@ fallback occurs, the details are given in the ``warning`` field of the response. *Optional* :` index would be picked in lack of indexes + available to support the query. Disabling this fallback logic + causes the endpoint immediately return an error in such cases. + Default is ``true``. *Optional* : Selector = mango_selector:normalize(Selector0), {UsableIndexes, Trace} = mango_idx:get_usable_indexes(Db, Selector, Opts, Kind), + UseIndex = use_index(Opts), case maybe_filter_indexes_by_ddoc(UsableIndexes, Opts) of [] -> % use_index doesn't match a valid index - determine how % this shall be handled by the further settings - case allow_fallback(Opts) of + case allow_fallback(Opts) orelse (UseIndex == [] andalso length(UsableIndexes) > 1) of true -> % fall back to a valid index create_cursor(Db, {UsableIndexes, Trace}, Selector, Opts); false -> - % return an error + % no usable index but all_docs, no fallback allowed: return an error Details = - case use_index(Opts) of + case UseIndex of [] -> []; [DesignId] -> [ddoc_name(DesignId)]; [DesignId, ViewName] -> [ddoc_name(DesignId), ViewName] @@ -598,8 +599,8 @@ create_test_() -> ?TDEF_FE(t_create_regular, 10), ?TDEF_FE(t_create_user_specified_index, 10), ?TDEF_FE(t_create_invalid_user_specified_index, 10), - ?TDEF_FE(t_create_invalid_user_specified_index_no_fallback_1, 10), - ?TDEF_FE(t_create_invalid_user_specified_index_no_fallback_2, 10) + ?TDEF_FE(t_create_invalid_user_specified_index_no_fallback, 10), + ?TDEF_FE(t_create_no_suitable_index_no_fallback, 10) ] }. @@ -690,7 +691,7 @@ t_create_invalid_user_specified_index(_) -> ), ?assertEqual(view_cursor, create(db, selector, Options, target)). -t_create_invalid_user_specified_index_no_fallback_1(_) -> +t_create_invalid_user_specified_index_no_fallback(_) -> IndexSpecial = #idx{type = <<"special">>, def = all_docs}, IndexView1 = #idx{type = <<"json">>, ddoc = <<"_design/view_idx1">>}, IndexView2 = #idx{type = <<"json">>, ddoc = <<"_design/view_idx2">>}, @@ -721,21 +722,17 @@ t_create_invalid_user_specified_index_no_fallback_1(_) -> Exception = {mango_error, mango_cursor, {invalid_index, UseIndex}}, ?assertThrow(Exception, create(db, selector, Options, target)). -t_create_invalid_user_specified_index_no_fallback_2(_) -> +t_create_no_suitable_index_no_fallback(_) -> IndexSpecial = #idx{type = <<"special">>, def = all_docs}, - IndexView1 = #idx{type = <<"json">>, ddoc = <<"_design/view_idx1">>}, - IndexView2 = #idx{type = <<"json">>, ddoc = <<"_design/view_idx2">>}, - IndexView3 = #idx{type = <<"json">>, ddoc = <<"_design/view_idx3">>}, - UsableIndexes = [IndexSpecial, IndexView1, IndexView2, IndexView3], - IndexesOfType = [IndexView1, IndexView2, IndexView3], + UsableIndexes = [IndexSpecial], + IndexesOfType = [], Trace1 = #{}, Trace2 = #{ filtered_indexes => sets:from_list(UsableIndexes), indexes_of_type => sets:from_list(IndexesOfType) }, - UseIndex = [], - Options = [{use_index, UseIndex}, {allow_fallback, false}], + Options = [{use_index, []}, {allow_fallback, false}], meck:expect(mango_selector, normalize, [selector], meck:val(normalized_selector)), meck:expect( mango_idx, @@ -749,7 +746,7 @@ t_create_invalid_user_specified_index_no_fallback_2(_) -> [db, {IndexesOfType, Trace2}, normalized_selector, Options], meck:val(view_cursor) ), - Exception = {mango_error, mango_cursor, {invalid_index, UseIndex}}, + Exception = {mango_error, mango_cursor, {invalid_index, []}}, ?assertThrow(Exception, create(db, selector, Options, target)). enhance_candidates_test() -> diff --git a/src/mango/test/05-index-selection-test.py b/src/mango/test/05-index-selection-test.py index 0cbbd0b8013..e484781e989 100644 --- a/src/mango/test/05-index-selection-test.py +++ b/src/mango/test/05-index-selection-test.py @@ -222,13 +222,13 @@ def test_use_index_with_invalid_name(self): raise AssertionError("did not fail on invalid index name") def test_use_index_without_fallback(self): - with self.subTest(use_index="valid"): + with self.subTest(use_index="valid", fallback_available=None): docs = self.db.find( {"manager": True}, use_index="manager", allow_fallback=False ) assert len(docs) > 0 - with self.subTest(use_index="invalid"): + with self.subTest(use_index="invalid", fallback_available=True): try: self.db.find( {"manager": True}, use_index="invalid", allow_fallback=False @@ -236,15 +236,53 @@ def test_use_index_without_fallback(self): except Exception as e: self.assertEqual(e.response.status_code, 400) else: - raise AssertionError("did not fail on invalid index") + raise AssertionError("did not fail on invalid index for use_index") - with self.subTest(use_index="empty"): + with self.subTest(use_index="empty", fallback_available=True): try: - self.db.find({"manager": True}, use_index=[], allow_fallback=False) + docs = self.db.find( + {"manager": True}, use_index=[], allow_fallback=False + ) + assert len(docs) > 0 + except Exception as e: + raise AssertionError( + "fail due to missing use_index with suitable indexes" + ) + + with self.subTest(use_index="empty", fallback_available=False): + try: + self.db.find({"company": "foobar"}, use_index=[], allow_fallback=False) + except Exception as e: + self.assertEqual(e.response.status_code, 400) + else: + raise AssertionError( + "did not fail due to missing use_index without suitable indexes" + ) + + with self.subTest(use_index="invalid", fallback_available=False): + try: + self.db.find( + {"company": "foobar"}, use_index="invalid", allow_fallback=False + ) except Exception as e: self.assertEqual(e.response.status_code, 400) else: - raise AssertionError("did not fail due to missing use_index") + raise AssertionError("did not fail on invalid index for use_index") + + def test_index_without_fallback(self): + try: + docs = self.db.find({"manager": True}, allow_fallback=False) + assert len(docs) > 0 + except Exception as e: + raise AssertionError("fail on usable indexes") + + def test_no_index_without_fallback(self): + try: + self.db.find({"company": "foobar"}, allow_fallback=False) + except Exception as e: + self.assertEqual(e.response.status_code, 400) + else: + raise AssertionError("did not fail on no usable indexes") class JSONIndexSelectionTests(mango.UserDocsTests, IndexSelectionTests):