mirror of
https://github.com/ChuckBuilds/LEDMatrix.git
synced 2026-10-10 17:16:36 +00:00
fix(config): drop retired skin/skin_options keys instead of validating them
A config.json written while the skin system existed can carry skin and skin_options in any plugin section, and most plugin schemas set additionalProperties: false. They are no longer core plugin properties; RETIRED_PLUGIN_KEYS in schema_manager lists them and drop_retired_plugin_keys removes them (unless the plugin's own schema declares the name) in prepare_plugin_config, which loading, hot reload, GET /plugins/config and both web saves already share, and in validate_config_against_schema for callers that validate a raw section. POST /plugins/config and /config/main also drop them from the stored section they merge into, so they leave config.json on the next save. Tests cover the load path (real PluginManager.load_plugin: no schema warning, not degraded), raw and prepared validation, validate_all_plugin_configs, and the JSON, form and /config/main saves. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
Chuck
@@ -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 {}
|
||||
|
||||
@@ -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
|
||||
@@ -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"}})
|
||||
|
||||
@@ -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.
|
||||
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
@@ -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:
|
||||
|
||||
Reference in New Issue
Block a user