From 7d05a59b9d46a415d85937630d1d812cf477f60a Mon Sep 17 00:00:00 2001 From: Yuri Shevtsov Date: Tue, 19 May 2026 19:43:26 -0400 Subject: [PATCH 1/2] Fix 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, #3403), so get_parameter_source() returned None inside ParamType.convert() and eager callbacks (#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. --- CHANGES.rst | 2 + src/click/core.py | 14 +++-- tests/test_defaults.py | 123 +++++++++++++++++++++++++++++++++++++++++ tests/test_options.py | 62 +++++++++++++++++++++ 4 files changed, 196 insertions(+), 5 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index 0af01f6476..23459bd83b 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -5,6 +5,8 @@ Version 8.4.1 Unreleased +- ``get_parameter_source()`` is available during eager callbacks and type + conversion again. :issue:`3458` - Zsh completion scripts parse correctly on Windows. :issue:`3277` - Shell completion of `Choice` `Enum` values produces a valid completion result. :issue:`3015` diff --git a/src/click/core.py b/src/click/core.py index 3544c03b3b..3944bb4403 100644 --- a/src/click/core.py +++ b/src/click/core.py @@ -2609,6 +2609,11 @@ def handle_parse_result( with augment_usage_errors(ctx, param=self): value, source = self.consume_value(ctx, opts) + # Record the source before processing so eager callbacks and type + # conversion can inspect it. Restored after arbitration if this + # option loses a feature-switch group. + ctx.set_parameter_source(self.name, source) + # Display a deprecation warning if necessary. if ( self.deprecated @@ -2654,14 +2659,13 @@ def handle_parse_result( ) if is_winner: - ctx.set_parameter_source(self.name, source) if self.expose_value: ctx.params[self.name] = value ctx._param_default_explicit[self.name] = self._default_explicit - elif existing_source is None: - # Nothing has claimed the slot yet. Record at least our source so downstream - # lookups don't return ``None``. - ctx.set_parameter_source(self.name, source) + elif existing_source is not None: + # Lost arbitration; restore the winning option's source. + ctx.set_parameter_source(self.name, existing_source) + # else: keep the provisional source recorded before process_value. return value, args diff --git a/tests/test_defaults.py b/tests/test_defaults.py index 49f0ac6ed0..88b3e01d8f 100644 --- a/tests/test_defaults.py +++ b/tests/test_defaults.py @@ -1,8 +1,11 @@ +import os + import pytest import click from click import UNPROCESSED from click._utils import UNSET +from click.core import ParameterSource @pytest.mark.parametrize( @@ -265,6 +268,126 @@ def cli(ctx, name): assert f"source={expected_source}" in result.output +def test_parameter_source_during_paramtype_convert(runner): + """``get_parameter_source()`` is available during ``ParamType.convert``. + + Uses the reproducer from https://github.com/pallets/click/issues/3458. + """ + + class Source(click.ParamType): + name = "source" + + def convert(self, value, param, ctx): + return { + "value": value, + "source": ctx.get_parameter_source(param.name), + } + + @click.command() + @click.option("--default", type=Source(), default="/tmp/file") + @click.option("--nodefault", type=Source()) + def cli(default, nodefault): + click.echo(f"default: {default}") + click.echo(f"nodefault: {nodefault}") + + result = runner.invoke(cli, []) + assert not result.exception + assert "default: {'value': '/tmp/file', 'source': " in result.output + assert "'source': None}" not in result.output.split("default:")[1].split("\n")[0] + assert ( + result.output == "default: {'value': '/tmp/file', 'source': " + f"{ParameterSource.DEFAULT!r}}}\nnodefault: None\n" + ) + + result = runner.invoke(cli, ["--default", "cli", "--nodefault", "also"]) + assert not result.exception + assert ( + "default: {'value': 'cli', 'source': " + f"{ParameterSource.COMMANDLINE!r}}}" in result.output + ) + assert ( + "nodefault: {'value': 'also', 'source': " + f"{ParameterSource.COMMANDLINE!r}}}" in result.output + ) + + +def test_parameter_source_during_eager_callback(runner): + """``get_parameter_source()`` is available during eager callbacks. + + Regression test for https://github.com/pallets/click/issues/3458. + """ + + def eager_cb(ctx, param, value): + source = ctx.get_parameter_source(param.name) + click.echo(f"callback source={source.name if source else None}") + + @click.command() + @click.option( + "--flag/--no-flag", + default=False, + is_eager=True, + callback=eager_cb, + expose_value=False, + ) + def cli(): + source = click.get_current_context().get_parameter_source("flag") + click.echo(f"final source={source.name}") + + result = runner.invoke(cli, []) + assert not result.exception + assert "callback source=DEFAULT" in result.output + assert "final source=DEFAULT" in result.output + + result = runner.invoke(cli, ["--flag"]) + assert not result.exception + assert "callback source=COMMANDLINE" in result.output + assert "final source=COMMANDLINE" in result.output + + +def test_flask_debug_env_not_stomped_by_default_flag(runner, monkeypatch): + """Eager callback must not overwrite env when the flag used its default. + + Covers the Flask ``_set_debug`` pattern (pallets/flask#6025). Regression test + for https://github.com/pallets/click/issues/3458. + """ + + monkeypatch.delenv("APP_DEBUG", raising=False) + + def set_debug(ctx, param, value): + source = ctx.get_parameter_source(param.name) + if source is not None and source in ( + ParameterSource.DEFAULT, + ParameterSource.DEFAULT_MAP, + ): + return None + os.environ["APP_DEBUG"] = "1" if value else "0" + return value + + @click.command() + @click.option( + "--debug/--no-debug", + default=False, + is_eager=True, + expose_value=False, + callback=set_debug, + ) + def cli(): + click.echo(f"APP_DEBUG={os.environ.get('APP_DEBUG', '')}") + + monkeypatch.setenv("APP_DEBUG", "1") + result = runner.invoke(cli, []) + assert result.exit_code == 0 + assert result.output.strip() == "APP_DEBUG=1" + + result = runner.invoke(cli, ["--debug"]) + assert result.exit_code == 0 + assert result.output.strip() == "APP_DEBUG=1" + + result = runner.invoke(cli, ["--no-debug"]) + assert result.exit_code == 0 + assert result.output.strip() == "APP_DEBUG=0" + + def test_lookup_default_override_respected(runner): """A subclass override of ``lookup_default()`` should be called by Click internals, not bypassed by a private method. diff --git a/tests/test_options.py b/tests/test_options.py index 25e649d045..44cb58f6df 100644 --- a/tests/test_options.py +++ b/tests/test_options.py @@ -2895,6 +2895,68 @@ def cli(enable_xyz): assert result.output == repr(expected) +@pytest.mark.parametrize( + ("opts", "args", "invoke_kwargs", "expected_value", "expected_source"), + [ + # https://github.com/pallets/click/issues/3458 + pytest.param( + [ + ("--without-xyz", {"flag_value": False}), + ("--with-xyz", {"flag_value": True, "default": True}), + ], + [], + {}, + True, + "DEFAULT", + id="explicit-default-wins", + ), + pytest.param( + [ + ("--without-xyz", {"flag_value": False}), + ("--with-xyz", {"flag_value": True, "default": True}), + ], + ["--without-xyz"], + {}, + False, + "COMMANDLINE", + id="cmdline-wins", + ), + pytest.param( + [ + ("--without-xyz", {"flag_value": False}), + ("--with-xyz", {"flag_value": True, "default": True}), + ], + ["--without-xyz"], + {"default_map": {"enable_xyz": True}}, + False, + "COMMANDLINE", + id="loser-default-map-restores-winner-source", + ), + ], +) +def test_bool_flag_group_parameter_source( + runner, opts, args, invoke_kwargs, expected_value, expected_source +): + """``get_parameter_source()`` stays correct for feature-switch groups. + + Regression test for https://github.com/pallets/click/issues/3458. + """ + + @click.command() + @click.pass_context + def cli(ctx, enable_xyz): + source = ctx.get_parameter_source("enable_xyz") + click.echo(f"value={enable_xyz!r} source={source.name}") + + for opt_name, opt_kwargs in opts: + cli = click.option(opt_name, "enable_xyz", **opt_kwargs)(cli) + + result = runner.invoke(cli, args, **invoke_kwargs) + assert result.exit_code == 0, result.output + assert f"value={expected_value!r}" in result.output + assert f"source={expected_source}" in result.output + + @pytest.mark.parametrize( ("opts", "args", "expected"), [ From 0baa8db07736fc7ad3d3eed97d4c73b0059c63e1 Mon Sep 17 00:00:00 2001 From: Kevin Deldycke Date: Thu, 21 May 2026 14:44:08 +0200 Subject: [PATCH 2/2] Document ctx.params bypass with test and doc --- CHANGES.rst | 3 +-- docs/advanced.md | 14 ++++++++++++-- src/click/core.py | 5 ++++- tests/test_defaults.py | 27 +++++++++++++++++++++++++++ 4 files changed, 44 insertions(+), 5 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index 23459bd83b..eb32cd87d1 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -6,12 +6,11 @@ Version 8.4.1 Unreleased - ``get_parameter_source()`` is available during eager callbacks and type - conversion again. :issue:`3458` + conversion again. :issue:`3458` :issue:`3484` - Zsh completion scripts parse correctly on Windows. :issue:`3277` - Shell completion of `Choice` `Enum` values produces a valid completion result. :issue:`3015` - Version 8.4.0 ------------- diff --git a/docs/advanced.md b/docs/advanced.md index 45e36ef753..889f5b4103 100644 --- a/docs/advanced.md +++ b/docs/advanced.md @@ -144,8 +144,18 @@ it's good to know that the system works this way. click.echo(f"{url}: {fp.code}") In this case the callback returns the URL unchanged but also passes a -second `fp` value to the callback. What's more recommended is to pass -the information in a wrapper, however: +second `fp` value to the callback. + +.. caution:: + + Writing to :attr:`~click.Context.params` directly bypasses Click's + per-parameter pipeline: :meth:`~click.Context.get_parameter_source` returns + ``None`` for it, the value is never run through any :class:`~click.ParamType` + conversion. And if the key collides with a declared parameter, then the + shared-name arbitration loses information about where the value came from. + Prefer the wrapper pattern below when any of those semantics matter. + +What's more recommended is to pass the information in a wrapper, however: .. click:example:: diff --git a/src/click/core.py b/src/click/core.py index 3944bb4403..bf6c813aac 100644 --- a/src/click/core.py +++ b/src/click/core.py @@ -2665,7 +2665,10 @@ def handle_parse_result( elif existing_source is not None: # Lost arbitration; restore the winning option's source. ctx.set_parameter_source(self.name, existing_source) - # else: keep the provisional source recorded before process_value. + # else: ctx.params[self.name] was populated by code that bypassed + # handle_parse_result (from another option's callback for example). Keep + # the provisional source recorded before process_value so downstream + # lookups don't return ``None``. return value, args diff --git a/tests/test_defaults.py b/tests/test_defaults.py index 88b3e01d8f..d7c1d80993 100644 --- a/tests/test_defaults.py +++ b/tests/test_defaults.py @@ -388,6 +388,33 @@ def cli(): assert result.output.strip() == "APP_DEBUG=0" +def test_parameter_source_on_parse_result_bypass(runner): + """A losing option keeps its provisional source when ``ctx.params[name]`` + is populated by code that bypassed ``handle_parse_result``. + + This replicate the pattern documented in the "Parameter Modifications" section + of ``docs/advanced.md``. This test highlight the current behavior of + ``get_parameter_source()`` but is not intended as a contract enforcement. + """ + + def hijack(ctx, param, value): + ctx.params["target"] = "hijacked" + return value + + @click.command() + @click.option("--hijacker", is_eager=True, callback=hijack, expose_value=False) + @click.option("--target", default="default_value") + @click.pass_context + def cli(ctx, target): + source = ctx.get_parameter_source("target") + click.echo(f"value={target} source={source.name if source else 'None'}") + + result = runner.invoke(cli, ["--hijacker", "anything"]) + assert result.exit_code == 0, result.output + assert "value=hijacked" in result.output + assert "source=DEFAULT" in result.output + + def test_lookup_default_override_respected(runner): """A subclass override of ``lookup_default()`` should be called by Click internals, not bypassed by a private method.