diff --git a/src/plugin_system/schema_manager.py b/src/plugin_system/schema_manager.py index 9f5e59fe..0042f75e 100644 --- a/src/plugin_system/schema_manager.py +++ b/src/plugin_system/schema_manager.py @@ -99,8 +99,7 @@ def normalize_legacy_booleans(config: Any, schema: Any, #: config section, so they are allowed in every plugin's config whether or not #: the plugin's schema declares them. The one list for validation, for the web #: save filter and for the load-time checks -- a private copy is how JSON saves -#: came to drop ``skin`` and the ``vegas_*`` keys while the validator accepted -#: them. +#: came to drop the ``vegas_*`` keys while the validator accepted them. #: #: Values are the schema used when the plugin does not declare the property. CORE_PLUGIN_PROPERTIES: Dict[str, Dict[str, Any]] = { @@ -123,19 +122,6 @@ CORE_PLUGIN_PROPERTIES: Dict[str, Dict[str, Any]] = { "default": False, "description": "Enable live priority takeover when plugin has live content" }, - # Skin selection (docs/SKIN_SYSTEM.md). Deliberately NOT an enum here: - # validation must keep passing when a configured skin gets uninstalled - # (rendering falls back to built-in). The install-dependent enum is - # injected only at serve time (inject_skin_selector) for the web UI - # dropdown. - "skin": { - "type": ["string", "object", "null"], - "description": "Visual skin id, or a per-mode mapping like {\"live\": \"my-skin\"}" - }, - "skin_options": { - "type": "object", - "description": "Options passed through to the selected skin" - }, # Vegas tuning read by vegas_mode/plugin_adapter.py and base_plugin.py. # Left untyped: the adapter validates them itself and ignores a bad # value with a log line, so a stored one must never block a save. @@ -158,6 +144,32 @@ CORE_VEGAS_TUNING_KEYS = frozenset({ }) +#: Per-plugin keys the core used to own and no longer reads. ``skin`` and +#: ``skin_options`` belonged to the skin system, which was removed; a +#: config.json written before then can still carry them in any plugin section, +#: and most plugin schemas set ``additionalProperties: false``. They are +#: dropped wherever a section is prepared (prepare_plugin_config) or validated, +#: and the web saves drop them from the stored section, so an old config loads +#: and saves without a validation error and loses them on its next save. +RETIRED_PLUGIN_KEYS = frozenset({'skin', 'skin_options'}) + + +def drop_retired_plugin_keys(config: Any, schema: Any) -> Any: + """``config`` without the RETIRED_PLUGIN_KEYS its plugin's schema leaves undeclared. + + A plugin whose schema declares one of these names owns it and keeps it; + without a schema nothing is dropped. Never mutates ``config``, and returns + it unchanged when there is nothing to drop. + """ + if not isinstance(config, dict) or not isinstance(schema, dict) \ + or RETIRED_PLUGIN_KEYS.isdisjoint(config): + return config + declared = schema.get('properties') + declared = declared if isinstance(declared, dict) else {} + return {key: value for key, value in config.items() + if key not in RETIRED_PLUGIN_KEYS or key in declared} + + def with_core_plugin_properties(schema: Dict[str, Any]) -> Dict[str, Any]: """A deep copy of a plugin schema with CORE_PLUGIN_PROPERTIES allowed. @@ -299,14 +311,15 @@ def prepare_plugin_config(config: Any, schema: Optional[Dict[str, Any]], changed_paths: Optional[List[str]] = None) -> Dict[str, Any]: """The config a plugin runs with, from its stored (or submitted) section. - Legacy booleans are read as ``{"enabled": ...}`` objects - (normalize_legacy_booleans), then schema defaults fill in whatever is - missing. Loading a plugin, both config saves, GET /plugins/config, hot - reload and the dev tools all go through this, so a plugin sees the same - shape however its config reached it. + Retired core keys are dropped (drop_retired_plugin_keys), legacy booleans + are read as ``{"enabled": ...}`` objects (normalize_legacy_booleans), then + schema defaults fill in whatever is missing. Loading a plugin, both config + saves, GET /plugins/config, hot reload and the dev tools all go through + this, so a plugin sees the same shape however its config reached it. """ config = config if isinstance(config, dict) else {} if schema: + config = drop_retired_plugin_keys(config, schema) config = normalize_legacy_booleans(config, schema, changed_paths) return merge_config_defaults(config, defaults) @@ -620,7 +633,8 @@ class SchemaManager: # Core plugin properties (CORE_PLUGIN_PROPERTIES) are handled by # the base plugin system and should not cause validation failures: # they are allowed even when the plugin's schema doesn't declare - # them, and never required. + # them, and never required. Retired ones are ignored. + config = drop_retired_plugin_keys(config, schema) enhanced_schema = with_core_plugin_properties(schema) if plugin_id: declared = schema.get("properties", {}) if isinstance(schema, dict) else {} diff --git a/test/test_retired_plugin_config_keys.py b/test/test_retired_plugin_config_keys.py new file mode 100644 index 00000000..7e7df496 --- /dev/null +++ b/test/test_retired_plugin_config_keys.py @@ -0,0 +1,167 @@ +"""``skin`` and ``skin_options`` outlive the skin system in stored configs. + +The skin system was removed, but a config.json written while it existed can +carry ``skin`` / ``skin_options`` in any plugin section. They used to be core +plugin properties; now they are RETIRED_PLUGIN_KEYS, dropped where a section +is prepared or validated. Most plugin schemas set +``"additionalProperties": false``, so without that a device upgraded with such +a config would flag the plugin degraded on every start. + +The web save paths are covered in +test/web_interface/test_plugin_config_json_saves.py (TestRetiredSkinKeys). +""" + +import json +from unittest.mock import MagicMock, patch + +import pytest + +from src.config_manager import ConfigManager +from src.plugin_system.plugin_manager import PluginManager +from src.plugin_system.schema_manager import ( + CORE_PLUGIN_PROPERTIES, + RETIRED_PLUGIN_KEYS, + SchemaManager, + drop_retired_plugin_keys, +) + +PLUGIN_ID = "scoreboard-shaped" + +STRICT_SCHEMA = { + "type": "object", + "additionalProperties": False, + "properties": { + "enabled": {"type": "boolean", "default": True}, + "favorite_teams": {"type": "array", "items": {"type": "string"}, + "default": []}, + }, +} + +OLD_SECTION = { + "enabled": True, + "favorite_teams": ["TB"], + "skin": {"live": "neon"}, + "skin_options": {"accent": "#ff0000"}, +} + + +def _copy(value): + return json.loads(json.dumps(value)) + + +class TestDropRetiredPluginKeys: + def test_they_are_no_longer_core_properties(self): + assert RETIRED_PLUGIN_KEYS == {"skin", "skin_options"} + assert RETIRED_PLUGIN_KEYS.isdisjoint(CORE_PLUGIN_PROPERTIES) + + def test_drops_both(self): + assert drop_retired_plugin_keys(OLD_SECTION, STRICT_SCHEMA) == { + "enabled": True, "favorite_teams": ["TB"]} + + def test_does_not_mutate_the_section(self): + section = _copy(OLD_SECTION) + drop_retired_plugin_keys(section, STRICT_SCHEMA) + assert section == OLD_SECTION + + def test_returns_the_same_object_when_there_is_nothing_to_drop(self): + section = {"enabled": True} + assert drop_retired_plugin_keys(section, STRICT_SCHEMA) is section + + def test_a_plugin_that_declares_skin_keeps_it(self): + schema = _copy(STRICT_SCHEMA) + schema["properties"]["skin"] = {"type": "string"} + assert drop_retired_plugin_keys(OLD_SECTION, schema) == { + "enabled": True, "favorite_teams": ["TB"], "skin": {"live": "neon"}} + + def test_without_a_schema_nothing_is_dropped(self): + assert drop_retired_plugin_keys(OLD_SECTION, None) is OLD_SECTION + + def test_tolerates_a_non_dict(self): + assert drop_retired_plugin_keys(None, STRICT_SCHEMA) is None + + +@pytest.fixture +def schema_manager(tmp_path): + plugin_dir = tmp_path / "plugins" / PLUGIN_ID + plugin_dir.mkdir(parents=True) + (plugin_dir / "config_schema.json").write_text(json.dumps(STRICT_SCHEMA), + encoding="utf-8") + (plugin_dir / "manifest.json").write_text(json.dumps({"id": PLUGIN_ID}), + encoding="utf-8") + return SchemaManager(plugins_dir=tmp_path / "plugins", project_root=tmp_path) + + +class TestValidation: + def test_the_prepared_section_has_neither_and_validates(self, schema_manager): + prepared = schema_manager.prepare_plugin_config(PLUGIN_ID, _copy(OLD_SECTION)) + assert "skin" not in prepared and "skin_options" not in prepared + assert prepared["favorite_teams"] == ["TB"] + assert schema_manager.validate_config_against_schema( + prepared, STRICT_SCHEMA, PLUGIN_ID) == (True, []) + + def test_the_raw_section_validates_too(self, schema_manager): + assert schema_manager.validate_config_against_schema( + _copy(OLD_SECTION), STRICT_SCHEMA, PLUGIN_ID) == (True, []) + + def test_validate_all_plugin_configs_passes(self, schema_manager, tmp_path): + config_file = tmp_path / "config.json" + config_file.write_text(json.dumps({"timezone": "UTC", PLUGIN_ID: OLD_SECTION}), + encoding="utf-8") + config_manager = ConfigManager(config_path=str(config_file), + secrets_path=str(tmp_path / "secrets.json")) + results = config_manager.validate_all_plugin_configs(schema_manager) + assert results[PLUGIN_ID] == {"valid": True, "errors": []} + + def test_a_real_violation_is_still_reported(self, schema_manager): + section = dict(OLD_SECTION, not_declared=1) + valid, errors = schema_manager.validate_config_against_schema( + section, STRICT_SCHEMA, PLUGIN_ID) + assert valid is False and len(errors) == 1 + assert "not_declared" in errors[0] + + +class TestLoadPath: + """The real PluginManager.load_plugin, with the plugin class stubbed.""" + + def test_an_old_section_loads_without_warning_or_degraded(self, tmp_path): + plugins_dir = tmp_path / "plugins" + plugin_dir = plugins_dir / PLUGIN_ID + plugin_dir.mkdir(parents=True) + manifest = {"id": PLUGIN_ID, "name": "Scoreboard-shaped", "version": "1.0.0", + "entry_point": "manager.py", "class_name": "Plugin"} + (plugin_dir / "manifest.json").write_text(json.dumps(manifest), encoding="utf-8") + (plugin_dir / "config_schema.json").write_text(json.dumps(STRICT_SCHEMA), + encoding="utf-8") + + config_manager = MagicMock() + config_manager.load_config.return_value = {PLUGIN_ID: _copy(OLD_SECTION)} + with patch('src.common.permission_utils.ensure_directory_permissions'): + manager = PluginManager(plugins_dir=str(plugins_dir), + config_manager=config_manager, + display_manager=MagicMock(), + cache_manager=MagicMock()) + manager.logger = MagicMock() + manager.health_tracker = MagicMock() + manager.plugin_manifests[PLUGIN_ID] = manifest + manager.plugin_loader.find_plugin_directory = MagicMock(return_value=plugin_dir) + + received = {} + + def fake_load_plugin(**kwargs): + received.update(kwargs["config"]) + instance = MagicMock() + instance.validate_config.return_value = True + return instance, MagicMock() + + manager.plugin_loader.load_plugin = MagicMock(side_effect=fake_load_plugin) + + assert manager.load_plugin(PLUGIN_ID) is True + assert "skin" not in received and "skin_options" not in received + assert received["favorite_teams"] == ["TB"] + warnings = [c for c in manager.logger.warning.call_args_list + if "does not match its schema" in str(c.args[0])] + assert warnings == [] + degraded = [c.args for c in manager.health_tracker.set_degraded.call_args_list + if c.args[0] == PLUGIN_ID] + assert degraded, "schema validation never ran" + assert degraded[-1][1] is None diff --git a/test/web_interface/test_plugin_config_json_saves.py b/test/web_interface/test_plugin_config_json_saves.py index 5d0c50f6..6e3e9792 100644 --- a/test/web_interface/test_plugin_config_json_saves.py +++ b/test/web_interface/test_plugin_config_json_saves.py @@ -8,7 +8,9 @@ test_api_v3_secret_roundtrip.py, so assertions are on config.json itself. - Legacy booleans (#588) were normalized only at load, so posting back what GET /plugins/config returned failed validation. - The JSON save's filter kept only enabled/display_duration/live_priority, so - a submitted skin, skin_options or vegas_* tuning key was silently dropped. + a submitted vegas_* tuning key was silently dropped. +- ``skin`` and ``skin_options`` outlived the skin system in stored configs: + every save path must accept a section carrying them, and drop them. - Plugin sections posted to /config/main skipped all of that and were stored verbatim, including values /plugins/config rejects. """ @@ -65,7 +67,9 @@ STORED = { "display": {"brightness": 80, "show_icons": False}, "global": {"dynamic_duration": {"enabled": False, "min_duration_seconds": 45}}, "vegas_width_pct": 60, + # Written by a release that still had the skin system "skin": "neon", + "skin_options": {"accent": "#00ff00"}, } _ATTRS = ('config_manager', 'plugin_manager', 'plugin_store_manager', @@ -174,28 +178,64 @@ class TestCoreOwnedKeysSurviveTheFilter: env.store({"enabled": True, "city": "Paris"}) resp = env.save({ "vegas_width_pct": 50, "vegas_overflow": "truncate", - "vegas_max_width_screens": 2, "skin": "retro", - "skin_options": {"accent": "#ff0000"}, "live_priority": True, + "vegas_max_width_screens": 2, "live_priority": True, }) assert resp.status_code == 200, resp.get_json() stored = env.stored() assert stored["vegas_width_pct"] == 50 assert stored["vegas_overflow"] == "truncate" assert stored["vegas_max_width_screens"] == 2 - assert stored["skin"] == "retro" - assert stored["skin_options"] == {"accent": "#ff0000"} assert stored["live_priority"] is True def test_stored_core_keys_survive_an_unrelated_save(self, env): assert env.save({"city": "Nice"}).status_code == 200 stored = env.stored() - assert stored["vegas_width_pct"] == 60 and stored["skin"] == "neon" + assert stored["vegas_width_pct"] == 60 def test_a_non_core_unknown_key_is_still_filtered(self, env): assert env.save({"not_in_schema": 1}).status_code == 200 assert "not_in_schema" not in env.stored() +class TestRetiredSkinKeys: + """STORED carries skin/skin_options, and SCHEMA sets + additionalProperties: false without declaring them.""" + + @staticmethod + def _assert_dropped(stored): + assert "skin" not in stored and "skin_options" not in stored + assert stored["vegas_width_pct"] == 60 # still a core-owned key + + def test_json_save_accepts_and_drops_them(self, env): + resp = env.save({"city": "Nice"}) + assert resp.status_code == 200, resp.get_json() + assert env.stored()["city"] == "Nice" + self._assert_dropped(env.stored()) + + def test_form_save_accepts_and_drops_them(self, env): + resp = env.client.post(f"/api/v3/plugins/config?plugin_id={PLUGIN_ID}", + data={"city": "Nice"}) + assert resp.status_code == 200, resp.get_json() + assert env.stored()["city"] == "Nice" + self._assert_dropped(env.stored()) + + def test_config_main_accepts_and_drops_them(self, env): + resp = env.client.post("/api/v3/config/main", json={PLUGIN_ID: {"city": "Nice"}}) + assert resp.status_code == 200, resp.get_json() + self._assert_dropped(env.stored()) + + def test_submitted_ones_are_not_stored(self, env): + env.store({"enabled": True, "city": "Paris", "vegas_width_pct": 60}) + resp = env.save({"skin": "retro", "skin_options": {"accent": "#ff0000"}}) + assert resp.status_code == 200, resp.get_json() + self._assert_dropped(env.stored()) + + def test_get_config_leaves_them_out(self, env): + data = env.client.get(f"/api/v3/plugins/config?plugin_id={PLUGIN_ID}").get_json()["data"] + assert data["city"] == "Paris" + assert "skin" not in data and "skin_options" not in data + + class TestLegacyBooleans: LEGACY = {"enabled": True, "city": "Paris", "global": {"dynamic_duration": True}} @@ -246,7 +286,7 @@ class TestPluginSectionsInConfigMain: assert stored["city"] == "Lyon" assert stored["display"] == {"brightness": 80, "show_icons": False} assert stored["vegas_overflow"] == "rotate" - assert stored["vegas_width_pct"] == 60 and stored["skin"] == "neon" + assert stored["vegas_width_pct"] == 60 def test_secrets_still_go_to_the_secrets_file(self, env): resp = self._post(env, {PLUGIN_ID: {"api_key": "s3cret"}}) diff --git a/web_interface/blueprints/api_v3/__init__.py b/web_interface/blueprints/api_v3/__init__.py index 17ca4c3f..7d180c94 100644 --- a/web_interface/blueprints/api_v3/__init__.py +++ b/web_interface/blueprints/api_v3/__init__.py @@ -1276,9 +1276,8 @@ def _enhance_schema_with_core_properties(schema): """ Enhance schema with the core-owned per-plugin properties. - ``enabled``, ``display_duration``, ``live_priority``, ``skin``, - ``skin_options`` and the ``vegas_*`` tuning keys are system-managed and - always allowed, even when the plugin's schema doesn't declare them. The + ``enabled``, ``display_duration``, ``live_priority`` and the ``vegas_*`` + tuning keys are system-managed and always allowed, even when the plugin's schema doesn't declare them. The list is ``schema_manager.CORE_PLUGIN_PROPERTIES``, the one validation uses, so the save filter keeps exactly what validation accepts. diff --git a/web_interface/blueprints/api_v3/config.py b/web_interface/blueprints/api_v3/config.py index 01c9b3d7..667cdda5 100644 --- a/web_interface/blueprints/api_v3/config.py +++ b/web_interface/blueprints/api_v3/config.py @@ -1052,10 +1052,14 @@ def save_main_config(): if error: return error - # Deep merge regular config into main config + # Deep merge regular config into main config, dropping + # retired core keys (skin, skin_options) from the stored section + from src.plugin_system.schema_manager import drop_retired_plugin_keys stored_section = current_config.get(plugin_id) current_config[plugin_id] = deep_merge( - stored_section if isinstance(stored_section, dict) else {}, regular_config) + drop_retired_plugin_keys( + stored_section if isinstance(stored_section, dict) else {}, schema), + regular_config) if secrets_config: plugin_secrets_updates[plugin_id] = secrets_config diff --git a/web_interface/blueprints/api_v3/plugins.py b/web_interface/blueprints/api_v3/plugins.py index ffd0e462..e92512ed 100644 --- a/web_interface/blueprints/api_v3/plugins.py +++ b/web_interface/blueprints/api_v3/plugins.py @@ -2190,7 +2190,10 @@ def save_plugin_config(): if plugin_id not in current_config: current_config[plugin_id] = {} - current_config[plugin_id] = deep_merge(current_config[plugin_id], regular_config) + # Retired core keys (skin, skin_options) leave the stored section here + from src.plugin_system.schema_manager import drop_retired_plugin_keys + current_config[plugin_id] = deep_merge( + drop_retired_plugin_keys(current_config[plugin_id], schema), regular_config) # Deep merge plugin secrets in secrets config if secrets_config: