django-oauth-toolkit icon indicating copy to clipboard operation
django-oauth-toolkit copied to clipboard

Multi database setup w/ read only endpoints

Open Amertz08 opened this issue 3 years ago • 7 comments

We're attempting to migrate our app to use read only db endpoints. In our implementation we decided that the read only endpoint should be the default endpoint i.e. you must explicitly decide you want to write. Which to us seems to make sense. Also we implemented our own database router as follows. Router docs. Which handles the direction of individual models read/writes.

class Router:
    def db_for_write(model, **hints):
        return "writer"

    def db_for_read(model, **hints):
        return "default"

This appears to be perfectly fine for areas where transactions are not used. However when transactions are used we get an error.

raise TransactionManagementError('select_for_update cannot be used outside of a transaction.')
django.db.transaction.TransactionManagementError: select_for_update cannot be used outside of a transaction

This emanates from Oauth2Validator.save_bearer_token which opens a transaction against"default" and at various points will call select_for_update. This can also happen when AbstractRefreshToken.revoke is called as it also opens a transaction.

Since we can't specify the using argument in the transaction.atomic we have resorted to implementing our own validator and refresh token model. Overriding the methods and copy pasting the original implementation with one that allows us to specify the using argument. For obvious reasons (we don't want to have to validate the implementation hasn't changed each oauth toolkit release) this is not ideal.

That all being said my initial questions are

What ways have other users solved this?

Is there a general Django convention for transaction connection management? e.g. assume you can always write against default

Amertz08 avatar Sep 11 '20 01:09 Amertz08

Is there any solutions for this issue? I'm getting django.db.transaction.TransactionManagementError: select_for_update cannot be used outside of a transaction when using DATABASE_ROUTERS Automatic database routing.

if is using "default", there will be no issue, but once using other naming, it will throw error mentioned above

MCOngTreal avatar Sep 22 '21 03:09 MCOngTreal

AbstractRefreshToken.revoke as well.

ippeiukai avatar Jun 17 '22 10:06 ippeiukai

This seems to work for now. (just specify the patched OAuth2Validator class as OAUTH2_VALIDATOR_CLASS)

from django.db import transaction
from oauth2_provider.models import AbstractRefreshToken
from oauth2_provider.oauth2_validators import OAuth2Validator as orig_OAuth2Validator
from wrapt import patch_function_wrapper

# https://github.com/jazzband/django-oauth-toolkit/issues/868

NON_DEFAULT_DB = 'writer'

class OAuth2Validator(orig_OAuth2Validator):
    @transaction.atomic(using=NON_DEFAULT_DB)
    def save_bearer_token(self, token, request, *args, **kwargs):
        return super().save_bearer_token(token, request, *args, **kwargs)

    @transaction.atomic(using=NON_DEFAULT_DB)
    def _save_id_token(self, jti, request, expires, *args, **kwargs):
        return super()._save_id_token(jti, request, expires, *args, **kwargs)


@patch_function_wrapper(AbstractRefreshToken, 'revoke')
def revoke(wrapped, instance, args, kwargs):
    with transaction.atomic(using=NON_DEFAULT_DB):
        return wrapped(*args, **kwargs)

ippeiukai avatar Jun 17 '22 10:06 ippeiukai

@ippeiukai If this looks like something that should be a fix to the library, please submit a PR!

n2ygk avatar Jun 17 '22 14:06 n2ygk

Thanks! I’ll give it a go when I have a time.

Any idea how it should be done by the way? I could add a config to the settings, but it feels wrong. There must be a way to automatically find out which database particular model writes to. 🤔

ippeiukai avatar Jun 17 '22 14:06 ippeiukai

@ippeiukai 🤷 replace the @transaction.atomic decorator with the context manager flavor so you can figure out the database from the token's model for using=? Not really sure as I've not used database routers.

See https://docs.djangoproject.com/en/4.0/topics/db/transactions/#managing-database-transactions

n2ygk avatar Jun 17 '22 14:06 n2ygk

Time ran out after a bit of desk research; theoretically this looks like how it should be done:

  • with a model
    from django.db import transaction, router
    with transaction.atomic(using=router.db_for_write(model)):
    
  • with an instance
    from django.db import transaction, router
    with transaction.atomic(using=router.db_for_write(instance._meta.model, instance=instance)):
    

ippeiukai avatar Jun 20 '22 02:06 ippeiukai

This feels like something that should be specified upstream in Django. I don't think this something we an easily generalize in DOT.

dopry avatar Oct 04 '23 15:10 dopry