SameerMesiah97 commented on code in PR #64751:
URL: https://github.com/apache/airflow/pull/64751#discussion_r3112082724
##########
airflow-core/src/airflow/serialization/encoders.py:
##########
@@ -210,9 +210,22 @@ def encode_deadline_alert(d: DeadlineAlert |
SerializedDeadlineAlert) -> dict[st
"""
from airflow.sdk.serde import serialize
+ if isinstance(d, dict):
+ return d
+
+ interval = d.interval
+
+ # Resolve dynamic interval providers (e.g. VariableInterval)
+ resolve = getattr(interval, "resolve", None)
+ if callable(resolve):
+ interval = resolve()
+
Review Comment:
> getattr(interval, "resolve", None) will call .resolve() on any object that
has it, not just VariableInterval. For example, if someone accidentally passes
an object with an unrelated .resolve() that returns a string, the encoder calls
it and then crashes later with a confusing error. We can use a Protocol so the
contract is explicit and caught at lint time.
>
> ```
> # With Protocol — explicit contract
> from typing import Protocol
>
> class ResolvableInterval(Protocol):
> def resolve(self) -> timedelta: ...
>
> # Now you can use it in type annotations:
> def __init__(self, interval: timedelta | ResolvableInterval): ...
> ```
The interval is now being checked like this before deadline evaluation under
the new approach. Please see the below:
```
if isinstance(interval, VariableInterval):
interval = interval.resolve()
```
The use of a Protocol is a good idea if we want to introduce other means of
configuring intervals besides Variables later on but I believe an object type
check is sufficient for now..
Let me know what you think.
--
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]
