Skip to content

Merge silverlogic/baseapp-backend#442: baseapp.files package - #1

Open
nossila wants to merge 24 commits into
masterfrom
merge/baseapp-files
Open

Merge silverlogic/baseapp-backend#442: baseapp.files package#1
nossila wants to merge 24 commits into
masterfrom
merge/baseapp-files

Conversation

@nossila

@nossila nossila commented Oct 3, 2026

Copy link
Copy Markdown
Member

Merges silverlogic/baseapp-backend#442 (baseapp.files: attach files to any DocumentId object, multipart uploads) into the fork, the same way #445 (baseapp.geo) was merged. The upstream PR is still open; this brings it in early because whatplant needs it to store iNaturalist observation photos.

  • --no-ff merge of upstream/feature/baseapp_files (dcab83ef) onto fork master (57ef29a4). No conflicts: the only overlap is testproject/settings.py, where both packages add their lines.
  • whatplant-backend pins this merge commit.

Test plan

  • pytest baseapp/files baseapp/geo baseapp_comments in this repo's docker stack: 324 passed, 9 skipped.
  • Note: run pip install -e . --no-deps (or rebuild) first. A stale generated baseapp_backend.egg-info has no baseapp_files entry point, so the plugin's queries and mutations are missing from the schema and the files GraphQL tests fail. The upstream branch on its own fails the same way.

🤖 Generated with Claude Code

https://claude.ai/code/session_019P1oyApi4BLS8iLfMEGm8b

nossila and others added 24 commits June 27, 2026 00:04
@nossila @claude
baseapp-files: initial package, ported to plugin architecture
5345e34 
- 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
@nossila @claude
files admin: use baseapp_core.admin_helpers.ModelAdmin
637521c 
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
@nossila @claude
Merge origin/master into feature/baseapp_files
3401dad 
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0146oeiGsELS6QVpVUQHDQXd
@nossila @claude
files: port the reactions performance pattern (annotate_queryset + sh…
0b8e044 
…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
@nossila @claude
files: enforce attach permission and files-enabled check in FileAttac…
e09e8c6 
…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
@nossila @claude
files docs: multipart-only scope, fix parent_id examples, real storag…
cd36439 
…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
@nossila @claude
files: query-count tests locking in the annotate_queryset optimisation
7a74436 
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
@nossila @claude
content_feed: expose CommentsInterface on ContentPost
3c33b0f 
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
@nossila @claude
files: backfill test coverage; fix myFiles scoping and attach not_fou…
0c46112 
…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
@nossila @claude
files: address code-review findings (authz, ordering, dedup, dead sub…
2d037cf 
…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
@nossila @claude
files: address SonarCloud new-code findings
140cc44 
- 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
@nossila @claude
files: drop null=True on AbstractFile string fields (Sonar S6553)
5c39f5b 
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
@nossila @claude
core graphql: remove commented-out debug code in parse_body (Sonar S125)
65af75b 
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
@nossila @claude
files: address PR review — REST target authz, error hygiene, validation
1e6728b 
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
@nossila @claude
files: second review round (CodeRabbit/CodeQL) — hardening + fixes
290fbaf 
- 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
@nossila @claude
files: wrap REST ValidationError messages in arrays + annotate authz …
4fa9579 
…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
@nossila @claude
files: move storage I/O out of transactions, i18n-ready fields + mess…
bd7ec37 
…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
@nossila @claude
files: isort the generated 0003 migration (CI isort --check runs repo…
c5b4055 
…-wide)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0146oeiGsELS6QVpVUQHDQXd
@nossila @claude
files: add 'from None' to serializer parent_id re-raises (Ruff B904)
5895207 
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
@nossila @claude
files: register count triggers via pgtrigger_register_default_track
3e239eb 
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
@nossila @claude
files: isolate presigned upload token logic in PresignedUploadToken
8433993 
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
@nossila
Merge remote-tracking branch 'origin/master' into feature/baseapp_files
f392bd8 
# Conflicts:
#	baseapp_core/graphql/fields.py
@nossila
Merge remote-tracking branch 'origin/master' into feature/baseapp_files
dcab83e
@nossila
Merge silverlogic/baseapp-backend#442: baseapp.files package
619ddf2
This was referenced Oct 3, 2026
Open
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@nossila