fix: enhance thumbnail handling for public and private files #1519
Conversation
Reviewer's Guide by SourceryThis pull request enhances thumbnail handling by using the default easy_thumbnail naming for public files and a custom naming pattern for private files. It modifies the directory listing to handle both naming patterns and adds tests to verify the correct thumbnail naming strategy. Sequence diagram for thumbnail generation (public file)sequenceDiagram
participant User
participant FilerAdmin
participant ThumbnailerNameMixin
participant easy_thumbnails
User->>FilerAdmin: Requests directory listing
FilerAdmin->>ThumbnailerNameMixin: get_thumbnail_name(options, transparent)
activate ThumbnailerNameMixin
ThumbnailerNameMixin->>ThumbnailerNameMixin: Check if file is public
ThumbnailerNameMixin->>easy_thumbnails: super().get_thumbnail_name(options, transparent)
activate easy_thumbnails
easy_thumbnails-->>ThumbnailerNameMixin: Thumbnail name (easy_thumbnails format)
deactivate easy_thumbnails
ThumbnailerNameMixin-->>FilerAdmin: Thumbnail name (easy_thumbnails format)
deactivate ThumbnailerNameMixin
FilerAdmin-->>User: Displays directory listing with thumbnail
Sequence diagram for thumbnail generation (private file)sequenceDiagram
participant User
participant FilerAdmin
participant ThumbnailerNameMixin
User->>FilerAdmin: Requests directory listing
FilerAdmin->>ThumbnailerNameMixin: get_thumbnail_name(options, transparent)
activate ThumbnailerNameMixin
ThumbnailerNameMixin->>ThumbnailerNameMixin: Check if file is public
ThumbnailerNameMixin->>ThumbnailerNameMixin: Generate custom thumbnail name
ThumbnailerNameMixin-->>FilerAdmin: Thumbnail name (Filer format)
deactivate ThumbnailerNameMixin
FilerAdmin-->>User: Displays directory listing with thumbnail
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @earthcomfy - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a comment explaining why
_q85is hardcoded in the query. - It might be clearer to use a boolean flag instead of checking the string representation of
self.thumbnail_storage.__class__.
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 2 issues found
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| "name" | ||
| )[:1] | ||
| ), | ||
| thumbnail_name=Coalesce("thumbnail_name_filer", "thumbnail_name_easy"), |
There was a problem hiding this comment.
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"),| # 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.
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.
| is_public = False | ||
| if hasattr(self, "thumbnail_storage"): | ||
| is_public = "PrivateFileSystemStorage" not in str( | ||
| self.thumbnail_storage.__class__ | ||
| ) |
There was a problem hiding this comment.
suggestion: Review the method for determining file privacy.
The approach of checking if 'PrivateFileSystemStorage' is part of the string representation of thumbnail_storage's class works, but it may be somewhat brittle if the class name changes. Consider whether a more robust type-check might be feasible.
| is_public = False | |
| if hasattr(self, "thumbnail_storage"): | |
| is_public = "PrivateFileSystemStorage" not in str( | |
| self.thumbnail_storage.__class__ | |
| ) | |
| is_public = False | |
| if hasattr(self, "thumbnail_storage"): | |
| from filer.storage import PrivateFileSystemStorage | |
| is_public = not isinstance(self.thumbnail_storage, PrivateFileSystemStorage) |
| 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.
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 crop, upscale, and different quality settings to ensure the naming pattern works correctly in all scenarios.
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.
| 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.
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.
| @@ -375,8 +375,30 @@ def directory_listing(self, request, folder_id=None, viewtype=None): | |||
| .order_by("-modified") | |||
| ) | |||
| file_qs = file_qs.annotate( | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Hi @earthcomfy ! Great start!
How about changing the filer implementation for private files to match the one for public files? This would lead to less cases needed to be looked at. The key difference is that the public ones would be configurable through easy-thumbnails settings the private onces not to keep it reversable.
Probably the regex would need to be adjusted, too.
What do you think?
| @@ -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.
Why do we need to change the signature here? Is this for testing?
| ), | ||
| # 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.
Indeed, _q85 or crop imho should not be part of the query.
Can this all be simplified by customizing the part before {size} (either . or __). Also, how would reversability be affected if we also did use the __ characters before size? Then this would dramatically simplify.
Description
We use a custom naming pattern to be able to do a reverse lookup to the filename to serve protected files in
serve_protected_thumbnail. I checked easy_thumbnail's default method of naming, and here is the difference:Easy thumbnail: "cat.jpg.100x100_q80_crop_upscale.jpg"
Filer: "cat.jpg__100x100_q80_crop.upscale.jpg"
This PR aims to use this custom naming only for private files. Otherwise, let the easy_thumbnail package handle it. Let me know if there is anything that can be improved.
Related resources
#1093
Checklist
masterSlack to find a “pr review buddy” who is going to review my pull request.
Summary by Sourcery
Tests: