Convert report API to ADRF views (prep for async embedding trigger)

[samuelvkwong]

Summary

Convert the report API from sync DRF to async ADRF as a 1:1 viewset translation, with the async/sync seam expressed uniformly across every handler. radis/reports/api/viewsets.py keeps its name and ReportViewSet keeps its place; the structural changes are:

• viewsets.GenericViewSet → adrf.viewsets.GenericViewSet with adrf.mixins.* async mixins.
• def create/retrieve/update/destroy → async def acreate/aretrieve/aupdate/adestroy.
• @action(detail=False, …) for bulk_upsert stays a single class member, now async def.
• New radis/reports/api/operations.py — async write operations (create_report_from_validated, update_report_from_validated, delete_report) using native async ORM (aget_or_create, acreate, asave, aset, aclear, adelete). Operations do not own transactions.
• ReportSerializer becomes adrf.serializers.ModelSerializer with async def acreate / aupdate. Each of those is a @sync_to_async(thread_sensitive=True) @transaction.atomic helper that bridges to the corresponding operation via async_to_sync(...). The serializer owns its atomicity; the view does not.
• The _bulk_upsert_reports helper (legacy) is renamed to bulk_upsert_reports, stays in viewsets.py, and is now async def. Its phases are explicitly split:
  • Phase 1 (payload dedupe, pure CPU) — @sync_to_async(thread_sensitive=True) helper.
  • Phase 2 (Language/Modality/existing-Reports preflight) — native async ORM (async for, await abulk_create).
  • Phase 3 (build new_reports / updated_reports lists, pure CPU) — @sync_to_async(thread_sensitive=True) helper.
  • Phase 4 (atomic writes) — @sync_to_async(thread_sensitive=True) @transaction.atomic helper with inline sync ORM (bulk_create / bulk_update / through-table churn).

URLs are wired through adrf.routers.DefaultRouter (not DRF's). See "Router choice is load-bearing" below.

Why now: the follow-up PR will embed each uploaded report inline, during the upload request, by awaiting the async embedding client from inside the view so the report's vector is populated by the time the API returns (no eventual-consistency window). The embedding call is I/O-bound, so an async view lets us yield to the event loop while it's in flight instead of pinning a worker thread. This PR is the structural prerequisite — no inline embedding wiring yet.

Bridge architecture

Each view handler is a pure async coordinator:

• await sync_to_async(serializer.is_valid)(raise_exception=True) — DRF is_valid is sync; sync_to_async-wrapped from the view.
• report = await serializer.asave() — async-native, the serializer's acreate/aupdate owns the atomic block.
• transaction.on_commit(on_commit) — registered after asave returns; fires immediately (atomic already committed) or queues under the test fixture (django_capture_on_commit_callbacks).
• await sync_to_async(lambda: serializer.data)() — DRF to_representation is sync.

The atomic-owning helper inside ReportSerializer.acreate / aupdate is:

@sync_to_async(thread_sensitive=True)
@transaction.atomic
def _atomic():
    return async_to_sync(operations.create_report_from_validated)(validated_data)
return await _atomic()

thread_sensitive=True ensures the outer wrapper, the inner async_to_sync event loop, and the sync adapters Django's async ORM calls internally all run on the same Django thread, so the transaction context applies to every write the operation performs.

adestroy follows the same shape directly (no serializer involved): a @sync_to_async(thread_sensitive=True) @transaction.atomic helper calling async_to_sync(operations.delete_report)(report).

bulk_upsert_reports Phase 4 keeps inline sync ORM in its atomic helper because the writes are single-statement bulk operations that don't decompose into per-entity ops.

Router choice is load-bearing

DRF's rest_framework.routers.DefaultRouter maps HTTP methods to the sync action names (POST → create, PUT → update, GET → retrieve, DELETE → destroy). Because adrf.mixins.* inherit from DRF's sync mixins, those sync method names exist on the class as fully-functional sync methods — so DRF-router-driven dispatch silently calls the inherited sync mixin implementations and never reaches our acreate/aretrieve/aupdate/adestroy overrides. The four tests that exercise behavior added by our overrides (created/deleted on_commit handlers, ?full=true documents, ?upsert=true 201) fail invisibly under this misconfiguration.

adrf.routers.DefaultRouter rewrites the action mapping to the a-prefixed names whenever view_is_async=True, so dispatch reaches our overrides. Verified locally with resolve(...).func.actions == {"post": "acreate", "put": "aupdate", ...}.

The async-shape guard in test_report_api.py pins every dispatched method to inspect.iscoroutinefunction to catch a future contributor accidentally overriding the sync sibling — but that guard cannot catch a mis-wired router. The router choice is part of the architectural contract.

API contract — byte-for-byte unchanged

URLs (/api/reports/, /api/reports/{document_id}/, /api/reports/bulk-upsert/), route names (report-list, report-detail, report-bulk-upsert), response shapes, status codes, query params (?upsert, ?full, ?replace), 405-for-PATCH, and the 403-vs-404 semantics on PUT?upsert=true against an unknown id are all preserved. The router's default lookup_value_regex = [^/.]+ already forbids . in document_id — the legacy behavior — without explicit configuration.

The browsable API root at /api/reports/ (auto-generated DefaultRouter index view) is preserved as well.

Contract is locked by radis/reports/tests/test_report_api.py — written before the rewrite, passing against both the legacy DRF viewset (during intermediate commits) and the new ADRF viewset.

Test plumbing

HTTP-based tests use django.test.AsyncClient + async def + @pytest.mark.asyncio + @pytest.mark.django_db(transaction=True). The sync Client would dispatch the async view via async_to_sync, which nested with our database_sync_to_async deadlocks asgiref's thread executor (Django 6.0 + channels 4). AsyncClient runs the view in the test's event loop with no outer wrapping. transaction=True is needed because the database_sync_to_async thread doesn't see a TestCase-wrapped transaction.

Sync ORM helpers (UserFactory.create, Token.objects.create_token) are wrapped in await sync_to_async(...)() at the call site to satisfy Django's async_unsafe guard.

Note on Django's current async ORM surface

Every a* method on QuerySet/Manager (aget, aget_or_create, abulk_create, aset, acreate, asave, adelete, ...) is literally await sync_to_async(self.X)() in Django 6.0 / 6.1 — there is no native async DB backend in core today. Phase 2's async for / await calls dispatch to the asgiref thread pool just like our explicit sync_to_async calls. The win today is architectural clarity, not runtime concurrency. Once a native async DB backend ships, the @sync_to_async @transaction.atomic + async_to_sync(operations.X) helpers in the serializer, adestroy, and Phase 4 collapse to async with async_atomic(): return await operations.X(...). operations.py does not change; Phases 1 and 3 stay sync_to_async-wrapped because they are pure CPU.

Out of scope

• Wiring the inline async embedding call into the create/update paths (follow-up PR).
• Other API surfaces (radis.search, radis.chats, radis.extractions, ...).
• Migrations, settings, env vars.

Design + plan docs

• docs/superpowers/specs/2026-06-08-adrf-report-views-design.md
• docs/superpowers/plans/2026-06-08-adrf-report-views.md

Both docs explain the router-choice trap explicitly so the next person reading the spec doesn't repeat the mistake this PR made and recovered from.

Test plan

• uv run cli lint — 0 errors, 0 warnings, 0 informations (ruff + pyright + djlint)
• uv run cli test -- --cov — 243 passed on the latest CI run.
• Manual smoke — endpoint coverage is in test_report_api.py (URL resolution, POST/GET/PUT/DELETE/PATCH→405, ?upsert=true 201/403, ?full=true document fetchers, bulk-upsert reject paths, async-shape guard pinning every dispatched method to iscoroutinefunction).

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Reports API now supports asynchronous request processing and async bulk-upsert for improved performance.

• Behavior Changes / Bug Fixes

  • PATCH is no longer supported (returns 405).
  • Bulk-upsert enforces payload validation and clearer per-item error reporting with truncation.

• Tests

  • Added end-to-end async tests covering list, detail, create, update/upsert, delete, bulk-upsert, permissions, and event handlers.

• Documentation

  • Expanded design and migration plan with verification checklist.

• Chores

  • Added configurable bulk-upsert DB batch size.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Rewrites the Reports API to a single async ADRF ReportViewSet with async CRUD entry points and an async bulk_upsert helper/action, switches routing to ADRF router, converts serializers/operations for async writes, adds async end-to-end tests and migrated bulk-upsert tests, and introduces a bulk DB batch-size setting.

Changes

Async ADRF Report ViewSet Migration

Layer / File(s)	Summary
Design & Implementation Plan docs/superpowers/plans/2026-06-08-adrf-report-views.md, docs/superpowers/specs/2026-06-08-adrf-report-views-design.md	Design spec and plan document async migration approach, preserved API invariants, async/sync hygiene, transaction.on_commit scheduling, and rollout/test verification.
Router & URL Registration radis/reports/api/urls.py	Router import switched to adrf.routers.DefaultRouter; ReportViewSet registered with basename="report" at the empty-string prefix to preserve endpoint routes.
Client Test Transaction Marker radis-client/tests/test_client.py	Updated @pytest.mark.django_db to @pytest.mark.django_db(transaction=True) for async test compatibility.
Async Operations (domain writes) radis/reports/api/operations.py	Add async domain write helpers (create_report_from_validated, update_report_from_validated, delete_report) that build/replace nested relations; functions do not manage caller transactions.
Async Serializers & Delegation radis/reports/api/serializers.py	Convert ReportSerializer to ADRF AsyncModelSerializer; add acreate/aupdate delegating to operations inside transaction wrappers bridged for sync compatibility.
Async ViewSet Module & Bulk Helper radis/reports/api/viewsets.py	Introduce ADRF-focused async viewset header and implement bulk_upsert_reports with async preflight, dedupe, async pre-creates, CPU-side build, and sync-wrapped atomic bulk persistence plus post-commit reloads for handlers.
ReportViewSet Methods & Actions radis/reports/api/viewsets.py	Implement ADRF async ReportViewSet methods: acreate, aretrieve, aupdate, adestroy, and async bulk_upsert action; disable PATCH at dispatcher level and validate per-item via sync-to-async serializer calls.
End-to-end Report API Tests radis/reports/tests/test_report_api.py	New async test suite covering URL resolution, POST/GET/PUT/DELETE/upsert/permission semantics, PATCH 405, full=true enrichment, bulk-upsert validation, on-commit handler capture, and an async-shape guard asserting coroutine methods.
Bulk-upsert Test Migration radis/reports/tests/test_bulk_upsert.py	Tests migrated from sync to async: use AsyncClient, sync_to_async setup helpers, awaited requests, async ORM assertions, and import updated to bulk_upsert_reports.
Settings: Bulk DB Batch Size radis/settings/base.py	Added REPORTS_BULK_DB_BATCH_SIZE env-backed setting (default 1000) to control bulk-upsert batching.

Estimated code review effort:
🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs:

• openradx/radis#187: Related bulk-upsert implementation changes and deduping/replace handling that overlap with the async bulk helper/action work.

"a rabbit pens this little rhyme,
ADRF hops in, step in time,
async views and bulk abide,
tests stand guard and handlers ride,
commit — then indexing chime."

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 11.32% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely summarizes the main change: converting the report API from DRF to ADRF views, with context about preparing for async embedding.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/adrf-views

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[gemini-code-assist]

Code Review

This pull request refactors the radiology report API by replacing the synchronous DRF ReportViewSet with three explicit asynchronous ADRF APIView subclasses (ReportListAPIView, ReportDetailAPIView, and ReportBulkUpsertAPIView) and extracting the bulk upsert logic into a separate module. End-to-end tests and async-shape guards are also introduced to ensure API contract preservation. The code review identified critical concurrency and transaction safety issues in the new async views, including thread-safety concerns when separating report.adelete() from transaction.on_commit registration, potential event loop blocking from accessing request.data inside database_sync_to_async blocks, and an opportunity to optimize external document fetching by utilizing asyncio.gather for parallel execution.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[coderabbitai]

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

docs/superpowers/specs/2026-06-08-adrf-report-views-design.md (2)

189-189: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Clarify “router removal” wording to “router migration.”

Given this design keeps routing via adrf.routers.DefaultRouter, “router removal” is inaccurate and may confuse regression intent for the route-resolution test.

🤖 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/superpowers/specs/2026-06-08-adrf-report-views-design.md` at line 189,
Update the test wording and assertion guidance: change the comment/commit
message that says “router removal” to “router migration” and ensure the
regression guard in radis/reports/tests/test_bulk_upsert.py asserts that the
bulk-upsert endpoint still resolves under the current routing (using
adrf.routers.DefaultRouter); specifically, add one explicit assertion that the
route resolves (e.g., client.get/post to the bulk-upsert route returns the
expected status) and update any related test text to reference “router
migration” instead of “router removal.”

---

19-25: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Resolve the architecture contradiction in “In scope.”

This section says to drop ReportViewSet/router and move to three APIViews + explicit path() routes, but later sections define the chosen design as a single ADRF ReportViewSet on adrf.routers.DefaultRouter. Keep one architecture only, or this spec will drive conflicting implementations/reviews.

🤖 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/superpowers/specs/2026-06-08-adrf-report-views-design.md` around lines
19 - 25, The spec contains a contradiction between using three
adrf.views.APIView subclasses (ReportListAPIView, ReportDetailAPIView,
ReportBulkUpsertAPIView) with explicit path() routes and using a single ADRF
ReportViewSet registered on adrf.routers.DefaultRouter; pick one approach and
make the entire document consistent: either remove all mentions of
ReportViewSet/DefaultRouter/router registration and ensure
radis/reports/api/urls.py and the “In scope” list describe the three APIViews
and the rename of _bulk_upsert_reports to bulk_upsert_reports, or keep
ReportViewSet/DefaultRouter as the single chosen design and remove the APIView
references and explicit path() instructions (also revert or document how
bulk_upsert_reports is exposed via the viewset action). Ensure every section (In
scope, endpoints list, URL wiring, and helper rename) aligns with the single
chosen architecture and reference the exact symbols above when updating text.

🤖 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 `@docs/superpowers/specs/2026-06-08-adrf-report-views-design.md`:
- Around line 87-90: Update the example imports to use ADRF modules and the
viewsets module: replace the DRF router import so it uses DefaultRouter from
adrf.routers (symbol DefaultRouter) and change the ReportViewSet import to come
from .viewsets (symbol ReportViewSet) instead of rest_framework.routers and
.views; keep the rest of the wiring unchanged so the router and ReportViewSet
integration matches the async dispatch design.
- Line 27: The spec incorrectly references Django's synchronous Client for the
new async ADRF contract; update all mentions (e.g., the line describing the new
test file `radis/reports/tests/test_report_api.py` and any other occurrences) to
recommend using Django's AsyncClient and async test plumbing (async test
functions or pytest-asyncio/Django async test utilities) so tests exercise async
dispatch end-to-end rather than the synchronous Client.

In `@radis/reports/api/viewsets.py`:
- Around line 283-296: The commit callback that reindexes touched_report_ids
(calling bulk_upsert_report_search_vectors or enqueue_bulk_index_reports) is
only present in the bulk path's on_commit closure; update the single-record
handlers in acreate() and aupdate() to mirror that logic: when a single Report
is created/updated, register the same on_commit behavior (collect the single
document_id into touched_report_ids and, inside the on_commit callback, call
bulk_upsert_report_search_vectors(touched_report_ids) if
settings.PGSEARCH_SYNC_INDEXING else
enqueue_bulk_index_reports(touched_report_ids)) and still invoke
reports_created_handlers / reports_updated_handlers as before so single-record
POST/PUT commits also update the PGSEARCH index.
- Around line 354-359: The document fetches wrapped by _fetch use
database_sync_to_async with its default thread_sensitive=True which serializes
work onto one worker thread; update the calls in the _fetch coroutine to use
database_sync_to_async(fetcher.fetch, thread_sensitive=False)(report) (or
convert fetcher.fetch implementations to be async) so asyncio.gather can run
them in parallel; locate _fetch, fetcher.fetch, document_fetchers and
asyncio.gather in this block and apply the thread_sensitive=False change (or
switch the fetchers to native async) accordingly.

---

Outside diff comments:
In `@docs/superpowers/specs/2026-06-08-adrf-report-views-design.md`:
- Line 189: Update the test wording and assertion guidance: change the
comment/commit message that says “router removal” to “router migration” and
ensure the regression guard in radis/reports/tests/test_bulk_upsert.py asserts
that the bulk-upsert endpoint still resolves under the current routing (using
adrf.routers.DefaultRouter); specifically, add one explicit assertion that the
route resolves (e.g., client.get/post to the bulk-upsert route returns the
expected status) and update any related test text to reference “router
migration” instead of “router removal.”
- Around line 19-25: The spec contains a contradiction between using three
adrf.views.APIView subclasses (ReportListAPIView, ReportDetailAPIView,
ReportBulkUpsertAPIView) with explicit path() routes and using a single ADRF
ReportViewSet registered on adrf.routers.DefaultRouter; pick one approach and
make the entire document consistent: either remove all mentions of
ReportViewSet/DefaultRouter/router registration and ensure
radis/reports/api/urls.py and the “In scope” list describe the three APIViews
and the rename of _bulk_upsert_reports to bulk_upsert_reports, or keep
ReportViewSet/DefaultRouter as the single chosen design and remove the APIView
references and explicit path() instructions (also revert or document how
bulk_upsert_reports is exposed via the viewset action). Ensure every section (In
scope, endpoints list, URL wiring, and helper rename) aligns with the single
chosen architecture and reference the exact symbols above when updating text.

🪄 Autofix (Beta)

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: defaults

Review profile: CHILL

Plan: Pro

Run ID: 3aface82-eb2b-4979-99f9-467994501c38

📥 Commits

Reviewing files that changed from the base of the PR and between 00232c9 and 103f36b.

📒 Files selected for processing (7)

• docs/superpowers/plans/2026-06-08-adrf-report-views.md
• docs/superpowers/specs/2026-06-08-adrf-report-views-design.md
• radis-client/tests/test_client.py
• radis/reports/api/urls.py
• radis/reports/api/viewsets.py
• radis/reports/tests/test_bulk_upsert.py
• radis/reports/tests/test_report_api.py

✅ Files skipped from review due to trivial changes (1)

• docs/superpowers/plans/2026-06-08-adrf-report-views.md

🚧 Files skipped from review as they are similar to previous changes (1)

• radis/reports/api/urls.py

[coderabbitai]

Caution

Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

docs/superpowers/specs/2026-06-08-adrf-report-views-design.md (2)

189-189: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Clarify “router removal” wording to “router migration.”

Given this design keeps routing via adrf.routers.DefaultRouter, “router removal” is inaccurate and may confuse regression intent for the route-resolution test.

🤖 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/superpowers/specs/2026-06-08-adrf-report-views-design.md` at line 189,
Update the test wording and assertion guidance: change the comment/commit
message that says “router removal” to “router migration” and ensure the
regression guard in radis/reports/tests/test_bulk_upsert.py asserts that the
bulk-upsert endpoint still resolves under the current routing (using
adrf.routers.DefaultRouter); specifically, add one explicit assertion that the
route resolves (e.g., client.get/post to the bulk-upsert route returns the
expected status) and update any related test text to reference “router
migration” instead of “router removal.”

---

19-25: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Resolve the architecture contradiction in “In scope.”

This section says to drop ReportViewSet/router and move to three APIViews + explicit path() routes, but later sections define the chosen design as a single ADRF ReportViewSet on adrf.routers.DefaultRouter. Keep one architecture only, or this spec will drive conflicting implementations/reviews.

🤖 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/superpowers/specs/2026-06-08-adrf-report-views-design.md` around lines
19 - 25, The spec contains a contradiction between using three
adrf.views.APIView subclasses (ReportListAPIView, ReportDetailAPIView,
ReportBulkUpsertAPIView) with explicit path() routes and using a single ADRF
ReportViewSet registered on adrf.routers.DefaultRouter; pick one approach and
make the entire document consistent: either remove all mentions of
ReportViewSet/DefaultRouter/router registration and ensure
radis/reports/api/urls.py and the “In scope” list describe the three APIViews
and the rename of _bulk_upsert_reports to bulk_upsert_reports, or keep
ReportViewSet/DefaultRouter as the single chosen design and remove the APIView
references and explicit path() instructions (also revert or document how
bulk_upsert_reports is exposed via the viewset action). Ensure every section (In
scope, endpoints list, URL wiring, and helper rename) aligns with the single
chosen architecture and reference the exact symbols above when updating text.

🤖 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 `@docs/superpowers/specs/2026-06-08-adrf-report-views-design.md`:
- Around line 87-90: Update the example imports to use ADRF modules and the
viewsets module: replace the DRF router import so it uses DefaultRouter from
adrf.routers (symbol DefaultRouter) and change the ReportViewSet import to come
from .viewsets (symbol ReportViewSet) instead of rest_framework.routers and
.views; keep the rest of the wiring unchanged so the router and ReportViewSet
integration matches the async dispatch design.
- Line 27: The spec incorrectly references Django's synchronous Client for the
new async ADRF contract; update all mentions (e.g., the line describing the new
test file `radis/reports/tests/test_report_api.py` and any other occurrences) to
recommend using Django's AsyncClient and async test plumbing (async test
functions or pytest-asyncio/Django async test utilities) so tests exercise async
dispatch end-to-end rather than the synchronous Client.

In `@radis/reports/api/viewsets.py`:
- Around line 283-296: The commit callback that reindexes touched_report_ids
(calling bulk_upsert_report_search_vectors or enqueue_bulk_index_reports) is
only present in the bulk path's on_commit closure; update the single-record
handlers in acreate() and aupdate() to mirror that logic: when a single Report
is created/updated, register the same on_commit behavior (collect the single
document_id into touched_report_ids and, inside the on_commit callback, call
bulk_upsert_report_search_vectors(touched_report_ids) if
settings.PGSEARCH_SYNC_INDEXING else
enqueue_bulk_index_reports(touched_report_ids)) and still invoke
reports_created_handlers / reports_updated_handlers as before so single-record
POST/PUT commits also update the PGSEARCH index.
- Around line 354-359: The document fetches wrapped by _fetch use
database_sync_to_async with its default thread_sensitive=True which serializes
work onto one worker thread; update the calls in the _fetch coroutine to use
database_sync_to_async(fetcher.fetch, thread_sensitive=False)(report) (or
convert fetcher.fetch implementations to be async) so asyncio.gather can run
them in parallel; locate _fetch, fetcher.fetch, document_fetchers and
asyncio.gather in this block and apply the thread_sensitive=False change (or
switch the fetchers to native async) accordingly.

---

Outside diff comments:
In `@docs/superpowers/specs/2026-06-08-adrf-report-views-design.md`:
- Line 189: Update the test wording and assertion guidance: change the
comment/commit message that says “router removal” to “router migration” and
ensure the regression guard in radis/reports/tests/test_bulk_upsert.py asserts
that the bulk-upsert endpoint still resolves under the current routing (using
adrf.routers.DefaultRouter); specifically, add one explicit assertion that the
route resolves (e.g., client.get/post to the bulk-upsert route returns the
expected status) and update any related test text to reference “router
migration” instead of “router removal.”
- Around line 19-25: The spec contains a contradiction between using three
adrf.views.APIView subclasses (ReportListAPIView, ReportDetailAPIView,
ReportBulkUpsertAPIView) with explicit path() routes and using a single ADRF
ReportViewSet registered on adrf.routers.DefaultRouter; pick one approach and
make the entire document consistent: either remove all mentions of
ReportViewSet/DefaultRouter/router registration and ensure
radis/reports/api/urls.py and the “In scope” list describe the three APIViews
and the rename of _bulk_upsert_reports to bulk_upsert_reports, or keep
ReportViewSet/DefaultRouter as the single chosen design and remove the APIView
references and explicit path() instructions (also revert or document how
bulk_upsert_reports is exposed via the viewset action). Ensure every section (In
scope, endpoints list, URL wiring, and helper rename) aligns with the single
chosen architecture and reference the exact symbols above when updating text.

🪄 Autofix (Beta)

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: defaults

Review profile: CHILL

Plan: Pro

Run ID: 3aface82-eb2b-4979-99f9-467994501c38

📥 Commits

Reviewing files that changed from the base of the PR and between 00232c9 and 103f36b.

📒 Files selected for processing (7)

• docs/superpowers/plans/2026-06-08-adrf-report-views.md
• docs/superpowers/specs/2026-06-08-adrf-report-views-design.md
• radis-client/tests/test_client.py
• radis/reports/api/urls.py
• radis/reports/api/viewsets.py
• radis/reports/tests/test_bulk_upsert.py
• radis/reports/tests/test_report_api.py

✅ Files skipped from review due to trivial changes (1)

• docs/superpowers/plans/2026-06-08-adrf-report-views.md

🚧 Files skipped from review as they are similar to previous changes (1)

• radis/reports/api/urls.py

🛑 Comments failed to post (4)

docs/superpowers/specs/2026-06-08-adrf-report-views-design.md (2)

27-27: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Update test client guidance to AsyncClient consistently.

The spec still says Django Client, but this migration’s contract is async ADRF behavior and async test plumbing. Keeping sync-client guidance here risks false confidence on async dispatch coverage.

Also applies to: 191-191

🤖 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/superpowers/specs/2026-06-08-adrf-report-views-design.md` at line 27,
The spec incorrectly references Django's synchronous Client for the new async
ADRF contract; update all mentions (e.g., the line describing the new test file
`radis/reports/tests/test_report_api.py` and any other occurrences) to recommend
using Django's AsyncClient and async test plumbing (async test functions or
pytest-asyncio/Django async test utilities) so tests exercise async dispatch
end-to-end rather than the synchronous Client.

---

87-90: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Fix the URL wiring sample to ADRF imports.

The snippet currently shows DRF router and .views import, which contradicts the async dispatch design and the implemented wiring. It should use from adrf.routers import DefaultRouter and import ReportViewSet from .viewsets.

🤖 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/superpowers/specs/2026-06-08-adrf-report-views-design.md` around lines
87 - 90, Update the example imports to use ADRF modules and the viewsets module:
replace the DRF router import so it uses DefaultRouter from adrf.routers (symbol
DefaultRouter) and change the ReportViewSet import to come from .viewsets
(symbol ReportViewSet) instead of rest_framework.routers and .views; keep the
rest of the wiring unchanged so the router and ReportViewSet integration matches
the async dispatch design.

radis/reports/api/viewsets.py (2)

283-296: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Mirror the bulk-upsert PGSEARCH callback in acreate() and aupdate().

The bulk path reindexes touched_report_ids on commit, but the single-record POST/PUT paths only invoke reports_created_handlers / reports_updated_handlers. That leaves /api/reports/ and /api/reports/{document_id}/ able to commit successfully while the PGSEARCH index stays stale for those reports.

Suggested fix

         def on_commit():
             for handler in reports_created_handlers:
                 logger.debug(
                     f"{handler.name} - handle newly created reports: "
                     f"{[report.document_id]}"
                 )
                 handler.handle([report])
+            report_ids = [report.pk]
+            if settings.PGSEARCH_SYNC_INDEXING:
+                bulk_upsert_report_search_vectors(report_ids)
+            else:
+                enqueue_bulk_index_reports(report_ids)

         def on_commit():
             handlers = (
                 reports_created_handlers
                 if report is None
                 else reports_updated_handlers
             )
             event = "newly created" if report is None else "updated"
             for handler in handlers:
                 logger.debug(
                     f"{handler.name} - handle {event} reports: "
                     f"{[saved.document_id]}"
                 )
                 handler.handle([saved])
+            report_ids = [saved.pk]
+            if settings.PGSEARCH_SYNC_INDEXING:
+                bulk_upsert_report_search_vectors(report_ids)
+            else:
+                enqueue_bulk_index_reports(report_ids)

Also applies to: 327-336, 392-406

🤖 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 `@radis/reports/api/viewsets.py` around lines 283 - 296, The commit callback
that reindexes touched_report_ids (calling bulk_upsert_report_search_vectors or
enqueue_bulk_index_reports) is only present in the bulk path's on_commit
closure; update the single-record handlers in acreate() and aupdate() to mirror
that logic: when a single Report is created/updated, register the same on_commit
behavior (collect the single document_id into touched_report_ids and, inside the
on_commit callback, call bulk_upsert_report_search_vectors(touched_report_ids)
if settings.PGSEARCH_SYNC_INDEXING else
enqueue_bulk_index_reports(touched_report_ids)) and still invoke
reports_created_handlers / reports_updated_handlers as before so single-record
POST/PUT commits also update the PGSEARCH index.

---

354-359: ⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🌐 Web query:

In Channels 4.2.0 / asgiref, does channels.db.database_sync_to_async default to thread_sensitive=True such that multiple calls awaited via asyncio.gather execute serially on the same thread executor?

💡 Result:

Yes, channels.db.database_sync_to_async (which inherits from asgiref.sync.SyncToAsync) defaults to thread_sensitive=True [1]. In this mode, multiple calls to database_sync_to_async that are awaited via asyncio.gather will execute serially because they share a single-threaded executor [2][3][4]. The thread_sensitive=True setting is designed to ensure that all synchronous functions marked as "thread-sensitive" run in the same specific thread [4][5]. This is necessary for compatibility with code that relies on thread-local storage or non-thread-safe resources, such as many database adapters used by the Django ORM [2][4][5]. When the outermost program is asynchronous, this shared resource is a single dedicated sub-thread [4]. Consequently, if you launch multiple synchronous tasks using asyncio.gather, they will queue up and execute one after the other within that single, shared thread, rather than running in parallel across multiple threads [2][4]. If you set thread_sensitive=False, each call will instead run in its own brand-new thread, which would allow for parallel execution but may lead to issues if your code depends on thread-local state or specific database connection constraints [4][5].

Citations:

• 1: https://github.com/django/channels/blob/main/channels/db.py
• 2: https://docs.djangoproject.com/en/4.2/_modules/asgiref/sync/
• 3: https://github.com/django/asgiref/blob/main/asgiref/sync.py
• 4: https://github.com/django/asgiref/blob/main/README.rst
• 5: https://docs.djangoproject.com/en/dev/topics/async/

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Show the exact code around the referenced lines
python3 - <<'PY'
from pathlib import Path
path = Path("radis/reports/api/viewsets.py")
start, end = 330, 390
lines = path.read_text().splitlines()
for i in range(start, end+1):
    if 1 <= i <= len(lines):
        print(f"{i}:{lines[i-1]}")
PY

# Find where document_fetchers is built and where fetcher.fetch is defined/used
rg -n "document_fetchers|_fetch\\(|fetcher\\.fetch\\(|database_sync_to_async\\(" radis/reports/api/viewsets.py

# If fetchers come from elsewhere, try to locate "document_fetchers" usage/definition across the repo
rg -n "document_fetchers" radis | head -n 50

# Locate the fetcher type/class (best-effort) by searching for ".source" and "fetch(" patterns
rg -n "class .*Fetcher|def fetch\\(|fetch\\(" radis/reports | head -n 50

Repository: openradx/radis

Length of output: 3788

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect imports and database_sync_to_async definition/usage near top of the file
python3 - <<'PY'
from pathlib import Path
p = Path("radis/reports/api/viewsets.py")
lines = p.read_text().splitlines()
for i in range(1, min(120, len(lines))+1):
    print(f"{i}:{lines[i-1]}")
PY

# Show definition of DocumentFetcher and how fetcher.fetch is constructed
python3 - <<'PY'
from pathlib import Path
p = Path("radis/reports/site.py")
lines = p.read_text().splitlines()
for i in range(1, min(140, len(lines))+1):
    print(f"{i}:{lines[i-1]}")
PY

# Find where DocumentFetcher fetch callables come from; also locate stub/fetchers usage
rg -n "DocumentFetcher\\(|fetchers|document_fetchers\\[" radis/reports | head -n 200

# Locate implementations of fetch functions (best-effort)
rg -n "def fetch\\(|async def fetch\\(" radis/reports | head -n 200

# Locate any database_sync_to_async calls that override thread_sensitive
rg -n "database_sync_to_async\\(.*thread_sensitive|thread_sensitive=" radis | head -n 200

Repository: openradx/radis

Length of output: 8683

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# 1) Locate all document fetcher registrations (to inspect what fetcher.fetch actually is)
rg -n "register_document_fetcher\\(" radis || true

# 2) Inspect the exact fetcher callables used in registrations (common names: fetch, get_document, etc.)
#    We search for functions passed as the 2nd arg to register_document_fetcher
rg -n "register_document_fetcher\\([^,]+,\\s*([A-Za-z0-9_\\.]+)\\)" radis || true

# 3) Look for any thread_sensitive overrides in the repo (esp. around database_sync_to_async)
rg -n "database_sync_to_async\\(.*thread_sensitive" radis || true
rg -n "thread_sensitive\\s*=" radis || true

# 4) Also list all database_sync_to_async usages inside radis/reports for context
rg -n "database_sync_to_async\\(" radis/reports || true

Repository: openradx/radis

Length of output: 461

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

python3 - <<'PY'
from pathlib import Path
p = Path("radis/reports/tests/test_report_api.py")
lines = p.read_text().splitlines()
start, end = 140, 240
for i in range(start, end+1):
    if 1 <= i <= len(lines):
        print(f"{i}:{lines[i-1]}")
PY

Repository: openradx/radis

Length of output: 3887

---

asyncio.gather() won’t parallelize these document fetches

channels.db.database_sync_to_async defaults to thread_sensitive=True, so the fetcher.fetch() calls wrapped here will queue onto the same single worker thread. As a result, ?full=true will still scale roughly linearly with the number of document_fetchers despite using asyncio.gather. If fetcher.fetch doesn’t rely on Django/thread-local constraints, switch to database_sync_to_async(..., thread_sensitive=False) (or make the fetchers native async).

async def _fetch(fetcher):
    return fetcher.source, await database_sync_to_async(fetcher.fetch)(report)

results = await asyncio.gather(*(_fetch(f) for f in document_fetchers.values()))

🤖 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 `@radis/reports/api/viewsets.py` around lines 354 - 359, The document fetches
wrapped by _fetch use database_sync_to_async with its default
thread_sensitive=True which serializes work onto one worker thread; update the
calls in the _fetch coroutine to use database_sync_to_async(fetcher.fetch,
thread_sensitive=False)(report) (or convert fetcher.fetch implementations to be
async) so asyncio.gather can run them in parallel; locate _fetch, fetcher.fetch,
document_fetchers and asyncio.gather in this block and apply the
thread_sensitive=False change (or switch the fetchers to native async)
accordingly.

[NumericalAdvantage]

My review :

Right now the lexical indexing (which is cheaper to calculate compared to embedding vector) is done locally (on radx server) in batches, routed through the background Procrastinate queue.
And now for semantic search (which takes longer to calculate, is done via an unreliable service) is being done asynchronously .. I dont think this is so good because it would run the expensive, remote, failure-prone operation (embedding) inline, while blocking the report upload .. while the cheap, local, reliable one (lexical) stays deferred. Right after upload a report would be findable by meaning (semantic, inline) but maybe not yet by keyword (lexical, still queued). I think for jobs like this eventual consistency is more important ..

If you are still not convinced, think of the following scenario :

today nothing in the indexing path can be "down," because there's no external service in it. The outage risk is entirely created by the proposed "inline embedding" design.

This is what I mean - you have two ways of wiring things up -

Ordering A — embed, then save. Request arrives → await embed(text) → embedding service is down → the await raises → request returns a 5xx → the report is never saved. During an embedding outage, ingestion stops dead. Every upload fails; the radis-client or the ETL pipeline either errors out or retry-loops against a dead service.

Ordering B — save, then embed. Request → save report → await embed → store vector → respond. Embedding down → the report is saved but the embed step throws. Now either the request 500s (the client thinks it failed and may retry, creating duplicate/re-upsert churn) or you swallow the error and return 201 — and either way you're left with a saved-but-unembedded report: it exists, but it's invisible to semantic search, silently inconsistent, with nothing scheduled to fix it.

(and you havent written any test cases to catch this behaviour .. ;))

All this is happening because you have chosen to do the upload and embedding together. You could just reuse the Procrastinate stuff of lexical indexing .. i think that is the more technically fitter solution ..

But ALSO:::

we dont have a lot of work to do to get ninja running for our async support. useing adrf is arguably the poorer solution .. especially now that there is nothing aprt from reportviewsets that uses DRF .. we have very little change to manage the migration to Ninja ... there would not be a better time. My strong sugesstion would be to use Ninja ..

Cheers

[coderabbitai]

Actionable comments posted: 4

🧹 Nitpick comments (1)

radis/reports/api/operations.py (1)

82-87: 💤 Low value

Minor: aclear() before aset() is redundant.

aset() already replaces the entire M2M relation, so the preceding aclear() is unnecessary. This doesn't affect correctness but adds an extra query.

♻️ Optional simplification

-    await report.modalities.aclear()
     modality_instances: list[Modality] = []
     for modality in modalities:
         instance, _ = await Modality.objects.aget_or_create(**modality)
         modality_instances.append(instance)
     await report.modalities.aset(modality_instances)

🤖 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 `@radis/reports/api/operations.py` around lines 82 - 87, The code calls
report.modalities.aclear() before building modality_instances and then
report.modalities.aset(modality_instances); remove the redundant await
report.modalities.aclear() since aset() replaces the entire M2M relation itself.
Keep the loop that uses Modality.objects.aget_or_create(...) to populate
modality_instances and then call await
report.modalities.aset(modality_instances) directly.

🤖 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 `@radis/reports/api/serializers.py`:
- Around line 48-56: The docstring for ReportSerializer is incorrect about
transaction ownership; update it to state that acreate and aupdate do own/manage
their own transactions via the inner `@_atomic`() helper (used in functions
acreate and aupdate) rather than relying on the caller to provide
`@transaction.atomic`; reference ReportSerializer, acreate, aupdate, and the inner
_atomic helper when rewording the docstring so it accurately reflects that these
methods run their own atomic blocks.

In `@radis/reports/api/viewsets.py`:
- Around line 432-433: In aretrieve and adestroy replace the current exception
re-raise that does "raise Http404" inside the "except Report.DoesNotExist"
blocks with "raise Http404 from None" so the original Report.DoesNotExist is
suppressed and the traceback is not chained; locate the except blocks in the
aretrieve and adestroy methods and update the raise statements accordingly.
- Around line 512-516: In adestroy, the Report.DoesNotExist except block should
suppress the original exception per Ruff B904; change the handler to capture the
exception (e.g., "except Report.DoesNotExist as err:") and re-raise Http404 with
explicit chaining to suppress the original (use "raise Http404 from None") so
the original traceback is not shown; update the except clause in the adestroy
method accordingly.

In `@radis/settings/base.py`:
- Around line 167-168: REPORTS_BULK_DB_BATCH_SIZE is read from the environment
and must be validated at startup: after you read REPORTS_BULK_DB_BATCH_SIZE in
radis/settings/base.py add a guard that raises
django.core.exceptions.ImproperlyConfigured if REPORTS_BULK_DB_BATCH_SIZE <= 0
(ensure you import ImproperlyConfigured from django.core.exceptions at top of
the file); this guarantees a clear error during startup rather than passing an
invalid batch_size into functions like
radis.reports.api.viewsets.bulk_upsert_reports which call
abulk_create/bulk_create/bulk_update.

---

Nitpick comments:
In `@radis/reports/api/operations.py`:
- Around line 82-87: The code calls report.modalities.aclear() before building
modality_instances and then report.modalities.aset(modality_instances); remove
the redundant await report.modalities.aclear() since aset() replaces the entire
M2M relation itself. Keep the loop that uses
Modality.objects.aget_or_create(...) to populate modality_instances and then
call await report.modalities.aset(modality_instances) directly.

🪄 Autofix (Beta)

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: defaults

Review profile: CHILL

Plan: Pro

Run ID: 63a41d2d-461b-4bb6-8a9f-407e069d7d9f

📥 Commits

Reviewing files that changed from the base of the PR and between 09b0a3c and dcbe6af.

📒 Files selected for processing (5)

• radis/reports/api/operations.py
• radis/reports/api/serializers.py
• radis/reports/api/viewsets.py
• radis/reports/tests/test_bulk_upsert.py
• radis/settings/base.py

🚧 Files skipped from review as they are similar to previous changes (1)

• radis/reports/tests/test_bulk_upsert.py

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Both aretrieve and adestroy should use raise Http404 from None.

Both methods raise Http404 within an except Report.DoesNotExist clause without exception chaining. Use from None to explicitly suppress the original exception and avoid confusing tracebacks.

🧰 Tools

🪛 Ruff (0.15.15)

[warning] 433-433: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)

🤖 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 `@radis/reports/api/viewsets.py` around lines 432 - 433, In aretrieve and
adestroy replace the current exception re-raise that does "raise Http404" inside
the "except Report.DoesNotExist" blocks with "raise Http404 from None" so the
original Report.DoesNotExist is suppressed and the traceback is not chained;
locate the except blocks in the aretrieve and adestroy methods and update the
raise statements accordingly.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Add exception chaining to suppress the original exception.

Per Ruff B904, within an except clause, use raise ... from None to indicate the original exception is intentionally suppressed, or raise ... from err to chain it.

🔧 Proposed fix

     async def adestroy(self, request: Request, *args: Any, **kwargs: Any) -> Response:
         try:
             report = await Report.objects.aget(document_id=kwargs[self.lookup_field])
         except Report.DoesNotExist:
-            raise Http404
+            raise Http404 from None

📝 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

	async def adestroy(self, request: Request, *args: Any, **kwargs: Any) -> Response:
	try:
	report = await Report.objects.aget(document_id=kwargs[self.lookup_field])
	except Report.DoesNotExist:
	raise Http404
	async def adestroy(self, request: Request, *args: Any, **kwargs: Any) -> Response:
	try:
	report = await Report.objects.aget(document_id=kwargs[self.lookup_field])
	except Report.DoesNotExist:
	raise Http404 from None

🧰 Tools

🪛 Ruff (0.15.15)

[warning] 516-516: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)

🤖 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 `@radis/reports/api/viewsets.py` around lines 512 - 516, In adestroy, the
Report.DoesNotExist except block should suppress the original exception per Ruff
B904; change the handler to capture the exception (e.g., "except
Report.DoesNotExist as err:") and re-raise Http404 with explicit chaining to
suppress the original (use "raise Http404 from None") so the original traceback
is not shown; update the except clause in the adestroy method accordingly.

Source: Linters/SAST tools