raphaelm / django-scopes

Safely separate multiple tenants in a Django database
Apache License 2.0
229 stars 15 forks source link

Help Understanding #22

Closed vabene1111 closed 3 years ago

vabene1111 commented 3 years ago

I am sry for asking because i am relatively certain that i am doing something wrong but i just dont know what ...

I have added a tenant ForeignKey field to all of my models and also the ScopedManager.

Now i started rewriting my queries with a filter and always got the error message that i need to have a scope active. Which makes sense because i did not have a with scope() call.

I added a middleware as stated in the documentation that wraps all calls in a with scope() call and also adds my tenant variable (space) to the request context.

class ScopeMiddleware:
    def __init__(self, get_response):
        self.get_response = get_response

    def __call__(self, request):
        if request.user.is_authenticated:
            request.space = request.user.userpreference.space

            with scope(space=request.space):
                return self.get_response(request)
        else:
            return self.get_response(request)

Now everything works but that is expected as you automatically add the required filters to all queries. As you state this is not recommended but i dont really see where i have forgotten to add a filter if everything just works magically.

Am i doing something wrong ? I understood that adding the with scope() call in the middleware is recommended but at the same time it conceals any mistakes i might have made. I am currently testing this manually by wrapping the middleware with as with scopes_disabled(): but that feels wrong.

I tried understanding this by looking at pretix but that was a little to much for me to understand just how this works 😂

But overall thank you very much for this package and also pretix, love to use it for all the CCC events :)

raphaelm commented 3 years ago

You're doing everything right. django-scopes does not fail loudly in the case you're looking at.

If you do a query to your protected model outside of a with scope block, django-scopes blows up in your face. However if you're inside a with scope block and forget the filter call, it fails silently and protects you by adding the filter.

I agree that failing loudly in both cases would be preferable. However when I created django-scopes, I did not find a way to make it do so reliably, because it's pretty hard to inspect a query before it is run. We'd basically need to ship our own database backend and parse and analyze the generated SQL again, which not only has an intense performance impact but can become incredibly complex when subqueries etc. occur.

So django-scopes is not a tool to help you find your mistakes. The only tool I know to do so is code review. django-scopes is a tool to help you sleep well despite not having a good tool to find the mistakes.

The recommendation for still adding the filters everywhere is (a) because we might switch to failing loudly if we find a way to do so and (b) because otherwise it's too easy to screw up when copying code etc. without realizing which context it previously ran in.

vabene1111 commented 3 years ago

thank you much for the fast and helpful answer and all the work you have put into this tool.

If this is the way it is intended to work then i am totally fine with it, i was just worries i might have done something wrong.

vabene1111 commented 3 years ago

I am super sorry to bother you again but i just cant figure this out: Where do i put the scopes_disabled() for the tests ?

I am using djangos default test runner manage.py test. My tests all live in a tests directory inside my app

i have looked at the pretalx repo and they add the monkeypatch code you show in the README.md in the __init__.py of the main test directoy but as far as i understand there documentation they use pytest and not the django test runner.

I have tried adding

from django.test import utils
from django_scopes import scopes_disabled

utils.setup_databases = scopes_disabled()(utils.setup_databases)

to every init inside my tests directory and also running it in the base classes setUp methods but i always get the following stacktrace

Traceback (most recent call last):
  File "manage.py", line 15, in <module>
    execute_from_command_line(sys.argv)
  File "F:\Developement\Django\recipes\venv\lib\site-packages\django\core\management\__init__.py", line 401, in execute_from_command_line
    utility.execute()
  File "F:\Developement\Django\recipes\venv\lib\site-packages\django\core\management\__init__.py", line 395, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "F:\Developement\Django\recipes\venv\lib\site-packages\django\core\management\commands\test.py", line 23, in run_from_argv
    super().run_from_argv(argv)
  File "F:\Developement\Django\recipes\venv\lib\site-packages\django\core\management\base.py", line 330, in run_from_argv
    self.execute(*args, **cmd_options)
  File "F:\Developement\Django\recipes\venv\lib\site-packages\django\core\management\base.py", line 371, in execute
    output = self.handle(*args, **options)
  File "F:\Developement\Django\recipes\venv\lib\site-packages\django\core\management\commands\test.py", line 53, in handle
    failures = test_runner.run_tests(test_labels)
  File "F:\Developement\Django\recipes\venv\lib\site-packages\django\test\runner.py", line 695, in run_tests
    old_config = self.setup_databases(aliases=databases)
  File "F:\Developement\Django\recipes\venv\lib\site-packages\django\test\runner.py", line 614, in setup_databases
    return _setup_databases(
  File "F:\Developement\Django\recipes\venv\lib\site-packages\django\test\utils.py", line 170, in setup_databases
    connection.creation.create_test_db(
  File "F:\Developement\Django\recipes\venv\lib\site-packages\django\db\backends\base\creation.py", line 88, in create_test_db
    self.connection._test_serialized_contents = self.serialize_db_to_string()
  File "F:\Developement\Django\recipes\venv\lib\site-packages\django\db\backends\base\creation.py", line 131, in serialize_db_to_string
    serializers.serialize("json", get_objects(), indent=None, stream=out)
  File "F:\Developement\Django\recipes\venv\lib\site-packages\django\core\serializers\__init__.py", line 128, in serialize
    s.serialize(queryset, **options)
  File "F:\Developement\Django\recipes\venv\lib\site-packages\django\core\serializers\base.py", line 90, in serialize
    for count, obj in enumerate(queryset, start=1):
  File "F:\Developement\Django\recipes\venv\lib\site-packages\django\db\backends\base\creation.py", line 125, in get_objects
    queryset = model._default_manager.using(
  File "F:\Developement\Django\recipes\venv\lib\site-packages\django_scopes\manager.py", line 14, in error
    raise ScopeError("A scope on dimension(s) {} needs to be active for this query.".format(
django_scopes.exceptions.ScopeError: A scope on dimension(s) space needs to be active for this query.
vabene1111 commented 3 years ago

In case anyone comes across the same issue in the future:

One can use the TEST_RUNNER = "project.module.foo.bar" to specify a custom test runner

The custom runner can look like this

from django.test.runner import DiscoverRunner
from django_scopes import scopes_disabled

class CustomTestRunner(DiscoverRunner):
    def run_tests(self, *args, **kwargs):
        with scopes_disabled():
            return super().run_tests(*args, **kwargs)

There is an excellent talk of Adam Johnson here explaining the django test runner