nickva commented on a change in pull request #3567:
URL: https://github.com/apache/couchdb/pull/3567#discussion_r633644907
##########
File path: rel/overlay/etc/default.ini
##########
@@ -106,8 +106,8 @@ couch = couch_bt_engine
;couch_server = false
[cluster]
-q=2
-n=3
+q = 2
Review comment:
Avoid unrelated changes in the PR. We can make a separate PR for
formatting
##########
File path: rel/overlay/etc/default.ini
##########
@@ -189,16 +211,22 @@ database_prefix = userdb-
[httpd]
port = {{backend_port}}
bind_address = 127.0.0.1
-authentication_handlers = {couch_httpd_auth, cookie_authentication_handler},
{couch_httpd_auth, default_authentication_handler}
-secure_rewrites = true
-allow_jsonp = false
+
; Options for the MochiWeb HTTP server.
;server_options = [{backlog, 128}, {acceptor_pool_size, 16}]
; For more socket options, consult Erlang's module 'inet' man page.
;socket_options = [{recbuf, undefined}, {sndbuf, 262144}, {nodelay, true}]
socket_options = [{sndbuf, 262144}]
-enable_cors = false
-enable_xframe_options = false
+
+
+;;;; Move to [chttpd] ;;;;
Review comment:
I think for the ones which were moved we could simply list them saying
"These settings were moved for chttpd, we don't have to keep their default and
values here.
##########
File path: rel/overlay/etc/default.ini
##########
@@ -147,6 +147,28 @@ max_db_number_for_dbs_info_req = 100
; prevent non-admins from accessing /_all_dbs
; admin_only_all_dbs = true
+;;;; Moved from [httpd] ;;;;;
+authentication_handlers = {couch_httpd_auth, cookie_authentication_handler},
{couch_httpd_auth, default_authentication_handler}
+secure_rewrites = true
+allow_jsonp = false
+
+enable_cors = false
+enable_xframe_options = false
Review comment:
I wonder if we could move these to code default and keep them commented
out and it would help out with another pending issue
https://github.com/apache/couchdb/issues/3473
##########
File path: rel/overlay/etc/default.ini
##########
@@ -275,7 +309,10 @@ require_valid_user = false
timeout = 600 ; number of seconds before automatic logout
auth_cache_size = 50 ; size is number of cache entries
allow_persistent_cookies = true ; set to false to disallow persistent cookies
-iterations = 10 ; iterations for password hashing
Review comment:
We should decide if we'd want to move all settings from
`couch_httpd_auth` (which can be moved) to `chttpd_auth` or not move any in
this PR. It is odd to move just one one setting leave say
`allow_persistent_cookies = true` behind.
##########
File path: rel/overlay/etc/default.ini
##########
@@ -662,7 +699,7 @@ compaction = false
; The default number of results returned from a search on a partition
; of a database.
; limit_partitions = 2000
-
Review comment:
Avoid whitespace changes only
##########
File path: src/chttpd/src/chttpd_node.erl
##########
@@ -137,7 +137,7 @@ handle_node_req(#httpd{path_parts=[_, Node | PathParts],
{_, Query, Fragment} = mochiweb_util:urlsplit_path(RawUri),
NewPath0 = "/" ++ lists:join("/", [couch_util:url_encode(P) || P <-
PathParts]),
NewRawPath = mochiweb_util:urlunsplit_path({NewPath0, Query, Fragment}),
- MaxSize = config:get_integer("httpd", "max_http_request_size",
4294967296),
Review comment:
Previously we expected an integer back but now we'd return a list
(string) value back if the file has the value set. We could technically tell
from the value of the default variable which type we expect back - if the
default is an integer we'd want to do a `config:get_integer/3`, if the default
is a boolean a `config:get_boolean/3`. So it would kind of work, but it would
be inconsistent with how the rest of the config code looks. So perhaps the best
option would be to have a set of `chttd_util:get_chttpd_config_integer/3` and
`chttpd_util:get_chttpd_config_boolean/3` functions.
##########
File path: src/chttpd/test/eunit/chttpd_util_test.erl
##########
@@ -0,0 +1,94 @@
+% 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_util_test).
+
+-include_lib("couch/include/couch_eunit.hrl").
+-include("chttpd_test.hrl").
+
+setup() -> ok.
+
+teardown(_) -> ok.
+
+chttpd_util_config_test_() ->
+ {
+ "chttpd util config tests",
+ {
+ setup,
+ fun test_util:start_couch/0,
+ fun test_util:stop_couch/1,
+ {
+ foreach,
+ fun setup/0,
+ fun teardown/1,
+ [
+
?TDEF_FE(get_chttpd_config_with_undefined_option),
+
?TDEF_FE(get_chttpd_config_with_httpd_option),
+
?TDEF_FE(get_chttpd_config_with_chttpd_option),
+
?TDEF_FE(get_chttpd_config_with_chttpd_option_which_moved_from_httpd),
+
?TDEF_FE(get_chttpd_auth_config_with_undefined_option),
+
?TDEF_FE(get_chttpd_auth_config_with_couch_httpd_auth_option),
+
?TDEF_FE(get_chttpd_auth_config_with_chttpd_auth_option_which_moved_from_couch_httpd_auth)
+ ]
+ }
+ }
+ }.
+
+get_chttpd_config_with_undefined_option(_) ->
+ ?assertEqual(undefined,
chttpd_util:get_chttpd_config(undefined_option)),
+ ?assertEqual(default, chttpd_util:get_chttpd_config(undefined_option,
default)),
+ ?assertEqual(123, chttpd_util:get_chttpd_config(undefined_option, 123)),
+ ?assertEqual(0.2, chttpd_util:get_chttpd_config(undefined_option, 0.2)),
+ ?assert(chttpd_util:get_chttpd_config(undefined_option, true)),
+ ?assertNot(chttpd_util:get_chttpd_config(undefined_option, false)),
+ ?assertEqual("abc", chttpd_util:get_chttpd_config(undefined_option,
"abc")),
+ ?assertEqual("", chttpd_util:get_chttpd_config(undefined_option, "")).
+
+get_chttpd_config_with_httpd_option(_) ->
+ ?assertEqual("0", chttpd_util:get_chttpd_config(port)),
+ ?assertEqual("127.0.0.1", chttpd_util:get_chttpd_config(bind_address)),
+ ?assertEqual("0", chttpd_util:get_chttpd_config(port, "8080")),
+ ?assertEqual("127.0.0.1", chttpd_util:get_chttpd_config(bind_address,
"255.255.255.255")).
Review comment:
We don't want to get the httpd socket options via the chttpd_util shim
functions. Options which pertain to the socket connection would stay in httpd
(port, bind_address, socket and server settings).
##########
File path: src/couch/src/couch_changes.erl
##########
@@ -370,9 +370,7 @@ get_changes_timeout(Args, Callback) ->
timeout = Timeout,
feed = ResponseType
} = Args,
- DefaultTimeout = list_to_integer(
- config:get("httpd", "changes_timeout", "60000")
- ),
+ DefaultTimeout = chttpd_util:get_chttpd_config(changes_timeout, 60000),
Review comment:
Good idea to convert it to an integer but we'd want to go through some
new `get_chttpd_config_integer/3` calls here probably
##########
File path: src/chttpd/test/eunit/chttpd_util_test.erl
##########
@@ -0,0 +1,94 @@
+% 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_util_test).
+
+-include_lib("couch/include/couch_eunit.hrl").
+-include("chttpd_test.hrl").
+
+setup() -> ok.
+
+teardown(_) -> ok.
+
+chttpd_util_config_test_() ->
+ {
+ "chttpd util config tests",
+ {
+ setup,
+ fun test_util:start_couch/0,
+ fun test_util:stop_couch/1,
+ {
+ foreach,
+ fun setup/0,
+ fun teardown/1,
+ [
+
?TDEF_FE(get_chttpd_config_with_undefined_option),
+
?TDEF_FE(get_chttpd_config_with_httpd_option),
+
?TDEF_FE(get_chttpd_config_with_chttpd_option),
+
?TDEF_FE(get_chttpd_config_with_chttpd_option_which_moved_from_httpd),
+
?TDEF_FE(get_chttpd_auth_config_with_undefined_option),
+
?TDEF_FE(get_chttpd_auth_config_with_couch_httpd_auth_option),
+
?TDEF_FE(get_chttpd_auth_config_with_chttpd_auth_option_which_moved_from_couch_httpd_auth)
+ ]
+ }
+ }
+ }.
+
+get_chttpd_config_with_undefined_option(_) ->
+ ?assertEqual(undefined,
chttpd_util:get_chttpd_config(undefined_option)),
+ ?assertEqual(default, chttpd_util:get_chttpd_config(undefined_option,
default)),
+ ?assertEqual(123, chttpd_util:get_chttpd_config(undefined_option, 123)),
+ ?assertEqual(0.2, chttpd_util:get_chttpd_config(undefined_option, 0.2)),
+ ?assert(chttpd_util:get_chttpd_config(undefined_option, true)),
+ ?assertNot(chttpd_util:get_chttpd_config(undefined_option, false)),
+ ?assertEqual("abc", chttpd_util:get_chttpd_config(undefined_option,
"abc")),
+ ?assertEqual("", chttpd_util:get_chttpd_config(undefined_option, "")).
+
+get_chttpd_config_with_httpd_option(_) ->
+ ?assertEqual("0", chttpd_util:get_chttpd_config(port)),
+ ?assertEqual("127.0.0.1", chttpd_util:get_chttpd_config(bind_address)),
+ ?assertEqual("0", chttpd_util:get_chttpd_config(port, "8080")),
+ ?assertEqual("127.0.0.1", chttpd_util:get_chttpd_config(bind_address,
"255.255.255.255")).
+
+get_chttpd_config_with_chttpd_option(_) ->
+ ?assertEqual("0", chttpd_util:get_chttpd_config(port)),
+ ?assertEqual("127.0.0.1", chttpd_util:get_chttpd_config(bind_address)),
+ ?assertEqual("100",
chttpd_util:get_chttpd_config(max_db_number_for_dbs_info_req)),
+ ?assertEqual("0", chttpd_util:get_chttpd_config(port, "8080")),
+ ?assertEqual("127.0.0.1", chttpd_util:get_chttpd_config(bind_address,
"127.0.0.2")),
+ ?assertEqual("100",
chttpd_util:get_chttpd_config(max_db_number_for_dbs_info_req, "0")).
+
+get_chttpd_config_with_chttpd_option_which_moved_from_httpd(_) ->
+ ?assertEqual("4294967296",
chttpd_util:get_chttpd_config(max_http_request_size)),
+ ?assertEqual("4294967296",
chttpd_util:get_chttpd_config(max_http_request_size, "0")),
+ ?assertEqual(undefined, chttpd_util:get_chttpd_config(max_uri_length)),
+ ?assertEqual(6000, chttpd_util:get_chttpd_config(max_uri_length, 6000)),
+ ?assertEqual("X-Forwarded-Host",
chttpd_util:get_chttpd_config(x_forwarded_host, "X-Forwarded-Host")),
+ ?assertEqual(undefined,
chttpd_util:get_chttpd_config('WWW-Authenticate')),
+ ?assertEqual("false", chttpd_util:get_chttpd_config(enable_cors,
"true")).
+
Review comment:
While it's good to test all the new options, what we should be after is
to test coverage of behavior. Try to set a setting in setup/0 in `httpd` only
then check how we read it, then set it in `chttpd` only, then both in `httpd`
and `chttpd`, then neither (and it would use the default though your first test
suite `get_chttpd_config_with_undefined_option` covers that case).
##########
File path: src/couch/src/couch_httpd_rewrite.erl
##########
@@ -438,7 +438,7 @@ path_to_list([<<>>|R], Acc, DotDotCount) ->
path_to_list([<<"*">>|R], Acc, DotDotCount) ->
path_to_list(R, [?MATCH_ALL|Acc], DotDotCount);
path_to_list([<<"..">>|R], Acc, DotDotCount) when DotDotCount == 2 ->
- case config:get("httpd", "secure_rewrites", "true") of
+ case chttpd_util:get_chttpd_config(secure_rewrites, "true") of
Review comment:
This could be a `get_boolean/3` I think. The setting might have been
created before we had a `config:get_boolean/3` I think.
##########
File path: src/chttpd/src/chttpd_util.erl
##########
@@ -0,0 +1,44 @@
+% 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_util).
+
+
+-export([
+ get_chttpd_config/1, get_chttpd_config/2,
+ get_chttpd_auth_config/1, get_chttpd_auth_config/2
+]).
+
+
+get_chttpd_config(Key) when is_atom(Key) ->
+ case config:get("httpd", atom_to_list(Key)) of
Review comment:
It is interesting to think about various cases of when the setting was
locally set by a user and if they want to update it after this PR. Checking
httpd first ensure we would always catch the user's `local.ini` setting they
had before this PR. However, we'd want to document that if they want to update
the setting, they will have to delete the httpd setting and set a new chttpd
one. If they don't their new chttpd setting will be ignored.
However, I think if we manage to switch all moved setting to be commented
out in the chttpd section, like we thought about here
https://github.com/apache/couchdb/pull/3567/files#r633678428, then we can have
the best of both worlds! We'd check `chttpd` first, and since it will be
`undefined` by default, we'd check the `httpd` section next. If the user wants
to set a value, they would just set `chttpd` and forget about `httpd`
altogether after that. But for this logic to work, we must not set the explicit
values in `[chttpd] ...` default.ini section and always use commented out
defaults.
##########
File path: rel/overlay/etc/default.ini
##########
@@ -662,7 +699,7 @@ compaction = false
; The default number of results returned from a search on a partition
; of a database.
; limit_partitions = 2000
-
Review comment:
Avoid whitespace changes
--
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:
[email protected]
