Skip to content

Commit 2e28da4

Browse files
committed
refactor: use discriminated unions for Trigger model to eliminate redundant validation
Changes: - Refactored Trigger.value from dict to typed TriggerValue union - Added discriminated union support using Literal type for CronTrigger.type - Removed redundant CronTrigger.model_validate() call in verify_graph.py - Updated all tests to include type field in trigger value dicts Benefits: - Single validation at request time (no re-validation needed) - Type safety with IDE autocomplete and static analysis - Cleaner code: trigger.value.expression vs trigger.value["expression"] - Extensible: easy to add new trigger types to the union - All 38 trigger-related tests passing Also bumped python-sdk version to 0.0.3b2
1 parent 9d55322 commit 2e28da4

5 files changed

Lines changed: 46 additions & 34 deletions

File tree

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
version = "0.0.3b1"
1+
version = "0.0.3b2"

state-manager/app/models/trigger_models.py

Lines changed: 14 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
1-
from pydantic import BaseModel, Field, field_validator, model_validator
1+
from pydantic import BaseModel, Field, field_validator
22
from enum import Enum
33
from croniter import croniter
4-
from typing import Self, Optional
4+
from typing import Union, Annotated, Optional, Literal
55
from zoneinfo import available_timezones
66

77
# Cache available timezones at module level to avoid repeated filesystem queries
@@ -18,6 +18,7 @@ class TriggerStatusEnum(str, Enum):
1818
TRIGGERING = "TRIGGERING"
1919

2020
class CronTrigger(BaseModel):
21+
type: Literal[TriggerTypeEnum.CRON] = Field(default=TriggerTypeEnum.CRON, description="Type of the trigger")
2122
expression: str = Field(..., description="Cron expression for the trigger")
2223
timezone: Optional[str] = Field(default="UTC", description="Timezone for the cron expression (e.g., 'America/New_York', 'Europe/London', 'UTC')")
2324

@@ -37,14 +38,16 @@ def validate_timezone(cls, v: Optional[str]) -> str:
3738
raise ValueError(f"Invalid timezone: {v}. Must be a valid IANA timezone (e.g., 'America/New_York', 'Europe/London', 'UTC')")
3839
return v
3940

41+
# Union type for all trigger types - add new trigger types here
42+
TriggerValue = Annotated[Union[CronTrigger], Field(discriminator="type")]
43+
4044
class Trigger(BaseModel):
45+
"""
46+
Extensible trigger model using discriminated unions.
47+
To add a new trigger type:
48+
1. Add the enum value to TriggerTypeEnum
49+
2. Create a new trigger class (e.g., WebhookTrigger) with type field
50+
3. Add it to the TriggerValue Union
51+
"""
4152
type: TriggerTypeEnum = Field(..., description="Type of the trigger")
42-
value: dict = Field(default_factory=dict, description="Value of the trigger")
43-
44-
@model_validator(mode="after")
45-
def validate_trigger(self) -> Self:
46-
if self.type == TriggerTypeEnum.CRON:
47-
CronTrigger.model_validate(self.value)
48-
else:
49-
raise ValueError(f"Unsupported trigger type: {self.type}")
50-
return self
53+
value: TriggerValue = Field(..., description="Value of the trigger")

state-manager/app/tasks/verify_graph.py

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
from app.singletons.logs_manager import LogsManager
1212
from app.models.trigger_models import TriggerStatusEnum, TriggerTypeEnum
1313
from app.models.db.trigger import DatabaseTriggers
14+
from app.models.trigger_models import CronTrigger
1415

1516
# Cache UTC timezone at module level to avoid repeated instantiation
1617
UTC = ZoneInfo("UTC")
@@ -105,13 +106,12 @@ async def verify_inputs(graph_template: GraphTemplate, registered_nodes: list[Re
105106
return errors
106107

107108
async def create_crons(graph_template: GraphTemplate):
108-
# Build a map of (expression, timezone) -> validated CronTrigger for deduplication
109+
# Build a map of (expression, timezone) -> CronTrigger for deduplication
109110
triggers_to_create = {}
110111
for trigger in graph_template.triggers:
111112
if trigger.type == TriggerTypeEnum.CRON:
112-
# Validate through CronTrigger model to normalize timezone (None -> "UTC")
113-
from app.models.trigger_models import CronTrigger
114-
cron_trigger = CronTrigger.model_validate(trigger.value)
113+
# trigger.value is already a validated CronTrigger instance
114+
cron_trigger = trigger.value
115115
triggers_to_create[(cron_trigger.expression, cron_trigger.timezone)] = cron_trigger
116116

117117
current_time = datetime.now(UTC).replace(tzinfo=None)

state-manager/tests/unit/models/test_trigger_models.py

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -84,27 +84,28 @@ def test_valid_trigger_with_cron_and_timezone(self):
8484
"""Test creating a valid trigger with CRON type and timezone"""
8585
trigger = Trigger(
8686
type=TriggerTypeEnum.CRON,
87-
value={"expression": "0 9 * * *", "timezone": "America/New_York"}
87+
value={"type": TriggerTypeEnum.CRON, "expression": "0 9 * * *", "timezone": "America/New_York"}
8888
)
8989
assert trigger.type == TriggerTypeEnum.CRON
90-
assert trigger.value["expression"] == "0 9 * * *"
91-
assert trigger.value["timezone"] == "America/New_York"
90+
assert trigger.value.expression == "0 9 * * *"
91+
assert trigger.value.timezone == "America/New_York"
9292

9393
def test_valid_trigger_with_cron_without_timezone(self):
9494
"""Test creating a valid trigger with CRON type without timezone"""
9595
trigger = Trigger(
9696
type=TriggerTypeEnum.CRON,
97-
value={"expression": "0 9 * * *"}
97+
value={"type": TriggerTypeEnum.CRON, "expression": "0 9 * * *"}
9898
)
9999
assert trigger.type == TriggerTypeEnum.CRON
100-
assert trigger.value["expression"] == "0 9 * * *"
100+
assert trigger.value.expression == "0 9 * * *"
101+
assert trigger.value.timezone == "UTC" # Should default to UTC
101102

102103
def test_invalid_trigger_with_invalid_cron_expression(self):
103104
"""Test creating a trigger with invalid cron expression"""
104105
with pytest.raises(ValidationError) as exc_info:
105106
Trigger(
106107
type=TriggerTypeEnum.CRON,
107-
value={"expression": "invalid cron"}
108+
value={"type": TriggerTypeEnum.CRON, "expression": "invalid cron"}
108109
)
109110

110111
errors = exc_info.value.errors()
@@ -115,7 +116,7 @@ def test_invalid_trigger_with_invalid_timezone(self):
115116
with pytest.raises(ValidationError) as exc_info:
116117
Trigger(
117118
type=TriggerTypeEnum.CRON,
118-
value={"expression": "0 9 * * *", "timezone": "Invalid/Zone"}
119+
value={"type": TriggerTypeEnum.CRON, "expression": "0 9 * * *", "timezone": "Invalid/Zone"}
119120
)
120121

121122
errors = exc_info.value.errors()

state-manager/tests/unit/tasks/test_create_crons.py

Lines changed: 19 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,14 @@
1414
from app.tasks.verify_graph import create_crons
1515
from app.models.trigger_models import Trigger, TriggerTypeEnum
1616

17+
# Helper function to create trigger value dict
18+
def make_trigger_value(expression: str, timezone: str = None):
19+
"""Helper to create trigger value dict with required 'type' field"""
20+
result = {"type": TriggerTypeEnum.CRON, "expression": expression}
21+
if timezone is not None:
22+
result["timezone"] = timezone
23+
return result
24+
1725

1826
@pytest.mark.asyncio
1927
async def test_create_crons_with_america_new_york_timezone():
@@ -24,7 +32,7 @@ async def test_create_crons_with_america_new_york_timezone():
2432
graph_template.triggers = [
2533
Trigger(
2634
type=TriggerTypeEnum.CRON,
27-
value={"expression": "0 9 * * *", "timezone": "America/New_York"}
35+
value=make_trigger_value("0 9 * * *", "America/New_York")
2836
)
2937
]
3038

@@ -58,7 +66,7 @@ async def test_create_crons_with_default_utc_timezone():
5866
graph_template.triggers = [
5967
Trigger(
6068
type=TriggerTypeEnum.CRON,
61-
value={"expression": "0 9 * * *"} # No timezone specified
69+
value=make_trigger_value("0 9 * * *") # No timezone specified
6270
)
6371
]
6472

@@ -83,7 +91,7 @@ async def test_create_crons_with_europe_london_timezone():
8391
graph_template.triggers = [
8492
Trigger(
8593
type=TriggerTypeEnum.CRON,
86-
value={"expression": "0 17 * * *", "timezone": "Europe/London"}
94+
value=make_trigger_value("0 17 * * *", "Europe/London")
8795
)
8896
]
8997

@@ -108,11 +116,11 @@ async def test_create_crons_with_multiple_different_timezones():
108116
graph_template.triggers = [
109117
Trigger(
110118
type=TriggerTypeEnum.CRON,
111-
value={"expression": "0 9 * * *", "timezone": "America/New_York"}
119+
value=make_trigger_value("0 9 * * *", "America/New_York")
112120
),
113121
Trigger(
114122
type=TriggerTypeEnum.CRON,
115-
value={"expression": "0 17 * * *", "timezone": "Europe/London"}
123+
value=make_trigger_value("0 17 * * *", "Europe/London")
116124
)
117125
]
118126

@@ -159,11 +167,11 @@ async def test_create_crons_deduplicates_same_expression_and_timezone():
159167
graph_template.triggers = [
160168
Trigger(
161169
type=TriggerTypeEnum.CRON,
162-
value={"expression": "0 9 * * *", "timezone": "America/New_York"}
170+
value=make_trigger_value("0 9 * * *", "America/New_York")
163171
),
164172
Trigger(
165173
type=TriggerTypeEnum.CRON,
166-
value={"expression": "0 9 * * *", "timezone": "America/New_York"}
174+
value=make_trigger_value("0 9 * * *", "America/New_York")
167175
)
168176
]
169177

@@ -190,11 +198,11 @@ async def test_create_crons_keeps_same_expression_different_timezones():
190198
graph_template.triggers = [
191199
Trigger(
192200
type=TriggerTypeEnum.CRON,
193-
value={"expression": "0 9 * * *", "timezone": "America/New_York"}
201+
value=make_trigger_value("0 9 * * *", "America/New_York")
194202
),
195203
Trigger(
196204
type=TriggerTypeEnum.CRON,
197-
value={"expression": "0 9 * * *", "timezone": "Europe/London"}
205+
value=make_trigger_value("0 9 * * *", "Europe/London")
198206
)
199207
]
200208

@@ -221,7 +229,7 @@ async def test_create_crons_trigger_time_is_datetime():
221229
graph_template.triggers = [
222230
Trigger(
223231
type=TriggerTypeEnum.CRON,
224-
value={"expression": "0 9 * * *", "timezone": "America/New_York"}
232+
value=make_trigger_value("0 9 * * *", "America/New_York")
225233
)
226234
]
227235

@@ -244,7 +252,7 @@ async def test_create_crons_with_null_timezone_normalizes_to_utc():
244252
graph_template.triggers = [
245253
Trigger(
246254
type=TriggerTypeEnum.CRON,
247-
value={"expression": "0 9 * * *", "timezone": None} # Explicit None
255+
value={"type": TriggerTypeEnum.CRON, "expression": "0 9 * * *", "timezone": None} # Explicit None
248256
)
249257
]
250258

0 commit comments

Comments
 (0)