diff --git a/include/clique_specs.hrl b/include/clique_specs.hrl new file mode 100644 index 0000000..709556d --- /dev/null +++ b/include/clique_specs.hrl @@ -0,0 +1,13 @@ +%% This record represents the specification for a key-value argument +%% or flag on the command line. +-record(clique_spec, + { + key :: atom(), + name :: string(), + shortname :: char() | undefined, + datatype :: cuttlefish_datatypes:datatype() | undefined, + validator :: fun((term()) -> ok | err()) | undefined, + typecast :: fun((string()) -> err() | term()) | undefined + }). + +-type spec() :: #clique_spec{}. diff --git a/src/clique_command.erl b/src/clique_command.erl index 48d5a54..79f9875 100644 --- a/src/clique_command.erl +++ b/src/clique_command.erl @@ -18,6 +18,7 @@ %% %% ------------------------------------------------------------------- -module(clique_command). +-include("clique_specs.hrl"). -define(cmd_table, clique_commands). @@ -40,7 +41,9 @@ init() -> %% @doc Register a cli command (i.e.: "riak-admin handoff status") -spec register([string()], list(), list(), fun()) -> true. -register(Cmd, Keys, Flags, Fun) -> +register(Cmd, Keys0, Flags0, Fun) -> + Keys = make_specs(Keys0), + Flags = make_specs(Flags0), ets:insert(?cmd_table, {Cmd, Keys, Flags, Fun}). -spec run(err()) -> err(); @@ -86,6 +89,11 @@ split_command(Cmd0) -> clique_parser:is_not_flag(Str) end, Cmd0). + +-spec make_specs([{atom(), proplist()}]) -> [spec()]. +make_specs(Specs) -> + [ clique_spec:make(Spec) || Spec <- Specs ]. + %% NB This is a bit sneaky. We normally only accept key/value args like %% "handoff.inbound=off" and flag-style arguments like "--node dev1@127.0.0.1" or "--all", %% but the builtin "show" and "describe" commands work a bit differently. diff --git a/src/clique_config.erl b/src/clique_config.erl index abf9ead..06fbb07 100644 --- a/src/clique_config.erl +++ b/src/clique_config.erl @@ -18,6 +18,7 @@ %% %% ------------------------------------------------------------------- -module(clique_config). +-include("clique_specs.hrl"). %% API -export([init/0, @@ -43,7 +44,9 @@ -type err() :: {error, term()}. -type status() :: clique_status:status(). -type proplist() :: [{atom(), term()}]. --type flags() :: [{atom() | char(), term()}]. +-type flagspecs() :: [spec()]. +-type flags() :: proplist(). +-type args() :: clique_parser:args(). -type envkey() :: {string(), {atom(), atom()}}. -type cuttlefish_flag_spec() :: {flag, atom(), atom()}. @@ -238,7 +241,7 @@ run_callback({Args, Flags, Status}) -> Status. -spec get_config(err()) -> err(); - ({proplist(), flags()}) -> + ({args(), flags()}) -> {proplist(), proplist(), flags()} | err(). get_config({error, _}=E) -> E; @@ -246,7 +249,7 @@ get_config({[], _Flags}) -> {error, set_no_args}; get_config({Args, Flags}) -> [{schema, Schema}] = ets:lookup(?schema_table, schema), - Conf = [{cuttlefish_variable:tokenize(atom_to_list(K)), V} || {K, V} <- Args], + Conf = [{cuttlefish_variable:tokenize(K), V} || {K, V} <- Args], case cuttlefish_generator:minimal_map(Schema, Conf) of {error, _, Msg} -> {error, {invalid_config, Msg}}; @@ -326,18 +329,18 @@ set_remote_app_config(AppConfig) -> [clique_status:alert([clique_status:text(Alert)])] end. --spec config_flags() -> flags(). +-spec config_flags() -> flagspecs(). config_flags() -> - [{node, [{shortname, "n"}, - {longname, "node"}, - {typecast, fun clique_typecast:to_node/1}, - {description, - "The node to apply the operation on"}]}, - - {all, [{shortname, "a"}, - {longname, "all"}, - {description, - "Apply the operation to all nodes in the cluster"}]}]. + [clique_spec:make({node, [{shortname, "n"}, + {longname, "node"}, + {typecast, fun clique_typecast:to_node/1}, + {description, + "The node to apply the operation on"}]}), + + clique_spec:make({all, [{shortname, "a"}, + {longname, "all"}, + {description, + "Apply the operation to all nodes in the cluster"}]})]. -spec get_valid_mappings([string()]) -> err() | [{string(), cuttlefish_mapping:mapping()}]. diff --git a/src/clique_error.erl b/src/clique_error.erl index 941f418..ebb82a0 100644 --- a/src/clique_error.erl +++ b/src/clique_error.erl @@ -81,7 +81,11 @@ format(_Cmd, {error, {config_not_settable, Keys}}) -> format(_Cmd, {error, {nodedown, Node}}) -> status(io_lib:format("Target node is down: ~p~n", [Node])); format(_Cmd, {error, bad_node}) -> - status("Invalid node name"). + status("Invalid node name"); +format(_Cmd, {error, {conversion, _}}=TypeError) -> + %% Type-conversion error originating in cuttlefish + status(cuttlefish_error:xlate(TypeError)). + -spec status(string()) -> status(). status(Str) -> diff --git a/src/clique_parser.erl b/src/clique_parser.erl index 9931ab9..12c60b6 100644 --- a/src/clique_parser.erl +++ b/src/clique_parser.erl @@ -18,7 +18,11 @@ %% %% ------------------------------------------------------------------- -module(clique_parser). +-include("clique_specs.hrl"). +-ifdef(TEST). +-include_lib("eunit/include/eunit.hrl"). +-endif. %% API -export([parse/1, parse_flags/1, @@ -28,19 +32,20 @@ is_not_kv_arg/1, is_not_flag/1]). +-export_type([flags/0, args/0]). + -type err() :: {error, term()}. --type flags() :: [{atom() | char(), term()}]. +-type flags() :: [{string() | char(), term()}]. +-type args() :: [{string(), string()}]. -type proplist() :: [{atom(), term()}]. -%% TODO: A spec should probably just be a record --type spec() :: {atom(), proplist()}. -type keyspecs() :: '_' | [spec()]. -type flagspecs() :: [spec()]. -spec parse(err()) -> err(); - ([string()]) -> {proplist(), flags()} | err(); + ([string()]) -> {args(), flags()} | err(); ({tuple(), [string()]}) -> - {tuple(), proplist(), flags()} | err(). + {tuple(), args(), flags()} | err(). parse({error, _}=E) -> E; parse({Spec, ArgsAndFlags}) -> @@ -65,18 +70,20 @@ parse(ArgsAndFlags) -> end end. --spec parse_kv_args([string()]) -> err() | proplist(). +-spec parse_kv_args([string()]) -> err() | args(). parse_kv_args(Args) -> parse_kv_args(Args, []). %% All args must be k/v args! --spec parse_kv_args([string()], proplist()) -> err() | proplist(). +-spec parse_kv_args([string()], args()) -> err() | args(). parse_kv_args([], Acc) -> Acc; parse_kv_args([Arg | Args], Acc) -> case string:tokens(Arg, "=") of [Key, Val] -> - parse_kv_args(Args, [{list_to_atom(Key), Val} | Acc]); + parse_kv_args(Args, [{Key, Val} | Acc]); + [Key] -> + {error, {invalid_kv_arg, Key}}; _ -> {error, {too_many_equal_signs, Arg}} end. @@ -86,7 +93,7 @@ parse_kv_args([Arg | Args], Acc) -> parse_flags(Flags) -> parse_flags(Flags, [], []). --spec parse_flags([string()], list(), proplist()) -> proplist() | err(). +-spec parse_flags([string()], list(), flags()) -> flags() | err(). parse_flags([], [], Acc) -> Acc; parse_flags([], [Flag], Acc) -> @@ -94,9 +101,9 @@ parse_flags([], [Flag], Acc) -> parse_flags(["--"++Long | T], [], Acc) -> case string:tokens(Long,"=") of [Flag, Val] -> - parse_flags(T, [], [{list_to_atom(Flag), Val} | Acc]); + parse_flags(T, [], [{Flag, Val} | Acc]); [Flag] -> - parse_flags(T, [list_to_atom(Flag)], Acc) + parse_flags(T, [Flag], Acc) end; parse_flags(["--"++_Long | _T]=Flags, [Flag], Acc) -> parse_flags(Flags, [], [{Flag, undefined} | Acc]); @@ -116,9 +123,9 @@ parse_flags([Val | _T], [], _Acc) -> %% TODO: If this gets more complicated, write out a function to extract %% the flag names from ?GFLAG_SPECS instead of hand-coding it in ?GLOBAL_FLAGS -define(GLOBAL_FLAGS, [$h, help, format]). --define(GFLAG_SPECS, [{help, [{shortname, "h"}, - {longname, "help"}]}, - {format, [{longname, "format"}]}]). +-define(GFLAG_SPECS, [clique_spec:make({help, [{shortname, "h"}, + {longname, "help"}]}), + clique_spec:make({format, [{longname, "format"}]})]). %% @doc Extracts a list of globally applicable flags (e.g. --help) from the %% the original command. -spec extract_global_flags(err()) -> err(); @@ -133,8 +140,8 @@ extract_global_flags({Spec, Args, Flags0}) -> {Spec, Args, Flags, GlobalFlags}. -spec validate(err()) -> err(); - ({tuple(), proplist(), [flags()]}) -> - err() | {fun(), proplist(), flags()}. + ({tuple(), args(), flags(), flags()}) -> + err() | {fun(), proplist(), proplist(), flags()}. validate({error, _}=E) -> E; validate({Spec, Args0, Flags0, GlobalFlags}) -> @@ -163,87 +170,126 @@ convert_args(_KeySpec, [], Acc) -> convert_args([], Args, _Acc) -> {error, {invalid_args, Args}}; convert_args(KeySpecs, [{Key, Val0} | Args], Acc) -> - case lists:keyfind(Key, 1, KeySpecs) of - false -> - {error, {invalid_key, Key}}; - {Key, Spec} -> - case convert_arg(Spec, Key, Val0) of + case lists:keyfind(Key, #clique_spec.name, KeySpecs) of + Spec=#clique_spec{} -> + case convert_arg(Spec, Val0) of {error, _}=E -> E; Val -> - convert_args(KeySpecs, Args, [{Key, Val} | Acc]) - end + case validate_arg(Spec, Val) of + ok -> + convert_args(KeySpecs, Args, [{Spec#clique_spec.key, Val} | Acc]); + {error, _}=VE -> + VE + end + end; + false -> + {error, {invalid_key, Key}} end. --spec convert_arg(proplist(), atom(), string()) -> err() | term(). -convert_arg(Spec, Key, Val) -> - {typecast, Fun} = lists:keyfind(typecast, 1, Spec), +-spec convert_arg(spec(), string()) -> err() | term(). +convert_arg(#clique_spec{key=Key, typecast=Fun}, Val) when is_function(Fun) -> try Fun(Val) catch error:badarg -> - {error, {invalid_argument, {Key, Val}}} + {error, {invalid_argument, {Key, Val}}} + end; +convert_arg(#clique_spec{key=_Key, datatype=Type}, Val) when Type /= undefined -> + case cuttlefish_datatypes:from_string(Val, Type) of + {error, _}=E -> E; + Casted -> Casted + end. + +-spec validate_arg(spec(), term()) -> ok | err(). +validate_arg(#clique_spec{validator=undefined}, _) -> ok; +validate_arg(#clique_spec{key=Key, validator=Validator}, Val) when is_function(Validator)-> + try + Validator(Val) + catch + _:_ -> + {error, {invalid_argument, {Key, Val}}} end. --spec validate_flags(flagspecs(), proplist()) -> err() | flags(). +-spec validate_flags(flagspecs(), flags()) -> err() | proplist(). validate_flags(FlagSpecs, Flags) -> convert_flags(FlagSpecs, Flags, []). --spec convert_flags(flagspecs(), proplist(), flags()) -> err() | flags(). +-spec convert_flags(flagspecs(), flags(), proplist()) -> err() | proplist(). +convert_flags([], [], Acc) -> + Acc; convert_flags(_FlagSpecs, [], Acc) -> Acc; convert_flags([], Provided, _Acc) -> Invalid = [Flag || {Flag, _} <- Provided], {error, {invalid_flags, Invalid}}; convert_flags(FlagSpecs, [{Key, Val0} | Flags], Acc) -> - case lists:keyfind(Key, 1, FlagSpecs) of - false -> - %% We may have been passed a -short option instead of a --long option - case find_shortname_key(Key, FlagSpecs) of - {error, _}=E -> - E; - NewKey -> - %% We just want to replace the shortname with a valid key - %% (atom of longname) and call the same function again. - convert_flags(FlagSpecs, [{NewKey, Val0} | Flags], Acc) + case find_flag(FlagSpecs, Key) of + #clique_spec{key=NewKey}=Spec -> + case convert_flag(Spec, NewKey, Val0) of + {error, _}=E -> E; + Val -> convert_flags(FlagSpecs, Flags, [{NewKey, Val} | Acc]) end; - {Key, Spec} -> - case convert_flag(Spec, Key, Val0) of - {error, _}=E -> - E; - Val -> - convert_flags(FlagSpecs, Flags, [{Key, Val} | Acc]) - end + {error, _}=E -> E end. --spec convert_flag(proplist(), atom(), string()) -> err() | term(). +-spec find_flag(flagspecs(), string() | char()) -> spec() | err(). +find_flag(FlagSpecs, Key) -> + lists:foldl(fun(Idx, Acc) -> + case lists:keyfind(Key, Idx, FlagSpecs) of + #clique_spec{}=Spec -> Spec; + false -> Acc + end + end, + {error, {invalid_key, Key}}, + [#clique_spec.name, #clique_spec.shortname]). + + +-spec convert_flag(spec(), atom(), string()) -> err() | term(). convert_flag(Spec, Key, Val) -> %% Flags don't necessarily have values, in which case Val is undefined here. %% Additionally, flag values can also be strings and not have typecast funs. %% It's not incorrect, so just return the value in that case. - case lists:keyfind(typecast, 1, Spec) of - false -> - Val; - {typecast, Fun} -> + case cast_flag(Spec, Key, Val) of + {error, _}=CastError -> CastError; + CastedValue -> + validate_flag(Spec, Key, CastedValue) + end. + +-spec cast_flag(spec(), atom(), string()) -> err() | term(). +cast_flag(_, _, undefined) -> undefined; +cast_flag(#clique_spec{datatype=Type, typecast=Fun}, Key, Val) -> + if is_function(Fun) -> try Fun(Val) catch error:badarg -> - {error, {invalid_flag, {Key, Val}}} - end + {error, {invalid_flag, {Key, Val}}} + end; + Type == atom -> + %% TODO: We convert atoms here until cuttlefish handles + %% this safely. + try + list_to_existing_atom(Val) + catch + error:badarg -> + {error, {conversion, {Val, atom}}} + end; + Type /= undefined -> + cuttlefish_datatypes:from_string(Val, Type); + true -> + {error, {invalid_flag, {Key, Val}}} end. --spec find_shortname_key(char(), flagspecs()) -> err() | atom(). -find_shortname_key(ShortVal, FlagSpecs) -> - %% Make it a string instead of an int - Short = [ShortVal], - Error = {error, {invalid_flag, Short}}, - lists:foldl(fun({Key, Props}, Acc) -> - case lists:member({shortname, Short}, Props) of - true -> - Key; - false -> - Acc - end - end, Error, FlagSpecs). +-spec validate_flag(spec(), atom(), term()) -> err() | term(). +validate_flag(#clique_spec{validator=undefined}, _Key, CastedVal) -> + CastedVal; +validate_flag(#clique_spec{validator=Validator}, Key, CastedVal) when is_function(Validator) -> + try Validator(CastedVal) of + ok -> CastedVal; + {error, _} = Error -> Error + catch + _:_ -> + {error, {invalid_flag, {Key, CastedVal}}} + end. -spec is_not_kv_arg(string()) -> boolean(). is_not_kv_arg("-"++_Str) -> @@ -265,26 +311,48 @@ is_not_flag(Str) -> _ = list_to_integer(Str), true catch error:badarg -> - false + false end; false -> true end. -ifdef(TEST). --include_lib("eunit/include/eunit.hrl"). spec() -> Cmd = ["riak-admin", "test", "something"], - KeySpecs = [{sample_size, [{typecast, fun list_to_integer/1}]}], - FlagSpecs = [{node, [{shortname, "n"}, - {longname, "node"}, - {typecast, fun list_to_atom/1}]}, - {force, [{shortname, "f"}, - {longname, "force"}]}], + KeySpecs = [clique_spec:make({sample_size, [{typecast, fun list_to_integer/1}]})], + FlagSpecs = [clique_spec:make({node, [{shortname, "n"}, + {longname, "node"}, + {typecast, fun list_to_atom/1}]}), + clique_spec:make({force, [{shortname, "f"}, + {longname, "force"}]})], Callback = undefined, {Cmd, KeySpecs, FlagSpecs, Callback}. +dt_validate_spec() -> + Cmd = ["riak-admin", "test", "something"], + KeySpecs = [clique_spec:make({sample_size, [{datatype, integer}, + {validator, fun greater_than_zero/1}]})], + FlagSpecs = [clique_spec:make({node, [{shortname, "n"}, + {longname, "node"}, + {datatype, atom}, + {validator, fun phony_is_node/1}]}), + clique_spec:make({force, [{shortname, "f"}, + {longname, "force"}]})], + Callback = undefined, + {Cmd, KeySpecs, FlagSpecs, Callback}. + +greater_than_zero(N) when N > 0 -> ok; +greater_than_zero(N) -> {error, {invalid_value, N}}. + +phony_is_node(N) -> + Nodes = ['a@dev1', 'b@dev2', 'c@dev3'], + case lists:member(N, Nodes) of + true -> ok; + false -> {error, bad_node} + end. + parse_valid_flag_test() -> Spec = spec(), Node = "dev2@127.0.0.1", @@ -298,7 +366,7 @@ parse_valid_args_and_flag_test() -> Node = "dev2@127.0.0.1", ArgsAndFlags = ["key=value", "-n", Node], {Spec, Args, Flags} = parse({Spec, ArgsAndFlags}), - ?assertEqual(Args, [{key, "value"}]), + ?assertEqual(Args, [{"key", "value"}]), ?assertEqual(Flags, [{$n, Node}]). %% All arguments must be of type k=v @@ -316,7 +384,7 @@ parse_valueless_flags_test() -> {Spec, _, Flags} = parse({Spec, Args}), %% Flags with no value, get the value undefined ?assert(lists:member({$f, undefined}, Flags)), - ?assert(lists:member({'do-something', undefined}, Flags)). + ?assert(lists:member({"do-something", undefined}, Flags)). validate_valid_short_flag_test() -> Spec = spec(), @@ -331,7 +399,7 @@ validate_valid_long_flag_test() -> Spec = spec(), Args = [], Node = "dev2@127.0.0.1", - Flags = [{node, Node}, {force, undefined}], + Flags = [{"node", Node}, {"force", undefined}], {undefined, [], ConvertedFlags, []} = validate({Spec, Args, Flags, []}), ?assert(lists:member({node, 'dev2@127.0.0.1'}, ConvertedFlags)), ?assert(lists:member({force, undefined}, ConvertedFlags)). @@ -340,20 +408,50 @@ validate_invalid_flags_test() -> Spec = spec(), Args = [], Node = "dev2@127.0.0.1", - InvalidFlags = [{'some-flag', Node}, + InvalidFlags = [{"some-flag", Node}, {$b, Node}, {$a, undefined}], [?assertMatch({error, _}, validate({Spec, Args, [F], []})) || F <- InvalidFlags]. validate_valid_args_test() -> Spec = spec(), - Args = [{sample_size, "5"}], + Args = [{"sample_size", "5"}], {undefined, ConvertedArgs, [], []} = validate({Spec, Args, [], []}), ?assertEqual(ConvertedArgs, [{sample_size, 5}]). validate_invalid_args_test() -> Spec = spec(), - InvalidArgs = [{key, "value"}, {sample_size, "ayo"}], + InvalidArgs = [{"key", "value"}, {"sample_size", "ayo"}], [?assertMatch({error, _}, validate({Spec, [A], [], []})) || A <- InvalidArgs]. + +arg_datatype_test() -> + Spec = dt_validate_spec(), + ValidArg = [{"sample_size", "10"}], + {undefined, ConvertedArgs, [], []} = validate({Spec, ValidArg, [], []}), + ?assertEqual(ConvertedArgs, [{sample_size, 10}]), + + InvalidTypeArg = [{"sample_size", "A"}], + ?assertMatch({error, {conversion, _}}, validate({Spec, InvalidTypeArg, [], []})). + +arg_validation_test() -> + Spec = dt_validate_spec(), + InvalidArg = [{"sample_size", "0"}], + ?assertMatch({error, {invalid_value, _}}, validate({Spec, InvalidArg, [], []})). + +flag_datatype_test() -> + Spec = dt_validate_spec(), + ValidFlag = [{$n, "a@dev1"}], + {undefined, _, Flags, []} = validate({Spec, [], ValidFlag, []}), + ?assertEqual([{node, 'a@dev1'}], Flags), + + InvalidFlag = [{"node", "someothernode@foo.bar"}], + ?assertMatch({error, {conversion, _}}, validate({Spec, [], InvalidFlag, []})). + +flag_validation_test() -> + Spec = dt_validate_spec(), + _BadNode = 'badnode@dev2', %% NB: Atom must exist for type conversion to succeed + InvalidFlag = [{"node", "badnode@dev2"}], + ?assertEqual({error, bad_node}, validate({Spec, [], InvalidFlag, []})). + -endif. diff --git a/src/clique_spec.erl b/src/clique_spec.erl new file mode 100644 index 0000000..3a11387 --- /dev/null +++ b/src/clique_spec.erl @@ -0,0 +1,65 @@ +%% ------------------------------------------------------------------- +%% +%% Copyright (c) 2015 Basho Technologies, Inc. All Rights Reserved. +%% +%% This file is provided to you 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. +%% +%% ------------------------------------------------------------------- + +%% @doc Functions related to specifications of key-value arguments and +%% flags. +-module(clique_spec). +-include("clique_specs.hrl"). + +-export([ + make/1, + key/1, + name/1, + shortname/1, + datatype/1, + validator/1, + typecast/1 + ]). + +-type err() :: {error, term()}. + +%% @doc Creates a spec from a list of options. +make({Key, Options}) -> + Shortname = case proplists:get_value(shortname, Options) of + %% Unwrap the shortname character so we can match + %% more efficiently in the parser. + [Char] -> Char; + _ -> undefined + end, + #clique_spec{ + key = Key, + name = proplists:get_value(longname, Options, atom_to_list(Key)), + shortname = Shortname, + datatype = proplists:get_value(datatype, Options, string), + validator = proplists:get_value(validator, Options), + typecast = proplists:get_value(typecast, Options) + }. + +key(#clique_spec{key=V}) -> V. + +name(#clique_spec{name=V}) -> V. + +shortname(#clique_spec{shortname=V}) -> V. + +datatype(#clique_spec{datatype=V}) -> V. + +validator(#clique_spec{validator=V}) -> V. + +typecast(#clique_spec{typecast=V}) -> V.