Repository navigation
grist(update-choice-options): refuse widgetOptions/choices shape failures by name (#6588) - #6719
Merged
Merged
Conversation
![developing-today[bot]](https://s.lequ.pw/_assets/images/gravatars/gravatar-user-420.png){kind=avatar}
…ures by name (#6588) merge_choices shredded a string choices value into one-character choices via list(), passed non-string entries through unchecked, and a non-object widgetOptions died as a raw ValueError out of dict() -- while the read-back verification compared against the merged value, so a corrupted merge verified as good. check_widget_options now validates the stored shape (JSON object; choices, when present and not null, a list of strings) and raises the named WidgetOptionsError before any PATCH is built; get_widget_options validates what it parses (including invalid JSON), and run() maps the error to a clean stderr message and exit 1, matching the documented exit-code contract. Null/absent choices keep their existing no-choices meaning. Tests: string/dict choices, non-object widgetOptions, and non-string entries refused at merge level; CLI-level cases pin exit 1 with no PATCH sent for string choices, non-object, and invalid-JSON stored values. Existing controls (append order/other keys, idempotence, empty options, duplicate additions, input not mutated, API timeouts) unchanged and green: 15 tests.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 26 minutes. View limit details
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. |
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
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
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.
Fixes #6588.
The bug
pm-stack/grist/update-choice-options.pynever checked the shape of the storedwidgetOptionsit read:choiceswas shredded bylist()into one-character choices ("code"→['c', 'o', 'd', 'e']), PATCHed, and then verified as good because read-back compares against the merged value;widgetOptionsdied as a rawValueErrortraceback out ofdict(), breaking the tool's documented exit-code contract;choicesentries passed through unchecked and would be written back as JSON1 / null / truechoices.The fix
check_widget_options: the stored value must be a JSON object, andchoices(when present and not null) must be a list of strings. Anything else raises the namedWidgetOptionsErrorwith the offending type/entry named.merge_choicesvalidates before merging — so the refusal happens before any PATCH is built and the stored column is untouched on a shape failure.get_widget_optionsvalidates what it parses, including invalid JSON (same named error, with the column named).run()catchesWidgetOptionsErrorlike the timeout path: clean message on stderr, exit 1 — the documented contract.choiceskeep their existing "no choices" meaning; all existing merge controls are unchanged.Tests
pm-stack/grist/test_update_choice_options.py— 15 tests, all green on the laptop (python3 -m unittest test_update_choice_options):choices, dictchoices, non-objectwidgetOptions(string/list/number), non-string entries — plus null-choices semantics.code -> ['c', 'o', 'd', 'e', 'id']).Filed from a queue-worker fallback shift (broker backend down, #6619): the issue's comments carry the claim and the release.
Open in [code]smith · Need help on this PR? Tag
@codesmith-botwith what you need.