diff options
author | Nick Vatamaniuc <vatamane@gmail.com> | 2022-06-05 00:04:47 -0400 |
---|---|---|
committer | Nick Vatamaniuc <nickva@users.noreply.github.com> | 2022-06-07 11:37:46 -0400 |
commit | 14a5213470f9372b2d97af0bfc214f438d059132 (patch) | |
tree | abbb028a1cf3e34a88b7337bfc44467932ad25cf | |
parent | 369ecc98115f60ee6cabb4dac246dcb6817c56a6 (diff) | |
download | couchdb-14a5213470f9372b2d97af0bfc214f438d059132.tar.gz |
Remove duplicate _revs_diff implementation
The implementation was duplicated because fabric needed to return a response
([]) for existing revs. It doesn't seem worth it to keep a whole separate copy
just to handle the [] case. Instead, make the primary couchdb implementation
always return [] and just filter it out for the local httpd endpoint handlers.
It seems we didn't have any _revs_diff tests, so add a few basic tests along
the way.
Also, it turns out we still support the _missing_revs API. That's an endpoint
that's not used by the replicator since before 1.2 days. At some point it might
be worth removing it, but for now ensure we also test it alongside _revs_diff.
-rw-r--r-- | src/chttpd/test/eunit/chttpd_revs_diff_tests.erl | 238 | ||||
-rw-r--r-- | src/couch/src/couch_db.erl | 87 | ||||
-rw-r--r-- | src/couch/src/couch_httpd_db.erl | 14 | ||||
-rw-r--r-- | src/fabric/src/fabric_rpc.erl | 53 |
4 files changed, 290 insertions, 102 deletions
diff --git a/src/chttpd/test/eunit/chttpd_revs_diff_tests.erl b/src/chttpd/test/eunit/chttpd_revs_diff_tests.erl new file mode 100644 index 000000000..9a9bd25b7 --- /dev/null +++ b/src/chttpd/test/eunit/chttpd_revs_diff_tests.erl @@ -0,0 +1,238 @@ +% Licensed under the Apache License, Version 2.0 (the "License"); you may not +% use this file except in compliance with the License. You may obtain a copy of +% the License at +% +% http://www.apache.org/licenses/LICENSE-2.0 +% +% Unless required by applicable law or agreed to in writing, software +% distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +% WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +% License for the specific language governing permissions and limitations under +% the License. + +-module(chttpd_revs_diff_tests). + +-include_lib("couch/include/couch_eunit.hrl"). +-include_lib("couch/include/couch_db.hrl"). + +-define(TDEF_FE(Name), fun(Arg) -> {atom_to_list(Name), ?_test(Name(Arg))} end). + +-define(USER, "chttpd_revs_diff_test_admin"). +-define(PASS, "pass"). +-define(AUTH, {basic_auth, {?USER, ?PASS}}). +-define(JSON, {"Content-Type", "application/json"}). + +-define(DOC1, <<"doc1">>). +-define(DOC2, <<"doc2">>). +-define(REVA, <<"reva">>). +-define(REVB, <<"revb">>). +-define(REVC, <<"revc">>). +-define(REVD, <<"revd">>). + +test_docs() -> + [ + {?DOC1, [?REVB, ?REVA]}, + {?DOC1, [?REVC, ?REVA]}, + {?DOC2, [?REVD]} + ]. + +setup() -> + Hashed = couch_passwords:hash_admin_password(?PASS), + ok = config:set("admins", ?USER, ?b2l(Hashed), _Persist = false), + Addr = config:get("chttpd", "bind_address", "127.0.0.1"), + Db = binary_to_list(?tempdb()), + Port = mochiweb_socket_server:get(chttpd, port), + Url = lists:concat(["http://", Addr, ":", Port, "/"]), + ok = create_db(Url, Db), + ok = create_docs(Url, Db, test_docs()), + {Url, Db}. + +teardown({Url, Db}) -> + delete_db(Url, Db), + ok = config:delete("admins", ?USER, _Persist = false). + +start_couch() -> + test_util:start_couch([chttpd]). + +stop_couch(Ctx) -> + test_util:stop_couch(Ctx). + +chttpd_revs_diff_test_() -> + { + "chttpd _revs_diff tests", + { + setup, + fun start_couch/0, + fun stop_couch/1, + { + foreach, + fun setup/0, + fun teardown/1, + [ + ?TDEF_FE(t_empty_revs_diff), + ?TDEF_FE(t_revs_diff_no_revs), + ?TDEF_FE(t_revs_diff_non_existent_doc), + ?TDEF_FE(t_revs_diff_all_revs), + ?TDEF_FE(t_revs_diff_some_missing_some_not), + ?TDEF_FE(t_empty_missing_revs), + ?TDEF_FE(t_missing_revs_no_revs), + ?TDEF_FE(t_missing_revs_non_existent_doc), + ?TDEF_FE(t_missing_revs_all_revs), + ?TDEF_FE(t_missing_revs_some_missing_some_not) + ] + } + } + }. + +t_empty_revs_diff({Top, Db}) -> + {Code, Res} = req(post, Top ++ Db ++ "/_revs_diff", #{}), + ?assertEqual(200, Code), + ?assertEqual(#{}, Res). + +t_revs_diff_no_revs({Top, Db}) -> + Body = #{?DOC1 => [], <<"non_existent_doc">> => []}, + {Code, Res} = req(post, Top ++ Db ++ "/_revs_diff", Body), + ?assertEqual(200, Code), + ?assertEqual(#{}, Res). + +t_revs_diff_non_existent_doc({Top, Db}) -> + Body = #{<<"non_existent_doc">> => [<<"1-rev">>]}, + {Code, Res} = req(post, Top ++ Db ++ "/_revs_diff", Body), + ?assertEqual(200, Code), + ?assertEqual( + #{ + <<"non_existent_doc">> => #{ + <<"missing">> => [<<"1-rev">>] + } + }, + Res + ). + +t_revs_diff_all_revs({Top, Db}) -> + Body = #{ + ?DOC1 => [<<"2-", ?REVB/binary>>, <<"2-", ?REVC/binary>>], + ?DOC2 => [<<"1-", ?REVD/binary>>] + }, + {Code, Res} = req(post, Top ++ Db ++ "/_revs_diff", Body), + ?assertEqual(200, Code), + ?assertEqual(#{}, Res). + +t_revs_diff_some_missing_some_not({Top, Db}) -> + Body = #{ + ?DOC1 => [<<"2-", ?REVB/binary>>, <<"1-xyz">>, <<"2-def">>, <<"3-klm">>], + ?DOC2 => [<<"1-pqr">>] + }, + {Code, Res} = req(post, Top ++ Db ++ "/_revs_diff", Body), + ?assertEqual(200, Code), + ?assertEqual( + #{ + ?DOC1 => #{ + <<"missing">> => [<<"1-xyz">>, <<"2-def">>, <<"3-klm">>], + <<"possible_ancestors">> => [<<"2-revb">>, <<"2-revc">>] + }, + ?DOC2 => #{ + <<"missing">> => [<<"1-pqr">>] + } + }, + Res + ). + +t_empty_missing_revs({Top, Db}) -> + {Code, Res} = req(post, Top ++ Db ++ "/_missing_revs", #{}), + ?assertEqual(200, Code), + ?assertEqual(#{<<"missing_revs">> => #{}}, Res). + +t_missing_revs_no_revs({Top, Db}) -> + Body = #{?DOC1 => [], <<"non_existent_doc">> => []}, + {Code, Res} = req(post, Top ++ Db ++ "/_missing_revs", Body), + ?assertEqual(200, Code), + ?assertEqual(#{<<"missing_revs">> => #{}}, Res). + +t_missing_revs_non_existent_doc({Top, Db}) -> + Body = #{<<"non_existent_doc">> => [<<"1-rev">>]}, + {Code, Res} = req(post, Top ++ Db ++ "/_missing_revs", Body), + ?assertEqual(200, Code), + ?assertEqual( + #{ + <<"missing_revs">> => #{ + <<"non_existent_doc">> => [<<"1-rev">>] + } + }, + Res + ). + +t_missing_revs_all_revs({Top, Db}) -> + Body = #{ + ?DOC1 => [<<"2-", ?REVB/binary>>, <<"2-", ?REVC/binary>>], + ?DOC2 => [<<"1-", ?REVD/binary>>] + }, + {Code, Res} = req(post, Top ++ Db ++ "/_missing_revs", Body), + ?assertEqual(200, Code), + ?assertEqual(#{<<"missing_revs">> => #{}}, Res). + +t_missing_revs_some_missing_some_not({Top, Db}) -> + Body = #{ + ?DOC1 => [<<"2-", ?REVB/binary>>, <<"1-xyz">>, <<"2-def">>, <<"3-klm">>], + ?DOC2 => [<<"1-pqr">>] + }, + {Code, Res} = req(post, Top ++ Db ++ "/_missing_revs", Body), + ?assertEqual(200, Code), + ?assertEqual( + #{ + <<"missing_revs">> => #{ + ?DOC1 => [<<"1-xyz">>, <<"2-def">>, <<"3-klm">>], + ?DOC2 => [<<"1-pqr">>] + } + }, + Res + ). + +create_db(Top, Db) -> + case req(put, Top ++ Db) of + {201, #{}} -> + ok; + Error -> + error({failed_to_create_test_db, Db, Error}) + end. + +delete_db(Top, Db) -> + case req(delete, Top ++ Db) of + {200, #{}} -> + ok; + Error -> + error({failed_to_delete_test_db, Db, Error}) + end. + +create_docs(Top, Db, DocRevs) -> + Docs = lists:map( + fun({Id, Revs}) -> + #{ + <<"_id">> => Id, + <<"_revisions">> => #{ + <<"ids">> => Revs, + <<"start">> => length(Revs) + } + } + end, + DocRevs + ), + Body = #{ + <<"docs">> => Docs, + <<"new_edits">> => false + }, + {Code, Res} = req(post, Top ++ Db ++ "/_bulk_docs", Body), + ?assertEqual(201, Code), + ?assertEqual([], Res), + ok. + +req(Method, Url) -> + Headers = [?JSON, ?AUTH], + {ok, Code, _, Res} = test_request:request(Method, Url, Headers), + {Code, jiffy:decode(Res, [return_maps])}. + +req(Method, Url, #{} = Body) -> + req(Method, Url, jiffy:encode(Body)); +req(Method, Url, Body) -> + Headers = [?JSON, ?AUTH], + {ok, Code, _, Res} = test_request:request(Method, Url, Headers, Body), + {Code, jiffy:decode(Res, [return_maps])}. diff --git a/src/couch/src/couch_db.erl b/src/couch/src/couch_db.erl index 473b7853e..572746229 100644 --- a/src/couch/src/couch_db.erl +++ b/src/couch/src/couch_db.erl @@ -354,52 +354,21 @@ open_doc_revs(Db, Id, Revs, Options) -> % Each returned result is a list of tuples: % {Id, MissingRevs, PossibleAncestors} -% if no revs are missing, it's omitted from the results. +% if no revs are missing, MissingRevs is [] get_missing_revs(Db, IdRevsList) -> - Results = get_full_doc_infos(Db, [Id1 || {Id1, _Revs} <- IdRevsList]), - {ok, find_missing(IdRevsList, Results)}. - -find_missing([], []) -> - []; -find_missing([{Id, Revs} | RestIdRevs], [FullInfo | RestLookupInfo]) when - is_record(FullInfo, full_doc_info) --> - case couch_key_tree:find_missing(FullInfo#full_doc_info.rev_tree, Revs) of - [] -> - find_missing(RestIdRevs, RestLookupInfo); - MissingRevs -> - #doc_info{revs = RevsInfo} = couch_doc:to_doc_info(FullInfo), - LeafRevs = [Rev || #rev_info{rev = Rev} <- RevsInfo], - % Find the revs that are possible parents of this rev - PossibleAncestors = - lists:foldl( - fun({LeafPos, LeafRevId}, Acc) -> - % this leaf is a "possible ancenstor" of the missing - % revs if this LeafPos lessthan any of the missing revs - case - lists:any( - fun({MissingPos, _}) -> - LeafPos < MissingPos - end, - MissingRevs - ) - of - true -> - [{LeafPos, LeafRevId} | Acc]; - false -> - Acc - end - end, - [], - LeafRevs - ), - [ - {Id, MissingRevs, PossibleAncestors} - | find_missing(RestIdRevs, RestLookupInfo) - ] - end; -find_missing([{Id, Revs} | RestIdRevs], [not_found | RestLookupInfo]) -> - [{Id, Revs, []} | find_missing(RestIdRevs, RestLookupInfo)]. + FDIs = get_full_doc_infos(Db, [Id || {Id, _Revs} <- IdRevsList]), + Results = lists:zipwith( + fun + ({Id, Revs}, #full_doc_info{rev_tree = RevTree} = FDI) -> + MissingRevs = couch_key_tree:find_missing(RevTree, Revs), + {Id, MissingRevs, possible_ancestors(FDI, MissingRevs)}; + ({Id, Revs}, not_found) -> + {Id, Revs, []} + end, + IdRevsList, + FDIs + ), + {ok, Results}. get_doc_info(Db, Id) -> case get_full_doc_info(Db, Id) of @@ -2128,6 +2097,34 @@ set_design_doc_end_key(Options, rev) -> lists:keystore(end_key_gt, 1, Options, {end_key_gt, Key2}) end. +possible_ancestors(_FullInfo, []) -> + []; +possible_ancestors(FullInfo, MissingRevs) -> + #doc_info{revs = RevsInfo} = couch_doc:to_doc_info(FullInfo), + LeafRevs = [Rev || #rev_info{rev = Rev} <- RevsInfo], + % Find the revs that are possible parents of this rev + lists:foldl( + fun({LeafPos, LeafRevId}, Acc) -> + % this leaf is a "possible ancenstor" of the missing + % revs if this LeafPos lessthan any of the missing revs + case + lists:any( + fun({MissingPos, _}) -> + LeafPos < MissingPos + end, + MissingRevs + ) + of + true -> + [{LeafPos, LeafRevId} | Acc]; + false -> + Acc + end + end, + [], + LeafRevs + ). + -ifdef(TEST). -include_lib("eunit/include/eunit.hrl"). diff --git a/src/couch/src/couch_httpd_db.erl b/src/couch/src/couch_httpd_db.erl index e82eea7f7..e33a52b36 100644 --- a/src/couch/src/couch_httpd_db.erl +++ b/src/couch/src/couch_httpd_db.erl @@ -451,11 +451,13 @@ db_req(#httpd{method = 'POST', path_parts = [_, <<"_missing_revs">>]} = Req, Db) || {Id, RevStrs} <- JsonDocIdRevs ], {ok, Results} = couch_db:get_missing_revs(Db, JsonDocIdRevs2), - Results2 = [{Id, couch_doc:revs_to_strs(Revs)} || {Id, Revs, _} <- Results], + % Skip docs with no missing revs, they have Revs = [] + Results2 = [{Id, Revs} || {Id, Revs, _} <- Results, Revs =/= []], + Results3 = [{Id, couch_doc:revs_to_strs(Revs)} || {Id, Revs} <- Results2], send_json( Req, {[ - {missing_revs, {Results2}} + {missing_revs, {Results3}} ]} ); db_req(#httpd{path_parts = [_, <<"_missing_revs">>]} = Req, _Db) -> @@ -466,7 +468,9 @@ db_req(#httpd{method = 'POST', path_parts = [_, <<"_revs_diff">>]} = Req, Db) -> JsonDocIdRevs2 = [{Id, couch_doc:parse_revs(RevStrs)} || {Id, RevStrs} <- JsonDocIdRevs], {ok, Results} = couch_db:get_missing_revs(Db, JsonDocIdRevs2), - Results2 = + % Skip docs with no missing revs, they have Revs = [] + Results2 = [Res || {_Id, Revs, _PAs} = Res <- Results, Revs =/= []], + Results3 = lists:map( fun({Id, MissingRevs, PossibleAncestors}) -> {Id, { @@ -479,9 +483,9 @@ db_req(#httpd{method = 'POST', path_parts = [_, <<"_revs_diff">>]} = Req, Db) -> end }} end, - Results + Results2 ), - send_json(Req, {Results2}); + send_json(Req, {Results3}); db_req(#httpd{path_parts = [_, <<"_revs_diff">>]} = Req, _Db) -> send_method_not_allowed(Req, "POST"); db_req(#httpd{method = 'PUT', path_parts = [_, <<"_security">>]} = Req, Db) -> diff --git a/src/fabric/src/fabric_rpc.erl b/src/fabric/src/fabric_rpc.erl index cfda1a37e..8780737ef 100644 --- a/src/fabric/src/fabric_rpc.erl +++ b/src/fabric/src/fabric_rpc.erl @@ -283,30 +283,7 @@ get_missing_revs(DbName, IdRevsList) -> get_missing_revs(DbName, IdRevsList, []). get_missing_revs(DbName, IdRevsList, Options) -> - % reimplement here so we get [] for Ids with no missing revs in response - set_io_priority(DbName, Options), - rexi:reply( - case get_or_create_db(DbName, Options) of - {ok, Db} -> - Ids = [Id1 || {Id1, _Revs} <- IdRevsList], - {ok, - lists:zipwith( - fun({Id, Revs}, FullDocInfoResult) -> - case FullDocInfoResult of - #full_doc_info{rev_tree = RevisionTree} = FullInfo -> - MissingRevs = couch_key_tree:find_missing(RevisionTree, Revs), - {Id, MissingRevs, possible_ancestors(FullInfo, MissingRevs)}; - not_found -> - {Id, Revs, []} - end - end, - IdRevsList, - couch_db:get_full_doc_infos(Db, Ids) - )}; - Error -> - Error - end - ). + with_db(DbName, Options, {couch_db, get_missing_revs, [IdRevsList]}). update_docs(DbName, Docs0, Options) -> {Docs1, Type} = @@ -601,34 +578,6 @@ maybe_filtered_json_doc(Doc, Opts, {selector, _Style, {_Selector, Fields}}) when maybe_filtered_json_doc(Doc, Opts, _Filter) -> couch_doc:to_json_obj(Doc, Opts). -possible_ancestors(_FullInfo, []) -> - []; -possible_ancestors(FullInfo, MissingRevs) -> - #doc_info{revs = RevsInfo} = couch_doc:to_doc_info(FullInfo), - LeafRevs = [Rev || #rev_info{rev = Rev} <- RevsInfo], - % Find the revs that are possible parents of this rev - lists:foldl( - fun({LeafPos, LeafRevId}, Acc) -> - % this leaf is a "possible ancenstor" of the missing - % revs if this LeafPos lessthan any of the missing revs - case - lists:any( - fun({MissingPos, _}) -> - LeafPos < MissingPos - end, - MissingRevs - ) - of - true -> - [{LeafPos, LeafRevId} | Acc]; - false -> - Acc - end - end, - [], - LeafRevs - ). - make_att_readers([]) -> []; make_att_readers([#doc{atts = Atts0} = Doc | Rest]) -> |