From 4585cc1eff40d6f65c37d581d78313cfabf5b5ea Mon Sep 17 00:00:00 2001 From: ChuckBuilds Date: Thu, 26 Mar 2026 18:41:47 -0400 Subject: [PATCH] fix: address review findings for dotted-key handling - ensure_array_defaults: replace None nodes with {} so recursion proceeds into nested objects (was skipping when key existed as None) - dotToNested: add tail-matching that checks the full remaining dotted tail against the current schema level before greedy intermediate matching, preventing leaf dotted keys from being split - syncFormToJson: replace naive key.split('.') reconstruction with dotToNested(flatConfig, schema) and schema-aware getSchemaProperty() so the JSON tab save path produces the same correct nesting as the form submit path - Add regression tests for dotted-key array normalization and None array default replacement Co-Authored-By: Claude Opus 4.6 (1M context) --- test/test_web_api.py | 126 +++++++++++++++++++++ web_interface/blueprints/api_v3.py | 3 +- web_interface/static/v3/plugins_manager.js | 48 ++++---- 3 files changed, 152 insertions(+), 25 deletions(-) diff --git a/test/test_web_api.py b/test/test_web_api.py index 1226453d..769d8d79 100644 --- a/test/test_web_api.py +++ b/test/test_web_api.py @@ -583,3 +583,129 @@ class TestAPIErrorHandling: response = client.get('/api/v3/display/on-demand/start') assert response.status_code in [200, 405] # Depends on implementation + + +class TestDottedKeyNormalization: + """Regression tests for fix_array_structures / ensure_array_defaults with dotted schema keys.""" + + def test_save_plugin_config_dotted_key_arrays(self, client, mock_config_manager): + """Nested dotted-key objects with numeric-keyed dicts are converted to arrays.""" + from web_interface.blueprints.api_v3 import api_v3 + + api_v3.config_manager = mock_config_manager + mock_config_manager.load_config.return_value = {} + + schema_mgr = MagicMock() + schema = { + 'type': 'object', + 'properties': { + 'leagues': { + 'type': 'object', + 'properties': { + 'eng.1': { + 'type': 'object', + 'properties': { + 'enabled': {'type': 'boolean', 'default': True}, + 'favorite_teams': { + 'type': 'array', + 'items': {'type': 'string'}, + 'default': [], + }, + }, + }, + }, + }, + }, + } + schema_mgr.load_schema.return_value = schema + schema_mgr.generate_default_config.return_value = { + 'leagues': {'eng.1': {'enabled': True, 'favorite_teams': []}}, + } + schema_mgr.merge_with_defaults.side_effect = lambda config, defaults: {**defaults, **config} + schema_mgr.validate_config_against_schema.return_value = [] + api_v3.schema_manager = schema_mgr + + request_data = { + 'plugin_id': 'soccer-scoreboard', + 'config': { + 'leagues': { + 'eng.1': { + 'enabled': True, + 'favorite_teams': ['Arsenal', 'Chelsea'], + }, + }, + }, + } + + response = client.post( + '/api/v3/plugins/config', + data=json.dumps(request_data), + content_type='application/json', + ) + + if response.status_code == 200: + saved = mock_config_manager.save_config_atomic.call_args[0][0] + soccer_cfg = saved.get('soccer-scoreboard', {}) + leagues = soccer_cfg.get('leagues', {}) + assert 'eng.1' in leagues, f"Expected 'eng.1' key, got: {list(leagues.keys())}" + assert isinstance(leagues['eng.1'].get('favorite_teams'), list) + assert leagues['eng.1']['favorite_teams'] == ['Arsenal', 'Chelsea'] + + def test_save_plugin_config_none_array_gets_default(self, client, mock_config_manager): + """None array fields under dotted-key parents are replaced with defaults.""" + from web_interface.blueprints.api_v3 import api_v3 + + api_v3.config_manager = mock_config_manager + mock_config_manager.load_config.return_value = {} + + schema_mgr = MagicMock() + schema = { + 'type': 'object', + 'properties': { + 'leagues': { + 'type': 'object', + 'properties': { + 'eng.1': { + 'type': 'object', + 'properties': { + 'favorite_teams': { + 'type': 'array', + 'items': {'type': 'string'}, + 'default': [], + }, + }, + }, + }, + }, + }, + } + schema_mgr.load_schema.return_value = schema + schema_mgr.generate_default_config.return_value = { + 'leagues': {'eng.1': {'favorite_teams': []}}, + } + schema_mgr.merge_with_defaults.side_effect = lambda config, defaults: {**defaults, **config} + schema_mgr.validate_config_against_schema.return_value = [] + api_v3.schema_manager = schema_mgr + + request_data = { + 'plugin_id': 'soccer-scoreboard', + 'config': { + 'leagues': { + 'eng.1': { + 'favorite_teams': None, + }, + }, + }, + } + + response = client.post( + '/api/v3/plugins/config', + data=json.dumps(request_data), + content_type='application/json', + ) + + if response.status_code == 200: + saved = mock_config_manager.save_config_atomic.call_args[0][0] + soccer_cfg = saved.get('soccer-scoreboard', {}) + teams = soccer_cfg.get('leagues', {}).get('eng.1', {}).get('favorite_teams') + assert isinstance(teams, list), f"Expected list, got: {type(teams)}" diff --git a/web_interface/blueprints/api_v3.py b/web_interface/blueprints/api_v3.py index a1899461..19a93d51 100644 --- a/web_interface/blueprints/api_v3.py +++ b/web_interface/blueprints/api_v3.py @@ -4113,8 +4113,7 @@ def save_plugin_config(): nested_dict = config_dict.get(prop_key) if nested_dict is None: - if prop_key not in config_dict: - config_dict[prop_key] = {} + config_dict[prop_key] = {} nested_dict = config_dict[prop_key] if isinstance(nested_dict, dict): diff --git a/web_interface/static/v3/plugins_manager.js b/web_interface/static/v3/plugins_manager.js index 5d6340c1..6feecfde 100644 --- a/web_interface/static/v3/plugins_manager.js +++ b/web_interface/static/v3/plugins_manager.js @@ -2334,6 +2334,15 @@ function dotToNested(obj, schema) { while (i < parts.length - 1) { let matched = false; if (currentSchema) { + // First, check if the full remaining tail is a leaf property + // (e.g., "eng.1" as a complete dotted key with no sub-properties) + const tailCandidate = parts.slice(i).join('.'); + if (tailCandidate in currentSchema) { + current[tailCandidate] = obj[key]; + matched = true; + i = parts.length; // consumed all parts + break; + } // Try progressively longer candidates (longest first) to greedily // match dotted property names like "eng.1" for (let j = parts.length - 1; j > i; j--) { @@ -4463,42 +4472,35 @@ function switchPluginConfigView(view) { function syncFormToJson() { const form = document.getElementById('plugin-config-form'); if (!form) return; - + const formData = new FormData(form); - const config = {}; - + const flatConfig = {}; + // Get schema for type conversion const schema = currentPluginConfigState.schema; - + for (let [key, value] of formData.entries()) { if (key === 'enabled') continue; // Skip enabled, managed separately - - // Handle nested keys (dot notation) - const keys = key.split('.'); - let current = config; - for (let i = 0; i < keys.length - 1; i++) { - if (!current[keys[i]]) { - current[keys[i]] = {}; - } - current = current[keys[i]]; - } - - const finalKey = keys[keys.length - 1]; - const prop = schema?.properties?.[finalKey] || (keys.length > 1 ? null : schema?.properties?.[key]); - + + // Use schema-aware property lookup for type conversion + const prop = schema ? getSchemaProperty(schema, key) : null; + // Type conversion based on schema if (prop?.type === 'array') { - current[finalKey] = value.split(',').map(item => item.trim()).filter(item => item.length > 0); + flatConfig[key] = value.split(',').map(item => item.trim()).filter(item => item.length > 0); } else if (prop?.type === 'integer' || key === 'display_duration') { - current[finalKey] = parseInt(value) || 0; + flatConfig[key] = parseInt(value) || 0; } else if (prop?.type === 'number') { - current[finalKey] = parseFloat(value) || 0; + flatConfig[key] = parseFloat(value) || 0; } else if (prop?.type === 'boolean') { - current[finalKey] = value === 'true' || value === true; + flatConfig[key] = value === 'true' || value === true; } else { - current[finalKey] = value; + flatConfig[key] = value; } } + + // Convert flat dot-notation keys to nested object using schema-aware matching + const config = dotToNested(flatConfig, schema); // Deep merge with existing config to preserve nested structures function deepMerge(target, source) {