You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@couchdb.apache.org by iilyak <gi...@git.apache.org> on 2015/02/25 17:43:09 UTC
[GitHub] couchdb-couch pull request: 2625 dbname validator
GitHub user iilyak opened a pull request:
https://github.com/apache/couchdb-couch/pull/38
2625 dbname validator
This depends on https://github.com/apache/couchdb-couch/pull/37
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/iilyak/couchdb-couch 2625-dbname_validator
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/couchdb-couch/pull/38.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 #38
----
commit 0e01a33cc6443f0690cc5f2460c0240b64eda335
Author: ILYA Khlopotov <ii...@ca.ibm.com>
Date: 2015-02-25T16:28:08Z
Return {error, Reason} for illegal_database_name
COUCHDB-2625
commit 33c0265b3d9f6efd23b9c5a8fdbf331c9871a511
Author: ILYA Khlopotov <ii...@ca.ibm.com>
Date: 2015-02-25T16:30:45Z
Implement custom dbname_validator
COUCHDB-2625
----
---
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-couch pull request: 2625 dbname validator
Posted by janl <gi...@git.apache.org>.
Github user janl commented on the pull request:
https://github.com/apache/couchdb-couch/pull/38#issuecomment-76022970
@iilyak for what reason? :) (sorry for being dense)
---
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-couch pull request: 2625 dbname validator
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on a diff in the pull request:
https://github.com/apache/couchdb-couch/pull/38#discussion_r25356432
--- Diff: src/couch_httpd.erl ---
@@ -837,7 +837,7 @@ error_info({bad_ctype, Reason}) ->
{415, <<"bad_content_type">>, Reason};
error_info(requested_range_not_satisfiable) ->
{416, <<"requested_range_not_satisfiable">>, <<"Requested range not satisfiable">>};
-error_info({error, illegal_database_name, Name}) ->
+error_info({error, {illegal_database_name, Name}}) ->
Message = "Name: '" ++ Name ++ "'. Only lowercase characters (a-z), "
--- End diff --
In case of db name validation customization this error message shouldn't be defined here since the requirements for the name could be different.
---
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-couch pull request: 2625 dbname validator
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on a diff in the pull request:
https://github.com/apache/couchdb-couch/pull/38#discussion_r25359432
--- Diff: src/couch_httpd.erl ---
@@ -837,7 +837,7 @@ error_info({bad_ctype, Reason}) ->
{415, <<"bad_content_type">>, Reason};
error_info(requested_range_not_satisfiable) ->
{416, <<"requested_range_not_satisfiable">>, <<"Requested range not satisfiable">>};
-error_info({error, illegal_database_name, Name}) ->
+error_info({error, {illegal_database_name, Name}}) ->
Message = "Name: '" ++ Name ++ "'. Only lowercase characters (a-z), "
--- End diff --
In case if this wouldn't break anything, ok.
---
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-couch pull request: 2625 dbname validator
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on the pull request:
https://github.com/apache/couchdb-couch/pull/38#issuecomment-76023888
hm... @iilyak I have a PR in stash that extends this list too by three more dbs (_nodes, _dbs, _metadata). But I ended with SYSTEM_DBS macro for now.
However, the most simple way to maintain this list is using ini file. On other hand, these databases are special and not sure if it's ok when user have a power to add more than we actually need.
---
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-couch pull request: 2625 dbname validator
Posted by janl <gi...@git.apache.org>.
Github user janl commented on the pull request:
https://github.com/apache/couchdb-couch/pull/38#issuecomment-76020612
I’d like to understand the underlying issue this is trying to solve, first :)
---
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-couch pull request: 2625 dbname validator
Posted by iilyak <gi...@git.apache.org>.
Github user iilyak closed the pull request at:
https://github.com/apache/couchdb-couch/pull/38
---
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-couch pull request: 2625 dbname validator
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on the pull request:
https://github.com/apache/couchdb-couch/pull/38#issuecomment-76012517
Fixing code duplicity with fabric is good, but do we actually need in check_dbname customization?
Say, I want to have Cyrillic database names and I put my validation function into the config file. Will the other couch parts be ready to work with such names? How chttpd/httpd will map urlencoded utf-8 database name with a file name if filesystem uses different charset, like utf-16 on windows? I have a strong fear that this will cause a server failure. And in this case why do I have to change the default database name validation?
---
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-couch pull request: 2625 dbname validator
Posted by iilyak <gi...@git.apache.org>.
Github user iilyak commented on the pull request:
https://github.com/apache/couchdb-couch/pull/38#issuecomment-76058067
Yes this would work but it makes your PR and my PR obsolete. Since it is a late night for you @kxepal I can implement it. Closing this PR for that reason.
---
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-couch pull request: 2625 dbname validator
Posted by iilyak <gi...@git.apache.org>.
Github user iilyak commented on a diff in the pull request:
https://github.com/apache/couchdb-couch/pull/38#discussion_r25358295
--- Diff: src/couch_httpd.erl ---
@@ -837,7 +837,7 @@ error_info({bad_ctype, Reason}) ->
{415, <<"bad_content_type">>, Reason};
error_info(requested_range_not_satisfiable) ->
{416, <<"requested_range_not_satisfiable">>, <<"Requested range not satisfiable">>};
-error_info({error, illegal_database_name, Name}) ->
+error_info({error, {illegal_database_name, Name}}) ->
Message = "Name: '" ++ Name ++ "'. Only lowercase characters (a-z), "
--- End diff --
We could change error to be `{error, {illegal_database_name, Name, ErrorMessage}}`
---
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-couch pull request: 2625 dbname validator
Posted by iilyak <gi...@git.apache.org>.
Github user iilyak commented on the pull request:
https://github.com/apache/couchdb-couch/pull/38#issuecomment-76047693
We have an existent dbs in the underscore namespace in our fork.
---
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-couch pull request: 2625 dbname validator
Posted by iilyak <gi...@git.apache.org>.
Github user iilyak commented on the pull request:
https://github.com/apache/couchdb-couch/pull/38#issuecomment-76053405
@kxepal Following could solve the problem [based on](https://github.com/apache/couchdb-couch/pull/39/files)
```erlang
Whitelist = config:get("couchdb", "extra_system_dbs", []),
case lists:member(DbName, ?SYSTEM_DATABASES ++ Whitelist) of
```
---
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-couch pull request: 2625 dbname validator
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on a diff in the pull request:
https://github.com/apache/couchdb-couch/pull/38#discussion_r25356693
--- Diff: src/couch_server.erl ---
@@ -145,6 +150,8 @@ maybe_add_sys_db_callbacks(DbName, Options) ->
path_ends_with(Path, Suffix) ->
Suffix == lists:last(binary:split(mem3:dbname(Path), <<"/">>, [global])).
+check_dbname(#server{dbname_validator={M, F, A}}, DbName) ->
+ M:F(DbName, A);
check_dbname(#server{dbname_regexp=RegExp}, DbName) ->
--- End diff --
Why do we have to maintain two different ways to apply dbname validation logic (MFA vs regex)? May be drop dbname_regexp and replace it with the default MFA which points to the original check_dbname? Would be a bit more consistent.
---
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-couch pull request: 2625 dbname validator
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on the pull request:
https://github.com/apache/couchdb-couch/pull/38#issuecomment-76051067
@iilyak so may be instead we make this list of system database names configurable while not expose ability to override default database name constraints? Sound as more solid approach.
---
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-couch pull request: 2625 dbname validator
Posted by iilyak <gi...@git.apache.org>.
Github user iilyak commented on the pull request:
https://github.com/apache/couchdb-couch/pull/38#issuecomment-76035782
The reason to have it configurable is to allow vendor specific customization.
---
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-couch pull request: 2625 dbname validator
Posted by janl <gi...@git.apache.org>.
Github user janl commented on the pull request:
https://github.com/apache/couchdb-couch/pull/38#issuecomment-76019644
I’m with @kxepal: sceptical about the usefulness of letting the end user define how to validate database names.
I’m not sure what problem this solves. The referenced issue in #37 (https://issues.apache.org/jira/browse/COUCHDB-2585) is about something else.
---
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-couch pull request: 2625 dbname validator
Posted by janl <gi...@git.apache.org>.
Github user janl commented on the pull request:
https://github.com/apache/couchdb-couch/pull/38#issuecomment-76043327
@iilyak why would you need a different set of db names? what are you trying to do there?
---
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-couch pull request: 2625 dbname validator
Posted by iilyak <gi...@git.apache.org>.
Github user iilyak commented on the pull request:
https://github.com/apache/couchdb-couch/pull/38#issuecomment-76020455
Would you accept a PR with configurable whitelist of DBs which would be consulted in couch_server:checkdb_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-couch pull request: 2625 dbname validator
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on the pull request:
https://github.com/apache/couchdb-couch/pull/38#issuecomment-76055859
How do you feel about such solution?
---
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-couch pull request: 2625 dbname validator
Posted by janl <gi...@git.apache.org>.
Github user janl commented on the pull request:
https://github.com/apache/couchdb-couch/pull/38#issuecomment-76021117
I found the related ticket: https://issues.apache.org/jira/browse/COUCHDB-2625 — It doesn’t answer my question though
---
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-couch pull request: 2625 dbname validator
Posted by iilyak <gi...@git.apache.org>.
Github user iilyak commented on the pull request:
https://github.com/apache/couchdb-couch/pull/38#issuecomment-76021769
I need to extend this list https://github.com/apache/couchdb-couch/blob/master/src/couch_server.erl#L152:L153 without modifying couchdb source code
---
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-couch pull request: 2625 dbname validator
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on the pull request:
https://github.com/apache/couchdb-couch/pull/38#issuecomment-76055790
mmm...yes, that certainly would works, but in long term personally I think about some some of dynamic registration of system databases: when couch_replicator loads, it registers _replicator database. When cassim app loads, it registers _metadata one and so on. Why? Because system databases plays some special role for CouchDB and users shouldn't register here their own to prevent possible collisions while these system databases are managed by some application which applies certain logic upon the data they holds.
Speaking about your case, the solution would be trivial: make a plugin which registers more system databases and here you go.
---
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.
---