diff --git a/CHANGELOG.md b/CHANGELOG.md index a5854114..a183def3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -77,6 +77,24 @@ Web interface: the Display form offers only row address types 0 and 2 on a Pi 5. The rule lives in `src/pi5_matrix_support.py` and must be re-checked when the submodule is bumped. +- **Check & Update All** no longer sends installed Starlark apps + (`starlark:` entries in `/plugins/installed`) to the plugin updater, + which answered each with a 500 "plugin not found". `POST /plugins/update` + now answers a `starlark:` id with a 400 saying it is a Starlark app. A + request that gets no HTTP answer (e.g. the web service restarting mid-run) is + re-sent with backoff instead of being counted as failed and skipped — that is + how a disabled plugin with an update waiting was silently left out. + +Plugin system: + +- A plugin no longer starts with a schema warning and a degraded flag because + config.json still holds a boolean where its schema now has an object with an + `enabled` property (news' `global.dynamic_duration: true`). The loader reads + the boolean as `{"enabled": }` before merging schema defaults and + validating, the same rule the settings form already applies + (`legacy_bool_as_object` in `src/plugin_system/schema_manager.py`). Nothing + is written at load; the next save of that plugin's settings stores the object. + Other type mismatches still warn. ## 3.4.0 diff --git a/src/plugin_system/plugin_manager.py b/src/plugin_system/plugin_manager.py index 2c8de821..c587acc8 100644 --- a/src/plugin_system/plugin_manager.py +++ b/src/plugin_system/plugin_manager.py @@ -22,7 +22,7 @@ from src.logging_config import get_logger from src.plugin_system.plugin_loader import PluginLoader from src.plugin_system.plugin_executor import PluginExecutor from src.plugin_system.plugin_state import PluginStateManager, PluginState -from src.plugin_system.schema_manager import SchemaManager +from src.plugin_system.schema_manager import SchemaManager, normalize_legacy_booleans from src.common.permission_utils import ( ensure_directory_permissions, get_plugin_dir_mode @@ -351,6 +351,7 @@ class PluginManager: config = {} # Check if plugin has a config schema + schema = None schema_path = self.schema_manager.get_schema_path(plugin_id) if schema_path is None: # Schema file doesn't exist @@ -368,6 +369,24 @@ class PluginManager: f"The schema may be invalid. Please verify the schema file at: {schema_path}" ) + # A plugin that turned an on/off boolean into an {enabled, ...} + # object still finds the boolean in config.json until the user saves + # its settings form, which carries it over (plugin_config.html). + # Read it the same way here, before the defaults fill in the rest + # of the object and before schema validation, so the plugin doesn't + # start with a schema warning and a degraded flag. In memory only: + # config.json is written by saves, never by loading a plugin. + if schema: + upgraded: List[str] = [] + config = normalize_legacy_booleans(config, schema, upgraded) + if upgraded: + self.logger.info( + "Plugin %s: reading legacy boolean setting %s as " + "{\"enabled\": ...}; saving the plugin's settings " + "stores the new shape", + plugin_id, ", ".join(upgraded), + ) + # Merge config with schema defaults to ensure all defaults are applied try: defaults = self.schema_manager.generate_default_config(plugin_id, use_cache=True) diff --git a/src/plugin_system/schema_manager.py b/src/plugin_system/schema_manager.py index 3db4a5a2..b56e6126 100644 --- a/src/plugin_system/schema_manager.py +++ b/src/plugin_system/schema_manager.py @@ -14,6 +14,85 @@ import jsonschema from jsonschema import Draft7Validator, ValidationError +def _renders_as_object(prop: Dict[str, Any]) -> bool: + """``field_type == 'object'`` as ``plugin_config.html`` computes it. + + The template takes a type list's *first* entry, so ``["object", "null"]`` + is an object and ``["boolean", "object"]`` is a checkbox. + """ + field_type = prop.get('type') + if isinstance(field_type, list): + field_type = field_type[0] if field_type else None + return field_type == 'object' + + +def legacy_bool_as_object(value: Any, prop: Any) -> Any: + """Read a boolean stored where the schema now has an ``{enabled, ...}`` object. + + Plugins turn an on/off switch into a settings object (news' + ``global.dynamic_duration: true`` became ``{enabled, min_duration_seconds, + ...}``), but config.json keeps the boolean until the user next saves that + plugin's form. The boolean was the switch the object's ``enabled`` now + holds, so it becomes ``{"enabled": value}`` and schema defaults fill the rest. + + Returns ``value`` itself when the rule does not apply: anything that is not + a real ``bool`` (``1``, ``"true"``, ``None``) or a schema property that is not + an object with an ``enabled`` child. Those stay as they are, so validation + still reports a genuine mismatch. + + This is the rule ``render_nested_section`` in + ``web_interface/templates/v3/partials/plugin_config.html`` applies when it + draws the form. ``test/test_legacy_boolean_config.py`` renders that macro + against this function, so change both together. + """ + if not isinstance(value, bool) or not isinstance(prop, dict): + return value + properties = prop.get('properties') + if (_renders_as_object(prop) and isinstance(properties, dict) + and 'enabled' in properties): + return {'enabled': value} + return value + + +def normalize_legacy_booleans(config: Any, schema: Any, + changed_paths: Optional[List[str]] = None, + _prefix: str = '') -> Any: + """Apply :func:`legacy_bool_as_object` at every depth of a plugin config. + + Walks the config along the schema's ``properties`` the way the settings + form does: into nested objects, not into array items (the form hands arrays + to widgets and never applies the rule there). + + Never mutates ``config``. Returns the same object when nothing changed, and + otherwise copies only the dicts on the path to each upgraded value. When + ``changed_paths`` is given, the dotted path of each upgraded value is + appended to it. + """ + if not isinstance(config, dict) or not isinstance(schema, dict): + return config + properties = schema.get('properties') + if not isinstance(properties, dict): + return config + + result = config + for key, value in config.items(): + prop = properties.get(key) + if not isinstance(prop, dict) or not _renders_as_object(prop): + continue + path = f"{_prefix}.{key}" if _prefix else key + new_value = legacy_bool_as_object(value, prop) + if new_value is not value: + if changed_paths is not None: + changed_paths.append(path) + elif isinstance(value, dict): + new_value = normalize_legacy_booleans(value, prop, changed_paths, path) + if new_value is not value: + if result is config: + result = dict(config) + result[key] = new_value + return result + + class SchemaManager: """ Manages plugin configuration schemas with caching and validation. diff --git a/test/js/README.md b/test/js/README.md index c635ce80..9d7c3b95 100644 --- a/test/js/README.md +++ b/test/js/README.md @@ -35,6 +35,7 @@ nothing is listening, so it stays useful in a bare checkout. | Suite | Needs a server | Covers | |---|---|---| | `unit/test_list_filter.js` | no | `ListFilter` search/filter/sort/count/sticky, and the installed-plugins config **extracted verbatim** from `plugins_manager.js` so the test can't drift from it | +| `unit/test_update_all.js` | no | `PluginInstallManager.updateAll` from `plugins/install_manager.js`: Check & Update All sends only plugin ids (never `starlark:` app entries), and re-sends a request that got no HTTP answer (web service restarting) instead of skipping that plugin. Also run by `test/web_interface/test_update_all_plugins.py` so CI covers it | | `unit/test_render_cards.js` | no | `renderInstalledCards` markup, both empty states, and HTML-escaping of hostile plugin metadata | | `unit/test_style_editor_element_keys.js` | no | `elementKeys()`/`styleRows()`/`positionRows()` from `widgets/style-editor.js`: every `customization.layout` entry gets exactly one row -- paired with its style element through core's `x-layout-key` (so `score` belongs to `score_text`, not a second row), or a position row of its own, leaves included -- since the widget claims the whole `layout` block from the generic fallback renderer | | `unit/test_style_editor_layout_leaf_columns.js` | no | `columnsFor()` from `widgets/style-editor.js`: a layout-only key whose own value is a leaf (no x/y sub-object, e.g. a `show_logo` toggle) gets a self-keyed column instead of a blank, uneditable row | diff --git a/test/js/run_all.js b/test/js/run_all.js index 99ddaa01..bf9ca625 100755 --- a/test/js/run_all.js +++ b/test/js/run_all.js @@ -17,7 +17,8 @@ const BASE = process.env.BASE || 'http://localhost:5000'; const UNIT = ['unit/test_list_filter.js', 'unit/test_render_cards.js', 'unit/test_html_escaping.js', 'unit/test_style_editor_element_keys.js', 'unit/test_style_editor_layout_leaf_columns.js', - 'unit/test_style_editor_layout_leaf_collision.js']; + 'unit/test_style_editor_layout_leaf_collision.js', + 'unit/test_update_all.js']; const DOM = ['dom/test_installed_dom.js', 'dom/test_store_dom.js', 'dom/test_no_double_fetch.js', 'dom/test_tools_sections.js']; diff --git a/test/js/unit/test_update_all.js b/test/js/unit/test_update_all.js new file mode 100644 index 00000000..acf29099 --- /dev/null +++ b/test/js/unit/test_update_all.js @@ -0,0 +1,151 @@ +// "Check & Update All" -- which ids it sends to POST /api/v3/plugins/update. +// +// Pins two bugs seen on a real device (core 3.4.0): +// +// 1. /plugins/installed lists installed Starlark apps as virtual +// `starlark:` entries. Update-all sent those to the plugin +// updater, which answered 500 "plugin not found" for each one. +// +// 2. A web-service restart landed mid-run. The request in flight died with +// the old process and the next one (stock-news) was refused while the +// service was coming back; both were recorded as failures and never sent +// again, so stock-news -- installed, disabled, with an update waiting -- +// was silently not updated. +// +// Runs the shipped install_manager.js (it exports itself under node) against +// a fake PluginAPI. The installed list is the device's, trimmed. + +const path = require('path'); +const V3 = path.resolve(__dirname, '../../../web_interface/static/v3'); + +let pass = 0, fail = 0; +const ok = (label, cond, extra) => cond + ? (pass++, console.log(' ok ' + label)) + : (fail++, console.log(' FAIL ' + label + (extra !== undefined ? ' ' + JSON.stringify(extra) : ''))); + +global.window = {}; +const Manager = require(path.join(V3, 'js/plugins/install_manager.js')); + +const INSTALLED = [ + { id: 'ledmatrix-flights', enabled: true, version: '1.14.0' }, + { id: 'stock-news', enabled: false, version: '2.6.2', latest_version: '2.8.0', update_available: true }, + { id: 'static-image', enabled: false, version: '1.1.3' }, + { id: 'starlark-apps', enabled: false, version: '1.0.0' }, // a real plugin -- keep it + { id: 'pomodoro-timer', enabled: false, version: '1.3.6' }, + { id: 'starlark:analogtime', enabled: false, version: 'starlark', is_starlark_app: true }, + { id: 'starlark:analogclock', enabled: true, version: 'starlark', is_starlark_app: true }, +]; +const EXPECTED = ['ledmatrix-flights', 'stock-news', 'static-image', 'starlark-apps', 'pomodoro-timer']; + +const netErr = () => ({ error_code: 'NETWORK_ERROR', message: 'Failed to fetch' }); + +function fakeApi(behaviour = {}) { + const calls = []; + return { + calls, + updatePlugin: async (id) => { + calls.push(id); + const b = behaviour[id]; + if (typeof b === 'function') return b(calls.filter(c => c === id).length); + return { status: 'success', message: `Plugin ${id} updated successfully` }; + }, + }; +} + +function setup(api, { stateList, windowList } = {}) { + global.window = { + PluginAPI: api, + installedPlugins: windowList, + PluginStateManager: stateList === undefined ? undefined : { + installedPlugins: stateList, + loadInstalledPlugins: async () => stateList, + }, + }; +} + +const noSleep = { sleep: async () => {} }; + +(async () => { + console.log('\nselection'); + ok('starlark app entries are not updatable', + !Manager.isUpdatablePlugin(INSTALLED[5]) && !Manager.isUpdatablePlugin(INSTALLED[6])); + ok('a starlark: id without the flag is still excluded', + !Manager.isUpdatablePlugin({ id: 'starlark:foo' })); + ok('the starlark-apps plugin itself is updatable', Manager.isUpdatablePlugin(INSTALLED[3])); + ok('a disabled plugin with an update is updatable', Manager.isUpdatablePlugin(INSTALLED[1])); + ok('entries without a usable id are skipped', + !Manager.isUpdatablePlugin({}) && !Manager.isUpdatablePlugin(null) && !Manager.isUpdatablePlugin({ id: '' })); + const sel = Manager.updatablePlugins(INSTALLED).map(p => p.id); + ok('updatablePlugins keeps list order and drops only starlark apps', + JSON.stringify(sel) === JSON.stringify(EXPECTED), sel); + + console.log('\nupdateAll sends only plugin ids'); + { + const api = fakeApi(); + setup(api, { windowList: INSTALLED }); + const progress = []; + const results = await Manager.updateAll((i, n, id) => progress.push([i, n, id]), noSleep); + ok('POSTs exactly the non-starlark ids, in order', + JSON.stringify(api.calls) === JSON.stringify(EXPECTED), api.calls); + ok('no starlark: id reached the plugin updater', !api.calls.some(id => id.startsWith('starlark:'))); + ok('one result per plugin sent', results.length === EXPECTED.length, results.length); + ok('progress total counts only what is sent', + progress.length === EXPECTED.length && progress.every(([, n]) => n === EXPECTED.length), progress); + } + { + const api = fakeApi(); + setup(api, { stateList: INSTALLED, windowList: [] }); + await Manager.updateAll(null, noSleep); + ok('the PluginStateManager list is filtered the same way', + JSON.stringify(api.calls) === JSON.stringify(EXPECTED), api.calls); + } + { + const api = fakeApi(); + setup(api, { windowList: INSTALLED.filter(p => p.is_starlark_app) }); + const results = await Manager.updateAll(null, noSleep); + ok('a list of only starlark apps sends nothing', api.calls.length === 0 && results.length === 0); + } + + console.log('\nweb service restarting mid-run (the stock-news case)'); + { + // ledmatrix-flights is in flight when the service stops (reset); stock-news + // is refused twice while it comes back; then everything answers. + const api = fakeApi({ + 'ledmatrix-flights': (n) => { if (n === 1) throw netErr(); return { status: 'success', message: 'ok' }; }, + 'stock-news': (n) => { if (n <= 2) throw netErr(); return { status: 'success', message: 'Plugin stock-news updated successfully' }; }, + }); + setup(api, { windowList: INSTALLED }); + const slept = []; + const results = await Manager.updateAll(null, { sleep: async ms => { slept.push(ms); }, retryDelaysMs: [5, 10, 20] }); + const byId = Object.fromEntries(results.map(r => [r.pluginId, r])); + ok('stock-news is sent again until the server answers', + api.calls.filter(id => id === 'stock-news').length === 3, api.calls); + ok('stock-news ends up updated, not skipped', byId['stock-news'] && byId['stock-news'].success === true, byId['stock-news']); + ok('the request lost with the old process is re-sent too', + byId['ledmatrix-flights'] && byId['ledmatrix-flights'].success === true); + ok('it backs off between attempts', JSON.stringify(slept) === JSON.stringify([5, 5, 10]), slept); + ok('every plugin still gets exactly one result', + JSON.stringify(results.map(r => r.pluginId)) === JSON.stringify(EXPECTED), results.map(r => r.pluginId)); + } + { + const api = fakeApi({ 'stock-news': () => { throw netErr(); } }); + setup(api, { windowList: INSTALLED }); + const results = await Manager.updateAll(null, { sleep: async () => {}, retryDelaysMs: [1, 1] }); + const r = results.find(x => x.pluginId === 'stock-news'); + ok('a server that never comes back is retried a bounded number of times', + api.calls.filter(id => id === 'stock-news').length === 3, api.calls); + ok('...then reported as a failure, and the run continues', + r && r.success === false && api.calls[api.calls.length - 1] === 'pomodoro-timer'); + } + { + const api = fakeApi({ 'static-image': () => { throw { error_code: 'PLUGIN_UPDATE_FAILED', message: 'check logs' }; } }); + setup(api, { windowList: INSTALLED }); + const results = await Manager.updateAll(null, { sleep: async () => { throw new Error('must not sleep'); } }); + ok('an HTTP error answer is not retried', + api.calls.filter(id => id === 'static-image').length === 1, api.calls); + ok('...and is reported as a failure', results.find(x => x.pluginId === 'static-image').success === false); + } + + console.log(`\n${pass} passed, ${fail} failed`); + process.exit(fail ? 1 : 0); +})().catch(e => { console.error(e); process.exit(1); }); diff --git a/test/test_legacy_boolean_config.py b/test/test_legacy_boolean_config.py new file mode 100644 index 00000000..e9f4d791 --- /dev/null +++ b/test/test_legacy_boolean_config.py @@ -0,0 +1,394 @@ +"""A boolean the schema has since turned into an ``{enabled, ...}`` object. + +The news plugin's ``global.dynamic_duration`` went from ``true`` to an object +with ``enabled``, ``min_duration_seconds``, ... An install that has not saved +the news settings since still holds ``true``, and on every start the display +service logged:: + + Plugin news config does not match its schema (loading anyway): + Field 'global.dynamic_duration': Expected type object, got bool + +and flagged news degraded. The settings form already reads such a boolean as +``{"enabled": }`` (``render_nested_section`` in plugin_config.html); the +loader did not. Both now follow ``legacy_bool_as_object``, and the parity tests +below render the real macro against it so the two cannot drift. +""" + +import json +import sys +from html.parser import HTMLParser +from pathlib import Path +from unittest.mock import MagicMock, patch + +import pytest + +PROJECT_ROOT = Path(__file__).parent.parent +if str(PROJECT_ROOT) not in sys.path: + sys.path.insert(0, str(PROJECT_ROOT)) + +from src.plugin_system.plugin_manager import PluginManager +from src.plugin_system.schema_manager import ( + legacy_bool_as_object, + normalize_legacy_booleans, +) + + +DYNAMIC_DURATION = { + "type": "object", + "properties": { + "enabled": {"type": "boolean", "default": True}, + "min_duration_seconds": {"type": "integer", "default": 30, + "minimum": 10, "maximum": 300}, + "max_duration_seconds": {"type": "integer", "default": 300, + "minimum": 30, "maximum": 600}, + "buffer_ratio": {"type": "number", "default": 0.1, + "minimum": 0.01, "maximum": 1.0}, + }, + "additionalProperties": False, +} + +# The shape of news' schema, cut down to what the tests need. +NEWS_SCHEMA = { + "type": "object", + "properties": { + "enabled": {"type": "boolean", "default": True}, + "global": { + "type": "object", + "properties": { + "font_size": {"type": "integer", "default": 12}, + "dynamic_duration": DYNAMIC_DURATION, + "display": { + "type": "object", + "properties": { + "scroll_speed": {"type": "number", "default": 1.0}, + }, + "additionalProperties": False, + }, + "headline_paging": { + "type": "object", + "properties": { + "enabled": {"type": "boolean", "default": False}, + "page_hold_seconds": {"type": "number", "default": 2.0}, + }, + "additionalProperties": False, + }, + }, + "additionalProperties": False, + }, + "feeds": { + "type": "object", + "properties": { + "custom_feeds": { + "type": "array", + "items": { + "type": "object", + "properties": { + "name": {"type": "string"}, + "enabled": {"type": "boolean", "default": True}, + "schedule": { + "type": "object", + "properties": {"enabled": {"type": "boolean"}}, + }, + }, + }, + }, + }, + }, + }, + "additionalProperties": False, +} + + +# -------------------------------------------------------------------------- +# The helper +# -------------------------------------------------------------------------- + +class TestLegacyBoolAsObject: + def test_true_under_an_enabled_object_becomes_its_switch(self): + assert legacy_bool_as_object(True, DYNAMIC_DURATION) == {"enabled": True} + + def test_false_stays_off(self): + """Upgrading must never turn a feature the user switched off back on.""" + assert legacy_bool_as_object(False, DYNAMIC_DURATION) == {"enabled": False} + + def test_nullable_object_type_list_counts_as_object(self): + prop = dict(DYNAMIC_DURATION, type=["object", "null"]) + assert legacy_bool_as_object(True, prop) == {"enabled": True} + + @pytest.mark.parametrize("value", [1, 0, "true", "false", None, [], 2.5]) + def test_a_value_that_is_not_a_real_bool_is_untouched(self, value): + assert legacy_bool_as_object(value, DYNAMIC_DURATION) is value + + def test_an_object_without_enabled_is_untouched(self): + prop = {"type": "object", "properties": {"speed": {"type": "number"}}} + assert legacy_bool_as_object(True, prop) is True + + def test_a_boolean_or_object_union_is_untouched(self): + """The form draws ``["boolean", "object"]`` as a checkbox, and the + schema accepts the boolean, so there is nothing to upgrade.""" + prop = dict(DYNAMIC_DURATION, type=["boolean", "object"]) + assert legacy_bool_as_object(True, prop) is True + + def test_a_non_object_schema_is_untouched(self): + assert legacy_bool_as_object(True, {"type": "boolean"}) is True + assert legacy_bool_as_object(True, None) is True + + +class TestNormalizeLegacyBooleans: + def test_top_level(self): + schema = {"type": "object", + "properties": {"dynamic_duration": DYNAMIC_DURATION}} + out = normalize_legacy_booleans({"dynamic_duration": True}, schema) + assert out == {"dynamic_duration": {"enabled": True}} + + def test_nested(self): + cfg = {"global": {"font_size": 12, "dynamic_duration": True}} + out = normalize_legacy_booleans(cfg, NEWS_SCHEMA) + assert out == {"global": {"font_size": 12, + "dynamic_duration": {"enabled": True}}} + + def test_several_depths_report_their_paths(self): + schema = {"type": "object", "properties": { + "dynamic_duration": DYNAMIC_DURATION, + "outer": {"type": "object", "properties": { + "inner": {"type": "object", "properties": { + "mode": DYNAMIC_DURATION}}}}, + }} + cfg = {"dynamic_duration": False, "outer": {"inner": {"mode": True}}} + changed = [] + out = normalize_legacy_booleans(cfg, schema, changed) + assert out == {"dynamic_duration": {"enabled": False}, + "outer": {"inner": {"mode": {"enabled": True}}}} + assert changed == ["dynamic_duration", "outer.inner.mode"] + + def test_array_items_are_left_alone_as_the_form_does(self): + """The form hands arrays to widgets and never upgrades inside them.""" + cfg = {"feeds": {"custom_feeds": [{"name": "a", "schedule": True}]}} + out = normalize_legacy_booleans(cfg, NEWS_SCHEMA) + assert out is cfg + + def test_already_an_object_is_untouched(self): + cfg = {"global": {"dynamic_duration": {"enabled": False, + "min_duration_seconds": 45}}} + assert normalize_legacy_booleans(cfg, NEWS_SCHEMA) is cfg + + def test_other_mismatches_are_untouched(self): + cfg = {"global": {"font_size": True, "display": True, + "dynamic_duration": "yes"}} + changed = [] + assert normalize_legacy_booleans(cfg, NEWS_SCHEMA, changed) is cfg + assert changed == [] + + def test_keys_the_schema_does_not_declare_are_untouched(self): + cfg = {"global": {"mystery": True}, "vegas_width_pct": 50} + assert normalize_legacy_booleans(cfg, NEWS_SCHEMA) is cfg + + def test_the_stored_config_is_not_mutated(self): + """load_config() may hand back a cached dict; it must stay as stored.""" + cfg = {"enabled": True, "global": {"dynamic_duration": True}} + before = json.loads(json.dumps(cfg)) + out = normalize_legacy_booleans(cfg, NEWS_SCHEMA) + assert cfg == before + assert out is not cfg and out["global"] is not cfg["global"] + + @pytest.mark.parametrize("config,schema", [ + (None, NEWS_SCHEMA), ([], NEWS_SCHEMA), ({"a": True}, None), + ({"a": True}, {"type": "object"}), + ]) + def test_tolerates_odd_inputs(self, config, schema): + assert normalize_legacy_booleans(config, schema) is config + + +# -------------------------------------------------------------------------- +# The load path +# -------------------------------------------------------------------------- + +PLUGIN_ID = "news-shaped" + + +@pytest.fixture +def load(tmp_path): + """Run the real ``PluginManager.load_plugin`` for a plugin with + ``NEWS_SCHEMA`` and the given stored section; the plugin class is stubbed. + + Returns ``(loaded, config the plugin received, logger, health_tracker)``. + """ + plugins_dir = tmp_path / "plugins" + plugin_dir = plugins_dir / PLUGIN_ID + plugin_dir.mkdir(parents=True) + manifest = {"id": PLUGIN_ID, "name": "News-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(NEWS_SCHEMA), + encoding="utf-8") + + def _load(stored_section): + config_manager = MagicMock() + config_manager.load_config.return_value = { + PLUGIN_ID: json.loads(json.dumps(stored_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) + loaded = manager.load_plugin(PLUGIN_ID) + return loaded, received, manager.logger, manager.health_tracker + + return _load + + +def _schema_warnings(logger): + return [c for c in logger.warning.call_args_list + if "does not match its schema" in str(c.args[0])] + + +def _degraded_reason(health_tracker): + calls = [c.args for c in health_tracker.set_degraded.call_args_list + if c.args[0] == PLUGIN_ID] + assert calls, "schema validation never ran" + return calls[-1][1] + + +class TestLoadPath: + def test_news_shaped_legacy_config_loads_without_warning_or_degraded(self, load): + stored = {"enabled": True, + "global": {"font_size": 12, "dynamic_duration": True, + "headline_paging": {"enabled": False}}} + loaded, received, logger, tracker = load(stored) + + assert loaded is True + assert _schema_warnings(logger) == [] + assert _degraded_reason(tracker) is None + # The plugin gets the object, with the schema defaults filled in. + assert received["global"]["dynamic_duration"] == { + "enabled": True, "min_duration_seconds": 30, + "max_duration_seconds": 300, "buffer_ratio": 0.1} + # And the upgrade is logged once, below warning level. + assert any("global.dynamic_duration" in str(c.args) + for c in logger.info.call_args_list) + + def test_a_legacy_false_loads_switched_off(self, load): + loaded, received, logger, tracker = load( + {"enabled": True, "global": {"dynamic_duration": False}}) + assert loaded is True + assert received["global"]["dynamic_duration"]["enabled"] is False + assert _schema_warnings(logger) == [] + assert _degraded_reason(tracker) is None + + def test_a_real_mismatch_still_warns_and_degrades(self, load): + loaded, received, logger, tracker = load( + {"enabled": True, "global": {"dynamic_duration": True, + "font_size": "big"}}) + assert loaded is True # warn-only, as before + warnings = _schema_warnings(logger) + assert len(warnings) == 1 + assert "font_size" in str(warnings[0].args) + assert "dynamic_duration" not in str(warnings[0].args) + reason = _degraded_reason(tracker) + assert reason and "font_size" in reason + + def test_a_boolean_under_an_object_without_enabled_still_warns(self, load): + loaded, received, logger, tracker = load( + {"enabled": True, "global": {"display": True}}) + assert loaded is True + assert received["global"]["display"] is True + warnings = _schema_warnings(logger) + assert len(warnings) == 1 and "global.display" in str(warnings[0].args) + assert _degraded_reason(tracker) + + +# -------------------------------------------------------------------------- +# Parity with the settings form +# -------------------------------------------------------------------------- + +PARITY_SCHEMA = { + "type": "object", + "properties": { + "enabled": {"type": "boolean", "default": True}, + "global": {"type": "object", "properties": { + "dynamic_duration": DYNAMIC_DURATION, + "outer": {"type": "object", "properties": { + "inner": {"type": "object", "properties": { + "enabled": {"type": "boolean", "default": True}, + "level": {"type": "integer", "default": 1}, + }}, + }}, + "union": {"type": ["boolean", "object"], "properties": { + "enabled": {"type": "boolean"}}}, + }}, + }, +} + + +class _Checkboxes(HTMLParser): + def __init__(self): + super().__init__() + self.checked = {} + + def handle_starttag(self, tag, attrs): + a = dict(attrs) + if tag == "input" and a.get("type") == "checkbox" and a.get("name"): + self.checked[a["name"]] = "checked" in a + + +def _form_checkboxes(stored): + from jinja2 import Environment, FileSystemLoader, select_autoescape + + env = Environment( + loader=FileSystemLoader(str(PROJECT_ROOT / "web_interface" / "templates")), + autoescape=select_autoescape(["html"]), + ) + plugin = {"id": "demo", "name": "Demo", "description": "", "enabled": True, + "author": "me", "version": "1.0.0"} + html = env.get_template("v3/partials/plugin_config.html").render( + plugin=plugin, schema=PARITY_SCHEMA, config=json.loads(json.dumps(stored))) + parser = _Checkboxes() + parser.feed(html) + return parser.checked + + +def _loader_enabled(normalized, *path): + node = normalized + for key in path: + node = node.get(key) if isinstance(node, dict) else None + return isinstance(node, dict) and node.get("enabled") is True + + +@pytest.mark.parametrize("value", [True, False, 1, "true", None]) +def test_form_and_loader_agree_on_what_a_stored_value_means(value): + """Where the loader reads a stored value as ``{"enabled": true}``, the form + draws the object's ``enabled`` checkbox ticked, and nowhere else.""" + stored = {"enabled": True, + "global": {"dynamic_duration": value, "outer": {"inner": value}}} + normalized = normalize_legacy_booleans(stored, PARITY_SCHEMA) + boxes = _form_checkboxes(stored) + + for path in (("global", "dynamic_duration"), ("global", "outer", "inner")): + name = ".".join(path) + ".enabled" + assert name in boxes, f"form drew no {name} checkbox" + assert boxes[name] is _loader_enabled(normalized, *path), name + + +@pytest.mark.parametrize("value", [True, False]) +def test_form_and_loader_agree_a_boolean_union_stays_a_boolean(value): + stored = {"enabled": True, "global": {"union": value}} + normalized = normalize_legacy_booleans(stored, PARITY_SCHEMA) + boxes = _form_checkboxes(stored) + + assert normalized is stored + assert boxes.get("global.union") is value + assert "global.union.enabled" not in boxes diff --git a/test/web_interface/test_update_all_plugins.py b/test/web_interface/test_update_all_plugins.py new file mode 100644 index 00000000..c9ad9b39 --- /dev/null +++ b/test/web_interface/test_update_all_plugins.py @@ -0,0 +1,104 @@ +"""'Check & Update All' must only send ids POST /plugins/update handles. + +Seen on a device running core 3.4.0: update-all posted every entry from +/plugins/installed, including the virtual `starlark:` entries that +list installed Starlark apps, and the route answered each with a 500 +"Plugin update failed: plugin not found". A web-service restart during the +same run also cost stock-news its update: its request was refused while the +service came back, and update-all never sent it again. + +The id selection and retry live in install_manager.js and are covered by +test/js/unit/test_update_all.js, which this module runs so CI sees it. The +route contract is tested here directly. +""" + +import shutil +import subprocess +from pathlib import Path +from unittest.mock import MagicMock, patch + +import pytest + +REPO = Path(__file__).resolve().parents[2] +JS_SUITE = REPO / 'test' / 'js' / 'unit' / 'test_update_all.js' + + +@pytest.fixture +def client(): + from web_interface.app import app + app.config['TESTING'] = True + with app.test_client() as c: + yield c + + +@pytest.fixture +def store(tmp_path): + """A store manager that records calls; no git, no network.""" + from web_interface.blueprints.api_v3 import api_v3 + sm = MagicMock() + sm.plugins_dir = str(tmp_path) + sm._get_local_git_info.return_value = None + sm.get_plugin_info.return_value = None + sm.update_plugin.return_value = True + with patch.object(api_v3, 'plugin_store_manager', sm, create=True), \ + patch.object(api_v3, 'plugin_manager', None, create=True), \ + patch.object(api_v3, 'schema_manager', None, create=True), \ + patch.object(api_v3, 'plugin_state_manager', None, create=True), \ + patch.object(api_v3, 'operation_history', None, create=True): + yield sm + + +class TestUpdateRouteRejectsStarlarkIds: + + @pytest.mark.parametrize('app_id', ['starlark:analogtime', 'starlark:analogclock']) + def test_a_starlark_id_is_a_400_not_a_500(self, client, store, app_id): + resp = client.post('/api/v3/plugins/update', json={'plugin_id': app_id}) + assert resp.status_code == 400, resp.get_json() + body = resp.get_json() + assert body['status'] == 'error' + assert body['error_code'] == 'INVALID_INPUT' + assert 'Starlark app' in body['message'], body + assert 'not found' not in body['message'], \ + "the app is installed; 'not found' is the misleading message this replaces" + + def test_the_store_manager_is_never_asked(self, client, store): + client.post('/api/v3/plugins/update', json={'plugin_id': 'starlark:analogtime'}) + store.update_plugin.assert_not_called() + + def test_form_encoded_starlark_id_is_rejected_the_same_way(self, client, store): + resp = client.post('/api/v3/plugins/update', data={'plugin_id': 'starlark:analogtime'}) + assert resp.status_code == 400 + + def test_a_plugin_id_still_reaches_the_updater(self, client, store, tmp_path): + # The guard is on the 'starlark:' prefix only: the starlark-apps plugin + # and a disabled plugin with an update are still updated. + for pid in ('stock-news', 'starlark-apps'): + (tmp_path / pid).mkdir() + (tmp_path / pid / 'manifest.json').write_text('{"id": "%s"}' % pid, encoding='utf-8') + resp = client.post('/api/v3/plugins/update', json={'plugin_id': pid}) + assert resp.status_code == 200, (pid, resp.get_json()) + assert [c.args[0] for c in store.update_plugin.call_args_list] == ['stock-news', 'starlark-apps'] + + +class TestInstalledListContract: + """What update-all filters on is what /plugins/installed publishes.""" + + def test_starlark_entries_are_flagged_and_prefixed(self, client): + apps = {'apps': {'analogtime': {'name': 'Analog Time', 'enabled': False}}} + with patch('web_interface.blueprints.api_v3._get_starlark_plugin', return_value=None), \ + patch('web_interface.blueprints.api_v3._read_starlark_manifest', return_value=apps): + resp = client.get('/api/v3/plugins/installed') + plugins = resp.get_json()['data']['plugins'] + entry = next(p for p in plugins if p['id'] == 'starlark:analogtime') + assert entry['is_starlark_app'] is True + assert not any(p.get('is_starlark_app') for p in plugins + if not p['id'].startswith('starlark:')), \ + "a real plugin carries the Starlark flag and would be dropped from update-all" + + +@pytest.mark.skipif(shutil.which('node') is None, reason='node is not installed') +def test_update_all_js_selection_and_retry(): + node = shutil.which('node') + result = subprocess.run([node, str(JS_SUITE)], capture_output=True, text=True, + timeout=120, cwd=str(JS_SUITE.parent)) + assert result.returncode == 0, result.stdout + result.stderr diff --git a/web_interface/blueprints/api_v3/plugins.py b/web_interface/blueprints/api_v3/plugins.py index 06cd65c2..b621151d 100644 --- a/web_interface/blueprints/api_v3/plugins.py +++ b/web_interface/blueprints/api_v3/plugins.py @@ -943,6 +943,19 @@ def update_plugin(): ) data = {'plugin_id': plugin_id} + # /plugins/installed lists installed Starlark apps as virtual + # 'starlark:' entries. They are not plugin directories, so the + # store manager can only fail to find them -- which used to surface as + # a 500 "plugin not found" for an app that is installed and working. + raw_id = data.get('plugin_id') + if isinstance(raw_id, str) and raw_id.startswith('starlark:'): + return error_response( + ErrorCode.INVALID_INPUT, + f'{raw_id} is a Starlark app, not a plugin; Starlark apps are ' + 'not updated through the plugin updater', + status_code=400 + ) + if not api_v3.plugin_store_manager: return error_response( ErrorCode.SYSTEM_ERROR, diff --git a/web_interface/static/v3/js/app-shell.js b/web_interface/static/v3/js/app-shell.js index 070919a2..4fbc22f7 100644 --- a/web_interface/static/v3/js/app-shell.js +++ b/web_interface/static/v3/js/app-shell.js @@ -2421,7 +2421,10 @@ async function updateAllPlugins() { try { - const plugins = Array.isArray(window.installedPlugins) ? window.installedPlugins : []; + // Starlark apps are listed as virtual 'starlark:' entries; + // /plugins/update does not handle them (see install_manager.js). + const plugins = (Array.isArray(window.installedPlugins) ? window.installedPlugins : []) + .filter(p => p && typeof p.id === 'string' && !p.is_starlark_app && !p.id.startsWith('starlark:')); if (!plugins.length) { showNotification('No installed plugins to update.', 'warning'); diff --git a/web_interface/static/v3/js/plugins/install_manager.js b/web_interface/static/v3/js/plugins/install_manager.js index 1a13d05d..49702332 100644 --- a/web_interface/static/v3/js/plugins/install_manager.js +++ b/web_interface/static/v3/js/plugins/install_manager.js @@ -78,33 +78,87 @@ const PluginInstallManager = { } }, + /** + * Whether POST /plugins/update can update this installed-list entry. + * + * /plugins/installed also lists installed Starlark apps as virtual + * `starlark:` entries (flagged `is_starlark_app`) so they can be + * seen and toggled with everything else. They are not plugin directories: + * the plugin updater has nothing to update for them, and there is no + * Starlark update route (reinstalling from the repository would reset the + * app's saved settings), so they are left out of update-all. + * + * @param {Object} plugin - Entry from /plugins/installed + * @returns {boolean} + */ + isUpdatablePlugin(plugin) { + if (!plugin || typeof plugin.id !== 'string' || !plugin.id) return false; + return !plugin.is_starlark_app && !plugin.id.startsWith('starlark:'); + }, + + /** + * The entries update-all sends to POST /plugins/update, in list order. + * + * @param {Array} plugins - Entries from /plugins/installed + * @returns {Array} + */ + updatablePlugins(plugins) { + return (Array.isArray(plugins) ? plugins : []).filter(p => this.isUpdatablePlugin(p)); + }, + + /** + * Backoff (ms) before re-sending an update whose request never got an + * answer. Covers a web-service restart (~3s on a Pi) with room to spare. + */ + NETWORK_RETRY_DELAYS_MS: [1000, 2000, 4000, 8000, 15000], + /** * Update all plugins. * * @param {Function} onProgress - Optional callback(index, total, pluginId) for progress updates - * @returns {Promise} Update results + * @param {Object} options - Optional { sleep(ms), retryDelaysMs } (tests inject these) + * @returns {Promise} Update results, one per plugin sent */ - async updateAll(onProgress) { + async updateAll(onProgress, options = {}) { // Prefer PluginStateManager if populated, fall back to window.installedPlugins // (plugins_manager.js populates window.installedPlugins independently) const stateManagerPlugins = window.PluginStateManager && window.PluginStateManager.installedPlugins; - const plugins = (stateManagerPlugins && stateManagerPlugins.length > 0) + const listed = (stateManagerPlugins && stateManagerPlugins.length > 0) ? stateManagerPlugins : (window.installedPlugins || []); + // Snapshot: the list can be replaced while this loop is awaiting. + const plugins = this.updatablePlugins(listed); if (!plugins.length) { return []; } + const sleep = options.sleep || (ms => new Promise(resolve => setTimeout(resolve, ms))); + const retryDelays = options.retryDelaysMs || this.NETWORK_RETRY_DELAYS_MS; const results = []; for (let i = 0; i < plugins.length; i++) { const plugin = plugins[i]; if (onProgress) onProgress(i + 1, plugins.length, plugin.id); - try { - const result = await window.PluginAPI.updatePlugin(plugin.id); - results.push({ pluginId: plugin.id, success: true, result }); - } catch (error) { - results.push({ pluginId: plugin.id, success: false, error }); + // Each plugin gets its own pass over the backoff schedule. + const pendingDelays = retryDelays.slice(); + for (;;) { + try { + const result = await window.PluginAPI.updatePlugin(plugin.id); + results.push({ pluginId: plugin.id, success: true, result }); + break; + } catch (error) { + // No HTTP answer at all (connection refused/reset, e.g. the + // web service restarting mid-run): the server never saw or + // never finished this plugin, so send it again once it is + // back rather than skipping it. An HTTP error response is + // the server's answer and is not retried. + if (error && error.error_code === 'NETWORK_ERROR' && pendingDelays.length > 0) { + await sleep(pendingDelays.shift()); + continue; + } + results.push({ pluginId: plugin.id, success: false, error }); + break; + } } } diff --git a/web_interface/templates/v3/partials/plugin_config.html b/web_interface/templates/v3/partials/plugin_config.html index 2c645250..87c85fac 100644 --- a/web_interface/templates/v3/partials/plugin_config.html +++ b/web_interface/templates/v3/partials/plugin_config.html @@ -1166,7 +1166,11 @@ TypeError that takes down the whole config page. A legacy boolean was the on/off switch the object's `enabled` now holds, so carry it over -- rendering {} would draw that checkbox unchecked and the next save would - quietly turn the feature off. The save then writes the object. #} + quietly turn the feature off. The save then writes the object. + The plugin loader applies the same rule before validating + (legacy_bool_as_object in src/plugin_system/schema_manager.py), and + test/test_legacy_boolean_config.py renders this macro against it: + change the two together. #} {% if value is mapping %} {% set nested_value = value %} {% elif value is boolean and 'enabled' in (prop.properties or {}) %}