mirror of
https://github.com/ChuckBuilds/LEDMatrix.git
synced 2026-08-01 16:58:06 +00:00
Address review findings on target_fps validation and config resolution
- Reject floats and bools before int() in the target_fps save path. A JSON
body can carry them, where int(90.5) silently stored 90 and true stored 1.
Form posts send strings, so '90.5' already failed in int().
- Assert the template's target_fps is 100, not merely an int, so the
documented default is actually pinned.
- Empty-config precedence: keeping the `and config` check deliberately, now
spelled out in the comment and covered by a test. Both managers default to
the same config/config.json, so falling through cannot pick up a different
file's settings; treating {} as an answer would instead return {} when the
first manager simply hasn't loaded yet, silently disabling every setting
read through the property -- the failure this property exists to fix.
Suite 1446 passed. The float-rejection test was checked to fail without the
guard.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ
This commit is contained in:
co-authored by
Claude Sonnet 5
parent
809d676d3b
commit
99d0ee2208
@@ -199,6 +199,14 @@ class BasePlugin(ABC):
|
||||
# Only a real mapping is usable: callers do .get() on this and feed
|
||||
# the result to numeric code, so handing back whatever a stub or a
|
||||
# half-built manager returned would fail later and further away.
|
||||
#
|
||||
# An empty dict is treated as "nothing here yet" rather than a
|
||||
# valid answer, so resolution continues to the next source. Both
|
||||
# managers default to the same config/config.json, so falling
|
||||
# through cannot pick up a different file's settings -- but it does
|
||||
# rescue the case where the first manager simply hasn't loaded yet,
|
||||
# which would otherwise return {} and silently disable every
|
||||
# setting read through this property.
|
||||
if isinstance(config, dict) and config:
|
||||
return config
|
||||
return {}
|
||||
|
||||
@@ -303,6 +303,21 @@ class TestBasePluginGlobalConfig:
|
||||
)
|
||||
assert plugin.global_config["target_fps"] == 100
|
||||
|
||||
def test_empty_plugin_manager_config_falls_through(self, mock_display_manager):
|
||||
"""An empty first source means "not loaded yet", not "the answer".
|
||||
|
||||
Both managers default to the same config/config.json, so falling
|
||||
through cannot pick up a different file. Returning {} here instead
|
||||
would silently disable every setting read through this property --
|
||||
the exact failure this property exists to fix.
|
||||
"""
|
||||
plugin = self._plugin(
|
||||
mock_display_manager,
|
||||
self._manager_with({"target_fps": 100}), # cache_manager
|
||||
self._manager_with({}), # plugin_manager: empty
|
||||
)
|
||||
assert plugin.global_config["target_fps"] == 100
|
||||
|
||||
def test_returns_empty_dict_when_no_config_manager(self, mock_display_manager):
|
||||
# Plain objects: no config_manager attribute at all.
|
||||
plugin = self._plugin(mock_display_manager, object(), object())
|
||||
@@ -356,4 +371,4 @@ class TestBasePluginGlobalConfig:
|
||||
import json
|
||||
with open("config/config.template.json") as fh:
|
||||
template = json.load(fh)
|
||||
assert isinstance(template.get("target_fps"), int)
|
||||
assert template.get("target_fps") == 100
|
||||
|
||||
@@ -201,6 +201,21 @@ class TestConfigAPI:
|
||||
saved = mock_config_manager.save_config_atomic.call_args[0][0]
|
||||
assert saved['web_display_autostart'] is True
|
||||
|
||||
@pytest.mark.parametrize('value', [90.5, 90.0, True])
|
||||
def test_save_target_fps_rejects_non_integer_json(self, client, mock_config_manager, value):
|
||||
"""int() would truncate silently: 90.5 -> 90, True -> 1.
|
||||
|
||||
Only JSON can carry these; a form post sends '90.5', which int()
|
||||
already rejects.
|
||||
"""
|
||||
response = client.post(
|
||||
'/api/v3/config/main',
|
||||
data=json.dumps({'target_fps': value}),
|
||||
content_type='application/json',
|
||||
)
|
||||
|
||||
assert response.status_code == 400
|
||||
|
||||
@pytest.mark.parametrize('value', ['20', '250', 'fast'])
|
||||
def test_save_target_fps_rejects_out_of_range(self, client, mock_config_manager, value):
|
||||
"""Values ScrollHelper would silently clamp are reported instead."""
|
||||
|
||||
@@ -753,8 +753,18 @@ def save_main_config():
|
||||
# would have been quietly altered is reported rather than appearing to
|
||||
# save and then behaving differently.
|
||||
if 'target_fps' in data and data['target_fps'] not in ('', None):
|
||||
raw_target_fps = data['target_fps']
|
||||
# A JSON body can carry real floats and bools, where int() would
|
||||
# silently truncate: 90.5 would save as 90, and true as 1. Reject
|
||||
# them rather than storing a value the user did not ask for. Form
|
||||
# posts arrive as strings, so '90.5' still fails in int() below.
|
||||
if isinstance(raw_target_fps, (bool, float)):
|
||||
return jsonify({
|
||||
'status': 'error',
|
||||
'message': "Invalid value for target_fps: must be an integer"
|
||||
}), 400
|
||||
try:
|
||||
target_fps = int(data['target_fps'])
|
||||
target_fps = int(raw_target_fps)
|
||||
except (ValueError, TypeError):
|
||||
return jsonify({
|
||||
'status': 'error',
|
||||
|
||||
Reference in New Issue
Block a user