Skip to content

fix: enhance thumbnail handling for public and private files #1519

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions filer/admin/folderadmin.py
Original file line number Diff line number Diff line change
Expand Up @@ -376,8 +376,8 @@ def directory_listing(self, request, folder_id=None, viewtype=None):
.order_by("-modified")
)
file_qs = file_qs.annotate(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (complexity): Consider refactoring the repeated subquery filtering into a helper function to reduce duplication and improve maintainability.

Consider refactoring the repeated subquery filtering into a helper function. This reduces duplication and makes it easier to update the filtering criteria in one place. For example:

def get_thumbnail_subquery(queryset, size, prefix, suffix):
    filter_expr = f"{prefix}{size}{suffix}"
    return Subquery(queryset.filter(name__contains=filter_expr).values_list("name")[:1])

Then update the annotation like so:

file_qs = file_qs.annotate(
    # For private files (Filer pattern)
    thumbnail_name_filer=get_thumbnail_subquery(thumbnail_qs, size, "__", "_"),
    thumbnailx2_name_filer=get_thumbnail_subquery(thumbnail_qs, size_x2, "__", "_"),
    # For public files (easy_thumbnails pattern)
    thumbnail_name_easy=get_thumbnail_subquery(thumbnail_qs, size, ".", "_q85_crop"),
    thumbnailx2_name_easy=get_thumbnail_subquery(thumbnail_qs, size_x2, ".", "_q85_crop"),
    thumbnail_name=Coalesce("thumbnail_name_filer", "thumbnail_name_easy"),
    thumbnailx2_name=Coalesce("thumbnailx2_name_filer", "thumbnailx2_name_easy"),
).select_related("owner")

This keeps all functionality intact while reducing the nesting and duplicated logic.

thumbnail_name=Subquery(thumbnail_qs.filter(name__contains=f"__{size}_").values_list("name")[:1]),
thumbnailx2_name=Subquery(thumbnail_qs.filter(name__contains=f"__{size_x2}_").values_list("name")[:1])
thumbnail_name=Subquery(thumbnail_qs.filter(name__contains=f".{size}_").values_list("name")[:1]),
thumbnailx2_name=Subquery(thumbnail_qs.filter(name__contains=f".{size_x2}_").values_list("name")[:1])
).select_related("owner")

try:
Expand Down
93 changes: 43 additions & 50 deletions filer/utils/filer_easy_thumbnails.py
Original file line number Diff line number Diff line change
@@ -1,14 +1,15 @@
import os
import re
from contextlib import contextmanager

from easy_thumbnails.files import Thumbnailer
from easy_thumbnails.namers import default


# match the source filename using `__` as the seperator. ``opts_and_ext`` is non
# greedy so it should match the last occurence of `__`.
# in ``ThumbnailerNameMixin.get_thumbnail_name`` we ensure that there is no `__`
# in the opts part.
RE_ORIGINAL_FILENAME = re.compile(r"^(?P<source_filename>.*)__(?P<opts_and_ext>.*?)$")
# easy-thumbnails default pattern
# e.g: source.jpg.100x100_q80_crop_upscale.jpg
RE_ORIGINAL_FILENAME = re.compile(
r"^(?P<source_filename>.*?)\.(?P<opts_and_ext>[^.]+\.[^.]+)$"
)


def thumbnail_to_original_filename(thumbnail_name):
Expand All @@ -18,59 +19,51 @@ def thumbnail_to_original_filename(thumbnail_name):
return None


@contextmanager
def use_default_namer(thumbnailer):
"""
Context manager to use the default easy-thumbnails namer for private files.
"""
original_namer = thumbnailer.thumbnail_namer
thumbnailer.thumbnail_namer = default
try:
yield
finally:
thumbnailer.thumbnail_namer = original_namer


class ThumbnailerNameMixin:
thumbnail_basedir = ''
thumbnail_subdir = ''
thumbnail_prefix = ''
thumbnail_basedir = ""
thumbnail_subdir = ""
thumbnail_prefix = ""

def get_thumbnail_name(self, thumbnail_options, transparent=False):
"""
A version of ``Thumbnailer.get_thumbnail_name`` that produces a
reproducible thumbnail name that can be converted back to the original
filename.
Get thumbnail name using easy-thumbnails pattern.
For public files: Uses configurable naming via THUMBNAIL_NAMER
For private files: Uses easy-thumbnails default naming pattern regardless of THUMBNAIL_NAMER
"""
path, source_filename = os.path.split(self.name)
source_extension = os.path.splitext(source_filename)[1][1:].lower()
preserve_extensions = self.thumbnail_preserve_extensions
if preserve_extensions is True or source_extension == 'svg' or \
isinstance(preserve_extensions, (list, tuple)) and source_extension in preserve_extensions:
extension = source_extension
elif transparent:
extension = self.thumbnail_transparency_extension
else:
extension = self.thumbnail_extension
extension = extension or 'jpg'

thumbnail_options = thumbnail_options.copy()
size = tuple(thumbnail_options.pop('size'))
initial_opts = ['{}x{}'.format(*size)]
quality = thumbnail_options.pop('quality', self.thumbnail_quality)
if extension == 'jpg':
initial_opts.append(f'q{quality}')
elif extension == 'svg':
thumbnail_options.pop('subsampling', None)
thumbnail_options.pop('upscale', None)

opts = list(thumbnail_options.items())
opts.sort() # Sort the options so the file name is consistent.
opts = ['{}'.format(v is not True and f'{k}-{v}' or k)
for k, v in opts if v]
all_opts = '_'.join(initial_opts + opts)
is_public = False
if hasattr(self, "thumbnail_storage"):
is_public = "PrivateFileSystemStorage" not in str(
self.thumbnail_storage.__class__
)

basedir = self.thumbnail_basedir
subdir = self.thumbnail_subdir

# make sure our magic delimiter is not used in all_opts
all_opts = all_opts.replace('__', '_')
filename = f'{source_filename}__{all_opts}.{extension}'
if is_public:
return super(ThumbnailerNameMixin, self).get_thumbnail_name(
thumbnail_options, transparent
)

return os.path.join(basedir, path, subdir, filename)
with use_default_namer(self):
return super(ThumbnailerNameMixin, self).get_thumbnail_name(
thumbnail_options, transparent
)


class ActionThumbnailerMixin:
thumbnail_basedir = ''
thumbnail_subdir = ''
thumbnail_prefix = ''
thumbnail_basedir = ""
thumbnail_subdir = ""
thumbnail_prefix = ""

def get_thumbnail_name(self, thumbnail_options, transparent=False):
"""
Expand All @@ -90,7 +83,7 @@ def thumbnail_exists(self, thumbnail_name):

class FilerThumbnailer(ThumbnailerNameMixin, Thumbnailer):
def __init__(self, *args, **kwargs):
self.thumbnail_basedir = kwargs.pop('thumbnail_basedir', '')
self.thumbnail_basedir = kwargs.pop("thumbnail_basedir", "")
super().__init__(*args, **kwargs)


Expand Down
2 changes: 1 addition & 1 deletion tests/test_models.py
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ def test_create_icons(self):
self.assertEqual(len(icons), len(filer_settings.FILER_ADMIN_ICON_SIZES))
for size in filer_settings.FILER_ADMIN_ICON_SIZES:
self.assertEqual(os.path.basename(icons[size]),
file_basename + '__{}x{}_q85_crop_subsampling-2_upscale.jpg'.format(size, size))
file_basename + '.{}x{}_q85_crop_upscale.jpg'.format(size, size))

def test_access_icons_property(self):
"""Test IconsMixin that calls static on a non-existent file"""
Expand Down
71 changes: 71 additions & 0 deletions tests/test_thumbnails.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
import os

from django.conf import settings
from django.core.files import File as DjangoFile
from django.test import TestCase, override_settings

from filer.models.filemodels import File
from filer.settings import FILER_IMAGE_MODEL
from filer.utils.loader import load_model
from tests.helpers import create_image, create_superuser

Image = load_model(FILER_IMAGE_MODEL)


def custom_namer(thumbnailer, **kwargs):
path, filename = os.path.split(thumbnailer.name)
return os.path.join(path, f"custom_prefix_{filename}")


class ThumbnailNameTests(TestCase):
def setUp(self):
self.superuser = create_superuser()
self.img = create_image()
self.image_name = "test_file.jpg"
self.filename = os.path.join(settings.FILE_UPLOAD_TEMP_DIR, self.image_name)
self.img.save(self.filename, "JPEG")

def tearDown(self):
os.remove(self.filename)
for f in File.objects.all():
f.delete()

def create_filer_image(self, is_public=True):
with open(self.filename, "rb") as f:
file_obj = DjangoFile(f)
image = Image.objects.create(
owner=self.superuser,
original_filename=self.image_name,
file=file_obj,
is_public=is_public,
)
return image

def test_thumbnailer_class_for_public_files(self):
image = self.create_filer_image(is_public=True)
thumbnailer = image.easy_thumbnails_thumbnailer
name = thumbnailer.get_thumbnail_name({"size": (100, 100)})
self.assertRegex(name, r"^.*\..*\.[^.]+$")

def test_thumbnailer_class_for_private_files(self):
image = self.create_filer_image(is_public=False)
thumbnailer = image.easy_thumbnails_thumbnailer
name = thumbnailer.get_thumbnail_name({"size": (100, 100)})
self.assertRegex(name, r"^.*\..*\.[^.]+$")

@override_settings(THUMBNAIL_NAMER="tests.test_thumbnails.custom_namer")
def test_thumbnail_custom_namer(self):
image = self.create_filer_image(is_public=True)
thumbnailer = image.easy_thumbnails_thumbnailer
name = thumbnailer.get_thumbnail_name({"size": (100, 100)})
filename = os.path.basename(name)
self.assertTrue(filename.startswith("custom_prefix_"))

@override_settings(THUMBNAIL_NAMER="tests.test_thumbnails.custom_namer")
def test_private_thumbnail_ignores_custom_namer(self):
image = self.create_filer_image(is_public=False)
thumbnailer = image.easy_thumbnails_thumbnailer
name = thumbnailer.get_thumbnail_name({"size": (100, 100)})
filename = os.path.basename(name)
self.assertFalse(filename.startswith("custom_prefix_"))
self.assertRegex(name, r"^.*\..*\.[^.]+$")
Loading