Fix bulk docs error reporting

The existing logic around return codes and term formats is labyrinthine.
This is the result of much trial and error to get the new logic to
behave exactly the same as the previous implementation.

3 files changed, 73 insertions, 57 deletions

diff --git a/src/chttpd/src/chttpd_db.erl b/src/chttpd/src/chttpd_db.erl
index 90869c6dd..abdd825f9 100644
--- a/src/chttpd/src/chttpd_db.erl
+++ b/src/chttpd/src/chttpd_db.erl
@@ -1337,6 +1337,8 @@ update_doc_result_to_json(DocId, {ok, NewRev}) ->
     {[{ok, true}, {id, DocId}, {rev, couch_doc:rev_to_str(NewRev)}]};
 update_doc_result_to_json(DocId, {accepted, NewRev}) ->
     {[{ok, true}, {id, DocId}, {rev, couch_doc:rev_to_str(NewRev)}, {accepted, true}]};
+update_doc_result_to_json(DocId, {{DocId, _}, Error}) ->
+    update_doc_result_to_json(DocId, Error);
 update_doc_result_to_json(DocId, Error) ->
     {_Code, ErrorStr, Reason} = chttpd:error_info(Error),
     {[{id, DocId}, {error, ErrorStr}, {reason, Reason}]}.
diff --git a/src/fabric/src/fabric2_db.erl b/src/fabric/src/fabric2_db.erl
index eb74a183c..3ea30e70f 100644
--- a/src/fabric/src/fabric2_db.erl
+++ b/src/fabric/src/fabric2_db.erl
@@ -584,46 +584,52 @@ update_docs(Db, Docs) ->
 
 update_docs(Db, Docs0, Options) ->
     Docs1 = apply_before_doc_update(Db, Docs0, Options),
-    Resps0 = case lists:member(replicated_changes, Options) of
-        false ->
-            fabric2_fdb:transactional(Db, fun(TxDb) ->
-                update_docs_interactive(TxDb, Docs1, Options)
-            end);
-        true ->
-            lists:map(fun(Doc) ->
+    try
+        validate_atomic_update(Docs0, lists:member(all_or_nothing, Options)),
+        Resps0 = case lists:member(replicated_changes, Options) of
+            false ->
                 fabric2_fdb:transactional(Db, fun(TxDb) ->
-                    update_doc_int(TxDb, Doc, Options)
-                end)
-            end, Docs1)
-    end,
-    % Convert errors
-    Resps1 = lists:map(fun(Resp) ->
-        case Resp of
-            {#doc{} = Doc, Error} ->
-                #doc{
-                    id = DocId,
-                    revs = Revs
-                } = Doc,
-                RevId = case Revs of
-                    {RevPos, [Rev | _]} -> {RevPos, Rev};
-                    {0, []} -> {0, <<>>}
-                end,
-                {{DocId, RevId}, Error};
-            Else ->
-                Else
+                    update_docs_interactive(TxDb, Docs1, Options)
+                end);
+            true ->
+                lists:map(fun(Doc) ->
+                    fabric2_fdb:transactional(Db, fun(TxDb) ->
+                        update_doc_int(TxDb, Doc, Options)
+                    end)
+                end, Docs1)
+        end,
+        % Convert errors
+        Resps1 = lists:map(fun(Resp) ->
+            case Resp of
+                {#doc{} = Doc, Error} ->
+                    #doc{
+                        id = DocId,
+                        revs = Revs
+                    } = Doc,
+                    RevId = case Revs of
+                        {RevPos, [Rev | _]} -> {RevPos, Rev};
+                        {0, []} -> {0, <<>>};
+                        Else -> Else
+                    end,
+                    {{DocId, RevId}, Error};
+                Else ->
+                    Else
+            end
+        end, Resps0),
+        case lists:member(replicated_changes, Options) of
+            true ->
+                {ok, lists:flatmap(fun(R) ->
+                    case R of
+                        {ok, []} -> [];
+                        {{_, _}, {ok, []}} -> [];
+                        Else -> [Else]
+                    end
+                end, Resps1)};
+            false ->
+                {ok, Resps1}
         end
-    end, Resps0),
-    case lists:member(replicated_changes, Options) of
-        true ->
-            {ok, [R || R <- Resps1, R /= {ok, []}]};
-        false ->
-            Status = lists:foldl(fun(Resp, Acc) ->
-                case Resp of
-                    {ok, _} -> Acc;
-                    _ -> error
-                end
-            end, ok, Resps1),
-            {Status, Resps1}
+    catch throw:{aborted, Errors} ->
+        {aborted, Errors}
     end.
 
 
@@ -1023,7 +1029,7 @@ update_docs_interactive(Db, #doc{id = <<?LOCAL_DOC_PREFIX, _/binary>>} = Doc,
 update_docs_interactive(Db, Doc, Options, Futures, SeenIds) ->
     case lists:member(Doc#doc.id, SeenIds) of
         true ->
-            {{error, conflict}, SeenIds};
+            {conflict, SeenIds};
         false ->
             Future = maps:get(doc_tag(Doc), Futures),
             case update_doc_interactive(Db, Doc, Future, Options) of
@@ -1066,12 +1072,12 @@ update_doc_interactive(Db, Doc0, Future, _Options) ->
     % Check that a revision was specified if required
     Doc0RevId = doc_to_revid(Doc0),
     if Doc0RevId /= {0, <<>>} orelse WinnerRevId == {0, <<>>} -> ok; true ->
-        ?RETURN({error, conflict})
+        ?RETURN({Doc0, conflict})
     end,
 
     % Check that we're not trying to create a deleted doc
     if Doc0RevId /= {0, <<>>} orelse not Doc0#doc.deleted -> ok; true ->
-        ?RETURN({error, conflict})
+        ?RETURN({Doc0, conflict})
     end,
 
     % Get the target revision to update
@@ -1088,7 +1094,7 @@ update_doc_interactive(Db, Doc0, Future, _Options) ->
                     % that we get not_found for a deleted revision
                     % because we only check for the non-deleted
                     % key in fdb
-                    ?RETURN({error, conflict})
+                    ?RETURN({Doc0, conflict})
             end
     end,
 
@@ -1191,7 +1197,7 @@ update_doc_replicated(Db, Doc0, _Options) ->
     if Status /= internal_node -> ok; true ->
         % We already know this revision so nothing
         % left to do.
-        ?RETURN({ok, []})
+        ?RETURN({Doc0, {ok, []}})
     end,
 
     % Its possible to have a replication with fewer than $revs_limit
@@ -1248,7 +1254,7 @@ update_doc_replicated(Db, Doc0, _Options) ->
 update_local_doc(Db, Doc0, _Options) ->
     Doc1 = case increment_local_doc_rev(Doc0) of
         {ok, Updated} -> Updated;
-        {error, _} = Error -> ?RETURN(Error)
+        {error, Error} -> ?RETURN({Doc0, Error})
     end,
 
     ok = fabric2_fdb:write_local_doc(Db, Doc1),
@@ -1367,6 +1373,20 @@ validate_ddoc(Db, DDoc) ->
     end.
 
 
+validate_atomic_update(_, false) ->
+    ok;
+validate_atomic_update(AllDocs, true) ->
+    % TODO actually perform the validation.  This requires some hackery, we need
+    % to basically extract the prep_and_validate_updates function from couch_db
+    % and only run that, without actually writing in case of a success.
+    Error = {not_implemented, <<"all_or_nothing is not supported">>},
+    PreCommitFailures = lists:map(fun(#doc{id=Id, revs = {Pos,Revs}}) ->
+        case Revs of [] -> RevId = <<>>; [RevId|_] -> ok end,
+        {{Id, {Pos, RevId}}, Error}
+    end, AllDocs),
+    throw({aborted, PreCommitFailures}).
+
+
 check_duplicate_attachments(#doc{atts = Atts}) ->
     lists:foldl(fun(Att, Names) ->
         Name = couch_att:fetch(name, Att),
diff --git a/src/fabric/test/fabric2_doc_crud_tests.erl b/src/fabric/test/fabric2_doc_crud_tests.erl
index 85b276679..c19c47421 100644
--- a/src/fabric/test/fabric2_doc_crud_tests.erl
+++ b/src/fabric/test/fabric2_doc_crud_tests.erl
@@ -408,7 +408,7 @@ conflict_on_create_new_with_rev({Db, _}) ->
         revs = {1, [fabric2_util:uuid()]},
         body = {[{<<"foo">>, <<"bar">>}]}
     },
-    ?assertThrow({error, conflict}, fabric2_db:update_doc(Db, Doc)).
+    ?assertThrow(conflict, fabric2_db:update_doc(Db, Doc)).
 
 
 conflict_on_update_with_no_rev({Db, _}) ->
@@ -421,7 +421,7 @@ conflict_on_update_with_no_rev({Db, _}) ->
         revs = {0, []},
         body = {[{<<"state">>, 2}]}
     },
-    ?assertThrow({error, conflict}, fabric2_db:update_doc(Db, Doc2)).
+    ?assertThrow(conflict, fabric2_db:update_doc(Db, Doc2)).
 
 
 conflict_on_create_as_deleted({Db, _}) ->
@@ -430,7 +430,7 @@ conflict_on_create_as_deleted({Db, _}) ->
         deleted = true,
         body = {[{<<"foo">>, <<"bar">>}]}
     },
-    ?assertThrow({error, conflict}, fabric2_db:update_doc(Db, Doc)).
+    ?assertThrow(conflict, fabric2_db:update_doc(Db, Doc)).
 
 
 conflict_on_recreate_as_deleted({Db, _}) ->
@@ -450,7 +450,7 @@ conflict_on_recreate_as_deleted({Db, _}) ->
         deleted = true,
         body = {[{<<"state">>, 3}]}
     },
-    ?assertThrow({error, conflict}, fabric2_db:update_doc(Db, Doc3)).
+    ?assertThrow(conflict, fabric2_db:update_doc(Db, Doc3)).
 
 
 conflict_on_extend_deleted({Db, _}) ->
@@ -470,7 +470,7 @@ conflict_on_extend_deleted({Db, _}) ->
         deleted = false,
         body = {[{<<"state">>, 3}]}
     },
-    ?assertThrow({error, conflict}, fabric2_db:update_doc(Db, Doc3)).
+    ?assertThrow(conflict, fabric2_db:update_doc(Db, Doc3)).
 
 
 open_doc_revs_basic({Db, _}) ->
@@ -725,18 +725,12 @@ create_local_doc_bad_rev({Db, _}) ->
         id = LDocId,
         revs = {0, [<<"not a number">>]}
     },
-    ?assertThrow(
-            {error, <<"Invalid rev format">>},
-            fabric2_db:update_doc(Db, Doc1)
-        ),
+    ?assertThrow(<<"Invalid rev format">>, fabric2_db:update_doc(Db, Doc1)),
 
     Doc2 = Doc1#doc{
         revs = bad_bad_rev_roy_brown
     },
-    ?assertThrow(
-            {error, <<"Invalid rev format">>},
-            fabric2_db:update_doc(Db, Doc2)
-        ).
+    ?assertThrow(<<"Invalid rev format">>, fabric2_db:update_doc(Db, Doc2)).
 
 
 create_local_doc_random_rev({Db, _}) ->