Merge pull request #1754 from cloudant/do-not-use-list-as-config-key

Do not use [] in feature_flags configuration

2 files changed, 67 insertions, 33 deletions

diff --git a/src/couch/src/couch_flags_config.erl b/src/couch/src/couch_flags_config.erl
index ad45add31..513a8154f 100644
--- a/src/couch/src/couch_flags_config.erl
+++ b/src/couch/src/couch_flags_config.erl
@@ -21,6 +21,7 @@
 ]).
 
 -define(DATA_INTERVAL, 1000).
+-define(MAX_FLAG_NAME_LENGTH, 256).
 
 -type pattern()
     :: binary(). %% non empty binary which optionally can end with *
@@ -120,14 +121,32 @@ parse_flags(_Tokens, _) ->
     [flag_id()] | {error, Reason :: term()}.
 
 parse_flags_term(FlagsBin) ->
-    case couch_util:parse_term(FlagsBin) of
-        {ok, Flags} when is_list(Flags) ->
-            lists:usort(Flags);
-        Term ->
-            {error, {
-                "Flags should be list of atoms (got \"~s\"): ~p",
-                [FlagsBin, Term]
-            }}
+    {Flags, Errors} = lists:splitwith(fun erlang:is_atom/1,
+        [parse_flag(F) || F <- split_by_comma(FlagsBin)]),
+    case Errors of
+       [] ->
+           lists:usort(Flags);
+       _ ->
+           {error, {
+               "Cannot parse list of tags: ~n~p",
+               Errors
+           }}
+    end.
+
+split_by_comma(Binary) ->
+    case binary:split(Binary, <<",">>, [global]) of
+       [<<>>] -> [];
+       Tokens -> Tokens
+    end.
+
+parse_flag(FlagName) when size(FlagName) > ?MAX_FLAG_NAME_LENGTH ->
+    {too_long, FlagName};
+parse_flag(FlagName) ->
+    FlagNameS = string:strip(binary_to_list(FlagName)),
+    try
+       list_to_existing_atom(FlagNameS)
+    catch
+        _:_ -> {invalid_flag, FlagName}
     end.
 
 -spec parse_pattern(Pattern :: binary()) -> parse_pattern().
@@ -272,12 +291,12 @@ get_config_section(Section) ->
 
 all_combinations_return_same_result_test_() ->
     Config = [
-         {"[foo, bar]||*", "true"},
-         {"[baz, qux]||*", "false"},
-         {"[baz]||shards/test*", "true"},
-         {"[baz]||shards/blacklist*", "false"},
-         {"[bar]||shards/test*", "false"},
-         {"[bar]||shards/test/blacklist*", "true"}
+         {"foo, bar||*", "true"},
+         {"baz, qux||*", "false"},
+         {"baz||shards/test*", "true"},
+         {"baz||shards/blacklist*", "false"},
+         {"bar||shards/test*", "false"},
+         {"bar||shards/test/blacklist*", "true"}
     ],
     Expected = [
         {{<<"shards/test/blacklist*">>},{<<"shards/test/blacklist*">>,22,[bar, foo]}},
@@ -303,12 +322,12 @@ rules_are_sorted_test() ->
 latest_overide_wins_test_() ->
     Cases = [
         {[
-            {"[flag]||*", "false"}, {"[flag]||a*", "true"},
-            {"[flag]||ab*", "true"}, {"[flag]||abc*", "true"}
+            {"flag||*", "false"}, {"flag||a*", "true"},
+            {"flag||ab*", "true"}, {"flag||abc*", "true"}
         ], true},
         {[
-            {"[flag]||*", "true"}, {"[flag]||a*", "false"},
-            {"[flag]||ab*", "true"}, {"[flag]||abc*", "false"}
+            {"flag||*", "true"}, {"flag||a*", "false"},
+            {"flag||ab*", "true"}, {"flag||abc*", "false"}
         ], false}
     ],
     [{test_id(Rules, Expected),
@@ -327,14 +346,29 @@ test_id(Items) ->
 
 test_config() ->
     [
-        {"[flag_foo]||*", "true"},
-        {"[flag_bar]||*", "false"},
-        {"[flag_bar]||shards/test*", "true"},
-        {"[flag_foo]||shards/blacklist*", "false"},
-        {"[baz]||shards/test*", "true"},
-        {"[baz]||shards/test/blacklist*", "false"},
-        {"[flag_bar]||shards/exact", "true"},
-        {"[flag_bar]||shards/test/exact", "true"}
+        {"flag_foo||*", "true"},
+        {"flag_bar||*", "false"},
+        {"flag_bar||shards/test*", "true"},
+        {"flag_foo||shards/blacklist*", "false"},
+        {"baz||shards/test*", "true"},
+        {"baz||shards/test/blacklist*", "false"},
+        {"flag_bar||shards/exact", "true"},
+        {"flag_bar||shards/test/exact", "true"}
     ].
 
+parse_flags_term_test_() ->
+    LongBinary = binary:copy(<<"a">>, ?MAX_FLAG_NAME_LENGTH + 1),
+    ExpectedError = {error, {"Cannot parse list of tags: ~n~p",
+       [{too_long, LongBinary}]}},
+    ExpectedUnknownError = {error,{"Cannot parse list of tags: ~n~p",
+       [{invalid_flag,<<"dddddddd">>}]}},
+	[
+		{"empty binary", ?_assertEqual([], parse_flags_term(<<>>))},
+		{"single flag", ?_assertEqual([fff], parse_flags_term(<<"fff">>))},
+		{"sorted", ?_assertEqual([aaa,bbb,fff], parse_flags_term(<<"fff,aaa,bbb">>))},
+		{"whitespace", ?_assertEqual([aaa,bbb,fff], parse_flags_term(<<"fff , aaa, bbb ">>))},
+		{"error", ?_assertEqual(ExpectedError, parse_flags_term(LongBinary))},
+		{"unknown_flag", ?_assertEqual(ExpectedUnknownError, parse_flags_term(<<"dddddddd">>))}
+	].
+
 -endif.
diff --git a/src/couch/test/couch_flags_tests.erl b/src/couch/test/couch_flags_tests.erl
index f8f27c7c0..a467265cb 100644
--- a/src/couch/test/couch_flags_tests.erl
+++ b/src/couch/test/couch_flags_tests.erl
@@ -136,11 +136,11 @@ match_performance() ->
 
 test_config() ->
     [
-        {"[foo]||/*", "true"},
-        {"[bar]||/*", "false"},
-        {"[bar]||/shards/test*", "true"},
-        {"[foo]||/shards/blacklist*", "false"},
-        {"[baz]||/shards/test*", "true"},
-        {"[bar]||/shards/exact", "true"},
-        {"[bar]||/shards/test/exact", "true"}
+        {"foo||/*", "true"},
+        {"bar||/*", "false"},
+        {"bar||/shards/test*", "true"},
+        {"foo||/shards/blacklist*", "false"},
+        {"baz||/shards/test*", "true"},
+        {"bar||/shards/exact", "true"},
+        {"bar||/shards/test/exact", "true"}
     ].