Honor database assignment from router

[shaleh]

Fixes #868

Description of the Change

The token models might not be stored in the default database. There might not be a default database.
Instead, the code now relies on Django's routers to determine the actual database to use when creating
transactions. This required moving from decorators to context managers for those transactions.

An addition to Django's system checks is included to detect when the Token models are not all in the same database together. This is important because a transaction is only made for the database where AccessToken is stored.

To test the multiple database scenario, a new settings file was added which derives from settings.py
and then defines different databases and the routers needed to access them. The commit is larger than
might be expected because when there are multiple databases the Django tests have to be told which
databases to work on. Rather than copying the various test cases or making multiple database specific
ones the decision was made to add wrappers around the standard Django TestCase classes and programmatically
define the databases for them. This enables all of the same test code to work for both the one database
and the multi database scenarios with minimal maintenance costs. A tox environment that uses the
multi db settings file has been added to ensure both scenarios are always tested.

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

[shaleh]

The contributing doc says I should request a reviewer but I do not see a way to do that. Trying by using an @ reference.

@jazzband/django-oauth-toolkit

[dopry]

@shaleh this is an exceptional PR! Thank you for this contribution. I've only got a few nitpicks, but otherwise everything looks great. If you can address the comments inline, I'd be satisfied. I want to wait on @n2ygk for final approval.

[n2ygk]

See my comment about requiring a single database rather than warning about only one being in a transaction.

Also, please update the documentation in advanced_topics to talk about multi-database, especially the constraint that the DOT models have to be in the same database as auth_user due to the FK.

Once you resolve @dopry's requests I'm good to go with this.

Really nice work!

[shaleh]

Pushed a commit which attempts to address the PR comments. As the commit message says, the tox addition is not something which can be merged. I am uncertain how to proceed there. Not include it? Mark it as one not to run requiring occasional manual checking? Move the check from ready() to some other part of the code?

[n2ygk]

Pushed a commit which attempts to address the PR comments. As the commit message says, the tox addition is not something which can be merged. I am uncertain how to proceed there. Not include it? Mark it as one not to run requiring occasional manual checking? Move the check from ready() to some other part of the code?

Since it raises ImproperlyConfigured I think your test case has to expect that exception and return a successful test if the exception happens. You may want to make a single test just for this rather than running over the entire suite of tests. One test should be sufficient to prove that the invalid configuration exception has happened.

[shaleh]

I agree with you but I was struggling with how to implement it and have been in a time crunch so I have not devoted tons of energy into this. The problem with me putting the check in ready() is it fails really really early. Which was my goal rather than having things look ok and then fail later when users actually do things.

What I can try to do is use Django TestCase's ability to override a setting and see if I can capture this instead of using a separate settings file.

[n2ygk]

I agree with you but I was struggling with how to implement it and have been in a time crunch so I have not devoted tons of energy into this. The problem with me putting the check in ready() is it fails really really early. Which was my goal rather than having things look ok and then fail later when users actually do things.

What I can try to do is use Django TestCase's ability to override a setting and see if I can capture this instead of using a separate settings file.

I think you can leave the check in ready() but remove the added tox.ini line and special-case test settings and instead code up a single test case that does a @pytest.mark.oauth2_settings() that sets the multiple databases instead of multi_db_settings_invalid_token_configuration.py

So, yes, what you said:-)

[n2ygk]

@shaleh FYI - I'm in the midst of merging several other PRs so hold off for a bit but then you'll need to rebase and probably rename/resequence your migration(s).

[n2ygk]

@shaleh FYI - I'm in the midst of merging several other PRs so hold off for a bit but then you'll need to rebase and probably rename/resequence your migration(s).

@shaleh OK I've merged some PRs that cause merge conflicts as well as undetected (by git) conflicts with the migrations graph. Please go ahead and rebase or let me know if you want me to try.

[shaleh]

No worries, I can update things.

[shaleh]

After re-reading some Django docs, I realized the checks system was a better way to implement the configuration validation. It is marked as a database check so it will run on migrations but also general system checks should catch it.

[shaleh]

I still need to 1) add validation for those new checks and 2) explore ways to avoid magic on import.

[shaleh]

I just removed the magic. This breaks all of the pytest functional based testing.

Here is where I am in my understanding. Ideas welcomed.

When there are multiple databases, the Django TestCase needs to know which databases to use. It will not guess. A pytest functional test behind the scenes generates a Django TestCase in _django_db_helper in pytest-django. This is a fixture that is instantiated when any of the pytest-django 'db' fixtures are loaded which happens whenever the mark django_db is used. To define the databases they have to be passed into that mark. Unlike fixtures I am not aware of a way to wrap a mark in code.

What I think this means is the only way to get this working would be code that looks like this:

@pytest.mark.django_db(**make_database_names())
def test_mytest(....):
    # Test logic

which is also kinda ugly.

[shaleh]

There is a flake8 package that checks for debug lines. But ruff does not support external lint packages. All lints have to be ported into ruff.

[9128305]

Ruff also has similar rules https://docs.astral.sh/ruff/rules/debugger/#debugger-t100

[dopry]

What I think this means is the only way to get this working would be code that looks like this:

@pytest.mark.django_db(**make_database_names())
def test_mytest(....):
    # Test logic

which is also kinda ugly.

This is kinda what I expected... alternatively you could be explicit and not use a function. I strongly prefer not being too clever in testing. In my experience when I make my tests too clever I spend more time debugging my test harnesses than writing production code.

Personally I'd lean toward something along the lines of,

@override_settings(DATABASE=...)
@pytest.mark.django_db(...)
class MyMultiDBTestCase(...):
   ...

but mind you it's coming from a more general frame of mind not specific to DOT's existing test setup so might not work for some reason I'm not aware of at the moment.. I'm also not very familiar with pytest... so I don't know what constraints it adds.

I should have some free time later in the afternoon to look at this in more detail.

[shaleh]

ok, django_db mark now uses the retrieve_current_databases function.

What remains is:

• test and validate the new checks work
• assuming they do, update the documentation to mention the checks are available and what their failure means.

[shaleh]

@dopry the issue is not the definition of the DATABASES themselves. The issue is in Django's own TestCase. It has to have a definition of the databases allowed in testing defined on the class itself. I made the change in setUpClass because Django does database work at the class level before the per instance __init__ is ever called.

When a functional test from pytest uses pytest-django's django_db this internally generates a one off, local TestCase class and loads it essentially like a context manager. https://github.com/pytest-dev/pytest-django/blob/5598264ce8b2b333632efd4f06b7d740276953de/pytest_django/fixtures.py#L212
I did not see a good way to interfere with this. Open to ideas. Instead I added the function call to the marks.

This means there is no implicit magic. If someone were to write a new test and not follow along with the pattern the multi db test will fail saying something like "Database 'alpha' cannot be used in tests. Define databases=." So the failure should be obvious enough and they can see what the rest of the project's tests are doing to follow along.

[shaleh]

Documentation for contributors was updated to document what changed with tests as well as to my original addition to mention Django's checks.

ok, I think at this point I have resolved open concerns. Let me know if there is anything I can improve. Thank you for the interest and attention. It is super appreciated.

[n2ygk]

@dopry Please provide final approval on this.

@shaleh Sorry for the delay in reviewing; I was on vacation last week.

[shaleh]

All good. These things move at their own pace. Thank you for the typo fixes.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.08%. Comparing base (460387a) to head (f78ba9b).
Report is 4 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1450      +/-   ##
==========================================
+ Coverage   97.06%   97.08%   +0.02%     
==========================================
  Files          33       34       +1     
  Lines        2145     2161      +16     
==========================================
+ Hits         2082     2098      +16     
  Misses         63       63              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[n2ygk]

@dopry sorry to be a pain but are you able to do a final review of this. If not I'll just go ahead and merge it. I'm pretty sure it's all good at this point.

[dopry]

No worries. I appreciate the reminder, been distracted by life

[dopry]

@shaleh Thank you for all your work on this and addressing my issues. I don't have the time for functional tests right now, but all my concerns with code style and developer accessibility are addressed! I got into the DB stuff with it after my last comments and I see why I never used it. And again, Thank you so much for your hard work on this!