You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@couchdb.apache.org by mikewallace1979 <gi...@git.apache.org> on 2015/06/03 13:39:09 UTC
[GitHub] couchdb-config pull request: 2708 stronger testing for config set
GitHub user mikewallace1979 opened a pull request:
https://github.com/apache/couchdb-config/pull/3
2708 stronger testing for config set
Closes COUCHDB-2708
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/apache/couchdb-config 2708-stronger-testing-for-config-set
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/couchdb-config/pull/3.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #3
----
commit f695b782f563de65913e8cd9d7a2597ef395369e
Author: Robert Newson <rn...@apache.org>
Date: 2014-11-07T17:47:12Z
strong testing for config:set calls
Closes COUCHDB-2708.
This is a cherry-pick of:
https://github.com/cloudant/config/commit/d48a2bfdaa7c7c1e0004835c42e98d6794050317
Conflicts:
src/config.erl
commit 6566dd43c6f2a6bc8c5c737be2c41477833a3065
Author: Mike Wallace <mi...@apache.org>
Date: 2015-06-03T11:16:58Z
[squash] Add tests for add stronger config set/get/delete testing
commit ee652d2ee00ab5b9afea28c2a2a529e4eacf4195
Author: Mike Wallace <mi...@apache.org>
Date: 2015-06-03T11:23:20Z
[squash] apply stronger testing to config:get/3
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-config pull request: 2708 stronger testing for config set
Posted by mikewallace1979 <gi...@git.apache.org>.
Github user mikewallace1979 commented on a diff in the pull request:
https://github.com/apache/couchdb-config/pull/3#discussion_r32032532
--- Diff: src/config.erl ---
@@ -177,9 +182,19 @@ delete(Section, Key, Reason) ->
delete(Sec, Key, Persist, Reason) when is_binary(Sec) and is_binary(Key) ->
delete(binary_to_list(Sec), binary_to_list(Key), Persist, Reason);
-delete(Section, Key, Persist, Reason) when is_list(Section), is_list(Key) ->
+delete(Section, Key, Persist, Reason) when is_boolean(Persist) ->
+ assert_string(Section),
+ assert_string(Key),
+ if Reason == nil -> ok; true -> assert_string(Reason) end,
gen_server:call(?MODULE, {delete, Section, Key, Persist, Reason}).
+assert_string(Term) ->
+ case io_lib:printable_list(Term) of
--- End diff --
I made the mistake of adding a test for non-latin1 characters which threw up a few issues elsewhere in the module (mainly calls to functions in the `re` module and the `file:write_file/2` call. They should be sorted in that last commit.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-config pull request: 2708 stronger testing for config set
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on a diff in the pull request:
https://github.com/apache/couchdb-config/pull/3#discussion_r31735649
--- Diff: src/config.erl ---
@@ -177,9 +182,19 @@ delete(Section, Key, Reason) ->
delete(Sec, Key, Persist, Reason) when is_binary(Sec) and is_binary(Key) ->
delete(binary_to_list(Sec), binary_to_list(Key), Persist, Reason);
-delete(Section, Key, Persist, Reason) when is_list(Section), is_list(Key) ->
+delete(Section, Key, Persist, Reason) when is_boolean(Persist) ->
+ assert_string(Section),
+ assert_string(Key),
+ if Reason == nil -> ok; true -> assert_string(Reason) end,
gen_server:call(?MODULE, {delete, Section, Key, Persist, Reason}).
+assert_string(Term) ->
+ case io_lib:printable_list(Term) of
--- End diff --
Btw, this PR also lacks related PRs for couchdb-couch and couchdb-chttpd to handle badarg error nicely as you may notice.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-config pull request: 2708 stronger testing for config set
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on the pull request:
https://github.com/apache/couchdb-config/pull/3#issuecomment-110699091
@rnewson I'm +1 for abandon since 1.x worked fine with for all the cases, but don't see any reasons for introducing BC as it never was a case.
Or figure why strings comes in so different format. Personally, I don't have any ideas here.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-config pull request: 2708 stronger testing for config set
Posted by rnewson <gi...@git.apache.org>.
Github user rnewson commented on a diff in the pull request:
https://github.com/apache/couchdb-config/pull/3#discussion_r31727769
--- Diff: src/config.erl ---
@@ -177,9 +182,19 @@ delete(Section, Key, Reason) ->
delete(Sec, Key, Persist, Reason) when is_binary(Sec) and is_binary(Key) ->
delete(binary_to_list(Sec), binary_to_list(Key), Persist, Reason);
-delete(Section, Key, Persist, Reason) when is_list(Section), is_list(Key) ->
+delete(Section, Key, Persist, Reason) when is_boolean(Persist) ->
+ assert_string(Section),
+ assert_string(Key),
+ if Reason == nil -> ok; true -> assert_string(Reason) end,
gen_server:call(?MODULE, {delete, Section, Key, Persist, Reason}).
+assert_string(Term) ->
+ case io_lib:printable_list(Term) of
--- End diff --
I think non-latin would trip bugs in all those areas, isn't it safer to exclude them here?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-config pull request: 2708 stronger testing for config set
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on a diff in the pull request:
https://github.com/apache/couchdb-config/pull/3#discussion_r32050277
--- Diff: src/config.erl ---
@@ -177,9 +182,19 @@ delete(Section, Key, Reason) ->
delete(Sec, Key, Persist, Reason) when is_binary(Sec) and is_binary(Key) ->
delete(binary_to_list(Sec), binary_to_list(Key), Persist, Reason);
-delete(Section, Key, Persist, Reason) when is_list(Section), is_list(Key) ->
+delete(Section, Key, Persist, Reason) when is_boolean(Persist) ->
+ assert_string(Section),
+ assert_string(Key),
+ if Reason == nil -> ok; true -> assert_string(Reason) end,
gen_server:call(?MODULE, {delete, Section, Key, Persist, Reason}).
+assert_string(Term) ->
+ case io_lib:printable_unicode_list(Term) of
--- End diff --
Hm...I feel sorry for unicode suggestion, but confusion as well. I put console log call before the case in order to print the terms that comes here and here is what I saw:
```
>>> "медвед"
>>> "водка"
>>> "медвед"
>>> "водка"
>>> "балалайка"
2015-06-09 21:01:11.116 [debug] node1@127.0.0.1 <0.1975.0> 'PUT' /_config/медвед/водка {1,1} from "127.0.0.1"
Headers: [{'Accept',"*/*"},{'Content-Length',"20"},{'Content-Type',"application/x-www-form-urlencoded"},{'Host',"localhost:15986"},{'User-Agent',"curl/7.42.1"}]
2015-06-09 21:01:11.117 [debug] node1@127.0.0.1 <0.1975.0> OAuth Params: []
2015-06-09 21:01:11.118 [notice] node1@127.0.0.1 <0.53.0> config: [медвед] водка set to балалайка for reason nil
2015-06-09 21:01:11.121 [notice] node1@127.0.0.1 <0.1975.0> 127.0.0.1 - - PUT /_config/медвед/водка 200
>>> "httpd"
>>> "enable_cors"
>>> "fabric"
>>> "changes_duration"
>>> "rexi"
>>> "server_per_node"
>>> "rexi"
>>> "server_per_node"
>>> "rexi"
>>> "server_per_node"
>>> "httpd"
>>> "x_forwarded_host"
2015-06-09 21:01:21.176 [debug] node1@127.0.0.1 <0.229.0> 'PUT' /_config/тест/тест {1,1} from "127.0.0.1"
Headers: [{'Accept',"*/*"},{'Content-Length',"10"},{'Content-Type',"application/x-www-form-urlencoded"},{'Host',"localhost:15986"},{'User-Agent',"curl/7.42.1"}]
>>> "httpd"
>>> "max_uri_length"
2015-06-09 21:01:21.176 [debug] node1@127.0.0.1 <0.229.0> OAuth Params: []
>>> "couch_httpd_auth"
>>> "require_valid_user"
>>> "httpd"
>>> "config_whitelist"
>>> "couchdb"
>>> "max_document_size"
>>> [209,130,208,181,209,129,209,130]
2015-06-09 21:01:21.177 [error] node1@127.0.0.1 <0.229.0> Badarg error in HTTP request
2015-06-09 21:01:21.177 [info] node1@127.0.0.1 <0.229.0> Stacktrace: [{config,assert_string,1,[{file,"src/config.erl"},{line,197}]},{config,get,3,[{file,"src/config.erl"},{line,141}]},{couch_httpd_misc_handlers,handle_approved_config_req,3,[{file,"src/couch_httpd_misc_handlers.erl"},{line,283}]},{couch_httpd,handle_request_int,5,[{file,"src/couch_httpd.erl"},{line,301}]},{mochiweb_http,headers,5,[{file,"src/mochiweb_http.erl"},{line,93}]},{proc_lib,init_p_do_apply,3,[{file,"proc_lib.erl"},{line,237}]}]
>>> "httpd"
2015-06-09 21:01:21.177 [notice] node1@127.0.0.1 <0.229.0> 127.0.0.1 - - PUT /_config/тест/тест 500
>>> "enable_cors"
2015-06-09 21:01:21.177 [error] node1@127.0.0.1 <0.229.0> httpd 500 error response:
{"error":"unknown_error","reason":"badarg"}
```
So it seems that `Term` could be both a Unicode string and a list of UTF-8 codepoints depending if some specifics character in string are present. I'm not sure now what causes such behaviour, but it makes assertion fail.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-config pull request: 2708 stronger testing for config set
Posted by mikewallace1979 <gi...@git.apache.org>.
Github user mikewallace1979 commented on the pull request:
https://github.com/apache/couchdb-config/pull/3#issuecomment-111560742
Looks like we're all in agreement that, though it'd be great to get to the bottom of this, we'll close for now. The branch will still be here when it's time to revisit.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-config pull request: 2708 stronger testing for config set
Posted by mikewallace1979 <gi...@git.apache.org>.
Github user mikewallace1979 closed the pull request at:
https://github.com/apache/couchdb-config/pull/3
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-config pull request: 2708 stronger testing for config set
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on a diff in the pull request:
https://github.com/apache/couchdb-config/pull/3#discussion_r31735209
--- Diff: src/config.erl ---
@@ -177,9 +182,19 @@ delete(Section, Key, Reason) ->
delete(Sec, Key, Persist, Reason) when is_binary(Sec) and is_binary(Key) ->
delete(binary_to_list(Sec), binary_to_list(Key), Persist, Reason);
-delete(Section, Key, Persist, Reason) when is_list(Section), is_list(Key) ->
+delete(Section, Key, Persist, Reason) when is_boolean(Persist) ->
+ assert_string(Section),
+ assert_string(Key),
+ if Reason == nil -> ok; true -> assert_string(Reason) end,
gen_server:call(?MODULE, {delete, Section, Key, Persist, Reason}).
+assert_string(Term) ->
+ case io_lib:printable_list(Term) of
--- End diff --
Technically speaking, this is BC. Personally, I have to rename all my admins after this PR or tweak CouchDB start options.
Few more fun facts about this constraint:
```
$ curl -X PUT http://localhost:15986/_config/test/тест -d '"failed"'
{"error":"unknown_error","reason":"badarg"}
$ curl -X PUT http://localhost:15986/_config/test/баз -d '"yes"'
""
$ curl -X PUT http://localhost:15986/_config/test/баз -d '"passed?"'
"yes"
```
The `баз` comes to the assert function as `"баз"` or `[1073,1072,1079]` while `тест` comes as `[209,130,208,181,209,129,209,130]`.
More fun:
```
$ curl -X PUT http://localhost:15986/_config/дома/водка -d '"медвед"'
""
```
Printable range on node:
```
$ dev/remsh
Erlang/OTP 17 [erts-6.4] [source] [64-bit] [smp:4:4] [async-threads:10] [kernel-poll:false]
Eshell V6.4 (abort with ^G)
(node1@127.0.0.1)1> io:printable_range().
latin1
```
So I think this assertion not only breaks compatibility, but also doesn't works well and may cause confusion.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-config pull request: 2708 stronger testing for config set
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on a diff in the pull request:
https://github.com/apache/couchdb-config/pull/3#discussion_r31618881
--- Diff: src/config.erl ---
@@ -177,9 +182,19 @@ delete(Section, Key, Reason) ->
delete(Sec, Key, Persist, Reason) when is_binary(Sec) and is_binary(Key) ->
delete(binary_to_list(Sec), binary_to_list(Key), Persist, Reason);
-delete(Section, Key, Persist, Reason) when is_list(Section), is_list(Key) ->
+delete(Section, Key, Persist, Reason) when is_boolean(Persist) ->
+ assert_string(Section),
+ assert_string(Key),
+ if Reason == nil -> ok; true -> assert_string(Reason) end,
gen_server:call(?MODULE, {delete, Section, Key, Persist, Reason}).
+assert_string(Term) ->
+ case io_lib:printable_list(Term) of
--- End diff --
This will cause a problem with non-latin1 configuration keys/values. By default, printable range is set to latin1 unless it changed via `+pc` option. The source of problems could be: admins login and passwords, secrets, oauth credentials, externals and query servers paths and even vendor name.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-config pull request: 2708 stronger testing for config set
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on a diff in the pull request:
https://github.com/apache/couchdb-config/pull/3#discussion_r31729178
--- Diff: src/config.erl ---
@@ -177,9 +182,19 @@ delete(Section, Key, Reason) ->
delete(Sec, Key, Persist, Reason) when is_binary(Sec) and is_binary(Key) ->
delete(binary_to_list(Sec), binary_to_list(Key), Persist, Reason);
-delete(Section, Key, Persist, Reason) when is_list(Section), is_list(Key) ->
+delete(Section, Key, Persist, Reason) when is_boolean(Persist) ->
+ assert_string(Section),
+ assert_string(Key),
+ if Reason == nil -> ok; true -> assert_string(Reason) end,
gen_server:call(?MODULE, {delete, Section, Key, Persist, Reason}).
+assert_string(Term) ->
+ case io_lib:printable_list(Term) of
--- End diff --
They didn't caused any bugs in 1.x, why should they eventually do that in 2.0?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-config pull request: 2708 stronger testing for config set
Posted by rnewson <gi...@git.apache.org>.
Github user rnewson commented on the pull request:
https://github.com/apache/couchdb-config/pull/3#issuecomment-110678055
I'd rather we didn't waste time enhancing this to work for unicode, which leaves two choices;
1) abandon this work
2) restore the original string check (that requires latin1)
We could come back in a future release (after 2.0) to officially support unicode in configuration. Since admin names could be non-latin1 (by chance, not design, it seems), we'll have to note it as a breaking change if we go with option 2.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-config pull request: 2708 stronger testing for config set
Posted by rnewson <gi...@git.apache.org>.
Github user rnewson commented on a diff in the pull request:
https://github.com/apache/couchdb-config/pull/3#discussion_r31925176
--- Diff: src/config.erl ---
@@ -177,9 +182,19 @@ delete(Section, Key, Reason) ->
delete(Sec, Key, Persist, Reason) when is_binary(Sec) and is_binary(Key) ->
delete(binary_to_list(Sec), binary_to_list(Key), Persist, Reason);
-delete(Section, Key, Persist, Reason) when is_list(Section), is_list(Key) ->
+delete(Section, Key, Persist, Reason) when is_boolean(Persist) ->
+ assert_string(Section),
+ assert_string(Key),
+ if Reason == nil -> ok; true -> assert_string(Reason) end,
gen_server:call(?MODULE, {delete, Section, Key, Persist, Reason}).
+assert_string(Term) ->
+ case io_lib:printable_list(Term) of
--- End diff --
@mikewallace1979 ok, let's change this to printable_unicode_list then (which does exist in R14B01).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---