User input refactor and option bug fixes

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"
             }
         }

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"
             }
         }

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
 

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,

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

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": {

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,

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
 			}

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