From 9f2743471cd9a52106dfe6d90cce106d6c552938 Mon Sep 17 00:00:00 2001 From: Chuck <33324927+ChuckBuilds@users.noreply.github.com> Date: Tue, 15 Sep 2026 18:09:11 -0400 Subject: [PATCH 1/2] fix(web): update-all skips Starlark apps and no longer misses plugins (#587) * fix(web): update-all skips Starlark apps and no longer misses plugins Check & Update All posted every entry from /plugins/installed to POST /plugins/update, including the virtual starlark: entries that list installed Starlark apps. The store manager cannot find those, so each answered 500 "plugin not found". Update-all now sends only plugin ids (install_manager.js, and the older app-shell.js copy), and the route answers a starlark: id with a 400 saying it is a Starlark app. A request that got no HTTP answer was recorded as failed and never sent again. On a device, a web-service restart mid-run killed the in-flight request and refused the next one, stock-news, which was left on 2.6.2 with 2.8.0 available. Such requests are now re-sent with backoff (about 30s) before being reported as failed. HTTP error answers are not retried. Tests: test/js/unit/test_update_all.js (run from pytest via test/web_interface/test_update_all_plugins.py so CI covers it) and the route contract for starlark: ids. Co-Authored-By: Claude Opus 5 * refactor(web): walk update-all retry delays without indexed lookup Codacy's ESLint security/detect-object-injection rule flagged retryDelays[attempt] as a High issue. The index was a bounded loop counter over a fixed array, but shifting a per-plugin copy of the schedule gives the same backoff without the pattern. No behaviour change: test/js/unit/test_update_all.js (21) and test/web_interface/test_update_all_plugins.py (7) pass unchanged. Co-Authored-By: Claude Opus 5 --------- Co-authored-by: Claude Opus 5 --- CHANGELOG.md | 7 + test/js/README.md | 1 + test/js/run_all.js | 3 +- test/js/unit/test_update_all.js | 151 ++++++++++++++++++ test/web_interface/test_update_all_plugins.py | 104 ++++++++++++ web_interface/blueprints/api_v3/plugins.py | 13 ++ web_interface/static/v3/js/app-shell.js | 5 +- .../static/v3/js/plugins/install_manager.js | 70 +++++++- 8 files changed, 344 insertions(+), 10 deletions(-) create mode 100644 test/js/unit/test_update_all.js create mode 100644 test/web_interface/test_update_all_plugins.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 209838e9..a33bab16 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -47,6 +47,13 @@ Web interface: JSON API saves are unaffected. Lets plugins keep deprecated or internal keys declared, e.g. countdown's row `id` and weather's `api_key` / `radar_zoom`. See `docs/widget-guide.md`. +- **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. ## 3.4.0 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/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; + } } } From c200b5837d0f606d08c7207a4935d90721d55f84 Mon Sep 17 00:00:00 2001 From: Chuck <33324927+ChuckBuilds@users.noreply.github.com> Date: Tue, 15 Sep 2026 18:09:23 -0400 Subject: [PATCH 2/2] fix(plugins): normalize legacy boolean settings before schema validation (#588) The news plugin's schema turned global.dynamic_duration from a boolean into an {enabled, min_duration_seconds, ...} object. Installs that have not saved the news settings since still hold `true`, so every start 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) and the next save writes the object. The loader did not. It now applies the same rule before merging schema defaults and validating, so the defaults fill in the rest of the object and the plugin receives it in the new shape. The rule lives in schema_manager.legacy_bool_as_object / normalize_legacy_booleans. It applies at any depth of nested objects but not inside arrays, matching the form, and only to a real bool under an object-typed property with an `enabled` child. Every other mismatch still warns. A parity test renders the template macro against the helper so the two cannot drift. Nothing is written to config.json at load: the normalization is in memory, and the next save of the plugin's settings persists the object. Co-authored-by: Claude Opus 5 --- CHANGELOG.md | 11 + src/plugin_system/plugin_manager.py | 21 +- src/plugin_system/schema_manager.py | 79 ++++ test/test_legacy_boolean_config.py | 394 ++++++++++++++++++ .../templates/v3/partials/plugin_config.html | 6 +- 5 files changed, 509 insertions(+), 2 deletions(-) create mode 100644 test/test_legacy_boolean_config.py diff --git a/CHANGELOG.md b/CHANGELOG.md index a33bab16..3c35c1c7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -55,6 +55,17 @@ Web interface: 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 Plugin-facing changes since 3.3.0 (tag `v3.3.1`) not covered further down: 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/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/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 {}) %}