From 1b106cbc32f2154e9e3e49656ded0016db2c6b6c Mon Sep 17 00:00:00 2001 From: jokob-sk Date: Fri, 9 Oct 2026 09:59:14 +1100 Subject: [PATCH] Review fixes --- .claude/skills/plugin-development/SKILL.md | 2 +- .gemini/skills/plugin-development/plugin-skill.md | 2 +- .github/skills/plugin-run-development/SKILL.md | 2 +- server/db/plugin_field_views.py | 12 +++++++----- test/backend/test_app_conf_string_escaping.py | 8 +++++++- 5 files changed, 17 insertions(+), 9 deletions(-) diff --git a/.claude/skills/plugin-development/SKILL.md b/.claude/skills/plugin-development/SKILL.md index a70cce240..60101671c 100644 --- a/.claude/skills/plugin-development/SKILL.md +++ b/.claude/skills/plugin-development/SKILL.md @@ -65,7 +65,7 @@ Every mapped field (`objectPrimaryId`/`objectSecondaryId`/`watchedValue1-4`/`ext `all_plugins` (every plugin's parsed `config.json`) is loaded once, in `initialise.py`'s `get_plugins_configs()` call, and reused as the same list object everywhere after that until an actual config reload replaces it. `update_api()` (`server/api.py`), which receives `all_plugins` as a parameter, runs far more often than that - on every scan cycle and every plugin completion (`server/__main__.py`, `server/plugin.py`), not just on reload. -Any function deriving something from `all_plugins` that gets called from a hot path like `update_api()` should cache its result keyed by `id(all_plugins)`, not recompute on every call - the input is almost always the same object even though the call site fires constantly. `server/db/plugin_field_views.py`'s `get_all_device_field_views()` is the pattern: check `id(all_plugins)` against the last-seen id, recomputing only when a genuinely new object (an actual reload) appears. +Any function deriving something from `all_plugins` that gets called from a hot path like `update_api()` should cache its result, not recompute on every call - the input is almost always the same object even though the call site fires constantly. `server/db/plugin_field_views.py`'s `get_all_device_field_views()` is the pattern: retain a reference to the last-seen `all_plugins` list and compare it to the current one with `is`, recomputing only when a genuinely new object (an actual reload) appears. Don't key the cache by `id(all_plugins)` alone - once the old list is garbage collected, Python can reuse that id for an unrelated object, which would wrongly serve a stale cached result for what is actually a new reload. ## Plugin Formats diff --git a/.gemini/skills/plugin-development/plugin-skill.md b/.gemini/skills/plugin-development/plugin-skill.md index 1d0e58c6f..93c7a83c7 100644 --- a/.gemini/skills/plugin-development/plugin-skill.md +++ b/.gemini/skills/plugin-development/plugin-skill.md @@ -77,7 +77,7 @@ Every mapped field (`objectPrimaryId`/`objectSecondaryId`/`watchedValue1-4`/`ext `all_plugins` is loaded once (`initialise.py`'s `get_plugins_configs()`) and reused as the same object until a reload - but `update_api()` (`server/api.py`), which takes it as a parameter, runs on every scan cycle and plugin completion, not just on reload. -- If you write a function that derives something from `all_plugins` and gets called from `update_api()` or another frequent call site, cache the result by `id(all_plugins)` instead of recomputing every call +- If you write a function that derives something from `all_plugins` and gets called from `update_api()` or another frequent call site, cache the result instead of recomputing every call - retain a reference to the last-seen `all_plugins` list and compare it to the current one with `is`, not by `id(all_plugins)` alone (once the old list is garbage collected, Python can reuse that id for an unrelated object, wrongly serving a stale cached result for what is actually a new reload) - Pattern: `server/db/plugin_field_views.py`'s `get_all_device_field_views()` ## Plugin Formats diff --git a/.github/skills/plugin-run-development/SKILL.md b/.github/skills/plugin-run-development/SKILL.md index 32e507c45..f85bf09fb 100644 --- a/.github/skills/plugin-run-development/SKILL.md +++ b/.github/skills/plugin-run-development/SKILL.md @@ -78,7 +78,7 @@ Every mapped field (`objectPrimaryId`/`objectSecondaryId`/`watchedValue1-4`/`ext - `all_plugins` is loaded once (`get_plugins_configs()` in `initialise.py`) and reused as the same object until a config reload - `update_api()` (`server/api.py`) receives `all_plugins` but runs on every scan cycle and plugin completion - far more often than reloads happen -- A function deriving something from `all_plugins`, called from a hot path like `update_api()`, should cache by `id(all_plugins)` rather than recompute every call +- A function deriving something from `all_plugins`, called from a hot path like `update_api()`, should cache its result rather than recompute every call - retain a reference to the last-seen `all_plugins` list and compare it to the current one with `is`, not by `id(all_plugins)` alone (once the old list is garbage collected, Python can reuse that id for an unrelated object, wrongly serving a stale cached result for what is actually a new reload) - Example: `server/db/plugin_field_views.py`'s `get_all_device_field_views()` ## Plugin Formats diff --git a/server/db/plugin_field_views.py b/server/db/plugin_field_views.py index e5629a2b5..c8fbc3889 100644 --- a/server/db/plugin_field_views.py +++ b/server/db/plugin_field_views.py @@ -90,7 +90,7 @@ def get_plugin_columns_for_field(all_plugins, field_key): return results -_field_views_cache = {"all_plugins_id": None, "result": None} +_field_views_cache = {"all_plugins_ref": None, "result": None} def get_all_device_field_views(all_plugins): @@ -99,14 +99,16 @@ def get_all_device_field_views(all_plugins): reload and reused as the same object across every later call (e.g. update_api() runs on every scan cycle and plugin completion) - caching by identity avoids re-scanning every plugin's config on each of those calls, recomputing only when a genuinely new all_plugins - object (an actual reload) is passed in.""" - cache_key = id(all_plugins) - if _field_views_cache["all_plugins_id"] != cache_key: + object (an actual reload) is passed in. The cache retains a reference to the list itself + (compared with `is`), not just its id() - an id() alone can be reused by an unrelated + object once the original all_plugins list is garbage collected, which would wrongly + serve a stale cached result for what is actually a new reload.""" + if _field_views_cache["all_plugins_ref"] is not all_plugins: _field_views_cache["result"] = { field_key: get_plugin_columns_for_field(all_plugins, field_key) for field_key in DEVICE_FIELD_VIEWS } - _field_views_cache["all_plugins_id"] = cache_key + _field_views_cache["all_plugins_ref"] = all_plugins return _field_views_cache["result"] diff --git a/test/backend/test_app_conf_string_escaping.py b/test/backend/test_app_conf_string_escaping.py index 52a1351e5..5e554b02d 100644 --- a/test/backend/test_app_conf_string_escaping.py +++ b/test/backend/test_app_conf_string_escaping.py @@ -25,13 +25,19 @@ pytestmark = pytest.mark.skipif(PHP_BIN is None, reason="PHP CLI (php or php83) # Stands in for the web request: reads {"front": ..., "settings": [...]} from stdin, # satisfies security.php's request-only dependencies and lets util.php dispatch savesettings. +# util.php only reads function/settings from $_POST (not $_REQUEST) and requires a matching +# csrf_token - seed $_SESSION directly (same PHP process, so security.php's session_start() +# just resumes it) rather than $_REQUEST, to mirror a real authenticated POST. PHP_RUNNER = ( '$in = json_decode(stream_get_contents(STDIN), true);' 'if (!function_exists("apache_request_headers")) { function apache_request_headers() { return []; } }' '$_SERVER["DOCUMENT_ROOT"] = $in["front"];' '$_SERVER["HTTP_HOST"] = "localhost";' '$_SERVER["REQUEST_URI"] = "/php/server/util.php";' - '$_REQUEST = ["function" => "savesettings", "settings" => json_encode($in["settings"])];' + 'session_start();' + '$_SESSION["csrf_token"] = "test-harness-csrf-token";' + '$_POST = ["function" => "savesettings", "csrf_token" => "test-harness-csrf-token", ' + '"settings" => json_encode($in["settings"])];' 'require $in["front"] . "/php/server/util.php";' )