feat(files): baseapp.files package — attach files to any object (multipart upload, plugin arch) - #442
Open
nossila wants to merge 21 commits into
Open
feat(files): baseapp.files package — attach files to any object (multipart upload, plugin arch)#442nossila wants to merge 21 commits into
nossila wants to merge 21 commits into
Conversation
- AbstractFile/AbstractFileTarget rebuilt on DocumentIdMixin + django-swappable-models
(drops the removed CommentableModel/ReactableModel/ReportableModel mixins)
- FilesPlugin registered via the plugin registry (GraphQL queries/mutations, v1 files
router, FilesPermissionsBackend auth slot) + pyproject entry point
- apps.py uses BaseAppConfig and registers FilesInterface as a shared GraphQL interface
- File ObjectType implements FileInterface and composes Comments/Reactions via
graphql_shared_interfaces.get()
- comments opt into files as a target (guarded FileableModel mixin + FilesInterface)
- baseapp_core.graphql: File type + ThumbnailField return the URL string directly
(no nested { url }); image-field consumers updated to the string contract
- testproject migrations regenerated for the ported concrete models
- rebased onto origin/master
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XyuGcRJryi9hQiWRVXU7Js
Replace stock django.contrib.admin.ModelAdmin with the project's baseapp_core.admin_helpers.ModelAdmin for FileAdmin and FileTargetAdmin. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XyuGcRJryi9hQiWRVXU7Js
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0146oeiGsELS6QVpVUQHDQXd
…ared service) - AbstractFileTarget.annotate_queryset: correlated Subquery annotations (_file_target_files_count, _file_target_is_files_enabled, files_count_total) mirroring ReactableMetadata.annotate_queryset, plus get_for_object helper. - FilesMetadataService (shared service "files_metadata") with annotation-aware getters and annotate_queryset delegate; registered via ServicesContributor. - FilesInterface resolvers now go through the service and filter files via the DocumentId join instead of per-row get_file_target()/DocumentId.get — no more N+1 on lists; disabled targets return an empty queryset. - Comments pre_optimization_hook applies files annotations alongside commentable/reactable. - Align ContentType lookups with baseapp_core (concrete model) and reuse DocumentId.get_or_create_for_object; for_concrete_model=False would miss trigger-created DocumentId rows for proxy models. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0146oeiGsELS6QVpVUQHDQXd
…hToTarget Replace the hardcoded created_by comparison with the attach_<model> object permission (FilesPermissionsBackend grants owners; superusers pass via has_perm), resolve the enabled flag through the files_metadata service instead of get_file_target() (which created rows as a side effect and skipped non-FileableModel targets), and reuse DocumentId.get_or_create_for_object. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0146oeiGsELS6QVpVUQHDQXd
…e selection - Mark resumable uploads explicitly not planned (in-session pause/resume only). - Docs showed parent_content_type/parent_object_id; the API takes parent_id (DocumentId public_id) — fix all request/response/client examples. - UPLOAD_STORAGE_HANDLER was documented but never read; document the actual default_storage sniffing in storage/__init__.py. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0146oeiGsELS6QVpVUQHDQXd
Flat-query assertions for FilesInterface (filesCount invariant to file volume, zero-files same budget), comments-with-files listing invariant to list size, and a single-SELECT test for FileTarget.annotate_queryset — mirrors the reactions/comments query-count test style. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0146oeiGsELS6QVpVUQHDQXd
Posts can now be commented (with files via the comments module); the commentable metadata annotation keeps post lists flat, mirroring reactable. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0146oeiGsELS6QVpVUQHDQXd
…nd path New tests: cleanup tasks (expired/failed uploads), local presigned token flow (happy path, tampered/expired/reused tokens, completed-file and bad part numbers), myFiles/file GraphQL queries, attach_files_from_relay_ids unit coverage. Package coverage now ~85%. Fixes surfaced by the new tests: - myFiles had no resolver: it returned every user's files and was listable anonymously. Now scoped to the authenticated user, empty for anonymous. - attach_files_from_relay_ids raised hashids' NoInstanceFound for relay ids whose DocumentId was deleted; now surfaces the intended not_found GraphQLError. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0146oeiGsELS6QVpVUQHDQXd
…query) - FileAttachToTarget: authorize the target — reject non-FileableModel targets (invalid_target) and delegate 'may attach to this object' to a new add_<model> permission (default: authenticated + files enabled) so projects can override it. Previously any authenticated user could attach files to any relay id, and a non-FilesInterface target committed writes then errored on serialization. - FileAttachToTarget: require each file_relay_id to resolve to a File (isinstance) — a non-File relay id (e.g. a Comment) previously hit an uncaught AttributeError/500 in the permission backend. - FileAttachToTarget: materialize the target DocumentId only after validation, so rejected attaches don't create-then-rollback a row. - attach_files_from_relay_ids: a NULL creator is no longer an implicit grant — fixes a file-stealing path that diverged from the mutation's owner-only rule. - resolve_my_files: order by (-created, -pk) — cursor pagination over an unordered queryset duplicated/dropped rows. - AbstractFileTarget: inherit DocumentIdUniqueTargetMixin instead of copying get_for_object + the target field; get_or_create_file_target now returns None for unsaved objects (was IntegrityError). - annotate_queryset: drop the unused files_count_total subquery (no consumer; every comments list paid for it) — keep it available to add when a filter needs it. Tests added for each: non-File / non-fileable-target rejection, NULL-creator steal, unsaved-object guard. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0146oeiGsELS6QVpVUQHDQXd
- permissions.py: reduce FilesPermissionsBackend.has_perm cognitive complexity 36->well under 15 (S3776) by extracting _can_add_to_target and _is_owner_or_has_global; behavior unchanged. - cleanup.py: logger.exception() with a lazy %-arg instead of logger.error on an f-string in an except block (S8572). - storage base/local/s3: get_file_url annotated Optional[str] since it returns None when no file is stored (S5886). - storage/local.py: drop the unused file_obj param from upload_part (S1172) — a local-only method (S3 uploads go direct); updated the one caller and test. Left intentionally: the S117 'rename File/FileTarget' hits are module-level swapper.load_model() model aliases (PascalCase is correct; the whole codebase uses this idiom), and the S6553 null=True flags are on already-deployed AbstractFile string fields where dropping null=True is a schema/data-migration decision, not a lint fix. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0146oeiGsELS6QVpVUQHDQXd
String fields now use blank=True + default="" instead of null=True, removing
the NULL-vs-"" ambiguity Sonar flags. upload_id follows suit ("" means no
active multipart upload); its three None assignments and truthiness checks
already work with "". testproject migration converts any existing NULLs to
"" before enforcing NOT NULL, so it is safe on populated databases too.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0146oeiGsELS6QVpVUQHDQXd
Dead logging/pdb.set_trace comment lines left in the multipart parse_body. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0146oeiGsELS6QVpVUQHDQXd
|
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:
WalkthroughAdds a swappable file platform with S3 and local multipart uploads, REST and GraphQL APIs, file metadata and permissions, cleanup services, admin integration, project wiring, migrations, and tests. Existing GraphQL image and thumbnail selections now return direct URL strings. ChangesFiles platform
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🧪 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a new baseapp.files Django package to support attaching files to any DocumentId-backed object, including multipart uploads (S3 in prod, local-token “presigned” fallback in dev/test) and dual API exposure via GraphQL (FilesInterface) and REST. It also updates shared GraphQL thumbnail/file field behavior and integrates files support into comments/content feed object types and query-optimization hooks.
Changes:
- Add
baseapp.filespackage: swappable models, pgtrigger-maintained counts, upload handlers, shared metadata service, REST + GraphQL APIs, docs, and tests. - Wire the plugin into the project (settings + plugin registry) and add testproject concrete models/migrations.
- Update shared GraphQL thumbnail/file field behavior and adjust dependent GraphQL queries/tests accordingly; add FilesInterface integration to comments and content feed.
Reviewed changes
Copilot reviewed 64 out of 72 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| testproject/settings.py | Enable baseapp.files + testproject concrete app; configure swappable model settings and auth backend plugin hook. |
| testproject/files/apps.py | Define testproject files AppConfig. |
| testproject/files/models.py | Add concrete swappable File / FileTarget models for testproject. |
| testproject/files/migrations/0001_initial.py | Initial schema for testproject files models + triggers. |
| testproject/files/migrations/0002_alter_file_description_alter_file_file_content_type_and_more.py | Data migration to convert NULL string fields to "" before enforcing NOT NULL. |
| testproject/files/migrations/init.py | Package marker for migrations. |
| testproject/files/init.py | Package marker. |
| pyproject.toml | Register baseapp_files plugin entrypoint. |
| baseapp/files/apps.py | BaseApp plugin AppConfig: registers shared services + GraphQL shared interface. |
| baseapp/files/plugin.py | Plugin definition: backends, GraphQL contributors, REST router inclusion. |
| baseapp/files/models.py | Define abstract models, triggers, annotations, and validation behavior. |
| baseapp/files/base.py | FileableModel mixin for attaching files to target objects. |
| baseapp/files/utils.py | Helper utilities for file targets, recounting, and re-parenting. |
| baseapp/files/permissions.py | Permission backend for file ownership and target-level attach authorization. |
| baseapp/files/services/init.py | Export upload/metadata/cleanup services. |
| baseapp/files/services/upload_service.py | Central upload business logic (initiate/complete/abort + validation). |
| baseapp/files/services/metadata.py | Shared-service provider for files count/enabled metadata + queryset annotation. |
| baseapp/files/services/cleanup.py | Celery cleanup task + helper for expired/failed uploads. |
| baseapp/files/storage/base.py | Abstract upload handler interface. |
| baseapp/files/storage/init.py | Handler factory selecting S3 vs local behavior. |
| baseapp/files/storage/s3.py | S3 multipart handler (presigned upload_part URLs, complete, abort). |
| baseapp/files/storage/local.py | Local dev/test handler (tokenized “presigned” backend upload-part endpoint). |
| baseapp/files/rest_framework/routers.py | Register REST routes for files + uploads + presigned uploads. |
| baseapp/files/rest_framework/files/views.py | REST CRUD viewset + set-parent endpoint. |
| baseapp/files/rest_framework/files/serializers.py | REST serializer for file representation (public_id, url, parent_id, etc.). |
| baseapp/files/rest_framework/uploads/views.py | REST endpoints for initiate/complete/abort multipart uploads. |
| baseapp/files/rest_framework/uploads/serializers.py | REST serializers for upload initiation/completion and set-parent validation. |
| baseapp/files/rest_framework/uploads/presigned_views.py | Token-authenticated local upload-part endpoint for “presigned” local uploads. |
| baseapp/files/rest_framework/uploads/permissions.py | Owner-based DRF permissions helpers for file access/mutation. |
| baseapp/files/rest_framework/init.py | Package marker. |
| baseapp/files/rest_framework/files/init.py | Package marker. |
| baseapp/files/rest_framework/uploads/init.py | Package marker. |
| baseapp/files/graphql/interfaces.py | Define FilesInterface GraphQL interface and resolvers. |
| baseapp/files/graphql/object_types.py | GraphQL object type + filterset for File. |
| baseapp/files/graphql/queries.py | GraphQL queries (myFiles, file). |
| baseapp/files/graphql/mutations.py | GraphQL mutations for attaching and deleting files. |
| baseapp/files/graphql/utils.py | Helper for attaching files from relay IDs with ownership checks. |
| baseapp/files/graphql/init.py | Package marker. |
| baseapp/files/README.md | Detailed REST upload documentation and integration notes. |
| baseapp/files/UPLOAD_FLOW.md | REST upload flow documentation for S3 + local fallback. |
| baseapp/files/admin.py | Admin registrations for File/FileTarget. |
| baseapp/files/init.py | Package marker. |
| baseapp/files/tests/conftest.py | Test fixtures imports for baseapp.files tests. |
| baseapp/files/tests/test_storage.py | Unit tests for storage handlers + factory behavior. |
| baseapp/files/tests/test_rest_api.py | REST API integration tests for upload + CRUD behavior. |
| baseapp/files/tests/test_presigned_uploads.py | Integration tests for local presigned upload-part token flow. |
| baseapp/files/tests/test_models.py | Model + trigger/count behavior tests for File/FileTarget utilities. |
| baseapp/files/tests/test_graphql.py | GraphQL API tests for FilesInterface, attach/delete, myFiles, etc. |
| baseapp/files/tests/test_graphql_utils.py | Unit tests for attach_files_from_relay_ids. |
| baseapp/files/tests/test_graphql_queries_object_files.py | Query-count tests to lock in flat-query behavior for files metadata. |
| baseapp/files/tests/test_cleanup.py | Tests for cleanup task/helpers for expired/failed uploads. |
| baseapp/files/tests/init.py | Package marker. |
| baseapp/content_feed/graphql/object_types.py | Ensure commentable metadata annotations are applied for content feed posts. |
| baseapp/content_feed/tests/test_graphql_mutations_create.py | Update query expectations for thumbnail/image field shape. |
| baseapp/activity_log/tests/test_graphql_mutations.py | Update query expectations for thumbnail/image field shape. |
| baseapp_profiles/tests/test_graphql_mutations_update.py | Update query expectations/assertions for thumbnail/image field shape. |
| baseapp_profiles/tests/test_get_queries.py | Update query expectations/assertions for thumbnail/image field shape. |
| baseapp_profiles/tests/integration/test_profiles_queries_without_baseapp_pages.py | Update query expectations for thumbnail/image field shape. |
| baseapp_chats/tests/test_graphql_subscriptions.py | Update subscription query/assertions for thumbnail/image field shape. |
| baseapp_chats/tests/test_graphql_queries.py | Update query expectations/assertions for thumbnail/image field shape. |
| baseapp_chats/tests/test_graphql_mutations.py | Update query expectations/assertions for thumbnail/image field shape. |
| baseapp_auth/tests/integration/test_auth_without_baseapp_profiles.py | Update query expectations for avatar field shape. |
| baseapp_comments/models.py | Add FileableModel inheritance when baseapp.files is installed. |
| baseapp_comments/graphql/object_types.py | Include FilesInterface + apply files metadata annotation in optimizer hook. |
| baseapp_comments/tests/test_graphql_mutations_update.py | Extend mutation response shape to include attached files edges. |
| baseapp_profiles/rest_framework/mixins.py | Add CurrentProfileMixin for DRF, matching middleware behavior. |
| baseapp_profiles/rest_framework/init.py | Export CurrentProfileMixin. |
| baseapp_profiles/rest_framework/README.md | Document DRF CurrentProfileMixin behavior and usage. |
| baseapp_core/graphql/views.py | Add multipart/form-data body parsing for GraphQL file upload spec. |
| baseapp_core/graphql/fields.py | Change thumbnail field return shape; introduce FileInterface + helpers. |
| baseapp_core/graphql/init.py | Update public exports for GraphQL file/thumbnail field utilities. |
| baseapp_organizations/tests/test_graphql_mutations_create.py | Update query expectations for thumbnail/image field shape. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Copilot / CodeQL review on PR #442: - REST initiate (parent_id) and files set-parent now enforce the same target authorization as fileAttachToTarget (FileableModel + add_<file> permission) via a shared enforce_can_attach_to_parent helper — previously these could attach files to arbitrary objects, bypassing the mutation's checks. Tests updated to use a files-enabled Comment target + new tests asserting non-fileable targets are rejected. - Stop leaking exception text to clients: the generic except blocks in the upload views now logger.exception() and return a generic message (CodeQL 'information exposure through an exception'). - presigned upload_part: bind the signed token to the file's current upload_id (block reuse of a token from a prior initiation) and return a clear error when the active storage handler has no upload_part (S3). - upload_service._validate_file_params: enforce the upper file-size bound too, mirroring InitiateUploadSerializer, so non-REST callers are protected. - recalculate_files_count: Count('id') (not the nullable content type) and bucket NULL content types under 'unknown', matching the pgtrigger. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0146oeiGsELS6QVpVUQHDQXd
Member
Author
|
Thanks for the thorough review — addressed in Security / authorization
Correctness / robustness
Files suite green (120 passed) and lint clean on a fresh DB. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
baseapp_core/graphql/fields.py (1)
64-80: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winCached
Nonevalues are never served from cache due to truthy check.When
NoSourceGeneratoris caught,absolute_urlis set toNoneand stored viacache.set(cache_key, None, ...). However, the retrieval checkif value_from_cache:(line 67) is falsy forNone, so every subsequent request for the same failed thumbnail re-attempts generation instead of returning the cachedNone. This wastes CPU on images that consistently fail to generate thumbnails.⚡ Proposed fix using a sentinel to distinguish "cached None" from "cache miss"
class ThumbnailField(graphene.Field): + _CACHE_MISS = object() + def __init__(self, type=graphene.String, **kwargs): kwargs.update( { "args": { "width": graphene.Argument(graphene.Int, required=True), "height": graphene.Argument(graphene.Int, required=True), } } ) super(ThumbnailField, self).__init__(type, **kwargs) def get_resolver(self, parent_resolver): resolver = self.resolver or parent_resolver def built_thumbnail(instance, info, width, height, **kwargs): instance = resolver(instance, info, **kwargs) if not instance: return None if cache: cache_key = self._get_cache_key(instance, width, height) - value_from_cache = cache.get(cache_key) - if value_from_cache: + value_from_cache = cache.get(cache_key, default=self._CACHE_MISS) + if value_from_cache is not self._CACHE_MISS: return value_from_cache thumbnailer = get_thumbnailer(instance) try: url = thumbnailer.get_thumbnail({"size": (width, height)}).url absolute_url = info.context.build_absolute_uri(url) except NoSourceGenerator: absolute_url = None if cache: cache.set(cache_key, absolute_url, timeout=None) return absolute_url return built_thumbnail🤖 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 `@baseapp_core/graphql/fields.py` around lines 64 - 80, Update the cache lookup in the thumbnail resolution flow around _get_cache_key and cache.get so it distinguishes a cache miss from a cached None value, using an appropriate sentinel or the cache API’s existence check. Return the cached value, including None, when the key is present, while preserving thumbnail generation and cache.set behavior for genuine misses.
🧹 Nitpick comments (16)
baseapp/files/storage/base.py (1)
14-49: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAnnotate the
file_objparameter across the abstract methods.
file_objis untyped ininitiate_upload,complete_upload,abort_upload, andget_file_url. Since this is the contract implemented by both handlers, a concrete type (e.g., the swappableFilemodel or aTYPE_CHECKINGimport) would document and enforce the interface.As per coding guidelines: "Type-annotate all function parameters and return values in Python code".
🤖 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 `@baseapp/files/storage/base.py` around lines 14 - 49, Annotate the file_obj parameter in the abstract methods initiate_upload, complete_upload, abort_upload, and get_file_url with the shared concrete File model type, using a TYPE_CHECKING import if needed to avoid runtime coupling; preserve their existing return annotations and abstract interface.Source: Coding guidelines
baseapp/files/rest_framework/uploads/presigned_views.py (1)
134-134: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winNarrow
Access-Control-Expose-Headersto onlyETag.
"*"exposes every response header (includingServer,X-Frame-Options, internal headers) to any cross-origin client. OnlyETagneeds to be readable by the uploader.♻️ Proposed fix
- response["Access-Control-Expose-Headers"] = "*" + response["Access-Control-Expose-Headers"] = "ETag"🤖 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 `@baseapp/files/rest_framework/uploads/presigned_views.py` at line 134, Update the CORS response header assignment in the presigned upload view to expose only ETag, replacing the wildcard value while leaving the surrounding response behavior unchanged.baseapp/files/rest_framework/files/views.py (1)
93-94: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUse
update_fieldswhen saving only the parent.
file_obj.save()persists the entire model instance, which can trigger unnecessary signals, re-process file fields, and overwrite concurrent changes. Since onlyparentchanged, scope the save.♻️ Proposed fix
file_obj.parent_id = serializer.validated_data["parent_id"] - file_obj.save() + file_obj.save(update_fields=["parent"])🤖 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 `@baseapp/files/rest_framework/files/views.py` around lines 93 - 94, Update the save call in the parent-update flow to use update_fields scoped to the parent foreign-key field, while preserving the assignment to file_obj.parent_id and avoiding a full-model save.baseapp/files/rest_framework/files/serializers.py (1)
50-72: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd type annotations to serializer getter methods.
Per coding guidelines, all function parameters and return values in Python code must be type-annotated. The
get_id,get_url,get_relay_id, andget_parent_idmethods lack annotations.♻️ Proposed type annotations
- def get_id(self, obj): + def get_id(self, obj: File) -> str: """Get public_id instead of primary key.""" return obj.public_id - def get_url(self, obj): + def get_url(self, obj: File) -> str | None: """Get file URL if upload completed.""" if obj.upload_status == "completed" and obj.file: request = self.context.get("request") url = obj.file.url if request: return request.build_absolute_uri(url) return url return None - def get_relay_id(self, obj): + def get_relay_id(self, obj: File) -> str | None: """Get GraphQL relay ID for the file.""" return obj.relay_id - def get_parent_id(self, obj): + def get_parent_id(self, obj: File) -> str | None: """Get parent DocumentId public_id.""" if obj.parent: return obj.parent.public_id return None🤖 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 `@baseapp/files/rest_framework/files/serializers.py` around lines 50 - 72, Add parameter and return type annotations to the serializer getter methods get_id, get_url, get_relay_id, and get_parent_id. Use the appropriate model/object type for obj and accurately reflect each method’s return value, including the optional URL and parent ID results.Source: Coding guidelines
baseapp/files/rest_framework/uploads/views.py (1)
118-122: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the
complete/destroychecks into object permissions.get_object()already callscheck_object_permissions(), butbaseapp/files/rest_framework/uploads/permissions.pyonly coversviewandchange; add a delete-aware permission class (or extendIsOwnerOrReadOnly) and wire it intopermission_classesso these inline 403 responses can go away.🤖 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 `@baseapp/files/rest_framework/uploads/views.py` around lines 118 - 122, Move the upload completion and deletion authorization from inline checks in the upload views into object-level permissions. Update the permission classes in permissions.py to cover delete access alongside existing view/change rules, then wire the appropriate permission class into the views’ permission_classes so get_object().check_object_permissions() enforces both complete and destroy operations and the inline 403 responses can be removed.Source: Coding guidelines
baseapp/files/graphql/mutations.py (2)
165-174: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd
isinstance(obj, File)guard inFileDeletefor defense-in-depth.
FileDeleteresolves the relay ID to any object viaget_obj_from_relay_id, then checkshas_permand callsobj.delete(). Without anisinstance(obj, File)check, a misconfigured permission backend that grantsdelete_filefor a non-File object would result in deleting that object.FileAttachToTarget(Line 106) already has this guard —FileDeleteshould be consistent.🔒 Proposed defense-in-depth fix
obj = get_obj_from_relay_id(info, relay_id) if not obj: raise error_exception + if not isinstance(obj, File): + raise error_exception + if not info.context.user.has_perm(f"{app_label}.delete_{file_model_name}", obj): raise error_exception🤖 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 `@baseapp/files/graphql/mutations.py` around lines 165 - 174, Add an isinstance(obj, File) validation in FileDelete immediately after resolving the relay ID and before the permission check or deletion, raising the existing error_exception for non-File objects. Keep the existing permission and obj.delete() flow unchanged for valid File instances.
104-128: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider bulk-fetching files to avoid N+1 queries in the validation loop.
Each iteration calls
get_obj_from_relay_id(info, file_relay_id), which issues a separate database query per file. For N files this results in N queries. Theattach_files_from_relay_idsutility inutils.py(Lines 19-26) resolves all relay IDs to PKs first, then does a singleFile.objects.filter(pk__in=file_pks)bulk fetch. The mutation could adopt the same approach — resolve PKs in bulk, fetch all files in one query, then iterate for permission and already-attached checks.♻️ Sketch of bulk-fetch approach
# Get all files and verify each is a File the user is allowed to attach. - files = [] - for file_relay_id in file_relay_ids: - file_obj = get_obj_from_relay_id(info, file_relay_id) - if not isinstance(file_obj, File): - raise GraphQLError( - str(_("File not found: {file_id}")).format(file_id=file_relay_id), - extensions={"code": "not_found"}, - ) - - # Object-level permission (FilesPermissionsBackend grants the owner) - if not info.context.user.has_perm(f"{app_label}.attach_{file_model_name}", file_obj): - raise GraphQLError( - str(_("You don't have permission to attach this file")), - extensions={"code": "permission_required"}, - ) - - # Check if file is already attached to another parent - if file_obj.parent_id: - raise GraphQLError( - str(_("File {file_name} is already attached to another object")).format( - file_name=file_obj.file_name - ), - extensions={"code": "already_attached"}, - ) - - files.append(file_obj) + from baseapp_core.graphql import get_pk_from_relay_id + + try: + file_pks = [get_pk_from_relay_id(fid) for fid in file_relay_ids] + except Exception: + raise GraphQLError( + str(_("One or more files could not be found")), + extensions={"code": "not_found"}, + ) from None + + files = list(File.objects.filter(pk__in=file_pks)) + found_pks = {f.pk for f in files} + if len(found_pks) != len(set(file_pks)): + raise GraphQLError( + str(_("One or more files could not be found")), + extensions={"code": "not_found"}, + ) + + # Preserve input order + files_by_pk = {f.pk: f for f in files} + files = [files_by_pk[pk] for pk in file_pks if pk in files_by_pk] + + for file_obj in files: + # Object-level permission (FilesPermissionsBackend grants the owner) + if not info.context.user.has_perm(f"{app_label}.attach_{file_model_name}", file_obj): + raise GraphQLError( + str(_("You don't have permission to attach this file")), + extensions={"code": "permission_required"}, + ) + + # Check if file is already attached to another parent + if file_obj.parent_id: + raise GraphQLError( + str(_("File {file_name} is already attached to another object")).format( + file_name=file_obj.file_name + ), + extensions={"code": "already_attached"}, + )🤖 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 `@baseapp/files/graphql/mutations.py` around lines 104 - 128, Update the file lookup in the mutation’s validation loop to resolve all file relay IDs to primary keys and bulk-fetch the corresponding File objects before iterating. Preserve the existing not-found, permission, already-attached, and files.append validation behavior, using the established attach_files_from_relay_ids approach or its bulk ID-resolution helpers.baseapp/files/utils.py (2)
23-37: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd type annotations and docstring to
recalculate_files_count.Per coding guidelines, all function parameters and return values should be type-annotated, and non-trivial functions should have docstrings.
♻️ Proposed refactor
-def recalculate_files_count(parent): +def recalculate_files_count(parent) -> None: + """Recalculate and persist the per-content-type files count for `parent`.""" if not parent:🤖 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 `@baseapp/files/utils.py` around lines 23 - 37, Add a type annotation for the parent parameter and the function’s None return type in recalculate_files_count, and add a concise docstring describing that it recalculates and persists the parent’s file counts. Keep the existing counting and save behavior unchanged.Source: Coding guidelines
40-66: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd type annotations and docstring to
set_files_parent.
parentlacks a type annotation and the function has no docstring.♻️ Proposed refactor
-def set_files_parent(parent, files: Iterable): +def set_files_parent(parent, files: Iterable) -> None: + """Reassign `files` to `parent`, recalculating counts for any previous parents.""" if not files:🤖 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 `@baseapp/files/utils.py` around lines 40 - 66, Update set_files_parent with an appropriate type annotation for parent and add a concise docstring describing that it assigns the provided files to the parent and recalculates affected previous parents. Preserve the existing Iterable annotation and implementation behavior.Source: Coding guidelines
baseapp/files/services/cleanup.py (2)
39-39: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUse
update_fieldswhen saving in the cleanup loop.
file_obj.save()persists all fields, including the potentially largeuploaded_partsJSONField. Since onlyupload_statusandupload_idare modified, restrict the save.♻️ Proposed fix
- file_obj.save() + file_obj.save(update_fields=["upload_status", "upload_id"])🤖 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 `@baseapp/files/services/cleanup.py` at line 39, Update the cleanup loop’s file_obj.save() call to use update_fields limited to upload_status and upload_id, so saving does not persist the unchanged uploaded_parts JSONField.
50-50: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd type annotation to
cleanup_failed_uploads.Per coding guidelines, all function parameters should be type-annotated.
♻️ Proposed fix
-def cleanup_failed_uploads(days_old=7): +def cleanup_failed_uploads(days_old: int = 7) -> str:🤖 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 `@baseapp/files/services/cleanup.py` at line 50, Add a type annotation to the days_old parameter in cleanup_failed_uploads, using the appropriate numeric type for the function’s expected day-count value while preserving its default of 7.Source: Coding guidelines
baseapp/files/models.py (1)
87-150: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUpdate trigger recalculates counts on every UPDATE, even for unrelated field changes.
The
file_target_update_triggerfires on every UPDATE and always recalculates the new parent's counts (lines 124-145) whenNEW.parent_id IS NOT NULL, regardless of whetherparent_idorfile_content_typeactually changed. For frequent status-only updates (e.g.,upload_statustransitions), this triggers unnecessary count recalculation queries.Consider adding a
pgtrigger.Conditionor aWHENclause to only fire whenparent_idorfile_content_typechanges.🤖 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 `@baseapp/files/models.py` around lines 87 - 150, Restrict file_target_update_trigger to updates where parent_id or file_content_type changes, using the trigger’s pgtrigger.Condition or WHEN configuration. Preserve recalculation for parent changes, including old and new parents, while skipping the trigger entirely for unrelated updates such as upload_status changes.baseapp/files/README.md (2)
32-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSpecify language for fenced code blocks.
The architecture diagram code block on line 32 has no language specified. Static analysis flags this as MD040. Adding a language (e.g.,
text) improves syntax highlighting consistency.📝 Proposed fix
-``` +```text Client → Backend (initiate) → S3 Presigned URLs🤖 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 `@baseapp/files/README.md` at line 32, Update the architecture diagram fenced code block in README.md to specify a language such as text, while preserving its diagram content unchanged.
176-177: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueResponse example shows integer ID instead of UUID.
The
"id": 456in the response example is inconsistent with the actual API, which returns a UUIDpublic_id(pertest_rest_api.pyline 67:File.get_by_public_id(data["id"])). This could confuse API consumers.📝 Proposed fix
- "id": 456, + "id": "b3c5d7e8-...",🤖 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 `@baseapp/files/README.md` around lines 176 - 177, The README response example should use a UUID-formatted public ID instead of the integer value 456. Update the example’s id field to a representative UUID while preserving the surrounding response structure and the API’s existing public_id contract.baseapp/files/tests/test_rest_api.py (1)
99-102: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove debug print statements from test.
The
🧹 Proposed cleanup
# Print response for debugging if it fails - if response.status_code != status.HTTP_201_CREATED: - print(f"Response status: {response.status_code}") - print(f"Response data: {response.json()}") - assert response.status_code == status.HTTP_201_CREATED🤖 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 `@baseapp/files/tests/test_rest_api.py` around lines 99 - 102, Remove the response.status_code and response.json debug print statements from the test’s failure-handling block, including the surrounding debug-only conditional if it becomes unused. Leave the test’s existing assertion and behavior unchanged.baseapp/files/UPLOAD_FLOW.md (1)
74-76: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSpecify language for fenced code block.
The response headers code block on line 74 has no language specified. Static analysis flags this as MD040.
📝 Proposed fix
-``` +```text ETag: abc123def456...🤖 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 `@baseapp/files/UPLOAD_FLOW.md` around lines 74 - 76, Update the fenced code block containing the ETag response header in UPLOAD_FLOW.md to specify the text language, using the existing block content 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 `@baseapp_profiles/rest_framework/mixins.py`:
- Line 41: Update the request user profile assignment to safely handle users
without an associated profile by retrieving the profile through getattr with an
appropriate fallback instead of directly accessing request.user.profile.
Preserve assigning the resolved value to request.user.current_profile.
In `@baseapp/files/graphql/interfaces.py`:
- Around line 39-42: Update the queryset returned by resolve_files to order
files by descending created timestamp and descending pk, matching the stable
cursor-pagination ordering used in queries.py.
In `@baseapp/files/models.py`:
- Around line 300-306: Add gettext_lazy help_text values to the
file_content_type, file_name, and name CharFields in the model, following the
existing field translation conventions. Leave their current types, defaults, and
other options unchanged.
In `@baseapp/files/permissions.py`:
- Around line 9-48: Update FilesPermissionsBackend.has_perm with type
annotations for user_obj, perm, obj, and its boolean return value, using the
project’s established types or compatible optional typing for obj. Add a concise
docstring describing its permission-dispatch behavior while preserving the
existing permission checks and return results.
In `@baseapp/files/README.md`:
- Line 474: Correct the JavaScript example around uploader.uploadFile so it
passes only the arguments supported by the uploadFile(file, parentId = null)
signature. Remove the leftover extra argument and ensure the remaining parentId
value matches the example’s intended API usage.
In `@baseapp/files/rest_framework/files/views.py`:
- Around line 59-61: Update the status_filter condition in the view’s query
filtering logic to check whether the value is not None rather than relying on
truthiness. Preserve the default completed status when the parameter is absent,
while ensuring an explicitly empty ?status= value still applies the
upload_status filter.
In `@baseapp/files/rest_framework/uploads/presigned_views.py`:
- Around line 138-141: Update the exception handlers in the upload-part flow to
log the caught exceptions server-side, while returning generic ValidationError
messages that do not include str(e) or other internal details. Preserve distinct
handling for ValueError and unexpected exceptions, and use the module’s existing
logger or logging setup if available.
In `@baseapp/files/rest_framework/uploads/serializers.py`:
- Line 30: Update all ValidationError raises in validate_parent_id and
validate_parts to wrap their messages in arrays, and update the cross-field
validate method to raise {"non_field_errors": [...]} with its message. Apply the
same formatting consistently to every referenced raise site without changing the
validation behavior.
In `@baseapp/files/rest_framework/uploads/views.py`:
- Around line 96-99: Update the exception handlers in the upload view, including
the paths around the shown handlers and the occurrences at the referenced
locations, to log caught exceptions with the view’s existing logger and raise
only generic client-facing ValidationError messages without str(e). Preserve any
distinct handling for expected ValueError cases, while ensuring broad Exception
handlers log the original exception before returning the generic error.
In `@baseapp/files/services/cleanup.py`:
- Line 10: Move the module-level File model loading into the existing
_get_file_model() helper, and update cleanup_failed_uploads to obtain the model
through _get_file_model() before using it. Ensure importing the services module
performs no swapper.load_model call while preserving the existing cleanup
behavior.
In `@baseapp/files/services/upload_service.py`:
- Around line 19-79: Refactor initiate_multipart_upload so the File record is
committed before calling handler.initiate_upload, rather than performing
external storage I/O inside `@transaction.atomic`. After the remote call,
reconcile upload_id and upload_status in a separate short transaction; on
initiation failure, mark or remove the persisted record according to the
service’s existing cleanup contract, while preserving consistent state if the
follow-up database update fails.
- Around line 124-129: Update the exception handler around handler.abort_upload
in the upload cleanup flow to log the caught exception with traceback details,
while preserving the existing behavior of continuing to mark the upload as
aborted. Use the service’s existing logger and avoid silently swallowing the
exception.
- Around line 81-115: Update complete_multipart_upload so the FAILED status is
persisted in a separate transaction after the atomic completion transaction
rolls back, ensuring cleanup_failed_uploads can detect it. Keep the existing
exception propagation and success-path updates unchanged, and avoid saving
FAILED within the rolled-back transaction.
In `@baseapp/files/storage/__init__.py`:
- Around line 4-18: Update get_upload_handler to inspect the resolved storage
backend class from default_storage rather than type(default_storage), so
S3Boto3Storage is detected correctly; add the function’s return type annotation
and adjust the test to configure/assert the resolved backend path instead of
changing the mock instance’s class name.
In `@baseapp/files/storage/local.py`:
- Around line 85-87: Update the ETag calculation in the local storage method to
call hashlib.md5 with usedforsecurity=False, preserving the existing data input
and hexadecimal digest return while documenting that the hash is used only as a
content checksum.
- Around line 103-114: Update complete_upload’s part assembly loop to fail
immediately when any expected part file is missing instead of skipping it.
Preserve the existing sorted part order, but raise an appropriate error for an
absent part_file so no truncated final file is returned as successful.
In `@baseapp/files/tests/test_storage.py`:
- Around line 246-280: Update both TestStorageFactory methods to mock the actual
default_storage type rather than assigning mock_storage.__class__.__name__.
Remove the unnecessary AWS skipif and no-op class-name assignments, ensuring
get_upload_handler() observes S3Boto3Storage for the S3 test and
FileSystemStorage for the local test so each factory branch is exercised.
---
Outside diff comments:
In `@baseapp_core/graphql/fields.py`:
- Around line 64-80: Update the cache lookup in the thumbnail resolution flow
around _get_cache_key and cache.get so it distinguishes a cache miss from a
cached None value, using an appropriate sentinel or the cache API’s existence
check. Return the cached value, including None, when the key is present, while
preserving thumbnail generation and cache.set behavior for genuine misses.
---
Nitpick comments:
In `@baseapp/files/graphql/mutations.py`:
- Around line 165-174: Add an isinstance(obj, File) validation in FileDelete
immediately after resolving the relay ID and before the permission check or
deletion, raising the existing error_exception for non-File objects. Keep the
existing permission and obj.delete() flow unchanged for valid File instances.
- Around line 104-128: Update the file lookup in the mutation’s validation loop
to resolve all file relay IDs to primary keys and bulk-fetch the corresponding
File objects before iterating. Preserve the existing not-found, permission,
already-attached, and files.append validation behavior, using the established
attach_files_from_relay_ids approach or its bulk ID-resolution helpers.
In `@baseapp/files/models.py`:
- Around line 87-150: Restrict file_target_update_trigger to updates where
parent_id or file_content_type changes, using the trigger’s pgtrigger.Condition
or WHEN configuration. Preserve recalculation for parent changes, including old
and new parents, while skipping the trigger entirely for unrelated updates such
as upload_status changes.
In `@baseapp/files/README.md`:
- Line 32: Update the architecture diagram fenced code block in README.md to
specify a language such as text, while preserving its diagram content unchanged.
- Around line 176-177: The README response example should use a UUID-formatted
public ID instead of the integer value 456. Update the example’s id field to a
representative UUID while preserving the surrounding response structure and the
API’s existing public_id contract.
In `@baseapp/files/rest_framework/files/serializers.py`:
- Around line 50-72: Add parameter and return type annotations to the serializer
getter methods get_id, get_url, get_relay_id, and get_parent_id. Use the
appropriate model/object type for obj and accurately reflect each method’s
return value, including the optional URL and parent ID results.
In `@baseapp/files/rest_framework/files/views.py`:
- Around line 93-94: Update the save call in the parent-update flow to use
update_fields scoped to the parent foreign-key field, while preserving the
assignment to file_obj.parent_id and avoiding a full-model save.
In `@baseapp/files/rest_framework/uploads/presigned_views.py`:
- Line 134: Update the CORS response header assignment in the presigned upload
view to expose only ETag, replacing the wildcard value while leaving the
surrounding response behavior unchanged.
In `@baseapp/files/rest_framework/uploads/views.py`:
- Around line 118-122: Move the upload completion and deletion authorization
from inline checks in the upload views into object-level permissions. Update the
permission classes in permissions.py to cover delete access alongside existing
view/change rules, then wire the appropriate permission class into the views’
permission_classes so get_object().check_object_permissions() enforces both
complete and destroy operations and the inline 403 responses can be removed.
In `@baseapp/files/services/cleanup.py`:
- Line 39: Update the cleanup loop’s file_obj.save() call to use update_fields
limited to upload_status and upload_id, so saving does not persist the unchanged
uploaded_parts JSONField.
- Line 50: Add a type annotation to the days_old parameter in
cleanup_failed_uploads, using the appropriate numeric type for the function’s
expected day-count value while preserving its default of 7.
In `@baseapp/files/storage/base.py`:
- Around line 14-49: Annotate the file_obj parameter in the abstract methods
initiate_upload, complete_upload, abort_upload, and get_file_url with the shared
concrete File model type, using a TYPE_CHECKING import if needed to avoid
runtime coupling; preserve their existing return annotations and abstract
interface.
In `@baseapp/files/tests/test_rest_api.py`:
- Around line 99-102: Remove the response.status_code and response.json debug
print statements from the test’s failure-handling block, including the
surrounding debug-only conditional if it becomes unused. Leave the test’s
existing assertion and behavior unchanged.
In `@baseapp/files/UPLOAD_FLOW.md`:
- Around line 74-76: Update the fenced code block containing the ETag response
header in UPLOAD_FLOW.md to specify the text language, using the existing block
content unchanged.
In `@baseapp/files/utils.py`:
- Around line 23-37: Add a type annotation for the parent parameter and the
function’s None return type in recalculate_files_count, and add a concise
docstring describing that it recalculates and persists the parent’s file counts.
Keep the existing counting and save behavior unchanged.
- Around line 40-66: Update set_files_parent with an appropriate type annotation
for parent and add a concise docstring describing that it assigns the provided
files to the parent and recalculates affected previous parents. Preserve the
existing Iterable annotation and implementation behavior.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fd80a186-2348-405c-92a1-0d66f1c8814e
📒 Files selected for processing (72)
baseapp/activity_log/tests/test_graphql_mutations.pybaseapp/content_feed/graphql/object_types.pybaseapp/content_feed/tests/test_graphql_mutations_create.pybaseapp/files/README.mdbaseapp/files/UPLOAD_FLOW.mdbaseapp/files/__init__.pybaseapp/files/admin.pybaseapp/files/apps.pybaseapp/files/base.pybaseapp/files/graphql/__init__.pybaseapp/files/graphql/interfaces.pybaseapp/files/graphql/mutations.pybaseapp/files/graphql/object_types.pybaseapp/files/graphql/queries.pybaseapp/files/graphql/utils.pybaseapp/files/models.pybaseapp/files/permissions.pybaseapp/files/plugin.pybaseapp/files/rest_framework/__init__.pybaseapp/files/rest_framework/files/__init__.pybaseapp/files/rest_framework/files/serializers.pybaseapp/files/rest_framework/files/views.pybaseapp/files/rest_framework/routers.pybaseapp/files/rest_framework/uploads/__init__.pybaseapp/files/rest_framework/uploads/permissions.pybaseapp/files/rest_framework/uploads/presigned_views.pybaseapp/files/rest_framework/uploads/serializers.pybaseapp/files/rest_framework/uploads/views.pybaseapp/files/services/__init__.pybaseapp/files/services/cleanup.pybaseapp/files/services/metadata.pybaseapp/files/services/upload_service.pybaseapp/files/storage/__init__.pybaseapp/files/storage/base.pybaseapp/files/storage/local.pybaseapp/files/storage/s3.pybaseapp/files/tests/__init__.pybaseapp/files/tests/conftest.pybaseapp/files/tests/test_cleanup.pybaseapp/files/tests/test_graphql.pybaseapp/files/tests/test_graphql_queries_object_files.pybaseapp/files/tests/test_graphql_utils.pybaseapp/files/tests/test_models.pybaseapp/files/tests/test_presigned_uploads.pybaseapp/files/tests/test_rest_api.pybaseapp/files/tests/test_storage.pybaseapp/files/utils.pybaseapp_auth/tests/integration/test_auth_without_baseapp_profiles.pybaseapp_chats/tests/test_graphql_mutations.pybaseapp_chats/tests/test_graphql_queries.pybaseapp_chats/tests/test_graphql_subscriptions.pybaseapp_comments/graphql/object_types.pybaseapp_comments/models.pybaseapp_comments/tests/test_graphql_mutations_update.pybaseapp_core/graphql/__init__.pybaseapp_core/graphql/fields.pybaseapp_core/graphql/views.pybaseapp_organizations/tests/test_graphql_mutations_create.pybaseapp_profiles/rest_framework/README.mdbaseapp_profiles/rest_framework/__init__.pybaseapp_profiles/rest_framework/mixins.pybaseapp_profiles/tests/integration/test_profiles_queries_without_baseapp_pages.pybaseapp_profiles/tests/test_get_queries.pybaseapp_profiles/tests/test_graphql_mutations_update.pypyproject.tomltestproject/files/__init__.pytestproject/files/apps.pytestproject/files/migrations/0001_initial.pytestproject/files/migrations/0002_alter_file_description_alter_file_file_content_type_and_more.pytestproject/files/migrations/__init__.pytestproject/files/models.pytestproject/settings.py
- Storage backend detection: use default_storage.__class__ (LazyObject proxies it) instead of type(default_storage), which returned the wrapper and never matched S3 — S3 was never selected. - Genericize the remaining except ValueError responses in the upload views (log server-side, return a generic message) — clears the residual CodeQL 'information exposure' alerts on those lines. - Local complete_upload now raises on a missing part file instead of silently producing a truncated file. - MD5 ETag marked usedforsecurity=False (content checksum, not security). - cleanup tasks load the swappable model lazily (avoid AppRegistryNotReady on Celery startup); log the swallowed storage-abort failure in upload_service. - FilesInterface.resolve_files orders by (-created, -pk) for stable cursor pagination, matching my_files. - files list: empty ?status= no longer bypasses the upload_status filter. - DRF ValidationError messages wrapped in arrays / non_field_errors per guidelines; FilesPermissionsBackend.has_perm typed + documented. - Fix README multipart JS example to the single parent_id argument. Tests updated for the generic completion error; new tests already cover the non-fileable-target rejection. Files suite green (120), migrations clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0146oeiGsELS6QVpVUQHDQXd
Member
Author
|
Second review round addressed in Security / correctness
Stability / hygiene
Deliberately deferred (noted, not done here):
Files suite green (120 passed), migrations clean. |
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 `@baseapp/files/rest_framework/uploads/views.py`:
- Around line 107-112: Update the ValidationError raises in the upload view
handlers, including initiate, complete(), and destroy(), to wrap each message in
an array and add “from None” to the exception raises. Preserve the existing
messages and non-field error behavior while resolving the Ruff B904 warnings
consistently with serializers.py.
In `@baseapp/files/rest_framework/utils.py`:
- Line 24: Update the ValidationError raise in the DocumentId.DoesNotExist
exception handler to use explicit exception suppression with from None. Preserve
the existing “Invalid parent: target not found.” message and handling behavior.
- Line 13: Update enforce_can_attach_to_parent so both user and
parent_document_pk parameters have explicit type annotations matching the
project’s user model and parent-document primary-key type, while preserving its
existing None return annotation and behavior.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 76f7ce8a-ce56-4f62-93a9-51c5a3d5b7e4
📒 Files selected for processing (14)
baseapp/files/README.mdbaseapp/files/graphql/interfaces.pybaseapp/files/permissions.pybaseapp/files/rest_framework/files/views.pybaseapp/files/rest_framework/uploads/presigned_views.pybaseapp/files/rest_framework/uploads/serializers.pybaseapp/files/rest_framework/uploads/views.pybaseapp/files/rest_framework/utils.pybaseapp/files/services/cleanup.pybaseapp/files/services/upload_service.pybaseapp/files/storage/__init__.pybaseapp/files/storage/local.pybaseapp/files/tests/test_rest_api.pybaseapp/files/utils.py
✅ Files skipped from review due to trivial changes (1)
- baseapp/files/README.md
🚧 Files skipped from review as they are similar to previous changes (8)
- baseapp/files/storage/init.py
- baseapp/files/graphql/interfaces.py
- baseapp/files/services/cleanup.py
- baseapp/files/rest_framework/files/views.py
- baseapp/files/utils.py
- baseapp/files/permissions.py
- baseapp/files/services/upload_service.py
- baseapp/files/tests/test_rest_api.py
…helper Follow-up to CodeRabbit: the upload view except-blocks now raise ValidationError([...]) ... from None (array form per DRF guideline, and from None clears Ruff B904), and enforce_can_attach_to_parent gains parameter type annotations. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0146oeiGsELS6QVpVUQHDQXd
Member
Author
|
Round-3 nits addressed in |
Member
Author
|
✅ All checks green on |
…ages - upload_service: the storage handler's initiate/complete/abort network calls now run OUTSIDE the DB transaction — initiate creates the row, calls the handler, then updates; complete/abort validate first, do the remote call unlocked, then flip status under a short select_for_update guard (no row lock held across network I/O). Addresses the two 'Heavy lift' review notes. - Models: verbose_name + help_text (gettext_lazy) on every AbstractFile and AbstractFileTarget field + Meta verbose_name/plural (migration 0003 in testproject; state-only). - i18n: wrap all user-facing REST strings (view/serializer/permission/util errors, 403/404 messages) in gettext_lazy; GraphQL layer already did. Files suite green (120), migrations clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0146oeiGsELS6QVpVUQHDQXd
Member
Author
|
Picked up the deferred items in
Files suite green (120 submodule / 88 template), migrations clean. Left open only the |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
baseapp/files/rest_framework/uploads/serializers.py (1)
22-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd type annotations to serializer validator methods.
As per coding guidelines, all Python function parameters and return values must be type-annotated. The validator methods (
validate_parent_id,validate,validate_parts, andSetParentSerializer.validate_parent_id) lack parameter and return type annotations.♻️ Proposed refactor
- def validate_parent_id(self, value): + def validate_parent_id(self, value: uuid.UUID | None) -> int | None: """Validate parent_id exists as a DocumentId public_id."""- def validate(self, data): + def validate(self, data: dict) -> dict: """Validate file size matches parts."""- def validate_parts(self, value): + def validate_parts(self, value: list[dict]) -> list[dict]: """Validate parts structure."""- def validate_parent_id(self, value): + def validate_parent_id(self, value: uuid.UUID) -> int: """Validate parent_id exists as a DocumentId public_id."""Note: add
import uuidat the top if not already present, or use the appropriate type for UUID fields.Also applies to: 33-50, 74-86, 96-102
🤖 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 `@baseapp/files/rest_framework/uploads/serializers.py` around lines 22 - 31, Add parameter and return type annotations to the validator methods validate_parent_id, validate, validate_parts, and SetParentSerializer.validate_parent_id, using the appropriate UUID-related type for parent_id values and matching the existing serializer data and return conventions. Add the uuid import if required, without changing validation behavior.Source: Coding guidelines
🤖 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 `@baseapp/files/rest_framework/uploads/serializers.py`:
- Line 31: Update both ValidationError re-raises in the relevant serializer
exception handlers to explicitly use “from None”, including the cases around the
invalid parent_id message and the corresponding re-raise near line 102, so Ruff
B904 is satisfied and implicit exception chaining is suppressed.
In
`@testproject/files/migrations/0003_alter_file_options_alter_filetarget_options_and_more.py`:
- Around line 3-7: Reorder the imports in migration 0003 so Django and other
third-party imports appear before the first-party baseapp and baseapp_core
imports, with a blank line separating the sections. Apply the repository’s
configured isort Black profile and preserve all imported modules.
---
Nitpick comments:
In `@baseapp/files/rest_framework/uploads/serializers.py`:
- Around line 22-31: Add parameter and return type annotations to the validator
methods validate_parent_id, validate, validate_parts, and
SetParentSerializer.validate_parent_id, using the appropriate UUID-related type
for parent_id values and matching the existing serializer data and return
conventions. Add the uuid import if required, without changing validation
behavior.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 31ee2256-2630-4a17-870b-ad7a47be458d
📒 Files selected for processing (8)
baseapp/files/models.pybaseapp/files/rest_framework/files/views.pybaseapp/files/rest_framework/uploads/presigned_views.pybaseapp/files/rest_framework/uploads/serializers.pybaseapp/files/rest_framework/uploads/views.pybaseapp/files/rest_framework/utils.pybaseapp/files/services/upload_service.pytestproject/files/migrations/0003_alter_file_options_alter_filetarget_options_and_more.py
🚧 Files skipped from review as they are similar to previous changes (6)
- baseapp/files/rest_framework/utils.py
- baseapp/files/rest_framework/files/views.py
- baseapp/files/rest_framework/uploads/views.py
- baseapp/files/rest_framework/uploads/presigned_views.py
- baseapp/files/services/upload_service.py
- baseapp/files/models.py
…-wide) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0146oeiGsELS6QVpVUQHDQXd
Suppress implicit exception chaining on the two validate_parent_id DocumentId.DoesNotExist -> ValidationError re-raises. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0146oeiGsELS6QVpVUQHDQXd
Member
Author
|
Addressed in |
nossila
added a commit
that referenced
this pull request
Jul 25, 2026
Mirror baseapp.files (PR #442): module path baseapp.geo, app label stays baseapp_geo so swapper settings, backend slot keys and consumer migrations are unchanged. Entry point now baseapp_geo = "baseapp.geo.plugin:GeoPlugin"; AppConfig aligned with namespace convention (BigAutoField). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DDeemQ7iF4D3VhAwQVui9n
Hercilio1
requested changes
Jul 31, 2026
Comment on lines
+66
to
+96
| # Get and validate the signed token | ||
| token = request.query_params.get("token") | ||
| if not token: | ||
| return Response( | ||
| {"error": _("Missing token parameter")}, | ||
| status=status.HTTP_401_UNAUTHORIZED, | ||
| ) | ||
|
|
||
| try: | ||
| # Verify the token (max age: 1 hour) | ||
| token_data = signing.loads(token, max_age=3600) | ||
|
|
||
| # Validate token matches request | ||
| if str(token_data.get("file_id")) != str(pk) or str( | ||
| token_data.get("part_number") | ||
| ) != str(part_number): | ||
| return Response( | ||
| {"error": _("Invalid token for this file/part")}, | ||
| status=status.HTTP_401_UNAUTHORIZED, | ||
| ) | ||
|
|
||
| except signing.SignatureExpired: | ||
| return Response( | ||
| {"error": _("Token has expired")}, | ||
| status=status.HTTP_401_UNAUTHORIZED, | ||
| ) | ||
| except signing.BadSignature: | ||
| return Response( | ||
| {"error": _("Invalid token signature")}, | ||
| status=status.HTTP_401_UNAUTHORIZED, | ||
| ) |
Contributor
There was a problem hiding this comment.
We could isolate the token logic in a reusable class or utils file. What do you think?
Comment on lines
+8
to
+31
| class FileableModel(models.Model): | ||
| class Meta: | ||
| abstract = True | ||
|
|
||
| @property | ||
| def files(self) -> models.QuerySet: | ||
| """Returns files related to this object through the DocumentId join.""" | ||
| File = swapper.load_model("baseapp_files", "File") | ||
| content_type = ContentType.objects.get_for_model(self) | ||
| return File.objects.filter( | ||
| parent__content_type=content_type, | ||
| parent__object_id=self.pk, | ||
| ) | ||
|
|
||
| def get_file_target(self): | ||
| return get_or_create_file_target(self) | ||
|
|
||
| @property | ||
| def files_count(self) -> dict: | ||
| return self.get_file_target().files_count | ||
|
|
||
| @property | ||
| def is_files_enabled(self) -> bool: | ||
| return self.get_file_target().is_files_enabled |
Contributor
There was a problem hiding this comment.
Do we want to create this? The ideal plugin architecture would be to use the shared services directly where needed instead of creating an uncontrollable inheritance chain.
Comment on lines
+191
to
+218
| @receiver(class_prepared) | ||
| def add_file_target_triggers(sender, **kwargs): | ||
| """ | ||
| Add the FileTarget count triggers to the model when it is prepared. | ||
| This handles swappable models by adding triggers to the concrete model. | ||
| """ | ||
| # Only models that inherit from AbstractFile | ||
| if not issubclass(sender, AbstractFile): | ||
| return | ||
|
|
||
| # Skip non-schema models | ||
| if sender._meta.abstract or sender._meta.proxy: | ||
| return | ||
|
|
||
| # Skip swapped-out models | ||
| if sender._meta.swapped: | ||
| return | ||
|
|
||
| if not hasattr(sender._meta, "triggers"): | ||
| sender._meta.triggers = [] | ||
|
|
||
| existing = [t.name for t in sender._meta.triggers] | ||
| if "update_file_target_on_insert" not in existing: | ||
| sender._meta.triggers.append(file_target_insert_trigger()) | ||
| if "update_file_target_on_update" not in existing: | ||
| sender._meta.triggers.append(file_target_update_trigger()) | ||
| if "update_file_target_on_delete" not in existing: | ||
| sender._meta.triggers.append(file_target_delete_trigger()) |
Contributor
There was a problem hiding this comment.
Instead of doing like that, you can use our pgtrigger_register_default_track from baseapp_core for registering the triggers resolving the Swapped Model (take a look on how it's used in the baseapp_chats).
| class Meta: | ||
| abstract = True | ||
| verbose_name = _("file target") | ||
| verbose_name_plural = _("file targets") |
Contributor
There was a problem hiding this comment.
Lets add the swappable frag in here. And in the AbstractFile Meta;
Hercilio1
reviewed
Jul 31, 2026
Comment on lines
+302
to
+311
| class AbstractFile(*file_inheritances, DocumentIdMixin, RelayModel, TimeStampedModel): | ||
| parent = models.ForeignKey( | ||
| DocumentId, | ||
| null=True, | ||
| blank=True, | ||
| on_delete=models.CASCADE, | ||
| related_name="files", | ||
| verbose_name=_("parent"), | ||
| help_text=_("The parent document this file belongs to"), | ||
| ) |
Contributor
There was a problem hiding this comment.
On a social media app usually we don't need to worry about alt text, but in a CMS we would need to (for accessability). Do you think we should worry about it? And if so, retrieving a string in the graphql won't be enough
Replace the hand-rolled class_prepared receiver with the baseapp_core helper, matching baseapp_chats. Resolves the swap target with init_swapped_models so the triggers attach to the concrete table, and routes registration through pgtrigger.register (via apply_pgtrigger_tracks) instead of mutating _meta.triggers directly. Also add the swappable Meta frag to AbstractFile and AbstractFileTarget, so the concrete models inherit it like every other consumer model in the repo rather than redeclaring it. No migration: the three triggers were already in files/0001_initial and the swappable move is state-neutral (makemigrations --check is clean). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013LkamMd6XMPeYNHbAV1DZ9
The signed tokens that authorize part uploads on the local-storage fallback were minted in storage/local.py and verified inline in presigned_views.py, with the payload shape spelled out in both places (plus twice more in tests) and the 1h lifetime hardcoded three times. Move mint/verify into baseapp/files/tokens.py, which cuts ~30 lines of branching out of the view. Two behaviour changes fall out of centralising it: - Tokens are now salted, so a signed payload minted elsewhere in the project can no longer satisfy this endpoint. - Lifetime comes from FILE_UPLOAD_PRESIGNED_URL_EXPIRATION, the setting the S3 handler and README already use, instead of a bare 3600. The local handler's reported `expires_in` now follows it too. Tests mint through the class rather than duplicating the payload, and cover the previously untested upload-session mismatch branch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013LkamMd6XMPeYNHbAV1DZ9
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
baseapp/files/tests/test_presigned_uploads.py (2)
176-177: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd types for the new fixture parameters.
Annotate
clientandinitiated_uploadwith their concrete fixture types. Do not add a return annotation solely for this test.As per coding guidelines, Python function parameters require type annotations.
Based on learnings, test functions in this repository do not require return annotations.🤖 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 `@baseapp/files/tests/test_presigned_uploads.py` around lines 176 - 177, Annotate the client and initiated_upload parameters in test_token_from_previous_upload_session with their concrete fixture types used by the repository. Do not add a return annotation to this test function.Sources: Coding guidelines, Learnings
21-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove this database-dependent test module under the integration directory.
This module uses
File.objectsand REST endpoints. Move it tobaseapp/files/tests/integration/to match the test organization rule.As per coding guidelines, database-dependent tests must be organized under an integration test directory.
🤖 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 `@baseapp/files/tests/test_presigned_uploads.py` around lines 21 - 22, Move the test module containing PresignedUploadToken and the File.objects/REST endpoint tests into baseapp/files/tests/integration/. Preserve its existing test behavior and imports while relocating it to the integration test directory.Source: Coding guidelines
🤖 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 `@baseapp/files/models.py`:
- Around line 410-414: Annotate cls and the return type of
get_graphql_object_type in baseapp/files/models.py (lines 410-414), using the
appropriate class and FileObjectType-related types. Also annotate cls in
max_age, mint, and verify in baseapp/files/tokens.py (lines 35-53); no other
behavior changes are needed.
In `@baseapp/files/tests/test_presigned_uploads.py`:
- Around line 135-138: Update the expiry test around PresignedUploadToken.mint
to age the token by PresignedUploadToken.max_age() + 1 seconds instead of a
fixed two-hour value, ensuring it remains expired for the configured
FILE_UPLOAD_PRESIGNED_URL_EXPIRATION.
---
Nitpick comments:
In `@baseapp/files/tests/test_presigned_uploads.py`:
- Around line 176-177: Annotate the client and initiated_upload parameters in
test_token_from_previous_upload_session with their concrete fixture types used
by the repository. Do not add a return annotation to this test function.
- Around line 21-22: Move the test module containing PresignedUploadToken and
the File.objects/REST endpoint tests into baseapp/files/tests/integration/.
Preserve its existing test behavior and imports while relocating it to the
integration test directory.
🪄 Autofix
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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 09dd7b15-146d-4d6f-a5f5-639acc0d5458
📒 Files selected for processing (8)
baseapp/files/UPLOAD_FLOW.mdbaseapp/files/models.pybaseapp/files/rest_framework/uploads/presigned_views.pybaseapp/files/rest_framework/uploads/serializers.pybaseapp/files/storage/local.pybaseapp/files/tests/test_presigned_uploads.pybaseapp/files/tokens.pytestproject/files/models.py
🚧 Files skipped from review as they are similar to previous changes (3)
- baseapp/files/UPLOAD_FLOW.md
- baseapp/files/rest_framework/uploads/serializers.py
- baseapp/files/rest_framework/uploads/presigned_views.py
Comment on lines
+410
to
+414
| @classmethod | ||
| def get_graphql_object_type(cls): | ||
| from .graphql.object_types import FileObjectType | ||
|
|
||
| return FileObjectType |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add the required function type annotations.
Annotate every cls parameter and each missing return type.
baseapp/files/models.py#L410-L414: Annotateclsand theget_graphql_object_typereturn value.baseapp/files/tokens.py#L35-L53: Annotateclsinmax_age,mint, andverify.
As per coding guidelines, “Type-annotate all function parameters and return values.”
📍 Affects 2 files
baseapp/files/models.py#L410-L414(this comment)baseapp/files/tokens.py#L35-L53
🤖 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 `@baseapp/files/models.py` around lines 410 - 414, Annotate cls and the return
type of get_graphql_object_type in baseapp/files/models.py (lines 410-414),
using the appropriate class and FileObjectType-related types. Also annotate cls
in max_age, mint, and verify in baseapp/files/tokens.py (lines 35-53); no other
behavior changes are needed.
Source: Coding guidelines
Comment on lines
+135
to
+138
| token = PresignedUploadToken.mint( | ||
| file_id=file_obj.id, | ||
| part_number=1, | ||
| upload_id=file_obj.upload_id, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use the configured token lifetime in the expiry test.
PresignedUploadToken.max_age() reads FILE_UPLOAD_PRESIGNED_URL_EXPIRATION, but the test uses a fixed two-hour age. A configuration of two hours or more makes the token valid and causes this test to fail. Age the token by PresignedUploadToken.max_age() + 1 seconds, or override the setting explicitly.
Proposed fix
- with freeze_time(timezone.now() - timedelta(hours=2)):
+ with freeze_time(
+ timezone.now()
+ - timedelta(seconds=PresignedUploadToken.max_age() + 1)
+ ):📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| token = PresignedUploadToken.mint( | |
| file_id=file_obj.id, | |
| part_number=1, | |
| upload_id=file_obj.upload_id, | |
| with freeze_time( | |
| timezone.now() | |
| - timedelta(seconds=PresignedUploadToken.max_age() + 1) | |
| ): | |
| token = PresignedUploadToken.mint( | |
| file_id=file_obj.id, | |
| part_number=1, | |
| upload_id=file_obj.upload_id, |
🤖 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 `@baseapp/files/tests/test_presigned_uploads.py` around lines 135 - 138, Update
the expiry test around PresignedUploadToken.mint to age the token by
PresignedUploadToken.max_age() + 1 seconds instead of a fixed two-hour value,
ensuring it remains expired for the configured
FILE_UPLOAD_PRESIGNED_URL_EXPIRATION.
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
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
Adds the
baseapp.filespackage — a plugin-architecture file feature that attaches files to anyDocumentId-enabled object via a sharedFilesInterface, with S3-style multipart upload for large files. Built on the same shared-service/annotation pattern asbaseapp_reactions/baseapp_comments.What's included
Core package (
baseapp/files/)AbstractFile/AbstractFileTarget(DocumentId-keyed, counts maintained by pgtriggers). Concrete models live in consumers.FilesInterface,FileObjectType,FileAttachToTarget/FileDeletemutations,myFiles/filequeries.ContentPostnow implementsCommentsInterfaceso posts can carry comments (which in turn carry files).Performance (mirrors the reactions pattern)
AbstractFileTarget.annotate_queryset+ afiles_metadatashared service;FilesInterfaceresolvers read annotations with a fallback (no per-rowget_or_create/DocumentIdlookups).pre_optimization_hookapplies the files annotations, so listing comments with files is flat.filesCountinvariant to file volume; comments-with-files listing invariant to list size; single-SELECT annotation test).Security / correctness
FileAttachToTargetauthorizes the target (rejects non-FileableModeltargets; delegates "may attach here" to an overridableadd_filepermission) and requires each id to resolve to aFile.myFilesscoped to the authenticated user (was world-readable) with deterministic ordering.attach_files_from_relay_idsno longer treats a NULL creator as an implicit grant.Quality
AbstractFilestring fields useblank=True, default=""(nonull=True); consumer migrations include a NULL→""data step for safety on populated databases.Test plan
docker compose run --rm web pytest baseapp/files/— 118 passed (fresh DB, exercises migrations).makemigrations --checkclean; black/isort/flake8 clean.Notes for consumers
Downstream projects bumping this library regenerate their concrete
filesmigration; if their table has existing NULL string values, include the same NULL→""data step before theAlterField(seetestproject/files/migrations/0002).🤖 Generated with Claude Code
Summary by CodeRabbit