From 3cd732ed0c90f443db258766a20b292b003d8d3b Mon Sep 17 00:00:00 2001 From: Gabor Pali Date: Thu, 5 Oct 2023 16:53:34 +0200 Subject: [PATCH 1/2] 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 c707a1c1e0f..7efdbe916af 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. @@ -573,7 +595,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) ] }. @@ -589,7 +613,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, @@ -648,7 +672,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, @@ -664,6 +688,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 b8a6cd9a5aa..b47463d19e8 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 449edf0de03143dbf41e5235d36726d2ed10c0a9 Mon Sep 17 00:00:00 2001 From: Gabor Pali Date: Thu, 5 Oct 2023 16:54:39 +0200 Subject: [PATCH 2/2] 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(