diff --git a/coco/client.py b/coco/client.py index 2ac1571..9317ebd 100644 --- a/coco/client.py +++ b/coco/client.py @@ -58,6 +58,9 @@ def __init__(self, endpoint): self.endpoint = endpoint name = endpoint["name"] + # Holds user-facing names of endpoint values + self._val_name = {} + # Figure out short help. if "summary" in endpoint: # If a there's a "summary" use that. @@ -78,7 +81,6 @@ def __init__(self, endpoint): help_ = ( endpoint.get("description", endpoint.get("summary", "NO DESCRIPTION")) - + "\n\n\b\n" + param_help ) @@ -97,40 +99,133 @@ def _endpoint_params(self): params = [] param_help_list = [] - for name, type_ in sorted(self.endpoint.get("values", {}).items()): - # Bools are special - if type_ == "bool": - # Convert underscore to dash for param name - oname = name.replace("_", "-") - # Handle bool specially + # At most one list-type argument may be defined. If we find one, + # it will be stored here temporarily + list_arg = None + + # Name of this endpoint + ename = self.endpoint["name"] + + for name, val in sorted(self.endpoint.get("values", {}).items()): + # Sanity check + type_ = val["type"] + if type_ not in VALUE_TYPE: + raise click.ClickException( + f"Unknown parameter type {type_!r} in endpoint {ename}" + ) + + # Extract list itemtype for convenience + if type_ == "list": + item_type = val.get("item-type", "str") + else: + item_type = None + + # Is this an --option or an ARGUMENT (option is default) + is_option = val.get("option", True) + + # Is this boolean-valued? + is_flag = type_ == "bool" or item_type == "bool" + + # Figure out the param_decls. This is a list whose first element + # is always just "name" (which will be used for the keyword parameter + # name). + param_decls = [name] + + # For non-options, there are no more param_decls + if is_option: + # Add "params" to param_decls + for param in val.get("params", ()): + if len(param) == 1: + param_decls.append("-" + param) + else: + param_decls.append("--" + param) + + # If there were no parameters, make one out of the name + if len(param_decls) == 1: + param_decls.append("--" + name.lower().replace("_", "-")) + + # Add false-type params for bools + if is_flag: + off_flags = val.get("off_flags", ()) + if off_flags: + for flag in off_flags: + # The space-slash tells click these are a false-type options + if len(param) == 1: + param_decls.append(" /-" + param) + else: + param_decls.append(" /--" + param) + else: + # Create the false flag from the name + param_decls.append(" /--no-" + name.lower().replace("_", "-")) + + # unpack defaults from the dict + _, param_type, param_help = VALUE_TYPE[item_type if item_type else type_] + + # override help text with config value, if given + param_help = val.get("help", param_help) + + # For options, append a hint to the help. for lists + if is_option and type_ == "list": + if param_help[-1] not in ".!?": + param_help += "." + param_help += " Use multiple times to specify each list element." + + # Instantiate the click.Parameter (Option or Argument) + if is_option: + self._val_name[name] = param_decls[1] param = click.Option( - (f"--{oname}/--no-{oname}",), - is_flag=True, + param_decls, + type=param_type, + metavar=val.get("meta", None), required=True, - help="value flag", + multiple=(item_type is not None), + default=None, + is_flag=is_flag, + help=param_help, ) - elif type_ in VALUE_TYPE: - _, param_type, param_help = VALUE_TYPE[type_] - param = click.Argument((name,), type=param_type, required=True) - param_help_list.append((name.upper(), param_help)) else: - raise click.ClickException( - f"Unknown parameter type {type_!r} " - f"in endpoint {self.endpoint['name']}" + self._val_name[name] = name.upper() + param = click.Argument( + param_decls, + type=param_type, + metavar=val.get("meta", None), + required=True, + nargs=-1 if (item_type is not None) else 1, ) - # Append the options to the parameter list - params.append(param) + + # For non-options (arguments), we need to append the help text to + # the command's help. We collect them here. + param_help_list.append((name.upper(), param_help)) + + if item_type is not None and not is_option: + # list-type arguements are special + if list_arg: + # cocod should have already prevented this from happening. + raise click.ClickException( + "multiple list-type arguments in Endpoint {ename!r}!" + ) + list_arg = param + else: + # Otherwise, append the options to the parameter list + params.append(param) if param_help_list: - # Make the help, if any + # Make the help, if any arguments were defined formatter = click.HelpFormatter(indent_increment=4) with formatter.section("Required Parameters"): formatter.write_dl(param_help_list) - param_help = formatter.getvalue() + + # The line with the bell (\b) will be deleted. It tells click that + # the text following is already formatted and shouldn't be reflowed. + param_help = "\n\n\b\n" + formatter.getvalue() else: param_help = "" + # A list argument goes at the end of the parameters, if one was defined + if list_arg: + params.append(list_arg) + return params, param_help def callback(self, **kwargs): @@ -145,23 +240,37 @@ def callback(self, **kwargs): """ data = {} values = self.endpoint.get("values", {}) - for key, type_ in values.items(): - if type_ in ("list", "dict"): + for key, val in values.items(): + type_ = val["type"] + + if kwargs[key] is None: + raise click.ClickException( + f"missing required parameter {self._val_name[key]!r}" + ) + + if type_ == "dict": try: data[key] = json.loads(kwargs[key]) except json.JSONDecodeError as e: raise click.ClickException( - f"Failure parsing {key.upper()!r}: {e}" + f"Failure parsing {self._val_name[key]!r}: {e}" ) from e - else: data[key] = kwargs[key] # Validate type conversion - if not isinstance(data[key], VALUE_TYPE[type_][0]): + if type_ == "list": + item_type, _, item_help = VALUE_TYPE[val.get("item-type", "str")] + for idx, item in enumerate(data[key]): + if not isinstance(item, item_type): + raise click.ClickException( + f"Invalid value for element {idx} of " + f"{self._val_name[key]!r}: {item_help} expected" + ) + elif not isinstance(data[key], VALUE_TYPE[type_][0]): raise click.ClickException( - f"Invalid value for {key.upper()!r}: " - f"{VALUE_TYPE[type_][2]} expected" + f"Invalid value for {self._val_name[key]!r}: " + f"{VALUE_TYPE[type_][2]} expected ({data[key]})" ) return client_send_request( diff --git a/coco/endpoint.py b/coco/endpoint.py index 4396091..45504bf 100644 --- a/coco/endpoint.py +++ b/coco/endpoint.py @@ -34,7 +34,7 @@ "dict": (dict, click.STRING, "JSON object"), "float": (float, click.FLOAT, "float"), "int": (int, click.INT, "integer"), - "list": (list, click.STRING, "JSON list"), + "list": (list, None, None), "str": (str, click.STRING, "text"), } @@ -74,7 +74,7 @@ def __init__(self, name, conf, forwarder, state): if self.values: for key, value in self.values.items(): - self.values[key] = VALUE_TYPE[value][0] + self.values[key] = VALUE_TYPE[value["type"]][0] if not self.state: return @@ -407,16 +407,16 @@ async def call(self, request, **_): def _validate_enum( - parameter: str, value: str, options: list[str], location: str | None = None + value: str, parameter: str, options: list | set | tuple, location: str | None = None ) -> str: """Validate an enum in the endpoint config. Parameters ---------- - parameter: - Name of the parameter being validated value: Value of the parameter, if any + parameter: + Name of the parameter being validated options: Allowed values location: @@ -432,12 +432,17 @@ def _validate_enum( if location: parameter += f" in {location}" + if not isinstance(value, str): + raise click.ClickException(f"expected string for {parameter}.") + if value not in options: raise click.ClickException(f"unknown {parameter}. Expected one of {options}") return value -def _validate_dict(conf: dict, name: str, keys: tuple, location: str) -> None: +def _validate_dict( + conf: dict, name: str, keys: list | tuple | set, location: str +) -> None: """Validate a dict in the endpoint config. Parameters @@ -455,7 +460,178 @@ def _validate_dict(conf: dict, name: str, keys: tuple, location: str) -> None: raise click.ClickException(f"expected mapping for {name!r} in {location}") for key in conf: - _validate_enum(f"parameter {key!r} in {name!r} in {location}", key, keys) + _validate_enum(key, f"parameter {key!r} in {name!r} in {location}", keys) + + +def _validate_params(params: list | str, name: str, location: str) -> list: + """Validate "params" for an Endpoint value. + + Parameters + ---------- + params: + The param name list to validate + name: + The name of the params being validated + location: + Location description for error strings. + + Returns + ------- + list + `params`, possibly converted to a list + """ + + # listify + if isinstance(params, str): + params = [params] + elif not isinstance(params, list): + raise click.ClickException( + f"expected list or string for {name!r} in {location}" + ) + + # Validate each entry + for param in params: + if not isinstance(param, str): + raise click.ClickException(f"expected string in {name!r} in {location}") + if param.startswith("-"): + raise click.ClickException( + f"invalid start character in {name!r} in {location}" + ) + return params + + +def _validate_values(invals: list, location: str) -> dict: + """Validate the endpoint values. + + An old-style dict-of-types spec has already been converted into a + new-style list-of-dicts. + + Returns a dict of dicts, whose top-level keys are the value names, + just to make it easier to find values by name. + """ + + # At most one list-type non-option is permitted + have_list_arg = False + + outvals = {} + for inval in invals: + _validate_dict( + inval, + "item in values list", + { + "help", + "meta", + "name", + "on-flags", + "off-flags", + "option", + "params", + "type", + }, + location, + ) + + # Name is equired + try: + name = inval["name"] + except KeyError as e: + raise click.ClickException("name missing in values for {location}") from e + + if not isinstance(name, (str, int, float)): + raise click.ClickException( + "{name!r} is not a valid name for a value in {location}" + ) + name = str(name) + if name in outvals: + raise click.ClickException( + "value {name!r} defined more than once in {location}" + ) + if "/" in name or name[0] in ("-", "_"): + raise click.ClickException( + "{name!r} is not a valid name for a value in {location}" + ) + + # Update location string + location = f"value {name!r} for {location}" + + # Type is also required + try: + # Valid types are all the VALUE_TYPEs, plus lists of all the VALUE_TYPEs + type_types = set(VALUE_TYPE) | {f"list[{t}]" for t in VALUE_TYPE} + outval = { + "type": _validate_enum( + inval["type"], "type", type_types, location=location + ) + } + except KeyError as e: + raise click.ClickException("type missing in {location}") from e + + if outval["type"].startswith("list["): + # Decompose list-of-type. This strips "list[" and "]" + outval["item-type"] = outval["type"][5:-1] + outval["type"] = "list" + elif outval["type"] == "list": + # Default item-type for lists + outval["item-type"] = "str" + + outval["option"] = _validate_bool(inval, "option", True, location) + + # At most one list-type argument is allowed + if not outval["option"] and outval["type"] == "list": + if have_list_arg: + raise click.ClickException( + "multiple list-type non-option values detected " + f"while processing {location}" + ) + have_list_arg = True + + if outval["type"] == "bool" or ( + outval["type"] == "list" and outval["item-type"] == "bool" + ): + # Bools require option to be True + if not outval["option"]: + raise click.ClickException( + f'"option" must be true for boolean types in {location}' + ) + + # Handle bool flags + if "on-flags" in inval: + outval["params"] = _validate_params( + inval["on-flags"], "on-flags", location + ) + elif "params" in inval: + outval["params"] = _validate_params(inval["params"], "params", location) + + if "off-flags" in inval: + outval["off-flags"] = _validate_params( + inval["off-flags"], "on-flags", location + ) + else: + # Other parameters + if "params" in inval: + # Can't use "params" if option is false + if not outval["option"]: + raise click.ClickException( + f'"params" not allowed when "option" is false in {location}' + ) + outval["params"] = _validate_params(inval["params"], "params", location) + + # Other, optional parameters + if "help" in inval: + outval["help"] = str(inval["help"]) + if "meta" in inval: + if not isinstance(inval["meta"], str): + raise click.ClickException(f"expected string for meta in {location}") + if not any(k in inval["meta"] for k in "_ABCDEFGHIJKLMNOPQRSTUVWXYZ"): + raise click.ClickException( + f'invalid character in "meta" in {location}: ', + "expected capital letters and underscore", + ) + outval["meta"] = inval["meta"] + + # Add to dict-of-dicts + outvals[name] = outval + return outvals def _validate_forwards( @@ -603,13 +779,14 @@ def _validate_state_values(path, state, values, param, endpoint): severity(f"{location} is empty.") return - for value, type_ in values.items(): - if value in state_path: - if state_path[value]: - if not isinstance(state_path[value], VALUE_TYPE[type_][0]): + for name, value in values.items(): + type_ = value["type"] + if name in state_path: + if state_path[name]: + if not isinstance(state_path[name], VALUE_TYPE[type_][0]): raise click.ClickException( - f"Value {value!r} in state at {location} has type " - f"{type(state_path[value]).__name__} " + f"Value {name!r} in state at {location} has type " + f"{type(state_path[name]).__name__} " f"(expected {type_})." ) @@ -618,11 +795,11 @@ def _validate_state_values(path, state, values, param, endpoint): # because it's overwritten by the caller-supplied value if param == "send": logger.debug( - f"Value {value} in state at {location} will be ignored " + f"Value {name} in state at {location} will be ignored " "because it is overwritten by the endpoint's 'values'" ) elif param == "save": - logger.debug(f"Value {value} not set in {location}.") + logger.debug(f"Value {name} not set in {location}.") def _validate_bool(config: dict, param: str, default: bool, location: str) -> bool: @@ -718,7 +895,7 @@ def validate_endpoint(config: dict, groups: dict, all_endpoints: set, state) -> if key == "name": continue # Not part of endpoint format _validate_enum( - f"parameter {key!r}", key, ENDPOINT_PARAMETERS, location=location + key, f"parameter {key!r}", ENDPOINT_PARAMETERS, location=location ) # Set default description if none given. @@ -734,13 +911,13 @@ def validate_endpoint(config: dict, groups: dict, all_endpoints: set, state) -> # Some enums endpoint["report_type"] = _validate_enum( - "report_type", config.get("report_type", "CODES_OVERVIEW"), + "report_type", RESULT_TYPES, location=location, ) endpoint["type"] = _validate_enum( - "type", config.get("type", "GET"), ("GET", "POST"), location=location + config.get("type", "GET"), "type", ("GET", "POST"), location=location ) # Check group @@ -805,16 +982,22 @@ def validate_endpoint(config: dict, groups: dict, all_endpoints: set, state) -> # Validate endpoint values if "values" in config: - values = config["values"] - if not isinstance(values, dict): - raise click.ClickException(f"expected mapping for 'values' in {location}") - - for value, type_ in values.items(): - _validate_enum( - f"type for value {value!r}", - type_, - tuple(VALUE_TYPE), - location=location, + if isinstance(config["values"], dict): + # Old-style dict-of-types. We do an inline conversion here to the + # new-style list-of-dicts. + values = _validate_values( + [ + {"name": name, "type": type_} + for name, type_ in config["values"].items() + ], + location, + ) + elif isinstance(config["values"], list): + # New-style list + values = _validate_values(config["values"], location) + else: + raise click.ClickException( + f"expected list or mapping for 'values' in {location}" ) endpoint["values"] = values @@ -919,8 +1102,8 @@ def validate_endpoint(config: dict, groups: dict, all_endpoints: set, state) -> f"'type' missing from 'schedule.require_state' in {location}" ) _validate_enum( - "'require_state' type in 'schedule'", requirement["type"], + "'require_state' type in 'schedule'", tuple(VALUE_TYPE), location=location, ) diff --git a/tests/coco_runner.py b/tests/coco_runner.py index cb9bed5..c0a73ee 100644 --- a/tests/coco_runner.py +++ b/tests/coco_runner.py @@ -325,15 +325,16 @@ def start_daemon(self, *args): # even after it successfully fetches the port. sleep(0.2) - def client(self, *args, expected_result=0, no_daemon=False, no_backend=False): + def client(self, *args, expect_failure=False, no_daemon=False, no_backend=False): """Invoke the coco client. Parameters ---------- *args : str Positional arguments are used as commandline arguments. - expected_result : int - The expected exit code from the client. + expect_failure : bool + Controls whether a non-zero (failure) or zero (success) exit code + will cause an assertion failure no_daemon : bool, optional If True, don't start the daemon before running the client. If the daemon is already running, this @@ -386,10 +387,11 @@ def client(self, *args, expected_result=0, no_daemon=False, no_backend=False): # Print output so it appears in the test log on failure print(result.output) - assert result.exit_code == expected_result - if expected_result: + if expect_failure: + assert result.exit_code != 0 assert type(result.exception) is SystemExit else: + assert result.exit_code == 0 assert result.exception is None # Reset the coco client after the test. This needs to be done diff --git a/tests/test_client.py b/tests/test_client.py index 6564f5b..0ccd6ae 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -131,133 +131,134 @@ def test_endpoint_command(coco_runner): { "group": "cluster", "type": "POST", - "values": { - "bool": "bool", - "str": "str", - "int": "int", - "float": "float", - "list": "list", - "dict": "dict", - }, + "values": [ + {"name": "bool", "type": "bool"}, + {"name": "str", "type": "str"}, + {"name": "int", "type": "int"}, + {"name": "float", "type": "float"}, + {"name": "list", "type": "list[int]", "option": False}, + {"name": "dict", "type": "dict"}, + ], }, ) # Check the help output. It's not terribly informative, but it should all be there. result = coco_runner.client("endpoint", "--help").output # click help text uppercases all arguments - assert "STR" in result - assert "INT" in result - assert "FLOAT" in result + assert "--str" in result + assert "--int" in result + assert "--float" in result assert "LIST" in result - assert "DICT" in result - assert "--bool" in result # Bools are presented as flags. + assert "--dict" in result + assert "--bool" in result # Not providing parameters results in a usage error (=2) - coco_runner.client("endpoint", expected_result=2) + coco_runner.client("endpoint", expect_failure=True) # Partial parameters is still an error result = coco_runner.client( - "endpoint", '{"object": "value"}', "1.1", "1", "[1,2,3]", expected_result=2 + "endpoint", + '--dict={"object": "value"}', + "--float=1.1", + "--int=1", + "--str=str", + "1", + "2", + "3", + expect_failure=True, ) - assert "argument 'STR'" in result.output + assert "parameter '--bool'" in result.output - # Missing the boolean is still an error result = coco_runner.client( "endpoint", - '{"object": "value"}', - "1.1", + "--bool", + "--float=1.1", + "--int=1", + "--str=str", "1", - "[1,2,3]", - "abc", - expected_result=2, + "2", + "3", + expect_failure=True, ) - assert "option '--bool'" in result.output + assert "parameter '--dict'" in result.output # Bad dict -- syntax error result = coco_runner.client( "endpoint", - "{1: 2: 3}", - "1.1", - "4", - "[1,2,3]", - "abc", + "--dict={1: 2: 3}", + "--float=1.1", + "--int=4", + "1", + "2", + "3", + "--str=abc", "--bool", - expected_result=1, + expect_failure=True, ) - assert "Failure parsing 'DICT'" in result.output + assert "Failure parsing '--dict'" in result.output # Also a parsing error because bareword "scalar" without quotation marks is # not a string in JSON - result = coco_runner.client( - "endpoint", "scalar", "1.1", "4", "[1,2,3]", "abc", "--bool", expected_result=1 - ) - assert "Failure parsing 'DICT'" in result.output - - result = coco_runner.client( - "endpoint", "3", "1.1", "4", "[1,2,3]", "abc", "--bool", expected_result=1 - ) - assert "Invalid value for 'DICT'" in result.output - result = coco_runner.client( "endpoint", - '{"object": "value"}', - "pi", - "4", - "[1,2,3]", - "abc", + "--dict=scalar", + "--float=1.1", + "--int=4", + "1", + "--str=abc", "--bool", - expected_result=2, + expect_failure=True, ) - assert "Invalid value for 'FLOAT'" in result.output + assert "Failure parsing '--dict'" in result.output - # Notably here an integer float works result = coco_runner.client( "endpoint", - '{"object": "value"}', + "--dict=3", + "--float=1.1", + "--int=4", "1", - "4.4", - "[1,2,3]", - "abc", + "--str=abc", "--bool", - expected_result=2, + expect_failure=True, ) - assert "Invalid value for 'INT'" in result.output + assert "Invalid value for '--dict'" in result.output result = coco_runner.client( "endpoint", - '{"object": "value"}', - "1.1", - "4", - "[1;2;3]", - "abc", + '--dict={"object": "value"}', + "--float=pi", + "--int=4", + "1", + "--str=abc", "--bool", - expected_result=1, + expect_failure=True, ) - assert "Failure parsing 'LIST'" in result.output + assert "Invalid value for '--float'" in result.output + # Notably here an integer float works result = coco_runner.client( "endpoint", - '{"object": "value"}', - "1.1", - "4", - '{"object": "value"}', - "abc", + '--dict={"object": "value"}', + "--float=1", + "--int=4.4", + "1", + "--str=abc", "--bool", - expected_result=1, + expect_failure=True, ) - assert "Invalid value for 'LIST'" in result.output + assert "Invalid value for '--int'" in result.output result = coco_runner.client( "endpoint", - '{"object": "value"}', - "1.1", - "4", - "5", - "abc", + '--dict={"object": "value"}', + "--float=1.1", + "--int=4", + "[1,2,3]", + "--str=abc", "--bool", - expected_result=1, + expect_failure=True, ) - assert "Invalid value for 'LIST'" in result.output + assert "Invalid value for 'LIST...'" in result.output def test_value_handling(coco_runner): @@ -274,7 +275,7 @@ def test_value_handling(coco_runner): "str": "str", "int": "int", "float": "float", - "list": "list", + "list": "list[int]", "dict": "dict", }, }, @@ -285,11 +286,13 @@ def test_value_handling(coco_runner): "--quiet", "--json", "endpoint", - '{"object": "value"}', - "1.1", - "4", - "[1,2,3]", - "abc", + '--dict={"object": "value"}', + "--float=1.1", + "--int=4", + "--list=1", + "--list=2", + "--list=3", + "--str=abc", "--bool", ) result = json.loads(result.stdout) diff --git a/tests/test_endpoint.py b/tests/test_endpoint.py index 706c3d0..cae9896 100644 --- a/tests/test_endpoint.py +++ b/tests/test_endpoint.py @@ -295,12 +295,21 @@ def test_forward_reply(set_endpoint, cocod): def test_values_type(set_endpoint, cocod): - """Endpoint 'values' must be a mapping.""" + """Check Endpoint "values" type checking. + It's either a dict-of-types or a list-of-dicts + """ + + # List of strings is not allowed set_endpoint({"values": ["value1", "value2"]}) result = cocod(1, ["--check-config"]) assert "values" in result.output + # dict of lists is not allowed + set_endpoint({"values": {"value1": {"type": "int"}}}) + result = cocod(1, ["--check-config"]) + assert "value1" in result.output + def test_values_types(set_endpoint, cocod): """Test endpoint value types.""" @@ -596,5 +605,25 @@ def test_endpoint_rewrite(coco_runner): # Should be one of the endpoints we expected assert name in endpoints + original = endpoints[name][1] + + # Update the "values" dict, in the origina endpoint, if present + if "values" in original: + if isinstance(original["values"], dict): + values = { + name: {"type": type_, "option": True} + for name, type_ in original["values"].items() + } + else: + values = {} + for item in original["values"]: + name = item["name"] + del item["name"] + if "option" not in item: + item["option"] = True + + values[name] = item + original["values"] = values + # Check the config - assert endpoint == endpoints[name][1], f"Mismatch for endpoint {name!r}" + assert endpoint == original, f"Mismatch for endpoint {name!r}"