Modernize file storage and transport using Django Storage API - #3732
Closed
cesarbenjamindotnet wants to merge 21 commits into
Closed
Modernize file storage and transport using Django Storage API#3732cesarbenjamindotnet wants to merge 21 commits into
cesarbenjamindotnet wants to merge 21 commits into
Conversation
Phase 0 inventory of CARE's runtime, verified against the code at 6a2976d. Documentation only; no application behavior changed. Adds docs/xii/architecture/inventory/: storage-call-sites.md 19 object-storage call sites task-call-sites.md 8 task definitions, 4 async dispatch sites cache-and-redis.md 69 cache/Redis call sites, 3 hard couplings frontend-file-flow.md file API contract and required changes runtime-and-deployment.md process/image/CI facts, baseline blockers plugin-impact.md plugin loading; no plugins bundled unresolved-items.md 12 defects, 10 open questions, 8 unknowns Corrections to 01-current-runtime.md: - remove stray ````markdown fence that made the frontmatter and sections 1-2 render as literal code - section 38: patient files do not use FILE_UPLOAD_REGION/KEY/SECRET; get_patient_bucket_config reads FACILITY_S3_* instead, leaving those three settings dead - section 40: note reports inherit the same FACILITY_S3_* credentials - section 26: add the three task definitions outside care/emr/tasks/ - fix two wrong paths to 02-target-runtime.md - record verified_against_commit Baseline commands could not run: Docker daemon down, make absent, no Python 3.13 environment, Redis not running. Blockers documented in runtime-and-deployment.md section 11. All 84 cited paths and 46 cited symbols verified to exist. Co-Authored-By: Claude <noreply@anthropic.com>
Record the Phase 0 green baseline for the current upstream-compatible CARE runtime. Documentation only; no runtime, settings, model, migration or dependency change. runtime-and-deployment.md - Replace the BLOCKED section 11 with the recorded baseline: environment, environment files, translated Makefile commands, build result, service health, startup sequence, migration and sync results, fixtures, tests. - 312 migrations applied; makemigrations --check clean; 115 permissions, 10 roles, 546 role-permissions, 30 valuesets; fixtures loaded. - 1912 tests, 0 skipped, green on 2 of 3 --parallel --shuffle runs. unresolved-items.md - Mark Part E resolved and record the disposition of E1-E6. - Withdraw E6: no root .env is required. docker/.local.env and docker/.prebuilt.env are tracked in git and have no .example variants. - Add E7 (flaky test_password_request_rate_limiting under --parallel) and E8 (transient BuildKit pip cache wheel corruption). - Resolve D8. Co-Authored-By: Claude <noreply@anthropic.com>
ADR-0001 / ES-01 (IS-01) milestone 1. Adds django-storages 1.14.6 with the s3 and google extras so that object persistence can move onto Django's Storage API. No application code uses it yet. Resolution notes: - A full `pipenv lock` moved 59 unrelated packages, which ES-01 section 8 forbids. Used `pipenv upgrade` instead, which merges only the resolved subtree into Pipfile.lock. - All 44 direct [packages] pins are unchanged, including boto3 ==1.43.6 (retained: SNS and the legacy signed-URL path still need it) and django ==6.0. - dev-packages and docs categories are untouched. - 12 packages added: django-storages plus the google-cloud-storage tree. - 8 transitive packages moved as an unavoidable consequence of resolving the google extra: asgiref, botocore, certifi, cffi, charset-normalizer, cryptography, idna, s3transfer. None is a direct dependency. django-storages 1.14.6 advertises Django support only through 5.1, so compatibility with Django 6.0 was verified by rebuilding the image and importing both S3Storage and GoogleCloudStorage. Co-Authored-By: Claude <noreply@anthropic.com>
ADR-0001 / ES-01 (IS-01) milestone 2. Configuration only; no application code is routed through the new aliases yet. Adds CARE_STORAGE_BACKEND (s3 default, gcs supported). An unsupported value raises ImproperlyConfigured naming the supported values. Defines the logical aliases patient, facility and report in STORAGES, built by config/storage.py. staticfiles is untouched and stays on WhiteNoise. `default` is deliberately still absent: Django 6.0 does not merge STORAGES with its global defaults and nothing in CARE resolves that alias, so adding it would change behaviour rather than preserve it. Bucket names come from provider-neutral CARE_*_STORAGE_BUCKET variables, each defaulting to the existing setting, so no local or deployed configuration has to be rewritten. report shares the patient bucket, as before, but is a separate alias so it can be repointed without touching application code. Behaviour change, per ES-01 section 26 (do not preserve a verified settings bug): the patient and report aliases now read FILE_UPLOAD_REGION, FILE_UPLOAD_KEY and FILE_UPLOAD_SECRET. get_patient_bucket_config and get_report_bucket_config read the FACILITY_S3_* credentials instead, leaving those three settings dead. Locally all of them resolve to the same MinIO values, so there is no local change; a deployment that sets FACILITY_S3_KEY to something other than BUCKET_KEY without also setting FILE_UPLOAD_KEY will need to set it. file_overwrite is set explicitly because CARE generates a unique internal_name and the boto3 put_object being replaced overwrote unconditionally; Django must not rename on collision. Verified against MinIO: re-saving returns the same name and replaces the content. BUCKET_HAS_FINE_ACL is honoured at the facility alias rather than per object. The facility bucket's only writer is cover_image.py, so this is equivalent. It is ignored under GCS, which requires uniform bucket-level access. Verified: all four aliases resolve; a MinIO save/exists/size/open/delete round trip succeeds; open of a missing object raises FileNotFoundError; GCS aliases construct with no credentials present; collectstatic and manage.py check pass. Co-Authored-By: Claude <noreply@anthropic.com>
ADR-0001 / ES-01 (IS-01) milestone 3. Ordinary object persistence now goes through Django Storage aliases. No provider client is constructed for persistence anywhere in CARE. file_manager.py replaces S3FilesManager with FilesManager, a transitional wrapper that resolves a logical alias and delegates to Django Storage. It imports no provider SDK and holds no provider branch. Retained rather than removed because files_manager is a class attribute on FileUpload and ReportUpload and is used by viewsets, tasks and report generation; rewriting every caller was wider than IS-01 warrants. get_storage_name() is the provider-neutral name helper. It preserves the verified <file_type>/<internal_name> convention exactly, returns a relative name with no bucket, URL or endpoint, and rejects traversal. Both components are already server-controlled (FileTypeChoices, the report-type registry, and a UUID internal_name), so the guard is defence in depth. Signed URLs move to the new legacy_signed_urls.py, isolated from ordinary CRUD per ES-01 section 21 and listing its exact callers. It is the only remaining provider SDK use for files and is S3-only; IS-02 removes it. Migrated call sites: - file_upload viewset put_object -> Storage.save (unchanged signature) - report generation -> Storage.save, wrapping the already-materialised bytes in ContentFile - cleanup task -> Storage.delete - cover_image.py -> storages[facility] directly, keeping its own <folder>/<id>_<token>.<ext> key convention, and now streaming the uploaded file instead of passing raw bytes delete_object no longer takes `quiet`. Django Storage delete is idempotent on both backends, and so was the boto3 call it replaces: S3 delete_object does not raise NoSuchKey, so the old quiet=False branch never fired. report generation keeps an explicit content type. S3Storage prefers content.content_type; GoogleCloudStorage derives it from the name, so the extension in internal_name stays the portable signal. The only registered generator is html/.html, which guesses identically. test_file_upload_api no longer asserts botocore ClientError/NoSuchKey. It now asserts the object does not exist and that opening it raises FileNotFoundError -- provider-neutral and one assertion stronger. care/emr/tasks/report_generation.py keeps autoretry_for=(ClientError,) deliberately. It imports an exception class for retry configuration and never instantiates boto3; under the default s3 profile django-storages raises ClientError from inside Storage.save, so retry behaviour is unchanged. It would not retry under gcs; recorded as a known gap rather than changing Celery retry semantics, which ES-01 section 31 excludes. Tests: 102 pass across file upload, facility, user and diagnostic report suites, including real MinIO round trips. Co-Authored-By: Claude <noreply@anthropic.com>
ADR-0001 / ES-01 (IS-01) milestone 4. 50 new tests. care/utils/tests/test_storage_config.py -- construction only, no provider contact and no Google credentials: - s3 is the default backend - patient, facility and report resolve to S3Storage under s3 - staticfiles stays on WhiteNoise - an invalid backend raises ImproperlyConfigured naming s3 and gcs - report shares the patient bucket while staying a distinct alias - alias names contain no provider name - GCS variables are not required under s3, and S3 options are not leaked into a GCS alias - GCS never uses object ACLs, and GoogleCloudStorage constructs with no credentials present - the endpoint is omitted for AWS and included for S3-compatible care/emr/tests/test_storage.py: - the <file_type>/<internal_name> convention, including Unicode, multi-part extensions and unusual names, all preserved byte-for-byte - names are relative: no bucket, URL or endpoint - traversal, absolute paths and empty components raise SuspiciousFileOperation - models are bound to logical aliases, cover images to facility - delegation proven by substituting InMemoryStorage at the Django Storage boundary; no provider SDK is mocked - MinIO integration on all three aliases: save, exists, size, open, delete, missing-object FileNotFoundError, idempotent delete, and overwrite keeping the name Writing the delegation tests surfaced a real distinction worth recording: overwrite-on-collision comes from the file_overwrite backend option, not from Django Storage. InMemoryStorage renames instead. So overwrite is asserted against the configured backends in the MinIO tests and the option itself in the config tests, while the delegation test asserts only that FilesManager reports back what the backend did. Integration test names are UUID-based because the 16 parallel workers share one MinIO. Co-Authored-By: Claude <noreply@anthropic.com>
…rence 07-configuration-reference.md section 12.4 specifies GCS_PROJECT_ID. The initial implementation used CARE_GCS_PROJECT_ID. The documentation is authoritative, so the code is aligned to it rather than the reverse. Optional either way: when unset, GoogleCloudStorage uses Application Default Credentials and no project_id option is emitted. Co-Authored-By: Claude <noreply@anthropic.com>
Documentation only. inventory/storage-call-sites.md - New section 11 marks the migration status of all 19 call sites: 9 migrated_to_django_storage, 7 legacy_signed_url_only, 2 temporary_wrapper, 1 removed, 1 not_storage_persistence, 0 blocked. - Records the alias table, the object-name helper, the corrected credential defect from section 2.1 and its behaviour change, the two remaining provider SDK uses, and the whole-file reads that remain. - Sections 1-8 are left as the Phase 0 snapshot; section 11 is current where they disagree. inventory/frontend-file-flow.md - New section 11: no route, request field or response field changed. - Maps the file and line references IS-01 relocated. - Records which persistence now uses Django Storage, and which signed upload, signed download and base64 flows remain for IS-02. - Marks D3 partly done and C3 resolved at the settings layer; the rest of the section 9 change list is unchanged. - Records that legacy_signed_urls is S3-only, so IS-02 is a prerequisite for a real GCS deployment. inventory/unresolved-items.md - New Part B2 with S1 (GCS cannot serve files end to end), S2 (report retry does not fire under GCS), S3 (overwrite safety depends on a backend option) and S4 (items unchanged by IS-01). - B10 resolved: delete_objects removed. - B1 partly resolved: the aliases read the correct credentials; the two csp/config.py resolvers still carry the defect but are now reached only by the legacy signed-URL path. - E7 scope corrected. It is not one flaky test but a defect class: six tests across rate limiting and favorites, all caused by a single Redis cache shared by 16 parallel workers while three modules call cache.clear() in setUp. Measured: full suite serial 1962/1962 OK; full suite parallel 1 of 6 green; the three cache-touching modules alone fail 4 of 4 in parallel with no storage code involved. Includes the recommended per-worker KEY_PREFIX fix. 02-target-runtime.md section 11 - States the four required claims explicitly: Django Storage API is the architecture; MinIO through S3Storage is the default local profile; generic S3-compatible storage remains supported; GCS is the initial GCP profile, not the only supported provider. - Adds the post-IS-01 status of the three SHALL NOT clauses, naming the two transitional exceptions. 07-configuration-reference.md - 11.2: records CARE_STORAGE_BACKEND as implemented, s3 default, and that filesystem is not a supported value per ES-01 section 9. - 12.8: corrects the suggestion that GCS_FILE_OVERWRITE=false may suit unique immutable names. false does not reject duplicates, it silently renames, which is exactly the database-reference hazard the section warns about. Overwrite is enabled on every alias, with the tests that demonstrate both behaviours cited. Co-Authored-By: Claude <noreply@anthropic.com>
… Django
ADR-0001 completion. Object transport is now entirely mediated by CARE:
no application code generates a storage-provider URL, and no client ever
receives one.
Removed:
- care/emr/utils/legacy_signed_urls.py (presigned PUT and GET generation)
- care/utils/csp/ entirely: BucketType, CSProvider, ClientConfig and
get_client_config existed only to resolve provider credentials and
external endpoints for those signed URLs
- the signed_url response field on FileUploadRetrieveSpec and
ReportUploadRetrieveSpec, and the _just_created write branch
- FACILITY_CDN, BUCKET_HAS_FINE_ACL, BUCKET_EXTERNAL_ENDPOINT,
FILE_UPLOAD_BUCKET_EXTERNAL_ENDPOINT and
FACILITY_S3_BUCKET_EXTERNAL_ENDPOINT, which had no consumer left
- the public-read ACL on the facility alias; every bucket is now private
Added care/emr/utils/file_download.py: a provider-neutral FileResponse
helper that streams through Django Storage. It carries the
inline-vs-attachment decision the presigned ResponseContentDisposition
used to make, so browser handling of patient documents is unchanged.
Download routes:
- GET /api/v1/files/{external_id}/download/ -- authorized by the existing
get_queryset file_authorizer, which runs for every detail action
- GET /api/v1/template_reports/{external_id}/download/ -- authorizes
explicitly, because ReportUploadViewSet.get_queryset only guards list
- GET /api/v1/assets/facility/{external_id}/cover_image/
- GET /api/v1/assets/user/{username}/profile_picture/
read_signed_url is replaced by download_url, a CARE route.
The two asset routes are unauthenticated, and deliberately so. Cover
images and avatars are already world-readable: AllFacilityViewSet and
FacilitySchedulableUsersViewSet are anonymous and expose them. Who can
see an image is unchanged; what changes is that CARE serves the bytes,
so the bucket becomes private. They are separate views because
FacilityViewSet and UserViewSet filter their querysets by request.user
and cannot serve an anonymous request.
Facility.read_cover_image_url and User.read_profile_picture_url now
reverse to those routes instead of concatenating a bucket URL, which also
removes the bucket-name divergence between persistence and URL building
that the review found.
BUCKET_PROVIDER survives as the credential-source switch, now compared
against a literal in config/storage.py rather than the deleted enum.
Tests: the presigned PUT round trip in test_upload_patient_file is
replaced by the Django upload endpoint plus a download through CARE,
asserting the route is a CARE URL and that Content-Type and
Content-Disposition are preserved. test_cleanup_incomplete_file_uploads
writes its object through Django Storage. The
FILE_UPLOAD_BUCKET_EXTERNAL_ENDPOINT override, which existed only for
signed URLs, is gone. 12 file API tests pass.
Co-Authored-By: Claude <noreply@anthropic.com>
plugin-impact.md records that S3FilesManager is importable from care.emr.utils.file_manager and that any plugin may use it. Renaming it to FilesManager in the previous commit was therefore a silent breaking change for external plugins. The name is restored as a thin subclass. Despite the name it is not S3-specific: it is FilesManager, delegating to whichever provider the alias is configured for. It emits a DeprecationWarning, accepts the historical positional argument that used to be a BucketType, and maps PATIENT/FACILITY/REPORT onto the logical aliases. An unrecognised value raises rather than silently writing to the wrong bucket. It deliberately exposes no signed_url or read_signed_url method. Those were removed with the provider-specific transport, and a plugin must not be able to mint a bucket URL that bypasses CARE. Also updates the module docstring, which still pointed at the deleted legacy_signed_urls module. Co-Authored-By: Claude <noreply@anthropic.com>
New care/emr/tests/test_storage_transport.py (25 tests) asserts the property the architecture depends on rather than the mechanics: - the base64 upload endpoint still works and returns no provider URL - no response offers signed_url or read_signed_url - download_url is a CARE route, matched against reverse() - detail and list responses are scanned for provider markers (minio, amazonaws, storage.googleapis, X-Amz-Signature, GoogleAccessId, :9100) - downloading through Django returns the stored bytes - download enforces authorization; a different user gets 403 - a missing object yields 404, not a provider error - facility cover image and user avatar URLs are CARE routes that serve the stored bytes - the cover-image route stays anonymous, matching the world-readable bucket objects it replaces, and 404s when no image is set - every alias round-trips through Django Storage - no manager exposes a signed-URL helper Plugin compatibility: S3FilesManager imports, warns, delegates to Django Storage, maps each legacy bucket name, rejects unknown aliases, and exposes no signed-URL method. Updated test_storage_config for the removed default_acl parameter and added a test that no configured alias can be public. 68 storage tests pass. Co-Authored-By: Claude <noreply@anthropic.com>
ADR-0001 - Implementation checklist: IS-01 complete, legacy storage removed, legacy signed URL flows removed. - Records what IS-01 delivered and that only the base64 upload transport remains for IS-02. storage-call-sites.md - Final per-call-site status: 11 migrated, 2 temporary wrapper, 6 removed. Nothing is legacy_signed_url_only; nothing is blocked. - New 11.1a: the four CARE routes that now mediate transport, their authorization, and why the two asset routes are anonymous. - New 11.3a: one source of truth per logical bucket. Records that this closes a defect introduced earlier in IS-01, where setting CARE_PATIENT_STORAGE_BUCKET moved persistence but not the signed URLs, so uploads and downloads silently addressed different buckets on the default s3 profile. - 11.5 rewritten: no storage module imports a provider SDK. Lists the deleted settings and the csp package. - New 11.8: the two remaining compatibility layers. frontend-file-flow.md - 11.3 rewritten: signed upload, signed download and unsigned bucket URLs are all gone; base64 upload is all that remains for IS-02. - 11.4: 13 of the 18 section 9 change items are now done. - 11.5: contract impact as delivered, including that the four URL fields are now relative paths where they used to be absolute. - 11.6: GCS is no longer blocked on transport. plugin-impact.md - Corrects the row claiming a plugin can mint presigned URLs through S3FilesManager. - New section 9: deprecation notice with a before/after table of every behavioural change, the DeprecationWarning, the migration target, and the fact that the no-signed-URL guarantee is CARE's rather than enforced against plugin code. 07-configuration-reference.md - Section 13 corrected. It specified S3_ACCESS_KEY, S3_SECRET_KEY, S3_ENDPOINT_URL, S3_REGION_NAME, S3_ADDRESSING_STYLE and S3_SIGNATURE_VERSION; none exists. Replaced with the actual per-alias credential and endpoint variables, BUCKET_PROVIDER as the credential source, and the bucket variables as the single point of resolution. - Records that addressing_style and signature_version are not configurable, and what that rules out. 02-target-runtime.md - Section 11 status: all three SHALL NOT clauses now hold for persistence and transport. Records that transport is entirely CARE-mediated and that csp/ is deleted. unresolved-items.md - S1 resolved: the GCS profile serves files end to end; IS-02 is no longer a prerequisite for a GCS deployment. - S2 is now the only item blocking that profile. - B9 largely defused: no client can write to the bucket, so an unverified mark_upload_completed is bookkeeping rather than an unverified external write. Co-Authored-By: Claude <noreply@anthropic.com>
ADR-0002 / ES-02. The last remaining transport issue: the Django-mediated upload still carried complete file contents as base64 inside JSON. POST /api/v1/files/upload-file/ now accepts multipart/form-data. The binary part is `file`; the base64 `file_data` field is removed with no fallback, per ES-02 section 23 (greenfield, no production client). FileUploadMultipartSerializer declares the transport contract and the binary field. It deliberately does not restate the domain rules: FileUploadCreateSpec remains the authoritative validator for file type, category and filename, so there is no second source of truth. `original_name` becomes optional and defaults to the uploaded part's filename, which multipart supplies and base64 could not. The file is never materialised: - Django's upload handlers decide InMemoryUploadedFile vs TemporaryUploadedFile before the view runs; - size is read from UploadedFile.size, not by reading content; - MIME sniffing reads only the leading 2048 bytes and seeks back, as before, so a browser-declared Content-Type is still not trusted; - the UploadedFile is handed straight to Storage.save() with no ContentFile round trip. Upload limits are now explicit rather than implicit, per ADR-0002. The added FILE_UPLOAD_MAX_MEMORY_SIZE and DATA_UPLOAD_MAX_MEMORY_SIZE are set to Django's own defaults, so behaviour is unchanged: anything over 2.5 MB is temp-file backed, and MAX_FILE_UPLOAD_SIZE still caps the total at 5 MB. DATA_UPLOAD_MAX_MEMORY_SIZE does not bound the file, because multipart file parts are exempt from it. Worth noting: base64 inflated a 5 MB file to roughly 6.7 MB of JSON body, which exceeds Django's 2.5 MB DATA_UPLOAD_MAX_MEMORY_SIZE. The old endpoint could not actually accept a file near its own documented limit over JSON. Multipart removes that. Persistence, object naming, alias selection and authorization are untouched: still Storage.save() through the patient alias, still get_storage_name, still authorize_create -> file_authorizer. The DB row is still written first inside the transaction so a storage failure rolls it back. Existing tests are updated to send multipart. test_upload_patient_file also stops conflating two rows: it previously created a row via POST /files/, uploaded a different row, then marked the first complete. 30 upload and transport tests pass. Co-Authored-By: Claude <noreply@anthropic.com>
39 focused tests in care/emr/tests/test_file_upload_multipart.py, covering ES-02 sections 31, 33 and 34. Successful upload: record contents, patient alias, the ES-01 object name unchanged, stored bytes match, download works, response carries no provider marker and no bucket or endpoint key. Rejection: missing file, missing metadata, oversized file (and that an oversized file persists nothing), and that the old base64 `file_data` body no longer works even by accident. Validation: the declared part Content-Type is not trusted. A shell script sent as image/jpeg is rejected on sniffed content; a real JPEG sent as application/x-sh is accepted and recorded as image/jpeg. Blocked, uppercase and double extensions behave as before, as do unknown file type and category. Authorization: unauthenticated and unauthorized uploads are rejected and persist nothing. Upload handlers: with a lowered FILE_UPLOAD_MAX_MEMORY_SIZE, the view is verified to receive an InMemoryUploadedFile below the threshold and a TemporaryUploadedFile above it, and the temp-file path round-trips. Failure consistency: a storage failure reports failure and leaves no row; a database failure after the object is written also rolls the row back. The surviving gap is the orphan object, which is the pre-existing B8. Provider neutrality: the whole upload and download flow is exercised against InMemoryStorage substituted at the Django Storage boundary, which would break on any surviving S3 assumption. A further test parses the AST of the three transport modules to assert none imports a provider SDK and none reads CARE_STORAGE_BACKEND. It inspects the AST rather than the text because those modules legitimately mention boto3 and the backend classes in prose describing what they replaced. Schema: the request is multipart/form-data only, `file` is string/binary, `file_data` is absent, and `original_name` is not required. Also adds BinaryFileField. drf-spectacular renders a plain FileField as format: uri, because DRF serialises it to a URL on output; in a multipart request body it is raw bytes. Annotating the field is preferable to enabling COMPONENT_SPLIT_REQUEST, which would reshape every schema in the project. Co-Authored-By: Claude <noreply@anthropic.com>
ADR-0002 - Implementation checklist complete: base64 removed, multipart implemented, upload handlers verified, size limits configured, schema implemented, authorization tested, MinIO round trip verified, provider-neutral behaviour verified, ES-02 complete. - Notes that CARE never materialises the file, that range requests remain undesigned despite FileResponse advertising them, and that POST /files/ plus mark_upload_completed are now vestigial. frontend-file-flow.md - 11.3: base64 upload marked removed. - New section 12: flow status table (signed upload, signed download, unsigned bucket URLs, base64 all removed; multipart upload and server-mediated download implemented), the multipart request contract with each part, the FormData migration snippet for the external frontend repository, and the memory/size table. - Records that base64 inflated a 5 MB file past DATA_UPLOAD_MAX_MEMORY_SIZE, so the old endpoint could not accept a file at its own documented limit over JSON. - Records what ES-02 did not change: cover images and avatars already used ordinary multipart and never touched the base64 endpoint; authorization, naming, alias selection and persistence are untouched. storage-call-sites.md - Call site 12 now covers transport as well as persistence. - The base64 whole-file read is marked gone. - Removes the note describing base64 as a remaining transport. 07-configuration-reference.md - Section 15 corrected. It specified CARE_MAX_UPLOAD_SIZE, CARE_ALLOWED_UPLOAD_MIME_TYPES, CARE_ALLOWED_UPLOAD_EXTENSIONS and CARE_BLOCKED_UPLOAD_EXTENSIONS; none exists. Replaced with the real MAX_FILE_UPLOAD_SIZE (megabytes, not bytes), the two explicit Django memory settings, ALLOWED_MIME_TYPES with the note that the value is sniffed rather than taken from the request, and a pointer to FileNameValidator for extension policy. - 16.1 corrected: inline behaviour comes from SAFE_INLINE_FORMATS in code, not a CARE_INLINE_MIME_TYPES setting. Co-Authored-By: Claude <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
📝 WalkthroughWalkthroughChangesPortable storage and file transport
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 14 UNAVAILABLE: read ECONNRESET 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 Warning |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 17
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (17)
docs/xii/architecture/inventory/cache-and-redis.md-491-491 (1)
491-491: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the excluded-call total.
Line 491 says that test-only calls are excluded from 67. Section 5 totals 69 at Line 470. State the correct total or show the calculation that produces 67. The inventory count is otherwise not auditable.
🤖 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 `@docs/xii/architecture/inventory/cache-and-redis.md` at line 491, Correct the excluded-call total in the “verified” statement near Section 5 so it matches the documented total of 69, or add the explicit calculation showing how exclusions produce 67. Ensure the inventory count is internally consistent and auditable.docs/xii/prompts/00-Complete-CARE-Runtime-Inventory-41-52 (1)
41-52: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the prompt to use the active documentation paths.
This prompt still directs work to
docs/gcp/. The architecture documents and inventories now live underdocs/xii/architecture/. Replace the old root in the reading list, required outputs, contradiction record, and baseline destination. Otherwise a future run will create duplicate obsolete documents.
unresolved-items.mdalready records the corrected architecture path.Also applies to: 90-101, 310-335, 368-372
🤖 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 `@docs/xii/prompts/00-Complete-CARE-Runtime-Inventory` around lines 41 - 52, Update the prompt’s documentation references from docs/gcp/ to docs/xii/architecture/ throughout the reading list, required outputs, contradiction record, and baseline destination, including the sections around the identified ranges. Preserve the existing filenames and use the already-corrected architecture path recorded in unresolved-items.md.docs/xii/architecture/inventory/plugin-impact.md-120-124 (1)
120-124: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDescribe the
ADDITIONAL_PLUGSmismatch direction correctly.A build-time plugin set that is a superset of the runtime set does not cause
ModuleNotFoundError. The failure occurs when the runtime set contains a plugin that the image did not install. Keep equality as a reproducibility requirement, but document the actual directional startup failure.🤖 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 `@docs/xii/architecture/inventory/plugin-impact.md` around lines 120 - 124, Update the `ADDITIONAL_PLUGS` explanation to state that `ModuleNotFoundError` occurs when the runtime plugin set includes a plugin absent from the build-time installed set; do not claim that build-time-only plugins cause this failure. Retain the requirement that build-time and runtime values be identical for reproducibility.docs/xii/architecture/00-scope-and-goals.md-633-649 (1)
633-649: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMake the signed-URL rule conditional.
The target runtime removes signed upload and download URLs and requires CARE-mediated transport. Keep this requirement only if an exceptional signed-URL flow remains. Otherwise, remove it to avoid directing implementers toward the retired transport.
🤖 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 `@docs/xii/architecture/00-scope-and-goals.md` around lines 633 - 649, Update the Security Scope signed-URL requirement to be conditional on an exceptional signed-URL flow remaining; otherwise remove the “Signed URLs MUST use short, documented expiration periods” rule so the scope reflects CARE-mediated transport.docs/xii/architecture/00-scope-and-goals.md-475-480 (1)
475-480: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winIdentify the component that scales to zero correctly.
Cloud Tasks is a managed queue. The private Cloud Run worker is the component that scales to zero. Replace
Cloud Tasks consumerswithprivate Cloud Run workerin this cost classification.🤖 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 `@docs/xii/architecture/00-scope-and-goals.md` around lines 475 - 480, Update the “Services expected to scale to zero” list to replace “Cloud Tasks consumers” with “private Cloud Run worker,” while preserving the other entries unchanged.docs/xii/architecture/03-migration-plan.md-777-786 (1)
777-786: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winName the Pipenv files in the dependency step.
State that this phase updates
PipfileandPipfile.lock. “Update the repository lockfile” is ambiguous and does not identify the package-management files that CI and contributors must change.As per coding guidelines, use Pipenv with
PipfileandPipfile.lock.🤖 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 `@docs/xii/architecture/03-migration-plan.md` around lines 777 - 786, Update the “40. Dependencies” section to explicitly state that adding django-storages requires updating both Pipfile and Pipfile.lock, replacing the ambiguous “repository lockfile” wording while preserving the listed backends.Source: Coding guidelines
docs/xii/architecture/00-scope-and-goals.md-845-851 (1)
845-851: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse one valid path for all architecture-document references.
The supplied documents are under
docs/xii/architecture/, but the references use bothdocs/xii/gcp/anddocs/gcp/. These paths will break navigation and dependency metadata unless duplicate documents exist.
docs/xii/architecture/00-scope-and-goals.md#L845-L851: point the next-document link todocs/xii/architecture/01-current-runtime.md.docs/xii/architecture/02-target-runtime.md#L17-L34: update the current-runtime and migration-plan references.docs/xii/architecture/02-target-runtime.md#L1874-L1880: update the migration-plan next-document link.docs/xii/architecture/03-migration-plan.md#L9-L12: update all dependency paths.docs/xii/architecture/03-migration-plan.md#L2008-L2014: update the testing-strategy next-document link.docs/xii/architecture/04-testing.md#L9-L13: update all dependency paths.docs/xii/architecture/04-testing.md#L1960-L1966: update the upstream-sync next-document link.🤖 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 `@docs/xii/architecture/00-scope-and-goals.md` around lines 845 - 851, Use the canonical docs/xii/architecture/ path for every architecture-document reference: update the next-document link in docs/xii/architecture/00-scope-and-goals.md:845-851 to 01-current-runtime.md; update current-runtime and migration-plan references in docs/xii/architecture/02-target-runtime.md:17-34 and its migration-plan link at 1874-1880; update all dependency paths in docs/xii/architecture/03-migration-plan.md:9-12 and its testing-strategy link at 2008-2014; and update all dependency paths in docs/xii/architecture/04-testing.md:9-13 and its upstream-sync link at 1960-1966. Remove inconsistent docs/xii/gcp/ and docs/gcp/ references without changing document ordering or link targets.docs/xii/architecture/04-testing.md-81-92 (1)
81-92: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDefine distinct GCP test profiles.
The target document defines the default GCP profile with PostgreSQL cache. Therefore,
GCP default profileandGCP PostgreSQL-cache profilecurrently describe the same configuration. Rename one profile or define a distinct backend combination, such as a LocMem-cache profile.🤖 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 `@docs/xii/architecture/04-testing.md` around lines 81 - 92, Update the supported deployment profile list in “Test supported deployment profiles” so the default GCP profile and GCP PostgreSQL-cache profile represent distinct configurations: either rename one appropriately or define the default GCP profile with a different backend combination such as LocMem cache. Keep the PostgreSQL-cache, optional Redis, and conditionally accepted PostgreSQL queue coverage requirements intact.docs/xii/implementation/ES-02-file-transport-modernization.md-4-6 (1)
4-6: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winSet the ES status to completed.
Line 4 marks ES-02 as
Draft. ADR-0002 records ES-02 as complete on August 7, 2026. The frontend inventory also records the implementation as complete. Update this status to prevent conflicting delivery documentation.🤖 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 `@docs/xii/implementation/ES-02-file-transport-modernization.md` around lines 4 - 6, Update the ES-02 status in the implementation document from Draft to Completed, leaving the related ADR and dependency metadata unchanged.docs/xii/adr/ADR-0002-file-transport.md-1-1 (1)
1-1: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the outer Markdown fences.
Both files are Markdown documents. The opening
```markdownfence and final closing fence make the complete document render as code instead of Markdown.
docs/xii/adr/ADR-0002-file-transport.md#L1-L1: remove the opening and closing outer fence.docs/xii/implementation/ES-02-file-transport-modernization.md#L1-L1: remove the opening and closing outer fence.🤖 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 `@docs/xii/adr/ADR-0002-file-transport.md` at line 1, Remove the outer Markdown opening and closing fences from docs/xii/adr/ADR-0002-file-transport.md (line 1) and docs/xii/implementation/ES-02-file-transport-modernization.md (line 1), leaving each document’s inner Markdown content unchanged.docs/xii/adr/ADR-0002-file-transport.md-242-250 (1)
242-250: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse the configured upload-limit setting name.
Both documents name
CARE_MAX_UPLOAD_SIZE, butconfig/settings/config.pyreadsMAX_FILE_UPLOAD_SIZE. An operator who sets the documented variable will silently retain the 5 MB default.
docs/xii/adr/ADR-0002-file-transport.md#L242-L250: replaceCARE_MAX_UPLOAD_SIZEwithMAX_FILE_UPLOAD_SIZE.docs/xii/implementation/ES-02-file-transport-modernization.md#L419-L427: replaceCARE_MAX_UPLOAD_SIZEwithMAX_FILE_UPLOAD_SIZE.🤖 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 `@docs/xii/adr/ADR-0002-file-transport.md` around lines 242 - 250, Replace CARE_MAX_UPLOAD_SIZE with MAX_FILE_UPLOAD_SIZE in the examples at docs/xii/adr/ADR-0002-file-transport.md:242-250 and docs/xii/implementation/ES-02-file-transport-modernization.md:419-427, ensuring both documents use the setting name read by config/settings/config.py.docs/xii/architecture/inventory/frontend-file-flow.md-387-466 (1)
387-466: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the ES-01 inventory chronology.
Section 11 is dated August 6, 2026 and says that no transport field changed. It then records
download_url, removed signed URLs, removed base64 upload, and completed ES-02 work. Section 12 records those ES-02 results on August 7, 2026. Keep Section 11 limited to ES-01 results, or move the later claims into Section 12.🤖 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 `@docs/xii/architecture/inventory/frontend-file-flow.md` around lines 387 - 466, Correct Section 11’s chronology by removing or relocating claims about ES-02 outcomes, including multipart upload replacing base64, removed signed upload/download fields, and completed ES-02 items such as U2, U3, U4, and S1. Keep Section 11 limited to ES-01 changes dated August 6, and place later transport and upload results in Section 12 dated August 7.docs/xii/architecture/inventory/storage-call-sites.md-368-371 (1)
368-371: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe totals do not reconcile with the table above.
Counting the call sites marked removed in §11.1 gives 8, not 6: sites 1, 2, 7, 19, plus the four sites collapsed into the
13-16row. The migrated count of 11 also includes the two URL builders (facility.py,users/models.py), which are not part of the 19 sites enumerated in §3, so11 + 2 + 6 = 19only looks right by coincidence. Please restate the totals against a fixed denominator, for example 21 tracked sites = 11 migrated + 2 temporary wrapper + 8 removed.Separately, line 510 writes
unresolved-items.mdS2 where the rest of the document uses§2.🤖 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 `@docs/xii/architecture/inventory/storage-call-sites.md` around lines 368 - 371, The summary totals must reconcile with the documented call-site inventory: update the totals to use a fixed denominator of 21 tracked sites, comprising 11 migrated, 2 temporary wrapper, and 8 removed, while clarifying that the two URL builders are included in the migrated count. Also correct the unresolved-items reference near `unresolved-items.md` from `S2` to `§2`.care/emr/tests/test_file_upload_multipart.py-299-305 (1)
299-305: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThis test does not assert what its name and comment claim.
test_incomplete_rows_remain_cleanablenever produces an incomplete row and never invokescleanup_incomplete_file_uploads. It assertsupload_completed is Trueand that the object exists — the successful path, already covered bytest_record_is_correctandtest_uses_the_patient_alias_and_es01_object_name.The invariant in the comment is that the multipart path sets
upload_completed=Falsebefore writing the object. That is the part worth protecting: if a future change sets the flag toTrueup front, storage failures would leave orphaned objects that the cleanup task can no longer find, and this test would still pass. Consider asserting the intermediate state instead, for example by capturingupload_completedinside aput_objectspy.💚 Sketch
def test_incomplete_rows_remain_cleanable(self): - # cleanup_incomplete_file_uploads keys off upload_completed=False, which - # the multipart path still sets before the object is written. - response = self.upload() - file_obj = FileUpload.objects.get(external_id=response.data["id"]) - self.assertTrue(file_obj.upload_completed) - self.assertTrue(file_obj.files_manager.exists(file_obj)) + # cleanup_incomplete_file_uploads keys off upload_completed=False, so + # the row must be incomplete while the object is being written. + seen = {} + original = FileUpload.files_manager.__class__.put_object + + def spy(manager, file_obj, file, content_type=None): + seen["completed_during_write"] = FileUpload.objects.get( + id=file_obj.id + ).upload_completed + return original(manager, file_obj, file, content_type=content_type) + + with patch.object(FileUpload.files_manager.__class__, "put_object", spy): + response = self.upload() + self.assertFalse(seen["completed_during_write"]) + file_obj = FileUpload.objects.get(external_id=response.data["id"]) + self.assertTrue(file_obj.upload_completed)🤖 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 `@care/emr/tests/test_file_upload_multipart.py` around lines 299 - 305, Update test_incomplete_rows_remain_cleanable to observe the FileUpload state during the files_manager.put_object operation, using a spy or mock that captures upload_completed before the object is written. Assert the captured intermediate value is False, and remove the existing successful-upload assertions that duplicate other tests.care/emr/resources/file_upload/spec.py-112-115 (1)
112-115: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReturn
download_urlonly for completed objects.Both retrieve serializers assign a CARE download route while the model can still have
upload_completed=False. Clients then receive a usable-looking URL for an object that is not available.
care/emr/resources/file_upload/spec.py#L112-L115: Setmapping["download_url"]tofile_download_url(obj)only whenobj.upload_completedis true.care/emr/resources/report/report_upload/spec.py#L49-L52: Setmapping["download_url"]toreport_download_url(obj)only whenobj.upload_completedis true.🤖 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 `@care/emr/resources/file_upload/spec.py` around lines 112 - 115, Update perform_extra_serialization in care/emr/resources/file_upload/spec.py at lines 112-115 to assign download_url only when obj.upload_completed is true; leave it absent or unchanged for incomplete uploads. Apply the same conditional behavior in care/emr/resources/report/report_upload/spec.py at lines 49-52, using report_download_url for completed report uploads.docs/xii/adr/ADR-0001-django-storage.md-319-346 (1)
319-346: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winSeparate IS-01 status from IS-02 transport delivery.
Lines 178-195 define file transport as outside ADR-0001. Lines 326-339 then list signed URL removal and CARE-mediated transport as IS-01 delivery while Line 323 says IS-02 is incomplete.
Move the transport statements to ADR-0002, or identify them as partial IS-02 delivery. Keep IS-01 status limited to storage persistence.
🤖 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 `@docs/xii/adr/ADR-0001-django-storage.md` around lines 319 - 346, The ADR-0001 implementation status incorrectly attributes file transport changes to IS-01 while IS-02 remains incomplete. Update the status and “What IS-01 delivered” section to cover only Django Storage persistence and provider configuration, and move the signed-URL removal, CARE-mediated object serving, and base64-to-multipart transport scope to ADR-0002 or explicitly mark them as partial IS-02 delivery.docs/xii/architecture/05-upstream-sync.md-10-15 (1)
10-15: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse one consistent documentation path namespace.
All three documents reference
docs/gcp/..., but the supplied files usedocs/xii/architecture/.... Update every affected dependency, next-document link, runbook path, and command example.
docs/xii/architecture/05-upstream-sync.md#L10-L15: updatedepends_onand remainingdocs/gcpreferences.docs/xii/architecture/06-operations.md#L9-L15: updatedepends_on, the next-document link, and runbook paths.docs/xii/architecture/07-configuration-reference.md#L9-L16: updatedepends_onand the next-document link.🤖 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 `@docs/xii/architecture/05-upstream-sync.md` around lines 10 - 15, Use the docs/xii/architecture namespace consistently: in docs/xii/architecture/05-upstream-sync.md lines 10-15, update depends_on and all remaining docs/gcp references; in docs/xii/architecture/06-operations.md lines 9-15, update depends_on, the next-document link, and runbook paths; in docs/xii/architecture/07-configuration-reference.md lines 9-16, update depends_on and the next-document link.
🧹 Nitpick comments (5)
care/emr/tests/test_file_upload_api.py (1)
75-114: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
test_direct_file_uploadnow duplicatestest_upload_patient_file.Both tests post
self.upload_payload()tofiles-upload-fileand then download the result. The first one already asserts everything the second does, plus the route shape and the headers. When the two upload paths were distinct the split made sense; after the multipart migration there is only one. Consider droppingtest_direct_file_uploador narrowing it to the one thing it uniquely covers, which is readingdownload_urlstraight off the upload response.🤖 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 `@care/emr/tests/test_file_upload_api.py` around lines 75 - 114, Remove the duplicated download/content assertions from test_direct_file_upload, since test_upload_patient_file already covers the upload and download behavior. Either delete test_direct_file_upload or narrow it to validating only that the upload response’s download_url can be used directly.care/emr/tests/test_file_upload_multipart.py (2)
42-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRun
ruff format .over this file.Line 42 is about 102 characters, past Ruff's default 88-column limit; lines 90-92 and 334 look similar. The formatter would rewrap them. It is the sort of thing CI notices.
As per coding guidelines: "After code changes, run 'ruff check --fix .' and 'ruff format .' to lint and format 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 `@care/emr/tests/test_file_upload_multipart.py` at line 42, Run Ruff formatting on the entire test file, using ruff format ., so the upload method signature and other overlong lines are wrapped to the configured style; also apply the project’s required ruff check --fix . command.Source: Coding guidelines
387-396: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winGenerate the schema once per class.
upload_operation()callsschema(), and three of the four tests call both, so each of those tests builds the full OpenAPI document twice. Over CARE's router that is not cheap. Caching it on the class removes eight generations, and the schema does not vary between these tests anyway.While you are in here:
"/api/v1/files/upload-file/"could bereverse("files-upload-file"), which keeps the key in sync if the route ever moves.♻️ Proposed refactor
class UploadSchemaTests(CareAPITestBase): """The generated OpenAPI schema must describe multipart, not JSON.""" - def schema(self): - from drf_spectacular.generators import SchemaGenerator - - return SchemaGenerator().get_schema(request=None, public=True) + `@classmethod` + def setUpClass(cls): + super().setUpClass() + from drf_spectacular.generators import SchemaGenerator + + cls._schema = SchemaGenerator().get_schema(request=None, public=True) + + def schema(self): + return self._schema def upload_operation(self): - return self.schema()["paths"]["/api/v1/files/upload-file/"]["post"] + return self.schema()["paths"][reverse("files-upload-file")]["post"]🤖 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 `@care/emr/tests/test_file_upload_multipart.py` around lines 387 - 396, Update UploadSchemaTests.schema to cache the generated OpenAPI document once on the test class and reuse it across all tests, avoiding repeated SchemaGenerator().get_schema calls. Update upload_operation to derive the path key with reverse("files-upload-file") instead of the hard-coded URL while preserving the existing POST operation lookup.care/emr/tests/test_storage.py (1)
186-199: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGuard
MinioIntegrationTestsagainst non-MinIO configurations.These tests need a live MinIO on the configured endpoint, and
test_aliases_are_s3_storagehard-asserts the backend class name. Both fail wheneverCARE_STORAGE_BACKEND=gcs, or in any environment that runs the suite without the compose service. The PR itself makes the backend configurable, so pinning the test suite to one provider works against that. A skip condition keeps the local profile mandatory without breaking the others.♻️ Suggested guard
class MinioIntegrationTests(SimpleTestCase): + def setUp(self): + super().setUp() + if settings.CARE_STORAGE_BACKEND != "s3": + self.skipTest("MinIO integration requires the s3 backend")🤖 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 `@care/emr/tests/test_storage.py` around lines 186 - 199, Guard MinioIntegrationTests with a skip condition that runs only when the configured storage backend is MinIO and the required local service is available. Apply the guard to the class or test setup so test_aliases_are_s3_storage and related integration tests are skipped for configurations such as CARE_STORAGE_BACKEND=gcs or unavailable compose services, while preserving mandatory execution for the local MinIO profile.care/emr/api/viewsets/file_assets.py (1)
26-53: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAdd caching headers, and consider throttling, on these anonymous routes.
Every cover image and avatar render now costs CARE one database query plus one object-storage GET, where the bucket or CDN previously absorbed that traffic. The routes are unauthenticated, so the cost is open to anyone. Two things would help:
- Set
Cache-Controlon the response. Object keys already carry a random token (<folder>/<external_id>_<token>.<ext>), so the content at a given key is immutable and can be cached hard by browsers and any proxy in front of CARE.- Apply a throttle class, since
permission_classes = ()also removes the default throttling that other views inherit.Neither is a correctness problem today. Both matter once these routes carry real render traffic.
🤖 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 `@care/emr/api/viewsets/file_assets.py` around lines 26 - 53, Update PublicAssetView.serve to add a long-lived immutable Cache-Control header to the response returned by storage_file_response, leveraging the existing tokenized object keys. Add an appropriate throttle class to PublicAssetView so both FacilityCoverImageView and UserProfilePictureView retain rate limiting despite using empty permission_classes; reuse the project’s established throttle symbol rather than defining a new policy.
🤖 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 `@care/emr/utils/file_download.py`:
- Around line 34-56: Update storage_file_response to set the
X-Content-Type-Options response header to nosniff on every returned
FileResponse, while preserving the existing MIME type and attachment behavior.
Use the framework’s standard response-header mechanism and verify any shared
security setting remains enabled if this helper relies on it.
In `@care/facility/models/facility.py`:
- Around line 208-219: The four file URL fields have inconsistent or
undocumented relative-versus-absolute behavior. Decide on one URL convention for
facility.read_cover_image_url, users.read_profile_picture_url, and
emr.utils.file_download.file_download_url/report_download_url; either join
reversed paths with the configured API/site base to preserve absolute URLs, or
update the frontend and both architecture documents to explicitly support
relative paths, ensuring all four fields follow the same convention.
In `@docs/xii/adr/ADR-0003-asynchronous-execution.md`:
- Around line 122-128: Update ADR-0003's database-dependent asynchronous
dispatch requirement to use SHALL instead of SHOULD, making post-commit dispatch
mandatory. Require transaction.on_commit(...) or an equivalent transactional
outbox for every such dispatch, while preserving the requirement that tasks
cannot observe state from a transaction that later rolls back.
- Around line 199-214: Update the “Idempotency” section to require every
asynchronous task handler to make an explicit idempotency decision, replacing
the vague “where required” wording. Require each handler to declare an
idempotency key, unique constraint, conditional update, execution record, or
explicit proof that the operation is intrinsically idempotent.
In `@docs/xii/adr/ADR-0004-configurable-application-cache.md`:
- Around line 13-26: Update the ADR’s responsibility classification to separate
rate-limit counters from ordinary cache behavior, and define their contract
covering scope, atomic updates, expiration, and failure behavior. Ensure the
decision and related configuration guidance identify the required consistency
guarantees when using LocMem or another shared cache profile.
In `@docs/xii/adr/ADR-0005-distributed-locking.md`:
- Around line 88-105: Update the “Redis locks” requirements in ADR-0005 to
address stale owners after lease expiration: require owner-token validation when
releasing locks and fencing tokens or conditional writes for lock-protected
operations, or explicitly restrict Redis locking to operations whose correctness
is enforced by the database.
In `@docs/xii/adr/ADR-0007-terraform-for-GCP.md`:
- Around line 99-111: Update the “Plans and review” section of ADR-0007 to make
reviewed Terraform plans mandatory for production applies: require the exact
approved plan artifact to be consumed and explicitly reject direct applies or
unreviewed/different plans, aligning the requirement with ADR-0008’s controlled
delivery process.
In `@docs/xii/adr/ADR-0008-automated-continuous-integration.md`:
- Around line 118-130: Update the ADR’s Rollback section to define schema
compatibility after migrations: require forward-compatible database migrations
and a tested application rollback procedure, or require verification of schema
compatibility before deploying a previous immutable image. Preserve the existing
metadata requirements and the rule that database rollback is not automatic.
- Around line 75-98: Update the deployment requirements in the controlled
deployment sequence and Migrations section to mandate backward-compatible
migrations while older API and worker versions remain active. Make
worker-before-API deployment mandatory, and require workers to accept both
queued and retried payload formats before the API emits new task payloads; make
expand-and-contract migration techniques required where schema changes affect
multiple deployed versions.
In `@docs/xii/architecture/02-target-runtime.md`:
- Around line 529-533: Update the report-generation retry flow in
care/emr/tasks/report_generation.py to use a storage-provider-independent retry
contract, or translate GCS backend failures into the existing retryable error
type so transient uploads retry under GCS as well as AWS. Add a test covering a
transient GCS upload failure and verify the configured retry behavior, then
update the target-runtime documentation and inventory entry to reflect the
implemented contract.
In `@docs/xii/architecture/07-configuration-reference.md`:
- Around line 301-316: Update the DJANGO_ALLOWED_HOSTS production example to
list only the exact CARE Cloud Run service hostname and approved custom domains,
removing the broad “.run.app” entry. If the exact hostnames are unavailable,
revise the documentation to require verification that the ingress layer rejects
all other Host values before presenting the configuration as the normal
production setup.
- Around line 2341-2350: Update the local profile configuration near
CARE_TRANSIENT_STATE_BACKEND to remove the obsolete S3_ENDPOINT_URL,
S3_ACCESS_KEY, and S3_SECRET_KEY variables, replacing them with the documented
BUCKET_* fallback or appropriate FILE_UPLOAD_* and FACILITY_S3_* variables from
Section 13.1 so MinIO remains configurable.
- Around line 2414-2430: Update the “Test Profile” configuration to use only
backend values supported by the documented contracts, especially replacing or
removing CARE_STORAGE_BACKEND=filesystem and CARE_TASK_BACKEND=fake. If these
are intended as test-only parser values, document them explicitly in the
corresponding backend sections and ensure startup validation accepts them;
otherwise move the choices into the test settings using the established
InMemoryStorage and supported task-backend configuration.
In `@docs/xii/architecture/inventory/runtime-and-deployment.md`:
- Around line 325-354: Update the dependency and GCP inventory section to
reflect the PR’s addition of django-storages and provider-neutral Django Storage
configuration, including the dependency table and repository search results.
Revise the “GCP support does not exist” conclusion to distinguish the new
configuration from any still-absent GCP infrastructure; if retaining the prior
findings, label them explicitly as the historical Phase 0 baseline.
In `@docs/xii/architecture/inventory/task-call-sites.md`:
- Around line 275-279: Update the recursive call in handle_cascade so it passes
child.pk when the function uses the value as parent_id, or change the query to
use the parent relation with the FacilityLocation instance. Add a regression
test covering cascading through a two-level FacilityLocation hierarchy.
- Around line 35-37: Update the asynchronous task count in the inventory
statement from 3 to 4, while retaining the existing list of four tasks and the
recursive-tail qualification for summarise_monetary_components.
In `@docs/xii/architecture/inventory/unresolved-items.md`:
- Around line 96-104: Update the resolved file-transport entries, including the
sections corresponding to the signed-URL, base64-upload, direct bucket URL, and
legacy bucket-config paths, to mark replaced behavior as resolved and document
the multipart upload plus CARE-mediated download/image routes. Remove pending
work that targets the retired contracts, retaining only migration questions for
clients still using the legacy contract.
---
Minor comments:
In `@care/emr/resources/file_upload/spec.py`:
- Around line 112-115: Update perform_extra_serialization in
care/emr/resources/file_upload/spec.py at lines 112-115 to assign download_url
only when obj.upload_completed is true; leave it absent or unchanged for
incomplete uploads. Apply the same conditional behavior in
care/emr/resources/report/report_upload/spec.py at lines 49-52, using
report_download_url for completed report uploads.
In `@care/emr/tests/test_file_upload_multipart.py`:
- Around line 299-305: Update test_incomplete_rows_remain_cleanable to observe
the FileUpload state during the files_manager.put_object operation, using a spy
or mock that captures upload_completed before the object is written. Assert the
captured intermediate value is False, and remove the existing successful-upload
assertions that duplicate other tests.
In `@docs/xii/adr/ADR-0001-django-storage.md`:
- Around line 319-346: The ADR-0001 implementation status incorrectly attributes
file transport changes to IS-01 while IS-02 remains incomplete. Update the
status and “What IS-01 delivered” section to cover only Django Storage
persistence and provider configuration, and move the signed-URL removal,
CARE-mediated object serving, and base64-to-multipart transport scope to
ADR-0002 or explicitly mark them as partial IS-02 delivery.
In `@docs/xii/adr/ADR-0002-file-transport.md`:
- Line 1: Remove the outer Markdown opening and closing fences from
docs/xii/adr/ADR-0002-file-transport.md (line 1) and
docs/xii/implementation/ES-02-file-transport-modernization.md (line 1), leaving
each document’s inner Markdown content unchanged.
- Around line 242-250: Replace CARE_MAX_UPLOAD_SIZE with MAX_FILE_UPLOAD_SIZE in
the examples at docs/xii/adr/ADR-0002-file-transport.md:242-250 and
docs/xii/implementation/ES-02-file-transport-modernization.md:419-427, ensuring
both documents use the setting name read by config/settings/config.py.
In `@docs/xii/architecture/00-scope-and-goals.md`:
- Around line 633-649: Update the Security Scope signed-URL requirement to be
conditional on an exceptional signed-URL flow remaining; otherwise remove the
“Signed URLs MUST use short, documented expiration periods” rule so the scope
reflects CARE-mediated transport.
- Around line 475-480: Update the “Services expected to scale to zero” list to
replace “Cloud Tasks consumers” with “private Cloud Run worker,” while
preserving the other entries unchanged.
- Around line 845-851: Use the canonical docs/xii/architecture/ path for every
architecture-document reference: update the next-document link in
docs/xii/architecture/00-scope-and-goals.md:845-851 to 01-current-runtime.md;
update current-runtime and migration-plan references in
docs/xii/architecture/02-target-runtime.md:17-34 and its migration-plan link at
1874-1880; update all dependency paths in
docs/xii/architecture/03-migration-plan.md:9-12 and its testing-strategy link at
2008-2014; and update all dependency paths in
docs/xii/architecture/04-testing.md:9-13 and its upstream-sync link at
1960-1966. Remove inconsistent docs/xii/gcp/ and docs/gcp/ references without
changing document ordering or link targets.
In `@docs/xii/architecture/03-migration-plan.md`:
- Around line 777-786: Update the “40. Dependencies” section to explicitly state
that adding django-storages requires updating both Pipfile and Pipfile.lock,
replacing the ambiguous “repository lockfile” wording while preserving the
listed backends.
In `@docs/xii/architecture/04-testing.md`:
- Around line 81-92: Update the supported deployment profile list in “Test
supported deployment profiles” so the default GCP profile and GCP
PostgreSQL-cache profile represent distinct configurations: either rename one
appropriately or define the default GCP profile with a different backend
combination such as LocMem cache. Keep the PostgreSQL-cache, optional Redis, and
conditionally accepted PostgreSQL queue coverage requirements intact.
In `@docs/xii/architecture/05-upstream-sync.md`:
- Around line 10-15: Use the docs/xii/architecture namespace consistently: in
docs/xii/architecture/05-upstream-sync.md lines 10-15, update depends_on and all
remaining docs/gcp references; in docs/xii/architecture/06-operations.md lines
9-15, update depends_on, the next-document link, and runbook paths; in
docs/xii/architecture/07-configuration-reference.md lines 9-16, update
depends_on and the next-document link.
In `@docs/xii/architecture/inventory/cache-and-redis.md`:
- Line 491: Correct the excluded-call total in the “verified” statement near
Section 5 so it matches the documented total of 69, or add the explicit
calculation showing how exclusions produce 67. Ensure the inventory count is
internally consistent and auditable.
In `@docs/xii/architecture/inventory/frontend-file-flow.md`:
- Around line 387-466: Correct Section 11’s chronology by removing or relocating
claims about ES-02 outcomes, including multipart upload replacing base64,
removed signed upload/download fields, and completed ES-02 items such as U2, U3,
U4, and S1. Keep Section 11 limited to ES-01 changes dated August 6, and place
later transport and upload results in Section 12 dated August 7.
In `@docs/xii/architecture/inventory/plugin-impact.md`:
- Around line 120-124: Update the `ADDITIONAL_PLUGS` explanation to state that
`ModuleNotFoundError` occurs when the runtime plugin set includes a plugin
absent from the build-time installed set; do not claim that build-time-only
plugins cause this failure. Retain the requirement that build-time and runtime
values be identical for reproducibility.
In `@docs/xii/architecture/inventory/storage-call-sites.md`:
- Around line 368-371: The summary totals must reconcile with the documented
call-site inventory: update the totals to use a fixed denominator of 21 tracked
sites, comprising 11 migrated, 2 temporary wrapper, and 8 removed, while
clarifying that the two URL builders are included in the migrated count. Also
correct the unresolved-items reference near `unresolved-items.md` from `S2` to
`§2`.
In `@docs/xii/implementation/ES-02-file-transport-modernization.md`:
- Around line 4-6: Update the ES-02 status in the implementation document from
Draft to Completed, leaving the related ADR and dependency metadata unchanged.
In `@docs/xii/prompts/00-Complete-CARE-Runtime-Inventory`:
- Around line 41-52: Update the prompt’s documentation references from docs/gcp/
to docs/xii/architecture/ throughout the reading list, required outputs,
contradiction record, and baseline destination, including the sections around
the identified ranges. Preserve the existing filenames and use the
already-corrected architecture path recorded in unresolved-items.md.
---
Nitpick comments:
In `@care/emr/api/viewsets/file_assets.py`:
- Around line 26-53: Update PublicAssetView.serve to add a long-lived immutable
Cache-Control header to the response returned by storage_file_response,
leveraging the existing tokenized object keys. Add an appropriate throttle class
to PublicAssetView so both FacilityCoverImageView and UserProfilePictureView
retain rate limiting despite using empty permission_classes; reuse the project’s
established throttle symbol rather than defining a new policy.
In `@care/emr/tests/test_file_upload_api.py`:
- Around line 75-114: Remove the duplicated download/content assertions from
test_direct_file_upload, since test_upload_patient_file already covers the
upload and download behavior. Either delete test_direct_file_upload or narrow it
to validating only that the upload response’s download_url can be used directly.
In `@care/emr/tests/test_file_upload_multipart.py`:
- Line 42: Run Ruff formatting on the entire test file, using ruff format ., so
the upload method signature and other overlong lines are wrapped to the
configured style; also apply the project’s required ruff check --fix . command.
- Around line 387-396: Update UploadSchemaTests.schema to cache the generated
OpenAPI document once on the test class and reuse it across all tests, avoiding
repeated SchemaGenerator().get_schema calls. Update upload_operation to derive
the path key with reverse("files-upload-file") instead of the hard-coded URL
while preserving the existing POST operation lookup.
In `@care/emr/tests/test_storage.py`:
- Around line 186-199: Guard MinioIntegrationTests with a skip condition that
runs only when the configured storage backend is MinIO and the required local
service is available. Apply the guard to the class or test setup so
test_aliases_are_s3_storage and related integration tests are skipped for
configurations such as CARE_STORAGE_BACKEND=gcs or unavailable compose services,
while preserving mandatory execution for the local MinIO profile.
🪄 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: dec447aa-0fc4-4c29-b3f3-78c68b81c288
⛔ Files ignored due to path filters (1)
Pipfile.lockis excluded by!**/*.lock
📒 Files selected for processing (53)
Pipfilecare/emr/api/viewsets/file_assets.pycare/emr/api/viewsets/file_upload.pycare/emr/api/viewsets/report/report_upload.pycare/emr/models/file_upload.pycare/emr/models/report/report_upload.pycare/emr/reports/context_builder/data_points/fileupload.pycare/emr/reports/report_utils.pycare/emr/resources/file_upload/spec.pycare/emr/resources/report/report_upload/spec.pycare/emr/tasks/cleanup_incomplete_file_uploads.pycare/emr/tests/test_file_upload_api.pycare/emr/tests/test_file_upload_multipart.pycare/emr/tests/test_storage.pycare/emr/tests/test_storage_transport.pycare/emr/utils/file_download.pycare/emr/utils/file_manager.pycare/facility/models/facility.pycare/users/models.pycare/utils/csp/__init__.pycare/utils/csp/config.pycare/utils/file_uploads/cover_image.pycare/utils/tests/test_storage_config.pyconfig/api_router.pyconfig/settings/base.pyconfig/settings/config.pyconfig/storage.pydocs/xii/adr/ADR-0001-django-storage.mddocs/xii/adr/ADR-0002-file-transport.mddocs/xii/adr/ADR-0003-asynchronous-execution.mddocs/xii/adr/ADR-0004-configurable-application-cache.mddocs/xii/adr/ADR-0005-distributed-locking.mddocs/xii/adr/ADR-0006-portable-runtime-profiles.mddocs/xii/adr/ADR-0007-terraform-for-GCP.mddocs/xii/adr/ADR-0008-automated-continuous-integration.mddocs/xii/architecture/00-scope-and-goals.mddocs/xii/architecture/01-current-runtime.mddocs/xii/architecture/02-target-runtime.mddocs/xii/architecture/03-migration-plan.mddocs/xii/architecture/04-testing.mddocs/xii/architecture/05-upstream-sync.mddocs/xii/architecture/06-operations.mddocs/xii/architecture/07-configuration-reference.mddocs/xii/architecture/inventory/cache-and-redis.mddocs/xii/architecture/inventory/frontend-file-flow.mddocs/xii/architecture/inventory/plugin-impact.mddocs/xii/architecture/inventory/runtime-and-deployment.mddocs/xii/architecture/inventory/storage-call-sites.mddocs/xii/architecture/inventory/task-call-sites.mddocs/xii/architecture/inventory/unresolved-items.mddocs/xii/implementation/ES-01-storage.mddocs/xii/implementation/ES-02-file-transport-modernization.mddocs/xii/prompts/00-Complete-CARE-Runtime-Inventory
💤 Files with no reviewable changes (1)
- care/utils/csp/config.py
Comment on lines
+34
to
+56
| def storage_file_response(storage, name, *, filename, mime_type=None): | ||
| """ | ||
| Stream ``name`` from ``storage`` as an HTTP response. | ||
|
|
||
| ``FileResponse`` streams in chunks and closes the handle when the response | ||
| is finished, so the object is never fully buffered in memory. | ||
| """ | ||
| if not mime_type: | ||
| mime_type = mimetypes.guess_type(filename or name)[0] | ||
| mime_type = mime_type or "application/octet-stream" | ||
|
|
||
| try: | ||
| handle = storage.open(name, "rb") | ||
| except FileNotFoundError as e: | ||
| msg = "File not found in storage" | ||
| raise NotFound(msg) from e | ||
|
|
||
| return FileResponse( | ||
| handle, | ||
| as_attachment=mime_type not in SAFE_INLINE_FORMATS, | ||
| filename=filename, | ||
| content_type=mime_type, | ||
| ) |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Set X-Content-Type-Options: nosniff on this response.
as_attachment is False for every type in SAFE_INLINE_FORMATS, so those objects render inline on the CARE origin — the same origin that serves the API and its session cookie. The presigned URLs this replaces rendered inline on the bucket origin, so the blast radius of a content-type mismatch just changed.
The declared type is not always derived from the bytes. file_object_response trusts file_obj.meta["mime_type"], and rows created through POST /api/v1/files/ carry a client-supplied mime_type that is validated against the allowlist but never compared with what is later stored. A client can therefore store arbitrary bytes and have CARE serve them inline as image/png. nosniff stops a browser from reinterpreting them.
🔒️ Proposed fix
- return FileResponse(
+ response = FileResponse(
handle,
as_attachment=mime_type not in SAFE_INLINE_FORMATS,
filename=filename,
content_type=mime_type,
)
+ response.headers["X-Content-Type-Options"] = "nosniff"
+ return responseRelated: the change details for config/settings/base.py line 18 say the CSP configuration import was replaced with the storage constants. If that removed a Content-Security-Policy header that used to cover these responses, the second layer of defence went with it. Worth confirming.
#!/bin/bash
# Check whether CSP configuration survives elsewhere in settings.
rg -n -C3 'csp|CONTENT_SECURITY_POLICY|SECURE_CONTENT_TYPE_NOSNIFF' --iglob '*.py' config/🤖 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 `@care/emr/utils/file_download.py` around lines 34 - 56, Update
storage_file_response to set the X-Content-Type-Options response header to
nosniff on every returned FileResponse, while preserving the existing MIME type
and attachment behavior. Use the framework’s standard response-header mechanism
and verify any shared security setting remains enabled if this helper relies on
it.
Comment on lines
208
to
+219
| def read_cover_image_url(self): | ||
| """ | ||
| CARE route serving the cover image (ADR-0001). | ||
|
|
||
| Never a storage-provider URL: the bytes are read through Django Storage | ||
| so the bucket can stay private and the provider interchangeable. | ||
| """ | ||
| if self.cover_image_url: | ||
| if settings.FACILITY_CDN: | ||
| return f"{settings.FACILITY_CDN}/{self.cover_image_url}" | ||
| return f"{settings.FACILITY_S3_BUCKET_EXTERNAL_ENDPOINT}/{settings.FACILITY_S3_BUCKET}/{self.cover_image_url}" | ||
| return reverse( | ||
| "facility-cover-image-asset", | ||
| kwargs={"external_id": self.external_id}, | ||
| ) |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Both public URL fields changed from absolute to relative, which is an unannounced API contract break. reverse returns a path such as /api/v1/assets/facility/<external_id>/cover_image/. The values these replace were absolute URLs built from the bucket external endpoint. A frontend served from a different origin than the API, or any consumer that used the field directly as an image src without a base, will not resolve the new value. The PR objectives state the frontend must migrate to the new download contract, but the relative-versus-absolute distinction is not recorded in docs/xii/architecture/inventory/frontend-file-flow.md or in §11.1a of docs/xii/architecture/inventory/storage-call-sites.md.
care/facility/models/facility.py#L208-L219: return an absolute URL by joiningreverse(...)with the configured site or API base, or document that the field is now relative and confirm the frontend resolves it against the API origin.care/users/models.py#L201-L212: apply the same decision toread_profile_picture_urlso both fields keep one shape.
Note that care/emr/utils/file_download.py file_download_url and report_download_url return relative paths too, so whichever convention you pick should cover all four.
📍 Affects 2 files
care/facility/models/facility.py#L208-L219(this comment)care/users/models.py#L201-L212
🤖 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 `@care/facility/models/facility.py` around lines 208 - 219, The four file URL
fields have inconsistent or undocumented relative-versus-absolute behavior.
Decide on one URL convention for facility.read_cover_image_url,
users.read_profile_picture_url, and
emr.utils.file_download.file_download_url/report_download_url; either join
reversed paths with the configured API/site base to preserve absolute URLs, or
update the frontend and both architecture documents to explicitly support
relative paths, ensuring all four fields follow the same convention.
Comment on lines
+122
to
+128
| When a task depends on a database change, dispatch SHOULD occur after successful commit using Django's transaction facilities, such as: | ||
|
|
||
| ```python | ||
| transaction.on_commit(...) | ||
| ``` | ||
|
|
||
| A task SHALL not observe state that was subsequently rolled back. |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make post-commit dispatch mandatory.
Line 122 uses SHOULD, but Line 128 requires that tasks never observe rolled-back state. A task can run before commit, observe old or missing state, and continue after the transaction rolls back. Change this requirement to SHALL and require transaction.on_commit(...) or an equivalent transactional outbox for every database-dependent asynchronous dispatch.
🤖 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 `@docs/xii/adr/ADR-0003-asynchronous-execution.md` around lines 122 - 128,
Update ADR-0003's database-dependent asynchronous dispatch requirement to use
SHALL instead of SHOULD, making post-commit dispatch mandatory. Require
transaction.on_commit(...) or an equivalent transactional outbox for every such
dispatch, while preserving the requirement that tasks cannot observe state from
a transaction that later rolls back.
Comment on lines
+199
to
+214
| ## Idempotency | ||
|
|
||
| Task execution SHALL be treated as at-least-once. | ||
|
|
||
| Handlers SHALL tolerate duplicate delivery where required. | ||
|
|
||
| Idempotency SHOULD rely on: | ||
|
|
||
| - existing database state; | ||
| - unique constraints; | ||
| - conditional updates; | ||
| - explicit idempotency keys; | ||
| - execution records; | ||
| - object existence where appropriate. | ||
|
|
||
| Redis SHALL not be the sole correctness mechanism. |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Require an idempotency decision for every asynchronous task.
At-least-once delivery makes duplicate execution normal. The phrase “where required” does not define an approval rule. Require every handler to declare an idempotency key, unique constraint, conditional update, execution record, or an explicit proof that the operation is intrinsically idempotent.
🤖 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 `@docs/xii/adr/ADR-0003-asynchronous-execution.md` around lines 199 - 214,
Update the “Idempotency” section to require every asynchronous task handler to
make an explicit idempotency decision, replacing the vague “where required”
wording. Require each handler to declare an idempotency key, unique constraint,
conditional update, execution record, or explicit proof that the operation is
intrinsically idempotent.
Comment on lines
+13
to
+26
| Repository inspection identified multiple operations that appear related to Redis but do not all represent ordinary cache behavior: | ||
|
|
||
| - standard cache reads and writes; | ||
| - report-progress values; | ||
| - rate-limit counters; | ||
| - `cache.set(..., nx=True)` used as lock-like behavior; | ||
| - `cache.delete_pattern(...)`; | ||
| - direct `get_redis_connection()` access; | ||
| - Celery broker and result storage; | ||
| - health checks. | ||
|
|
||
| A backend swap from Redis to PostgreSQL or LocMem cannot safely replace all these responsibilities. | ||
|
|
||
| The existing LocMem shim accepts an `nx` argument while always returning success, silently removing mutual exclusion. This demonstrates that cache configuration and distributed locking must be separated. |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Classify rate-limit counters outside ordinary cache semantics.
Line 17 lists rate-limit counters as a Redis responsibility, but the decision does not define their required consistency or atomicity. If a rate limit is a security control, a switch to LocMem or another shared cache profile can weaken enforcement. Define a separate rate-limit contract for scope, atomic updates, expiration, and failure behavior.
Also applies to: 54-60
🤖 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 `@docs/xii/adr/ADR-0004-configurable-application-cache.md` around lines 13 -
26, Update the ADR’s responsibility classification to separate rate-limit
counters from ordinary cache behavior, and define their contract covering scope,
atomic updates, expiration, and failure behavior. Ensure the decision and
related configuration guidance identify the required consistency guarantees when
using LocMem or another shared cache profile.
Comment on lines
+2414
to
+2430
| # 48. Test Profile | ||
|
|
||
| Conceptual fast test configuration: | ||
|
|
||
| ```text | ||
| CARE_TASK_BACKEND=fake | ||
| CARE_CACHE_BACKEND=dummy | ||
| CARE_RATE_LIMIT_BACKEND=postgres | ||
| CARE_TRANSIENT_STATE_BACKEND=postgres | ||
| CARE_STORAGE_BACKEND=filesystem | ||
| ``` | ||
|
|
||
| A `fake` task backend MAY exist only in test settings. | ||
|
|
||
| Production settings SHALL reject it. | ||
|
|
||
| Provider integration tests SHALL explicitly override the fake backends. |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Align the test profile with the backend contracts.
CARE_STORAGE_BACKEND=filesystem conflicts with Section 11.2, which says that filesystem is not a supported value and that tests use InMemoryStorage through override_settings. CARE_TASK_BACKEND=fake is also absent from the supported backend list in Section 17.1. Move these choices into test settings, or document them explicitly as parser-supported test-only values. As written, this profile can fail startup validation.
🤖 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 `@docs/xii/architecture/07-configuration-reference.md` around lines 2414 -
2430, Update the “Test Profile” configuration to use only backend values
supported by the documented contracts, especially replacing or removing
CARE_STORAGE_BACKEND=filesystem and CARE_TASK_BACKEND=fake. If these are
intended as test-only parser values, document them explicitly in the
corresponding backend sections and ensure startup validation accepts them;
otherwise move the choices into the test settings using the established
InMemoryStorage and supported task-backend configuration.
Comment on lines
+325
to
+354
| **verified** **Absent** from `Pipfile`: `django-storages`, any | ||
| `google-cloud-*` package, `django-celery-beat`, `django-celery-results`. | ||
|
|
||
| **verified** `django-anymail` is installed with the `amazon-ses` extra. | ||
| **inferred** email delivery is AWS SES today; GCP has no drop-in equivalent, so | ||
| this needs an explicit decision. | ||
|
|
||
| --- | ||
|
|
||
| ## 10. Existing GCP-related code | ||
|
|
||
| **verified** A repository-wide search for `gcp`, `google.cloud`, `cloud run`, | ||
| `cloudsql`, `cloud_tasks`, `django-storages` and `django_storages` across | ||
| `*.py`, `*.yml`, `*.yaml`, `*.sh`, `Pipfile` and `*.toml`, excluding | ||
| `docs/xii/`, returns exactly **two** matches: | ||
|
|
||
| | File | Line | Content | | ||
| | --- | --- | --- | | ||
| | `care/utils/csp/config.py` | 20 | `GCP = "GCP"` — a `CSProvider` enum member | | ||
| | `care/emr/utils/file_manager.py` | 130 | `# bulk delete is not supported by some providers: GCP` | | ||
|
|
||
| **verified** The `CSProvider.GCP` member is **never branched on**. | ||
| `BUCKET_PROVIDER` is only ever compared against `CSProvider.AWS_ROLE_BASED` | ||
| (`care/utils/csp/config.py:35, 48, 62`). | ||
|
|
||
| **verified** There is no Terraform, no Cloud Build config, no `app.yaml`, no | ||
| `service.yaml`, and no GCP credentials handling anywhere in the repository. | ||
|
|
||
| **Conclusion (verified):** GCP support does not exist. This is genuinely | ||
| greenfield. |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Refresh the dependency and GCP inventory.
These lines state that django-storages is absent and that GCP support has no storage configuration. This PR adds django-storages and provider-neutral Django Storage configuration. Update the dependency table, the GCP search result, and the greenfield conclusion. If this section must preserve the Phase 0 state, label it explicitly as a historical baseline.
The PR objective confirms the new storage configuration.
🤖 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 `@docs/xii/architecture/inventory/runtime-and-deployment.md` around lines 325 -
354, Update the dependency and GCP inventory section to reflect the PR’s
addition of django-storages and provider-neutral Django Storage configuration,
including the dependency table and repository search results. Revise the “GCP
support does not exist” conclusion to distinguish the new configuration from any
still-absent GCP infrastructure; if retaining the prior findings, label them
explicitly as the historical Phase 0 baseline.
Comment on lines
+35
to
+37
| **verified** Only **3 tasks are ever dispatched asynchronously** in non-test code: | ||
| `generate_report_task`, `send_totp_enabled_email`, `send_totp_disabled_email`, plus | ||
| `summarise_monetary_components` in its own recursive tail. |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Correct the asynchronous task count.
Lines [35-37] say that only 3 tasks run asynchronously, but the sentence lists summarise_monetary_components as a fourth task. Lines [383-389] confirm 4 asynchronous call sites across 4 distinct tasks. Update this count to 4 to avoid an incomplete migration inventory.
Proposed wording
-**verified** Only **3 tasks are ever dispatched asynchronously** in non-test code:
+**verified** Only **4 tasks are ever dispatched asynchronously** in non-test code:📝 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
| **verified** Only **3 tasks are ever dispatched asynchronously** in non-test code: | |
| `generate_report_task`, `send_totp_enabled_email`, `send_totp_disabled_email`, plus | |
| `summarise_monetary_components` in its own recursive tail. | |
| **verified** Only **4 tasks are ever dispatched asynchronously** in non-test code: | |
| `generate_report_task`, `send_totp_enabled_email`, `send_totp_disabled_email`, plus | |
| `summarise_monetary_components` in its own recursive tail. |
🤖 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 `@docs/xii/architecture/inventory/task-call-sites.md` around lines 35 - 37,
Update the asynchronous task count in the inventory statement from 3 to 4, while
retaining the existing list of four tasks and the recursive-tail qualification
for summarise_monetary_components.
Comment on lines
+275
to
+279
| **verified** The recursion at `:167` passes `child`, a **`FacilityLocation` | ||
| instance**, while the entry point at `:128` passes `self.id`, an **int**. The | ||
| function body at `:165` uses the argument as `parent_id=base_location`, which | ||
| works for an int; passing a model instance relies on Django coercing the instance | ||
| to its PK in the filter. Inconsistent, but functional today. |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Fix the recursive handle_cascade argument contract.
Lines [275-279] state that recursion passes a FacilityLocation instance into a query using parent_id=base_location, then classify the behavior as functional. In Django, ForeignKey.get_prep_value() delegates to the target field, and IntegerField.get_prep_value() converts the value with int(value). A model instance is therefore not a safe scalar parent_id value for this path. (raw.githubusercontent.com)
Pass child.pk, or use the relation lookup parent=base_location. Add a regression test for a two-level hierarchy.
#!/bin/bash
set -euo pipefail
rg -n -C 8 'def handle_cascade|handle_cascade\(|parent_id\s*=' care config --glob '*.py'
fd -a '^(Pipfile|Pipfile.lock|pyproject.toml|setup.cfg|setup.py)$' \
| xargs -r rg -n 'django'🤖 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 `@docs/xii/architecture/inventory/task-call-sites.md` around lines 275 - 279,
Update the recursive call in handle_cascade so it passes child.pk when the
function uses the value as parent_id, or change the query to use the parent
relation with the FacilityLocation instance. Add a regression test covering
cascading through a two-level FacilityLocation hierarchy.
Comment on lines
+96
to
+104
| ### A6. "All uploads pass through Django" is partly already true | ||
|
|
||
| **verified** `POST /api/v1/files/upload-file/` | ||
| (`care/emr/api/viewsets/file_upload.py:213-270`) already proxies uploads through | ||
| Django as base64. | ||
|
|
||
| **inferred** Any document describing the Django-proxied upload as new work should | ||
| account for this endpoint — the task is to replace a base64 path with a streaming | ||
| one and to remove the presigned alternative, not to build from nothing. |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Update the resolved file-transport entries.
These sections describe the signed-URL, base64-upload, direct bucket URL, and legacy bucket-config paths as current or pending work. This PR replaces those paths with multipart uploads and CARE-mediated download and image routes. Mark resolved items as resolved, and retain only migration questions about clients that used the legacy contract. Otherwise this inventory directs frontend and deployment work toward removed behavior.
The PR objective confirms the new file transport contract.
Also applies to: 130-135, 223-229, 312-322, 338-346, 412-418
🤖 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 `@docs/xii/architecture/inventory/unresolved-items.md` around lines 96 - 104,
Update the resolved file-transport entries, including the sections corresponding
to the signed-URL, base64-upload, direct bucket URL, and legacy bucket-config
paths, to mark replaced behavior as resolved and document the multipart upload
plus CARE-mediated download/image routes. Remove pending work that targets the
retired contracts, retaining only migration questions for clients still using
the legacy contract.
4 tasks
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
1 participant
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.
This PR modernizes CARE's file storage implementation by adopting Django's Storage API and simplifying the HTTP file transport contract, according to #3731
The objective is to reduce provider-specific code while preserving local MinIO compatibility and enabling additional storage backends through configuration.
What changed
Storage
django-storagesFile transport
FileResponseBenefits
Local development
The existing Docker development workflow continues using MinIO through
S3Storage.No cloud provider is required for local development.
Breaking changes
This PR changes the file API contract.
Removed:
signed_urlread_signed_urlAdded:
download_url)The frontend must be updated accordingly.
Out of scope
This PR intentionally does not include:
Those are planned as separate architectural phases.
Testing
The implementation has been validated with:
Notes
The architectural work was intentionally split into independent phases.
This PR contains only the storage and file-transport modernization to keep review focused.
Merge Checklist
/docsSummary by CodeRabbit
New Features
Improvements