From ea95f3f7a437e4eefd27f7feb562c18e96934d12 Mon Sep 17 00:00:00 2001 From: Gary Reynolds Date: Fri, 4 Nov 2016 07:04:48 +1100 Subject: [PATCH] Refactor ImproperlyConfigured to system check framework - ensure TENANT_APPS is defined in settings.py - ensure TENANT_MODEL is defined in settings.py - ensure TenantSyncRouter appears in DATABASE_ROUTERS - ensure public schema and any existing tenant schemas are not listed in PG_EXTRA_SEARCH_PATHS - raise error when TENANT_APPS is empty - issue warning when 'tenant_schemas' is not the last item in INSTALLED_APPS - issue warning when items in TENANT_APPS are not in INSTALLED_APPS - add checks for SHARED_APPS - add test cases for the best_practice system check - update install documentation which previously suggested concatenation - fix import path of get_public_schema_name - fix failing test case for TenantContextFilter - update tox.ini - add .travis.yml - PEP8 fixes --- .editorconfig | 20 +++ docs/install.rst | 13 +- dts_test_project/.gitignore | 1 + dts_test_project/dts_test_project/settings.py | 11 +- dts_test_project/dts_test_project/urls.py | 22 +--- .../tenant_tutorial/settings.py | 3 +- .../tenant_tutorial/tenant_tutorial/wsgi.py | 2 +- setup.cfg | 3 + tenant_schemas/__init__.py | 41 +----- tenant_schemas/apps.py | 78 ++++++++++++ .../management/commands/migrate_schemas.py | 7 +- tenant_schemas/template_loaders.py | 2 +- tenant_schemas/tests/models.py | 1 - tenant_schemas/tests/test_apps.py | 119 ++++++++++++++++++ tenant_schemas/tests/test_log.py | 16 ++- tenant_schemas/tests/test_routes.py | 2 +- tenant_schemas/tests/test_tenants.py | 2 +- tenant_schemas/tests/testcases.py | 3 +- tenant_schemas/urlresolvers.py | 8 +- tenant_schemas/utils.py | 4 +- tox.ini | 18 +-- version.py | 3 +- 22 files changed, 286 insertions(+), 93 deletions(-) create mode 100644 .editorconfig create mode 100644 dts_test_project/.gitignore create mode 100644 setup.cfg create mode 100644 tenant_schemas/apps.py create mode 100644 tenant_schemas/tests/test_apps.py diff --git a/.editorconfig b/.editorconfig new file mode 100644 index 0000000..3b2bec5 --- /dev/null +++ b/.editorconfig @@ -0,0 +1,20 @@ +# EditorConfig is awesome: http://EditorConfig.org + +root = true + +# Unix-style newlines with a newline ending every file +[*] +end_of_line = lf +trim_trailing_whitespace = true +insert_final_newline = true + +[*.{js,py}] +charset = utf-8 + +[*.py] +indent_style = space +indent_size = 4 + +[*.{css,js,less}] +indent_style = space +indent_size = 2 diff --git a/docs/install.rst b/docs/install.rst index 9ffd82f..80c2e5a 100644 --- a/docs/install.rst +++ b/docs/install.rst @@ -103,7 +103,18 @@ To make use of shared and tenant-specific applications, there are two settings c 'myapp.houses', ) - INSTALLED_APPS = list(SHARED_APPS) + [app for app in TENANT_APPS if app not in SHARED_APPS] + INSTALLED_APPS = ( + 'customers', + 'django.contrib.contenttypes', + 'django.contrib.auth', + 'django.contrib.sessions', + 'django.contrib.sites', + 'django.contrib.messages', + 'django.contrib.admin', + 'myapp.hotels', + 'myapp.houses', + 'tenant_schemas', + ) You also have to set where your tenant model is. diff --git a/dts_test_project/.gitignore b/dts_test_project/.gitignore new file mode 100644 index 0000000..6350e98 --- /dev/null +++ b/dts_test_project/.gitignore @@ -0,0 +1 @@ +.coverage diff --git a/dts_test_project/dts_test_project/settings.py b/dts_test_project/dts_test_project/settings.py index ae8de72..66f4245 100644 --- a/dts_test_project/dts_test_project/settings.py +++ b/dts_test_project/dts_test_project/settings.py @@ -48,7 +48,16 @@ TENANT_MODEL = "customers.Client" # app.Model TEST_RUNNER = 'django.test.runner.DiscoverRunner' -INSTALLED_APPS = list(set(TENANT_APPS + SHARED_APPS)) +INSTALLED_APPS = ( + 'dts_test_app', + 'customers', + 'django.contrib.auth', + 'django.contrib.contenttypes', + 'django.contrib.sessions', + 'django.contrib.messages', + 'django.contrib.staticfiles', + 'tenant_schemas', +) MIDDLEWARE_CLASSES = ( 'django.contrib.sessions.middleware.SessionMiddleware', diff --git a/dts_test_project/dts_test_project/urls.py b/dts_test_project/dts_test_project/urls.py index a000786..637600f 100644 --- a/dts_test_project/dts_test_project/urls.py +++ b/dts_test_project/dts_test_project/urls.py @@ -1,21 +1 @@ -"""wtf URL Configuration - -The `urlpatterns` list routes URLs to views. For more information please see: - https://docs.djangoproject.com/en/1.9/topics/http/urls/ -Examples: -Function views - 1. Add an import: from my_app import views - 2. Add a URL to urlpatterns: url(r'^$', views.home, name='home') -Class-based views - 1. Add an import: from other_app.views import Home - 2. Add a URL to urlpatterns: url(r'^$', Home.as_view(), name='home') -Including another URLconf - 1. Add an import: from blog import urls as blog_urls - 2. Import the include() function: from django.conf.urls import url, include - 3. Add a URL to urlpatterns: url(r'^blog/', include(blog_urls)) -""" -from django.conf.urls import url -from django.contrib import admin - -urlpatterns = [ -] +urlpatterns = [] diff --git a/examples/tenant_tutorial/tenant_tutorial/settings.py b/examples/tenant_tutorial/tenant_tutorial/settings.py index c0d8b5a..0a3d190 100644 --- a/examples/tenant_tutorial/tenant_tutorial/settings.py +++ b/examples/tenant_tutorial/tenant_tutorial/settings.py @@ -1,3 +1,5 @@ +import os + # Django settings for tenant_tutorial project. DEBUG = True @@ -121,7 +123,6 @@ PUBLIC_SCHEMA_URLCONF = 'tenant_tutorial.urls_public' # Python dotted path to the WSGI application used by Django's runserver. WSGI_APPLICATION = 'tenant_tutorial.wsgi.application' -import os TEMPLATE_DIRS = (os.path.join(os.path.dirname(__file__), '..', 'templates').replace('\\', '/'),) SHARED_APPS = ( diff --git a/examples/tenant_tutorial/tenant_tutorial/wsgi.py b/examples/tenant_tutorial/tenant_tutorial/wsgi.py index c326bd8..32a9fb2 100644 --- a/examples/tenant_tutorial/tenant_tutorial/wsgi.py +++ b/examples/tenant_tutorial/tenant_tutorial/wsgi.py @@ -14,6 +14,7 @@ framework. """ import os +from django.core.wsgi import get_wsgi_application # We defer to a DJANGO_SETTINGS_MODULE already in the environment. This breaks # if running multiple sites in the same mod_wsgi process. To fix this, use @@ -24,7 +25,6 @@ os.environ.setdefault("DJANGO_SETTINGS_MODULE", "tenant_tutorial.settings") # This application object is used by any WSGI server configured to use this # file. This includes Django's development server, if the WSGI_APPLICATION # setting points here. -from django.core.wsgi import get_wsgi_application application = get_wsgi_application() # Apply WSGI middleware here. diff --git a/setup.cfg b/setup.cfg new file mode 100644 index 0000000..315d73c --- /dev/null +++ b/setup.cfg @@ -0,0 +1,3 @@ +[flake8] +exclude = .tox,docs,build,migrations,__init__.py +ignore = C901,E501,E731 diff --git a/tenant_schemas/__init__.py b/tenant_schemas/__init__.py index c8b5093..7aa0d8a 100644 --- a/tenant_schemas/__init__.py +++ b/tenant_schemas/__init__.py @@ -1,40 +1 @@ -from django.conf import settings -from django.core.exceptions import ImproperlyConfigured - -from tenant_schemas.utils import get_public_schema_name, get_tenant_model - -recommended_config = """ -Warning: You should put 'tenant_schemas' at the end of INSTALLED_APPS: -INSTALLED_APPS = TENANT_APPS + SHARED_APPS + ('tenant_schemas',) -This is necessary to overwrite built-in django management commands with -their schema-aware implementations. -""" -# Test for configuration recommendations. These are best practices, -# they avoid hard to find bugs and unexpected behaviour. -if not hasattr(settings, 'TENANT_APPS'): - raise ImproperlyConfigured('TENANT_APPS setting not set') - -if not settings.TENANT_APPS: - raise ImproperlyConfigured("TENANT_APPS is empty. " - "Maybe you don't need this app?") - -if not hasattr(settings, 'TENANT_MODEL'): - raise ImproperlyConfigured('TENANT_MODEL setting not set') - -if 'tenant_schemas.routers.TenantSyncRouter' not in settings.DATABASE_ROUTERS: - raise ImproperlyConfigured("DATABASE_ROUTERS setting must contain " - "'tenant_schemas.routers.TenantSyncRouter'.") - -if hasattr(settings, 'PG_EXTRA_SEARCH_PATHS'): - if get_public_schema_name() in settings.PG_EXTRA_SEARCH_PATHS: - raise ImproperlyConfigured( - "%s can not be included on PG_EXTRA_SEARCH_PATHS." - % get_public_schema_name()) - - # make sure no tenant schema is in settings.PG_EXTRA_SEARCH_PATHS - invalid_schemas = set(settings.PG_EXTRA_SEARCH_PATHS).intersection( - get_tenant_model().objects.all().values_list('schema_name', flat=True)) - if invalid_schemas: - raise ImproperlyConfigured( - "Do not include tenant schemas (%s) on PG_EXTRA_SEARCH_PATHS." - % list(invalid_schemas)) +default_app_config = 'tenant_schemas.apps.TenantSchemaConfig' diff --git a/tenant_schemas/apps.py b/tenant_schemas/apps.py new file mode 100644 index 0000000..68d35f2 --- /dev/null +++ b/tenant_schemas/apps.py @@ -0,0 +1,78 @@ +from django.apps import AppConfig +from django.conf import settings +from django.core.checks import Critical, Error, Warning, register + +from tenant_schemas.utils import get_public_schema_name, get_tenant_model + + +class TenantSchemaConfig(AppConfig): + name = 'tenant_schemas' + + +@register('config') +def best_practice(app_configs, **kwargs): + """ + Test for configuration recommendations. These are best practices, they + avoid hard to find bugs and unexpected behaviour. + """ + if not hasattr(settings, 'TENANT_APPS'): + return [Critical('TENANT_APPS setting not set')] + + if not hasattr(settings, 'TENANT_MODEL'): + return [Critical('TENANT_MODEL setting not set')] + + if not hasattr(settings, 'SHARED_APPS'): + return [Critical('SHARED_APPS setting not set')] + + if 'tenant_schemas.routers.TenantSyncRouter' not in settings.DATABASE_ROUTERS: + return [ + Critical("DATABASE_ROUTERS setting must contain " + "'tenant_schemas.routers.TenantSyncRouter'.") + ] + + errors = [] + + if settings.INSTALLED_APPS[-1] != 'tenant_schemas': + errors.append( + Warning("You should put 'tenant_schemas' at the end of INSTALLED_APPS.", + obj="django.conf.settings", + hint="This is necessary to overwrite built-in django " + "management commands with their schema-aware " + "implementations.")) + + if not settings.TENANT_APPS: + errors.append( + Error("TENANT_APPS is empty.", + hint="Maybe you don't need this app?")) + + if hasattr(settings, 'PG_EXTRA_SEARCH_PATHS'): + if get_public_schema_name() in settings.PG_EXTRA_SEARCH_PATHS: + errors.append(Critical( + "%s can not be included on PG_EXTRA_SEARCH_PATHS." + % get_public_schema_name())) + + # make sure no tenant schema is in settings.PG_EXTRA_SEARCH_PATHS + invalid_schemas = set(settings.PG_EXTRA_SEARCH_PATHS).intersection( + get_tenant_model().objects.all().values_list('schema_name', flat=True)) + if invalid_schemas: + errors.append(Critical( + "Do not include tenant schemas (%s) on PG_EXTRA_SEARCH_PATHS." + % ", ".join(sorted(invalid_schemas)))) + + if not settings.SHARED_APPS: + errors.append( + Warning("SHARED_APPS is empty.")) + + if not set(settings.TENANT_APPS).issubset(settings.INSTALLED_APPS): + delta = set(settings.TENANT_APPS).difference(settings.INSTALLED_APPS) + errors.append( + Error("You have TENANT_APPS that are not in INSTALLED_APPS", + hint=[a for a in settings.TENANT_APPS if a in delta])) + + if not set(settings.SHARED_APPS).issubset(settings.INSTALLED_APPS): + delta = set(settings.SHARED_APPS).difference(settings.INSTALLED_APPS) + errors.append( + Error("You have SHARED_APPS that are not in INSTALLED_APPS", + hint=[a for a in settings.SHARED_APPS if a in delta])) + + return errors diff --git a/tenant_schemas/management/commands/migrate_schemas.py b/tenant_schemas/management/commands/migrate_schemas.py index 7886ff3..7878629 100644 --- a/tenant_schemas/management/commands/migrate_schemas.py +++ b/tenant_schemas/management/commands/migrate_schemas.py @@ -4,6 +4,9 @@ from django.conf import settings from django.core.management.commands.migrate import Command as MigrateCommand from django.db import connection +from tenant_schemas.management.commands import SyncCommon +from tenant_schemas.utils import get_tenant_model, get_public_schema_name, schema_exists + if django.VERSION >= (1, 9, 0): from django.db.migrations.exceptions import MigrationSchemaMissing else: @@ -11,10 +14,6 @@ else: pass -from tenant_schemas.management.commands import SyncCommon -from tenant_schemas.utils import get_tenant_model, get_public_schema_name, schema_exists - - class Command(SyncCommon): help = "Updates database schema. Manages both apps with migrations and those without." diff --git a/tenant_schemas/template_loaders.py b/tenant_schemas/template_loaders.py index 583793a..c8c98ee 100644 --- a/tenant_schemas/template_loaders.py +++ b/tenant_schemas/template_loaders.py @@ -13,7 +13,7 @@ from django.db import connection from django.template.loaders.base import Loader as BaseLoader from tenant_schemas.postgresql_backend.base import FakeTenant -import django + class CachedLoader(BaseLoader): is_usable = True diff --git a/tenant_schemas/tests/models.py b/tenant_schemas/tests/models.py index da1ba7b..054fe8a 100644 --- a/tenant_schemas/tests/models.py +++ b/tenant_schemas/tests/models.py @@ -1,4 +1,3 @@ -from django.db import models from tenant_schemas.models import TenantMixin diff --git a/tenant_schemas/tests/test_apps.py b/tenant_schemas/tests/test_apps.py new file mode 100644 index 0000000..e841411 --- /dev/null +++ b/tenant_schemas/tests/test_apps.py @@ -0,0 +1,119 @@ +from django.apps.registry import Apps +from django.core.checks import Critical, Error, Warning +from django.test import TestCase +from django.test.utils import override_settings + +from tenant_schemas.apps import best_practice +from tenant_schemas.utils import get_tenant_model + + +class AppConfigTests(TestCase): + + maxDiff = None + + def assertBestPractice(self, expected): + from django.conf import settings + registry = Apps(settings.INSTALLED_APPS) + actual = best_practice(registry.get_app_configs()) + self.assertEqual(expected, actual) + + @override_settings() + def test_unset_tenant_apps(self): + from django.conf import settings + del settings.TENANT_APPS + self.assertBestPractice([ + Critical('TENANT_APPS setting not set'), + ]) + + @override_settings() + def test_unset_tenant_model(self): + from django.conf import settings + del settings.TENANT_MODEL + self.assertBestPractice([ + Critical('TENANT_MODEL setting not set'), + ]) + + @override_settings() + def test_unset_shared_apps(self): + from django.conf import settings + del settings.SHARED_APPS + self.assertBestPractice([ + Critical('SHARED_APPS setting not set'), + ]) + + @override_settings(DATABASE_ROUTERS=()) + def test_database_routers(self): + self.assertBestPractice([ + Critical("DATABASE_ROUTERS setting must contain " + "'tenant_schemas.routers.TenantSyncRouter'."), + ]) + + @override_settings(INSTALLED_APPS=[ + 'tenant_schemas', + 'dts_test_app', + 'customers', + 'django.contrib.auth', + 'django.contrib.contenttypes', + 'django.contrib.sessions', + 'django.contrib.messages', + 'django.contrib.staticfiles', + ]) + def test_tenant_schemas_last_installed_apps(self): + self.assertBestPractice([ + Warning("You should put 'tenant_schemas' at the end of INSTALLED_APPS.", + obj="django.conf.settings", + hint="This is necessary to overwrite built-in django " + "management commands with their schema-aware " + "implementations."), + ]) + + @override_settings(TENANT_APPS=()) + def test_tenant_apps_empty(self): + self.assertBestPractice([ + Error("TENANT_APPS is empty.", + hint="Maybe you don't need this app?"), + ]) + + @override_settings(PG_EXTRA_SEARCH_PATHS=['public', 'demo1', 'demo2']) + def test_public_schema_on_extra_search_paths(self): + TenantModel = get_tenant_model() + TenantModel.objects.create( + schema_name='demo1', domain_url='demo1.example.com') + TenantModel.objects.create( + schema_name='demo2', domain_url='demo2.example.com') + self.assertBestPractice([ + Critical("public can not be included on PG_EXTRA_SEARCH_PATHS."), + Critical("Do not include tenant schemas (demo1, demo2) on PG_EXTRA_SEARCH_PATHS."), + ]) + + @override_settings(SHARED_APPS=()) + def test_shared_apps_empty(self): + self.assertBestPractice([ + Warning("SHARED_APPS is empty."), + ]) + + @override_settings(TENANT_APPS=( + 'dts_test_app', + 'django.contrib.flatpages', + )) + def test_tenant_app_missing_from_install_apps(self): + self.assertBestPractice([ + Error("You have TENANT_APPS that are not in INSTALLED_APPS", + hint=['django.contrib.flatpages']), + ]) + + @override_settings(SHARED_APPS=( + 'tenant_schemas', + 'customers', + 'django.contrib.auth', + 'django.contrib.contenttypes', + 'django.contrib.flatpages', + 'django.contrib.messages', + 'django.contrib.sessions', + 'django.contrib.staticfiles', + )) + def test_shared_app_missing_from_install_apps(self): + self.assertBestPractice([ + Error("You have SHARED_APPS that are not in INSTALLED_APPS", + hint=['django.contrib.flatpages']), + ]) diff --git a/tenant_schemas/tests/test_log.py b/tenant_schemas/tests/test_log.py index a145d15..4dd27dd 100644 --- a/tenant_schemas/tests/test_log.py +++ b/tenant_schemas/tests/test_log.py @@ -1,16 +1,28 @@ import logging +from mock import patch from django.test import TestCase from tenant_schemas import log +@patch('tenant_schemas.log.connection.tenant', autospec=True, + schema_name='context') class LoggingFilterTests(TestCase): - def test_tenant_context_filter(self): + def test_tenant_context_filter(self, mock_connection): + mock_connection.domain_url = 'context.example.com' filter_ = log.TenantContextFilter() record = logging.makeLogRecord({}) res = filter_.filter(record) self.assertEqual(res, True) - self.assertEqual(record.schema_name, 'public') + self.assertEqual(record.schema_name, 'context') + self.assertEqual(record.domain_url, 'context.example.com') + + def test_tenant_context_filter_blank_domain_url(self, mock_connection): + filter_ = log.TenantContextFilter() + record = logging.makeLogRecord({}) + res = filter_.filter(record) + self.assertEqual(res, True) + self.assertEqual(record.schema_name, 'context') self.assertEqual(record.domain_url, '') diff --git a/tenant_schemas/tests/test_routes.py b/tenant_schemas/tests/test_routes.py index 0af0afd..6a5bcb1 100644 --- a/tenant_schemas/tests/test_routes.py +++ b/tenant_schemas/tests/test_routes.py @@ -1,10 +1,10 @@ from django.conf import settings from django.test.client import RequestFactory -from tenant_schemas import get_public_schema_name from tenant_schemas.middleware import TenantMiddleware from tenant_schemas.tests.models import Tenant from tenant_schemas.tests.testcases import BaseTestCase +from tenant_schemas.utils import get_public_schema_name class RoutesTestCase(BaseTestCase): diff --git a/tenant_schemas/tests/test_tenants.py b/tenant_schemas/tests/test_tenants.py index c6b222b..5dd27e7 100644 --- a/tenant_schemas/tests/test_tenants.py +++ b/tenant_schemas/tests/test_tenants.py @@ -87,7 +87,7 @@ class TenantDataAndSettingsTest(BaseTestCase): self.assertFalse(schema_exists('auto_drop_tenant')) Tenant.auto_drop_schema = True tenant = Tenant(domain_url='something.test.com', - schema_name='auto_drop_tenant') + schema_name='auto_drop_tenant') tenant.save(verbosity=BaseTestCase.get_verbosity()) self.assertTrue(schema_exists(tenant.schema_name)) cursor = connection.cursor() diff --git a/tenant_schemas/tests/testcases.py b/tenant_schemas/tests/testcases.py index 25ff443..75a923a 100644 --- a/tenant_schemas/tests/testcases.py +++ b/tenant_schemas/tests/testcases.py @@ -1,4 +1,3 @@ -import django import inspect from django.conf import settings from django.core.management import call_command @@ -27,7 +26,7 @@ class BaseTestCase(TestCase): connection.set_schema_to_public() cursor = connection.cursor() cursor.execute('DROP SCHEMA %s CASCADE; CREATE SCHEMA %s;' - % (get_public_schema_name(), get_public_schema_name(), )) + % (get_public_schema_name(), get_public_schema_name())) super(BaseTestCase, cls).setUpClass() def setUp(self): diff --git a/tenant_schemas/urlresolvers.py b/tenant_schemas/urlresolvers.py index 00e36f6..0351a8a 100644 --- a/tenant_schemas/urlresolvers.py +++ b/tenant_schemas/urlresolvers.py @@ -6,10 +6,10 @@ from tenant_schemas.utils import clean_tenant_url def reverse(viewname, urlconf=None, args=None, kwargs=None, prefix=None, current_app=None): url = reverse_default( - viewname=viewname, - urlconf=urlconf, - args=args, - kwargs=kwargs, + viewname=viewname, + urlconf=urlconf, + args=args, + kwargs=kwargs, current_app=current_app ) return clean_tenant_url(url) diff --git a/tenant_schemas/utils.py b/tenant_schemas/utils.py index 9054125..338ebc6 100644 --- a/tenant_schemas/utils.py +++ b/tenant_schemas/utils.py @@ -55,8 +55,8 @@ def clean_tenant_url(url_string): Removes the TENANT_TOKEN from a particular string """ if hasattr(settings, 'PUBLIC_SCHEMA_URLCONF'): - if (settings.PUBLIC_SCHEMA_URLCONF - and url_string.startswith(settings.PUBLIC_SCHEMA_URLCONF)): + if (settings.PUBLIC_SCHEMA_URLCONF and + url_string.startswith(settings.PUBLIC_SCHEMA_URLCONF)): url_string = url_string[len(settings.PUBLIC_SCHEMA_URLCONF):] return url_string diff --git a/tox.ini b/tox.ini index bb7edff..7f2d7c6 100644 --- a/tox.ini +++ b/tox.ini @@ -4,16 +4,18 @@ envlist = py{27,35}-dj{18,19,110} [testenv] usedevelop = True -deps = - coverage - dj18: Django~=1.8.0 - dj19: Django~=1.9.0 - dj110: Django~=1.10.0 +deps = + coverage + mock + tblib + dj18: Django~=1.8.0 + dj19: Django~=1.9.0 + dj110: Django~=1.10.0 changedir = dts_test_project passenv = PG_NAME PG_USER PG_PASSWORD PG_HOST -commands = - coverage run manage.py test --noinput {posargs:tenant_schemas.tests -v 2} - coverage report -m --include=../tenant_schemas/* +commands = + coverage run manage.py test --noinput {posargs:tenant_schemas} + coverage report -m --include=../tenant_schemas/* diff --git a/version.py b/version.py index c1e513f..9a235c5 100644 --- a/version.py +++ b/version.py @@ -30,11 +30,10 @@ # contains the following line: # # include VERSION +from subprocess import Popen, PIPE __all__ = ("get_git_version") -from subprocess import Popen, PIPE - def call_git_describe(): try: