#36633 closed Bug (wontfix)
TaskBackend run_after validation assumes datetime when USE_TZ=True
| Reported by: | Chris M | Owned by: | |
|---|---|---|---|
| Component: | Tasks | Version: | 6.0 |
| Severity: | Release blocker | Keywords: | |
| Cc: | Jake Howard | Triage Stage: | Unreviewed |
| Has patch: | no | Needs documentation: | no |
| Needs tests: | no | Patch needs improvement: | no |
| Easy pickings: | no | UI/UX: | no |
Description
I’ve been working on an implementation of a TaskBackend using Google Cloud Tasks. I ran across a bit of confusion regarding the run_after parameter validation.
The DEP originally referenced that the value could be a datetime or a timedelta. The released implementation looks like it only documents it as a datetime, so I’m not sure if it was meant to support timedelta. However, I tried using a timedelta with the USE_TZ=True and the validation in BaseTaskBackend assumes that the run_after value is a datetime and I got an error
Calling code: my_task.using(run_after=timedelta(seconds=10)).enqueue()
Error: AttributeError: 'datetime.timedelta' object has no attribute 'utcoffset'
If I run the code using a timedelta and with USE_TZ=False then the error isn't thrown and my backend can process it.
The code throwing the error in the BaseTaskBackend is
if ( settings.USE_TZ and task.run_after is not None and not timezone.is_aware(task.run_after) ): raise InvalidTask("run_after must be an aware datetime.")
I think it would be best for this condition to verify that the provided run_after is a datetime before calling timezone.is_aware. I'm hoping for some confirmation if this is the right way to proceed.
I think it would be great to support timedelta directly as well by updating the type annotations on Task, but I couldn't determine through the PR history if this was removed during implementation.
Change History (5)
comment:1 by , 3 weeks ago
| Cc: | added |
|---|---|
| Severity: | Normal → Release blocker |
| Type: | Uncategorized → Bug |
comment:2 by , 3 weeks ago
| Triage Stage: | Unreviewed → Accepted |
|---|
follow-up: 5 comment:3 by , 3 weeks ago
| Resolution: | → wontfix |
|---|---|
| Status: | new → closed |
comment:4 by , 3 weeks ago
| Triage Stage: | Accepted → Unreviewed |
|---|
comment:5 by , 3 weeks ago
Thanks for the context. I figured there was probably a conversation about the timedelta that I was just missing. Personally, I think it is more confusing to have the time be static, when the transaction close time is dynamic, but I understand needing to make a call and go with it, not a problem.
If you are open to it, Jake, I'm happy to cut a PR against django-tasks for the type check before the timezone aware check. You are correct that it is failing, which is the final outcome either way. However, as a developer interacting with it, I was confused about what steps I should take, since the error had less to do with making the value timezone aware and more that I just passed in the wrong type completely. I think the clean up here could create a better developer experience.
I believe this is behaving as intended. The
timedeltafunctionality was removed post-DEP, because when interacting with the previous transaction handling, it wasn't obvious if it was "10 seconds after enqueue" or "10 seconds from.enqueuecall". We could add a type guard that only adatetimeis passed in, but given an exception is already raised, rather than strange behaviour, I suspect it's fine.