Validate preference values set via the CLI (#7346)#10047
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
Walkthrough
ChangesCLI preference validation (Issue
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes ✨ Finishing Touches🧪 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 |
49b4d77 to
5fb1856
Compare
The CLI set-prefs path (save_pref -> Preferences.save_cli) wrote the raw value to the configuration database without any type validation and always reported success, unlike the GUI path which validates via _Preference.set(). Route save_cli through the same set() validation (set() now accepts an explicit user_id so it works outside a request context), and make setup.py set-prefs check the result and report preferences whose value was invalid. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
5fb1856 to
a868adb
Compare
asheshv
left a comment
There was a problem hiding this comment.
The threading of user_id through set() and routing save_cli via _Preference.set() is the right approach, but three issues:
- Critical — boolean prefs via
--input-filewill universally fail.setup.py:719builds the value list viastr(v). For native JSON booleans this produces'True'/'False'(capital T/F).save_prefonly coerces lowercase['true', 'false'], so the value reaches_Preference.set()as the string'True'and tripsassert isinstance(value, bool). Either normalize insave_pref(data['value'].lower() in ['true', 'false']) or stop stringifying booleans at thesetup.pyboundary. - Error message discarded.
res, _ = Preferences.save_cli(...)throws away the validator's diagnostic. Users see "invalid value" with no detail — the message should propagate up throughsave_prefand intosetup.py'sinvalid_value_prefsreport. - No new tests for
save_pref/save_cliwith invalid integer, valid integer-as-string, boolean, unknown options value, or missing preference. The validation surface is currently untested.
Non-blocking observation (pre-existing): setup.py:741 uses opt.split("=")[1], which truncates any value containing = (URLs, regexes, base64). Worth a follow-up issue.
…0105) PR #10047 made `Preferences.save_cli()` validate against the registered preference object, but `setup.py set-prefs` only enters `app_context()` and never calls `run_before_app_start()`, leaving `Preferences.modules` empty. Every CLI write then fell into the "Module 'X' is no longer in use." path and was reported as "Invalid value provided". Trigger registration explicitly (run_before_app_start enters both app and test_request contexts internally, satisfying registration callbacks that touch current_user) and propagate the typed error message from save_cli so users see the actual reason a value was rejected.
Summary
Fixes #7346.
Preferences set via the CLI (
setup.py set-prefs) were not validated:Preferences.save_cliwrote the raw value straight to the configuration database and always returned success, so an invalid value (e.g. a non-numeric string for an integer preference) was stored silently. The GUI path validates via_Preference.set().Fix:
_Preference.set()now accepts an optionaluser_id, so it can be used outside a request context (the CLI has nocurrent_user).Preferences.save_clinow looks up the preference (likePreferences.save) and delegates toset(), so the value is validated against the preference type.setup.py set-prefschecks the result and reports any preference whose value was invalid instead of silently claiming success.The GUI path is unaffected (
save_cliis only used by the CLI;update()callspref.set()directly).Changes
web/pgadmin/utils/preferences.py,web/setup.py🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes