Conversation
There was a problem hiding this comment.
💡 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".
| existing_qs = base_qs.select_for_update().filter(is_current=True) | ||
| existing = list(existing_qs[:1]) | ||
|
|
||
| if not existing: | ||
| instance = model_class( |
There was a problem hiding this comment.
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 👍 / 👎.
| upload = _extract_file(request) | ||
| instance = model_class( | ||
| is_current=True, | ||
| version=1, |
There was a problem hiding this comment.
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 👍 / 👎.
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
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
querysetandon_createare 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):
GET/me/avatar/POST/me/avatar/versioning_enabled=False.GET/me/avatar/versions/versioning_enabled=False).GET/me/avatar/versions/<n>/GET/me/avatar/download/Content-Disposition.DELETE/me/avatar/Multi mode (many distinct attachments per scope — issue files, document libraries):
GET/documents/POST/documents/GET/documents/<id>/POST/documents/<id>/versions/GET/documents/<id>/versions/GET/documents/<id>/download/DELETE/documents/<id>/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)
Security stance
allowed_content_typeslist. 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.UploadedFile.size, then again as a streaming counter while the hash is computed. A client lying about size still hits a ceiling.{prefix}/{attachment_uid}/{version}/{sanitised_filename}— uid and version are server-side state, so user input never controls anything beyond the trailing component.transaction.atomic()+select_for_update()over the affectedattachment_uid(or singleton scope). Concurrent POSTs serialise; no version-number races.hard_delete_with_blob,hard_delete_logical) since Django's defaultdelete()doesn't.Configuration
System checks validate the section at
manage.py checktime — invalid storage alias, unknown hash algorithm, non-positive sniff size, or wrong INSTALLED_APPS order all surface asErrorrecords 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 acrosstest_models,test_validators,test_storage,test_checks, andtest_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.pyextendsdemo.settingsto add the test app — the demo project itself stays free of test scaffolding (pytest.ininow points atnside_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 atvue/is hard-wired to talk tohttp://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).django-storages>=1.14andpython-magic>=0.4.27added topyproject.toml. python-magic needslibmagicon the host (brew install libmagicon macOS; already present on the standard Linux CI image).Test plan
cd django && uv run ruff format --check .— cleanuv run ruff check .— cleanuv run mypy nside_wefa/ demo/— 173 source files, no errorsuv run pytest— 416 passed (398 prior + 18 new demo). No regressions.uv run python manage.py check— no system check issues with default and overridden settingsuv run python manage.py runserver, hitPOST /me/avatar/with a PNG → 201; second POST → 200, single row in DBPOST /documents/with a PDF → 201;POST /documents/<id>/versions/with a different PDF → 201,version=2, prior row flippedis_current=FalsePOST /me/avatar/with a PDF (renamed to.png) → 400, sniffed MIME rejectedCompatibility / migration notes
STORAGESsetting was added to demo settings (Django 5+ form); deployments that haven't declaredSTORAGESshould add the snippet above.nside_wefa.attachmentsitself (the model is abstract). Consumers runmakemigrationsfor their app after subclassing.pytest.ini'sDJANGO_SETTINGS_MODULEmoved fromdemo.settings→nside_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_scopehelper is where the singleton POST atomicity lives.django/nside_wefa/attachments/urls.py— publicregister_attachment_endpointsentry point.django/demo/profiles/anddjango/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.