Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update template loaders and static file storage for Django > 2.0 #198

Merged

Conversation

jcass77
Copy link
Contributor

@jcass77 jcass77 commented Nov 2, 2018

It looks like TenantStaticFileStorage has not been updated since Django 1.8, and is no longer compatible with Django > 2.0.

This PR just rebases the implementation on the standard django.template.loaders package to bring django-tenants back up to parity.

Fixes #197.

@jcass77 jcass77 force-pushed the fix/197_relative_static_root branch from af6e355 to 5d7010b Compare November 6, 2018 02:42
@jcass77
Copy link
Contributor Author

jcass77 commented Nov 6, 2018

Ok, I think this PR is ready for review. It resolves a large number of long-standing issues related to static file handling.

Apologies for the large commit - most of the diffs are generated by documentation changes and new test cases.

@phalt
Copy link
Contributor

phalt commented Nov 6, 2018

+1 this will resolve the tests, which do not currently pass when running setup.py locally

@tomturner
Copy link
Member

@jcass77 Thanks for this.

@rdraju
Copy link

rdraju commented Nov 8, 2018

django_tenants.template.loaders.filesystem.Loader get initialized only once. It happens with the first request and directories are set based on that tenant. Same template directories get utilized for all further requests from any tenant.

@rdraju
Copy link

rdraju commented Nov 8, 2018

Not relying on self.dirs and overriding get_dirs is the way to go? Any performance considerations? Can they be mitigated using CachedLoader?

@jcass77
Copy link
Contributor Author

jcass77 commented Nov 8, 2018

Hmmm...I thought I tested that scenario but you are right @rdraju, the Loader is only initialised once so I can see how the same template might be returned for all subsequent tenants.

Have a look at TenantFileSystemFinder for ideas on doing lazy loading so that the dirs are still only initialised once for each tenant.

@selvara80
Copy link

Unfortunately, This change breaks other functionalities.
1)STATICFILES_FINDERS is only looking for static files from the tenant folder. No file is fetched from the public folder.
2)When testing Media, No module named 'django_tenants.files.storages' .Believe the file name incorrectly got changed to 'storage'

@jcass77
Copy link
Contributor Author

jcass77 commented Dec 18, 2018

  1. As far as I can recall, the standard Django finders will take care of looking up static files in the public folder. You still need to reference it in your settings.py though, which should include something like the following:
STATICFILES_FINDERS = [
    # https://django-tenants.readthedocs.io/en/latest/static.html#static-files-finders-aware-tenant
    "django_tenants.staticfiles.finders.TenantFileSystemFinder",  # Must be first
    "django.contrib.staticfiles.finders.FileSystemFinder",  # <-- include Django standard finder.
    "django.contrib.staticfiles.finders.AppDirectoriesFinder",
]

2.django_tenants.files.storages was renamed to django_tenants.files.storage to align more closely with the Django standard naming convention. I updated all of the internal references but might have underestimated the impact that this could have on existing users with pre-existing configurations. Have you tried updating your settings to use the new module name?

@gnalbandian
Copy link

gnalbandian commented Dec 27, 2018

  1. As far as I can recall, the standard Django finders will take care of looking up static files in the public folder. You still need to reference it in your settings.py though, which should include something like the following:
STATICFILES_FINDERS = [
    "django_tenants.staticfiles.finders.TenantFileSystemFinder",
    "django_tenants.staticfiles.finders.TenantFileSystemFinder",  # Must be first
    "django.contrib.staticfiles.finders.FileSystemFinder",  # <-- include Django standard finder.
    "django.contrib.staticfiles.finders.AppDirectoriesFinder",
]

2.django_tenants.files.storages was renamed to django_tenants.files.storage to align more closely with the Django standard naming convention. I updated all of the internal references but might have underestimated the impact that this could have on existing users with pre-existing configurations. Have you tried updating your settings to use the new module name?

The issue seems to be that the {% static %} is always replaced by the STATIC_ROOT path and the schema name of the tenant appended ( STATIC_ROOT/tenant_schema ) even though there is no specific folder for tenant static files

@jcass77
Copy link
Contributor Author

jcass77 commented Dec 27, 2018

@gnalbandian can you log a ticket so that we can track an resolve this separately.

If there are no tenant-specific static files for the project, then removing it from the STATICFILES_FINDERS config section and relying on the Django standard django.contrib.staticfiles.finders.FileSystemFinder should be all that is required for that specific use case - or am I overlooking something else?

@gnalbandian
Copy link

@gnalbandian can you log a ticket so that we can track an resolve this separately.

If there are no tenant-specific static files for the project, then removing it from the STATICFILES_FINDERS config section and relying on the Django standard django.contrib.staticfiles.finders.FileSystemFinder should be all that is required for that specific use case - or am I overlooking something else?

If that is the intended behavior, then it's working as its intended.
I thought that the desired behavior was:
First, check for specific tenant static files.
Then, if none found, check for global static files

@jcass77
Copy link
Contributor Author

jcass77 commented Dec 28, 2018

Thanks for the feedback @gnalbandian. Yes I think that is the intended behaviour: each STATICFILES_FINDERS only looks for static files in the specific folders that it has been designed / configured for.

For django.contrib.staticfiles.finders.FileSystemFinder, that means tenant-specific files.

atodorov added a commit to MrSenko/django-tenants that referenced this pull request Mar 29, 2019
…tenants#249

because the both depend on the current schema_name they can't
be cached, nor can be initialized only once in the constructor.
An important reason is that the `default_storage` class in Django
is initialized only once!

Also includes a small performance improvement which prevents the
raising and catching of exceptions when
MULTITENANT_RELATIVE_MEDIA_ROOT is not specified at all!

Note: looks like this changed in django-tenants#198 but looking at the PR doesn't
reveal why that is. There is a discussion about static file finders
and template loaders but nothing for TenantFileSystemStorage().
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants