From 2076de307463b9c659dab36c6e61b3531f9815c6 Mon Sep 17 00:00:00 2001 From: Jesse Bannon Date: Wed, 10 Jan 2024 18:16:12 -0800 Subject: [PATCH] [BUGFIX] Smarter subscription validation (#895) With the right functions, it was possible for subscription validation to raise a false-positive error. This should hopefully resolve that issue and give validation a small performance increase --- .../config/validators/variable_validation.py | 38 +++++++----- src/ytdl_sub/script/script.py | 53 ++++++++++++++++ src/ytdl_sub/utils/scriptable.py | 8 +-- .../validators/string_formatter_validators.py | 62 ++++++++++++++++--- tests/unit/config/test_preset.py | 28 +++++++-- 5 files changed, 157 insertions(+), 32 deletions(-) diff --git a/src/ytdl_sub/config/validators/variable_validation.py b/src/ytdl_sub/config/validators/variable_validation.py index cc554a70..f5de3ed5 100644 --- a/src/ytdl_sub/config/validators/variable_validation.py +++ b/src/ytdl_sub/config/validators/variable_validation.py @@ -13,9 +13,10 @@ from ytdl_sub.config.plugin.preset_plugins import PresetPlugins from ytdl_sub.config.preset_options import OutputOptions from ytdl_sub.config.validators.options import OptionsValidator from ytdl_sub.downloaders.url.validators import MultiUrlValidator -from ytdl_sub.entries.script.variable_definitions import VARIABLE_SCRIPTS from ytdl_sub.entries.variables.override_variables import SubscriptionVariables from ytdl_sub.script.script import Script +from ytdl_sub.utils.scriptable import BASE_SCRIPT +from ytdl_sub.validators.string_formatter_validators import to_variable_dependency_format_string from ytdl_sub.validators.string_formatter_validators import validate_formatters @@ -32,10 +33,10 @@ def _add_dummy_overrides(overrides: Overrides) -> Dict[str, str]: # Have the dummy override variable contain all variable deps that it uses in the string dummy_overrides: Dict[str, str] = {} for override_name in _override_variables(overrides): - dummy_overrides[override_name] = "" # pylint: disable=protected-access - for variable_dependency in overrides.script._variables[override_name].variables: - dummy_overrides[override_name] += f"{{ {variable_dependency.name } }}" + dummy_overrides[override_name] = to_variable_dependency_format_string( + script=overrides.script, parsed_format_string=overrides.script._variables[override_name] + ) # pylint: enable=protected-access return dummy_overrides @@ -72,8 +73,15 @@ def _override_variables(overrides: Overrides) -> Set[str]: } -def _entry_variables() -> Set[str]: - return set(list(VARIABLE_SCRIPTS.keys())) +_DUMMY_ENTRY_VARIABLES: Dict[str, str] = { + name: to_variable_dependency_format_string( + # pylint: disable=protected-access + script=BASE_SCRIPT, + parsed_format_string=BASE_SCRIPT._variables[name] + # pylint: enable=protected-access + ) + for name in BASE_SCRIPT.variable_names +} class VariableValidation: @@ -97,12 +105,12 @@ class VariableValidation: """ Do some gymnastics to initialize the Overrides script. """ - entry_variables = _entry_variables() override_variables = _override_variables(overrides) # Set resolved variables as all entry + override variables # at this point to generate every possible added/modified variable - self.resolved_variables = entry_variables | override_variables + self.resolved_variables = set(_DUMMY_ENTRY_VARIABLES.keys()) | override_variables + plugin_variables: Set[str] = set() for ( plugin_options, @@ -125,6 +133,7 @@ class VariableValidation: # Set unresolved as variables that are added but do not exist as # entry/override variables since they are created at run-time self.unresolved_variables |= added_variables | modified_variables + plugin_variables |= added_variables | modified_variables # Then update resolved variables to reflect that self.resolved_variables -= self.unresolved_variables @@ -137,10 +146,11 @@ class VariableValidation: ) # copy the script and mock entry variables - self.script = copy.deepcopy(overrides.script).add(_add_dummy_variables(entry_variables)) + self.script = copy.deepcopy(overrides.script) self.script.add( - variables=_add_dummy_overrides(overrides=overrides), - unresolvable=self.unresolved_variables, + variables=_add_dummy_overrides(overrides=overrides) + | _add_dummy_variables(variables=plugin_variables) + | _DUMMY_ENTRY_VARIABLES ) return self @@ -158,7 +168,6 @@ class VariableValidation: resolved_variables = added_variables | modified_variables - self.script.add(_add_dummy_variables(resolved_variables)) self.resolved_variables |= resolved_variables self.unresolved_variables -= resolved_variables @@ -177,13 +186,10 @@ class VariableValidation: ): self._add_variables(PluginOperation.MODIFY_ENTRY_METADATA, options=plugin_options) - self._update_script() for plugin_options in PluginMapping.order_options_by( self.plugins.zipped(), PluginOperation.MODIFY_ENTRY ): - added = self._add_variables(PluginOperation.MODIFY_ENTRY, options=plugin_options) - if added: - self._update_script() + self._add_variables(PluginOperation.MODIFY_ENTRY, options=plugin_options) # Validate that any formatter in the plugin options can resolve validate_formatters( diff --git a/src/ytdl_sub/script/script.py b/src/ytdl_sub/script/script.py index 0560984b..3f3b42ff 100644 --- a/src/ytdl_sub/script/script.py +++ b/src/ytdl_sub/script/script.py @@ -248,6 +248,52 @@ class Script: for variable_name, resolved in resolved_variables.items(): self._variables[variable_name] = SyntaxTree(ast=[resolved]) + def _recursive_get_unresolved_output_filter_variables( + self, current_var: SyntaxTree, subset_to_resolve: Set[str], unresolvable: Set[Variable] + ) -> Set[str]: + for var_dep in current_var.variables: + if var_dep in unresolvable: + raise ScriptVariableNotResolved( + f"Output filter variable contains the variable {var_dep} " + f"which is set as unresolvable" + ) + subset_to_resolve.add(var_dep.name) + subset_to_resolve |= self._recursive_get_unresolved_output_filter_variables( + current_var=self._variables[var_dep.name], + subset_to_resolve=subset_to_resolve, + unresolvable=unresolvable, + ) + + return subset_to_resolve + + def _get_unresolved_output_filter( + self, + unresolved: Dict[Variable, SyntaxTree], + output_filter: Set[str], + unresolvable: Set[Variable], + ) -> Dict[Variable, SyntaxTree]: + """ + When an output filter is applied, only a subset of variables that the filter + depends on need to be resolved. + """ + subset_to_resolve: Set[str] = set() + + for output_filter_variable in output_filter: + subset_to_resolve.add(output_filter_variable) + + if output_filter_variable not in self._variables: + raise ScriptVariableNotResolved( + "Tried to specify an output filter variable that does not exist" + ) + + subset_to_resolve |= self._recursive_get_unresolved_output_filter_variables( + current_var=self._variables[output_filter_variable], + subset_to_resolve=subset_to_resolve, + unresolvable=unresolvable, + ) + + return {var: syntax for var, syntax in unresolved.items() if var.name in subset_to_resolve} + def _resolve( self, pre_resolved: Optional[Dict[str, Resolvable]] = None, @@ -288,6 +334,13 @@ class Script: if Variable(name) not in unresolved_filter } + if output_filter: + unresolved = self._get_unresolved_output_filter( + unresolved=unresolved, + output_filter=output_filter, + unresolvable=unresolvable, + ) + while unresolved: unresolved_count: int = len(unresolved) diff --git a/src/ytdl_sub/utils/scriptable.py b/src/ytdl_sub/utils/scriptable.py index b1dc20e7..6aff1db3 100644 --- a/src/ytdl_sub/utils/scriptable.py +++ b/src/ytdl_sub/utils/scriptable.py @@ -14,10 +14,8 @@ from ytdl_sub.script.utils.exceptions import RuntimeException from ytdl_sub.utils.exceptions import StringFormattingException from ytdl_sub.utils.script import ScriptUtils -_BASE_SCRIPT: Script = Script( - ScriptUtils.add_sanitized_variables( - dict(copy.deepcopy(VARIABLE_SCRIPTS), **copy.deepcopy(CUSTOM_FUNCTION_SCRIPTS)) - ) +BASE_SCRIPT: Script = Script( + dict(ScriptUtils.add_sanitized_variables(VARIABLE_SCRIPTS), **CUSTOM_FUNCTION_SCRIPTS) ) @@ -37,7 +35,7 @@ class Scriptable(ABC): """ Initializes with base values """ - self._script = copy.deepcopy(_BASE_SCRIPT) + self._script = copy.deepcopy(BASE_SCRIPT) self._unresolvable = copy.deepcopy(UNRESOLVED_VARIABLES) @property diff --git a/src/ytdl_sub/validators/string_formatter_validators.py b/src/ytdl_sub/validators/string_formatter_validators.py index 7957169d..def6e6d8 100644 --- a/src/ytdl_sub/validators/string_formatter_validators.py +++ b/src/ytdl_sub/validators/string_formatter_validators.py @@ -6,8 +6,10 @@ from typing import final from ytdl_sub.entries.script.variable_definitions import VARIABLES from ytdl_sub.script.parser import parse from ytdl_sub.script.script import Script +from ytdl_sub.script.types.syntax_tree import SyntaxTree +from ytdl_sub.script.utils.exceptions import RuntimeException +from ytdl_sub.script.utils.exceptions import ScriptVariableNotResolved from ytdl_sub.script.utils.exceptions import UserException -from ytdl_sub.script.utils.exceptions import VariableDoesNotExist from ytdl_sub.utils.exceptions import StringFormattingVariableNotFoundException from ytdl_sub.validators.validators import DictValidator from ytdl_sub.validators.validators import ListValidator @@ -142,21 +144,67 @@ class OverridesDictFormatterValidator(DictFormatterValidator): _key_validator = OverridesStringFormatterValidator +def to_variable_dependency_format_string(script: Script, parsed_format_string: SyntaxTree) -> str: + """ + Create a dummy format string that contains all variable deps as a string. + """ + dummy_format_string = "" + for var in parsed_format_string.variables: + dummy_format_string += f"{{ {var.name} }}" + # pylint: disable=protected-access + for variable_dependency in script._variables[var.name].variables: + dummy_format_string += f"{{ {variable_dependency.name} }}" + # pylint: enable=protected-access + return dummy_format_string + + def _validate_formatter( mock_script: Script, unresolved_variables: Set[str], formatter_validator: Union[StringFormatterValidator, OverridesStringFormatterValidator], ) -> None: - try: - unresolvable = unresolved_variables - if isinstance(formatter_validator, OverridesStringFormatterValidator): - unresolvable = unresolved_variables.union({VARIABLES.entry_metadata.variable_name}) + is_static_formatter = False + unresolvable = unresolved_variables + if isinstance(formatter_validator, OverridesStringFormatterValidator): + is_static_formatter = True + unresolvable = unresolved_variables.union({VARIABLES.entry_metadata.variable_name}) + parsed = parse( + text=formatter_validator.format_string, + ) + variable_names = {var.name for var in parsed.variables} + custom_function_names = {f"%{func.name}" for func in parsed.custom_functions} + + if not variable_names.issubset(mock_script.variable_names): + raise StringFormattingVariableNotFoundException( + "contains the following variables that do not exist: " + f"{', '.join(sorted(variable_names - mock_script.variable_names))}" + ) + if not custom_function_names.issubset(mock_script.function_names): + raise StringFormattingVariableNotFoundException( + "contains the following custom functions that do not exist: " + f"{', '.join(sorted(custom_function_names - mock_script.function_names))}" + ) + if unresolved := variable_names.intersection(unresolvable): + raise StringFormattingVariableNotFoundException( + "contains the following variables that are unresolved when executing this " + f"formatter: {', '.join(sorted(unresolved))}" + ) + try: mock_script.resolve_once( - {"tmp_var": formatter_validator.format_string}, + { + "tmp_var": to_variable_dependency_format_string( + script=mock_script, parsed_format_string=parsed + ) + }, unresolvable=unresolvable, ) - except VariableDoesNotExist as exc: + except RuntimeException as exc: + if isinstance(exc, ScriptVariableNotResolved) and is_static_formatter: + raise StringFormattingVariableNotFoundException( + "static formatters must contain variables that have no dependency to " + "entry variables" + ) from exc raise StringFormattingVariableNotFoundException(exc) from exc diff --git a/tests/unit/config/test_preset.py b/tests/unit/config/test_preset.py index dff51a50..8afedd35 100644 --- a/tests/unit/config/test_preset.py +++ b/tests/unit/config/test_preset.py @@ -129,7 +129,7 @@ class TestPreset: ): with pytest.raises( StringFormattingVariableNotFoundException, - match="Variable dne_var does not exist.", + match="contains the following variables that do not exist: dne_var", ): _ = Preset( config=config_file, @@ -145,7 +145,7 @@ class TestPreset: ): with pytest.raises( StringFormattingVariableNotFoundException, - match="Variable dne_var does not exist", + match="contains the following variables that do not exist: dne_var", ): _ = Preset( config=config_file, @@ -161,7 +161,7 @@ class TestPreset: ): with pytest.raises( StringFormattingVariableNotFoundException, - match="Variable dne_var does not exist", + match="contains the following variables that do not exist: dne_var", ): _ = Preset( config=config_file, @@ -182,7 +182,7 @@ class TestPreset: ): with pytest.raises( StringFormattingVariableNotFoundException, - match="Variable dne_var does not exist", + match="contains the following variables that do not exist: dne_var", ): _ = Preset( config=config_file, @@ -198,6 +198,26 @@ class TestPreset: }, ) + def test_preset_error__dict_override_variable_not_static( + self, config_file, output_options, youtube_video + ): + with pytest.raises( + StringFormattingVariableNotFoundException, + match="static formatters must contain variables that " + "have no dependency to entry variables", + ): + _ = Preset( + config=config_file, + name="test", + value={ + "download": youtube_video, + "output_options": { + "output_directory": "{title}", + "file_name": "{uid}", + }, + }, + ) + def test_preset_with_multi_url__contains_empty_url(self, config_file, output_options): _ = Preset( config=config_file,