-
Notifications
You must be signed in to change notification settings - Fork 589
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
base: master
Are you sure you want to change the base?
Changes from 1 commit
6961cbc
ab02845
e939b46
1908ab5
1934f89
f7a1a60
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -375,8 +375,30 @@ def directory_listing(self, request, folder_id=None, viewtype=None): | |
.order_by("-modified") | ||
) | ||
file_qs = file_qs.annotate( | ||
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]) | ||
# For private files (Filer pattern) | ||
thumbnail_name_filer=Subquery( | ||
thumbnail_qs.filter(name__contains=f"__{size}_").values_list("name")[:1] | ||
), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion (bug_risk): Review the slicing of subqueries in annotations. Each of the Subquery filters uses slicing ([:1]), implicitly assuming that exactly one match is relevant. It could be worth explicitly ensuring that this behavior is intentional and that multiple potential matches won’t cause unexpected results. Suggested implementation: # For private files (Filer pattern)
# Explicit ordering ensures that only a predictable single value is taken
thumbnail_name_filer=Subquery(
thumbnail_qs.filter(name__contains=f"__{size}_")
.order_by("id")
.values_list("name", flat=True)[:1]
),
thumbnailx2_name_filer=Subquery(
thumbnail_qs.filter(name__contains=f"__{size_x2}_")
.order_by("id")
.values_list("name", flat=True)[:1]
),
# For public files (easy_thumbnails pattern)
# Explicit ordering here as well to enforce a single, predictable match
thumbnail_name_easy=Subquery(
thumbnail_qs.filter(name__contains=f".{size}_q85_crop")
.order_by("id")
.values_list("name", flat=True)[:1]
),
thumbnailx2_name_easy=Subquery(
thumbnail_qs.filter(name__contains=f".{size_x2}_q85_crop")
.order_by("id")
.values_list("name", flat=True)[:1]
), Review the ordering field ("id" in this example) to confirm it aligns with your domain's logic for which record should be returned. Also, verify that the use of flat=True does not conflict with downstream code expecting a different format. |
||
thumbnailx2_name_filer=Subquery( | ||
thumbnail_qs.filter(name__contains=f"__{size_x2}_").values_list("name")[ | ||
:1 | ||
] | ||
), | ||
# For public files (easy_thumbnails pattern) | ||
thumbnail_name_easy=Subquery( | ||
thumbnail_qs.filter(name__contains=f".{size}_q85_crop").values_list( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indeed, Can this all be simplified by customizing the part before |
||
"name" | ||
)[:1] | ||
), | ||
thumbnailx2_name_easy=Subquery( | ||
thumbnail_qs.filter(name__contains=f".{size_x2}_q85_crop").values_list( | ||
"name" | ||
)[:1] | ||
), | ||
thumbnail_name=Coalesce("thumbnail_name_filer", "thumbnail_name_easy"), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion: Clarify the use of Coalesce for merging file name variants. The logic for selecting between the filer and easy thumbnail names using Coalesce seems to assume that one of these will always have a value. It might be useful to verify that the underlying queries—and their ordering via __contains filters—align with expectations in all cases, especially if both are missing. Suggested implementation: # Consolidate thumbnail names by prioritizing the filer value.
# Assumes that either 'thumbnail_name_filer' or 'thumbnail_name_easy' is non-null.
# Verify that the order and filters in the underlying queries always yield a valid result.
thumbnail_name=Coalesce("thumbnail_name_filer", "thumbnail_name_easy"), # Consolidate x2 thumbnail names by preferring the filer variant.
# If both 'thumbnailx2_name_filer' and 'thumbnailx2_name_easy' are null,
# the result will be null. Ensure the underlying query ordering guarantees a valid entry.
thumbnailx2_name=Coalesce("thumbnailx2_name_filer", "thumbnailx2_name_easy"), |
||
thumbnailx2_name=Coalesce( | ||
"thumbnailx2_name_filer", "thumbnailx2_name_easy" | ||
), | ||
).select_related("owner") | ||
|
||
try: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -35,13 +35,14 @@ def tearDown(self): | |
for f in File.objects.all(): | ||
f.delete() | ||
|
||
def create_filer_image(self, owner=None): | ||
def create_filer_image(self, owner=None, is_public=True): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need to change the signature here? Is this for testing? |
||
if owner is None: | ||
owner = self.superuser | ||
file_obj = DjangoFile(open(self.filename, 'rb'), name=self.image_name) | ||
image = Image.objects.create(owner=owner, | ||
original_filename=self.image_name, | ||
file=file_obj) | ||
file=file_obj, | ||
is_public=is_public) | ||
return image | ||
|
||
def test_create_folder_structure(self): | ||
|
@@ -80,7 +81,7 @@ def test_create_clipboard_item(self): | |
self.assertEqual(Clipboard.objects.count(), 1) | ||
|
||
def test_create_icons(self): | ||
image = self.create_filer_image() | ||
image = self.create_filer_image(is_public=False) | ||
image.save() | ||
icons = image.icons | ||
file_basename = os.path.basename(image.file.path) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,62 @@ | ||
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.assertNotIn("__", name) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion (testing): Add tests for different thumbnail options. The current tests only cover the default thumbnail size. It would be beneficial to add tests for different thumbnail options like Suggested implementation: @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)}) # New tests for additional thumbnail options for public files
def test_thumbnailer_with_crop_option_public(self):
image = self.create_filer_image(is_public=True)
thumbnailer = image.easy_thumbnails_thumbnailer
opts = {"size": (100, 100), "crop": True}
name = thumbnailer.get_thumbnail_name(opts)
self.assertNotIn("__", name)
def test_thumbnailer_with_upscale_option_public(self):
image = self.create_filer_image(is_public=True)
thumbnailer = image.easy_thumbnails_thumbnailer
opts = {"size": (100, 100), "upscale": True}
name = thumbnailer.get_thumbnail_name(opts)
self.assertNotIn("__", name)
def test_thumbnailer_with_quality_option_public(self):
image = self.create_filer_image(is_public=True)
thumbnailer = image.easy_thumbnails_thumbnailer
opts = {"size": (100, 100), "quality": 80}
name = thumbnailer.get_thumbnail_name(opts)
self.assertNotIn("__", name)
# New tests for additional thumbnail options for private files
def test_thumbnailer_with_crop_option_private(self):
image = self.create_filer_image(is_public=False)
thumbnailer = image.easy_thumbnails_thumbnailer
opts = {"size": (100, 100), "crop": True}
name = thumbnailer.get_thumbnail_name(opts)
self.assertIn("__", name)
def test_thumbnailer_with_upscale_option_private(self):
image = self.create_filer_image(is_public=False)
thumbnailer = image.easy_thumbnails_thumbnailer
opts = {"size": (100, 100), "upscale": True}
name = thumbnailer.get_thumbnail_name(opts)
self.assertIn("__", name)
def test_thumbnailer_with_quality_option_private(self):
image = self.create_filer_image(is_public=False)
thumbnailer = image.easy_thumbnails_thumbnailer
opts = {"size": (100, 100), "quality": 80}
name = thumbnailer.get_thumbnail_name(opts)
self.assertIn("__", name) These new tests are appended at the end of the file after the existing tests. They ensure that when thumbnail options such as crop, upscale, or quality are used, the naming convention applied by the thumbnailer stays consistent with the public/private file policy. You may need to adjust these tests according to your custom thumbnail namer or any additional options supported in your project. |
||
|
||
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.assertIn("__", name) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion (testing): Add tests for edge cases like file names with special characters or very long file names. Testing with edge cases can help uncover potential issues with the naming pattern. Consider adding tests with file names containing spaces, special characters, or very long names to ensure the generated thumbnail names are valid and don't cause any problems. Suggested implementation: @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_"))
def test_thumbnailer_special_characters(self):
image = self.create_filer_image(is_public=True)
# Set a file name with spaces and special characters
image.file.name = "special @#$ file name.png"
thumbnailer = image.easy_thumbnails_thumbnailer
name = thumbnailer.get_thumbnail_name({"size": (100, 100)})
# Check that problematic characters are removed or replaced
self.assertNotIn(" ", name)
self.assertNotIn("@", name)
self.assertNotIn("#", name)
self.assertNotIn("$", name)
def test_thumbnailer_long_file_name(self):
image = self.create_filer_image(is_public=True)
# Create a very long file name (e.g., 255 characters before the extension)
long_name = ("a" * 255) + ".png"
image.file.name = long_name
thumbnailer = image.easy_thumbnails_thumbnailer
name = thumbnailer.get_thumbnail_name({"size": (100, 100)})
# Assert the generated thumbnail name is within acceptable length limits
self.assertLessEqual(len(os.path.basename(name)), 255) Ensure that the image object returned by self.create_filer_image has an attribute "file" that allows overriding its "name" property. Adjust the test assertions if your thumbnail naming logic treats these edge cases differently. |
||
|
||
@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_")) |
There was a problem hiding this comment.
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:
Then update the annotation like so:
This keeps all functionality intact while reducing the nesting and duplicated logic.