fix(tasks): fix error caused by relative import

[alexei]

Description of the Change

Running oauth2_provider.tasks.clear_tokens results in an error e.g.:

>>> from oauth2_provider.tasks import clear_tokens
>>> clear_tokens()
Traceback (most recent call last):
  File "[python3.9]/code.py", line 90, in runcode
    exec(code, self.locals)
  File "<console>", line 1, in <module>
  File "[site-packages]/celery/local.py", line 188, in __call__
    return self._get_current_object()(*a, **kw)
  File "[site-packages]/celery/app/task.py", line 392, in __call__
    return self.run(*args, **kwargs)
  File "[site-packages]/oauth2_provider/tasks.py", line 6, in clear_tokens
    from ...models import clear_expired  # noqa
ImportError: attempted relative import beyond top-level package

This update fixes the import path.

Checklist

• PR only contains one change (considered splitting up PR)
• unit-test added
• documentation updated
• CHANGELOG.md updated (only for user relevant changes)
• author name in AUTHORS

[n2ygk]

I don't quite understand what is broken and that you are fixing. That task works as designed when using celery-beat. It works fine relative as it is part of the package. If you are not using celery-beat you should never invoke that code but instead directly call the cleartokens management command or invoke the clear_expired function

[n2ygk]

Closing as not applicable.

[alexei]

I first saw this failing in production running inside celery. From Sentry:

My example was to illustrate what's going on. But clear_tokens too is a regular function, so I see no reason why it should fail when called inside the shell.

Regardless, the reason why the import is failing is because clear_expired is imported from ...models, but tasks and models are on the same level. At worst, the path would be .models i.e. from .models import clear_expired.

[n2ygk]

clear_tokens works as designed for use with celery-beat. I've tested it as such even though I don't use celery. It is not "regular" function as it is preceded by the celery @shared_task decorator. It is not meant to be invoked directlyl.

[alexei]

@n2ygk why are you arguing it works as indended when I just offered proof that it doesn't, and even upon reviewing the code it becomes clear the import can't possibly work since it looks up the function outside of the package?

[n2ygk]

@n2ygk why are you arguing it works as indended when I just offered proof that it doesn't, and even upon reviewing the code it becomes clear the import can't possibly work since it looks up the function outside of the package?

Good question. So how come it worked for me and the original contributor @Natureshadow? Can you please help me out here and document how this is supposed to be configured with celery? I had asked @Natureshadow to do so but he just provided a link so I configured celery in a test app as best I could and didn't see any errors and that the task showed up in the django admin intervface. I suspect that the @shared_task decorator is changing the relative path location somehow but it's a bit over my head when reading it.

Perhaps you can share a full sample app configuration that demonstrates the problem since it is not reproducible given this report alone and I frankly don't have unlimited time or knowledge to debug this. Cutting and pasting an error message without context is not enough.

Given the other conflict with Huey #1114 I'm seriously considering reverting #1070 and leaving it up to someone to document how to add a Celery or Huey autoloaded tasks rather than including it in this package.

[codecov]

Codecov Report

Merging #1123 (559478e) into master (c48b751) will not change coverage.
The diff coverage is 0.00%.

@@           Coverage Diff           @@
##           master    #1123   +/-   ##
=======================================
  Coverage   96.62%   96.62%           
=======================================
  Files          32       32           
  Lines        1806     1806           
=======================================
  Hits         1745     1745           
  Misses         61       61           

Impacted Files	Coverage Δ
oauth2_provider/tasks.py	0.00% <0.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c48b751...559478e. Read the comment docs.

[alexei]

Good question. So how come it worked for me and the original contributor @Natureshadow?

I don't think it did, really. I don't think you're liars either, but I checked #1070 and I see no proof it ever worked. Both you and @Natureshadow confirm the task is discovered, but none of you attested it actually ran. For example, at one point you're saying:

And see the admin interface and was able to autodiscover the task. I didn't get it completely working (econnrefused) but close enough to understand what I needed to.

---

Detective work aside, in my experience with Celery I'd say simply importing the task and running it inside the shell ought to do it. [unbound] celery tasks are regular functions and can be called directly like any other function.

Initially I started writing a smoke test, but I was faced with the choice of adding celery to the requirements or manipulating sys.modules, so I abandoned the idea. I agree to the point made by @Natureshadow in #1070 (comment):

I don't think it is worth adding that complexity to the test suite

But an alternative that would work as a smoke test would be to move the import statement at the top of the file. If there was an issue it would fail a lot sooner.

---

I don't have experience with Huey. Regardless, personally I'm ok with reverting the original commit if it's causing problems.

[n2ygk]

Good question. So how come it worked for me and the original contributor @Natureshadow?

I don't think it did, really. I don't think you're liars either, but I checked #1070 and I see no proof it ever worked. Both you and @Natureshadow confirm the task is discovered, but none of you attested it actually ran. For example, at one point you're saying:

You will note that the import statement is inside the function definition not top-level in the tasks.py file. As such, it is not executed in the context of importing tasks.py but in the context of the decorator function for which the function definition is an argument.

[alexei]

@n2ygk I reported a bug, showed proof of it and offered instructions to reproduce it, showed there was no proof that it ever worked, and offered to fix it. But rather than listening and possibly executing the code, it seems to me you're looking to argue and trying to be right or something like that. Whatever it is, it's not helping advance this conversation and not making the celery task run either. As such, I'll take a step back until we can have a reasonable conversation. Do let me know if you’re willing to do that.

Luckily, in my project I'm not using celery’s autodiscover feature, so I'll set up a custom task that imports clear_expired from oauth2_provider.models, where it actually lives, not the root of the virtual env like the builtin clear_tokens does.

[n2ygk]

@alexei I asked you to provide an example that reproduces the bug in context. You did not.

[alexei]

@n2ygk I showed you a screenshot with the error in production running in celery beat, and offered simple steps to reproduce it in the shell. Calling an unbound celery task directly is reasonable use. That said, I think I did my part of the deal. Besides, the error literally says you're importing from the outside of the package and the extra dots confirm it. If you don't connect the dots, there's nothing I can do about it.

[n2ygk]

@alexei With tremendous amounts of egg on my face, I have connected the dots and apologize profusely. It seems @Natureshadow supplied an untested PR #1070. I was sticking by his word over yours. That was a mistake.

I've configured Celery and reproduced the error and applied your fix and have documented how to use it here.

I'll be reverting #1070 and documenting how to do instead add the clear_tokens task into an app that uses DOT.

[alexei]

Apology accepted. I appreciate your looking into this.

What do you think about documenting the clear_expired function in the official docs? My thinking is, the cleartokens management command is documented, but that's only useful in a setup where one's using cron for periodic tasks. For those using Celery, Huey etc. that's not really useful, and sadly Django doesn't provide a mechanism to execute management commands programatically. Sure enough, writing a task to call clear_expired is trivial for either Celery or Huey, but a newcomer reading the official docs is unaware of it unless they check out the source or the DOT tutorial which btw I didn't know existed before this.

[n2ygk]

Apology accepted. I appreciate your looking into this.

What do you think about documenting the clear_expired function in the official docs? My thinking is, the cleartokens management command is documented, but that's only useful in a setup where one's using cron for periodic tasks. For those using Celery, Huey etc. that's not really useful, and sadly Django doesn't provide a mechanism to execute management commands programatically. Sure enough, writing a task to call clear_expired is trivial for either Celery or Huey, but a newcomer reading the official docs is unaware of it unless they check out the source or the DOT tutorial which btw I didn't know existed before this.

See #1128

[n2ygk]

@alexei PS: that dot-tutorial is just my sandbox. As you can see now at https://django-oauth-toolkit--1128.org.readthedocs.build/en/1128/tutorial/tutorial.html I've attempted to document this. Please feel free to review if you have the time.

[alexei]

Both look fine by me 👍