SEPURI-SAI-KRISHNA commented on PR #72171:
URL: https://github.com/apache/airflow/pull/72171#issuecomment-5728553802
Expanding here on the inline thread about `trigger=` expressions the sweep
cannot read, since that thread is now outdated and collapses by default.
I checked the two cases reported there and then went through the guard for
the other cases in the same category instead of fixing only the reported ones.
| `trigger=` expression | constructions | flagged unreadable | outcome
before |
| -------------------------- | ------------: | ------------------ |
------------------------------------------------------ |
| `SomeTrigger(...)` | 1 | no | covered
|
| `trigger` (bare name) | 0 | yes | covered
through the allowlist |
| `self._trigger` | 0 | **no** | silently
missed |
| `triggers[kind]` | 0 | **no** | silently
missed |
| `A() if flag else self._t` | **1** | **no** | worse:
looked covered, but only one branch was checked |
| `TRIGGERS[kind](x=1)` | 1 | no | recorded
with an **empty** class name |
The fifth row was the main one I wanted to fix. It ends up in `DEFER_SITES`,
so the site appears in the parametrized test and looks covered, but the
unreadable branch contributes nothing. Someone looking at the test ids would
see the file listed and assume it had been checked.
I found the sixth case while doing the sweep. A construction whose callee
cannot be resolved is still a `Call`, so it was kept. However,
`PENDING_MIGRATION` and `UNCONFIGURABLE_TRIGGERS` are keyed by class name. An
unnamed construction can therefore never match either allowlist, and the
failure would end up saying `defers to without passing ...`.
I fixed this as you suggested. `trigger_constructions` now returns `None`
instead of `[]` whenever the expression cannot be resolved. That covers bare
references, attributes, subscripts, conditionals where either branch is
unreadable, and calls where the callee cannot be named.
`find_unreadable_defer_sites` now checks for exactly that `None` result
instead of checking for `ast.Name`. Anything the sweep cannot resolve therefore
has to be added to `UNREADABLE_DEFER_SITES` or the suite fails. The entries use
`ast.unparse`, so a future entry shows the actual expression.
I also cleaned up two things while I was there.
The two almost identical walks are now a single `walk_defer_sites()`
generator that both functions use. This also removes the duplicated
`self.defer` matching that came up earlier in the review.
The sweep also no longer skips files whose `path.parent.name` is not
`operators` or `sensors`. `defer` is a `BaseOperator` method, so a site can
appear anywhere. A nested subpackage would previously have been silently
skipped. There are no such sites today, so the counts are unchanged: 113 defer
sites, 44 hand built hook constructions, and one allowlisted `operators/eks.py`
entry.
`test_unreadable_trigger_expressions_resolve_to_none` is parametrized over
all eight shapes. Five of the eight fail with the previous implementation, so
the test is checking the actual gap rather than just exercising the new code.
There are no affected provider sites today. Apart from the allowlisted bare
name in `operators/eks.py`, the only trigger expressions that are not a plain
`SomeTrigger(...)` call are the two `EmrContainerTrigger` conditionals in
`operators/emr.py` and `sensors/emr.py`, where both branches are readable, and
one `self.trigger_class(...)` in `sensors/bedrock.py`. So this was a latent
hole in the guard rather than a current gap.
I also checked four related cases and left them unchanged:
* `find_hand_built_hooks` still relies on the callee name ending in `Hook`,
so a hook created through a variable or `self.hook_class` would not be found.
That is a limitation of the current heuristic and fixing it would need a
different approach.
* `find_waiter_triggers()` imports every trigger module, so a partial
install can turn an `ImportError` into a collection error. This was already
noted earlier in the review and will go into the prek hook follow up, together
with changing the exception lists to use relative paths instead of bare
filenames. There are no colliding basenames today.
* A hook construction using `**kwargs` would report all three parameters as
missing. That fails loudly rather than passing silently, so it is a false
positive rather than a hole. There are no such cases today.
* `read_trigger_name` returns the attribute name for a callee like
`self.trigger_class(...)` in `sensors/bedrock.py`, so that site is recorded as
`trigger_class` rather than a real class name. It passes today because all
three parameters are passed there, and a site like that would fail loudly
rather than silently, so I left it as is.
Finally, `defer()` takes `trigger` as a keyword only argument, so the
keyword lookup cannot miss a positional argument. That case is safe by
construction.
---
Drafted-by: Claude Opus 5; reviewed by @SEPURI-SAI-KRISHNA before posting
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
