feat: Add comprehensive report state management tests #88
Conversation
|
Warning Rate limit exceeded@svenaric has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 10 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughAdds four async methods to ReportRepository to create an initial PENDING state, apply partial updates (transitions PENDING→RUNNING), finalize reports (COMPLETED/FAILED) and retrieve state; implements transaction rollbacks for error cases and introduces an async test suite validating these flows against an in-memory DB. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Repo as ReportRepository
participant DB as Database
rect rgb(230,245,255)
Note over Client,Repo: save initial state
Client->>Repo: save_report_initial_state(report_id)
Repo->>DB: BEGIN / INSERT Report (if needed)
Repo->>DB: INSERT ReportState(status=PENDING)
DB-->>Repo: OK
Repo->>DB: COMMIT
Repo-->>Client: ReportState(status=PENDING)
end
rect rgb(230,255,230)
Note over Client,Repo: partial update
Client->>Repo: update_report_partial_results(report_id, partial_data)
Repo->>DB: BEGIN / SELECT ReportState
alt status == PENDING
Repo->>DB: UPDATE partial_results_json, status=RUNNING, updated_at
Repo->>DB: COMMIT
Repo-->>Client: ReportState(status=RUNNING)
else status in (RUNNING, COMPLETED, FAILED)
Repo->>DB: ROLLBACK or no-op
Repo-->>Client: current ReportState or None
end
end
rect rgb(255,245,230)
Note over Client,Repo: finalize report
Client->>Repo: update_report_final_report(report_id, final_data, status, error_message)
Repo->>DB: BEGIN / UPDATE final_report_json, status, error_message, updated_at
Repo->>DB: COMMIT
Repo-->>Client: ReportState(status=COMPLETED/FAILED)
end
rect rgb(240,230,255)
Note over Client,Repo: retrieve state
Client->>Repo: get_report_state(report_id)
Repo->>DB: SELECT Report + ReportState (delegates to get_report_by_id)
Repo-->>Client: ReportState or None
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/app/db/repositories/report_repository.py (1)
1-1: Remove duplicate import statement.Line 9 redefines
Callable,Dict, andAnyalready imported on line 1. Consolidate imports by addingOptionalto the existing import.Apply this diff:
-from typing import Callable, Dict, Any +from typing import Callable, Dict, Any, Optional from datetime import datetime, timedelta, timezone from sqlalchemy.ext.asyncio import AsyncSession from sqlalchemy import select, update from sqlalchemy.exc import IntegrityError from backend.app.db.models.report import Report from backend.app.db.models.report_state import ReportState, ReportStatusEnum -from typing import Callable, Dict, Any, OptionalAlso applies to: 9-9
🧹 Nitpick comments (3)
backend/app/db/repositories/report_repository.py (1)
43-68: Consider preventing updates on terminal states.The method correctly transitions
PENDING → RUNNINGbut allowspartial_agent_outputupdates onCOMPLETEDorFAILEDreports. The tests acknowledge this with a comment about ideally preventing invalid transitions. Consider adding a guard to return early when the report is in a terminal state.Optional enhancement if you want stricter state machine behavior:
current_state_result = await session.execute(select(ReportState.status).where(ReportState.report_id == report_id)) current_status = current_state_result.scalar_one_or_none() +if current_status in (ReportStatusEnum.COMPLETED, ReportStatusEnum.FAILED): + # Return current state without modification for terminal states + result = await session.execute(select(ReportState).where(ReportState.report_id == report_id)) + return result.scalar_one_or_none() + values_to_update = {backend/app/tests/state_management/test_state_management.py (2)
108-109: Add blank line between test functions.PEP 8 recommends two blank lines between top-level function definitions.
assert db_state_obj.status == ReportStatusEnum.COMPLETED assert db_state_obj.final_report_json == final_report_data + @pytest.mark.asyncio async def test_update_report_final_report_failure(report_repository, async_session_factory):
136-136: Remove unusedasync_session_factoryparameter from test functions.The
report_repositoryfixture already declaresasync_session_factoryas a dependency, so pytest will ensure proper ordering. The explicit parameter in these test functions is unnecessary.Apply these changes:
-async def test_get_report_state(report_repository, async_session_factory): +async def test_get_report_state(report_repository):-async def test_report_state_transitions(report_repository, async_session_factory): +async def test_report_state_transitions(report_repository):-async def test_report_state_transitions_to_failed(report_repository, async_session_factory): +async def test_report_state_transitions_to_failed(report_repository):Also applies to: 159-159, 191-191
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/app/db/repositories/report_repository.py(1 hunks)backend/app/tests/state_management/test_state_management.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
backend/app/tests/state_management/test_state_management.py (3)
backend/app/db/models/report.py (1)
Report(4-6)backend/app/db/models/report_state.py (2)
ReportState(26-39)ReportStatusEnum(8-21)backend/app/db/repositories/report_repository.py (4)
save_report_initial_state(14-41)update_report_partial_results(43-68)update_report_final_report(70-95)get_report_state(97-101)
backend/app/db/repositories/report_repository.py (2)
backend/app/db/models/report_state.py (2)
ReportState(26-39)ReportStatusEnum(8-21)backend/app/db/models/report.py (1)
Report(4-6)
🪛 Ruff (0.14.7)
backend/app/tests/state_management/test_state_management.py
136-136: Unused function argument: async_session_factory
(ARG001)
159-159: Unused function argument: async_session_factory
(ARG001)
191-191: Unused function argument: async_session_factory
(ARG001)
backend/app/db/repositories/report_repository.py
9-9: Redefinition of unused Callable from line 1
Remove definition: Callable
(F811)
9-9: Redefinition of unused Dict from line 1
Remove definition: Dict
(F811)
9-9: Redefinition of unused Any from line 1
Remove definition: Any
(F811)
31-31: Consider moving this statement to an else block
(TRY300)
65-65: Consider moving this statement to an else block
(TRY300)
92-92: Consider moving this statement to an else block
(TRY300)
🔇 Additional comments (7)
backend/app/db/repositories/report_repository.py (3)
14-41: LGTM!The method correctly creates both
ReportandReportStateatomically, handlesIntegrityErrorfor idempotency by returning existing state, and properly rolls back on failure.
70-95: LGTM!The method provides flexible final state updates with proper error handling and rollback. The design allows any status to be set, which supports various error recovery scenarios.
97-101: LGTM!Clean delegation to
get_report_by_idprovides a semantically clear API for state retrieval.backend/app/tests/state_management/test_state_management.py (4)
14-36: LGTM!Well-structured async fixtures with proper lifecycle management: in-memory SQLite for isolation, table creation/teardown, and engine disposal. The
expire_on_commit=Falsesetting correctly allows objects to remain accessible after commit.
38-55: LGTM!Good test coverage verifying both the returned state and database persistence. All initial field values are properly validated.
57-82: LGTM!Comprehensive test covering status transition (
PENDING → RUNNING), partial data persistence, and subsequent updates. Good use of database verification.
221-231: LGTM!Good edge case coverage testing the not-found scenario for all three methods (
get_report_state,update_report_partial_results,update_report_final_report).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
backend/app/db/repositories/report_repository.py (2)
12-39: IntegrityError handling and idempotency ofsave_report_initial_stateThe transactional pattern here looks sound, and the method is nicely idempotent when a
ReportStatealready exists. One nuance: the comment says “If a Report or ReportState with this ID already exists…”, but the fallback only considers an existingReportState. If you can ever end up with aReportrow without a correspondingReportState(e.g., legacy paths usingcreate_report_entry), this will still re‑raise onIntegrityErrorinstead of healing the missing state.If that scenario is in-scope, consider either:
- Making the docstring/comment explicit that only existing
ReportStateis handled, or- Extending the handler to create a missing
ReportStatewhen aReportexists but no state row is found.
99-103: Thinget_report_statewrapper keeps public API readableDelegating to
get_report_by_idkeeps the public method name domain-specific without duplicating query logic. This also keeps all direct DB access going through a single helper, which is good for future refactors (e.g., eager-loading related entities).If you ever need to load additional related data for “state” (e.g., the
Reportitself), this is the natural choke point to centralize that behavior.backend/app/tests/state_management/test_state_management.py (1)
14-37: Comprehensive async coverage; consider relaxing strict time ordering assertionsThe fixtures and tests give very good end-to-end coverage of the new repository API: they exercise initial creation, partial updates, finalization (success and failure), retrieval at each phase, post-finalization no-op behavior, and not-found cases, all against a real async engine. This is exactly the level of coverage you want for this state machine.
One small robustness tweak: in
test_report_state_transitionsandtest_report_state_transitions_to_failedyou assert strict monotonicity:assert state_completed.updated_at > state_running.updated_at ... assert state_failed.updated_at > state_running.updated_atBecause both updates happen back-to-back and
updated_atis set fromdatetime.now(timezone.utc), there’s a (very small) chance of equal timestamps on fast systems. If you want to make the tests maximally stable over time and across environments, you could relax these to>=or assert thatupdated_atchanges by reloading the row after a short sleep.Overall though, the suite looks well-structured and should give you good confidence in the new repo methods.
Also applies to: 38-221
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/app/db/repositories/report_repository.py(1 hunks)backend/app/tests/state_management/test_state_management.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
backend/app/db/repositories/report_repository.py (2)
backend/app/db/models/report.py (1)
Report(4-6)backend/app/db/models/report_state.py (2)
ReportState(26-39)ReportStatusEnum(8-21)
backend/app/tests/state_management/test_state_management.py (3)
backend/app/db/models/report.py (1)
Report(4-6)backend/app/db/models/report_state.py (2)
ReportState(26-39)ReportStatusEnum(8-21)backend/app/db/repositories/report_repository.py (4)
save_report_initial_state(12-39)update_report_partial_results(41-70)update_report_final_report(72-97)get_report_state(99-103)
🪛 Ruff (0.14.7)
backend/app/db/repositories/report_repository.py
29-29: Consider moving this statement to an else block
(TRY300)
67-67: Consider moving this statement to an else block
(TRY300)
94-94: Consider moving this statement to an else block
(TRY300)
🔇 Additional comments (1)
backend/app/db/repositories/report_repository.py (1)
72-97:update_report_final_reportsemantics are straightforward and align with testsThis method cleanly encapsulates finalization: it sets
status,final_report_json,error_message, andupdated_atin one atomic UPDATE and returns the updated row orNonefor a missing report, which matches the test expectations (test_update_report_final_report_*andtest_report_not_found). The genericexcept Exceptionwith rollback is consistent with the rest of the repository.No changes needed here from a correctness perspective.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/app/db/repositories/report_repository.py (1)
73-98: Consider adding idempotency protection for final state updates.Unlike
update_report_partial_results, this method allows overwriting an already-finalized report. If this is called twice (e.g., due to a retry or race condition), it will silently overwrite the previous final state.If reports should be immutable once finalized, consider adding similar protection:
try: + # Prevent overwriting already-finalized reports + final_statuses = [ReportStatusEnum.COMPLETED, ReportStatusEnum.FAILED] values_to_update = { "status": status, "final_report_json": final_report_data, "error_message": error_message, "updated_at": datetime.now(timezone.utc) } - stmt = update(ReportState).where(ReportState.report_id == report_id).values(**values_to_update).returning(ReportState) + stmt = update(ReportState).where( + ReportState.report_id == report_id, + ReportState.status.notin_(final_statuses) + ).values(**values_to_update).returning(ReportState)If intentional (e.g., allowing status corrections), a brief docstring note would clarify this design choice.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/db/repositories/report_repository.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/app/db/repositories/report_repository.py (2)
backend/app/db/models/report.py (1)
Report(4-6)backend/app/db/models/report_state.py (2)
ReportState(26-39)ReportStatusEnum(8-21)
🪛 Ruff (0.14.7)
backend/app/db/repositories/report_repository.py
29-29: Consider moving this statement to an else block
(TRY300)
68-68: Consider moving this statement to an else block
(TRY300)
95-95: Consider moving this statement to an else block
(TRY300)
🔇 Additional comments (3)
backend/app/db/repositories/report_repository.py (3)
12-39: LGTM!The
save_report_initial_statemethod correctly handles the race condition where multiple callers attempt to create the same report concurrently. TheIntegrityErrorhandling properly rolls back before fetching the existing state via a new session.
41-71: Verify intended behavior when report is in final state.The TOCTOU race condition from the past review is now correctly addressed by including
ReportState.status.notin_(final_statuses)in the WHERE clause (line 63). However, the method now returnsNonewhen a report is already in a final state, whereas the past review's suggested fix retained returning the current state for final reports.This means callers cannot distinguish between:
- Report not found →
None- Report in final state →
NoneIf callers need to differentiate these cases, consider restoring the early return:
+ if current_status in final_statuses: + return await self.get_report_by_id(report_id) + values_to_update = { "partial_agent_output": partial_data,
100-104: LGTM!Clean delegation that provides a semantic alias for the public API.
|
that test suite in |
This PR introduces a new set of tests for the report state management logic, ensuring robust handling of the report lifecycle within the application.
Changes
tests/state_management/to cover saving, updating, and retrieving report states.Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.