fix: handle dotted schema keys in plugin settings save#295
Open
ChuckBuilds wants to merge 3 commits intomainfrom
Open
fix: handle dotted schema keys in plugin settings save#295ChuckBuilds wants to merge 3 commits intomainfrom
ChuckBuilds wants to merge 3 commits intomainfrom
Conversation
The soccer plugin uses dotted keys like "eng.1" for league identifiers. PR #260 fixed backend helpers but the JS frontend still corrupted these keys by naively splitting on dots. This fixes both the JS and remaining Python code paths: - JS getSchemaProperty(): greedy longest-match for dotted property names - JS dotToNested(): schema-aware key grouping to preserve "eng.1" as one key - Python fix_array_structures(): remove broken prefix re-navigation in recursion - Python ensure_array_defaults(): same prefix navigation fix Closes #254 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughRefactors backend plugin-config post-processing to operate on current nested dicts (removing prefix-based dot-path traversal) and updates frontend form-to-config normalization to be schema-aware (greedy longest-match) so schema property names that contain dots are preserved end-to-end. Tests for dotted-key behavior were added. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Browser as Browser (plugins_manager.js)
participant API as API /api/v3/plugins/config
participant SchemaMgr as Schema Manager
participant Server as save_plugin_config
participant DB as Storage
User->>Browser: fill form (fields may include dotted names)
Browser->>Browser: normalizeFormDataForConfig (flatten, type-coerce via getSchemaProperty)
Browser->>Browser: dotToNested(flatConfig, schema) (greedy longest-match)
Browser->>API: POST nested plugin config
API->>SchemaMgr: load plugin schema
API->>Server: save_plugin_config(payload, schema)
Server->>Server: ensure_array_defaults / fix_array_structures (operate on current nested dicts)
Server->>DB: persist normalized config
DB-->>Server: saved
Server-->>API: OK
API-->>Browser: response
Browser-->>User: show success
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
web_interface/blueprints/api_v3.py (1)
4027-4128: Please add a regression test for nested dotted-key + array normalization.Current API test coverage (see
test/test_web_api.pyaround Line 413) only exercises a simple boolean config save, so this path can regress without detection.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web_interface/blueprints/api_v3.py` around lines 4027 - 4128, Add a regression test that exercises nested dotted-key + array normalization through the same API path covered by test/test_web_api.py: construct a plugin_config payload where nested object keys are provided as dotted strings and arrays are represented as dicts with numeric-string keys (and include a None array to trigger defaults), POST/PUT it to the plugin config save endpoint used by existing tests, then retrieve the saved config (or call the same normalization function via the API) and assert that fix_array_structures and ensure_array_defaults produced the expected result (numeric-key dicts converted to lists in nested objects, numeric strings converted to number/integer types per items schema, and None arrays replaced with default empty or schema-default lists); reference the functions fix_array_structures and ensure_array_defaults to locate the logic to validate in the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web_interface/blueprints/api_v3.py`:
- Around line 4115-4121: The code skips recursion when config_dict[prop_key]
exists but is None; inside the branch where nested_dict is None, always replace
None with an empty dict instead of only creating the key when missing: set
config_dict[prop_key] = {} and then nested_dict = config_dict[prop_key] so
ensure_array_defaults(prop_schema['properties']) runs; update the logic around
nested_dict, prop_key, config_dict and the call to ensure_array_defaults so None
nodes are converted to {} before recursing.
In `@web_interface/static/v3/plugins_manager.js`:
- Around line 2618-2619: syncFormToJson currently serializes dotted keys by
splitting on '.' and reconstructs nested objects incorrectly, bypassing the
dotToNested fix used on form submit; update syncFormToJson (and any helper it
uses) to call dotToNested(flatConfig, schema) instead of using key.split('.')
reconstruction or, alternatively, replace the key.split('.') path with the same
dotToNested logic so the JSON tab save produces the same nested structure as the
form-submit path; ensure you reference the existing dotToNested function and the
flatConfig/schema variables within syncFormToJson to canonicalize dotted keys
before serializing.
- Around line 2324-2376: dotToNested() currently never tries to match the full
remaining dotted tail against the current schema before creating intermediate
objects, so leaf keys like "eng.1.favorite_team" get split; modify dotToNested
to, at the start of the inner matching logic (before the "No schema match or no
schema — use single segment" branch), compute const tailCandidate =
parts.slice(i).join('.') and if currentSchema && tailCandidate in currentSchema
then set current[tailCandidate] = obj[key] and break out to the outer loop (skip
creating intermediate nodes), otherwise continue with the existing greedy
matching and single-segment fallback; refer to the dotToNested function and
variables parts, i, currentSchema, current and finalKey when making the change.
---
Nitpick comments:
In `@web_interface/blueprints/api_v3.py`:
- Around line 4027-4128: Add a regression test that exercises nested dotted-key
+ array normalization through the same API path covered by test/test_web_api.py:
construct a plugin_config payload where nested object keys are provided as
dotted strings and arrays are represented as dicts with numeric-string keys (and
include a None array to trigger defaults), POST/PUT it to the plugin config save
endpoint used by existing tests, then retrieve the saved config (or call the
same normalization function via the API) and assert that fix_array_structures
and ensure_array_defaults produced the expected result (numeric-key dicts
converted to lists in nested objects, numeric strings converted to
number/integer types per items schema, and None arrays replaced with default
empty or schema-default lists); reference the functions fix_array_structures and
ensure_array_defaults to locate the logic to validate in the test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6657e435-7fea-4534-a07d-36b18ebf5e4f
📒 Files selected for processing (2)
web_interface/blueprints/api_v3.pyweb_interface/static/v3/plugins_manager.js
- 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) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/test_web_api.py`:
- Around line 591-652: The test test_save_plugin_config_dotted_key_arrays
currently guards its assertions behind "if response.status_code == 200",
allowing silent success when the endpoint returns an error; remove that
conditional and assert the expected status code unconditionally (e.g., assert
response.status_code == 200) immediately after the client.post, then proceed to
inspect mock_config_manager.save_config_atomic.call_args and validate the saved
config (references: test_save_plugin_config_dotted_key_arrays,
client.post('/api/v3/plugins/config'), mock_config_manager.save_config_atomic,
api_v3.config_manager, api_v3.schema_manager).
- Around line 654-711: The test test_save_plugin_config_none_array_gets_default
currently guards the assertions with "if response.status_code == 200" which
silently skips failures; change this to assert the response status explicitly
(e.g., assert response.status_code == 200) and then proceed to inspect the saved
config from mock_config_manager.save_config_atomic, asserting the favorite_teams
field is a list and equals the expected default ([]) to catch regressions; refer
to the test function name test_save_plugin_config_none_array_gets_default, the
local variables response, saved, soccer_cfg, and teams, and replace the
conditional block with explicit asserts for status_code, isinstance(teams,
list), and teams == [].
In `@web_interface/static/v3/plugins_manager.js`:
- Around line 2337-2345: When a full-tail dotted key (tailCandidate) is matched
against currentSchema and you set matched = true and i = parts.length, ensure
the later unconditional write that uses finalKey does not run and create an
empty-string key; modify the control flow around the finalKey write so it only
executes when i < parts.length (or matched is false), i.e., skip or return when
tailCandidate matched/consumed all parts. Adjust logic using the existing
variables/functions (tailCandidate, currentSchema, current, parts, matched,
finalKey) so exact dotted leaves like "leagues.eng.1" are written only once and
the finalKey block is bypassed.
- Around line 4476-4503: The JSON-tab serializer in syncFormToJson() must share
the same FormData→flatConfig→dotToNested normalization as
handlePluginConfigSubmit(); extract that logic into a reusable helper (e.g.,
normalizeFormDataForConfig(formData, schema)) and call it from both
syncFormToJson() and handlePluginConfigSubmit(). The helper should: 1) consult
currentPluginConfigState.schema (or accept schema param) via getSchemaProperty
to perform type conversions (array, integer/display_duration, number, boolean),
2) treat checkbox values of "on" as true and ensure unchecked booleans are
represented as false, 3) handle helper inputs like _data and [] for checkbox
groups, patternProperties, array-of-objects and file-upload widgets so they
produce proper arrays/objects instead of raw strings/bogus keys, and 4) call
dotToNested(flatConfig, schema) to build the nested config object; replace the
duplicated code in both locations with calls to this new helper.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a640fed9-7c44-411c-9873-eabe954330a2
📒 Files selected for processing (3)
test/test_web_api.pyweb_interface/blueprints/api_v3.pyweb_interface/static/v3/plugins_manager.js
✅ Files skipped from review due to trivial changes (1)
- web_interface/blueprints/api_v3.py
- Tests: replace conditional `if response.status_code == 200` guards with unconditional `assert response.status_code == 200` so failures are not silently swallowed - dotToNested: guard finalKey write with `if (i < parts.length)` to prevent empty-string key pollution when tail-matching consumed all parts - Extract normalizeFormDataForConfig() helper from handlePluginConfigSubmit and call it from both handlePluginConfigSubmit and syncFormToJson so the JSON tab sync uses the same robust FormData processing (including _data JSON inputs, bracket-notation checkboxes, array-of-objects, file-upload widgets, checkbox DOM detection, and unchecked boolean handling via collectBooleanFields) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
web_interface/static/v3/plugins_manager.js (2)
2420-2598:⚠️ Potential issue | 🟠 MajorHandle the schema-less fallback before treating form fields as nested config paths.
When
schemaisnull,generateSimpleConfigForm()submits a singleconfigtextarea containing the whole JSON document. This shared normalizer currently turns that into{ config: parsedValue }, so schema-less plugins now save the wrong shape and the JSON tab mirrors the extra wrapper.♻️ Suggested fix
function normalizeFormDataForConfig(form, schema) { const formData = new FormData(form); const flatConfig = {}; @@ - // Convert dot notation to nested object - return dotToNested(flatConfig, schema); + // Simple fallback form posts the entire config document in one textarea. + if (!schema && + Object.prototype.hasOwnProperty.call(flatConfig, 'config') && + Object.keys(flatConfig).length === 1) { + return flatConfig.config; + } + + // Convert dot notation to nested object + return dotToNested(flatConfig, schema); }Also applies to: 2600-2631, 4473-4479
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web_interface/static/v3/plugins_manager.js` around lines 2420 - 2598, The normalizer wraps a standalone JSON config inside {config: ...} when schema is null (used by generateSimpleConfigForm()), so update normalizeFormDataForConfig to detect when schema is falsy and the form contains the single textarea named "config": parse its value as JSON and return that parsed object directly (or set flatConfig to the parsed object and skip dotToNested) instead of leaving it as { config: parsedValue }; implement this check early (when iterating FormData or immediately after building flatConfig) and ensure errors fall back to returning the raw string, and apply the same logic locations referenced around normalizeFormDataForConfig/dotToNested handling.
4477-4499:⚠️ Potential issue | 🟠 MajorDon't merge the canonical form payload back into stale config state.
normalizeFormDataForConfig()now returns the exact submit payload, butdeepMerge()never removes keys that disappeared from the form. That lets deleted entries and old broken branches likeleagues.eng.1 -> leagues.eng -> 1survive when the user switches to the JSON tab and then saves from there.♻️ Suggested fix
- // Deep merge new form data into existing config - currentPluginConfigState.config = deepMerge( - JSON.parse(JSON.stringify(currentPluginConfigState.config)), // Deep clone - config - ); + // Use the canonical form payload so removed keys stay removed. + currentPluginConfigState.config = JSON.parse(JSON.stringify(config));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web_interface/static/v3/plugins_manager.js` around lines 4477 - 4499, normalizeFormDataForConfig now returns the canonical submit payload but deepMerge preserves keys that are missing from the form, causing stale/removed keys to persist; to fix, stop merging canonical payload back into stale config by either (A) replacing currentPluginConfigState.config with the normalized config returned from normalizeFormDataForConfig (i.e., assign config directly to currentPluginConfigState.config) or (B) update deepMerge used by currentPluginConfigState.config to perform a destructive sync: for each key in the target that is not present in the source delete it, and for keys present recursively merge/replace as needed so absent keys are removed; change references around normalizeFormDataForConfig, deepMerge, and the assignment to currentPluginConfigState.config accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@web_interface/static/v3/plugins_manager.js`:
- Around line 2420-2598: The normalizer wraps a standalone JSON config inside
{config: ...} when schema is null (used by generateSimpleConfigForm()), so
update normalizeFormDataForConfig to detect when schema is falsy and the form
contains the single textarea named "config": parse its value as JSON and return
that parsed object directly (or set flatConfig to the parsed object and skip
dotToNested) instead of leaving it as { config: parsedValue }; implement this
check early (when iterating FormData or immediately after building flatConfig)
and ensure errors fall back to returning the raw string, and apply the same
logic locations referenced around normalizeFormDataForConfig/dotToNested
handling.
- Around line 4477-4499: normalizeFormDataForConfig now returns the canonical
submit payload but deepMerge preserves keys that are missing from the form,
causing stale/removed keys to persist; to fix, stop merging canonical payload
back into stale config by either (A) replacing currentPluginConfigState.config
with the normalized config returned from normalizeFormDataForConfig (i.e.,
assign config directly to currentPluginConfigState.config) or (B) update
deepMerge used by currentPluginConfigState.config to perform a destructive sync:
for each key in the target that is not present in the source delete it, and for
keys present recursively merge/replace as needed so absent keys are removed;
change references around normalizeFormDataForConfig, deepMerge, and the
assignment to currentPluginConfigState.config accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 39de2c46-cece-44d9-b886-b66281e20aa3
📒 Files selected for processing (2)
test/test_web_api.pyweb_interface/static/v3/plugins_manager.js
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
getSchemaProperty()anddotToNested()to use greedy longest-match for dotted schema keys (e.g.,eng.1in soccer league configs), preventing the client from corrupting config structure before sending to the backendfix_array_structures()andensure_array_defaults()by removing broken prefix-based re-navigation that failed at nesting depth > 0Context
PR #260 fixed backend
_get_schema_propertyand_set_nested_valuefor dotted keys, but the JS frontend path (dotToNested) still naively split all dots — convertingleagues.eng.1.favorite_teamsinto{leagues: {eng: {"1": {favorite_teams: ...}}}}instead of{leagues: {"eng.1": {favorite_teams: ...}}}.Test plan
config/config.jsonhas{"soccer-scoreboard": {"leagues": {"eng.1": {...}}}}(not{"eng": {"1": {...}}})Closes #254
🤖 Generated with Claude Code
Summary by CodeRabbit