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.

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])}.

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").
 

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) ->