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 2022/12/03 16:58:02 UTC
[GitHub] [couchdb] nickva opened a new pull request, #4287: Allow = in config key values
nickva opened a new pull request, #4287:
URL: https://github.com/apache/couchdb/pull/4287
There are allowed only for the `k = v` form and not `k=v`. The idea is to split on ` = ` first, and if that fails to produce a valid kv pair we split on `=` as before.
To implement, simplify the parsing logic and remove the undocumented multi-line config value feature. The continuation syntax is not documented anywhere and not used by our default.ini or in documentation.
Also added 100% test coverage to the parser.
Fix: https://github.com/apache/couchdb/issues/3319
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on a diff in pull request #4287: Allow = in config key names
Posted by GitBox <gi...@apache.org>.
nickva commented on code in PR #4287:
URL: https://github.com/apache/couchdb/pull/4287#discussion_r1038896038
##########
src/config/src/config.erl:
##########
@@ -414,89 +415,73 @@ is_sensitive(Section, Key) ->
end.
parse_ini_file(IniFile) ->
+ IniBin = read_ini_file(IniFile),
+ ParsedIniValues = parse_ini(IniBin),
+ {ok, lists:filter(fun delete_keys/1, ParsedIniValues)}.
+
+parse_ini(IniBin) when is_binary(IniBin) ->
+ Lines0 = re:split(IniBin, "\r\n|\n|\r|\032", [{return, list}]),
+ Lines1 = lists:map(fun remove_comments/1, Lines0),
+ Lines2 = lists:map(fun trim/1, Lines1),
+ Lines3 = lists:filter(fun(Line) -> Line =/= "" end, Lines2),
+ {_, IniValues} = lists:foldl(fun parse_fold/2, {"", []}, Lines3),
+ lists:reverse(IniValues).
+
+parse_fold("[" ++ Rest, {Section, KVs}) ->
+ % Check for end ] brackend, if not found or empty section skip the rest
+ case string:split(Rest, "]") of
+ ["", _] -> {Section, KVs};
+ [NewSection, ""] -> {NewSection, KVs};
+ _Else -> {Section, KVs}
+ end;
+parse_fold(_Line, {"" = Section, KVs}) ->
+ % Empty section don't parse any lines until we're in a section
+ {Section, KVs};
+parse_fold(Line, {Section, KVs}) ->
+ case string:split(Line, " = ") of
Review Comment:
Good idea! Added mentions to docs and default ini jwt_keys section.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on pull request #4287: Allow = in config key names
Posted by GitBox <gi...@apache.org>.
nickva commented on PR #4287:
URL: https://github.com/apache/couchdb/pull/4287#issuecomment-1336334575
> Just a couple nits: First, the title of the PR "Allow = in config key values" makes it sound like we're allowing the `=` character in the _values_, but I think you mean "key names"?
>
Good catch. Yeah that is ambiguous, I meant "key names".
> > There are allowed only for the `k = v` form and not `k=v`.
>
> Do you mean "They are allowed ..."?
>
Yup! Silly mistake. Thanks for noticing!
> > The idea is to split on `=` first, and if that fails to produce a valid kv pair we split on `=` as before.
>
> Should the first one contain spaces on either side of the equal sign, like `=`?
>
I had it in the commit comment but then tried to make prettier in the PR comment messed it up instead. I'll return back to using quotes.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on a diff in pull request #4287: Allow = in config key names
Posted by GitBox <gi...@apache.org>.
nickva commented on code in PR #4287:
URL: https://github.com/apache/couchdb/pull/4287#discussion_r1038896038
##########
src/config/src/config.erl:
##########
@@ -414,89 +415,73 @@ is_sensitive(Section, Key) ->
end.
parse_ini_file(IniFile) ->
+ IniBin = read_ini_file(IniFile),
+ ParsedIniValues = parse_ini(IniBin),
+ {ok, lists:filter(fun delete_keys/1, ParsedIniValues)}.
+
+parse_ini(IniBin) when is_binary(IniBin) ->
+ Lines0 = re:split(IniBin, "\r\n|\n|\r|\032", [{return, list}]),
+ Lines1 = lists:map(fun remove_comments/1, Lines0),
+ Lines2 = lists:map(fun trim/1, Lines1),
+ Lines3 = lists:filter(fun(Line) -> Line =/= "" end, Lines2),
+ {_, IniValues} = lists:foldl(fun parse_fold/2, {"", []}, Lines3),
+ lists:reverse(IniValues).
+
+parse_fold("[" ++ Rest, {Section, KVs}) ->
+ % Check for end ] brackend, if not found or empty section skip the rest
+ case string:split(Rest, "]") of
+ ["", _] -> {Section, KVs};
+ [NewSection, ""] -> {NewSection, KVs};
+ _Else -> {Section, KVs}
+ end;
+parse_fold(_Line, {"" = Section, KVs}) ->
+ % Empty section don't parse any lines until we're in a section
+ {Section, KVs};
+parse_fold(Line, {Section, KVs}) ->
+ case string:split(Line, " = ") of
Review Comment:
Good idea!
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] jaydoane commented on a diff in pull request #4287: Allow = in config key values
Posted by GitBox <gi...@apache.org>.
jaydoane commented on code in PR #4287:
URL: https://github.com/apache/couchdb/pull/4287#discussion_r1038871687
##########
src/config/src/config.erl:
##########
@@ -414,89 +415,73 @@ is_sensitive(Section, Key) ->
end.
parse_ini_file(IniFile) ->
+ IniBin = read_ini_file(IniFile),
+ ParsedIniValues = parse_ini(IniBin),
+ {ok, lists:filter(fun delete_keys/1, ParsedIniValues)}.
+
+parse_ini(IniBin) when is_binary(IniBin) ->
+ Lines0 = re:split(IniBin, "\r\n|\n|\r|\032", [{return, list}]),
+ Lines1 = lists:map(fun remove_comments/1, Lines0),
+ Lines2 = lists:map(fun trim/1, Lines1),
+ Lines3 = lists:filter(fun(Line) -> Line =/= "" end, Lines2),
+ {_, IniValues} = lists:foldl(fun parse_fold/2, {"", []}, Lines3),
+ lists:reverse(IniValues).
+
+parse_fold("[" ++ Rest, {Section, KVs}) ->
+ % Check for end ] brackend, if not found or empty section skip the rest
+ case string:split(Rest, "]") of
+ ["", _] -> {Section, KVs};
+ [NewSection, ""] -> {NewSection, KVs};
+ _Else -> {Section, KVs}
+ end;
+parse_fold(_Line, {"" = Section, KVs}) ->
+ % Empty section don't parse any lines until we're in a section
+ {Section, KVs};
+parse_fold(Line, {Section, KVs}) ->
+ case string:split(Line, " = ") of
+ [K, V] when V =/= "" ->
+ % Key may have an "=" in it. Also, assert we'll never have a
+ % deletion case here since we're working with a stripped line
+ {Section, [{{Section, trim(K)}, trim(V)} | KVs]};
+ [Line] ->
Review Comment:
I'm seeing "Bound variable in pattern: Line" here and line 454, so maybe want to use different names?
##########
src/config/src/config.erl:
##########
@@ -414,89 +415,73 @@ is_sensitive(Section, Key) ->
end.
parse_ini_file(IniFile) ->
+ IniBin = read_ini_file(IniFile),
+ ParsedIniValues = parse_ini(IniBin),
+ {ok, lists:filter(fun delete_keys/1, ParsedIniValues)}.
+
+parse_ini(IniBin) when is_binary(IniBin) ->
+ Lines0 = re:split(IniBin, "\r\n|\n|\r|\032", [{return, list}]),
+ Lines1 = lists:map(fun remove_comments/1, Lines0),
+ Lines2 = lists:map(fun trim/1, Lines1),
+ Lines3 = lists:filter(fun(Line) -> Line =/= "" end, Lines2),
+ {_, IniValues} = lists:foldl(fun parse_fold/2, {"", []}, Lines3),
+ lists:reverse(IniValues).
+
+parse_fold("[" ++ Rest, {Section, KVs}) ->
+ % Check for end ] brackend, if not found or empty section skip the rest
+ case string:split(Rest, "]") of
+ ["", _] -> {Section, KVs};
+ [NewSection, ""] -> {NewSection, KVs};
+ _Else -> {Section, KVs}
+ end;
+parse_fold(_Line, {"" = Section, KVs}) ->
+ % Empty section don't parse any lines until we're in a section
+ {Section, KVs};
+parse_fold(Line, {Section, KVs}) ->
+ case string:split(Line, " = ") of
Review Comment:
Should we add documentation that describes this algorithm?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on pull request #4287: Allow = in config key names
Posted by GitBox <gi...@apache.org>.
nickva commented on PR #4287:
URL: https://github.com/apache/couchdb/pull/4287#issuecomment-1336335490
Updated and added docs for it.
When adding docs a typo (`[jwt_key]` should be `[jwt_keys]`) so fixed that along the way as well.
In addition manually tested that it's possible to set and get the value via the `_config/` API:
```
% curl -XPUT -H'Content-Type:application/json' $DB/_node/_local/_config/s1/k1 -d '"v1"'
% curl -XPUT -H'Content-Type:application/json' $DB/_node/_local/_config/s1/k=1 -d '"v1"'
% curl -XPUT -H'Content-Type:application/json' $DB/_node/_local/_config/s1/k=== -d '"v1"'
% curl -s $DB/_node/_local/_config/s1 | jq '.'
{
"k1": "v1",
"k=1": "v1",
"k===": "v1"
}
% cat dev/lib/node1/etc/local.ini
...
[s1]
k1 = v1
k=1 = v1
k=== = v1
```
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] jaydoane commented on pull request #4287: Allow = in config key values
Posted by GitBox <gi...@apache.org>.
jaydoane commented on PR #4287:
URL: https://github.com/apache/couchdb/pull/4287#issuecomment-1336273945
Just a couple nits: First, the title of the PR "Allow = in config key values" makes it sound like we're allowing the `=` character in the _values_, but I think you mean "key names"?
> There are allowed only for the `k = v` form and not `k=v`.
Do you mean "They are allowed ..."?
> The idea is to split on `=` first, and if that fails to produce a valid kv pair we split on `=` as before.
Should the first one contain spaces on either side of the equal sign, like ` = `?
> Also added 100% test coverage to the parser.
💯 awesome
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on a diff in pull request #4287: Allow = in config key names
Posted by GitBox <gi...@apache.org>.
nickva commented on code in PR #4287:
URL: https://github.com/apache/couchdb/pull/4287#discussion_r1038896075
##########
src/config/src/config.erl:
##########
@@ -414,89 +415,73 @@ is_sensitive(Section, Key) ->
end.
parse_ini_file(IniFile) ->
+ IniBin = read_ini_file(IniFile),
+ ParsedIniValues = parse_ini(IniBin),
+ {ok, lists:filter(fun delete_keys/1, ParsedIniValues)}.
+
+parse_ini(IniBin) when is_binary(IniBin) ->
+ Lines0 = re:split(IniBin, "\r\n|\n|\r|\032", [{return, list}]),
+ Lines1 = lists:map(fun remove_comments/1, Lines0),
+ Lines2 = lists:map(fun trim/1, Lines1),
+ Lines3 = lists:filter(fun(Line) -> Line =/= "" end, Lines2),
+ {_, IniValues} = lists:foldl(fun parse_fold/2, {"", []}, Lines3),
+ lists:reverse(IniValues).
+
+parse_fold("[" ++ Rest, {Section, KVs}) ->
+ % Check for end ] brackend, if not found or empty section skip the rest
+ case string:split(Rest, "]") of
+ ["", _] -> {Section, KVs};
+ [NewSection, ""] -> {NewSection, KVs};
+ _Else -> {Section, KVs}
+ end;
+parse_fold(_Line, {"" = Section, KVs}) ->
+ % Empty section don't parse any lines until we're in a section
+ {Section, KVs};
+parse_fold(Line, {Section, KVs}) ->
+ case string:split(Line, " = ") of
+ [K, V] when V =/= "" ->
+ % Key may have an "=" in it. Also, assert we'll never have a
+ % deletion case here since we're working with a stripped line
+ {Section, [{{Section, trim(K)}, trim(V)} | KVs]};
+ [Line] ->
Review Comment:
It's intentional to assert that we got the same Line as a single value, meaning the split didn't work. But to avoid the extra warning can match on `[_]` as well there.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva merged pull request #4287: Allow = in config key names
Posted by GitBox <gi...@apache.org>.
nickva merged PR #4287:
URL: https://github.com/apache/couchdb/pull/4287
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org