Fix get_parameter_source() during type conversion and eager callbacks#3484
Conversation
kdeldycke
changed the title
get_parameter_source() during type conversion and eager callbacks
Record parameter source on the context immediately after consume_value(), before process_value() runs. In 8.4.0, set_parameter_source() was deferred until after type conversion and flag-group arbitration (0f71fe7, pallets#3403), so get_parameter_source() returned None inside ParamType.convert() and eager callbacks (pallets#3458). When several options share one parameter name, the losing option still sets a provisional source before process_value(); restore the previous source if arbitration rejects it so the winner's origin is not replaced unintentionally.
kdeldycke
force-pushed
the
fix-parameter-source-before-process-value
branch
from
547d9a4 to
0baa8db
Compare
kdeldycke
left a comment
kdeldycke
left a comment
There was a problem hiding this comment.
This fix is more generic than my own fix in Click Extra at: kdeldycke/click-extra@9fe4e8b
Tests are good to me and checks all variations. One thing I realized: in https://click.palletsprojects.com/en/stable/advanced/#parameter-modifications , we document the overriding of one parameter from the callback of another. I didn't know how the get_parameter_source would react in that case so I added a unittest to lockdown the current behavior. Also used the opportunity to add an admonition in the docs to highlight the danger of that kind of bypass.
Also added a reference to the PR in the changelog.
|
An alternative, long term solution would be to restructure the pipeline and the parameter loop that is happening above. But that is a bigger refactor for later. So this PR is a good local minimal patch for 8.4.1. |
|
Thanks @davidism for the merge! :) |
3 participants
Summary
Click 8.4.0 moved
set_parameter_source()to afterprocess_value()when fixing flag-group arbitration (#3403, #3404). That broke a case wasn't meant to change: anything that callsget_parameter_source()during type conversion or in an eager callback sawNone, even thoughconsume_value()had already figured out where the value came from (#3458).This is affecting Flask: with Click 8.4.0,
FLASK_DEBUG=1in the environment was ignored because_set_debugruns as an eager callback and could no longer see that the--debugflag was only at its default (pallets/flask#6025).The fix is to record the source right after
consume_value()and beforeprocess_value(), matching 8.3.3 timing for callbacks and custom types. Flag-group arbitration is the same as today.There is a second, narrower issue on the same code path: recording the source early means a losing option in a feature-switch group briefly overwrites
_parameter_sourcebefore arbitration runs. I am solving it by restoring the previous source when an option loses the slot. That path is easy to miss in tests (sharedoptsmay give the loser the same source as the winner), so there is a dedicated case intest_bool_flag_group_parameter_source. More detail is in the comments aroundhandle_parse_result.fixes #3458
Tests added / updated
tests/test_defaults.py— issue repro (ParamType.convert), eager callback, Flask-style_set_debugguardtests/test_options.py—test_bool_flag_group_parameter_source(includes a loser-restore case withenvvar, not only CLI +default_map)