From caeb72502d6c32ab08651b665d4585d145582e12 Mon Sep 17 00:00:00 2001 From: Dale Wahl Date: Thu, 16 Jul 2026 12:20:09 +0200 Subject: [PATCH 01/20] update_input: don't swallow bad dates --- common/lib/user_input.py | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/common/lib/user_input.py b/common/lib/user_input.py index 55aac477f..7c5c243d9 100644 --- a/common/lib/user_input.py +++ b/common/lib/user_input.py @@ -396,14 +396,10 @@ 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 @@ -417,6 +413,16 @@ def parse_value(settings, choice, other_input=None, silently_correct=True): 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 + 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. + if not silently_correct: + raise QueryParametersException("'%s' is not a valid date." % choice) + return None if not choice: return settings.get("default", []) From 541b8c42fb955a4517c529e2bbb2b88d9ed75cdf Mon Sep 17 00:00:00 2001 From: Dale Wahl Date: Thu, 16 Jul 2026 12:22:03 +0200 Subject: [PATCH 02/20] update_input: fix OPTION_MULTI fail on list; raise on invalid options --- common/lib/user_input.py | 29 ++++++++++++----------------- 1 file changed, 12 insertions(+), 17 deletions(-) diff --git a/common/lib/user_input.py b/common/lib/user_input.py index 7c5c243d9..834141d43 100644 --- a/common/lib/user_input.py +++ b/common/lib/user_input.py @@ -400,19 +400,6 @@ def parse_value(settings, choice, other_input=None, silently_correct=True): # this end of the range was left empty return None try: - - 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 return int(choice) except (ValueError, TypeError): pass @@ -423,16 +410,24 @@ def parse_value(settings, choice, other_input=None, silently_correct=True): 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. if not choice: return settings.get("default", []) 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", [])] + 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 From 26b0c55d39c2baaf28cf20880c21bc0a23b05597 Mon Sep 17 00:00:00 2001 From: Dale Wahl Date: Thu, 16 Jul 2026 12:27:02 +0200 Subject: [PATCH 03/20] update_input: clean up text input: ensure coarse works, determine number first to improve user exception message, inform user when input not a number (instead of always correcting) --- common/lib/user_input.py | 54 +++++++++++++++++++++++++++++----------- 1 file changed, 40 insertions(+), 14 deletions(-) diff --git a/common/lib/user_input.py b/common/lib/user_input.py index 834141d43..e1592786f 100644 --- a/common/lib/user_input.py +++ b/common/lib/user_input.py @@ -222,8 +222,21 @@ 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) + # not provided? use the default. the default is set by the + # developer, so if it is somehow invalid that is our mistake, + # not the user's: correct it silently and warn developers via + # the log, rather than surfacing the mistake to the user. + problem = UserInput.validate_default(settings) + if problem: + if log: + log.warning("Option '%s' has an invalid default (%s); using a corrected value. " + "Please fix the option definition." % (option, problem)) + try: + parsed_input[option] = UserInput.parse_value(settings, settings.get("default"), parsed_input, silently_correct=True) + except RequirementsNotMetException: + pass + else: + parsed_input[option] = settings.get("default", None) else: # normal parsing and sanitisation @@ -321,6 +334,7 @@ def _requirement_met(req, other_input): @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,7 +420,9 @@ def parse_value(settings, choice, other_input=None, silently_correct=True): try: return int(parse_datetime(choice).timestamp()) except (ValueError, TypeError, OverflowError): - # not a number and not a date we can recognise. + # 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 @@ -415,7 +431,7 @@ def parse_value(settings, choice, other_input=None, silently_correct=True): # 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. + # callers), so accept both shapes. (OPTION_MULTI used to assume a if not choice: return settings.get("default", []) @@ -458,18 +474,28 @@ 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 + # 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") + # the option is declared wrong: coerce_type should be a type + # such as int or float, not e.g. the string "int" + 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 + number_type = None - if "max" in settings: + # no value given: fall back to the default + choice = settings.get("default") + if choice is None: + choice = 0 if has_range else "" try: choice = min(settings["max"], value_type(choice)) except (ValueError, TypeError): From 8c710262e2427795acdf92009473198cae2e755d Mon Sep 17 00:00:00 2001 From: Dale Wahl Date: Thu, 16 Jul 2026 12:29:02 +0200 Subject: [PATCH 04/20] whoops: refactor missed lines --- common/lib/user_input.py | 53 +++++++++++++++++++++++----------------- 1 file changed, 31 insertions(+), 22 deletions(-) diff --git a/common/lib/user_input.py b/common/lib/user_input.py index e1592786f..867897e0e 100644 --- a/common/lib/user_input.py +++ b/common/lib/user_input.py @@ -431,7 +431,7 @@ def parse_value(settings, choice, other_input=None, silently_correct=False): # 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 + # callers), so accept both shapes. if not choice: return settings.get("default", []) @@ -475,12 +475,16 @@ def parse_value(settings, choice, other_input=None, silently_correct=False): elif input_type in (UserInput.OPTION_TEXT, UserInput.OPTION_TEXT_LARGE, UserInput.OPTION_HUE): # 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 @@ -492,40 +496,45 @@ def parse_value(settings, choice, other_input=None, silently_correct=False): else: number_type = None + if choice is None or choice == "": # no value given: fall back to the default choice = settings.get("default") if choice is None: choice = 0 if has_range else "" - 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"])) - choice = settings.get("default") - - if "min" in settings: + 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 From 17e051ef54def49b98ffb1785b840844fcb0c55a Mon Sep 17 00:00:00 2001 From: Dale Wahl Date: Thu, 16 Jul 2026 12:30:26 +0200 Subject: [PATCH 05/20] user_input: add knowns keys to check (to catch "coerce" vs "coerce_type" bug and potentially others) --- common/lib/user_input.py | 43 +++++++++++++++++++++++++++++++++++++--- 1 file changed, 40 insertions(+), 3 deletions(-) diff --git a/common/lib/user_input.py b/common/lib/user_input.py index 867897e0e..ba5f8efb6 100644 --- a/common/lib/user_input.py +++ b/common/lib/user_input.py @@ -43,6 +43,39 @@ 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", "default", "options", "requires", "min", "max", "coerce_type", + "indirect", "delegated", "dict_key", "columns", "sensitive", + # read by the interface templates + "help", "tooltip", "label", "inline", "original_default", "value", + "saturation", "accept", "update", "password", "multiple", "cache", + # annotation options + "to_parent", "hide_in_explorer", + # datasource options + "board_specific", + }) + + @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): """ @@ -60,7 +93,12 @@ def parse_all(options, input, silently_correct=True): :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 """ @@ -178,7 +216,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 @@ -333,7 +371,6 @@ 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 From 09ce69b93244df6a073027c1766444ce2db1abbd Mon Sep 17 00:00:00 2001 From: Dale Wahl Date: Thu, 16 Jul 2026 12:31:20 +0200 Subject: [PATCH 06/20] user_input: swithc silently_correct default to False (more secure), add validate_default and log for devs --- common/lib/user_input.py | 70 +++++++++++++++++++++++++++++++++++- webtool/views/api_tool.py | 3 +- webtool/views/views_admin.py | 2 +- 3 files changed, 72 insertions(+), 3 deletions(-) diff --git a/common/lib/user_input.py b/common/lib/user_input.py index ba5f8efb6..d7bf44cfd 100644 --- a/common/lib/user_input.py +++ b/common/lib/user_input.py @@ -77,7 +77,7 @@ def unknown_option_keys(settings): 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 @@ -576,3 +576,71 @@ def parse_value(settings, choice, other_input=None, silently_correct=False): 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/webtool/views/api_tool.py b/webtool/views/api_tool.py index 903f5fb75..37e86e58a 100644 --- a/webtool/views/api_tool.py +++ b/webtool/views/api_tool.py @@ -452,7 +452,7 @@ def queue_dataset(): # just in case try: # first sanitise values - sanitised_query = UserInput.parse_all(search_worker.get_options(None, g.config), request.form, silently_correct=False) + sanitised_query = UserInput.parse_all(search_worker.get_options(None, g.config), request.form, silently_correct=False, log=g.log) # then validate for this particular datasource sanitised_query = {"frontend-confirm": has_confirm, **sanitised_query} @@ -1207,6 +1207,7 @@ def queue_processor(key=None, processor=None): processor_worker.get_options(dataset, g.config), request.form, silently_correct=False, + log=g.log, ) sanitised_query["frontend-confirm"] = bool(request.form.get("frontend-confirm", False)) diff --git a/webtool/views/views_admin.py b/webtool/views/views_admin.py index f61ae2382..60c840314 100644 --- a/webtool/views/views_admin.py +++ b/webtool/views/views_admin.py @@ -654,7 +654,7 @@ def manipulate_settings(): # this gives us the parsed values, as Python variables, i.e. before # potentially encoding them as JSON new_settings = UserInput.parse_all(definition, request.form, - silently_correct=False) + silently_correct=False, log=g.log) for setting, value in new_settings.items(): if tag: From 38136ca8465d08584d3d52b06ad1f9e38fd23d8f Mon Sep 17 00:00:00 2001 From: Dale Wahl Date: Thu, 16 Jul 2026 12:32:02 +0200 Subject: [PATCH 07/20] add tests! check option defaults (where possible) and ensure no bad option keys --- tests/test_modules.py | 75 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 75 insertions(+) diff --git a/tests/test_modules.py b/tests/test_modules.py index 027a9c4ef..19e330bc2 100644 --- a/tests/test_modules.py +++ b/tests/test_modules.py @@ -252,6 +252,81 @@ 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.") + + @pytest.mark.dependency(depends=["test_module_collector"]) def test_compatibility_coverage(logger, fourcat_modules): """ From 601cc129b71954a4f54b371646a2eda28fb51911 Mon Sep 17 00:00:00 2001 From: Dale Wahl Date: Thu, 16 Jul 2026 14:31:28 +0200 Subject: [PATCH 08/20] user_input: tell user which option failed --- common/lib/user_input.py | 27 +++++++++++++--------- tests/test_modules.py | 49 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+), 11 deletions(-) diff --git a/common/lib/user_input.py b/common/lib/user_input.py index d7bf44cfd..395722bef 100644 --- a/common/lib/user_input.py +++ b/common/lib/user_input.py @@ -173,6 +173,10 @@ def parse_all(options, input, silently_correct=False, log=None): 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 @@ -186,6 +190,8 @@ def parse_all(options, input, silently_correct=False, log=None): 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 @@ -260,15 +266,9 @@ def parse_all(options, input, silently_correct=False, log=None): 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 the default. the default is set by the - # developer, so if it is somehow invalid that is our mistake, - # not the user's: correct it silently and warn developers via - # the log, rather than surfacing the mistake to the user. - problem = UserInput.validate_default(settings) - if problem: - if log: - log.warning("Option '%s' has an invalid default (%s); using a corrected value. " - "Please fix the option definition." % (option, problem)) + # not provided? use the default. an invalid one was warned + # about above; parsing it leniently corrects it quietly + if default_problem: try: parsed_input[option] = UserInput.parse_value(settings, settings.get("default"), parsed_input, silently_correct=True) except RequirementsNotMetException: @@ -279,9 +279,13 @@ def parse_all(options, input, silently_correct=False, log=None): else: # normal parsing and sanitisation try: - parsed_input[option] = UserInput.parse_value(settings, input[option], parsed_input, silently_correct) + parsed_input[option] = UserInput.parse_value(settings, input[option], parsed_input, option_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 @@ -468,7 +472,8 @@ def parse_value(settings, choice, other_input=None, silently_correct=False): # 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. + # callers), so accept both shapes. (OPTION_MULTI used to assume a + # string and crashed on a real list.) if not choice: return settings.get("default", []) diff --git a/tests/test_modules.py b/tests/test_modules.py index 19e330bc2..d1004488d 100644 --- a/tests/test_modules.py +++ b/tests/test_modules.py @@ -327,6 +327,55 @@ def test_option_keys_known(logger, fourcat_modules, mock_job, mock_job_queue, mo logger.info("All option keys are recognised.") +def test_parse_all_bad_default_is_not_blamed_on_the_user(): + """ + The form pre-fills an option's default, so a bad default is submitted back + looking exactly like a value the user typed. That is our mistake, not + theirs: it must be corrected quietly and logged for developers, never + raised as an error at the user. + """ + from common.lib.user_input import UserInput + + 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 submits the pre-filled (bad) default back to us + parsed = UserInput.parse_all(options, {"option-threshold": "27.0"}, silently_correct=False, log=log) + + assert parsed["threshold"] == 5, "a bad default should be corrected, not rejected" + 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): """ From 388a301e4355fb6f970a393a6b03f03e68f92cc5 Mon Sep 17 00:00:00 2001 From: Dale Wahl Date: Thu, 16 Jul 2026 14:51:39 +0200 Subject: [PATCH 09/20] user_input: warn on bad default _before_ converted to user input --- common/lib/user_input.py | 30 +++++++++++++++++++++++------- 1 file changed, 23 insertions(+), 7 deletions(-) diff --git a/common/lib/user_input.py b/common/lib/user_input.py index 395722bef..4ad4922ea 100644 --- a/common/lib/user_input.py +++ b/common/lib/user_input.py @@ -147,7 +147,23 @@ def parse_all(options, input, silently_correct=False, log=None): # ignored continue - elif settings.get("type") == UserInput.OPTION_DATERANGE: + # an option's default is set by us, not by the user, so an invalid + # one is a bug in the option definition and the user must not pay + # for it. the form pre-fills defaults, which means a bad default + # comes back looking exactly like a value the user typed and would + # otherwise be reported to them as their mistake. so: warn + # developers, and parse this one option leniently, which quietly + # corrects the bad value instead of rejecting it. + default_problem = UserInput.validate_default(settings) + if default_problem: + if log: + log.warning("Option '%s' has an invalid default (%s); correcting it quietly. " + "Please fix the option definition." % (option, default_problem)) + option_silently_correct = True + else: + option_silently_correct = silently_correct + + if settings.get("type") == UserInput.OPTION_DATERANGE: # special case, since it combines two inputs option_min = option + "-min" option_max = option + "-max" @@ -162,11 +178,11 @@ def parse_all(options, input, silently_correct=False, log=None): # 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)) + after, before = (UserInput.parse_value(settings, input.get(option_min), parsed_input, option_silently_correct), UserInput.parse_value(settings, input.get(option_max), parsed_input, option_silently_correct)) if before and after and after > before: - if not silently_correct: - raise QueryParametersException("End of date range must be after beginning of date range.") + if not option_silently_correct: + raise QueryParametersException("the start of the range must be before its end.") else: before = after @@ -184,7 +200,7 @@ def parse_all(options, input, silently_correct=False, log=None): try: if option in input: # Toggle needs to be parsed - parsed_input[option] = UserInput.parse_value(settings, input[option], parsed_input, silently_correct) + parsed_input[option] = UserInput.parse_value(settings, input[option], parsed_input, option_silently_correct) else: # Toggle was left blank parsed_input[option] = False @@ -222,7 +238,7 @@ def parse_all(options, input, silently_correct=False, log=None): 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=silently_correct) + table_input[datasource][column] = UserInput.parse_value(column_settings, choice, table_input, silently_correct=option_silently_correct) parsed_input[option] = table_input @@ -245,7 +261,7 @@ def parse_all(options, input, silently_correct=False, log=None): if input_index not in input_items: input_items[input_index] = {} - input_items[input_index][option_item] = UserInput.parse_value(item_options[option_item], value, input_items[input_index], silently_correct) + input_items[input_index][option_item] = UserInput.parse_value(item_options[option_item], value, input_items[input_index], option_silently_correct) # discard items that are only default values parsed_input[option] = [] From d5e4ea9897675854d9c8c8eb940879bab8940dce Mon Sep 17 00:00:00 2001 From: Dale Wahl Date: Thu, 16 Jul 2026 15:33:11 +0200 Subject: [PATCH 10/20] revert: still warn, but do not silently correct (better than user value being overwritten) --- common/lib/user_input.py | 47 ++++++++++++++++------------------------ tests/test_modules.py | 18 ++++++++------- 2 files changed, 29 insertions(+), 36 deletions(-) diff --git a/common/lib/user_input.py b/common/lib/user_input.py index 4ad4922ea..379691147 100644 --- a/common/lib/user_input.py +++ b/common/lib/user_input.py @@ -148,20 +148,17 @@ def parse_all(options, input, silently_correct=False, log=None): continue # an option's default is set by us, not by the user, so an invalid - # one is a bug in the option definition and the user must not pay - # for it. the form pre-fills defaults, which means a bad default - # comes back looking exactly like a value the user typed and would - # otherwise be reported to them as their mistake. so: warn - # developers, and parse this one option leniently, which quietly - # corrects the bad value instead of rejecting it. + # 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: - if log: - log.warning("Option '%s' has an invalid default (%s); correcting it quietly. " - "Please fix the option definition." % (option, default_problem)) - option_silently_correct = True - else: - option_silently_correct = silently_correct + 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 @@ -178,10 +175,10 @@ def parse_all(options, input, silently_correct=False, log=None): # save as a tuple of unix timestamps (or None) try: - after, before = (UserInput.parse_value(settings, input.get(option_min), parsed_input, option_silently_correct), UserInput.parse_value(settings, input.get(option_max), parsed_input, option_silently_correct)) + 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 option_silently_correct: + if not silently_correct: raise QueryParametersException("the start of the range must be before its end.") else: before = after @@ -200,7 +197,7 @@ def parse_all(options, input, silently_correct=False, log=None): try: if option in input: # Toggle needs to be parsed - parsed_input[option] = UserInput.parse_value(settings, input[option], parsed_input, option_silently_correct) + parsed_input[option] = UserInput.parse_value(settings, input[option], parsed_input, silently_correct) else: # Toggle was left blank parsed_input[option] = False @@ -238,7 +235,7 @@ def parse_all(options, input, silently_correct=False, log=None): 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=option_silently_correct) + table_input[datasource][column] = UserInput.parse_value(column_settings, choice, table_input, silently_correct=silently_correct) parsed_input[option] = table_input @@ -261,7 +258,7 @@ def parse_all(options, input, silently_correct=False, log=None): if input_index not in input_items: input_items[input_index] = {} - input_items[input_index][option_item] = UserInput.parse_value(item_options[option_item], value, input_items[input_index], option_silently_correct) + input_items[input_index][option_item] = UserInput.parse_value(item_options[option_item], value, input_items[input_index], silently_correct) # discard items that are only default values parsed_input[option] = [] @@ -282,20 +279,14 @@ def parse_all(options, input, silently_correct=False, log=None): 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 the default. an invalid one was warned - # about above; parsing it leniently corrects it quietly - if default_problem: - try: - parsed_input[option] = UserInput.parse_value(settings, settings.get("default"), parsed_input, silently_correct=True) - except RequirementsNotMetException: - pass - else: - parsed_input[option] = settings.get("default", None) + # not provided? use the default (an invalid one was warned + # about above, but is not silently replaced with something else) + parsed_input[option] = settings.get("default", None) else: # normal parsing and sanitisation try: - parsed_input[option] = UserInput.parse_value(settings, input[option], parsed_input, option_silently_correct) + parsed_input[option] = UserInput.parse_value(settings, input[option], parsed_input, silently_correct) except RequirementsNotMetException: pass except QueryParametersException as e: diff --git a/tests/test_modules.py b/tests/test_modules.py index d1004488d..8531546e2 100644 --- a/tests/test_modules.py +++ b/tests/test_modules.py @@ -327,14 +327,16 @@ def test_option_keys_known(logger, fourcat_modules, mock_job, mock_job_queue, mo logger.info("All option keys are recognised.") -def test_parse_all_bad_default_is_not_blamed_on_the_user(): +def test_parse_all_warns_about_a_bad_default_but_does_not_correct_it(): """ - The form pre-fills an option's default, so a bad default is submitted back - looking exactly like a value the user typed. That is our mistake, not - theirs: it must be corrected quietly and logged for developers, never - raised as an error at the user. + 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, @@ -346,10 +348,10 @@ def test_parse_all_bad_default_is_not_blamed_on_the_user(): }} log = MagicMock() - # the form submits the pre-filled (bad) default back to us - parsed = UserInput.parse_all(options, {"option-threshold": "27.0"}, silently_correct=False, log=log) + # 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 parsed["threshold"] == 5, "a bad default should be corrected, not rejected" assert log.warning.called, "developers should be warned about the bad default" From 3a0676e7c53cadf87b9058162215120b52176e33 Mon Sep 17 00:00:00 2001 From: Dale Wahl Date: Thu, 16 Jul 2026 15:50:23 +0200 Subject: [PATCH 11/20] remove_author_info: itertools.chain is a ONE use iterator (make it a list) --- processors/conversion/remove_author_info.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) 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, From 7f63546977e39ca0dc4a7a226bdd6da2b0e2fdf7 Mon Sep 17 00:00:00 2001 From: Dale Wahl Date: Thu, 16 Jul 2026 15:51:35 +0200 Subject: [PATCH 12/20] fix bad defaults --- processors/filtering/column_filter.py | 2 +- processors/machine_learning/clarifai_api.py | 6 ++++-- processors/text-analysis/collocations.py | 2 +- processors/text-analysis/similar_words.py | 2 +- processors/visualisation/histwords.py | 7 +++++-- processors/visualisation/rankflow.py | 2 +- processors/visualisation/video_frames.py | 2 +- processors/visualisation/video_scene_frames.py | 2 +- processors/visualisation/video_scene_identifier.py | 2 +- 9 files changed, 16 insertions(+), 11 deletions(-) diff --git a/processors/filtering/column_filter.py b/processors/filtering/column_filter.py index c681f3ca7..f06a93de7 100644 --- a/processors/filtering/column_filter.py +++ b/processors/filtering/column_filter.py @@ -60,7 +60,7 @@ def get_options(cls, parent_dataset=None, config=None) -> dict: "top-top": "is in the top n results for this attribute", "top-bottom": "is in the bottom n results for this attribute" }, - "default": "exact" + "default": "value-equals" }, "strict-top": { "type": UserInput.OPTION_TOGGLE, 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/text-analysis/collocations.py b/processors/text-analysis/collocations.py index 147608334..e777dc499 100644 --- a/processors/text-analysis/collocations.py +++ b/processors/text-analysis/collocations.py @@ -40,7 +40,7 @@ def get_options(cls, parent_dataset=None, config=None) -> dict: return { "n_size": { "type": UserInput.OPTION_CHOICE, - "default": 2, + "default": "2", "options": { "2": "2 (bigrams)", "3": "3 (trigrams)"}, diff --git a/processors/text-analysis/similar_words.py b/processors/text-analysis/similar_words.py index 7db6573c6..ad8f7db8a 100644 --- a/processors/text-analysis/similar_words.py +++ b/processors/text-analysis/similar_words.py @@ -63,7 +63,7 @@ def get_options(cls, parent_dataset=None, config=None) -> dict: }, "crawl_depth": { "type": UserInput.OPTION_CHOICE, - "default": 1, + "default": "1", "options": {"1": 1, "2": 2, "3": 3}, "help": "The crawl depth. 1 only gets the neighbours of the input word(s), 2 also their neighbours, etc." } diff --git a/processors/visualisation/histwords.py b/processors/visualisation/histwords.py index 1fbe4302c..93846d0cd 100644 --- a/processors/visualisation/histwords.py +++ b/processors/visualisation/histwords.py @@ -78,7 +78,7 @@ def get_options(cls, parent_dataset=None, config=None) -> dict: "PCA": "PCA", "TruncatedSVD": "Truncated SVD (randomised, 5 iterations)" }, - "default": "tsne" + "default": "t-SNE" }, "num-words": { "type": UserInput.OPTION_TEXT, @@ -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/rankflow.py b/processors/visualisation/rankflow.py index 99397321d..09f007dff 100644 --- a/processors/visualisation/rankflow.py +++ b/processors/visualisation/rankflow.py @@ -107,7 +107,7 @@ def get_options(cls, parent_dataset=None, config=None) -> dict: "weight": "Value (items with a higher value are bigger)", "none": "None (same size for all elements)", }, - "default": "change", + "default": "weight", "help": "Size according to", }, "only_adjacent_flows": { diff --git a/processors/visualisation/video_frames.py b/processors/visualisation/video_frames.py index 9a24621fa..8f0cc7fc1 100644 --- a/processors/visualisation/video_frames.py +++ b/processors/visualisation/video_frames.py @@ -58,7 +58,7 @@ def get_options(cls, parent_dataset=None, config=None) -> dict: }, "frame_size": { "type": UserInput.OPTION_CHOICE, - "default": "medium", + "default": "432x432", "options": { "no_modify": "Do not modify", "144x144": "Tiny (144x144)", diff --git a/processors/visualisation/video_scene_frames.py b/processors/visualisation/video_scene_frames.py index 65e136fee..b6bf50ebc 100644 --- a/processors/visualisation/video_scene_frames.py +++ b/processors/visualisation/video_scene_frames.py @@ -51,7 +51,7 @@ def get_options(cls, parent_dataset=None, config=None) -> dict: return { "frame_size": { "type": UserInput.OPTION_CHOICE, - "default": "medium", + "default": "432x432", "options": { "no_modify": "Do not modify", "144x144": "Tiny (144x144)", diff --git a/processors/visualisation/video_scene_identifier.py b/processors/visualisation/video_scene_identifier.py index 7fa6c03b8..a4003aa5a 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": { From 5634bbbd9bef8db28db0baba0450f3e4b5258933 Mon Sep 17 00:00:00 2001 From: Dale Wahl Date: Thu, 16 Jul 2026 15:52:06 +0200 Subject: [PATCH 13/20] "hidden_in_explorer" should be "hide_in_explorer" --- processors/visualisation/video_scene_identifier.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/processors/visualisation/video_scene_identifier.py b/processors/visualisation/video_scene_identifier.py index a4003aa5a..4978b14c6 100644 --- a/processors/visualisation/video_scene_identifier.py +++ b/processors/visualisation/video_scene_identifier.py @@ -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 } From afc3f5bbf521857660fad77efb06a97db6dde01e Mon Sep 17 00:00:00 2001 From: Dale Wahl Date: Thu, 16 Jul 2026 15:52:30 +0200 Subject: [PATCH 14/20] video_scene_identifier: options use keys not values --- processors/visualisation/video_scene_identifier.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/processors/visualisation/video_scene_identifier.py b/processors/visualisation/video_scene_identifier.py index 4978b14c6..0f7ea158e 100644 --- a/processors/visualisation/video_scene_identifier.py +++ b/processors/visualisation/video_scene_identifier.py @@ -203,7 +203,9 @@ def get_options(cls, parent_dataset=None, config=None) -> dict: ffmpeg_path = config.get("video-downloader.ffmpeg_path") if not ffmpeg_path or not os.path.exists(shutil.which(ffmpeg_path)): del options["detector_type"]["options"]["ffmpeg_select"] - options["detector_type"]["default"] = list(options["detector_type"]["options"].values())[0] + # the default must be one of the option's keys, not one of the + # labels shown next to them + options["detector_type"]["default"] = list(options["detector_type"]["options"].keys())[0] return options From 4f0b3d039f7296d5fd0a4bc929fbad15664a8285 Mon Sep 17 00:00:00 2001 From: Dale Wahl Date: Thu, 16 Jul 2026 15:53:23 +0200 Subject: [PATCH 15/20] image_wall: image uses different default than video --- processors/visualisation/image_wall.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/processors/visualisation/image_wall.py b/processors/visualisation/image_wall.py index ebbb25f2b..1547ec7c1 100644 --- a/processors/visualisation/image_wall.py +++ b/processors/visualisation/image_wall.py @@ -72,6 +72,10 @@ def get_options(cls, parent_dataset=None, config=None): "kmeans-dominant": "Dominant K-means (precise, slow)", "average-hsv": "Average colour (HSV; imprecise, fastest)", } + # the inherited default sorts by video length, which is not one of + # these, so the interface silently fell back to the first option; + # say so explicitly instead + options["sort-mode"]["default"] = "" else: # add some caveats for running this directly on a video dataset options["sort-mode"]["tooltip"] = ("To sort by e.g. average colour, first extract frames as images and " From 92a3e6e2ccd990a6be012fbf69f5cb74728a09d0 Mon Sep 17 00:00:00 2001 From: Dale Wahl Date: Thu, 16 Jul 2026 15:53:57 +0200 Subject: [PATCH 16/20] random_filter: validate_query not needed if get_option is fixed --- processors/filtering/random_filter.py | 24 ++++-------------------- 1 file changed, 4 insertions(+), 20 deletions(-) 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 From 40b642805fe6809c81ad7777642e3fa41c3a8723 Mon Sep 17 00:00:00 2001 From: Dale Wahl Date: Fri, 17 Jul 2026 11:06:55 +0200 Subject: [PATCH 17/20] add comments to all known option keys --- common/lib/user_input.py | 47 +++++++++++++++++++++++++++++++--------- 1 file changed, 37 insertions(+), 10 deletions(-) diff --git a/common/lib/user_input.py b/common/lib/user_input.py index 8f02655fc..43d6fa1e6 100644 --- a/common/lib/user_input.py +++ b/common/lib/user_input.py @@ -48,16 +48,43 @@ class UserInput: # "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", "default", "options", "requires", "min", "max", "coerce_type", - "indirect", "delegated", "dict_key", "columns", "sensitive", - # read by the interface templates - "help", "tooltip", "label", "inline", "original_default", "value", - "saturation", "accept", "update", "password", "multiple", "cache", - # annotation options - "to_parent", "hide_in_explorer", - # datasource options - "board_specific", + # -- 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 + + # -- 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 From 75b39c53e13b90de6a620d41a3158a2f2692121b Mon Sep 17 00:00:00 2001 From: Dale Wahl Date: Fri, 17 Jul 2026 11:29:54 +0200 Subject: [PATCH 18/20] add mandatory user option flag/tag --- common/lib/user_input.py | 33 +++++ tests/test_user_input_mandatory.py | 212 +++++++++++++++++++++++++++++ 2 files changed, 245 insertions(+) create mode 100644 tests/test_user_input_mandatory.py diff --git a/common/lib/user_input.py b/common/lib/user_input.py index 43d6fa1e6..692af5962 100644 --- a/common/lib/user_input.py +++ b/common/lib/user_input.py @@ -61,6 +61,7 @@ class UserInput: "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 @@ -206,6 +207,9 @@ def parse_all(options, input, silently_correct=False, log=None): 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)) @@ -279,6 +283,10 @@ def parse_all(options, input, silently_correct=False, log=None): # 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]) @@ -319,6 +327,9 @@ def parse_all(options, input, silently_correct=False, log=None): 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 @@ -329,6 +340,26 @@ def parse_all(options, input, silently_correct=False, log=None): 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): """ @@ -519,6 +550,8 @@ def parse_value(settings, choice, other_input=None, silently_correct=False): # callers), so accept both shapes. (OPTION_MULTI used to assume a # string and crashed on a real list.) if not choice: + if settings.get("mandatory"): + raise QueryParametersException("This field is required.") return settings.get("default", []) if type(choice) is str: diff --git a/tests/test_user_input_mandatory.py b/tests/test_user_input_mandatory.py new file mode 100644 index 000000000..92a082b60 --- /dev/null +++ b/tests/test_user_input_mandatory.py @@ -0,0 +1,212 @@ +""" +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_default_does_not_raise(self): + """ + If the option was never submitted and has no default, mandatory should + not raise. + """ + options = { + "query": { + "type": UserInput.OPTION_TEXT, + "help": "Search query", + "mandatory": True, + } + } + parsed = UserInput.parse_all(options, {}) + assert parsed == {"query": None} + + def test_mandatory_text_with_default_missing_input_does_not_raise(self): + """ + If the option was not submitted but has a default, the default is used. + """ + options = { + "query": { + "type": UserInput.OPTION_TEXT, + "help": "Search query", + "default": "default query", + "mandatory": True, + } + } + parsed = UserInput.parse_all(options, {}) + assert parsed == {"query": "default query"} + + def test_non_mandatory_text_empty_is_allowed(self): + """ + Without mandatory=True, an empty value should fall back to the default. + """ + options = { + "query": { + "type": UserInput.OPTION_TEXT, + "help": "Search query", + "default": "default query", + } + } + parsed = UserInput.parse_all(options, {"query": ""}) + assert parsed == {"query": "default query"} + + 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_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" From 08c6a1f389f3fe33ce91ff024501d289d3c1c7c5 Mon Sep 17 00:00:00 2001 From: Dale Wahl Date: Fri, 17 Jul 2026 13:03:55 +0200 Subject: [PATCH 19/20] options mandatory check: add missing multi-select and update test --- common/lib/user_input.py | 8 ++++++++ tests/test_user_input_mandatory.py | 31 +++++++++++++++++++++++++----- 2 files changed, 34 insertions(+), 5 deletions(-) diff --git a/common/lib/user_input.py b/common/lib/user_input.py index 692af5962..2b5cbf17c 100644 --- a/common/lib/user_input.py +++ b/common/lib/user_input.py @@ -324,6 +324,14 @@ def parse_all(options, input, silently_correct=False, log=None): # about above, but is not silently replaced with something else) parsed_input[option] = settings.get("default", None) + # a mandatory option that was not submitted at all is only + # satisfied if its default fills it in. this is the shape an + # unselected multi-select arrives in: like an unchecked + # checkbox, the form leaves the field out altogether rather + # than sending it empty + if settings.get("mandatory") and UserInput.is_empty_value(parsed_input[option]): + raise QueryParametersException("%s: This field is required." % (settings.get("help") or option)) + else: # normal parsing and sanitisation try: diff --git a/tests/test_user_input_mandatory.py b/tests/test_user_input_mandatory.py index 92a082b60..7d8aca844 100644 --- a/tests/test_user_input_mandatory.py +++ b/tests/test_user_input_mandatory.py @@ -59,10 +59,12 @@ def test_mandatory_text_accepts_value(self): parsed = UserInput.parse_all(options, {"query": "hello"}) assert parsed == {"query": "hello"} - def test_mandatory_text_missing_default_does_not_raise(self): + def test_mandatory_text_missing_raises(self): """ - If the option was never submitted and has no default, mandatory should - not raise. + 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": { @@ -71,8 +73,8 @@ def test_mandatory_text_missing_default_does_not_raise(self): "mandatory": True, } } - parsed = UserInput.parse_all(options, {}) - assert parsed == {"query": None} + with pytest.raises(QueryParametersException): + UserInput.parse_all(options, {}) def test_mandatory_text_with_default_missing_input_does_not_raise(self): """ @@ -118,6 +120,25 @@ def test_mandatory_multi_raises_when_empty(self): 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_accepts_value(self): """ A mandatory multi-select with at least one selected value should parse. From 22f611a29e562a5dc4518cbca1923dc78cf0f6ce Mon Sep 17 00:00:00 2001 From: Dale Wahl Date: Fri, 17 Jul 2026 15:56:24 +0200 Subject: [PATCH 20/20] user_input: never fill in a value the user did not send A default is the form's starting value. It was also being filled back in while parsing, which meant a value the user never sent could pass the checks that follow. Three ways in: an option missing from the submission, a cleared text field, and an unticked multi-select all quietly became the default again. Parsing strictly now says so instead: - a missing option raises, naming it, since our own form submits every field it shows. Options a form legitimately leaves out stay exempt: unchecked toggles, file uploads (sent outside the form values), and fields gated by an unmet requires or board_specific condition. - a cleared text field stays empty; a cleared numeric field raises, as there is no honest empty number. - unticking every option of a multi-select means an empty selection. A browser leaves a select with nothing selected out of the submission altogether, which is indistinguishable from the field never having been shown, so the form now submits an empty marker alongside it; parsing discards the marker. The chan scope fields were kept out of submissions by the form disabling them, which is a hand-rolled "requires"; they now declare it instead. Parsing tolerantly (silently_correct) still fills the default in, so internal callers that ask to be corrected quietly are unaffected. --- common/lib/user_input.py | 99 ++++++++++--- datasources/eightchan/search_8chan.py | 6 +- datasources/eightkun/search_8kun.py | 6 +- datasources/fourchan/search_4chan.py | 22 ++- tests/test_modules.py | 10 +- tests/test_user_input_mandatory.py | 137 +++++++++++++++++- .../components/datasource-option.html | 7 + .../components/processor-option.html | 7 + 8 files changed, 250 insertions(+), 44 deletions(-) diff --git a/common/lib/user_input.py b/common/lib/user_input.py index 2b5cbf17c..3bada4489 100644 --- a/common/lib/user_input.py +++ b/common/lib/user_input.py @@ -111,11 +111,23 @@ def parse_all(options, input, silently_correct=False, log=None): 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 @@ -320,18 +332,41 @@ def parse_all(options, input, silently_correct=False, log=None): 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 the default (an invalid one was warned - # about above, but is not silently replaced with something else) - parsed_input[option] = settings.get("default", None) - - # a mandatory option that was not submitted at all is only - # satisfied if its default fills it in. this is the shape an - # unselected multi-select arrives in: like an unchecked - # checkbox, the form leaves the field out altogether rather - # than sending it empty - if settings.get("mandatory") and UserInput.is_empty_value(parsed_input[option]): + # 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: @@ -557,13 +592,25 @@ def parse_value(settings, choice, other_input=None, silently_correct=False): # 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: + choice = choice.split(",") + + # 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.") - return settings.get("default", []) - - if type(choice) is str: - choice = choice.split(",") + 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] @@ -624,10 +671,20 @@ def parse_value(settings, choice, other_input=None, silently_correct=False): number_type = None if choice is None or choice == "": - # no value given: fall back to the default - choice = settings.get("default") - if choice is None: - choice = 0 if has_range else "" + # 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 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. 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/tests/test_modules.py b/tests/test_modules.py index 7577804b6..c08ecfa03 100644 --- a/tests/test_modules.py +++ b/tests/test_modules.py @@ -641,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 index 7d8aca844..37cfd60f8 100644 --- a/tests/test_user_input_mandatory.py +++ b/tests/test_user_input_mandatory.py @@ -76,9 +76,12 @@ def test_mandatory_text_missing_raises(self): with pytest.raises(QueryParametersException): UserInput.parse_all(options, {}) - def test_mandatory_text_with_default_missing_input_does_not_raise(self): + def test_mandatory_text_with_default_missing_input_raises(self): """ - If the option was not submitted but has a default, the default is used. + 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": { @@ -88,12 +91,17 @@ def test_mandatory_text_with_default_missing_input_does_not_raise(self): "mandatory": True, } } - parsed = UserInput.parse_all(options, {}) - assert parsed == {"query": "default query"} + with pytest.raises(QueryParametersException): + UserInput.parse_all(options, {}) - def test_non_mandatory_text_empty_is_allowed(self): + def test_missing_option_raises_unless_correcting_silently(self): """ - Without mandatory=True, an empty value should fall back to the default. + 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": { @@ -102,8 +110,68 @@ def test_non_mandatory_text_empty_is_allowed(self): "default": "default query", } } - parsed = UserInput.parse_all(options, {"query": ""}) - assert parsed == {"query": "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): """ @@ -139,6 +207,59 @@ def test_mandatory_multi_raises_when_nothing_selected(self): 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. 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 #} +