Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Validate --event-time-start is before --event-time-end #10820

Merged
merged 6 commits into from
Oct 7, 2024
Merged

Validate --event-time-start is before --event-time-end #10820

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
13f25cc
Validate that `event_time_start` is before `event_time_end` when pass…
QMalcolm Oct 3, 2024
5dc16b9
Add changie doc
QMalcolm Oct 3, 2024
ee994c1
If `--event-time-end` is not specififed, ensure `--event-time-start` …
QMalcolm Oct 3, 2024
ca9b381
Fixup error message about event_time_start and event_time_end
QMalcolm Oct 3, 2024
7b995c5
Move logic to validate `event_time` cli flags to `flags.py`
QMalcolm Oct 7, 2024
cad0006
Update validation of `--event-time-start` against `datetime.now` to u…
QMalcolm Oct 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changes/unreleased/Features-20241003-170529.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
kind: Features
body: Ensure `--event-time-start` is before `--event-time-end`
time: 2024-10-03T17:05:29.984398-05:00
custom:
Author: QMalcolm
Issue: "10786"
22 changes: 22 additions & 0 deletions core/dbt/cli/flags.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import os
import sys
from dataclasses import dataclass
from datetime import datetime
from importlib import import_module
from pathlib import Path
from pprint import pformat as pf
Expand Down Expand Up @@ -313,6 +314,9 @@
self._assert_mutually_exclusive(params_assigned_from_default, ["SELECT", "INLINE"])
self._assert_mutually_exclusive(params_assigned_from_default, ["SELECTOR", "INLINE"])

# Check event_time configs for validity
self._validate_event_time_configs()

# Support lower cased access for legacy code.
params = set(
x for x in dir(self) if not callable(getattr(self, x)) and not x.startswith("__")
Expand Down Expand Up @@ -349,6 +353,24 @@
elif flag_set_by_user:
set_flag = flag

def _validate_event_time_configs(self) -> None:
event_time_start = (
getattr(self, "EVENT_TIME_START") if hasattr(self, "EVENT_TIME_START") else None
)
if event_time_start is not None:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: do we need this level of nesting?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so? We need to check whether or not event_time_start is None, as it only makes sense to do the validation if event_time_start is a datetime value. We could get around this layer of nesting by doing something like:

if not hasattr(self, "EVENT_TIME_START"):
  return

event_time_start = getattr(self, "EVENT_TIME_START")
...

I'm personally not a huge fan of multiple returns. I can see how doing an early return would reduce our level of if's from 2 down to 1. However, given we're only at 2 levels of if's that seems unnecessary.

Side note that's not applicable here, but a corollary to my above "I avoid multiple returns".

If there were more levels of nesting, I still likely woudn't add early returns. I'd instead end up looking at splitting it into multiple functions, thus continuing allowing to maintain low levels of if context nesting and avoiding early multiple returns in a function.

event_time_end = (

Check warning on line 361 in core/dbt/cli/flags.py

View check run for this annotation

Codecov / codecov/patch

core/dbt/cli/flags.py#L361 

Added line #L361 was not covered by tests
getattr(self, "EVENT_TIME_END") if hasattr(self, "EVENT_TIME_END") else None
)

if event_time_end is not None and event_time_start >= event_time_end:
raise DbtUsageException(

Check warning on line 366 in core/dbt/cli/flags.py

View check run for this annotation

Codecov / codecov/patch

core/dbt/cli/flags.py#L365-L366 

Added lines #L365 - L366 were not covered by tests
"Value for `--event-time-start` must be less than `--event-time-end`"
)
elif event_time_start >= datetime.now():
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are two separate conditions, so let's do two ifs?
Also, do we need to specify UTC timezone for datetime.now()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To the first part, they are not two separate ifs. This was intentional as the commit message for ee994c1 states. We only need to do the second check if event_time_end is not set. Said another way, the relation of event_time_start to datetime.now() is irrelevant if event_time_end is set, and thus doesn't need to be checked.

As for the second part, maybe? The issue is that the click parsing of datetimes creates naive datetimes (lacking timezone information). It won't even parse something like --event-time-start "2024-10-07 13:45:00+00:00". It'll give you an error like:

Invalid value for '--event-time-start': '2024-10-07 00:00:00+00:00' does not match the formats '%Y-%m-%d', '%Y-%m-%dT%H:%M:%S', '%Y-%m-%d %H:%M:%S'.

So our options are to either assume what they give us is UTC, and give the passed in --event-time-start a timezone, or compare to a naive datetime. Given the rest of our system assumes UTC, you are right, we should probably do the former instead of what we are currently doing (the latter).

raise DbtUsageException(

Check warning on line 370 in core/dbt/cli/flags.py

View check run for this annotation

Codecov / codecov/patch

core/dbt/cli/flags.py#L369-L370 

Added lines #L369 - L370 were not covered by tests
"Value for `--event-time-start` must be less than the current time if `--event-time-end` is not specififed"
)

def fire_deprecations(self):
"""Fires events for deprecated env_var usage."""
[dep_fn() for dep_fn in self.deprecated_env_var_warnings]
Expand Down
55 changes: 55 additions & 0 deletions tests/functional/cli/test_option_interaction_validations.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
from datetime import datetime

import pytest
from freezegun import freeze_time

from dbt.tests.util import run_dbt


class TestEventTimeEndEventTimeStart:
@pytest.mark.parametrize(
"event_time_start,event_time_end,expect_pass",
[
("2024-10-01", "2024-10-02", True),
("2024-10-02", "2024-10-01", False),
],
)
def test_option_combo(self, project, event_time_start, event_time_end, expect_pass):
try:
run_dbt(
[
"build",
"--event-time-start",
event_time_start,
"--event-time-end",
event_time_end,
]
)
assert expect_pass
except Exception as e:
assert (
"Value for `--event-time-start` must be less than `--event-time-end`"
in e.__str__()
)
assert not expect_pass


class TestEventTimeStartCurrent_time:
@pytest.mark.parametrize(
"event_time_start,current_time,expect_pass",
[
("2024-10-01", "2024-10-02", True),
("2024-10-02", "2024-10-01", False),
],
)
def test_option_combo(self, project, event_time_start, current_time, expect_pass):
with freeze_time(datetime.fromisoformat(current_time)):
try:
run_dbt(["build", "--event-time-start", event_time_start])
assert expect_pass
except Exception as e:
assert (
"Value for `--event-time-start` must be less than the current time if `--event-time-end` is not specififed"
in e.__str__()
)
assert not expect_pass
Loading