airflowctl: fix datetime.datetime CLI parameters rejecting all input - #70249
airflowctl: fix datetime.datetime CLI parameters rejecting all input#70249bramhanandlingala wants to merge 8 commits into
Conversation
|
@ bugraoz93, @dheerajturaga, @henry3260 and @potiuk @kaxil |
|
@ bugraoz93, @dheerajturaga, @henry3260 and @potiuk @kaxil @Lee-W @shahar1 Did I miss anyone in the list, please advise |
| """An ISO-8601 datetime string (date-only or full) is parsed into a datetime. | ||
|
|
||
| Regression test for https://github.com/apache/airflow/issues/70232: previously the | ||
| bare ``datetime.datetime`` class was used as the argparse ``type=`` callable, so | ||
| argparse called ``datetime.datetime(value)`` on the raw string, which always raised | ||
| a ``TypeError`` regardless of the input. | ||
| """ |
There was a problem hiding this comment.
| """An ISO-8601 datetime string (date-only or full) is parsed into a datetime. | |
| Regression test for https://github.com/apache/airflow/issues/70232: previously the | |
| bare ``datetime.datetime`` class was used as the argparse ``type=`` callable, so | |
| argparse called ``datetime.datetime(value)`` on the raw string, which always raised | |
| a ``TypeError`` regardless of the input. | |
| """ | |
| """An ISO-8601 datetime string (date-only or full) is parsed into a datetime.""" |
| "tuple": tuple, | ||
| "set": set, | ||
| "datetime.datetime": datetime.datetime, | ||
| "datetime.datetime": iso_datetime_type, |
There was a problem hiding this comment.
Could we add a test like this?
def test_command_factory_wires_iso_parser_to_datetime_params(self):
"""A generated datetime CLI arg parses an ISO date end to end."""
command_factory = CommandFactory()
dagrun_list_args = []
for group in command_factory.group_commands:
if group.name != "dagrun":
continue
for sub in group.subcommands:
if sub.name == "list":
dagrun_list_args = list(sub.args)
break
start_date_arg = next(a for a in dagrun_list_args if a.flags == ("--start-date",))
assert start_date_arg.kwargs["type"]("2026-07-01") == datetime.datetime(2026, 7, 1)
93e7f65 to
24e9f97
Compare
|
Hi @henry3260 Thanks for the detailed review and the helpful suggestion! I've updated the PR by simplifying the docstring and adding the end-to-end regression test to verify that CommandFactory wires iso_datetime_type for datetime CLI parameters. I'd appreciate it if you could take another look when you have a chance. |
potiuk
left a comment
There was a problem hiding this comment.
The fix itself is right. argparse calls type(value) on the raw string, so datetime.datetime("2026-07-01") could never work regardless of format, and routing through a real parser is the correct shape — following the dict → json_dict_type precedent already in that map rather than inventing a new one. The end-to-end wiring test that was requested in the earlier review is in place too, which is the test that would actually have caught the original bug.
Three small things, none of them blocking the approach.
datetime.date immediately above has the identical bug
"datetime.date": datetime.date maps to the bare class, so datetime.date("2026-07-01") raises TypeError in exactly the same way. It's latent right now — the live date parameters (partition_date_gte / partition_date_lte in operations.py) are served by a hand-written command with its own _parse_partition_date — but the next generated datetime.date parameter walks straight into the bug this PR just fixed. Closing it costs a few lines while the file is open.
fromisoformat() and the Z suffix differ by Python version
airflow-ctl supports >=3.10, and datetime.fromisoformat() only learned to accept a trailing Z in 3.11. So --start-date 2026-07-01T12:00:00Z parses on 3.11+ and fails on 3.10, for the same command against the same server. Normalising a trailing Z to +00:00 before parsing removes that split, and Z is the form the API itself emits, so users will paste it back in.
Smaller observations
tests/airflow_ctl/ctl/test_cli_config.py:371— a function-localimport datetime, while the same PR addsimport datetimeat module scope (line 21). Imports belong at the top of the file; the local one can go.
On the review state: the change request on this PR points at the gen-AI disclosure discussion rather than at the code, and the body now carries a disclosure paragraph. That block is shahar1's to lift once they're satisfied — I'm not approving around it.
This review was drafted by an AI-assisted tool and
confirmed by an Airflow maintainer. The findings
below are observations, not blockers; an Airflow
maintainer — a real person — will take the next look at the
PR. If you think a finding is mis-applied, please reply on
the PR and a maintainer will weigh in.More on how Airflow handles maintainer review:
contributing-docs/05_pull_requests.rst.
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
| @@ -637,7 +647,7 @@ def _python_type_from_string(type_name: str | type) -> type | Callable: | |||
| "tuple": tuple, | |||
| "set": set, | |||
| "datetime.date": datetime.date, | |||
There was a problem hiding this comment.
Same bug, one line up: datetime.date("2026-07-01") raises TypeError just like datetime.datetime did. Latent today because the live date params are served by a hand-written command, but the next generated datetime.date parameter inherits it. Worth fixing in the same PR:
| "datetime.date": datetime.date, | |
| "datetime.date": iso_date_type, | |
| "datetime.datetime": iso_datetime_type, |
…with an iso_date_type() alongside iso_datetime_type() using datetime.date.fromisoformat().
| if isinstance(val, datetime.datetime): | ||
| return val | ||
| try: | ||
| return datetime.datetime.fromisoformat(val) |
There was a problem hiding this comment.
fromisoformat() only accepts a trailing Z from Python 3.11 onwards, and airflow-ctl supports >=3.10. So --start-date 2026-07-01T12:00:00Z parses on 3.11+ and raises on 3.10 — same command, same server, different interpreter. Since Z is the form the API itself emits, users will paste it back in:
| return datetime.datetime.fromisoformat(val) | |
| return datetime.datetime.fromisoformat(val.replace("Z", "+00:00") if val.endswith("Z") else val) |
|
|
||
| def test_iso_datetime_type_returns_datetime_input_unchanged(self): | ||
| """A datetime.datetime input is returned as-is without re-parsing.""" | ||
| import datetime |
There was a problem hiding this comment.
datetime is imported at module scope (line 21) by this same PR, so this local import can go.
10174ba to
85d8f4f
Compare
closes: #70232
What
datetime.datetime-typed CLI parameters inairflowctl(e.g.dagrun list --start-date/--end-date) rejected every input value.Root cause
_python_type_from_string()incli_config.pymappeddatetime.datetimeto the bare class as the argparsetype=callable. argparse calls
type(value)on the raw CLI string, so ittried
datetime.datetime("2026-07-01")— but that constructor expectspositional
(year, month, day, ...)ints, not a string, so it alwaysraised
TypeError, regardless of the input format.Fix
Added
iso_datetime_type(), a real parser usingdatetime.datetime.fromisoformat(), and mappeddatetime.datetimetoit instead of the bare class. This mirrors the existing fix already
applied for
dict→json_dict_type.Gen-AI disclosure: I used a generative AI tool to help identify the root
cause, write tests, and draft the PR description. I reviewed, tested, and
verified all changes locally before submitting.
Was generative AI tooling used to co-author this PR?
Generated-by: Claude following the guidelines