diff options
author | Nick Vatamaniuc <vatamane@apache.org> | 2020-07-30 17:21:25 -0400 |
---|---|---|
committer | Nick Vatamaniuc <nickva@users.noreply.github.com> | 2020-07-31 12:08:43 -0400 |
commit | 8b49b0d63485afe68d559f3532be4827eee5bd8c (patch) | |
tree | 91a7cc8af5bc98f0aab7efcdf178e89f86f23d31 | |
parent | 41d946b5e08f59339033ceccbcea607edd35479a (diff) | |
download | couchdb-8b49b0d63485afe68d559f3532be4827eee5bd8c.tar.gz |
Allow interactive requests to reopen a re-created db instance
Previously, if a database was re-created on another node, a request with that
database might have found the previous db instance in the cache. In that case
it would have correctly reopened the db while in a transaction, but, because
the old db instance was deleted it would throw a database_does_not_exist which
was not the correct behavior.
To prevent that from happening, introduce an interactive = true|false option
when opening a database. User requests may specify that option and then when
the db is re-opened, it will allow it to automatically upgrade to the new db
instance instead returning an error.
Background processes will still get a database_doest_not_exist error if they
keep a db open which has now been re-created.
The interactive option may also be used in the future to set other transaction
parameters like timeouts and retries that might be different for interactive
requests vs background tasks.
-rw-r--r-- | src/chttpd/src/chttpd_db.erl | 3 | ||||
-rw-r--r-- | src/fabric/src/fabric2_db.erl | 8 | ||||
-rw-r--r-- | src/fabric/src/fabric2_fdb.erl | 24 | ||||
-rw-r--r-- | src/fabric/src/fabric2_server.erl | 3 | ||||
-rw-r--r-- | src/fabric/test/fabric2_db_crud_tests.erl | 28 | ||||
-rw-r--r-- | src/fabric/test/fabric2_db_security_tests.erl | 55 |
6 files changed, 99 insertions, 22 deletions
diff --git a/src/chttpd/src/chttpd_db.erl b/src/chttpd/src/chttpd_db.erl index fdaf4af8c..8acccb461 100644 --- a/src/chttpd/src/chttpd_db.erl +++ b/src/chttpd/src/chttpd_db.erl @@ -395,7 +395,8 @@ delete_db_req(#httpd{user_ctx=Ctx}=Req, DbName) -> end. do_db_req(#httpd{path_parts=[DbName|_], user_ctx=Ctx}=Req, Fun) -> - {ok, Db} = fabric2_db:open(DbName, [{user_ctx, Ctx}]), + Options = [{user_ctx, Ctx}, {interactive, true}], + {ok, Db} = fabric2_db:open(DbName, Options), Fun(Req, Db). db_req(#httpd{method='GET',path_parts=[_DbName]}=Req, Db) -> diff --git a/src/fabric/src/fabric2_db.erl b/src/fabric/src/fabric2_db.erl index 667cf35c6..4ac105589 100644 --- a/src/fabric/src/fabric2_db.erl +++ b/src/fabric/src/fabric2_db.erl @@ -201,7 +201,8 @@ open(DbName, Options) -> case fabric2_server:fetch(DbName, UUID) of #{} = Db -> Db1 = maybe_set_user_ctx(Db, Options), - {ok, require_member_check(Db1)}; + Db2 = maybe_set_interactive(Db1, Options), + {ok, require_member_check(Db2)}; undefined -> Result = fabric2_fdb:transactional(DbName, Options, fun(TxDb) -> fabric2_fdb:open(TxDb, Options) @@ -1426,6 +1427,11 @@ get_all_docs_meta(TxDb, Options) -> end ++ [{total, DocCount}, {offset, null}]. +maybe_set_interactive(#{} = Db, Options) -> + Interactive = fabric2_util:get_value(interactive, Options, false), + Db#{interactive := Interactive}. + + maybe_set_user_ctx(Db, Options) -> case fabric2_util:get_value(user_ctx, Options) of #user_ctx{} = UserCtx -> diff --git a/src/fabric/src/fabric2_fdb.erl b/src/fabric/src/fabric2_fdb.erl index f721ca4ab..52303cef1 100644 --- a/src/fabric/src/fabric2_fdb.erl +++ b/src/fabric/src/fabric2_fdb.erl @@ -243,7 +243,8 @@ create(#{} = Db0, Options) -> after_doc_read => undefined, % All other db things as we add features, - db_options => Options1 + db_options => Options1, + interactive => false }, aegis:init_db(Db2, Options). @@ -270,6 +271,9 @@ open(#{} = Db0, Options) -> UUID = fabric2_util:get_value(uuid, Options1), Options2 = lists:keydelete(uuid, 1, Options1), + Interactive = fabric2_util:get_value(interactive, Options2, false), + Options3 = lists:keydelete(interactive, 1, Options2), + Db2 = Db1#{ db_prefix => DbPrefix, db_version => DbVersion, @@ -287,7 +291,8 @@ open(#{} = Db0, Options) -> before_doc_update => undefined, after_doc_read => undefined, - db_options => Options2 + db_options => Options3, + interactive => Interactive }, Db3 = load_config(Db2), @@ -318,7 +323,8 @@ refresh(#{tx := undefined, name := DbName} = Db) -> #{md_version := Ver} = Db1 when Ver > OldVer -> Db1#{ user_ctx := maps:get(user_ctx, Db), - security_fun := maps:get(security_fun, Db) + security_fun := maps:get(security_fun, Db), + interactive := maps:get(interactive, Db) }; _ -> Db @@ -337,18 +343,20 @@ reopen(#{} = OldDb) -> uuid := UUID, db_options := Options, user_ctx := UserCtx, - security_fun := SecurityFun + security_fun := SecurityFun, + interactive := Interactive } = OldDb, Options1 = lists:keystore(user_ctx, 1, Options, {user_ctx, UserCtx}), NewDb = open(init_db(Tx, DbName, Options1), Options1), % Check if database was re-created - case maps:get(uuid, NewDb) of - UUID -> ok; - _OtherUUID -> error(database_does_not_exist) + case {Interactive, maps:get(uuid, NewDb)} of + {true, _} -> ok; + {false, UUID} -> ok; + {false, _OtherUUID} -> error(database_does_not_exist) end, - NewDb#{security_fun := SecurityFun}. + NewDb#{security_fun := SecurityFun, interactive := Interactive}. delete(#{} = Db) -> diff --git a/src/fabric/src/fabric2_server.erl b/src/fabric/src/fabric2_server.erl index b557da8c7..be674b10e 100644 --- a/src/fabric/src/fabric2_server.erl +++ b/src/fabric/src/fabric2_server.erl @@ -271,5 +271,6 @@ sanitize(#{} = Db) -> Db#{ tx := undefined, user_ctx := #user_ctx{}, - security_fun := undefined + security_fun := undefined, + interactive := false }. diff --git a/src/fabric/test/fabric2_db_crud_tests.erl b/src/fabric/test/fabric2_db_crud_tests.erl index 000f3709c..3d90c65b5 100644 --- a/src/fabric/test/fabric2_db_crud_tests.erl +++ b/src/fabric/test/fabric2_db_crud_tests.erl @@ -38,6 +38,8 @@ crud_test_() -> ?TDEF_FE(open_db), ?TDEF_FE(delete_db), ?TDEF_FE(recreate_db), + ?TDEF_FE(recreate_db_interactive), + ?TDEF_FE(recreate_db_non_interactive), ?TDEF_FE(undelete_db), ?TDEF_FE(remove_deleted_db), ?TDEF_FE(scheduled_remove_deleted_db, 15), @@ -179,6 +181,32 @@ recreate_db(_) -> ?assertError(database_does_not_exist, fabric2_db:open(DbName, BadOpts)). +recreate_db_interactive(_) -> + DbName = ?tempdb(), + ?assertMatch({ok, _}, fabric2_db:create(DbName, [])), + + {ok, Db1} = fabric2_db:open(DbName, [{interactive, true}]), + + ?assertEqual(ok, fabric2_db:delete(DbName, [])), + ?assertMatch({ok, _}, fabric2_db:create(DbName, [])), + + ?assertMatch({ok, _}, fabric2_db:get_db_info(Db1)). + + +recreate_db_non_interactive(_) -> + % This is also the default case, but we check that parsing the `false` open + % value works correctly. + DbName = ?tempdb(), + ?assertMatch({ok, _}, fabric2_db:create(DbName, [])), + + {ok, Db1} = fabric2_db:open(DbName, [{interactive, false}]), + + ?assertEqual(ok, fabric2_db:delete(DbName, [])), + ?assertMatch({ok, _}, fabric2_db:create(DbName, [])), + + ?assertError(database_does_not_exist, fabric2_db:get_db_info(Db1)). + + undelete_db(_) -> DbName = ?tempdb(), ?assertError(database_does_not_exist, fabric2_db:delete(DbName, [])), diff --git a/src/fabric/test/fabric2_db_security_tests.erl b/src/fabric/test/fabric2_db_security_tests.erl index 063979a3f..3d7167a00 100644 --- a/src/fabric/test/fabric2_db_security_tests.erl +++ b/src/fabric/test/fabric2_db_security_tests.erl @@ -40,7 +40,8 @@ security_test_() -> ?TDEF(check_set_user_ctx), ?TDEF(check_forbidden), ?TDEF(check_fail_no_opts), - ?TDEF(check_fail_name_null) + ?TDEF(check_fail_name_null), + ?TDEF(check_forbidden_with_interactive_reopen) ]) } }. @@ -51,6 +52,18 @@ setup() -> DbName = ?tempdb(), PubDbName = ?tempdb(), {ok, Db1} = fabric2_db:create(DbName, [?ADMIN_CTX]), + ok = set_test_security(Db1), + {ok, _} = fabric2_db:create(PubDbName, [?ADMIN_CTX]), + {DbName, PubDbName, Ctx}. + + +cleanup({DbName, PubDbName, Ctx}) -> + ok = fabric2_db:delete(DbName, []), + ok = fabric2_db:delete(PubDbName, []), + test_util:stop_couch(Ctx). + + +set_test_security(Db) -> SecProps = {[ {<<"admins">>, {[ {<<"names">>, [<<"admin_name1">>, <<"admin_name2">>]}, @@ -61,16 +74,7 @@ setup() -> {<<"roles">>, [<<"member_role1">>, <<"member_role2">>]} ]}} ]}, - ok = fabric2_db:set_security(Db1, SecProps), - {ok, _} = fabric2_db:create(PubDbName, [?ADMIN_CTX]), - {DbName, PubDbName, Ctx}. - - -cleanup({DbName, PubDbName, Ctx}) -> - ok = fabric2_db:delete(DbName, []), - ok = fabric2_db:delete(PubDbName, []), - test_util:stop_couch(Ctx). - + ok = fabric2_db:set_security(Db, SecProps). check_is_admin({DbName, _, _}) -> @@ -184,3 +188,32 @@ check_fail_name_null({DbName, _, _}) -> UserCtx = #user_ctx{name = null}, {ok, Db} = fabric2_db:open(DbName, [{user_ctx, UserCtx}]), ?assertThrow({unauthorized, _}, fabric2_db:get_db_info(Db)). + + +check_forbidden_with_interactive_reopen({DbName, _, _}) -> + UserCtx = #user_ctx{name = <<"foo">>}, + Options = [{user_ctx, UserCtx}, {interactive, true}], + + {ok, Db1} = fabric2_db:open(DbName, Options), + + % Verify foo is forbidden by default + ?assertThrow({forbidden, _}, fabric2_db:get_db_info(Db1)), + + % Allow foo + {ok, Db2} = fabric2_db:open(DbName, [?ADMIN_CTX]), + AllowFoo = {[ + {<<"members">>, {[ + {<<"names">>, [<<"foo">>]} + ]}} + ]}, + ok = fabric2_db:set_security(Db2, AllowFoo), + + ?assertMatch({ok, _}, fabric2_db:get_db_info(Db1)), + + % Recreate test db instance with the default security + ok = fabric2_db:delete(DbName, [?ADMIN_CTX]), + {ok, Db3} = fabric2_db:create(DbName, [?ADMIN_CTX]), + ok = set_test_security(Db3), + + % Original handle is forbidden to again + ?assertThrow({forbidden, _}, fabric2_db:get_db_info(Db1)). |