You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@couchdb.apache.org by GitBox <gi...@apache.org> on 2020/10/12 19:11:29 UTC
[GitHub] [couchdb] davisp opened a new pull request #3214: Disable custom reduce functions by default
davisp opened a new pull request #3214:
URL: https://github.com/apache/couchdb/pull/3214
## Overview
Disable custom reduce functions by default.
Custom reduce functions are notoriously difficult to implement correctly and often lead to fairly bad user experience when they're inevitably misunderstood or used incorrectly. The feature will remain available via config toggle for at least 4.0.
## Testing recommendations
`make check`
## Checklist
- [x] Code is written and works correctly
- [x] Changes are covered by tests
- [x] Any new configurable parameters are documented in `rel/overlay/etc/default.ini`
- [ ] A PR for documentation changes has been made in https://github.com/apache/couchdb-documentation
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on a change in pull request #3214: Disable custom reduce functions by default
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3214:
URL: https://github.com/apache/couchdb/pull/3214#discussion_r503524676
##########
File path: src/couch_views/test/couch_views_custom_red_test.erl
##########
@@ -0,0 +1,194 @@
+% 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(couch_views_custom_red_test).
+
+-include_lib("couch/include/couch_eunit.hrl").
+-include_lib("couch/include/couch_db.hrl").
+-include_lib("couch_mrview/include/couch_mrview.hrl").
+-include_lib("fabric/test/fabric2_test.hrl").
+-include("couch_views.hrl").
+
+
+-define(NUM_DOCS, 100).
+
+
+custom_reduce_disabled_test_() ->
+ {
+ "Custom Reduce Disabled",
+ {
+ setup,
+ fun setup_disabled/0,
+ fun teardown/1,
+ with([
+ ?TDEF(builtin_reductions_work),
+ ?TDEF(custom_reduces_disabled)
+ ])
+ }
+ }.
+
+
+custom_reduce_enabled_test_() ->
+ {
+ "Custom Reduce Disabled",
+ {
+ setup,
+ fun setup_enabled/0,
+ fun teardown/1,
+ with([
+ ?TDEF(builtin_reductions_work),
+ ?TDEF(custom_reduces_enabled)
+ ])
+ }
+ }.
+
+
+sigs_change_test_() ->
+ {
+ "Sigs Change Test",
+ {
+ setup,
+ fun setup_sigs_change/0,
+ fun teardown_sigs_change/1,
+ with([
+ ?TDEF(sigs_change)
+ ])
+ }
+ }.
+
+setup_disabled() ->
+ setup_common(false).
+
+
+setup_enabled() ->
+ setup_common(true).
+
+
+setup_common(Enabled) ->
+ Ctx = test_util:start_couch([
+ fabric,
+ couch_jobs,
+ couch_js,
+ couch_views
+ ]),
+ config:set_boolean("couch_views", "custom_reduce_enabled", Enabled),
Review comment:
This might leave the config written to the last value it was set after the last teardown and might affect other tests. Could set `Persist=false` here, then in teardown do `config:delete("couch_views", "custom_reduce_enabled", _Persist=false).`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on a change in pull request #3214: Disable custom reduce functions by default
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3214:
URL: https://github.com/apache/couchdb/pull/3214#discussion_r503523185
##########
File path: src/couch_views/test/couch_views_custom_red_test.erl
##########
@@ -0,0 +1,194 @@
+% 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(couch_views_custom_red_test).
+
+-include_lib("couch/include/couch_eunit.hrl").
+-include_lib("couch/include/couch_db.hrl").
+-include_lib("couch_mrview/include/couch_mrview.hrl").
+-include_lib("fabric/test/fabric2_test.hrl").
+-include("couch_views.hrl").
+
+
+-define(NUM_DOCS, 100).
+
+
+custom_reduce_disabled_test_() ->
+ {
+ "Custom Reduce Disabled",
+ {
+ setup,
+ fun setup_disabled/0,
+ fun teardown/1,
+ with([
+ ?TDEF(builtin_reductions_work),
+ ?TDEF(custom_reduces_disabled)
+ ])
+ }
+ }.
+
+
+custom_reduce_enabled_test_() ->
+ {
+ "Custom Reduce Disabled",
+ {
+ setup,
+ fun setup_enabled/0,
+ fun teardown/1,
+ with([
+ ?TDEF(builtin_reductions_work),
+ ?TDEF(custom_reduces_enabled)
+ ])
+ }
+ }.
+
+
+sigs_change_test_() ->
+ {
+ "Sigs Change Test",
+ {
+ setup,
+ fun setup_sigs_change/0,
+ fun teardown_sigs_change/1,
+ with([
+ ?TDEF(sigs_change)
+ ])
+ }
+ }.
+
+setup_disabled() ->
+ setup_common(false).
+
+
+setup_enabled() ->
+ setup_common(true).
+
+
+setup_common(Enabled) ->
+ Ctx = test_util:start_couch([
+ fabric,
+ couch_jobs,
+ couch_js,
+ couch_views
+ ]),
Review comment:
Maybe move `]),` to match the Ctx column
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] davisp commented on a change in pull request #3214: Disable custom reduce functions by default
Posted by GitBox <gi...@apache.org>.
davisp commented on a change in pull request #3214:
URL: https://github.com/apache/couchdb/pull/3214#discussion_r503529686
##########
File path: src/couch_views/test/couch_views_custom_red_test.erl
##########
@@ -0,0 +1,194 @@
+% 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(couch_views_custom_red_test).
+
+-include_lib("couch/include/couch_eunit.hrl").
+-include_lib("couch/include/couch_db.hrl").
+-include_lib("couch_mrview/include/couch_mrview.hrl").
+-include_lib("fabric/test/fabric2_test.hrl").
+-include("couch_views.hrl").
+
+
+-define(NUM_DOCS, 100).
+
+
+custom_reduce_disabled_test_() ->
+ {
+ "Custom Reduce Disabled",
+ {
+ setup,
+ fun setup_disabled/0,
+ fun teardown/1,
+ with([
+ ?TDEF(builtin_reductions_work),
+ ?TDEF(custom_reduces_disabled)
+ ])
+ }
+ }.
+
+
+custom_reduce_enabled_test_() ->
+ {
+ "Custom Reduce Disabled",
+ {
+ setup,
+ fun setup_enabled/0,
+ fun teardown/1,
+ with([
+ ?TDEF(builtin_reductions_work),
+ ?TDEF(custom_reduces_enabled)
+ ])
+ }
+ }.
+
+
+sigs_change_test_() ->
+ {
+ "Sigs Change Test",
+ {
+ setup,
+ fun setup_sigs_change/0,
+ fun teardown_sigs_change/1,
+ with([
+ ?TDEF(sigs_change)
+ ])
+ }
+ }.
+
+setup_disabled() ->
+ setup_common(false).
+
+
+setup_enabled() ->
+ setup_common(true).
+
+
+setup_common(Enabled) ->
+ Ctx = test_util:start_couch([
+ fabric,
+ couch_jobs,
+ couch_js,
+ couch_views
+ ]),
+ config:set_boolean("couch_views", "custom_reduce_enabled", Enabled),
Review comment:
Fixed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] davisp commented on a change in pull request #3214: Disable custom reduce functions by default
Posted by GitBox <gi...@apache.org>.
davisp commented on a change in pull request #3214:
URL: https://github.com/apache/couchdb/pull/3214#discussion_r503529100
##########
File path: src/couch_views/test/couch_views_custom_red_test.erl
##########
@@ -0,0 +1,194 @@
+% 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(couch_views_custom_red_test).
+
+-include_lib("couch/include/couch_eunit.hrl").
+-include_lib("couch/include/couch_db.hrl").
+-include_lib("couch_mrview/include/couch_mrview.hrl").
+-include_lib("fabric/test/fabric2_test.hrl").
+-include("couch_views.hrl").
+
+
+-define(NUM_DOCS, 100).
+
+
+custom_reduce_disabled_test_() ->
+ {
+ "Custom Reduce Disabled",
+ {
+ setup,
+ fun setup_disabled/0,
+ fun teardown/1,
+ with([
+ ?TDEF(builtin_reductions_work),
+ ?TDEF(custom_reduces_disabled)
+ ])
+ }
+ }.
+
+
+custom_reduce_enabled_test_() ->
+ {
+ "Custom Reduce Disabled",
+ {
+ setup,
+ fun setup_enabled/0,
+ fun teardown/1,
+ with([
+ ?TDEF(builtin_reductions_work),
+ ?TDEF(custom_reduces_enabled)
+ ])
+ }
+ }.
+
+
+sigs_change_test_() ->
+ {
+ "Sigs Change Test",
+ {
+ setup,
+ fun setup_sigs_change/0,
+ fun teardown_sigs_change/1,
+ with([
+ ?TDEF(sigs_change)
+ ])
+ }
+ }.
+
+setup_disabled() ->
+ setup_common(false).
+
+
+setup_enabled() ->
+ setup_common(true).
+
+
+setup_common(Enabled) ->
+ Ctx = test_util:start_couch([
+ fabric,
+ couch_jobs,
+ couch_js,
+ couch_views
+ ]),
Review comment:
That's copy pasta'ed from the other test modules. I don't think changing style here makes sense.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] davisp merged pull request #3214: Disable custom reduce functions by default
Posted by GitBox <gi...@apache.org>.
davisp merged pull request #3214:
URL: https://github.com/apache/couchdb/pull/3214
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] davisp commented on a change in pull request #3214: Disable custom reduce functions by default
Posted by GitBox <gi...@apache.org>.
davisp commented on a change in pull request #3214:
URL: https://github.com/apache/couchdb/pull/3214#discussion_r504092776
##########
File path: src/couch_views/src/couch_views_util.erl
##########
@@ -327,6 +328,33 @@ active_tasks_info(ChangesDone, DbName, DDocId, LastSeq, DBSeq) ->
}.
+maybe_disable_custom_reduce_funs(Views) ->
+ case config:get_boolean("couch_views", "custom_reduce_enabled", false) of
+ true ->
+ Views;
+ false ->
+ disable_custom_reduce_funs(Views)
+ end.
+
+
+disable_custom_reduce_funs(Views) ->
+ lists:map(fun(View) ->
+ #mrview{
+ reduce_funs = ReduceFuns
+ } = View,
+ {Builtin, Custom} = lists:partition(fun({_Name, RedSrc}) ->
+ case RedSrc of
+ <<"_", _/binary>> -> true;
Review comment:
That is to say, I don't think this PR is the place to change the definition of a builtin reduce. Even if a regexp or even a list membership for known builtins would be more useful from the user experience point of view.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] davisp commented on a change in pull request #3214: Disable custom reduce functions by default
Posted by GitBox <gi...@apache.org>.
davisp commented on a change in pull request #3214:
URL: https://github.com/apache/couchdb/pull/3214#discussion_r504090900
##########
File path: src/couch_views/src/couch_views_util.erl
##########
@@ -327,6 +328,33 @@ active_tasks_info(ChangesDone, DbName, DDocId, LastSeq, DBSeq) ->
}.
+maybe_disable_custom_reduce_funs(Views) ->
+ case config:get_boolean("couch_views", "custom_reduce_enabled", false) of
+ true ->
+ Views;
+ false ->
+ disable_custom_reduce_funs(Views)
+ end.
+
+
+disable_custom_reduce_funs(Views) ->
+ lists:map(fun(View) ->
+ #mrview{
+ reduce_funs = ReduceFuns
+ } = View,
+ {Builtin, Custom} = lists:partition(fun({_Name, RedSrc}) ->
+ case RedSrc of
+ <<"_", _/binary>> -> true;
Review comment:
The definition of a builtin reduce function is that the first character is an underscore:
https://github.com/apache/couchdb/commit/c27a3643180dba63e725ad63f274e77542884470#diff-75f428250d78edfec3309859ab3cc91d5bd8deedc74437263b095d820b62530fR106-R109
Same detection has been there for nearly 12 years since they were first introduced.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] rnewson commented on a change in pull request #3214: Disable custom reduce functions by default
Posted by GitBox <gi...@apache.org>.
rnewson commented on a change in pull request #3214:
URL: https://github.com/apache/couchdb/pull/3214#discussion_r504084308
##########
File path: src/couch_views/src/couch_views_util.erl
##########
@@ -327,6 +328,33 @@ active_tasks_info(ChangesDone, DbName, DDocId, LastSeq, DBSeq) ->
}.
+maybe_disable_custom_reduce_funs(Views) ->
+ case config:get_boolean("couch_views", "custom_reduce_enabled", false) of
+ true ->
+ Views;
+ false ->
+ disable_custom_reduce_funs(Views)
+ end.
+
+
+disable_custom_reduce_funs(Views) ->
+ lists:map(fun(View) ->
+ #mrview{
+ reduce_funs = ReduceFuns
+ } = View,
+ {Builtin, Custom} = lists:partition(fun({_Name, RedSrc}) ->
+ case RedSrc of
+ <<"_", _/binary>> -> true;
Review comment:
This isn't very strong, it would allow `_foo = 12; fun(keys, values, rereduce) { ... }` through and I think we'd run that? I think we need to match the whole thing with a regex like `^_[a-z]+]$`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] rnewson commented on pull request #3214: Disable custom reduce functions by default
Posted by GitBox <gi...@apache.org>.
rnewson commented on pull request #3214:
URL: https://github.com/apache/couchdb/pull/3214#issuecomment-707852703
fired off a dev@ thread
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] rnewson commented on a change in pull request #3214: Disable custom reduce functions by default
Posted by GitBox <gi...@apache.org>.
rnewson commented on a change in pull request #3214:
URL: https://github.com/apache/couchdb/pull/3214#discussion_r504084308
##########
File path: src/couch_views/src/couch_views_util.erl
##########
@@ -327,6 +328,33 @@ active_tasks_info(ChangesDone, DbName, DDocId, LastSeq, DBSeq) ->
}.
+maybe_disable_custom_reduce_funs(Views) ->
+ case config:get_boolean("couch_views", "custom_reduce_enabled", false) of
+ true ->
+ Views;
+ false ->
+ disable_custom_reduce_funs(Views)
+ end.
+
+
+disable_custom_reduce_funs(Views) ->
+ lists:map(fun(View) ->
+ #mrview{
+ reduce_funs = ReduceFuns
+ } = View,
+ {Builtin, Custom} = lists:partition(fun({_Name, RedSrc}) ->
+ case RedSrc of
+ <<"_", _/binary>> -> true;
Review comment:
This isn't very strong, it would allow `_foo = 12; fun(keys, values, rereduce) {}` through and I think we'd run that? I think we need to match the whole thing with a regex like `^_[a-z]+]$`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] rnewson commented on a change in pull request #3214: Disable custom reduce functions by default
Posted by GitBox <gi...@apache.org>.
rnewson commented on a change in pull request #3214:
URL: https://github.com/apache/couchdb/pull/3214#discussion_r504121273
##########
File path: src/couch_views/src/couch_views_util.erl
##########
@@ -327,6 +328,33 @@ active_tasks_info(ChangesDone, DbName, DDocId, LastSeq, DBSeq) ->
}.
+maybe_disable_custom_reduce_funs(Views) ->
+ case config:get_boolean("couch_views", "custom_reduce_enabled", false) of
+ true ->
+ Views;
+ false ->
+ disable_custom_reduce_funs(Views)
+ end.
+
+
+disable_custom_reduce_funs(Views) ->
+ lists:map(fun(View) ->
+ #mrview{
+ reduce_funs = ReduceFuns
+ } = View,
+ {Builtin, Custom} = lists:partition(fun({_Name, RedSrc}) ->
+ case RedSrc of
+ <<"_", _/binary>> -> true;
Review comment:
That doesn't change anything so I'm ignoring it. However;
https://github.com/apache/couchdb/blob/master/src/couch_mrview/src/couch_mrview.erl#L194
This does prevent the saving of any reduce function starting with `_` that is not specifically one of the actual built-in reduces, so we get the validation I'm after by a different route. I think it is sufficient.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] wohali commented on pull request #3214: Disable custom reduce functions by default
Posted by GitBox <gi...@apache.org>.
wohali commented on pull request #3214:
URL: https://github.com/apache/couchdb/pull/3214#issuecomment-707357614
@davisp @nickva As a deprecation (I think?), this needs to be brought up on the dev@couchdb.a.o ML before merging.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] davisp commented on a change in pull request #3214: Disable custom reduce functions by default
Posted by GitBox <gi...@apache.org>.
davisp commented on a change in pull request #3214:
URL: https://github.com/apache/couchdb/pull/3214#discussion_r503528610
##########
File path: src/couch_views/test/couch_views_custom_red_test.erl
##########
@@ -0,0 +1,194 @@
+% 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(couch_views_custom_red_test).
+
+-include_lib("couch/include/couch_eunit.hrl").
+-include_lib("couch/include/couch_db.hrl").
+-include_lib("couch_mrview/include/couch_mrview.hrl").
+-include_lib("fabric/test/fabric2_test.hrl").
+-include("couch_views.hrl").
+
+
+-define(NUM_DOCS, 100).
+
+
+custom_reduce_disabled_test_() ->
+ {
+ "Custom Reduce Disabled",
+ {
+ setup,
+ fun setup_disabled/0,
+ fun teardown/1,
+ with([
+ ?TDEF(builtin_reductions_work),
+ ?TDEF(custom_reduces_disabled)
+ ])
+ }
+ }.
+
+
+custom_reduce_enabled_test_() ->
+ {
+ "Custom Reduce Disabled",
+ {
+ setup,
+ fun setup_enabled/0,
+ fun teardown/1,
+ with([
+ ?TDEF(builtin_reductions_work),
+ ?TDEF(custom_reduces_enabled)
+ ])
+ }
+ }.
+
+
+sigs_change_test_() ->
+ {
+ "Sigs Change Test",
+ {
+ setup,
+ fun setup_sigs_change/0,
+ fun teardown_sigs_change/1,
+ with([
+ ?TDEF(sigs_change)
+ ])
+ }
+ }.
+
+setup_disabled() ->
+ setup_common(false).
+
+
+setup_enabled() ->
+ setup_common(true).
+
+
+setup_common(Enabled) ->
+ Ctx = test_util:start_couch([
+ fabric,
+ couch_jobs,
+ couch_js,
+ couch_views
+ ]),
+ config:set_boolean("couch_views", "custom_reduce_enabled", Enabled),
Review comment:
Yarps. Derps. Thought to add it and forgot.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org