diff --git a/common/lib/user_input.py b/common/lib/user_input.py index cc5505aa2..3bada4489 100644 --- a/common/lib/user_input.py +++ b/common/lib/user_input.py @@ -43,24 +43,102 @@ class UserInput: OPTIONS_COSMETIC = (OPTION_INFO, OPTION_DIVIDER) + # the keys the framework understands in an option's settings dict. a key + # outside this set is almost always a typo (e.g. "coerce" for + # "coerce_type"), which is silently ignored and so has no effect - the + # module test flags these so they are caught rather than shipped. + KNOWN_OPTION_KEYS = frozenset({ + # -- read by the parser / module loader -- + "type", # UI/control type of the option (toggle, choice, string, ...) + "default", # value used when the option is not present in submitted input + "options", # valid choices for choice/multi/annotation options + "requires", # condition(s) controlling when the option is shown/parsed + "min", # minimum allowed value for numeric text options + "max", # maximum allowed value for numeric text options + "coerce_type", # Python type (int, float) the parsed value is coerced to + "indirect", # derived from other settings; skipped during parsing + "delegated", # shown/parsed only when dynamically delegated by another option + "dict_key", # sub-field or callable used as key for multi_option items + "columns", # sub-option definitions for table-style options + "sensitive", # treated as sensitive; excluded from public displays/logs and deleted on run + "mandatory", # user-submitted value must not be blank/empty + + # -- read by the interface templates -- + "help", # primary label/help text shown next to the form control + "tooltip", # longer explanatory text shown as tooltip or placeholder + "label", # display label for annotation badges + "inline", # render multi/multi_select choices inline + "original_default", # configured default preserved when the live value is overridden (settings panel) + "value", # HSV lightness/value component for hue colour pickers + "saturation", # HSV saturation component for hue colour pickers + "accept", # accepted file type for file inputs + "update", # CSS selector to update with hue picker changes + "password", # render string input as a password field + "multiple", # allow selecting multiple files + "cache", # tell the frontend to cache the value locally + + # -- annotation options -- + "to_parent", # attach annotations to the parent dataset + "hide_in_explorer", # hide annotation column from the Explorer UI + + # -- datasource options -- + "board_specific", # list of board IDs for which the option is shown + + # -- config/admin settings -- + "global", # setting applies across all users (not user-scoped) + }) + + @staticmethod + def unknown_option_keys(settings): + """ + List an option's settings keys that the framework does not understand + + Unknown keys are silently ignored at parse time, so a typo like + "coerce" (for "coerce_type") quietly does nothing. Used by the module + test to catch these. + + :param dict settings: An option's settings dictionary + :return list: The unrecognised keys, sorted (empty if all are known) + """ + if not isinstance(settings, dict): + return [] + return sorted(set(settings) - UserInput.KNOWN_OPTION_KEYS) + @staticmethod - def parse_all(options, input, silently_correct=True): + def parse_all(options, input, silently_correct=False, log=None): """ Parse form input for the provided options Ignores all input not belonging to any of the defined options: parses and sanitises the rest, and returns a dictionary with the sanitised - options. If an option is *not* present in the input, the default value - is used, and if that is absent, `None`. + options. + + An option that is *not* present in the input at all is an error when + parsing strictly: our own form always submits every field it shows, so + a missing option means an incomplete submission (e.g. an API call that + left it out), and quietly proceeding without it would let later checks + pass on values the caller never sent. Exceptions are options a form + legitimately leaves out: unchecked toggles (submitted as nothing, read + as False), file uploads (sent outside the form values), fields gated + by an unmet "requires" condition or by "board_specific" (left out of + the result), and unticked multi-selects (the form submits an empty + marker so they are still present). When parsing tolerantly + (silently_correct), a missing option gets its default instead. In other words, this ensures a dictionary with 1) only white-listed - keys, 2) a value of an expected type for each key. + keys, 2) a value of an expected type for each key, and 3) when strict, + a complete submission. :param dict options: Options, as a name -> settings dictionary :param dict input: Input, as a form field -> value dictionary :param bool silently_correct: If true, replace invalid values with the given default value; else, raise a QueryParametersException if a value - is invalid. + is invalid. Defaults to false: every real caller wants the strict + behaviour, and tolerant parsing should be opted into deliberately. + :param log: Optional logger. When an option's *default* value turns + out to be invalid (a developer mistake, not user input), a warning is + logged here and the default is corrected silently rather than shown to + the user. :return dict: Sanitised form input """ @@ -115,6 +193,19 @@ def parse_all(options, input, silently_correct=True): # whether or not the (hidden) form field was submitted. continue + # an option's default is set by us, not by the user, so an invalid + # one is a bug in the option definition: warn developers about it. + # it is deliberately not corrected quietly. the form pre-fills + # defaults, so a bad default comes back looking like a value the + # user typed; silently replacing it would run the analysis on a + # value the user never chose and cannot see, which is worse than an + # error. it is validated like any other value below, so it gets + # reported (naming the option) rather than hidden. + default_problem = UserInput.validate_default(settings) + if default_problem and log: + log.warning("Option '%s' has an invalid default (%s). " + "Please fix the option definition." % (option, default_problem)) + if settings.get("type") == UserInput.OPTION_DATERANGE: # special case, since it combines two inputs option_min = option + "-min" @@ -128,19 +219,26 @@ def parse_all(options, input, silently_correct=True): if option_max not in input or input.get(option_max) == "-1": option_max += "_proxy" + if settings.get("mandatory") and UserInput.is_empty_value(input.get(option_min)) and UserInput.is_empty_value(input.get(option_max)): + raise QueryParametersException("%s: This field is required." % (settings.get("help") or option)) + # save as a tuple of unix timestamps (or None) try: after, before = (UserInput.parse_value(settings, input.get(option_min), parsed_input, silently_correct), UserInput.parse_value(settings, input.get(option_max), parsed_input, silently_correct)) if before and after and after > before: if not silently_correct: - raise QueryParametersException("The start of the date range must be before its end.") + raise QueryParametersException("the start of the date range must be before its end.") else: before = after parsed_input[option] = (after, before) except RequirementsNotMetException: pass + except QueryParametersException as e: + # say which option the problem is about; parse_value only + # sees an option's settings, not its name + raise QueryParametersException("%s: %s" % (settings.get("help") or option, e)) elif settings.get("type") in (UserInput.OPTION_TOGGLE, UserInput.OPTION_ANNOTATION): # special case too, since if a checkbox is unchecked, it simply @@ -154,6 +252,8 @@ def parse_all(options, input, silently_correct=True): parsed_input[option] = False except RequirementsNotMetException: pass + except QueryParametersException as e: + raise QueryParametersException("%s: %s" % (settings.get("help") or option, e)) elif settings.get("type") == UserInput.OPTION_DATASOURCES: # special case, because this combines multiple inputs to @@ -184,7 +284,7 @@ def parse_all(options, input, silently_correct=True): choice = input.get(option + "-" + datasource + "-" + column, False) column_settings = settings["columns"][column] # sub-settings per column - table_input[datasource][column] = UserInput.parse_value(column_settings, choice, table_input, silently_correct=True) + table_input[datasource][column] = UserInput.parse_value(column_settings, choice, table_input, silently_correct=silently_correct) parsed_input[option] = table_input @@ -195,6 +295,10 @@ def parse_all(options, input, silently_correct=True): # i.e. forms within forms!!! item_options = settings["options"] input_items = {} + + if settings.get("mandatory") and not any(re.match(f"{option}-([0-9]+)-(.+)", key) for key in input): + raise QueryParametersException("%s: This field is required." % (settings.get("help") or option)) + for key, value in input.items(): if key_match := re.match(f"{option}-([0-9]+)-(.+)", key): input_index = int(key_match[1]) @@ -228,18 +332,77 @@ def parse_all(options, input, silently_correct=True): parsed_input[option] = {value[settings["dict_key"]]: {**value, "_id": value[settings["dict_key"]]} for value in parsed_input[option]} elif option not in input: - # not provided? use default - parsed_input[option] = settings.get("default", None) + # the option was not part of the submission at all. our own + # form always submits every field it shows - fields hidden by + # an unmet "requires" condition were already dropped above - + # so apart from the two exceptions below, a missing option + # means an incomplete submission. + if settings.get("type") == UserInput.OPTION_FILE: + # a file input's content does not travel among the form + # values (it is sent separately, in request.files), so it + # is never present here; whether a file was actually + # uploaded is for the module's validate_query to check + pass + + elif settings.get("board_specific"): + # only shown for particular boards: the form disables the + # field when another board is selected, and a disabled + # field is not submitted. like an unmet "requires", the + # user never saw it, so leave it out + pass + + elif settings.get("mandatory"): + raise QueryParametersException("%s: This field is required." % (settings.get("help") or option)) + + elif silently_correct: + # a caller that asked to be corrected quietly gets the + # default filled in + parsed_input[option] = settings.get("default", None) + + else: + # strict parsing: quietly filling in the default - or + # quietly leaving the option out - would let later checks + # pass on a value the caller never sent. an incomplete + # submission is the caller's mistake, so say so. + raise QueryParametersException( + "%s: this field was missing from the submitted form." % (settings.get("help") or option)) else: # normal parsing and sanitisation try: + if settings.get("mandatory") and UserInput.is_empty_value(input[option]): + raise QueryParametersException("This field is required.") + parsed_input[option] = UserInput.parse_value(settings, input[option], parsed_input, silently_correct) except RequirementsNotMetException: pass + except QueryParametersException as e: + # say which option the problem is about; parse_value only + # sees an option's settings, not its name + raise QueryParametersException("%s: %s" % (settings.get("help") or option, e)) return parsed_input + @staticmethod + def is_empty_value(value): + """ + Check whether a raw submitted value counts as blank for mandatory fields + + None, empty strings, and empty lists are treated as blank. Whitespace- + only strings are also considered blank. This is intentionally lenient + for other types; parse_value handles detailed validation. + + :param value: Raw submitted value + :return bool: True if the value is blank + """ + if value is None: + return True + if isinstance(value, str) and value.strip() == "": + return True + if isinstance(value, list) and len(value) == 0: + return True + return False + @staticmethod def requirements_met(requires, other_input): """ @@ -363,7 +526,7 @@ def _requirement_met(req, other_input): return True @staticmethod - def parse_value(settings, choice, other_input=None, silently_correct=True): + def parse_value(settings, choice, other_input=None, silently_correct=False): """ Filter user input @@ -406,37 +569,55 @@ def parse_value(settings, choice, other_input=None, silently_correct=True): # parse either integers (unix timestamps) or try to guess the date # format (the latter may be used for input if JavaScript is turned # off in the front-end and the input comes from there) - value = None + if choice is None or choice == "": + # this end of the range was left empty + return None try: - value = int(choice) - except ValueError: - parsed_choice = parse_datetime(choice) - value = int(parsed_choice.timestamp()) - finally: - return value - - elif input_type in (UserInput.OPTION_MULTI, UserInput.OPTION_ANNOTATIONS): - # any number of values out of a list of possible values - # comma-separated during input, returned as a list of valid options - if not choice: - return settings.get("default", []) - - chosen = choice.split(",") - return [item for item in chosen if item in settings.get("options", [])] - - elif input_type == UserInput.OPTION_MULTI_SELECT: - # multiple number of values out of a dropdown list of possible values - # comma-separated during input, returned as a list of valid options - if not choice: - return settings.get("default", []) - + return int(choice) + except (ValueError, TypeError): + pass + try: + return int(parse_datetime(choice).timestamp()) + except (ValueError, TypeError, OverflowError): + # not a number and not a date we can recognise. (previously this + # was swallowed by a return inside a finally block and silently + # became an open-ended range.) + if not silently_correct: + raise QueryParametersException("'%s' is not a valid date." % choice) + return None + + elif input_type in (UserInput.OPTION_MULTI, UserInput.OPTION_ANNOTATIONS, UserInput.OPTION_MULTI_SELECT): + # any number of values out of a list of possible values. these + # arrive either comma-separated (text controls and some client-side + # helpers) or as an actual list (real multi-selects and raw API + # callers), so accept both shapes. (OPTION_MULTI used to assume a + # string and crashed on a real list.) if type(choice) is str: - # should be a list if the form control was actually a multiselect - # but we have some client side UI helpers that may produce a string - # instead choice = choice.split(",") - return [item for item in choice if item in settings.get("options", [])] + # the form submits an empty value alongside whatever is selected, so + # that the field is present even when nothing is: a browser leaves a + # select with no selection out of the submission entirely, which is + # indistinguishable from the field never having been shown. discard + # that empty marker, and treat what remains as the selection. + choice = [item for item in (choice or []) if item != ""] + + if not choice: + if settings.get("mandatory"): + raise QueryParametersException("This field is required.") + if silently_correct: + return settings.get("default", []) + # a submitted-but-empty selection means the user unticked + # everything: a choice of nothing, not a request for the + # default back + return [] + + options = settings.get("options", []) + invalid = [item for item in choice if item not in options] + if invalid and not silently_correct: + raise QueryParametersException("Invalid value(s) selected: %s." % ", ".join(str(i) for i in invalid)) + + return [item for item in choice if item in options] elif input_type == UserInput.OPTION_CHOICE: # select box @@ -467,49 +648,146 @@ def parse_value(settings, choice, other_input=None, silently_correct=True): return json.loads(choice) elif input_type in (UserInput.OPTION_TEXT, UserInput.OPTION_TEXT_LARGE, UserInput.OPTION_HUE): - # text string - # optionally clamp it as an integer; return default if not a valid - # integer (or float; inferred from default or made explicit via the - # coerce_type setting) - if settings.get("coerce_type"): - value_type = settings["coerce_type"] + # a text field. it may be a plain string, or constrained to a number + # by a coerce_type (e.g. int) and/or a min/max. for a numeric field, + # a non-number or an out-of-range value is rejected (when + # silently_correct is off) or corrected to the default / nearest + # bound (when it is on). + coerce_type = settings.get("coerce_type") + if coerce_type is not None and not callable(coerce_type): + # the option is declared wrong: coerce_type should be a type + # such as int or float, not e.g. the string "int" + if not silently_correct: + raise QueryParametersException("This option is misconfigured: its coerce_type is not a type.") + coerce_type = None + + has_range = "min" in settings or "max" in settings + if coerce_type in (int, float): + number_type = coerce_type + elif has_range: + default_type = type(settings.get("default")) + number_type = default_type if default_type in (int, float) else int else: - value_type = type(settings.get("default")) - if value_type not in (int, float): - value_type = int - - if "max" in settings: - try: - choice = min(settings["max"], value_type(choice)) - except (ValueError, TypeError): - if not silently_correct: - raise QueryParametersException("Provide a value of %s or lower." % str(settings["max"])) + number_type = None + if choice is None or choice == "": + # an empty value that was actively submitted - an option left + # out of the submission never reaches this point. the default + # is not quietly filled back in: the form showed it, and the + # user removed it. for a text field, empty is an answer + # ("nothing"); for a numeric field it is not one. a tolerant + # caller gets the correcting behaviour, i.e. the default. + if silently_correct: choice = settings.get("default") - - if "min" in settings: + if choice is None: + choice = 0 if number_type is not None else "" + elif number_type is None: + return "" + else: + raise QueryParametersException("This field needs a number.") + + elif number_type is not None: + # a number is expected: coerce it first, then keep it in range. + # coercion and range are checked separately so the error + # actually matches the problem (a non-number no longer reports + # an out-of-range message, and an out-of-range value is no + # longer silently clamped away). try: - choice = max(settings["min"], value_type(choice)) + choice = number_type(choice) except (ValueError, TypeError): if not silently_correct: - raise QueryParametersException("Provide a value of %s or more." % str(settings["min"])) - + raise QueryParametersException("'%s' is not a valid number." % choice) choice = settings.get("default") + else: + if "max" in settings and choice > settings["max"]: + if not silently_correct: + raise QueryParametersException("Provide a value of %s or lower." % str(settings["max"])) + choice = settings["max"] + if "min" in settings and choice < settings["min"]: + if not silently_correct: + raise QueryParametersException("Provide a value of %s or more." % str(settings["min"])) + choice = settings["min"] - if choice is None or choice == "": - choice = settings.get("default") - - if choice is None: - choice = 0 if "min" in settings or "max" in settings else "" - - if settings.get("coerce_type"): + # apply an explicit coerce_type so a value that came from the default + # still has the declared type + if coerce_type: try: - return value_type(choice) + return coerce_type(choice) except (ValueError, TypeError): + if not silently_correct: + raise QueryParametersException("This option's default value cannot be parsed.") return settings.get("default") - else: - return choice + + return choice else: # no filtering return choice + + @staticmethod + def validate_default(settings): + """ + Check an option's own default value against the option's own rules + + A default is set by the developer, so a bad one is a bug in the option + definition, not something a user should ever see. This is what the + module test uses to catch those bugs, and what `parse_all` uses at + runtime to correct them quietly (see there). + + The value rules live in one place, `parse_value`: for the types where it + applies, this feeds the default back through `parse_value` (strictly, + and ignoring any "requires" gate, since we are checking the value on its + own) and reports whatever it rejects. Only a few structural checks that + `parse_value` cannot express are done here directly. + + Membership of a choice or multi-choice default in its option list is + checked only when that list is populated. Such lists are often built + from the parent dataset, so at test time (with no real parent) they may + be empty, in which case the check is skipped; at runtime the list is + authoritative and the check catches a default that isn't a real option. + + :param dict settings: An option's settings dictionary + :return str|None: A short developer-facing description of the problem, + or None if the default is fine + """ + default = settings.get("default") + input_type = settings.get("type") + + # an empty or absent default means "no default; the user must choose", + # which is a legitimate pattern rather than a mistake + if default is None or default == "" or default == [] or default == {}: + return None + + if input_type in (UserInput.OPTION_MULTI, UserInput.OPTION_MULTI_SELECT, UserInput.OPTION_ANNOTATIONS): + # a multi-value default must be a list; a bare string or, worse, a + # generator leaks a wrong-typed value into stored parameters + if not isinstance(default, list): + return "default should be a list, not %s" % type(default).__name__ + + # for choice/multi options, membership can only be checked when the + # list of options is populated (see docstring); skip it otherwise + if input_type in (UserInput.OPTION_CHOICE, UserInput.OPTION_MULTI, + UserInput.OPTION_MULTI_SELECT, UserInput.OPTION_ANNOTATIONS) and not settings.get("options"): + return None + + if input_type == UserInput.OPTION_TEXT_JSON: + # a JSON option's default is a Python object, not a raw string, so + # check it is serialisable instead of sending it through parse_value + try: + json.dumps(default) + return None + except (TypeError, ValueError) as e: + return "default is not valid JSON (%s)" % e + + if input_type == UserInput.OPTION_DATERANGE: + # date ranges have no meaningful single default value + return None + + probe = {key: value for key, value in settings.items() if key != "requires"} + try: + UserInput.parse_value(probe, default, silently_correct=False) + return None + except QueryParametersException as e: + return str(e) + except RequirementsNotMetException: + return None diff --git a/datasources/eightchan/search_8chan.py b/datasources/eightchan/search_8chan.py index 2a079e29d..23dffe8a8 100644 --- a/datasources/eightchan/search_8chan.py +++ b/datasources/eightchan/search_8chan.py @@ -101,13 +101,15 @@ def get_options(cls, parent_dataset=None, config=None): "min": 0, "max": 100, "default": 15, - "tooltip": "At least this many % of messages in the thread must match the query" + "tooltip": "At least this many % of messages in the thread must match the query", + "requires": "search_scope==dense-threads" }, "scope_length": { "type": UserInput.OPTION_TEXT, "help": "Min. dense thread length", "min": 30, "default": 30, - "tooltip": "A thread must at least be this many messages long to qualify as a 'dense thread'" + "tooltip": "A thread must at least be this many messages long to qualify as a 'dense thread'", + "requires": "search_scope==dense-threads" } } diff --git a/datasources/eightkun/search_8kun.py b/datasources/eightkun/search_8kun.py index dab7c6fbb..4e4d9f2a3 100644 --- a/datasources/eightkun/search_8kun.py +++ b/datasources/eightkun/search_8kun.py @@ -104,13 +104,15 @@ def get_options(cls, parent_dataset=None, config=None): "min": 0, "max": 100, "default": 15, - "tooltip": "At least this many % of messages in the thread must match the query" + "tooltip": "At least this many % of messages in the thread must match the query", + "requires": "search_scope==dense-threads" }, "scope_length": { "type": UserInput.OPTION_TEXT, "help": "Min. dense thread length", "min": 30, "default": 30, - "tooltip": "A thread must at least be this many messages long to qualify as a 'dense thread'" + "tooltip": "A thread must at least be this many messages long to qualify as a 'dense thread'", + "requires": "search_scope==dense-threads" } } diff --git a/datasources/fourchan/search_4chan.py b/datasources/fourchan/search_4chan.py index ef8409d27..d6235c6c1 100644 --- a/datasources/fourchan/search_4chan.py +++ b/datasources/fourchan/search_4chan.py @@ -390,18 +390,21 @@ def get_options(cls, parent_dataset=None, config=None): "min": 0, "max": 100, "default": 15, - "tooltip": "At least this many % of posts in the thread must match the query" + "tooltip": "At least this many % of posts in the thread must match the query", + "requires": "search_scope==dense-threads" }, "scope_length": { "type": UserInput.OPTION_TEXT, "help": "Min. dense thread length", "min": 30, "default": 30, - "tooltip": "A thread must at least be this many posts long to qualify as a 'dense thread'" + "tooltip": "A thread must at least be this many posts long to qualify as a 'dense thread'", + "requires": "search_scope==dense-threads" }, "valid_ids": { "type": UserInput.OPTION_TEXT, - "help": "Post IDs (comma-separated)" + "help": "Post IDs (comma-separated)", + "requires": "search_scope==match-ids" } } @@ -870,12 +873,17 @@ def validate_query(query, request, config): query["max_date"] = before del query["daterange"] + + # the scope fields are declared with a "requires" on the matching + # search scope, so on the web path they are already left out of the + # parsed input when their scope is not selected; this also covers + # callers that skip that parsing and pass them along anyway if query.get("search_scope") not in ("dense-threads",): - del query["scope_density"] - del query["scope_length"] + query.pop("scope_density", None) + query.pop("scope_length", None) - if query.get("search_scope") not in ("match-ids",) and "valid_ids" in query.keys(): - del query["valid_ids"] + if query.get("search_scope") not in ("match-ids",): + query.pop("valid_ids", None) return query diff --git a/processors/conversion/remove_author_info.py b/processors/conversion/remove_author_info.py index ad1e8daf1..49922c5f9 100644 --- a/processors/conversion/remove_author_info.py +++ b/processors/conversion/remove_author_info.py @@ -76,7 +76,10 @@ def get_options(cls, parent_dataset=None, config=None): parent_columns = parent_dataset.get_columns() parent_columns = {c: c for c in sorted(parent_columns)} - default_fields = itertools.chain(*[[c for c in parent_columns if fnmatch.fnmatch(c, d)] for d in default_fields]) + # a list, not the generator itertools.chain returns: this ends up as + # the option's default, which is stored and must survive being read + # more than once + default_fields = list(itertools.chain(*[[c for c in parent_columns if fnmatch.fnmatch(c, d)] for d in default_fields])) options["fields"] = { "type": UserInput.OPTION_MULTI_SELECT, diff --git a/processors/filtering/random_filter.py b/processors/filtering/random_filter.py index 6f13f88d1..665e9c0b1 100644 --- a/processors/filtering/random_filter.py +++ b/processors/filtering/random_filter.py @@ -7,7 +7,6 @@ from processors.filtering.base_filter import BaseFilter from common.lib.compatibility import Compatibility from common.lib.helpers import UserInput -from common.lib.exceptions import QueryParametersException __author__ = "Sal Hagen" __credits__ = ["Sal Hagen"] @@ -43,7 +42,8 @@ def get_options(cls, parent_dataset=None, config=None) -> dict: "type": UserInput.OPTION_TEXT, "help": "Sample size", "default": 10, - "coerce": int, + "coerce_type": int, + "min": 1, } } @@ -69,6 +69,8 @@ def filter_items(self): # Get the amount of rows to sample sample_size = self.parameters.get("sample_size") + # the sample size is checked when the job is queued, but jobs started by + # presets and other code do not go through that check, so check again try: sample_size = int(sample_size) except ValueError: @@ -104,24 +106,6 @@ def filter_items(self): count += 1 - @staticmethod - def validate_query(query, request, config): - """ - Validate input - - Checks if everything needed is filled in. - - :param query: - :param request: - :param config: - :return: - """ - - if not query["sample_size"] or not query["sample_size"].isnumeric() or not int(query["sample_size"]) > 0: - raise QueryParametersException("Please enter a valid sample size.") - - return query - class RandomProcessorFilter(RandomFilter): """ Retain only posts where a given column matches a given value diff --git a/processors/machine_learning/clarifai_api.py b/processors/machine_learning/clarifai_api.py index 1b6dfaab1..34ebe67cf 100644 --- a/processors/machine_learning/clarifai_api.py +++ b/processors/machine_learning/clarifai_api.py @@ -96,8 +96,10 @@ def get_options(cls, parent_dataset=None, config=None) -> dict: "annotation_threshold": { "type": UserInput.OPTION_TEXT, "help": "Only add labels above this confidence", - "default": "0.95", - "coerce_type": "float", + "default": 0.95, + "coerce_type": float, + "min": 0, + "max": 1, "requires": "save_annotations==true" }, "csv_info": { diff --git a/processors/visualisation/histwords.py b/processors/visualisation/histwords.py index 0bcb28311..93846d0cd 100644 --- a/processors/visualisation/histwords.py +++ b/processors/visualisation/histwords.py @@ -92,7 +92,10 @@ def get_options(cls, parent_dataset=None, config=None) -> dict: "type": UserInput.OPTION_TEXT, "help": "Similarity threshold", "tooltip": "Decimal value between 0 and 1; only neighbours with a higher similarity score than this will be included", - "default": "0.3" + "coerce_type": float, + "default": 0.3, + "min": -1, + "max": 1 }, "overlay": { "type": UserInput.OPTION_TOGGLE, diff --git a/processors/visualisation/video_scene_identifier.py b/processors/visualisation/video_scene_identifier.py index 0fc1b6a8b..a20c9487c 100644 --- a/processors/visualisation/video_scene_identifier.py +++ b/processors/visualisation/video_scene_identifier.py @@ -135,7 +135,7 @@ def get_options(cls, parent_dataset=None, config=None) -> dict: "coerce_type": float, "default": 27.0, "min": 0, - "max": 5, + "max": 255, "requires": "detector_type==content_detector" }, "ad_adaptive_threshold": { @@ -194,7 +194,7 @@ def get_options(cls, parent_dataset=None, config=None) -> dict: "save_annotations": { "type": UserInput.OPTION_ANNOTATION, "label": "scene data", - "hidden_in_explorer": True, + "hide_in_explorer": True, "tooltip": "Add amount of scenes per video to top dataset", "default": False } diff --git a/tests/test_modules.py b/tests/test_modules.py index ebb09dd36..c08ecfa03 100644 --- a/tests/test_modules.py +++ b/tests/test_modules.py @@ -252,6 +252,132 @@ def test_processors(logger, fourcat_modules, mock_job, mock_job_queue, mock_data else: logger.info("All processors passed successfully.") + +@pytest.mark.dependency(depends=["test_module_collector"]) +def test_option_default_validity(logger, fourcat_modules, mock_job, mock_job_queue, mock_dataset, mock_database, mock_basic_config): + """ + Every option's declared default must be valid under that option's own + rules. A bad default -- a number outside its own min/max, a coerce_type + that is not a callable type, a multi-value default that is not a list, + unparseable JSON -- is a mistake in the option definition, not user input, + and should never reach a user. Catch those here instead. + + Uses UserInput.validate_default. Membership of choice/multi defaults in + option lists that are built from the parent dataset at runtime is + intentionally not checked (it cannot be verified out of context). + """ + from common.lib.user_input import UserInput + + problems = [] + for processor_name, processor_class in fourcat_modules.processors.items(): + try: + options = processor_class.get_options(parent_dataset=mock_dataset, config=mock_basic_config) + except Exception: + # a failing get_options is test_processors' concern, not this test's + continue + + if not isinstance(options, dict): + continue + + for option_name, settings in options.items(): + if not isinstance(settings, dict): + continue + problem = UserInput.validate_default(settings) + if problem: + logger.error(f"{processor_name} option '{option_name}': {problem}") + problems.append(f"{processor_name}.{option_name}: {problem}") + + if problems: + pytest.fail("The following option defaults are invalid:\n" + "\n".join(problems)) + else: + logger.info("All option defaults are valid.") + + +@pytest.mark.dependency(depends=["test_module_collector"]) +def test_option_keys_known(logger, fourcat_modules, mock_job, mock_job_queue, mock_dataset, mock_database, mock_basic_config): + """ + Every key in an option's settings dictionary must be one the framework + understands. An unknown key (e.g. 'coerce' instead of 'coerce_type') is + silently ignored at parse time, so the setting it was meant to express + quietly has no effect. Catch those typos here. + + See UserInput.KNOWN_OPTION_KEYS for the recognised set. + """ + from common.lib.user_input import UserInput + + problems = [] + for processor_name, processor_class in fourcat_modules.processors.items(): + try: + options = processor_class.get_options(parent_dataset=mock_dataset, config=mock_basic_config) + except Exception: + continue + + if not isinstance(options, dict): + continue + + for option_name, settings in options.items(): + unknown = UserInput.unknown_option_keys(settings) + if unknown: + logger.error(f"{processor_name} option '{option_name}': unknown key(s) {unknown}") + problems.append(f"{processor_name}.{option_name}: unknown key(s) {', '.join(unknown)}") + + if problems: + pytest.fail("The following options have unrecognised keys:\n" + "\n".join(problems)) + else: + logger.info("All option keys are recognised.") + + +def test_parse_all_warns_about_a_bad_default_but_does_not_correct_it(): + """ + An option's default is set by us, so an invalid one is our bug and + developers are warned about it. It is deliberately not corrected quietly: + the form pre-fills defaults, so silently replacing the value would run the + analysis on something the user never chose and cannot see, which is worse + than an error. It is reported like any other invalid value instead. + """ + from common.lib.user_input import UserInput + from common.lib.exceptions import QueryParametersException + + options = {"threshold": { + "type": UserInput.OPTION_TEXT, + "help": "Threshold", + "coerce_type": float, + "default": 27.0, # invalid: above the max below + "min": 0, + "max": 5, + }} + log = MagicMock() + + # the form pre-fills the (bad) default and submits it back + with pytest.raises(QueryParametersException): + UserInput.parse_all(options, {"option-threshold": "27.0"}, silently_correct=False, log=log) + + assert log.warning.called, "developers should be warned about the bad default" + + +def test_parse_all_error_names_the_option(): + """ + A genuine user error should say which option it is about, rather than a + bare 'Provide a value of 5 or lower.' with no context. + """ + from common.lib.user_input import UserInput + from common.lib.exceptions import QueryParametersException + + options = {"cd_threshold": { + "type": UserInput.OPTION_TEXT, + "help": "Pixel intensity delta threshold", + "coerce_type": float, + "default": 1.0, # valid, so the value below is the user's own doing + "min": 0, + "max": 5, + }} + + with pytest.raises(QueryParametersException) as raised: + UserInput.parse_all(options, {"option-cd_threshold": "99"}, silently_correct=False) + + assert "Pixel intensity delta threshold" in str(raised.value) + + @pytest.mark.dependency(depends=["test_module_collector"]) def test_compatibility_coverage(logger, fourcat_modules): """ @@ -515,12 +641,14 @@ def test_parse_all_gated_options(): assert parsed == {"gate": False, "plain": "y"} # gate off, gated field submitted anyway (e.g. hidden field still posts): - # still absent - parsed = UserInput.parse_all(options, {"option-gated": ";"}) + # still absent. (plain must be submitted: a missing non-gated option is an + # incomplete submission and raises) + parsed = UserInput.parse_all(options, {"option-plain": "y", "option-gated": ";"}) assert "gated" not in parsed and "gated_toggle" not in parsed - # gate on: gated options are parsed (submitted value) or defaulted (absent) - parsed = UserInput.parse_all(options, {"option-gate": "on", "option-gated": ";"}) + # gate on: the gated text field is parsed; the gated toggle is absent from + # the submission, which for a toggle simply means unchecked + parsed = UserInput.parse_all(options, {"option-gate": "on", "option-plain": "y", "option-gated": ";"}) assert parsed["gated"] == ";" and parsed["gated_toggle"] is False and parsed["gate"] is True diff --git a/tests/test_user_input_mandatory.py b/tests/test_user_input_mandatory.py new file mode 100644 index 000000000..37cfd60f8 --- /dev/null +++ b/tests/test_user_input_mandatory.py @@ -0,0 +1,354 @@ +""" +Tests for the UserInput.mandatory option setting. + +This validates that options marked as mandatory raise QueryParametersException +when the user submits an empty value, while still allowing absent/null defaults +and non-empty values. +""" + +import pytest + +from common.lib.user_input import UserInput +from common.lib.exceptions import QueryParametersException + + +class TestMandatoryOption: + """ + Test the mandatory option key for a variety of input types. + """ + + def test_mandatory_text_raises_when_empty(self): + """ + A mandatory string option with an empty submitted value should raise. + """ + options = { + "query": { + "type": UserInput.OPTION_TEXT, + "help": "Search query", + "mandatory": True, + } + } + with pytest.raises(QueryParametersException): + UserInput.parse_all(options, {"query": ""}) + + def test_mandatory_text_raises_when_whitespace_only(self): + """ + Whitespace-only input should be treated as empty for mandatory fields. + """ + options = { + "query": { + "type": UserInput.OPTION_TEXT, + "help": "Search query", + "mandatory": True, + } + } + with pytest.raises(QueryParametersException): + UserInput.parse_all(options, {"query": " "}) + + def test_mandatory_text_accepts_value(self): + """ + A mandatory text option with a non-empty value should parse normally. + """ + options = { + "query": { + "type": UserInput.OPTION_TEXT, + "help": "Search query", + "mandatory": True, + } + } + parsed = UserInput.parse_all(options, {"query": "hello"}) + assert parsed == {"query": "hello"} + + def test_mandatory_text_missing_raises(self): + """ + A mandatory option that was not submitted at all, and whose default + does not fill it in, is not satisfied. This is how an API caller + omitting a field arrives, and it matches what a mandatory date range + or multi_option already does when absent. + """ + options = { + "query": { + "type": UserInput.OPTION_TEXT, + "help": "Search query", + "mandatory": True, + } + } + with pytest.raises(QueryParametersException): + UserInput.parse_all(options, {}) + + def test_mandatory_text_with_default_missing_input_raises(self): + """ + A default is the form's starting value, not a stand-in for an answer. + Defaults are no longer filled in for a strict caller, so an option that + was not submitted has not been given - and a mandatory option that was + not given fails, whether or not it has a default. + """ + options = { + "query": { + "type": UserInput.OPTION_TEXT, + "help": "Search query", + "default": "default query", + "mandatory": True, + } + } + with pytest.raises(QueryParametersException): + UserInput.parse_all(options, {}) + + def test_missing_option_raises_unless_correcting_silently(self): + """ + Our own form always submits every field it shows, so an option that is + missing from the input altogether means an incomplete submission (e.g. + an API call that left it out). Strict parsing refuses it, naming the + option - quietly proceeding would let later checks pass on values the + caller never sent. A caller that asked to be corrected quietly gets + the default filled in instead. + """ + options = { + "query": { + "type": UserInput.OPTION_TEXT, + "help": "Search query", + "default": "default query", + } + } + + assert UserInput.parse_all(options, {}, silently_correct=True) == {"query": "default query"} + with pytest.raises(QueryParametersException) as raised: + UserInput.parse_all(options, {}) + assert "Search query" in str(raised.value) + + def test_cleared_text_stays_cleared(self): + """ + The form pre-fills the default into the field, so an empty submitted + value means the user deliberately removed it. That is an answer + ("nothing"), and it is stored as such - not quietly swapped back for + the default the user just deleted. A tolerant caller still gets the + default. + """ + options = { + "query": { + "type": UserInput.OPTION_TEXT, + "help": "Search query", + "default": "default query", + } + } + assert UserInput.parse_all(options, {"query": ""}) == {"query": ""} + assert UserInput.parse_all(options, {"query": ""}, silently_correct=True) == {"query": "default query"} + + def test_cleared_number_field_raises(self): + """ + There is no honest empty number: a user who clears a numeric field and + submits is told to enter a number, rather than having the deleted + default quietly restored. + """ + options = { + "amount": { + "type": UserInput.OPTION_TEXT, + "help": "Number of items", + "coerce_type": int, + "min": 1, + "default": 10, + } + } + with pytest.raises(QueryParametersException) as raised: + UserInput.parse_all(options, {"amount": ""}) + assert "Number of items" in str(raised.value) + + assert UserInput.parse_all(options, {"amount": ""}, silently_correct=True) == {"amount": 10} + + def test_unticking_an_optional_multi_means_none(self): + """ + Unticking every option of a non-mandatory multi-select is a choice of + nothing - the result is an empty selection, not the default the user + just unticked. + """ + options = { + "boards": { + "type": UserInput.OPTION_MULTI, + "help": "Boards", + "options": {"pol": "/pol/", "v": "/v/"}, + "default": ["pol"], + } + } + # the form's empty marker on its own: nothing selected + assert UserInput.parse_all(options, {"boards": ""}) == {"boards": []} + assert UserInput.parse_all(options, {"boards": ""}, silently_correct=True) == {"boards": ["pol"]} + + def test_mandatory_multi_raises_when_empty(self): + """ + A mandatory multi-select with no selected values should raise. + """ + options = { + "boards": { + "type": UserInput.OPTION_MULTI, + "help": "Boards", + "options": {"pol": "/pol/", "v": "/v/"}, + "mandatory": True, + } + } + with pytest.raises(QueryParametersException): + UserInput.parse_all(options, {"boards": ""}) + + def test_mandatory_multi_raises_when_nothing_selected(self): + """ + When a user selects nothing in a multi-select, the form leaves the + field out of the submission altogether rather than sending it empty - + the same way an unchecked checkbox behaves. That absent shape, not an + empty value, is what actually reaches us, so it is the one that has to + be caught. + """ + options = { + "boards": { + "type": UserInput.OPTION_MULTI, + "help": "Boards", + "options": {"pol": "/pol/", "v": "/v/"}, + "mandatory": True, + } + } + with pytest.raises(QueryParametersException): + UserInput.parse_all(options, {}) + + def test_mandatory_multi_raises_when_default_is_unselected(self): + """ + A user who unticks every option, including a selected default, has + chosen nothing and should be told so rather than quietly handed the + default back. + + A browser submits nothing at all for a select with no selection, which + would be indistinguishable from the field never having been shown, so + the form submits an empty value alongside it. That empty marker on its + own is what "I unticked everything" looks like here. + """ + options = { + "boards": { + "type": UserInput.OPTION_MULTI, + "help": "Boards", + "options": {"pol": "/pol/", "v": "/v/"}, + "default": ["pol"], + "mandatory": True, + } + } + with pytest.raises(QueryParametersException): + UserInput.parse_all(options, {"boards": ""}) + + def test_multi_discards_the_empty_marker(self): + """ + The empty value the form submits alongside the selected ones is not a + selection, and must not be mistaken for an invalid choice. + """ + options = { + "boards": { + "type": UserInput.OPTION_MULTI, + "help": "Boards", + "options": {"pol": "/pol/", "v": "/v/"}, + } + } + parsed = UserInput.parse_all(options, {"boards": ["", "pol"]}) + assert parsed == {"boards": ["pol"]} + + def test_multi_still_rejects_an_invalid_choice_alongside_the_marker(self): + """ + Discarding the marker must not become a way to smuggle bad values past + validation. + """ + options = { + "boards": { + "type": UserInput.OPTION_MULTI, + "help": "Boards", + "options": {"pol": "/pol/", "v": "/v/"}, + } + } + with pytest.raises(QueryParametersException): + UserInput.parse_all(options, {"boards": ["", "nope"]}) + + def test_mandatory_multi_accepts_value(self): + """ + A mandatory multi-select with at least one selected value should parse. + """ + options = { + "boards": { + "type": UserInput.OPTION_MULTI, + "help": "Boards", + "options": {"pol": "/pol/", "v": "/v/"}, + "mandatory": True, + } + } + parsed = UserInput.parse_all(options, {"boards": "pol"}) + assert parsed == {"boards": ["pol"]} + + def test_mandatory_daterange_raises_when_both_empty(self): + """ + A mandatory date range with neither bound provided should raise. + """ + options = { + "daterange": { + "type": UserInput.OPTION_DATERANGE, + "help": "Date range", + "mandatory": True, + } + } + with pytest.raises(QueryParametersException): + UserInput.parse_all(options, { + "daterange-min": "", + "daterange-max": "", + }) + + def test_mandatory_daterange_accepts_partial_range(self): + """ + A mandatory date range with only one bound should parse that bound. + """ + options = { + "daterange": { + "type": UserInput.OPTION_DATERANGE, + "help": "Date range", + "mandatory": True, + } + } + parsed = UserInput.parse_all(options, { + "daterange-min": "2024-01-01", + "daterange-max": "", + }) + assert parsed["daterange"][0] is not None + assert parsed["daterange"][1] is None + + def test_mandatory_multi_option_raises_when_empty(self): + """ + A mandatory multi_option with no submitted sub-items should raise. + """ + options = { + "filters": { + "type": UserInput.OPTION_MULTI_OPTION, + "help": "Filters", + "mandatory": True, + "options": { + "column": { + "type": UserInput.OPTION_TEXT, + "default": "", + "help": "Column", + } + } + } + } + with pytest.raises(QueryParametersException): + UserInput.parse_all(options, {}) + + def test_mandatory_multi_option_accepts_item(self): + """ + A mandatory multi_option with at least one submitted item should parse. + """ + options = { + "filters": { + "type": UserInput.OPTION_MULTI_OPTION, + "help": "Filters", + "mandatory": True, + "options": { + "column": { + "type": UserInput.OPTION_TEXT, + "default": "", + "help": "Column", + } + } + } + } + parsed = UserInput.parse_all(options, {"filters-1-column": "body"}) + assert len(parsed["filters"]) == 1 + assert parsed["filters"][0]["column"] == "body" diff --git a/webtool/templates/components/datasource-option.html b/webtool/templates/components/datasource-option.html index cd8694fec..3130c8204 100644 --- a/webtool/templates/components/datasource-option.html +++ b/webtool/templates/components/datasource-option.html @@ -103,6 +103,11 @@

{{ settings.help }}

{% endif %} {% elif settings.type == "multi" %}
+ {# a browser submits nothing at all for a select with nothing selected, which + would be indistinguishable from the field never having been shown. this + empty value keeps the field in the submission either way; it is discarded + while parsing #} + {% elif option_settings.type in ("multi", "annotations") %}
+ {# a browser submits nothing at all for a select with nothing selected, which + would be indistinguishable from the field never having been shown. this + empty value keeps the field in the submission either way; it is discarded + while parsing #} +