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 2021/05/17 13:33:07 UTC
[GitHub] [couchdb] jiahuili430 opened a new pull request #3567: CouchDB 3.x: Move config options from [httpd] to [chttpd] [WIP]
jiahuili430 opened a new pull request #3567:
URL: https://github.com/apache/couchdb/pull/3567
## Overview
Move some configuration options from [httpd] to [chttpd], but port, bind_address, server_options and socket_options remain in [httpd].
Since users have local settings, we cannot just move all options directly to [chttpd].
<!-- Please give a short brief for the pull request,
what problem it solves or how it makes things better. -->
## Testing recommendations
make eunit apps=chttpd suites=chttpd_util_test
<!-- Describe how we can test your changes.
Does it provides any behaviour that the end users
could notice? -->
## Related Issues or Pull Requests
This fixes #3472
<!-- If your changes affects multiple components in different
repositories please put links to those issues or pull requests here. -->
## Checklist
- [x] Code is written and works correctly
- [x] Changes are covered by tests
- [ ] 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] jiahuili430 commented on a change in pull request #3567: CouchDB 3.x: Move config options from [httpd] to [chttpd]
Posted by GitBox <gi...@apache.org>.
jiahuili430 commented on a change in pull request #3567:
URL: https://github.com/apache/couchdb/pull/3567#discussion_r637183051
##########
File path: src/chttpd/src/chttpd_util.erl
##########
@@ -0,0 +1,43 @@
+% 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_config_integer/2,
+ get_chttpd_config_boolean/2
+]).
+
+
+get_chttpd_config(Key) when is_atom(Key) ->
+ config:get("chttpd", atom_to_list(Key),
+ config:get("httpd", atom_to_list(Key))).
+
+
+get_chttpd_config(Key, Default) when is_atom(Key) ->
+ config:get("chttpd", atom_to_list(Key),
+ config:get("httpd", atom_to_list(Key), Default)).
+
+
+get_chttpd_config_integer(Key, Default)
+ when is_atom(Key) andalso is_integer(Default) ->
+ config:get_integer("chttpd", atom_to_list(Key),
+ config:get_integer("httpd", atom_to_list(Key), Default)).
+
+
+get_chttpd_config_boolean(Key, Default)
Review comment:
Thanks for your review, considering the price of type conversion, I will change them back to strings.
--
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] jiahuili430 commented on a change in pull request #3567: CouchDB 3.x: Move config options from [httpd] to [chttpd] [WIP]
Posted by GitBox <gi...@apache.org>.
jiahuili430 commented on a change in pull request #3567:
URL: https://github.com/apache/couchdb/pull/3567#discussion_r635598243
##########
File path: rel/overlay/etc/default.ini
##########
@@ -276,6 +299,7 @@ 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:
changed
--
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 #3567: CouchDB 3.x: Move config options from [httpd] to [chttpd] [WIP]
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3567:
URL: https://github.com/apache/couchdb/pull/3567#discussion_r633648899
##########
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.
--
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] jiahuili430 commented on a change in pull request #3567: CouchDB 3.x: Move config options from [httpd] to [chttpd] [WIP]
Posted by GitBox <gi...@apache.org>.
jiahuili430 commented on a change in pull request #3567:
URL: https://github.com/apache/couchdb/pull/3567#discussion_r634558051
##########
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:
Added get_chttpd_config_integer/3 and get_chttpd_config_boolean/3 functions.
--
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 #3567: CouchDB 3.x: Move config options from [httpd] to [chttpd] [WIP]
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3567:
URL: https://github.com/apache/couchdb/pull/3567#discussion_r633671838
##########
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
--
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 #3567: CouchDB 3.x: Move config options from [httpd] to [chttpd] [WIP]
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3567:
URL: https://github.com/apache/couchdb/pull/3567#discussion_r635590060
##########
File path: rel/overlay/etc/default.ini
##########
@@ -147,6 +147,27 @@ max_db_number_for_dbs_info_req = 100
; prevent non-admins from accessing /_all_dbs
; admin_only_all_dbs = true
+;;;; Moved from [httpd] ;;;;;
+;secure_rewrites = true
+;allow_jsonp = false
+
+;enable_cors = false
+;enable_xframe_options = false
+
+; CouchDB can optionally enforce a maximum uri length;
+; max_uri_length = 8000
Review comment:
It might be better to be consistent and use the format where there is no space between ';' and the configuration key but there is a space between ';` and comment line. For example:
```
; This is a comment
;this_is_a_config_key = foo
```
The reason to do that is it makes the comment and the keys stand out a bit.
--
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 #3567: CouchDB 3.x: Move config options from [httpd] to [chttpd] [WIP]
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3567:
URL: https://github.com/apache/couchdb/pull/3567#discussion_r635613456
##########
File path: src/chttpd/src/chttpd_util.erl
##########
@@ -0,0 +1,45 @@
+% 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,
Review comment:
Let's keep these as one per-line
--
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 #3567: CouchDB 3.x: Move config options from [httpd] to [chttpd] [WIP]
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3567:
URL: https://github.com/apache/couchdb/pull/3567#discussion_r635557609
##########
File path: rel/overlay/etc/default.ini
##########
@@ -147,6 +147,27 @@ 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}
Review comment:
For the scheme of checking chttpd first then httpd to work, the settings which moved should all be commented out, so we should try to have this one commented out as well
--
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] jiahuili430 commented on a change in pull request #3567: CouchDB 3.x: Move config options from [httpd] to [chttpd] [WIP]
Posted by GitBox <gi...@apache.org>.
jiahuili430 commented on a change in pull request #3567:
URL: https://github.com/apache/couchdb/pull/3567#discussion_r635585544
##########
File path: rel/overlay/etc/default.ini
##########
@@ -147,6 +147,27 @@ 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
+
+; CouchDB can optionally enforce a maximum uri length;
+; max_uri_length = 8000
+; changes_timeout = 60000
Review comment:
added
--
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 #3567: CouchDB 3.x: Move config options from [httpd] to [chttpd] [WIP]
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3567:
URL: https://github.com/apache/couchdb/pull/3567#discussion_r633678529
##########
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
--
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 #3567: CouchDB 3.x: Move config options from [httpd] to [chttpd] [WIP]
Posted by GitBox <gi...@apache.org>.
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
--
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] jiahuili430 closed pull request #3567: CouchDB 3.x: Move config options from [httpd] to [chttpd] [WIP]
Posted by GitBox <gi...@apache.org>.
jiahuili430 closed pull request #3567:
URL: https://github.com/apache/couchdb/pull/3567
--
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] jiahuili430 commented on a change in pull request #3567: CouchDB 3.x: Move config options from [httpd] to [chttpd] [WIP]
Posted by GitBox <gi...@apache.org>.
jiahuili430 commented on a change in pull request #3567:
URL: https://github.com/apache/couchdb/pull/3567#discussion_r635598169
##########
File path: rel/overlay/etc/default.ini
##########
@@ -189,27 +210,29 @@ 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
+
+; These settings were moved to [chttpd]
+;authentication_handlers =
Review comment:
changed
--
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 #3567: CouchDB 3.x: Move config options from [httpd] to [chttpd] [WIP]
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3567:
URL: https://github.com/apache/couchdb/pull/3567#discussion_r636495298
##########
File path: src/couch/src/couch_httpd.erl
##########
@@ -837,7 +837,7 @@ initialize_jsonp(Req) ->
CallBack ->
try
% make sure jsonp is configured on (default off)
- case config:get("httpd", "allow_jsonp", "false") of
+ case chttpd_util:get_chttpd_config(allow_jsonp, "false") of
Review comment:
We can do a `get_chttpd_config_boolean/3` I think here too?
--
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 #3567: CouchDB 3.x: Move config options from [httpd] to [chttpd] [WIP]
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3567:
URL: https://github.com/apache/couchdb/pull/3567#discussion_r636505046
##########
File path: rel/overlay/etc/default.ini
##########
@@ -147,6 +147,27 @@ max_db_number_for_dbs_info_req = 100
; prevent non-admins from accessing /_all_dbs
; admin_only_all_dbs = true
+;;;; Moved from [httpd] ;;;;;
Review comment:
Minor nit: since we're not using ';;;...' type headers anywhere else in the file, let's stick to just a plain comment, otherwise I think it looks inconsistent
--
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 #3567: CouchDB 3.x: Move config options from [httpd] to [chttpd] [WIP]
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3567:
URL: https://github.com/apache/couchdb/pull/3567#discussion_r635559049
##########
File path: rel/overlay/etc/default.ini
##########
@@ -276,6 +299,7 @@ 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:
Whitespace-only change
--
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 #3567: CouchDB 3.x: Move config options from [httpd] to [chttpd] [WIP]
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [couchdb] jiahuili430 commented on a change in pull request #3567: CouchDB 3.x: Move config options from [httpd] to [chttpd] [WIP]
Posted by GitBox <gi...@apache.org>.
jiahuili430 commented on a change in pull request #3567:
URL: https://github.com/apache/couchdb/pull/3567#discussion_r635595213
##########
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:
A "chttpd_util_behavior_test(_)" was written for this.
--
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 #3567: CouchDB 3.x: Move config options from [httpd] to [chttpd] [WIP]
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3567:
URL: https://github.com/apache/couchdb/pull/3567#discussion_r633705296
##########
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.
--
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] jiahuili430 commented on a change in pull request #3567: CouchDB 3.x: Move config options from [httpd] to [chttpd] [WIP]
Posted by GitBox <gi...@apache.org>.
jiahuili430 commented on a change in pull request #3567:
URL: https://github.com/apache/couchdb/pull/3567#discussion_r634552992
##########
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:
I will pay attention next time.
--
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 #3567: CouchDB 3.x: Move config options from [httpd] to [chttpd] [WIP]
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3567:
URL: https://github.com/apache/couchdb/pull/3567#discussion_r636493640
##########
File path: src/chttpd/test/eunit/chttpd_util_test.erl
##########
@@ -0,0 +1,114 @@
+% 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 = config:set("httpd", "both_in_httpd_chttpd", "get_in_httpd", _Persist = false),
+ ok = config:set("chttpd", "both_in_httpd_chttpd", "get_in_chttpd", _Persist = false),
+ ok = config:set("chttpd", "only_in_httpd", "true", _Persist = false),
+ ok = config:set("chttpd", "only_in_chttpd", "100", _Persist = false).
+
+teardown(_) ->
+ ok = config:delete("httpd", "both_in_httpd_chttpd", _Persist = false),
+ ok = config:delete("chttpd", "both_in_httpd_chttpd", _Persist = false),
+ ok = config:delete("chttpd", "only_in_httpd", _Persist = false),
+ ok = config:delete("chttpd", "only_in_chttpd", _Persist = false).
+
+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(chttpd_util_behavior_test),
+ ?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_config_integer_test),
+ ?TDEF_FE(get_chttpd_config_boolean_test),
+ ?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)
Review comment:
Let's keep chttpd_auth changes for another PR
--
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 #3567: CouchDB 3.x: Move config options from [httpd] to [chttpd] [WIP]
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3567:
URL: https://github.com/apache/couchdb/pull/3567#discussion_r636490603
##########
File path: src/chttpd/src/chttpd_rewrite.erl
##########
@@ -442,7 +442,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:
Let use `get_chttpd_config_boolean(secure_rewrites, true)` since we a boolean getter function now. Then result should be `false` or `true`.
--
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] jiahuili430 commented on a change in pull request #3567: CouchDB 3.x: Move config options from [httpd] to [chttpd]
Posted by GitBox <gi...@apache.org>.
jiahuili430 commented on a change in pull request #3567:
URL: https://github.com/apache/couchdb/pull/3567#discussion_r637130956
##########
File path: src/chttpd/src/chttpd_util.erl
##########
@@ -0,0 +1,43 @@
+% 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_config_integer/2,
+ get_chttpd_config_boolean/2
+]).
+
+
+get_chttpd_config(Key) when is_atom(Key) ->
Review comment:
To simplify the code. Otherwise, I need to use the key with quotation marks.
Thanks for the review.
--
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 #3567: CouchDB 3.x: Move config options from [httpd] to [chttpd] [WIP]
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3567:
URL: https://github.com/apache/couchdb/pull/3567#discussion_r636492621
##########
File path: src/chttpd/src/chttpd_util.erl
##########
@@ -0,0 +1,45 @@
+% 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_config_integer/2, get_chttpd_config_boolean/2,
+ get_chttpd_auth_config/1, get_chttpd_auth_config/2
+]).
+
+
+get_chttpd_config(Key) when is_atom(Key) ->
+ config:get("chttpd", atom_to_list(Key),
+ config:get("httpd", atom_to_list(Key))).
+
+get_chttpd_config(Key, Default) when is_atom(Key) ->
+ config:get("chttpd", atom_to_list(Key),
+ config:get("httpd", atom_to_list(Key), Default)).
+
+get_chttpd_config_integer(Key, Default) when is_atom(Key) andalso is_integer(Default) ->
+ config:get_integer("chttpd", atom_to_list(Key),
+ config:get_integer("httpd", atom_to_list(Key), Default)).
+
+get_chttpd_config_boolean(Key, Default) when is_atom(Key) andalso is_boolean(Default) ->
+ config:get_boolean("chttpd", atom_to_list(Key),
+ config:get_boolean("httpd", atom_to_list(Key), Default)).
+
+get_chttpd_auth_config(Key) when is_atom(Key) ->
+ config:get("chttpd_auth", atom_to_list(Key),
+ config:get("couch_httpd_auth", atom_to_list(Key))).
+
+get_chttpd_auth_config(Key, Default) when is_atom(Key) ->
+ config:get("chttpd_auth", atom_to_list(Key),
+ config:get("couch_httpd_auth", atom_to_list(Key), Default)).
Review comment:
Let's keep the auth config updates for another PR for now.
--
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] tonysun83 commented on a change in pull request #3567: CouchDB 3.x: Move config options from [httpd] to [chttpd]
Posted by GitBox <gi...@apache.org>.
tonysun83 commented on a change in pull request #3567:
URL: https://github.com/apache/couchdb/pull/3567#discussion_r637127327
##########
File path: src/chttpd/src/chttpd_util.erl
##########
@@ -0,0 +1,43 @@
+% 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_config_integer/2,
+ get_chttpd_config_boolean/2
+]).
+
+
+get_chttpd_config(Key) when is_atom(Key) ->
+ config:get("chttpd", atom_to_list(Key),
+ config:get("httpd", atom_to_list(Key))).
+
+
+get_chttpd_config(Key, Default) when is_atom(Key) ->
+ config:get("chttpd", atom_to_list(Key),
+ config:get("httpd", atom_to_list(Key), Default)).
+
+
+get_chttpd_config_integer(Key, Default)
+ when is_atom(Key) andalso is_integer(Default) ->
+ config:get_integer("chttpd", atom_to_list(Key),
+ config:get_integer("httpd", atom_to_list(Key), Default)).
+
+
+get_chttpd_config_boolean(Key, Default)
Review comment:
I noticed that previously we were doing something like:
```
config:get("httpd", "enable_cors", "false")
```
Similar to what @iilyak asked above, was there a specific reason you decided to convert the string key/value pairs into boolean/integer, but then convert them back to with `atom_to_list`? Was it to make it more organized between different types?
--
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 #3567: CouchDB 3.x: Move config options from [httpd] to [chttpd] [WIP]
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3567:
URL: https://github.com/apache/couchdb/pull/3567#discussion_r636492894
##########
File path: src/chttpd/src/chttpd_xframe_options.erl
##########
@@ -71,7 +71,7 @@ check_host(#httpd{mochi_req = MochiReq} = Req, Config) ->
get_xframe_config(#httpd{xframe_config = undefined}) ->
- EnableXFrame = config:get("httpd", "enable_xframe_options", "false") =:= "true",
+ EnableXFrame = chttpd_util:get_chttpd_config_boolean(enable_xframe_options, false) =:= true,
Review comment:
Would be just `chttpd_util:get_chttpd_config_boolean(enable_xframe_options, false)` I think?
--
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 #3567: CouchDB 3.x: Move config options from [httpd] to [chttpd] [WIP]
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3567:
URL: https://github.com/apache/couchdb/pull/3567#discussion_r633693810
##########
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.
--
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 merged pull request #3567: CouchDB 3.x: Move config options from [httpd] to [chttpd]
Posted by GitBox <gi...@apache.org>.
nickva merged pull request #3567:
URL: https://github.com/apache/couchdb/pull/3567
--
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 #3567: CouchDB 3.x: Move config options from [httpd] to [chttpd] [WIP]
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3567:
URL: https://github.com/apache/couchdb/pull/3567#discussion_r635557609
##########
File path: rel/overlay/etc/default.ini
##########
@@ -147,6 +147,27 @@ 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}
Review comment:
I think we'd probably want to keep the authentication handlers for http in httpd section. Chttpd already handles its own authentication handlers with `"{chttpd_auth,..."` defaults in the code
--
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 #3567: CouchDB 3.x: Move config options from [httpd] to [chttpd] [WIP]
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3567:
URL: https://github.com/apache/couchdb/pull/3567#discussion_r635557086
##########
File path: rel/overlay/etc/default.ini
##########
@@ -189,27 +210,29 @@ 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
+
+; These settings were moved to [chttpd]
+;authentication_handlers =
+;secure_rewrites =
+;allow_jsonp =
+;enable_cors =
+;enable_xframe_options =
; CouchDB can optionally enforce a maximum uri length;
-; max_uri_length = 8000
-; changes_timeout = 60000
-; config_whitelist =
-; max_uri_length =
-; rewrite_limit = 100
-; x_forwarded_host = X-Forwarded-Host
-; x_forwarded_proto = X-Forwarded-Proto
-; x_forwarded_ssl = X-Forwarded-Ssl
-; Maximum allowed http request size. Applies to both clustered and local port.
-max_http_request_size = 4294967296 ; 4GB
+; max_uri_length =
+; changes_timeout =
+; config_whitelist =
+; rewrite_limit =
+; x_forwarded_host =
+; x_forwarded_proto =
+; x_forwarded_ssl =
+;Maximum allowed http request size. Applies to both clustered and local port.
Review comment:
this one was removed too I think, we can remove its comment here
--
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] jiahuili430 closed pull request #3567: CouchDB 3.x: Move config options from [httpd] to [chttpd] [WIP]
Posted by GitBox <gi...@apache.org>.
jiahuili430 closed pull request #3567:
URL: https://github.com/apache/couchdb/pull/3567
--
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 #3567: CouchDB 3.x: Move config options from [httpd] to [chttpd] [WIP]
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3567:
URL: https://github.com/apache/couchdb/pull/3567#discussion_r636486686
##########
File path: src/chttpd/src/chttpd_cors.erl
##########
@@ -280,7 +280,7 @@ allow_credentials(Config, Origin) ->
get_cors_config(#httpd{cors_config = undefined, mochi_req = MochiReq}) ->
Host = couch_httpd_vhost:host(MochiReq),
- EnableCors = config:get("httpd", "enable_cors", "false") =:= "true",
+ EnableCors = chttpd_util:get_chttpd_config_boolean(enable_cors, false) =:= true,
Review comment:
It might be simpler to use just `chttpd_util:get_chttpd_config_boolean(enable_cors, false)` I think?
--
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 #3567: CouchDB 3.x: Move config options from [httpd] to [chttpd] [WIP]
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3567:
URL: https://github.com/apache/couchdb/pull/3567#discussion_r635558621
##########
File path: rel/overlay/etc/default.ini
##########
@@ -189,27 +210,29 @@ 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
+
+; These settings were moved to [chttpd]
+;authentication_handlers =
Review comment:
We don't have to necessarily list them one their own line with = signs, could probably do a list ensuring it is less than 80 columns per line. Something like:
```
; These settings were moved to [chttpd]
;authentication_handlers, secure_rewrites, allow_jsonp, enable_cors,
;enable_xframe_options, max_uri_length, ...
--
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 pull request #3567: CouchDB 3.x: Move config options from [httpd] to [chttpd] [WIP]
Posted by GitBox <gi...@apache.org>.
nickva commented on pull request #3567:
URL: https://github.com/apache/couchdb/pull/3567#issuecomment-846007641
@jiahuili430 In the light of almost merging the erlfmt formatting PR, I think let's not worry about line length and emilio formatting as all that will be done automatically afterwards and it makes things simpler for you.
--
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 #3567: CouchDB 3.x: Move config options from [httpd] to [chttpd] [WIP]
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3567:
URL: https://github.com/apache/couchdb/pull/3567#discussion_r635558621
##########
File path: rel/overlay/etc/default.ini
##########
@@ -189,27 +210,29 @@ 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
+
+; These settings were moved to [chttpd]
+;authentication_handlers =
Review comment:
We don't have to necessarily list them one their own line with = signs, could probably do a list ensuring it is less than 80 columns per line. Something like:
```
; These settings were moved to [chttpd]
;authentication_handlers, secure_rewrites, allow_jsonp, enable_cors,
;enable_xframe_options, max_uri_length, ...
``` perhaps?
--
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 #3567: CouchDB 3.x: Move config options from [httpd] to [chttpd]
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3567:
URL: https://github.com/apache/couchdb/pull/3567#discussion_r637102294
##########
File path: src/chttpd/test/eunit/chttpd_util_test.erl
##########
@@ -0,0 +1,107 @@
+% 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 = config:set("httpd", "both_exist", "get_in_httpd", _Persist = false),
+ ok = config:set("chttpd", "both_exist", "get_in_chttpd", _Persist = false),
+ ok = config:set("chttpd", "only_in_httpd", "true", _Persist = false),
Review comment:
I think one was supposed to be `:set("httpd", ...`
##########
File path: src/chttpd/test/eunit/chttpd_util_test.erl
##########
@@ -0,0 +1,107 @@
+% 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 = config:set("httpd", "both_exist", "get_in_httpd", _Persist = false),
+ ok = config:set("chttpd", "both_exist", "get_in_chttpd", _Persist = false),
+ ok = config:set("chttpd", "only_in_httpd", "true", _Persist = false),
+ ok = config:set("chttpd", "only_in_chttpd", "10", _Persist = false).
+
+
+teardown(_) ->
+ ok = config:delete("httpd", "both_exist", _Persist = false),
+ ok = config:delete("chttpd", "both_exist", _Persist = false),
+ ok = config:delete("chttpd", "only_in_httpd", _Persist = false),
Review comment:
"httpd"
--
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 #3567: CouchDB 3.x: Move config options from [httpd] to [chttpd] [WIP]
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3567:
URL: https://github.com/apache/couchdb/pull/3567#discussion_r633678428
##########
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.
--
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 #3567: CouchDB 3.x: Move config options from [httpd] to [chttpd] [WIP]
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3567:
URL: https://github.com/apache/couchdb/pull/3567#discussion_r633713265
##########
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.
--
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 #3567: CouchDB 3.x: Move config options from [httpd] to [chttpd] [WIP]
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3567:
URL: https://github.com/apache/couchdb/pull/3567#discussion_r633699250
##########
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).
--
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] jiahuili430 commented on a change in pull request #3567: CouchDB 3.x: Move config options from [httpd] to [chttpd] [WIP]
Posted by GitBox <gi...@apache.org>.
jiahuili430 commented on a change in pull request #3567:
URL: https://github.com/apache/couchdb/pull/3567#discussion_r635208897
##########
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:
Moved back. I will create another PR to move options from [couch_httpd_auth] to [chttpd_auth].
--
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 #3567: CouchDB 3.x: Move config options from [httpd] to [chttpd] [WIP]
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3567:
URL: https://github.com/apache/couchdb/pull/3567#discussion_r636493827
##########
File path: src/chttpd/test/eunit/chttpd_util_test.erl
##########
@@ -0,0 +1,114 @@
+% 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 = config:set("httpd", "both_in_httpd_chttpd", "get_in_httpd", _Persist = false),
+ ok = config:set("chttpd", "both_in_httpd_chttpd", "get_in_chttpd", _Persist = false),
+ ok = config:set("chttpd", "only_in_httpd", "true", _Persist = false),
+ ok = config:set("chttpd", "only_in_chttpd", "100", _Persist = false).
+
+teardown(_) ->
+ ok = config:delete("httpd", "both_in_httpd_chttpd", _Persist = false),
+ ok = config:delete("chttpd", "both_in_httpd_chttpd", _Persist = false),
+ ok = config:delete("chttpd", "only_in_httpd", _Persist = false),
+ ok = config:delete("chttpd", "only_in_chttpd", _Persist = false).
+
+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(chttpd_util_behavior_test),
+ ?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_config_integer_test),
+ ?TDEF_FE(get_chttpd_config_boolean_test),
+ ?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)
+ ]
+ }
+ }
+ }.
+
+chttpd_util_behavior_test(_) ->
+ ?assertEqual("get_in_chttpd", chttpd_util:get_chttpd_config(both_in_httpd_chttpd)),
+ ?assertEqual(100, chttpd_util:get_chttpd_config_integer(only_in_chttpd, 0)),
+ ?assert(chttpd_util:get_chttpd_config_boolean(only_in_httpd, false)).
+
+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(undefined, chttpd_util:get_chttpd_config(max_uri_length)),
+ ?assertEqual(8000, chttpd_util:get_chttpd_config(max_uri_length, 8000)),
+ ?assertEqual("X-Forwarded-Host", chttpd_util:get_chttpd_config(x_forwarded_host, "X-Forwarded-Host")),
+ ?assertEqual(undefined, chttpd_util:get_chttpd_config('WWW-Authenticate')),
+ ?assert(chttpd_util:get_chttpd_config(enable_cors, true)).
+
+get_chttpd_config_integer_test(_) ->
+ ?assertEqual(4294967296, chttpd_util:get_chttpd_config_integer(max_http_request_size, 4294967296)).
+
+get_chttpd_config_boolean_test(_) ->
+ ?assert(chttpd_util:get_chttpd_config_boolean(allow_jsonp, true)).
+
+get_chttpd_auth_config_with_undefined_option(_) ->
+ ?assertEqual(undefined, chttpd_util:get_chttpd_auth_config(undefined_option)),
+ ?assertEqual(default, chttpd_util:get_chttpd_auth_config(undefined_option, default)),
+ ?assertEqual(123, chttpd_util:get_chttpd_auth_config(undefined_option, 123)),
+ ?assertEqual(0.2, chttpd_util:get_chttpd_auth_config(undefined_option, 0.2)),
+ ?assertEqual("abc", chttpd_util:get_chttpd_auth_config(undefined_option, "abc")).
+
+get_chttpd_auth_config_with_couch_httpd_auth_option(_) ->
+ ?assertEqual("_users", chttpd_util:get_chttpd_auth_config(authentication_db)),
+ ?assertEqual("/_utils/session.html", chttpd_util:get_chttpd_auth_config(authentication_redirect)),
+ ?assertEqual("_users", chttpd_util:get_chttpd_auth_config(authentication_db, "user")),
+ ?assertEqual("/_utils/session.html", chttpd_util:get_chttpd_auth_config(authentication_redirect, "session.html")).
Review comment:
Let's keep line lengths below 80 columns
--
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 #3567: CouchDB 3.x: Move config options from [httpd] to [chttpd] [WIP]
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3567:
URL: https://github.com/apache/couchdb/pull/3567#discussion_r635563144
##########
File path: rel/overlay/etc/default.ini
##########
@@ -147,6 +147,27 @@ 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
+
+; CouchDB can optionally enforce a maximum uri length;
+; max_uri_length = 8000
+; changes_timeout = 60000
Review comment:
Let's leave an empty line in between since "optional enforce" applies to URI length but not `changes_timeout`
--
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] jiahuili430 commented on a change in pull request #3567: CouchDB 3.x: Move config options from [httpd] to [chttpd] [WIP]
Posted by GitBox <gi...@apache.org>.
jiahuili430 commented on a change in pull request #3567:
URL: https://github.com/apache/couchdb/pull/3567#discussion_r635585544
##########
File path: rel/overlay/etc/default.ini
##########
@@ -147,6 +147,27 @@ 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
+
+; CouchDB can optionally enforce a maximum uri length;
+; max_uri_length = 8000
+; changes_timeout = 60000
Review comment:
added empty lines
--
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 #3567: CouchDB 3.x: Move config options from [httpd] to [chttpd] [WIP]
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3567:
URL: https://github.com/apache/couchdb/pull/3567#discussion_r636485210
##########
File path: src/chttpd/src/chttpd.erl
##########
@@ -648,8 +647,7 @@ body(#httpd{mochi_req=MochiReq, req_body=ReqBody}) ->
case ReqBody of
undefined ->
% Maximum size of document PUT request body (4GB)
- MaxSize = config:get_integer("httpd", "max_http_request_size",
- 4294967296),
+ MaxSize = chttpd_util:get_chttpd_config_integer(max_http_request_size, 4294967296),
Review comment:
Let's keep the lines less than =<80 columns
--
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 #3567: CouchDB 3.x: Move config options from [httpd] to [chttpd] [WIP]
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3567:
URL: https://github.com/apache/couchdb/pull/3567#discussion_r633700532
##########
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
--
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 #3567: CouchDB 3.x: Move config options from [httpd] to [chttpd] [WIP]
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3567:
URL: https://github.com/apache/couchdb/pull/3567#discussion_r635563719
##########
File path: src/chttpd/src/chttpd.erl
##########
@@ -489,7 +489,7 @@ set_auth_handlers() ->
AuthenticationDefault = "{chttpd_auth, cookie_authentication_handler},
{chttpd_auth, default_authentication_handler}",
AuthenticationSrcs = couch_httpd:make_fun_spec_strs(
- config:get("chttpd", "authentication_handlers", AuthenticationDefault)),
+ chttpd_util:get_chttpd_config(authentication_handlers, AuthenticationDefault)),
Review comment:
This one will read from chttpd only as discussed in https://github.com/apache/couchdb/pull/3567/files#r635557609
--
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] jiahuili430 commented on a change in pull request #3567: CouchDB 3.x: Move config options from [httpd] to [chttpd] [WIP]
Posted by GitBox <gi...@apache.org>.
jiahuili430 commented on a change in pull request #3567:
URL: https://github.com/apache/couchdb/pull/3567#discussion_r635596179
##########
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:
Changed the order, check [chttpd] first, and then [httpd]. If it is not defined, it will return the default value.
--
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] iilyak commented on a change in pull request #3567: CouchDB 3.x: Move config options from [httpd] to [chttpd]
Posted by GitBox <gi...@apache.org>.
iilyak commented on a change in pull request #3567:
URL: https://github.com/apache/couchdb/pull/3567#discussion_r637116210
##########
File path: src/chttpd/src/chttpd_util.erl
##########
@@ -0,0 +1,43 @@
+% 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_config_integer/2,
+ get_chttpd_config_boolean/2
+]).
+
+
+get_chttpd_config(Key) when is_atom(Key) ->
Review comment:
What was the reason to use atom for keys?
--
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 #3567: CouchDB 3.x: Move config options from [httpd] to [chttpd] [WIP]
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3567:
URL: https://github.com/apache/couchdb/pull/3567#discussion_r636491623
##########
File path: src/chttpd/src/chttpd_util.erl
##########
@@ -0,0 +1,45 @@
+% 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_config_integer/2, get_chttpd_config_boolean/2,
+ get_chttpd_auth_config/1, get_chttpd_auth_config/2
+]).
+
+
+get_chttpd_config(Key) when is_atom(Key) ->
+ config:get("chttpd", atom_to_list(Key),
+ config:get("httpd", atom_to_list(Key))).
+
+get_chttpd_config(Key, Default) when is_atom(Key) ->
+ config:get("chttpd", atom_to_list(Key),
+ config:get("httpd", atom_to_list(Key), Default)).
+
+get_chttpd_config_integer(Key, Default) when is_atom(Key) andalso is_integer(Default) ->
Review comment:
Let's keep number of columns < 80
--
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] jiahuili430 commented on a change in pull request #3567: CouchDB 3.x: Move config options from [httpd] to [chttpd]
Posted by GitBox <gi...@apache.org>.
jiahuili430 commented on a change in pull request #3567:
URL: https://github.com/apache/couchdb/pull/3567#discussion_r637130956
##########
File path: src/chttpd/src/chttpd_util.erl
##########
@@ -0,0 +1,43 @@
+% 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_config_integer/2,
+ get_chttpd_config_boolean/2
+]).
+
+
+get_chttpd_config(Key) when is_atom(Key) ->
Review comment:
To simplify the code. Otherwise, I need to use the key with quotation marks.
--
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] jiahuili430 commented on a change in pull request #3567: CouchDB 3.x: Move config options from [httpd] to [chttpd] [WIP]
Posted by GitBox <gi...@apache.org>.
jiahuili430 commented on a change in pull request #3567:
URL: https://github.com/apache/couchdb/pull/3567#discussion_r635596417
##########
File path: rel/overlay/etc/default.ini
##########
@@ -189,27 +210,29 @@ 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
+
+; These settings were moved to [chttpd]
+;authentication_handlers =
+;secure_rewrites =
+;allow_jsonp =
+;enable_cors =
+;enable_xframe_options =
; CouchDB can optionally enforce a maximum uri length;
-; max_uri_length = 8000
-; changes_timeout = 60000
-; config_whitelist =
-; max_uri_length =
-; rewrite_limit = 100
-; x_forwarded_host = X-Forwarded-Host
-; x_forwarded_proto = X-Forwarded-Proto
-; x_forwarded_ssl = X-Forwarded-Ssl
-; Maximum allowed http request size. Applies to both clustered and local port.
-max_http_request_size = 4294967296 ; 4GB
+; max_uri_length =
+; changes_timeout =
+; config_whitelist =
+; rewrite_limit =
+; x_forwarded_host =
+; x_forwarded_proto =
+; x_forwarded_ssl =
+;Maximum allowed http request size. Applies to both clustered and local port.
Review comment:
deleted
--
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 #3567: CouchDB 3.x: Move config options from [httpd] to [chttpd] [WIP]
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3567:
URL: https://github.com/apache/couchdb/pull/3567#discussion_r633704495
##########
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).
--
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 #3567: CouchDB 3.x: Move config options from [httpd] to [chttpd] [WIP]
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3567:
URL: https://github.com/apache/couchdb/pull/3567#discussion_r636496068
##########
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:
I think we could do a `get_chttpd_config_boolean/3` here too?
--
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] jiahuili430 closed pull request #3567: CouchDB 3.x: Move config options from [httpd] to [chttpd] [WIP]
Posted by GitBox <gi...@apache.org>.
jiahuili430 closed pull request #3567:
URL: https://github.com/apache/couchdb/pull/3567
--
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] jiahuili430 commented on a change in pull request #3567: CouchDB 3.x: Move config options from [httpd] to [chttpd] [WIP]
Posted by GitBox <gi...@apache.org>.
jiahuili430 commented on a change in pull request #3567:
URL: https://github.com/apache/couchdb/pull/3567#discussion_r635596804
##########
File path: rel/overlay/etc/default.ini
##########
@@ -147,6 +147,27 @@ 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}
Review comment:
authentication_handlers moved back to the [httpd].
--
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 #3567: CouchDB 3.x: Move config options from [httpd] to [chttpd] [WIP]
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3567:
URL: https://github.com/apache/couchdb/pull/3567#discussion_r633678529
##########
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:
users@infra.apache.org