-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Fixes #20524: Enhance API script scheduling validation #20616
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.
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
Fixes #20524: Enhance API script scheduling validation #20616
Conversation
jnovinger
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @pheus. This fix looks good, although I did note one other edge case (w/ a suggestion) that the original bug report did not mention. Can you please address that, since we're already making changes there?
Could you please also add the following tests for this? Probably in ScriptTest around line 910 in netbox/extras/tests/test_api.py:
def test_schedule_script_past_time_rejected(self):
"""Scheduling with past schedule_at should
fail"""
# POST with schedule_at = timezone.now() - timedelta(hours=1)
# Expect 400 with "must be in the future" error
def test_schedule_script_interval_only(self):
"""Interval without schedule_at should auto-set
schedule_at to now"""
# POST with interval=60, no schedule_at
# Verify job created with schedule_at set
def test_schedule_script_when_disabled(self):
"""Scheduling should fail when
script.scheduling_enabled=False"""
# POST with schedule_at or intervalLet me know if you have any questions.
| Validates the given data and ensures the necessary fields are populated. | ||
| """ | ||
| # Set the schedule_at time to now if only an interval is provided. | ||
| if 'interval' in data and 'schedule_at' not in data: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if 'interval' in data and 'schedule_at' not in data: | |
| if 'interval' in data and not data.get('schedule_at'): |
It seems there's a case not noticed in the original issue, where the API caller might pass something like {"scheduled": null, "interval": 60} and they'd end up with a recurring job that has no set start time. All other edge-case-y values that I could think of that might satisfy this conditional fail because they aren't a valid datetime format.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great point, and I agree. I’ll use .get() for both schedule_at and interval. Using .get() treats an explicit null the same as a missing key, so the “interval‑only” behavior still applies when schedule_at is null.
I’ll normalize & validate accordingly:
- If both are
None→ no scheduling (run now). - If
intervalis set andschedule_atis falsy →schedule_at = local_now(). - If
schedule_atis set → must be in the future. - If either is set and
scheduling_enabledisFalse→ reject.
Add validation for script scheduling to ensure the `schedule_at` time is in the future and default to the current time when only `interval` is provided. Incorporate `local_now` for accurate time comparisons. Fixes netbox-community#20524
70d7a0a to
f8b2efa
Compare
| @property | ||
| def python_class(self): | ||
| return self.TestScriptClass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make this a property to mirror the model’s cached_property. This returns the class object (not a bound function), which fixes the test setup and matches production behavior.
|
Thanks for the thoughtful review! I’ve pushed updates to address your suggestion and added tests:
Happy to tweak anything. |
Fixes: #20524
Tightens script scheduling validation:
schedule_atvalues in the past.intervalis provided, starts now and repeats.local_now()for accurate, timezone‑aware comparisons.Small, backward‑compatible fix to make scheduling behave as expected.