[fix] Fixed ssid boolean coercion#381
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRemoves boolean-substitution logic from the OpenWrt renderer and adds explicit boolean handling to the openwrt.jinja2 template so true/false values are rendered as '1'/'0' when the value is a non-string scalar. Adds unit tests that render multiple SSID variants (including strings containing "True") to ensure SSIDs are preserved and booleans are handled correctly. No public API or method signatures changed; indentation normalization and other post-processing are unchanged. Sequence Diagram(s)(Skipped — conditions for diagram generation not met.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@netjsonconfig/backends/openwrt/renderer.py`:
- Line 1: Replace the literal-regex calls to re.sub that change the strings
'True' and 'False' in netjsonconfig/backends/openwrt/renderer.py with
str.replace for simplicity and performance (i.e., swap each re.sub(...) that
targets the exact text 'True' or 'False' to use .replace(...)), ensure
replacement order doesn't accidentally re-replace results, and remove the
now-unused import re if there are no other regex uses in the file.
In `@tests/openwrt/test_wireless.py`:
- Around line 66-78: Add edge-case unit tests in tests/openwrt/test_wireless.py
to cover SSIDs that are exactly "True" and "False" and an SSID with "True" as a
mid-word (e.g., "MyTrueNetwork"); copy the pattern used by
test_render_wifi_issue_250_true_ssid/test_render_wifi_issue_250_false_ssid:
deepcopy self._wifi_netjson, set netjson["interfaces"][0]["wireless"]["ssid"] to
the exact value, create OpenWrt(netjson), compute expected_uci by replacing
"'MyWifiAP'" in self._wifi_uci with the new quoted SSID, and assert o.render()
== self._tabs(expected_uci) so the OpenWrt.render behavior is verified for these
edge cases.
- Around line 66-80: There are two consecutive blank lines between the test
methods test_render_wifi_issue_250_true_ssid and
test_render_wifi_issue_250_false_ssid; remove the extra blank line so there is
exactly one blank line separating the method definitions in
tests/openwrt/test_wireless.py to satisfy PEP8 formatting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7260d4bf-0f44-48f9-8fdd-a02ee4fcc58a
📒 Files selected for processing (2)
netjsonconfig/backends/openwrt/renderer.pytests/openwrt/test_wireless.py
📜 Review details
🧰 Additional context used
🪛 GitHub Actions: Netjsonconfig CI Build
tests/openwrt/test_wireless.py
[error] 74-74: Black check failed! Hint: did you forget to run openwisp-qa-format?
[error] 81-81: E303 too many blank lines (2)
Viscous106
force-pushed
the
fix-ssid-true-coercion
branch
2 times, most recently
from
56df23b to
ec90d55
Compare
Viscous106
force-pushed
the
fix-ssid-true-coercion
branch
from
a453bbf to
6277cad
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@netjsonconfig/backends/openwrt/renderer.py`:
- Around line 19-20: The current cleanup() in renderer.py mutates the rendered
string and therefore converts user strings like "True"/"False" as well as real
booleans; instead move boolean coercion to a typed stage by either updating
netjsonconfig/backends/openwrt/templates/openwrt.jinja2 to render booleans as
1/0 (use Jinja conditional or a custom filter) or normalizing booleans in the
renderer's input data before templating; locate the cleanup() call and the
output variable in renderer.py and remove the "'True'"/"'False'" replace, then
implement the conversion in the template (or in the data-prep function that
feeds the template) so only actual boolean values become 1/0 while literal
strings remain unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: baf8f36a-efa8-44a3-86e1-146bad74dfed
📒 Files selected for processing (2)
netjsonconfig/backends/openwrt/renderer.pytests/openwrt/test_wireless.py
📜 Review details
🔇 Additional comments (1)
tests/openwrt/test_wireless.py (1)
66-85: Good regression coverage for the reported substring cases.These tests cover the user-facing failure mode with spaced and mid-word SSIDs without overfitting to implementation details.
Viscous106
force-pushed
the
fix-ssid-true-coercion
branch
from
6277cad to
6e9d79d
Compare
There was a problem hiding this comment.
Pull request overview
Fixes incorrect coercion of SSIDs containing "True"/"False" by moving boolean-to-1/0 rendering out of post-processing and into the Jinja2 templates (where actual boolean types can be detected reliably).
Changes:
- Add explicit boolean rendering in OpenWrt/OpenWisp UCI Jinja2 templates via
is sameas true/false. - Remove the fragile
output.replace("True", "1").replace("False", "0")logic fromOpenWrtRenderer.cleanup(). - Add OpenWrt regression tests covering SSIDs that contain or equal
"True"/"False".
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
tests/openwrt/test_wireless.py |
Adds regression tests ensuring SSID strings aren’t altered by boolean coercion. |
netjsonconfig/backends/openwrt/templates/openwrt.jinja2 |
Implements type-aware boolean coercion at template render time. |
netjsonconfig/backends/openwrt/renderer.py |
Removes substring-based boolean replacement from cleanup. |
netjsonconfig/backends/openwisp/templates/openwrt.jinja2 |
Mirrors the OpenWrt template boolean coercion for OpenWisp output. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Viscous106
force-pushed
the
fix-ssid-true-coercion
branch
from
6e9d79d to
5eacd3c
Compare
Viscous106
force-pushed
the
fix-ssid-true-coercion
branch
from
5eacd3c to
2243fd9
Compare
Viscous106
force-pushed
the
fix-ssid-true-coercion
branch
from
2243fd9 to
ecdc1e5
Compare
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
@Viscous106 thanks, I simplified a bit the if/else chain in the jinja2 templates, should be good to merge now.
|
/backport 1.2 |
The naive .replace("True", "1") in cleanup() was incorrectly replacing substrings, e.g. SSID "TrueGait Living Guest" → "1Gait Living Guest".
Update: As suggested in the review, fixing this via string replacement in cleanup() is flawed because a real boolean True and a string exactly equal to "True" both arrive as the same string indistinguishable from one another.
The proper fix implemented here handles boolean coercion at the Jinja2 template level using the is sameas true / is sameas false type checks, and removes the boolean str.replace from cleanup() entirely. This ensures Python booleans are coerced to 1/0, while string values (like an SSID literally named "True") remain untouched as strings.
Fixes #383