Update before_doc_update/2 to before_doc_update/3 by jiangphcn · Pull Request #1808 · apache/couchdb

jiangphcn · 2018-12-13T07:06:08Z

Overview

Pass update type to before_doc_update/3 to allow additional check. The possible update type includes

interactive_edit
replicated_changes

Testing recommendations

make check skip_deps+=couch_epi apps=couch tests=callback_test_

Related Issues or Pull Requests

Checklist

Code is written and works correctly;
Changes are covered by tests;
Documentation reflects the changes;

iilyak · 2018-12-18T11:21:49Z

src/fabric/src/fabric_doc_update.erl

            %% fake db is expensive to create so we only do it if we have to
            Db = fabric_util:fake_db(DbName, Opts),
-            [couch_replicator_manager:before_doc_update(Doc, Db) || Doc <- Docs];
+            [couch_replicator_manager:before_doc_update(Doc, Db, []) || Doc <- Docs];


According to update description of the PR the update type can be either:

replicated_changes

interactive_updates

iilyak · 2018-12-18T11:25:53Z

src/couch/src/couch_db_plugin.erl

-        [Doc1, _Db] when is_function(Fun) -> Fun(Doc1, Db);
-        [Doc1, _Db] -> Doc1
+    case with_pipe(before_doc_update, [Doc0, Db, UpdateType]) of
+        [Doc1, _Db, UpdateType] when is_function(Fun) ->


We are matching the same variable UpdateType which means plugin wouldn't be able to change update type. Is there any reason to allow it? I think you need to change this line to [Doc1, _Db] when is_function(Fun) ->.

this is still an issue

thanks @iilyak for double check. For this case, I tried to use [Doc1, _Db] when is_function(Fun) -> here and [Doc, Db]. in https://github.com/cloudant/showroom/pull/212/commits/7706fba5776466ba23963213ace114d85068e777#diff-984e3fb55ca9cd5d963758ec7cdc91e0L43. But this causes that db instance can't start.

So I try to pass all variables here [Doc, Db, _UpdateType]. and use [Doc, Db, _UpdateType]. in https://github.com/cloudant/showroom/pull/212/commits/7706fba5776466ba23963213ace114d85068e777#diff-984e3fb55ca9cd5d963758ec7cdc91e0R40. My test works fine now https://github.com/cloudant/showroom/pull/212#issuecomment-449840808

I noticed that _Db is still passed to Fun even if it is not changed. Thinking that we need the same case for _UpdateType.

I am sorry I think I confused you by my wrong example. I meant it should be [Doc1, _Db, _UpdateType]. The UpdateTypename cannot be used.

No problem, Ilya. I think that I got your point now. Totally make sense and I should use different name after calling with_pipe/2. The change was reflected in recent commit.

iilyak · 2019-01-03T20:19:25Z

src/couch/src/couch_db_plugin.erl

-        [Doc1, _Db] -> Doc1
+    case with_pipe(before_doc_update, [Doc0, Db, UpdateType]) of
+        [Doc1, _Db, _UpdateType] when is_function(Fun) ->
+            Fun(Doc1, Db, _UpdateType);


Ahh. I didn't notice earlier that you are passing _UpdateType to Fun. In this case it is better to rename it into UpdateType1 or something. According to erlang's conventions (roughly).

Use new name for variable when the value need to be changed (usually the integer is added as suffix)

Use _Name style for variables you don't care about.

In this case we do care about the variable since we are passing it to another function. Also we cannot use UpdateType since the value returned from with_pipe can differ from the passed value. In this case we would have a badmatch.

iilyak · 2019-01-03T20:43:11Z

src/couch_replicator/src/couch_replicator_docs.erl


-spec before_doc_update(#doc{}, Db::any()) -> #doc{}.
-before_doc_update(#doc{id = <<?DESIGN_DOC_PREFIX, _/binary>>} = Doc, _Db) ->
+-spec before_doc_update(#doc{}, Db::any(), any()) -> #doc{}.


we shouldn't use any() here. By using any we disable dializer. Try defining update_type() in couch_db and refer to it using couch_db:update_type().

diff --git a/src/couch/src/couch_db.erl b/src/couch/src/couch_db.erl index 1f8bc42..43ed562 100644 --- a/src/couch/src/couch_db.erl +++ b/src/couch/src/couch_db.erl @@ -135,6 +135,10 @@ -include_lib("couch/include/couch_db.hrl"). -include("couch_db_int.hrl"). +-type update_type() + :: replicated_changes + | interactive_edit. +

iilyak · 2019-01-03T20:46:24Z

src/fabric/src/fabric_doc_update.erl

            %% fake db is expensive to create so we only do it if we have to
            Db = fabric_util:fake_db(DbName, Opts),
-            [couch_replicator_docs:before_doc_update(Doc, Db) || Doc <- Docs];
+            [couch_replicator_docs:before_doc_update(Doc, Db, interactive_updates)


Hmm. I couldn't find where interactive_updates is handled. I think it is typo and should be interactive_edit.

Good catch. Because this is for placeholder and I directly use #1808 (comment). I already change it to replicated_changes.

iilyak · 2019-01-03T20:47:20Z

src/fabric/src/fabric_doc_update.erl

            %% fake db is expensive to create so we only do it if we have to
            Db = fabric_util:fake_db(DbName, Opts),
-            [couch_users_db:before_doc_update(Doc, Db) || Doc <- Docs];
+            [couch_users_db:before_doc_update(Doc, Db, interactive_updates)


I think it should be interactive_edit unless interactive_updates is a new thing defined in some other dependent PR I am not aware of.

yes, change it to interactive_edit

jiangphcn · 2019-01-04T03:20:49Z

Thanks @iilyak for careful review. I addressed your comments. I will rebase and merge them if getting approval later.

iilyak

+1 after squashing of all commits into one.

- Pass UpdateType to before_doc_update/3

jiangphcn force-pushed the before_doc_update branch from 0adaafa to e7d1de7 Compare December 13, 2018 07:40

iilyak reviewed Dec 18, 2018

View reviewed changes

jiangphcn force-pushed the before_doc_update branch from 30327c9 to 2e78a26 Compare December 25, 2018 01:11

iilyak reviewed Jan 3, 2019

View reviewed changes

iilyak approved these changes Jan 4, 2019

View reviewed changes

Update before_doc_update/2 to before_doc_update/3

c68863a

- Pass UpdateType to before_doc_update/3

jiangphcn force-pushed the before_doc_update branch from 82ae880 to c68863a Compare January 5, 2019 01:53

jiangphcn merged commit 72b788e into master Jan 5, 2019

jiangphcn deleted the before_doc_update branch January 5, 2019 07:40

wohali mentioned this pull request Feb 7, 2019

2.3.1 Release Proposal #1908

Merged

Conversation

jiangphcn commented Dec 13, 2018

Overview

Testing recommendations

Related Issues or Pull Requests

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jiangphcn commented Jan 4, 2019

Uh oh!

iilyak left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants