Skip to content

WeFa attachments plugin#395

Open
srozen wants to merge 1 commit intomainfrom
feature/attachment-plugin
Open

WeFa attachments plugin#395
srozen wants to merge 1 commit intomainfrom
feature/attachment-plugin

Conversation

@srozen
Copy link
Copy Markdown
Contributor

@srozen srozen commented Apr 29, 2026

Summary

Introduces nside_wefa.attachments — an opinionated, abstract attachment app for the WeFa toolkit. Subclasses gain versioned file storage, MIME sniffing via python-magic, pluggable storage backends via django-storages, streaming size enforcement, content hashing, and a generic CRUD endpoint factory in either singleton or multi mode.

Also ships two demo apps (demo.profiles, demo.documents) showing both modes end-to-end.

How consumers use it

1. Define a concrete subclass

from django.conf import settings
from django.db import models
from nside_wefa.attachments.models import Attachment


class Avatar(Attachment):
    versioning_enabled = False                              # replace in place
    allowed_content_types = ["image/png", "image/jpeg"]     # whitelist; required
    max_size = 2 * 1024 * 1024
    upload_path_prefix = "avatars/"

    user = models.OneToOneField(
        settings.AUTH_USER_MODEL, on_delete=models.CASCADE, related_name="avatar"
    )


class Document(Attachment):
    versioning_enabled = True                                # default
    allowed_content_types = [
        "application/pdf",
        "application/vnd.ms-excel",
        "application/vnd.openxmlformats-officedocument.spreadsheetml.sheet",
    ]
    max_size = 25 * 1024 * 1024
    upload_path_prefix = "documents/"

    owner = models.ForeignKey(
        settings.AUTH_USER_MODEL, on_delete=models.CASCADE, related_name="documents"
    )

The base contributes the persisted columns (attachment_uid, version, is_current, file, filename, content_type, byte_size, file_hash, hash_algorithm, uploaded_by, created_at, updated_at) onto the subclass's table. Subclasses add their own owning FKs and any extra fields they need.

2. Wire the endpoints

from nside_wefa.attachments.urls import register_attachment_endpoints
from rest_framework.permissions import IsAuthenticated
from .models import Avatar, Document

urlpatterns = [
    *register_attachment_endpoints(
        Avatar, prefix="me/avatar", singleton=True,
        permissions=[IsAuthenticated],
        queryset=lambda request, **_: Avatar.objects.filter(user=request.user),
        on_create=lambda request, instance, **_: setattr(instance, "user", request.user),
    ),
    *register_attachment_endpoints(
        Document, prefix="documents",                       # singleton=False
        permissions=[IsAuthenticated],
        queryset=lambda request, **_: Document.objects.filter(owner=request.user),
        on_create=lambda request, instance, **_: setattr(instance, "owner", request.user),
    ),
]

queryset and on_create are the two extension points: the first scopes every endpoint to whatever the caller is allowed to see (declarative authorisation), the second binds owning FKs on creation.

3. URL surface

Singleton mode (one logical attachment per scope — avatars, contract PDFs):

Method URL Behavior
GET /me/avatar/ Current row, or 404.
POST /me/avatar/ First call → v1; later calls bump to v+1, or replace in place if versioning_enabled=False.
GET /me/avatar/versions/ History (omitted when versioning_enabled=False).
GET /me/avatar/versions/<n>/ A specific historical version.
GET /me/avatar/download/ Stream current bytes with Content-Disposition.
DELETE /me/avatar/ Hard-delete every version + blob.

Multi mode (many distinct attachments per scope — issue files, document libraries):

Method URL Behavior
GET /documents/ List of current versions, one per logical attachment.
POST /documents/ Create a new logical attachment (v1, fresh uid).
GET /documents/<id>/ A specific row.
POST /documents/<id>/versions/ Bump that logical attachment's version.
GET /documents/<id>/versions/ History for that uid.
GET /documents/<id>/download/ Stream that row.
DELETE /documents/<id>/ Hard-delete the entire logical attachment.

POST is the only mutating verb in either mode — there is no PUT/PATCH on the public surface, by design.

4. Programmatic API (when you don't want to go through HTTP)

# Create v1
Document.upload(file=request.FILES["file"], owner=user)

# Bump to v2 (versioned subclasses)
Document.add_version(file=request.FILES["file"], parent=current_row)
# add_version inherits owning FKs from the parent automatically.

# Replace in place (non-versioned subclasses)
existing_avatar.replace_in_place(file=request.FILES["file"])

# Manager helpers
user.documents.current()                        # one row per logical doc
Document.objects.history(some_uid)              # full chronological history

# Hard delete
existing.hard_delete_with_blob()                # this row + its blob
Document.hard_delete_logical(uid)               # every version + every blob

Security stance

  • Whitelist-only content types. Every concrete subclass must declare a non-empty allowed_content_types list. Empty list / missing attribute → ImproperlyConfigured. There is no blacklist, no wildcards, no "allow anything that isn't an executable" path — the policy is exact-match MIME strings, so it's auditable.
  • MIME comes from the bytes, not the client header. python-magic reads the first ~2 KB of the stream; the client header is logged but never trusted.
  • Size is checked twice — eagerly via UploadedFile.size, then again as a streaming counter while the hash is computed. A client lying about size still hits a ceiling.
  • Filenames are sanitised (path separators, control chars, NUL, Unicode normalisation) but the storage key is {prefix}/{attachment_uid}/{version}/{sanitised_filename} — uid and version are server-side state, so user input never controls anything beyond the trailing component.
  • Versioning runs in transaction.atomic() + select_for_update() over the affected attachment_uid (or singleton scope). Concurrent POSTs serialise; no version-number races.
  • Hard delete removes the blob, not just the row. Provided as an explicit method (hard_delete_with_blob, hard_delete_logical) since Django's default delete() doesn't.

Configuration

# settings.py
STORAGES = {
    "default": {"BACKEND": "django.core.files.storage.FileSystemStorage"},
    # Register additional aliases (S3, Azure, GCS, SFTP) per
    # https://django-storages.readthedocs.io/
}

NSIDE_WEFA = {
    "ATTACHMENTS": {
        # All keys optional; defaults shown.
        "STORAGE": "default",                # alias into settings.STORAGES
        "MAX_FILE_SIZE": 10 * 1024 * 1024,    # global cap; subclasses may override
        "UPLOAD_PATH_PREFIX": "attachments/",
        "HASH_ALGORITHM": "sha256",
        "CONTENT_TYPE_SNIFF_BYTES": 2048,
    },
}

System checks validate the section at manage.py check time — invalid storage alias, unknown hash algorithm, non-positive sniff size, or wrong INSTALLED_APPS order all surface as Error records before the server ever boots.

What's in the PR

  • nside_wefa/attachments/ — the app itself: abstract model, validators, storage resolver, settings helper, system checks, serializer factory, view factory, URL helper, admin mixin, README.
  • nside_wefa/attachments/tests/ — 73 tests across test_models, test_validators, test_storage, test_checks, and test_views/{singleton,multi,replace_in_place}. The fixture suite uses real magic-byte headers so libmagic returns the expected MIMEs.
    • tests/test_app/ is a tiny Django app with three concrete subclasses (SingletonPdfAttachment, MultiImageAttachment, AvatarAttachment) that exercise every mode.
    • tests/settings.py extends demo.settings to add the test app — the demo project itself stays free of test scaffolding (pytest.ini now points at nside_wefa.attachments.tests.settings).
  • demo/profiles/ + demo/documents/ — two illustrative consumer apps. Heavily commented for first-time readers (the demo's job is teaching, not minimalism). 18 HTTP-level tests demonstrate the wiring patterns a real consumer's test suite would use.
  • demo/README.md — explains the demo's purpose, the per-app reading order, and notes that the Vue demo at vue/ is hard-wired to talk to http://localhost:8000.
  • django/README.md — links every per-app README properly, adds the new Demo project and Attachments sections, lists the new dependencies (django-storages, python-magic).
  • Dependencies: django-storages>=1.14 and python-magic>=0.4.27 added to pyproject.toml. python-magic needs libmagic on the host (brew install libmagic on macOS; already present on the standard Linux CI image).

Test plan

  • cd django && uv run ruff format --check . — clean
  • uv run ruff check . — clean
  • uv run mypy nside_wefa/ demo/ — 173 source files, no errors
  • uv run pytest416 passed (398 prior + 18 new demo). No regressions.
  • uv run python manage.py check — no system check issues with default and overridden settings
  • Reviewer manual smoke (optional):
    • uv run python manage.py runserver, hit POST /me/avatar/ with a PNG → 201; second POST → 200, single row in DB
    • POST /documents/ with a PDF → 201; POST /documents/<id>/versions/ with a different PDF → 201, version=2, prior row flipped is_current=False
    • POST /me/avatar/ with a PDF (renamed to .png) → 400, sniffed MIME rejected

Compatibility / migration notes

  • No breaking changes to existing apps. The STORAGES setting was added to demo settings (Django 5+ form); deployments that haven't declared STORAGES should add the snippet above.
  • No new migrations ship from nside_wefa.attachments itself (the model is abstract). Consumers run makemigrations for their app after subclassing.
  • pytest.ini's DJANGO_SETTINGS_MODULE moved from demo.settingsnside_wefa.attachments.tests.settings. CI scripts that hard-code the settings module path may need an update.

Files worth a closer look

  • django/nside_wefa/attachments/models/attachment.py — abstract base, save-time pipeline, versioning + replace-in-place semantics.
  • django/nside_wefa/attachments/validators.py — security perimeter (sniff, whitelist, size, hash, sanitise).
  • django/nside_wefa/attachments/views/attachment_views.py — view factory; the _create_or_replace_in_singleton_scope helper is where the singleton POST atomicity lives.
  • django/nside_wefa/attachments/urls.py — public register_attachment_endpoints entry point.
  • django/demo/profiles/ and django/demo/documents/ — read these to understand what consumer code looks like in practice.
  • django/nside_wefa/attachments/README.md — full usage doc with config reference.

@srozen srozen requested a review from ALambot April 29, 2026 09:18
@srozen srozen self-assigned this Apr 29, 2026
@srozen srozen added the enhancement New feature or request label Apr 29, 2026
@srozen srozen requested a review from fsouris April 29, 2026 09:18
@srozen srozen changed the title Introduce the Attachment plugin WeFa attachments plugin Apr 29, 2026
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 64cdefe83f

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +130 to +134
existing_qs = base_qs.select_for_update().filter(is_current=True)
existing = list(existing_qs[:1])

if not existing:
instance = model_class(
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Serialize singleton creation when no row exists

When the singleton scope is empty, select_for_update() locks zero rows, so two concurrent POST requests can both enter if not existing and each insert a separate v1 row. This breaks the singleton contract (one logical attachment per scope) for scopes that are not protected by a DB-level unique constraint, and can leave multiple current attachments that make later reads/updates ambiguous.

Useful? React with 👍 / 👎.

Comment on lines +364 to +367
upload = _extract_file(request)
instance = model_class(
is_current=True,
version=1,
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Apply writable form fields during attachment creation

The create path only reads request.FILES['file'] and manually instantiates the model, so non-file fields from multipart form data are silently ignored. That means fields exposed via build_attachment_serializer(..., extra_writable=...) cannot actually be set through these endpoints, despite being declared writable in the serializer API.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant