Feature/26 implement drf for backendfrontend separation

[frenchfaso]

Summary

Implements the complete REST API backend and establishes a production-ready infrastructure.

Key Changes

• API: Full DRF implementation with nested routers and file upload support.
• Docs: Integrated OpenAPI 3.0 (Swagger/Redoc) via drf-spectacular.
• Infra: Dockerized with Gunicorn and WhiteNoise for static files.
• Security: Implemented .env management and global authentication.

[Copilot]

Pull request overview

This PR implements a comprehensive REST API backend using Django REST Framework, establishing production-ready infrastructure with Docker containerization, OpenAPI documentation, and security enhancements for the BFD9000 medical imaging system.

Key Changes

• Complete DRF API implementation with viewsets for all major resources (Subject, Encounter, Record, etc.) and nested routing for hierarchical relationships
• File upload system with validation for PNG/STL medical imaging files, thumbnail generation, and integrated storage via ImagingStudy model
• Production infrastructure including multi-stage Dockerfile with Gunicorn, WhiteNoise for static files, docker-compose for development, and environment-based configuration

Reviewed changes

Copilot reviewed 20 out of 28 changed files in this pull request and generated 23 comments.

Show a summary per file

File	Description
bfd9000_web/archive/views.py	Implements all API viewsets including custom actions for image/thumbnail serving and valueset endpoints
bfd9000_web/archive/serializers.py	Defines serializers for all models with specialized upload serializer for file handling
bfd9000_web/archive/urls.py	Configures DRF routers including nested routes for hierarchical resources
bfd9000_web/archive/models.py	Adds source_file field to ImagingStudy for local file storage
bfd9000_web/archive/constants.py	Defines coding system URIs following FHIR-like conventions
bfd9000_web/archive/management/commands/init_codings.py	Provides management command to initialize default medical coding values
bfd9000_web/BFD9000/settings.py	Configures DRF, CORS, pagination, authentication, and drf-spectacular for OpenAPI
bfd9000_web/BFD9000/urls.py	Adds API routes and OpenAPI schema endpoints (Swagger/Redoc)
bfd9000_web/BFD9000/exceptions.py	Implements custom exception handler for standardized API error responses
bfd9000_web/requirements.txt	Adds DRF ecosystem packages (drf-spectacular, nested-routers, filters, CORS, etc.)
Dockerfile	Multi-stage build with Python 3.13, Gunicorn, and non-root user for security
docker-compose.yml	Development container configuration with volume mounts and environment variables
.env.example	Template for environment configuration
.dockerignore	Excludes Python cache and virtual environment files from Docker builds
.gitignore	Updated to exclude Docker overrides (media files still need exclusion)
bfd9000_web/archive/tests/test_api_flows.py	Integration tests for full workflow: subject creation → encounter → file upload
bfd9000_web/archive/migrations/0002_imagingstudy_source_file.py	Migration adding source_file field to ImagingStudy model
bfd9000_web/media/uploads/*	User-uploaded test files (should not be committed to repository)
.github/prompts/*.prompt.md	Planning documents for API implementation phases

Comments suppressed due to low confidence (3)

bfd9000_web/archive/serializers.py:156

• Variable mime is not used.

                mime = magic.from_buffer(value.read(2048), mime=True)

bfd9000_web/archive/serializers.py:159

• Variable mime is not used.

                mime = 'application/octet-stream'

bfd9000_web/archive/serializers.py:161

• Variable mime is not used.

            mime = 'application/octet-stream'

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[frenchfaso]

done

[zgypa]

i think you have stuff outside of the bfd9000_web folder, like docker files; however, the docker you are building here shjjould onl y be for the web app, so everything should reside in there.

Also the github prompts: if they are useful for the entire project, than OK, but if they are _web specific, then put them there, if they are needed.

Add header documentation to each file, explaining the basic reason for the file's existance. If its obvious, a single sentence suffices. In general, use pylint, activate it and follow all of its recommendations. Specifically, use typing as much as possible, at least in method signatures. This helps a lot to reduce errors.

[zgypa]

we discussed having coding initialization as a migration and using official dicom and snomed codes, like the ones defined in the spreadsheet. What is the reason for doing them like this, using custom codes? If there is one, it should be documented in the python header docs.

[frenchfaso]

Looks like Copilot inserted that to satisfy the tests quickly.

[frenchfaso]

Converted initialization to a Data Migration (0003_initial_codings.py 924de98) and updated all codes to standard SNOMED/DICOM values based on the spreadsheet and online verification.
Please review the codes in the migration file to ensure they match your expectations.

[zgypa]

I added a new comment about this on the migration. See that one.

[zgypa]

ask MC about this, i tend to agree with this:

The migration will probably work in many environments, but it has a few fragility points and one important operational caveat (non-reversibility). I recommend a small rewrite to make it robust and reversible. Below I summarize the issues I found, the risk they pose, and a small, safe replacement migration you can apply instead.

Risks and issues (what could break, and when)

1. Top-level import of archive.constants (fragility)

   • Problem: importing app code from a migration module at the top level is fragile. When Django imports migrations it will import that module; if archive.constants later changes (or begins importing something that’s not available at migration time), or if someone runs migrations after changing code structure, import can fail. Migrations should be as self-contained and stable as possible.
   • Severity: Medium. Usually constants-only modules are stable, but it’s a brittle practice and can produce confusing failures during deploys / CI when code and migrations mismatch.

2. No reverse (migration is not reversible)

   • Problem: RunPython(init_codings) is provided without reverse_code. If you later try to migrate backwards (rollback), Django cannot automatically remove the inserted Codings. That may be acceptable, but you need to be explicit.
   • Severity: Medium-low (depends on your rollback policy). If you want clean reversibility, add a reverse function that deletes the created codings.

3. get_or_create with defaults does not update existing rows

   • Problem: if an existing Coding exists with the same (system, code) but a different display value, get_or_create returns the existing row and does not update display. If the intent was to ensure display text is current, use update_or_create or explicitly update after get_or_create.
   • Severity: Low (only relevant when display text must be updated by migration).

4. Depending on model fields being present

   • Problem: init_codings calls Coding.objects.get_or_create(..., defaults={'display': display}). This assumes the Coding model (as of the migration’s app state) has a 'display' field. If the model schema had not included 'display' until a later migration, this would fail. In your case the repository already has a Coding model with 'display' (normal), but it's good to be cautious.
   • Severity: Low (likely fine here since Coding is a core model; nonetheless keep migrations aligned to the historical model state).

5. Idempotency & repeated runs

   • get_or_create ensures the migration is idempotent in creation (won’t duplicate entries), but because display is placed in defaults only, it won’t enforce display text if you expected that.

6. Runtime vs migration-time constants

   • Since constants.py is new in the same PR, applying migrations in a fresh clone will have constants.py present (so import will succeed). But best practice remains to avoid importing app modules inside migration modules.

Suggested fixes

• Avoid importing archive.constants at module level. Instead embed the system strings inside the migration (literal strings) or define them inside the RunPython callable. That makes the migration self-contained.
• Use update_or_create rather than get_or_create if you want to ensure the display is set to the value in the migration.
• Provide a reverse function that removes the created codings (or at least documents that the migration is irreversible).
• Keep the migration atomic (RunPython is atomic when migrations support atomic; fine).
• If you want to be extra-safe, explicitly handle cases where Coding model fields might differ in older historical models (rare here).

Proposed safer migration

• Below is a replacement migration that:
  • defines system constants inside the migration function (no top-level import of app code),
  • uses update_or_create to ensure the display matches,
  • provides a reverse function to delete the inserted codings (so the migration can be reversed cleanly),
  • is idempotent.

# Generated by Django 5.2.8 on 2025-12-02 20:03 (REWRITTEN FOR STABILITY)
from django.db import migrations

def init_codings(apps, schema_editor):
    Coding = apps.get_model('archive', 'Coding')

    # Define systems locally to avoid importing app modules at migration import time
    SYSTEM_RECORD_TYPE = 'http://snomed.info/sct'
    SYSTEM_ORIENTATION = 'http://snomed.info/sct'
    SYSTEM_MODALITY = 'http://dicom.nema.org/resources/ontology/DCM'
    SYSTEM_PROCEDURE = 'http://snomed.info/sct'
    SYSTEM_BODY_SITE = 'http://snomed.info/sct'

    # Helper that uses update_or_create to ensure display is set/updated
    def ensure(system, code, display):
        Coding.objects.update_or_create(
            system=system,
            code=code,
            defaults={'display': display}
        )

    # Record Types
    record_types = [
        ('201456002', 'Cephalogram'),
        ('268425006', 'Pelvis X-ray'),
        ('39714003', 'Skeletal X-ray of wrist and hand'),
        ('1597004', 'Skeletal X-ray of ankle and foot'),
        ('302189007', 'Dental model'),
    ]
    for code, display in record_types:
        ensure(SYSTEM_RECORD_TYPE, code, display)

    # Orientations (projections & laterality)
    orientations = [
        ('399198007', 'Right lateral projection'),
        ('399173006', 'Left lateral projection'),
        ('272479007', 'Posteroanterior projection'),
        ('399348003', 'Antero-posterior projection'),
        ('7771000', 'Left'),
        ('24028007', 'Right'),
    ]
    for code, display in orientations:
        ensure(SYSTEM_ORIENTATION, code, display)

    # Modalities (DICOM)
    modalities = [
        ('RG', 'Radiographic Imaging'),
        ('SI', 'Scanned Image'),
        ('XC', 'External-camera Photography'),
        ('M3D', 'Model for 3D Manufacturing'),
    ]
    for code, display in modalities:
        ensure(SYSTEM_MODALITY, code, display)

    # Body Sites (SNOMED)
    body_sites = [
        ('69536005', 'Head'),
        ('609617007', 'Structure of pelvic segment of trunk'),
        ('731298009', 'Entire ankle and foot'),
        ('729875002', 'Entire wrist and hand'),
        ('1927002', 'Entire left elbow region'),
        ('71889004', 'Entire right elbow region'),
        ('210659002', 'Entire left knee'),
        ('210562007', 'Entire right knee'),
    ]
    for code, display in body_sites:
        ensure(SYSTEM_BODY_SITE, code, display)

    # Procedures (currently empty placeholder)
    procedures = []
    for code, display in procedures:
        ensure(SYSTEM_PROCEDURE, code, display)

def remove_codings(apps, schema_editor):
    Coding = apps.get_model('archive', 'Coding')

    SYSTEMS_AND_CODES = [
        ('http://snomed.info/sct', [
            '201456002','268425006','39714003','1597004','302189007',
            '399198007','399173006','272479007','399348003','7771000','24028007',
            '69536005','609617007','731298009','729875002','1927002','71889004','210659002','210562007'
        ]),
        ('http://dicom.nema.org/resources/ontology/DCM', [
            'RG','SI','XC','M3D'
        ]),
    ]
    for system, codes in SYSTEMS_AND_CODES:
        Coding.objects.filter(system=system, code__in=codes).delete()

class Migration(migrations.Migration):

    dependencies = [
        ('archive', '0002_imagingstudy_source_file'),
    ]

    operations = [
        migrations.RunPython(init_codings, reverse_code=remove_codings),
    ]

Short rationale for the replacement

• No top-level import of archive.constants: makes the migration a stable artifact independent of later code changes.
• update_or_create: ensures the display is set/updated to the value in the migration.
• reverse_code provided: enables reversing the migration cleanly if needed.
• Idempotent and clear about what it creates/deletes.

Other runtime notes (not migration-specific, but useful)

• The codebase now uses FileField.source_file on ImagingStudy and the views use source_file.path and open FileField with PIL. If you ever switch to a remote storage backend (S3, GCS), FileField.path may not be available. To support remote storage, the thumbnail/image views should use storage.open(...) and avoid relying on .path or os.path.exists(source_file.path). (This is a runtime compatibility issue, not a migration one.)
• The tests included in the PR create required Collection and Coding test rows, so test-suite will be OK. If you want a fresh DB to be bootstrapped (so a fresh install without running tests still has codings), the migration should create those entries — which it does (but see the import caveat above).

Conclusion

• The migration as authored will likely succeed in normal installs now (because the constants file exists in the same PR), but it is fragile (top-level import) and non-reversible. I recommend applying the safer replacement above (or equivalent changes): move constants into the migration (or reference literal strings), use update_or_create, and add a reverse_code. This will make the migration robust across versions and safer to roll back.

[zgypa]

[nitpick] can you double check that you are not putting in the docker image anything that is not needed for the docker to run? Are you sure we don´t have anything in there? If we don't, its fine.

[zgypa]

looks good in general. I'm not 100% sure about the clincal informatics.interoperability part, but it looks close enough for now, to have an infrastructure in place that we can build upon after the MVP.

Maybe some tests are lacking, specifically for the codes. This is what MC told me:

Let me look more specifically at the test files and the viewsets implementation in this PR to answer your question about whether API tests verify code set separation. Based on my review of PR #28, no, the developer is not currently testing that the API returns the correct code sets for the viewsets to prevent cross-contamination.

Current State

One of the Copilot review comments explicitly points this out:

"The ValuesetViewSet lacks test coverage. The endpoint handles multiple valueset types (sex_options, collections, record_types, orientations, modalities) and error cases, but there are no tests verifying this behavior."

Is This Relevant?

**Yes, this is very relevant to the code. ** The ValuesetViewSet appears to handle multiple different code sets based on a type parameter (orientations, record_types, modalities, collections, sex_options). Without tests that verify each valueset type returns only its specific codes, there's a real risk that:

1. Requesting orientations could accidentally return record_types or other codes
2. Changes to the filtering logic could silently break the separation between code sets
3. The behavior of each endpoint isn't documented through tests

Recommended Tests

The PR should include tests for:

• Valid valueset type requests - verify each type returns the correct data
• Cross-contamination checks - explicitly verify that orientations doesn't include record_types, etc.
• Missing 'type' parameter - should return 400
• Unknown valueset type - should return 404
• Correct data structure - verify the response format for each valueset type

This is especially important given the medical/orthodontic nature of this application where code integrity matters.

---

Note: Results may be incomplete. You can view all PR comments at the PR's Files Changed tab.