Thumbnails API for SourceImages#1306
Open
loppear wants to merge 35 commits into
Open
Conversation
✅ Deploy Preview for antenna-ssec canceled.
|
✅ Deploy Preview for antenna-preview canceled.
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds end-to-end thumbnail support: THUMBNAILS settings, SourceImageThumbnail model and migration, SourceImage.find_or_generate_thumbnail_for_label, a thumbnail retrieval viewset (retrieve → redirect), serializer thumbnails field, admin registration, router entry, and tests for generation and API behavior. ChangesSourceImage Thumbnails
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (8)
ami/main/models.py (2)
2401-2401: ⚖️ Poor tradeoffConsider CASCADE instead of SET_NULL for the source_image foreign key.
The current
on_delete=models.SET_NULLwithnull=Truewill orphan thumbnail records when the parent SourceImage is deleted. Since thumbnails are derived artifacts with no value without their source image,on_delete=models.CASCADEwould be more appropriate to automatically clean up thumbnails when their source is deleted.♻️ Proposed fix
- source_image = models.ForeignKey(SourceImage, on_delete=models.SET_NULL, null=True, related_name="thumbnails") + source_image = models.ForeignKey(SourceImage, on_delete=models.CASCADE, related_name="thumbnails")Note: This would require a new migration and removing
null=Truemay require a two-step migration if there are existing NULL records.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ami/main/models.py` at line 2401, Change the Thumbnail model's source_image ForeignKey to use on_delete=models.CASCADE instead of SET_NULL and remove null=True (i.e., update the source_image field referencing SourceImage and the related_name "thumbnails"); then create the Django migration(s) to apply this change and, if there are existing NULL source_image rows, perform a two-step migration (first backfill or delete orphaned thumbnails, then make the field non-nullable) so the DB transition is safe.
2390-2402: ⚡ Quick winAdd unique constraint and index for (source_image, label).
The model allows duplicate thumbnail records for the same source_image and label combination. Consider adding a unique constraint to prevent duplicates and an index to optimize thumbnail lookups by the ViewSet.
♻️ Proposed fix
`@final` class SourceImageThumbnail(BaseModel): """A thumbnail cache of a SourceImage""" path = models.CharField(max_length=255, blank=True) label = models.CharField(max_length=255, blank=True, null=True) width = models.IntegerField(null=True, blank=True) height = models.IntegerField(null=True, blank=True) size = models.BigIntegerField(null=True, blank=True) last_modified = models.DateTimeField(null=True, blank=True) source_image = models.ForeignKey(SourceImage, on_delete=models.SET_NULL, null=True, related_name="thumbnails") + + class Meta: + constraints = [ + models.UniqueConstraint( + fields=["source_image", "label"], + name="unique_source_image_thumbnail" + ), + ] + indexes = [ + models.Index(fields=["source_image", "label"]), + ]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ami/main/models.py` around lines 2390 - 2402, The SourceImageThumbnail model allows duplicates for the (source_image, label) pair; add a database-level uniqueness constraint and an index to speed lookups by that pair: update the SourceImageThumbnail class to include a Meta with a UniqueConstraint(fields=["source_image","label"], name="uniq_sourceimage_label") and an Index(fields=["source_image","label"], name="idx_sourceimage_label"), then create and run the Django migration so the DB enforces uniqueness and provides the lookup index.config/settings/base.py (1)
592-603: ⚡ Quick winConsider adding explicit format and quality settings.
The configuration only specifies width for each size preset. Consider adding explicit settings for image format (JPEG/PNG/WebP) and quality/compression to make the thumbnail generation behavior more predictable and configurable.
💡 Example enhancement
# Sizes for Source Image Thumbnails THUMBNAILS = { "STORAGE_PREFIX": "thumbnails/", + "FORMAT": "JPEG", # or "WebP" for better compression + "QUALITY": 85, # JPEG quality 1-100 "SIZES": { "small": { - "width": 240 + "width": 240, + # height calculated to preserve aspect ratio }, "medium": { - "width": 1024 + "width": 1024, } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@config/settings/base.py` around lines 592 - 603, The THUMBNAILS config currently only defines width for each preset (THUMBNAILS -> SIZES -> "small"/"medium") which makes output format and compression unpredictable; extend each preset to include explicit format (e.g., "format": "jpeg"|"png"|"webp") and quality (e.g., "quality": 75) keys, and add top-level defaults under THUMBNAILS (e.g., "DEFAULT_FORMAT" and "DEFAULT_QUALITY") so thumbnail generation code can read format/quality from THUMBNAILS or fall back to defaults; update any thumbnail generator (references: THUMBNAILS, SIZES, "small", "medium") to use these new keys when producing images.ami/main/tests.py (1)
226-226: 💤 Low valueRemove unnecessary f-string prefix.
The f-string on line 226 contains no placeholders and should use a regular string literal.
♻️ Proposed fix
def test_thumbnail_no_list(self): - response = self.client.get(f"/api/v2/captures/thumbnails/") + response = self.client.get("/api/v2/captures/thumbnails/") self.assertEqual(response.status_code, 404)As per coding guidelines: Ruff static analysis detected F541 (f-string without placeholders).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ami/main/tests.py` at line 226, The test uses an unnecessary f-string when calling self.client.get(f"/api/v2/captures/thumbnails/"); change it to a regular string literal by removing the f-prefix so the call becomes self.client.get("/api/v2/captures/thumbnails/") to eliminate the F541 warning and satisfy Ruff static analysis.ami/main/api/serializers.py (1)
1103-1112: ⚡ Quick winAdd error handling for THUMBNAILS settings access.
The
get_thumbnailsmethod accessessettings.THUMBNAILS["SIZES"]without checking if the setting exists. IfTHUMBNAILSis not configured (e.g., in a test environment or during migration), this will raise aKeyErrorduring serialization, breaking the entire captures API response.Consider adding a guard to return
Noneor an empty dict if the setting is missing:🛡️ Proposed fix
def get_thumbnails(self, obj: SourceImage) -> dict | None: + if not hasattr(settings, 'THUMBNAILS') or 'SIZES' not in settings.THUMBNAILS: + return None return { label: reverse_with_params( "sourceimagethumbnail-detail",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ami/main/api/serializers.py` around lines 1103 - 1112, The get_thumbnails serializer method currently assumes settings.THUMBNAILS["SIZES"] exists and will raise if THUMBNAILS is missing; modify get_thumbnails to guard access to settings.THUMBNAILS (and/or the "SIZES" key) and return None or an empty dict when the config is absent, e.g., check for getattr(settings, "THUMBNAILS", None) and/or use .get("SIZES") before iterating; keep the existing reverse_with_params logic for when sizes are present so callers still receive the thumbnail URLs.ami/main/api/views.py (3)
719-719: 💤 Low valueClarify queryset model choice or update naming.
The
querysetisSourceImage.objects.all()but the viewset is namedSourceImageThumbnailViewSetand registered with basenamesourceimagethumbnail. This means the URL PK parameter refers to a SourceImage ID (not a thumbnail ID), which may confuse API consumers who expect/thumbnails/<thumbnail_id>.If this design is intentional (thumbnails accessed by source image ID + label), consider adding a docstring clarifying that the PK is a source image ID, or renaming the viewset to better reflect this pattern.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ami/main/api/views.py` at line 719, The viewset SourceImageThumbnailViewSet currently uses queryset = SourceImage.objects.all() while being registered under basename "sourceimagethumbnail", which makes the URL PK refer to a SourceImage ID rather than a thumbnail ID; either update the viewset name/registration to reflect it operates on SourceImage (e.g., rename to SourceImageThumbnailProxyViewSet or change basename to "sourceimage") or keep the name and add a clear docstring on SourceImageThumbnailViewSet stating that the PK is a SourceImage ID and that thumbnails are accessed by source image ID + label; update any related API docs/registration to match the chosen naming so consumers aren’t confused.
771-771: 💤 Low valueVerify deletion strategy for existing thumbnails.
Line 771 deletes all prior thumbnails matching the label before creating a new one. This prevents stale thumbnails when settings change (e.g., dimensions updated), but creates a small window where no thumbnail exists if the subsequent create fails. Consider whether this is acceptable or if the deletion should happen after successful creation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ami/main/api/views.py` at line 771, Current code deletes existing thumbnails via obj.thumbnails.filter(label=label).delete() before creating a new one, creating a window with no thumbnail if creation fails; change the flow to delete only after a successful creation by first creating/saving the new thumbnail (the block that constructs/saves the new thumbnail object where save() or create(...) is called) and then calling obj.thumbnails.filter(label=label).exclude(pk=new_thumb.pk).delete(), or wrap creation + swap in a transaction (transaction.atomic) so deletion happens only on success; ensure you reference obj.thumbnails.filter(label=label) and the new thumbnail instance (new_thumb) when implementing this change.
727-727: 💤 Low valueRemove unnecessary f-string prefix.
The string has no placeholders, so the
fprefix is redundant.🔧 Suggested fix
- raise api_exceptions.NotFound(detail=f"No collection of thumbnails") + raise api_exceptions.NotFound(detail="No collection of thumbnails")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ami/main/api/views.py` at line 727, The raise statement uses an unnecessary f-string; replace the f-prefixed string in the api_exceptions.NotFound call so the detail argument is a normal string (change api_exceptions.NotFound(detail=f"No collection of thumbnails") to api_exceptions.NotFound(detail="No collection of thumbnails")), leaving the exception type and detail parameter unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ami/main/admin.py`:
- Around line 314-319: get_queryset is calling select_related with an invalid
path "deployment__data_source" which causes a FieldError because
SourceImageThumbnail has no direct deployment FK; change the select_related call
in get_queryset to traverse via source_image (e.g. use "source_image",
"source_image__deployment", and "source_image__deployment__data_source" or
combine into a single list without the incorrect "deployment__data_source") so
the relationships are resolved through source_image__deployment__data_source.
In `@ami/main/api/views.py`:
- Around line 759-766: The BytesIO buffer is left at EOF after img.save so
default_storage.save reads nothing; before calling default_storage.save (where
thumbnail_key and thumbnail_path are used) reset the buffer position to the
start (seek to 0) or pass the earlier captured contents instead so the saved
file contains the JPEG data; update the code around buffer, img.save and
default_storage.save to ensure buffer.seek(0) is called (or use contents) prior
to saving.
- Around line 746-748: Replace the unconventional except clause that references
ami.utils.s3.botocore.exceptions.ClientError with the standard import pattern:
import botocore.exceptions (or from botocore.exceptions import ClientError) at
the top of views.py, catch botocore.exceptions.ClientError (or ClientError) in
the except block that currently logs via logger.error for obj.path, and re-raise
the api_exceptions.NotFound using "raise ... from e" to preserve the original
exception context while keeping the existing logger.error and NotFound detail
message.
In `@ami/main/tests.py`:
- Around line 260-269: The test test_thumbnail_settings_change_regenerates
mutates settings.THUMBNAILS["SIZES"]["small"]["width"] directly which can
pollute other tests; change it to save the original value before mutation and
restore it after the test (either by restoring in tearDown of the TestCase that
contains test_thumbnail_settings_change_regenerates or by using a
try/finally/context-manager inside the test) so the original settings are always
reinstated; reference the test method name
test_thumbnail_settings_change_regenerates and the settings key
THUMBNAILS["SIZES"]["small"]["width"] when implementing the save/restore logic.
---
Nitpick comments:
In `@ami/main/api/serializers.py`:
- Around line 1103-1112: The get_thumbnails serializer method currently assumes
settings.THUMBNAILS["SIZES"] exists and will raise if THUMBNAILS is missing;
modify get_thumbnails to guard access to settings.THUMBNAILS (and/or the "SIZES"
key) and return None or an empty dict when the config is absent, e.g., check for
getattr(settings, "THUMBNAILS", None) and/or use .get("SIZES") before iterating;
keep the existing reverse_with_params logic for when sizes are present so
callers still receive the thumbnail URLs.
In `@ami/main/api/views.py`:
- Line 719: The viewset SourceImageThumbnailViewSet currently uses queryset =
SourceImage.objects.all() while being registered under basename
"sourceimagethumbnail", which makes the URL PK refer to a SourceImage ID rather
than a thumbnail ID; either update the viewset name/registration to reflect it
operates on SourceImage (e.g., rename to SourceImageThumbnailProxyViewSet or
change basename to "sourceimage") or keep the name and add a clear docstring on
SourceImageThumbnailViewSet stating that the PK is a SourceImage ID and that
thumbnails are accessed by source image ID + label; update any related API
docs/registration to match the chosen naming so consumers aren’t confused.
- Line 771: Current code deletes existing thumbnails via
obj.thumbnails.filter(label=label).delete() before creating a new one, creating
a window with no thumbnail if creation fails; change the flow to delete only
after a successful creation by first creating/saving the new thumbnail (the
block that constructs/saves the new thumbnail object where save() or create(...)
is called) and then calling
obj.thumbnails.filter(label=label).exclude(pk=new_thumb.pk).delete(), or wrap
creation + swap in a transaction (transaction.atomic) so deletion happens only
on success; ensure you reference obj.thumbnails.filter(label=label) and the new
thumbnail instance (new_thumb) when implementing this change.
- Line 727: The raise statement uses an unnecessary f-string; replace the
f-prefixed string in the api_exceptions.NotFound call so the detail argument is
a normal string (change api_exceptions.NotFound(detail=f"No collection of
thumbnails") to api_exceptions.NotFound(detail="No collection of thumbnails")),
leaving the exception type and detail parameter unchanged.
In `@ami/main/models.py`:
- Line 2401: Change the Thumbnail model's source_image ForeignKey to use
on_delete=models.CASCADE instead of SET_NULL and remove null=True (i.e., update
the source_image field referencing SourceImage and the related_name
"thumbnails"); then create the Django migration(s) to apply this change and, if
there are existing NULL source_image rows, perform a two-step migration (first
backfill or delete orphaned thumbnails, then make the field non-nullable) so the
DB transition is safe.
- Around line 2390-2402: The SourceImageThumbnail model allows duplicates for
the (source_image, label) pair; add a database-level uniqueness constraint and
an index to speed lookups by that pair: update the SourceImageThumbnail class to
include a Meta with a UniqueConstraint(fields=["source_image","label"],
name="uniq_sourceimage_label") and an Index(fields=["source_image","label"],
name="idx_sourceimage_label"), then create and run the Django migration so the
DB enforces uniqueness and provides the lookup index.
In `@ami/main/tests.py`:
- Line 226: The test uses an unnecessary f-string when calling
self.client.get(f"/api/v2/captures/thumbnails/"); change it to a regular string
literal by removing the f-prefix so the call becomes
self.client.get("/api/v2/captures/thumbnails/") to eliminate the F541 warning
and satisfy Ruff static analysis.
In `@config/settings/base.py`:
- Around line 592-603: The THUMBNAILS config currently only defines width for
each preset (THUMBNAILS -> SIZES -> "small"/"medium") which makes output format
and compression unpredictable; extend each preset to include explicit format
(e.g., "format": "jpeg"|"png"|"webp") and quality (e.g., "quality": 75) keys,
and add top-level defaults under THUMBNAILS (e.g., "DEFAULT_FORMAT" and
"DEFAULT_QUALITY") so thumbnail generation code can read format/quality from
THUMBNAILS or fall back to defaults; update any thumbnail generator (references:
THUMBNAILS, SIZES, "small", "medium") to use these new keys when producing
images.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4960fc79-aeb3-4154-9954-83da5c3b9892
📒 Files selected for processing (9)
ami/main/admin.pyami/main/api/serializers.pyami/main/api/views.pyami/main/migrations/0085_sourceimagethumbnail.pyami/main/models.pyami/main/tests.pyami/utils/s3.pyconfig/api_router.pyconfig/settings/base.py
💤 Files with no reviewable changes (1)
- ami/utils/s3.py
mihow
reviewed
May 14, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
ami/main/tests.py (1)
238-238: 💤 Low valueRemove unnecessary f-string prefix.
The f-string has no placeholders.
📝 Suggested change
- response = self.client.get(f"/api/v2/captures/thumbnails/") + response = self.client.get("/api/v2/captures/thumbnails/")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ami/main/tests.py` at line 238, In tests.py replace the unnecessary f-string used in the self.client.get call by removing the "f" prefix so the literal URL string passed to self.client.get("/api/v2/captures/thumbnails/") is a normal string; locate the call to self.client.get(f"/api/v2/captures/thumbnails/") (the assignment to response) and change it to use a plain string.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ami/main/api/views.py`:
- Around line 771-775: The thumbnail regeneration only deletes DB rows
(obj.thumbnails.filter(label=label).delete()) but not the underlying storage
blob; update the logic in the thumbnail creation flow (the block around
obj.thumbnails.filter(label=label).delete() and obj.thumbnails.create(...)) to
first iterate existing thumbnail instances for that label, call
default_storage.delete(existing.path) for each existing.path (checking
storage.exists if desired), then remove the DB rows and create the new thumbnail
via obj.thumbnails.create(path=thumbnail_path, ...). Ensure you use the same
identifiers: default_storage, obj.thumbnails.filter(label=label), existing.path,
and thumbnail_path so old media files are removed from storage when
regenerating.
- Around line 741-742: The thumbnail cache currently ignores source changes;
modify the thumbnail lookup in the view (where it checks thumb and
default_storage.exists(thumb.path) and in the similar block at the other
occurrence) to also compare the cached row's stored last_modified (or size)
against obj.last_modified (which is updated by sync) and treat the cache as a
miss if they differ; persist obj.last_modified into the thumbnail/cache model
when creating/updating a thumbnail so future requests can compare and invalidate
stale thumbs, and on mismatch either delete the stored thumbnail
(default_storage.delete(thumb.path)) or skip returning the redirect so a fresh
thumbnail is generated.
- Around line 753-758: The code assigns new_size only inside the "if not height"
branch causing an UnboundLocalError when size includes height; to fix it, always
build new_size: retrieve width = size["width"] and height = size.get("height",
None), compute height = int(orig_height * (width / float(orig_width))) only when
height is falsy, then set new_size = (width, height) outside/after that
conditional so img.thumbnail(new_size) always has a defined tuple (refer to
variables width, height, new_size, orig_width, orig_height and the img.thumbnail
call).
- Around line 723-724: _class attributes `_sizes` and `_prefix` are read at
import time and must be changed to per-request properties that read
settings.THUMBNAILS each access so runtime changes (e.g. via override_settings)
take effect; replace the static attributes `_sizes` and `_prefix` with `@property`
methods (or equivalent instance properties) that return
settings.THUMBNAILS["SIZES"] and settings.THUMBNAILS["STORAGE_PREFIX"]
respectively, update any references in the class (e.g., inside the thumbnail
generation methods) to use self._sizes and self._prefix so the view (tested by
test_thumbnail_settings_change_regenerates) reads fresh values on every request.
---
Nitpick comments:
In `@ami/main/tests.py`:
- Line 238: In tests.py replace the unnecessary f-string used in the
self.client.get call by removing the "f" prefix so the literal URL string passed
to self.client.get("/api/v2/captures/thumbnails/") is a normal string; locate
the call to self.client.get(f"/api/v2/captures/thumbnails/") (the assignment to
response) and change it to use a plain string.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 841ee2b6-dc36-4a0e-94fd-826bffbf030b
📒 Files selected for processing (6)
ami/main/admin.pyami/main/api/serializers.pyami/main/api/views.pyami/main/models.pyami/main/tests.pyconfig/settings/base.py
🚧 Files skipped from review as they are similar to previous changes (2)
- config/settings/base.py
- ami/main/api/serializers.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ami/main/api/views.py`:
- Around line 740-744: Change the thumbnail cache lookup so it doesn't filter by
configured dimensions (obj.thumbnails.get(**size))—instead fetch the thumbnail
by its label (e.g. obj.thumbnails.get(label=label) or
filter(label=label).first()) and then treat a width/height mismatch as a cache
miss in the same check that uses thumb.last_modified and default_storage.exists;
specifically compare thumb.width (and thumb.height when height is explicitly
configured in the SIZES entry) against the requested size and proceed to
delete/recreate only when dimensions differ or when last_modified/exists
indicate staleness. Ensure you still handle SourceImageThumbnail.DoesNotExist
and preserve the existing delete/upload flow.
In `@ami/main/migrations/0085_sourceimagethumbnail.py`:
- Around line 25-33: Change the ForeignKey on SourceImageThumbnail named
source_image to use on_delete=django.db.models.deletion.CASCADE and remove
null=True from the field definition in the SourceImageThumbnail model so
thumbnails are deleted with their SourceImage; add a pre_delete signal (or model
delete override) for SourceImageThumbnail that calls
django.core.files.storage.default_storage.delete(instance.path) to remove the
file from storage when a thumbnail is deleted; ensure you create a follow-up
migration to update existing rows that have source_image_id IS NULL (or
delete/associate them) before removing nullability.
In `@ami/main/tests.py`:
- Line 237: The test uses a no-op f-string in the GET request (response =
self.client.get(f"/api/v2/captures/thumbnails/")) which triggers Ruff F541;
change it to a plain string by removing the leading f (response =
self.client.get("/api/v2/captures/thumbnails/")) to eliminate the warning and
keep behavior identical.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f36689e1-7ffe-40f8-9061-c2db396a8a2d
📒 Files selected for processing (7)
ami/main/admin.pyami/main/api/serializers.pyami/main/api/views.pyami/main/migrations/0085_sourceimagethumbnail.pyami/main/models.pyami/main/tests.pyconfig/api_router.py
🚧 Files skipped from review as they are similar to previous changes (3)
- ami/main/admin.py
- ami/main/models.py
- ami/main/api/serializers.py
mihow
reviewed
May 15, 2026
mihow
reviewed
May 15, 2026
mihow
reviewed
May 15, 2026
mihow
reviewed
May 15, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ami/main/api/views.py`:
- Around line 726-740: The retrieve view currently issues a permanent redirect
which causes caching of the media URL; update the redirect in retrieve (the
redirect(...) call) to be temporary instead of permanent so browsers/CDNs won't
cache the target and lazy-regeneration still works (i.e., set permanent=False or
otherwise use a temporary redirect in the redirect(...) invocation in retrieve).
In `@ami/main/models.py`:
- Around line 2224-2262: The code only generates thumbnails when
self.deployment.data_source exists, causing uploaded captures (saved to
default_storage by create_source_image_from_upload()) to 404; change the
conditional in the thumbnail generation method to handle a fallback when
deployment.data_source is missing but self.path exists: attempt to read the
image via read_image(config=config, key=self.path) when deployment.data_source
is present, and otherwise open the file directly from default_storage (e.g.,
default_storage.open(self.path) and PIL.Image.open()) with the same error
handling and thumbnail creation flow (keep existing uses of read_image,
default_storage.save, self.thumbnails.create and the prior thumbnail cleanup),
so both data-source-backed and uploaded images produce thumbnails.
- Around line 2451-2463: Add a DB-level uniqueness constraint and index on the
(source_image, label) pair to prevent concurrent-creation duplicates: update the
SourceImageThumbnail model (class SourceImageThumbnail) Meta to declare a
UniqueConstraint on ('source_image', 'label') and add a matching Index for the
same columns (or use unique_together plus an Index) so lookups performed by
find_or_generate_thumbnail_for_label() are safe and fast; then create and run
the corresponding Django migration to apply the constraint.
- Around line 2247-2258: The code writes the new thumbnail to thumbnail_key via
default_storage.save() before removing existing thumbnails, which causes the
just-saved file to be deleted when iterating
self.thumbnails.filter(label=label); change the order so you first delete
existing storage objects and DB rows (iterate
self.thumbnails.filter(label=label) and call default_storage.delete(t.path) then
t.delete()) and only then call default_storage.save(thumbnail_key, buffer) and
create the new thumbnail record (thumb = self.thumbnails.create(...)). Also add
a uniqueness constraint on the SourceImageThumbnail model for (source_image,
label) (and adjust any .get(label=label) usage) to enforce the
one-thumbnail-per-label assumption.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3208e6c1-5e4e-4068-8721-2eb36d86e717
📒 Files selected for processing (2)
ami/main/api/views.pyami/main/models.py
…edirects, and add index for (source_image, label).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
ami/main/tests.py (1)
237-237:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove the no-op f-string to fix Ruff F541.
The f-prefix serves no purpose here since there are no placeholders.
🔧 Proposed fix
- response = self.client.get(f"/api/v2/captures/thumbnails/") + response = self.client.get("/api/v2/captures/thumbnails/")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ami/main/tests.py` at line 237, The failing Ruff F541 is caused by a no-op f-string in the test where you call self.client.get(f"/api/v2/captures/thumbnails/"); remove the unnecessary f-prefix so the call becomes self.client.get("/api/v2/captures/thumbnails/"). Update the line in ami/main/tests.py (the test containing the response = self.client.get(...) call) to use a plain string literal to eliminate the lint error.ami/main/migrations/0085_sourceimagethumbnail.py (1)
25-33:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMigration mirrors the
SET_NULLFK that orphans thumbnail rows.Same issue as raised on the model definition in
ami/main/models.py: when aSourceImageis deleted, this leavesSourceImageThumbnailrows withsource_image=NULLand danglingdefault_storageblobs. Switch toCASCADEhere in lockstep with the model change so the two stay consistent.🐛 Proposed migration change
( "source_image", models.ForeignKey( - null=True, - on_delete=django.db.models.deletion.SET_NULL, + on_delete=django.db.models.deletion.CASCADE, related_name="thumbnails", to="main.sourceimage", ), ),If any rows have been created with
source_image_id IS NULLduring testing, you'll need a follow-up data migration to delete them before thenull=Truecan be dropped.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ami/main/migrations/0085_sourceimagethumbnail.py` around lines 25 - 33, Migration 0085_sourceimagethumbnail.py currently defines the SourceImageThumbnail field "source_image" as models.ForeignKey(..., null=True, on_delete=SET_NULL) which leaves orphaned thumbnails and blobs when a SourceImage is deleted; change the FK to use on_delete=django.db.models.deletion.CASCADE (and remove null=True if you also intend to make the column non-nullable) so thumbnails are deleted together with SourceImage, and if any records with source_image_id IS NULL exist add a small follow-up data migration to delete those orphaned SourceImageThumbnail rows before removing null=True.ami/main/models.py (1)
2454-2475:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
SET_NULLorphans thumbnail rows and their storage objects.A
SourceImageThumbnailis meaningless without itsSourceImage(the viewset only resolves thumbnails through aSourceImagepk), soon_delete=SET_NULLleaves rows that are unreachable from the API while still occupying the DB and the underlyingdefault_storageblob. The unique constraint also won't deduplicate orphans — Postgres treatsNULLs as distinct, so multiple orphaned(NULL, "small")rows can coexist. PreferCASCADE(dropnull=True) and pair it with file cleanup via apre_deletesignal that callsdefault_storage.delete(instance.path).Note: this also requires updating the migration in
ami/main/migrations/0085_sourceimagethumbnail.pyto match, and any production data withsource_image_id IS NULLwill need to be cleaned up before the migration can drop nullability.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ami/main/models.py` around lines 2454 - 2475, Change SourceImageThumbnail to not orphan thumbnails: set source_image ForeignKey on_delete=models.CASCADE and remove null=True (and blank if present) on the source_image field in the SourceImageThumbnail model; add a pre_delete signal handler for SourceImageThumbnail that calls default_storage.delete(instance.path) to remove the underlying file when a thumbnail row is deleted; update the migration ami/main/migrations/0085_sourceimagethumbnail.py to reflect the nullability and on_delete change, and ensure any production rows with source_image_id IS NULL are deleted or remapped before applying the migration.
🧹 Nitpick comments (3)
ami/main/tests.py (1)
275-275: ⚡ Quick winUse timezone-aware datetime for Django compatibility.
datetime.datetime.now()creates a naive datetime, which can cause comparison warnings or errors in Django projects withUSE_TZ=True. Usetimezone.now()fromdjango.utilsinstead to ensure timezone-aware datetimes.🔧 Proposed fix
First, ensure the import at the top of the file:
from django.utils import timezoneThen update line 275:
- self.first_capture.last_modified = datetime.datetime.now() + self.first_capture.last_modified = timezone.now()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ami/main/tests.py` at line 275, Replace the naive datetime assignment to self.first_capture.last_modified with a timezone-aware timestamp: import timezone from django.utils (add "from django.utils import timezone") and set self.first_capture.last_modified = timezone.now() so tests use Django-compatible aware datetimes.ami/main/models.py (2)
2232-2235: 💤 Low valueClose the storage file handle and handle missing-file errors symmetrically.
For the uploaded-capture branch,
default_storage.open(self.path)returns a file object that is never closed (PIL keeps the underlying stream alive but the storage wrapper isn't released), and a missing key raises a genericFileNotFoundError/backend-specific error that isn't translated toObjectDoesNotExistlike the S3 branch does. Use awithblock and surface a consistentObjectDoesNotExistso the viewset can map this to a 404 instead of a 500.♻️ Proposed refactor
- elif self.path: - img = PIL.Image.open(default_storage.open(self.path)) + elif self.path: + try: + with default_storage.open(self.path) as fh: + img = PIL.Image.open(fh) + img.load() # force decode before fh closes + except (FileNotFoundError, OSError) as e: + logger.error(f"Could not read uploaded image for {self.path}: {e}") + raise ObjectDoesNotExist( + f"SourceImage with id {self.pk} media not found" + ) from e🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ami/main/models.py` around lines 2232 - 2235, The uploaded-capture branch opens a storage file with default_storage.open(self.path) and never closes it and it raises backend-specific FileNotFoundError instead of ObjectDoesNotExist; change the code in the SourceImage loading path (the branch that checks self.path and uses PIL.Image.open) to open the storage file inside a with default_storage.open(self.path) as f: block, pass f (or f.file/stream) into PIL.Image.open so the file handle is released, and catch FileNotFoundError (and any storage-specific missing-file exceptions) to re-raise django.core.exceptions.ObjectDoesNotExist(f"SourceImage with id {self.pk} media config not found") so missing files are reported symmetrically with the S3 branch.
2462-2462: 💤 Low value
last_modifiedwithauto_now_add=TrueduplicatesBaseModel.created_at.
find_or_generate_thumbnail_for_labelalways deletes the existing row andcreate()s a fresh one on regeneration, solast_modifiedis set to "now" on every (re)create and never updates afterwards — semantically identical tocreated_atfromBaseModel. If the intent is "when was this cached thumbnail last regenerated", useauto_now=Trueinstead (and dropnull=True, blank=True, which never apply when auto-managed). Otherwise consider dropping the field and usingcreated_atin the staleness check.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ami/main/models.py` at line 2462, The last_modified DateTimeField is declared with auto_now_add=True, making it duplicate BaseModel.created_at because thumbnails are deleted and recreated by find_or_generate_thumbnail_for_label via create(); change last_modified to auto-managed "last updated" semantics by setting auto_now=True and remove null=True, blank=True (i.e., last_modified = models.DateTimeField(auto_now=True)), or if you prefer to rely on BaseModel.created_at for staleness checks, drop the last_modified field entirely and update any references in find_or_generate_thumbnail_for_label to use created_at instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ami/main/api/views.py`:
- Around line 727-730: The code assumes settings.THUMBNAILS["SIZES"] (_sizes) is
non-empty and uses next(iter(_sizes)) which raises StopIteration if empty;
update the code that computes label/size (the block using _sizes, label, size
and self.request.query_params) to first check if _sizes is falsy/empty and
return a clear error (e.g., raise an APIException/ValidationError or return a
Response with a descriptive message about missing thumbnail size configuration)
instead of calling next(iter(_sizes)); then only compute label =
query_params.get("label", next(iter(_sizes))) and size = _sizes.get(label, None)
when _sizes is non-empty.
In `@ami/main/models.py`:
- Around line 2218-2223: The thumbnail re-generation check can raise TypeError
because SourceImage.last_modified is nullable (set by
create_source_image_from_upload), so guard the datetime comparison in the
condition that uses thumb.last_modified < self.last_modified: ensure both
thumb.last_modified and self.last_modified are truthy datetimes before comparing
(e.g. only perform the < check when both are non-None), keeping the existing
checks for thumb, width, and default_storage.exists; update the conditional
around thumb, size, and default_storage.exists in the method that currently
contains the if (not thumb or thumb.width != size["width"] or
thumb.last_modified < self.last_modified or not
default_storage.exists(thumb.path)) to avoid comparing None with datetime.
- Around line 2236-2248: The JPEG save fails for non-RGB source images (e.g.,
RGBA, P) because img.save(buffer, format="JPEG") cannot write those modes;
before saving the thumbnail, convert the PIL Image to an appropriate mode (e.g.,
call img.convert("RGB") when img.mode is not already JPEG-compatible) so that
the subsequent img.save(buffer, format="JPEG") succeeds; update the code around
the thumbnail creation/BytesIO block (the img.thumbnail(...), buffer =
BytesIO(), img.save(...), contents = buffer.getvalue()) to perform the
conversion on the img object prior to saving.
---
Duplicate comments:
In `@ami/main/migrations/0085_sourceimagethumbnail.py`:
- Around line 25-33: Migration 0085_sourceimagethumbnail.py currently defines
the SourceImageThumbnail field "source_image" as models.ForeignKey(...,
null=True, on_delete=SET_NULL) which leaves orphaned thumbnails and blobs when a
SourceImage is deleted; change the FK to use
on_delete=django.db.models.deletion.CASCADE (and remove null=True if you also
intend to make the column non-nullable) so thumbnails are deleted together with
SourceImage, and if any records with source_image_id IS NULL exist add a small
follow-up data migration to delete those orphaned SourceImageThumbnail rows
before removing null=True.
In `@ami/main/models.py`:
- Around line 2454-2475: Change SourceImageThumbnail to not orphan thumbnails:
set source_image ForeignKey on_delete=models.CASCADE and remove null=True (and
blank if present) on the source_image field in the SourceImageThumbnail model;
add a pre_delete signal handler for SourceImageThumbnail that calls
default_storage.delete(instance.path) to remove the underlying file when a
thumbnail row is deleted; update the migration
ami/main/migrations/0085_sourceimagethumbnail.py to reflect the nullability and
on_delete change, and ensure any production rows with source_image_id IS NULL
are deleted or remapped before applying the migration.
In `@ami/main/tests.py`:
- Line 237: The failing Ruff F541 is caused by a no-op f-string in the test
where you call self.client.get(f"/api/v2/captures/thumbnails/"); remove the
unnecessary f-prefix so the call becomes
self.client.get("/api/v2/captures/thumbnails/"). Update the line in
ami/main/tests.py (the test containing the response = self.client.get(...) call)
to use a plain string literal to eliminate the lint error.
---
Nitpick comments:
In `@ami/main/models.py`:
- Around line 2232-2235: The uploaded-capture branch opens a storage file with
default_storage.open(self.path) and never closes it and it raises
backend-specific FileNotFoundError instead of ObjectDoesNotExist; change the
code in the SourceImage loading path (the branch that checks self.path and uses
PIL.Image.open) to open the storage file inside a with
default_storage.open(self.path) as f: block, pass f (or f.file/stream) into
PIL.Image.open so the file handle is released, and catch FileNotFoundError (and
any storage-specific missing-file exceptions) to re-raise
django.core.exceptions.ObjectDoesNotExist(f"SourceImage with id {self.pk} media
config not found") so missing files are reported symmetrically with the S3
branch.
- Line 2462: The last_modified DateTimeField is declared with auto_now_add=True,
making it duplicate BaseModel.created_at because thumbnails are deleted and
recreated by find_or_generate_thumbnail_for_label via create(); change
last_modified to auto-managed "last updated" semantics by setting auto_now=True
and remove null=True, blank=True (i.e., last_modified =
models.DateTimeField(auto_now=True)), or if you prefer to rely on
BaseModel.created_at for staleness checks, drop the last_modified field entirely
and update any references in find_or_generate_thumbnail_for_label to use
created_at instead.
In `@ami/main/tests.py`:
- Line 275: Replace the naive datetime assignment to
self.first_capture.last_modified with a timezone-aware timestamp: import
timezone from django.utils (add "from django.utils import timezone") and set
self.first_capture.last_modified = timezone.now() so tests use Django-compatible
aware datetimes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 331b6a24-7049-49b5-963a-a068cdf22aaf
📒 Files selected for processing (4)
ami/main/api/views.pyami/main/migrations/0085_sourceimagethumbnail.pyami/main/models.pyami/main/tests.py
Collaborator
Author
|
Now adds:
Not sure why the frontend lint/tests are cancelling for this PR, but I did get them running locally to check. |
Collaborator
|
Excellent work! Everything is working in my brief testing, and the session detail view is loading way faster! One funny quirk, I think the bounding box overlay uses the size of the original image to calculate the placement of boxes, so now they end up outside the image! Probably one good reason for us to offer relative bounding box coordinates in the API instead of absolute ones. 
|
Member
|
I pushed a small fix for the thing you pointed out @mihow . We are actually inspecting the natural size of the image after load. The reason is we had problems with the the API not always returning the size, see this old PR: #459. After my fix here, we will prioritize the size returned by the API and only use the natural size as a fallback. Since bounding boxes are currently specified in pixels, not in percent, we need the size to render them. If we can trust the API to always return a size for the image, we could simplify the FE code and skip the natural size check. We could also do this if bounding boxes were specified in percent, instead of pixels. |
Collaborator
I remembered the reason while we can't guarantee that image sizes will always be there! We only know the filename, URL, file size and MD5 hash when images are synced. We don't know the image size until the image is processed. Only then is the original image downloaded & read. But! If an image has detections, that means it has been processed. Right now if the image hasn't been processed, we infer the image size based on the first image in the session. |
Collaborator
|
@loppear this is the 500 error I see on arctia 
https://arctia.dev.antenna.insectai.org/api/v2/captures/thumbnails/4820504/?label=small
There are only a couple places where Antenna loads a source image, here is one. This will work no matter if the image is stored in S3 or another storage backend. This will work for both public and private s3 buckets too. example usage: There is also Lines 615 to 629 in f585ddc |
Conflicts: - ami/main/tests.py: combined import additions (create_captures_from_files + create_detections, both used). - ami/main/migrations/0088_*: renamed 0088_sourceimagethumbnail_and_more.py to 0089_*, repointed dependency to the new 0088_detection_det_srcimg_created_idx from main. Linear chain, no merge migration needed.
JPEG natively supports only L, RGB, and CMYK modes. Source images in RGBA/P/LA/PA modes (common for uploaded PNGs) raised "OSError: cannot write mode <X> as JPEG" on `img.save(format="JPEG")`, 500-ing the thumbnail endpoint. Convert to RGB first when needed. Adds a regression test that feeds an RGBA PNG through find_or_generate and asserts the redirect succeeds and the thumbnail row lands. Closes review thread on ami/main/models.py L2241. Co-Authored-By: Claude <noreply@anthropic.com>
Two changes, one root cause.
`SourceImage.last_modified` tracks the source bytes' mtime (S3 LastModified
on the sync path) and is the freshness signal the thumbnail regen check uses
to decide whether to invalidate a cached thumbnail. It is NOT the same as
`updated_at` (BaseModel auto_now), which bumps on every row save (cached
counts, detection counts, etc.) and would force regen far too often.
The upload path (`create_source_image_from_upload`) never set this field,
so:
1. Uploaded captures had `last_modified=None` forever, breaking parity
with sync-created rows.
2. The thumbnail regen check `thumb.last_modified < self.last_modified`
raised `TypeError` on the 2nd+ request for any uploaded capture's
thumbnail (datetime < None).
Fixes:
* `create_source_image_from_upload`: set `last_modified=timezone.now()` so
uploaded captures have a real source mtime, matching the sync path's
behaviour of copying the S3 LastModified header.
* `find_or_generate_thumbnail_for_label`: guard the comparison. None on
either side now means "no signal of source change, trust the cached
thumb" instead of crashing. Covers legacy rows synced before the upload
backfill.
Tests:
* `test_thumbnail_source_last_modified_none_does_not_crash` reproduces the
TypeError class of bug by nulling `last_modified` after a thumbnail row
exists and asserts the 2nd request reuses the cache.
* `test_upload_handler_backfills_last_modified` covers the upload-path
backfill (asserts the new field falls inside [before, after] of the
upload call).
Closes review thread on ami/main/models.py L2227.
Co-Authored-By: Claude <noreply@anthropic.com>
Two F541 (Ruff: f-string without placeholders) hits, plus a small behavioural cleanup on the list action. * `SourceImageThumbnailViewSet.list` now raises `MethodNotAllowed` (405) instead of `NotFound` (404). The route exists for `/captures/thumbnails/<pk>/?label=...` only; listing has no defined semantics. 405 sets the Allow header and is the correct REST response. * Updated `test_thumbnail_no_list` to expect 405 and to use a plain string literal (the previous test used an f-string with no placeholders). Closes review thread on ami/main/tests.py L237 (Ruff F541). Co-Authored-By: Claude <noreply@anthropic.com>
When `settings.THUMBNAILS['SIZES']` is empty (misconfiguration), the default-label fallback `next(iter(_sizes))` raised `StopIteration` and surfaced as a 500 with no useful detail. Replace with an explicit empty check that returns 404 with a clear API error message, and use `next(iter(_sizes))` only after we know the dict is non-empty (also switch the fallback expression to `or next(...)` since the previous form passed `next(iter(...))` as the default arg unconditionally — fine for non-empty dicts but still worth tidying alongside the guard). Adds a test that overrides THUMBNAILS with an empty SIZES dict and asserts the endpoint returns 404 with the configured-error message. Co-Authored-By: Claude <noreply@anthropic.com>
`requests.get(url)` with no timeout blocks indefinitely if the upstream storage is slow or unreachable. The thumbnail-generation path (`SourceImage.find_or_generate_thumbnail_for_label`) runs this synchronously inside a Django web request, so a stuck upstream ties up a gunicorn worker; a gallery's worth of stuck calls drains the pool. Default timeout is `(connect=5s, read=30s)`. Callers that need different limits (e.g. very large originals on a slow link) can override by passing `timeout=`. Behaviour for fast / responsive upstreams is unchanged. Tests verify the default is finite and that explicit overrides are honored. Co-Authored-By: Claude <noreply@anthropic.com>
…rent-gen race Two related bugs in `find_or_generate_thumbnail_for_label`: 1. Under concurrent gen for the same (source_image, label) pair the loser could either (a) hit the new UniqueConstraint and 500 with IntegrityError when its delete-loop ran before the winner's INSERT, or (b) destroy the winner's row + storage blob when the delete-loop ran after. Either outcome is wrong; both were possible because the code did `filter(label=label).delete()` then `thumbnails.create(...)` instead of a single atomic upsert. 2. The pre-save delete-loop also deleted the storage blob BEFORE the new `default_storage.save()`, which on storage backends that suffix colliding keys (FileSystemStorage default) left the row pointing at a path that the next save could overwrite in the same way the racer would. Belt-and-braces failure where the cleanup order itself was the bug. Fix: * Replace the delete-then-create with `update_or_create` keyed on `label`. Django serializes via the UniqueConstraint and, on collision, catches IntegrityError on INSERT and re-runs as UPDATE. No 500s, no destructive deletes of concurrent rows. * Snapshot the prior `path` before the new save and clean it up after the row update only when the backend gave us a new key (`prior_path != thumbnail_path`). Backends that overwrite in place (boto3 with `AWS_S3_FILE_OVERWRITE=True`) skip the cleanup and avoid the file-deletion race entirely. * Force-bump `thumb.last_modified` on the UPDATE branch via `QuerySet.update` because `auto_now_add=True` only fires on INSERT — without this, regen via source-mtime bump would re-trigger regen on every subsequent request as the field's pre_save callback preserved the existing (now-stale) value. Tests: * `test_thumbnail_regen_reuses_row_via_upsert`: delete the storage blob out from under a cached row, re-request, assert the row's PK is preserved (proving the UPDATE path was taken, not a fresh INSERT). * `test_thumbnail_exists_newer_modified_source` updated to assert row reuse + freshness bump rather than the previous (incidental) PK change. The `SourceImageThumbnail.last_modified` field semantics are still muddled (see PR thread; rename or drop tracked separately). Co-Authored-By: Claude <noreply@anthropic.com>
…w delete Thumbnails are pure derivatives of (source_image, label). The previous `on_delete=SET_NULL, null=True` left orphan rows AND their storage blobs behind forever whenever the parent SourceImage was deleted, with no other reaper in the codebase. Silent storage growth and orphan-row accumulation; CodeRabbit flagged this in PR #1306 and the thread was closed without a fix. Changes: * `SourceImageThumbnail.source_image` → `on_delete=CASCADE`, no longer nullable. Deleting a SourceImage now takes its thumbnail rows with it. * New `pre_delete` signal in `ami.main.signals` does best-effort `default_storage.delete(instance.path)` so the blob doesn't outlive the row. Fires on explicit row deletes AND CASCADE deletes (signals fire for both), so this covers parent SourceImage deletion automatically. Storage failures are logged and swallowed — a transient backing-store error must not block the delete chain. * Migration 0090 is two-step: reap pre-existing rows where `source_image_id IS NULL` (the SET_NULL aftermath) first, then AlterField the FK. Orphan-row reap runs through the same pre_delete signal so their blobs are cleaned alongside. Tests: * `test_source_image_delete_cascades_to_thumbnails` — create 2 thumbnail rows for a capture, delete the capture, assert both rows gone via CASCADE. * `test_thumbnail_delete_removes_storage_blob` — plant a real blob in storage, wire a row to it, delete the row, assert the blob is gone (proving the pre_delete signal fired and cleaned up). Co-Authored-By: Claude <noreply@anthropic.com>
`size`, `last_modified`, `checksum`, `checksum_algorithm` all record metadata read from the source image file in storage during sync or upload, not row-level mtime. Document that in help_text so the field's intent shows up in the admin UI and in API schema output (the four fields previously had no help_text at all). Mark the cluster read-only in the SourceImage admin so operators can't clobber the cached metadata by mistake through the change form. Left writable on the API so a project owner can still create SourceImage entries by hand via an API client when the sync/upload flows don't fit; the sync endpoint at POST /api/v2/deployments/<pk>/sync/ remains the default population path. Adds migration 0091 for the help_text changes (no schema change beyond the AlterField metadata). Co-Authored-By: Claude <noreply@anthropic.com>
…0089 0089 was renamed-into-existence this branch (originally 0088, bumped when main added 0088_detection_det_srcimg_created_idx). The follow-up 0090 only existed to switch the just-created FK from SET_NULL to CASCADE — same model, same concern, accidentally split because 0089 shipped wrong on the first cut. This commit: * Rewrites 0089 to create the FK with CASCADE from the start (drops null=True and the SET_NULL on_delete). The RunPython orphan-reaper from 0090 is gone — a fresh table can't have orphans. * Deletes 0090. * Updates 0091's dependency to point at 0089 (was 0090). End DB state is identical to the old 0089+0090 pair, so anyone who already migrated past 0090 just pulls — Django sees 0089 as already-applied (same name) and skips. 0090 will appear as an applied migration with no matching file; warning only, no breakage. Local devs who want a clean django_migrations table can manually DELETE FROM django_migrations WHERE app='main' AND name='0090_sourceimagethumbnail_cascade_source_image'. Safe because this migration only landed on the in-flight feature branch — no production state to preserve. CI re-runs from scratch each time and passed (TestImageThumbnailViews 18/18 with --noinput on a fresh DB). Co-Authored-By: Claude <noreply@anthropic.com>
The collapse rationale is unnecessary in-tree — the branch was never merged, so no production state ever ran the SET_NULL form. Anyone mid-rebase on this branch will just see the new migration replace the old one cleanly. Co-Authored-By: Claude <noreply@anthropic.com>
Collaborator
|
@loppear I pushed a few small fixes related to data integrity checks, clarification and formatting. The CI is passing now. I made one bigger change, but pushed it to a separate branch/PR to see what you think. This avoids n+1 queries and hitting the Django server and S3 API for every thumbnail URL. Which is mostly important for any of the list views: #1331 I re-deployed the arctia environment with the latest |
Collaborator
Author
|
Cleanup looks fine to me, CASCADE and update_or_create changes seem right. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Provide thumbnails for SourceImages, stored in
default_storage, generated on image request and paths "cached" in new tableSourceImageThumbnail.List of Changes
THUMBNAILSto define storage prefix and pre-defined sizes.SourceImageThumbnail/api/v2/captures/thumbnails/<source_image_id>(that only implements detail) for proxying image requests and perform thumbnailingcapturesAPI responses to include athumbnailsproperty with urls keyed by thumbnail size label, e.g..thumbnails.small.Related Issues
Implements #236
Detailed Description
Thumbnails are not generated during capture requests, but delayed until frontend requests image, at which point we check if there is a valid thumbnail already or need to generate.
This does NOT implement front-end changes to use this
thumbnailsproperty of captures. Happy to include a first stab at that for the captures list gallery before this is accepted.Other considerations implicated:
How to Test the Changes
Adds tests (
manage.py test -k thumbnail) for the new API view and modifications to serializing captures.Deployment Notes
Adds database migration for new
SourceImageThumbnailtableChecklist
Summary by CodeRabbit
New Features
Behavior
Tests