Skip to content

Commit 8142f52

Browse files
committed
refactor: simplify CronTrigger timezone field and add deduplication tests
Change timezone field from Optional[str] to str since the default="UTC" ensures it's never None at runtime. This makes the type annotation accurately reflect the actual behavior. Changes: - Change CronTrigger.timezone from Optional[str] to str - Remove None check from timezone validator (Pydantic default handles it) - Remove Optional import (no longer needed) - Rename test_duplicate_triggers_with_identical_values to test_trigger_model_equality_with_identical_values to clarify it tests Pydantic model equality, not business logic deduplication - Remove test_none_timezone_defaults_to_utc (timezone=None no longer valid) - Add integration tests for trigger deduplication in create_crons function - test_create_crons_deduplicates_identical_triggers - test_create_crons_keeps_triggers_with_different_timezones - test_create_crons_keeps_triggers_with_different_expressions - test_create_crons_complex_deduplication_scenario The integration tests verify actual database behavior to ensure the deduplication logic in verify_graph.py correctly creates only one DatabaseTriggers row per (expression, timezone) pair. Signed-off-by: Sparsh <sparsh.raj30@gmail.com>
1 parent 077ca50 commit 8142f52

4 files changed

Lines changed: 274 additions & 45 deletions

File tree

state-manager/app/models/trigger_models.py

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

77
# Cache available timezones at module level to avoid repeated filesystem queries
@@ -20,7 +20,7 @@ class TriggerStatusEnum(str, Enum):
2020
class CronTrigger(BaseModel):
2121
type: Literal[TriggerTypeEnum.CRON] = Field(default=TriggerTypeEnum.CRON, description="Type of the trigger")
2222
expression: str = Field(..., description="Cron expression for the trigger")
23-
timezone: Optional[str] = Field(default="UTC", description="Timezone for the cron expression (e.g., 'America/New_York', 'Europe/London', 'UTC')")
23+
timezone: str = Field(default="UTC", description="Timezone for the cron expression (e.g., 'America/New_York', 'Europe/London', 'UTC')")
2424

2525
@field_validator("expression")
2626
@classmethod
@@ -31,9 +31,7 @@ def validate_expression(cls, v: str) -> str:
3131

3232
@field_validator("timezone")
3333
@classmethod
34-
def validate_timezone(cls, v: Optional[str]) -> str:
35-
if v is None:
36-
return "UTC"
34+
def validate_timezone(cls, v: str) -> str:
3735
if v not in _AVAILABLE_TIMEZONES:
3836
raise ValueError(f"Invalid timezone: {v}. Must be a valid IANA timezone (e.g., 'America/New_York', 'Europe/London', 'UTC')")
3937
return v

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

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -59,11 +59,6 @@ def test_invalid_timezone(self):
5959
assert "Invalid timezone" in error_msg
6060
assert "Invalid/Timezone" in error_msg
6161

62-
def test_none_timezone_defaults_to_utc(self):
63-
"""Test that None timezone defaults to UTC"""
64-
trigger = CronTrigger(expression="0 9 * * *", timezone=None)
65-
assert trigger.timezone == "UTC"
66-
6762
def test_complex_cron_expression_with_timezone(self):
6863
"""Test complex cron expression with timezone"""
6964
trigger = CronTrigger(expression="0 0 1,15 * *", timezone="America/Los_Angeles")
@@ -118,8 +113,12 @@ def test_invalid_trigger_with_invalid_timezone(self):
118113
errors = exc_info.value.errors()
119114
assert len(errors) > 0
120115

121-
def test_duplicate_triggers_with_identical_values(self):
122-
"""Test creating two triggers with identical values"""
116+
def test_trigger_model_equality_with_identical_values(self):
117+
"""Test Pydantic model equality for triggers with identical values.
118+
119+
Note: This test verifies model-level equality only, not deduplication logic.
120+
The actual deduplication of triggers is handled in verify_graph.py's create_crons function.
121+
"""
123122
# Create first trigger
124123
trigger1 = Trigger(
125124
value={"type": TriggerTypeEnum.CRON, "expression": "0 9 * * *", "timezone": "America/New_York"}

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

Lines changed: 0 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,6 @@ async def test_create_crons_with_america_new_york_timezone():
3131
graph_template.namespace = "test_ns"
3232
graph_template.triggers = [
3333
Trigger(
34-
type=TriggerTypeEnum.CRON,
3534
value=make_trigger_value("0 9 * * *", "America/New_York")
3635
)
3736
]
@@ -65,7 +64,6 @@ async def test_create_crons_with_default_utc_timezone():
6564
graph_template.namespace = "test_ns"
6665
graph_template.triggers = [
6766
Trigger(
68-
type=TriggerTypeEnum.CRON,
6967
value=make_trigger_value("0 9 * * *") # No timezone specified
7068
)
7169
]
@@ -90,7 +88,6 @@ async def test_create_crons_with_europe_london_timezone():
9088
graph_template.namespace = "test_ns"
9189
graph_template.triggers = [
9290
Trigger(
93-
type=TriggerTypeEnum.CRON,
9491
value=make_trigger_value("0 17 * * *", "Europe/London")
9592
)
9693
]
@@ -115,11 +112,9 @@ async def test_create_crons_with_multiple_different_timezones():
115112
graph_template.namespace = "test_ns"
116113
graph_template.triggers = [
117114
Trigger(
118-
type=TriggerTypeEnum.CRON,
119115
value=make_trigger_value("0 9 * * *", "America/New_York")
120116
),
121117
Trigger(
122-
type=TriggerTypeEnum.CRON,
123118
value=make_trigger_value("0 17 * * *", "Europe/London")
124119
)
125120
]
@@ -166,11 +161,9 @@ async def test_create_crons_deduplicates_same_expression_and_timezone():
166161
graph_template.namespace = "test_ns"
167162
graph_template.triggers = [
168163
Trigger(
169-
type=TriggerTypeEnum.CRON,
170164
value=make_trigger_value("0 9 * * *", "America/New_York")
171165
),
172166
Trigger(
173-
type=TriggerTypeEnum.CRON,
174167
value=make_trigger_value("0 9 * * *", "America/New_York")
175168
)
176169
]
@@ -197,11 +190,9 @@ async def test_create_crons_keeps_same_expression_different_timezones():
197190
graph_template.namespace = "test_ns"
198191
graph_template.triggers = [
199192
Trigger(
200-
type=TriggerTypeEnum.CRON,
201193
value=make_trigger_value("0 9 * * *", "America/New_York")
202194
),
203195
Trigger(
204-
type=TriggerTypeEnum.CRON,
205196
value=make_trigger_value("0 9 * * *", "Europe/London")
206197
)
207198
]
@@ -228,7 +219,6 @@ async def test_create_crons_trigger_time_is_datetime():
228219
graph_template.namespace = "test_ns"
229220
graph_template.triggers = [
230221
Trigger(
231-
type=TriggerTypeEnum.CRON,
232222
value=make_trigger_value("0 9 * * *", "America/New_York")
233223
)
234224
]
@@ -243,26 +233,3 @@ async def test_create_crons_trigger_time_is_datetime():
243233
call_kwargs = mock_db_class.call_args[1]
244234
assert isinstance(call_kwargs['trigger_time'], datetime)
245235

246-
@pytest.mark.asyncio
247-
async def test_create_crons_with_null_timezone_normalizes_to_utc():
248-
"""Test create_crons with null/None timezone normalizes to UTC via model validation"""
249-
graph_template = MagicMock()
250-
graph_template.name = "test_graph"
251-
graph_template.namespace = "test_ns"
252-
graph_template.triggers = [
253-
Trigger(
254-
type=TriggerTypeEnum.CRON,
255-
value={"type": TriggerTypeEnum.CRON, "expression": "0 9 * * *", "timezone": None} # Explicit None
256-
)
257-
]
258-
259-
with patch('app.tasks.verify_graph.DatabaseTriggers') as mock_db_class:
260-
mock_db_class.return_value = MagicMock()
261-
mock_db_class.insert_many = AsyncMock()
262-
263-
await create_crons(graph_template)
264-
265-
# Verify timezone was normalized to "UTC" (not None)
266-
call_kwargs = mock_db_class.call_args[1]
267-
assert call_kwargs['timezone'] == "UTC"
268-
assert call_kwargs['timezone'] is not None

0 commit comments

Comments
 (0)