couchdb-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From dav...@apache.org
Subject [couchdb] 01/03: Fix bulk docs error reporting
Date Fri, 12 Jul 2019 14:40:23 GMT
This is an automated email from the ASF dual-hosted git repository.

davisp pushed a commit to branch prototype/fdb-layer
in repository https://gitbox.apache.org/repos/asf/couchdb.git

commit 8d167afaf3592270282e801ea4e0431c78c7dc3e
Author: Paul J. Davis <paul.joseph.davis@gmail.com>
AuthorDate: Thu Jul 11 14:25:24 2019 -0500

    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.
---
 src/chttpd/src/chttpd_db.erl               |   2 +
 src/fabric/src/fabric2_db.erl              | 108 +++++++++++++++++------------
 src/fabric/test/fabric2_doc_crud_tests.erl |  20 ++----
 3 files changed, 73 insertions(+), 57 deletions(-)

diff --git a/src/chttpd/src/chttpd_db.erl b/src/chttpd/src/chttpd_db.erl
index 90869c6..abdd825 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 eb74a18..3ea30e7 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 85b2766..c19c474 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, _}) ->


Mime
View raw message