Skip to content

Fix robustness issue: empty JSON {} in cfg.json or wsec.json#355

Merged
softhack007 merged 8 commits intomdevfrom
copilot/fix-empty-json-issues
Apr 5, 2026
Merged

Fix robustness issue: empty JSON {} in cfg.json or wsec.json#355
softhack007 merged 8 commits intomdevfrom
copilot/fix-empty-json-issues

Conversation

Copy link
Copy Markdown

Copilot AI commented Apr 5, 2026

  • Simplify initPresetsFile() in presets.cpp: remove the sz local variable, use bool empty = f && f.size() < 3 instead — cleaner and equally correct

Summary by CodeRabbit

  • Bug Fixes
    • Treat empty or effectively-empty configuration files as invalid: restore defaults, optionally fall back to stored settings, and avoid silently using blank configurations.
    • Improve presets handling by detecting and recreating empty or undersized preset files so presets are reliably initialized and usable.

Copy link
Copy Markdown
Collaborator

@softhack007 softhack007 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me.

@softhack007 softhack007 marked this pull request as ready for review April 5, 2026 13:32
@softhack007
Copy link
Copy Markdown
Collaborator

@coderabbitai please review.

This PR is intended to solve the "empty JSON" corner case identified in #343, plus handle empty presets.json (e.g. { }, `{}``) gracefully.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 5, 2026

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Get PR details and diff
gh pr view 355 --repo MoonModules/WLED-MM --json title,body,files,state

# Get the diff
gh pr diff 355 --repo MoonModules/WLED-MM

Length of output: 2235


Let me check the appendObjectToFile() function to verify the < 3 threshold and look for other read paths.


🏁 Script executed:

#!/bin/bash
# Check appendObjectToFile for the threshold used
rg -n "appendObjectToFile|size.*<.*3\|< 3" wled00/presets.cpp wled00/json.cpp wled00/cfg.cpp -A5 -B5

# Also look at readObjectFromFile and any other places that read presets.json / cfg.json / wsec.json
rg -n "readObjectFromFile\|getFileName\(\)\|presets\.json\|cfg\.json\|wsec\.json" wled00/presets.cpp wled00/cfg.cpp -A2 -B2

Length of output: 710


🏁 Script executed:

#!/bin/bash
# Find appendObjectToFile implementation
rg -n "appendObjectToFile" wled00/ -A 20 -B 2

# Find all readObjectFromFile calls in presets.cpp
rg -n "readObjectFromFile" wled00/presets.cpp -A 3 -B 3

# Check if there are other places that open/read the presets file directly
rg -n "getFileName" wled00/presets.cpp -A 2 -B 2

# Check json.cpp for the size threshold
rg -n "size.*< 3" wled00/ -A 3 -B 3

Length of output: 10271


@softhack007: The PR is well-structured and addresses the robustness corner case cleanly. Here's my review:


wled00/cfg.cpp — ✅ Clean & correct

The doc.size() == 0 check after readObjectFromFile() is the right approach — it's whitespace-agnostic (handles {}, { }, { }, etc.) and consistently applied to both deserializeConfigFromFS() and deserializeConfigSec(). Lock management follows the same early-exit pattern as the original !success path.


wled00/presets.cpp — ✅ Correct, one minor observation

The f.size() < 3 threshold is confirmed consistent with appendObjectToFile() in wled00/file.cpp:226, which uses the same threshold to detect an uninitialized presets file before writing the {"0":{}} seed structure. Good internal consistency.

One minor observation: The PR description states it handles both {} and { } in presets.json, but the size-based check only catches {} (2 bytes); { } is exactly 3 bytes and falls through the < 3 guard. In practice this isn't a real-world concern — the fs_data/ placeholder is exactly {} (2 bytes), and any file that went through appendObjectToFile() will be ≥ 8 bytes — but the comment/PR description is slightly misleading. Worth updating the comment on line 127 to say "{}" only (not { }), or adjusting to <= 4 if you want to be defensive against minor whitespace variants (though matching appendObjectToFile's exact threshold is the safer choice for consistency).

WLED_FS.remove() return value (line 133): Not checked, but harmless — the subsequent WLED_FS.open(getFileName(), "w") will truncate and overwrite the file even if remove() failed, and errorFlag = ERR_FS_GENERAL is set on open failure. No action needed.


Other read paths

The readObjectFromFileUsingId() call sites in presets.cpp (e.g., getPresetName, applyPreset) already handle a missing/empty preset correctly by returning ERR_FS_PLOAD or false, so no additional changes are needed there.


Overall: The fix is correct and appropriately scoped. The only suggestion is a small comment clarification in presets.cpp line 127 to avoid implying { } (3 bytes) is handled at the filesystem level.


✏️ Learnings added
Learnt from: softhack007
URL: https://github.com/MoonModules/WLED-MM/pull/355

Timestamp: 2026-04-05T13:40:29.493Z
Learning: In MoonModules/WLED-MM, `appendObjectToFile()` in `wled00/file.cpp` uses `f.size() < 3` to detect an uninitialized presets file and writes `{"0":{}}` as the seed structure. The `initPresetsFile()` in `wled00/presets.cpp` intentionally uses the same `f.size() < 3` threshold for consistency. This threshold catches `{}` (2 bytes) but NOT `{ }` (3 bytes).

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


🧠 Learnings used
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 343
File: fs_data/README.md:12-14
Timestamp: 2026-04-05T13:09:31.720Z
Learning: In MoonModules/WLED-MM, empty JSON files (containing only `{}`) in fs_data (e.g., cfg.json, presets.json, wsec.json) are intended to be treated the same as non-existing files by the deserialization code in wled00/cfg.cpp and related files. The bugfix for this robustness issue (avoiding null/empty-object dereference) is tracked in PR `#355`. After the fix, `{}` is a safe placeholder for these config files.

Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-23T21:29:14.927Z
Learning: In MoonModules/WLED-MM, presets.json writes must be serialized and atomic: guard all write paths with a dedicated file-level mutex (presetFileMux) and use temp-file + rename to replace the file. The “immediate write” branch in presets.cpp can race with async doSaveState() and corrupt presets.json; prefer routing all saves through the async path or enforce the same file mutex + atomic write there.

Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-29T20:31:11.736Z
Learning: In MoonModules/WLED-MM wled00/presets.cpp, savePreset() must restore both presetToSave and playlistSave (and quickLoad if modified) on all early-return paths in the immediate API save branch (e.g., invalid index or missing fileDoc). Failing to restore leaves presetToSave > 0 (triggering unintended async save) and can drop a previously pending playlist save.
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 5, 2026

Important

Review skipped

Bot user detected.

To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 33700ee2-071d-412c-a959-25b595864ffa

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • ✅ Review completed - (🔄 Check again to review again)
📝 Walkthrough

Walkthrough

The changes improve handling of empty or incomplete JSON configuration files. Both config and presets initialization functions now detect empty files and rebuild them instead of accepting minimal or zero-length JSON as valid initialization states.

Changes

Cohort / File(s) Summary
Config Deserialization
wled00/cfg.cpp
Updated deserializeConfigFromFS() and deserializeConfigSec() to treat zero-length parsed JSON as failure cases, triggering buffer release, optional EEPROM fallback, and file rebuild.
Presets Initialization
wled00/presets.cpp
Modified initPresetsFile() to check existing file size and remove files smaller than 3 bytes before reinitializing, ensuring empty/minimal JSON files are rebuilt.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 A file that's hollow, void, and bare,
Gets caught by my detecting snare,
No phantom configs shall remain,
We rebuild them fresh again!
JSON wholesome, files so fair!

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: fixing handling of empty JSON objects in cfg.json and wsec.json files. It accurately reflects the primary bug fix across the changeset.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch copilot/fix-empty-json-issues

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@softhack007 softhack007 merged commit 72b1f59 into mdev Apr 5, 2026
36 checks passed
@softhack007 softhack007 deleted the copilot/fix-empty-json-issues branch April 5, 2026 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants