feat: type checking with mypy in strict mode #39
Conversation
|
Important Review skippedToo many files! 40 files out of 190 files are above the max files limit of 150. You can disable this status message by setting the 📝 WalkthroughWalkthroughAdds stricter MyPy checking and broadens typing refinements: advanced generics in tracing utilities, introduction of Mongo document type aliases and DB/Collection/Cursor abstractions, and widespread replacement of motor driver types with the new database-context types and parameterized motor generics for local client usage. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
backend/app/db/schema/schema_manager.py (1)
110-132: Fix missing type parameters for dict return type.MyPy strict mode requires explicit type parameters for generic types. The return type annotation is missing, causing the pipeline failure.
🔎 Proposed fix
- @staticmethod - def _event_json_schema() -> dict: + @staticmethod + def _event_json_schema() -> dict[str, Any]:backend/workers/dlq_processor.py (1)
1-12: Fix import organization to resolve Ruff linting failure.The imports need to be sorted according to the project convention to resolve the I001 linting error.
🔎 Proposed fix
Organize imports into groups: stdlib, third-party, local. Run:
ruff check --select I --fix backend/workers/dlq_processor.pybackend/app/db/repositories/execution_repository.py (3)
58-58: Fix type incompatibility for execution_id parameter.MyPy strict mode correctly identifies that
document.get("execution_id")returnsAny | None, but theDomainExecutionconstructor expectsstr. This needs explicit handling.🔎 Proposed fix
sv = document.get("status") return DomainExecution( - execution_id=document.get("execution_id"), + execution_id=str(document.get("execution_id", "")), script=document.get("script", ""),Alternatively, if execution_id is truly required:
execution_id = document.get("execution_id") if not execution_id: raise ValueError(f"Missing execution_id in document") return DomainExecution( execution_id=str(execution_id), ... )
68-68: Fix type incompatibility for from_dict argument.The
document.get("resource_usage")can returnAny | None, butResourceUsageDomain.from_dictexpectsdict[str, Any]. The None check on Line 69 is correct, but mypy needs the type to be narrowed before the call.🔎 Proposed fix
resource_usage=( - ResourceUsageDomain.from_dict(document.get("resource_usage")) - if document.get("resource_usage") is not None + ResourceUsageDomain.from_dict(resource_usage) + if (resource_usage := document.get("resource_usage")) is not None + and isinstance(resource_usage, dict) else None ),Or more explicitly:
resource_usage_data = document.get("resource_usage") resource_usage = ( ResourceUsageDomain.from_dict(resource_usage_data) if resource_usage_data is not None and isinstance(resource_usage_data, dict) else None )
77-77: Add type parameters to generic dict types.MyPy strict mode requires type parameters for generic types. Specify the key and value types for these dict parameters.
🔎 Proposed fix
- async def update_execution(self, execution_id: str, update_data: dict) -> bool: + async def update_execution(self, execution_id: str, update_data: dict[str, object]) -> bool: - async def get_executions( - self, - query: dict, + async def get_executions( + self, + query: dict[str, object], - async def count_executions(self, query: dict) -> int: + async def count_executions(self, query: dict[str, object]) -> int:Also applies to: 126-126, 163-163
backend/app/dlq/manager.py (1)
51-53: Add type parameters to asyncio.Task generic types.MyPy strict mode requires type parameters for generic
Tasktypes. Specify the return type for these task attributes.🔎 Proposed fix
self._running = False - self._process_task: asyncio.Task | None = None - self._monitor_task: asyncio.Task | None = None + self._process_task: asyncio.Task[None] | None = None + self._monitor_task: asyncio.Task[None] | None = Nonebackend/app/core/providers.py (1)
128-133: Fix: Use typedping()method instead of untypedexecute_command().The pipeline failure explicitly indicates:
Call to untyped function "execute_command" in typed context [no-untyped-call]. Theexecute_commandmethod lacks proper type annotations in redis-py's async stubs, causing mypy strict mode to fail.🔎 Proposed fix
# Test connection - await client.execute_command("PING") + await client.ping()
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
backend/app/core/database_context.pybackend/app/core/dishka_lifespan.pybackend/app/core/providers.pybackend/app/db/repositories/admin/admin_events_repository.pybackend/app/db/repositories/admin/admin_settings_repository.pybackend/app/db/repositories/admin/admin_user_repository.pybackend/app/db/repositories/dlq_repository.pybackend/app/db/repositories/event_repository.pybackend/app/db/repositories/execution_repository.pybackend/app/db/repositories/notification_repository.pybackend/app/db/repositories/replay_repository.pybackend/app/db/repositories/resource_allocation_repository.pybackend/app/db/repositories/saga_repository.pybackend/app/db/repositories/saved_script_repository.pybackend/app/db/repositories/sse_repository.pybackend/app/db/repositories/user_repository.pybackend/app/db/repositories/user_settings_repository.pybackend/app/db/schema/schema_manager.pybackend/app/dlq/manager.pybackend/app/events/event_store.pybackend/app/services/coordinator/coordinator.pybackend/app/services/k8s_worker/worker.pybackend/workers/dlq_processor.pybackend/workers/run_event_replay.pybackend/workers/run_saga_orchestrator.py
✅ Files skipped from review due to trivial changes (1)
- backend/app/db/repositories/admin/admin_settings_repository.py
🧰 Additional context used
🧬 Code graph analysis (14)
backend/app/dlq/manager.py (1)
backend/app/core/database_context.py (3)
database(68-70)database(150-153)database(205-206)
backend/app/db/repositories/saga_repository.py (2)
backend/tests/conftest.py (2)
app(130-138)db(178-180)backend/app/core/database_context.py (3)
database(68-70)database(150-153)database(205-206)
backend/app/db/repositories/replay_repository.py (1)
backend/app/core/database_context.py (3)
database(68-70)database(150-153)database(205-206)
backend/app/core/dishka_lifespan.py (2)
backend/tests/conftest.py (1)
app(130-138)backend/app/core/database_context.py (3)
database(68-70)database(150-153)database(205-206)
backend/app/db/repositories/sse_repository.py (2)
backend/tests/conftest.py (2)
app(130-138)db(178-180)backend/app/core/database_context.py (3)
database(68-70)database(150-153)database(205-206)
backend/app/db/repositories/user_settings_repository.py (1)
backend/app/core/database_context.py (3)
database(68-70)database(150-153)database(205-206)
backend/app/db/repositories/user_repository.py (1)
backend/tests/conftest.py (2)
app(130-138)db(178-180)
backend/app/db/repositories/event_repository.py (1)
backend/app/core/database_context.py (3)
database(68-70)database(150-153)database(205-206)
backend/app/db/repositories/saved_script_repository.py (2)
backend/tests/conftest.py (2)
app(130-138)db(178-180)backend/app/core/database_context.py (3)
database(68-70)database(150-153)database(205-206)
backend/workers/dlq_processor.py (1)
backend/app/core/database_context.py (6)
db_name(73-75)db_name(156-157)db_name(209-210)database(68-70)database(150-153)database(205-206)
backend/app/db/repositories/dlq_repository.py (1)
backend/tests/conftest.py (2)
app(130-138)db(178-180)
backend/app/db/repositories/resource_allocation_repository.py (1)
backend/app/core/database_context.py (3)
database(68-70)database(150-153)database(205-206)
backend/app/services/k8s_worker/worker.py (1)
backend/app/core/database_context.py (3)
database(68-70)database(150-153)database(205-206)
backend/app/db/repositories/notification_repository.py (1)
backend/app/core/database_context.py (3)
database(68-70)database(150-153)database(205-206)
🪛 GitHub Actions: MyPy Type Checking
backend/app/db/schema/schema_manager.py
[error] 1-1: CI step failed: 'mypy --config-file pyproject.toml --strict .' reported errors (exit code 1).
[error] 111-111: Missing type parameters for generic type "dict" [type-arg].
backend/workers/run_event_replay.py
[error] 1-1: CI step failed: 'mypy --config-file pyproject.toml --strict .' reported errors (exit code 1).
backend/app/dlq/manager.py
[error] 1-1: CI step failed: 'mypy --config-file pyproject.toml --strict .' reported errors (exit code 1).
[error] 52-52: Missing type parameters for generic type "Task" [type-arg].
[error] 53-53: Missing type parameters for generic type "Task" [type-arg].
backend/app/db/repositories/saga_repository.py
[error] 1-1: CI step failed: 'mypy --config-file pyproject.toml --strict .' reported errors (exit code 1).
backend/app/db/repositories/replay_repository.py
[error] 1-1: CI step failed: 'mypy --config-file pyproject.toml --strict .' reported errors (exit code 1).
backend/workers/run_saga_orchestrator.py
[error] 1-1: CI step failed: 'mypy --config-file pyproject.toml --strict .' reported errors (exit code 1).
backend/app/db/repositories/execution_repository.py
[error] 1-1: CI step failed: 'mypy --config-file pyproject.toml --strict .' reported errors (exit code 1).
[error] 58-58: Argument "execution_id" to "DomainExecution" has incompatible type "Any | None"; expected "str" [arg-type].
[error] 68-68: Argument 1 to "from_dict" of "ResourceUsageDomain" has incompatible type "Any | None"; expected "dict[str, Any]" [arg-type].
[error] 77-77: Missing type parameters for generic type "dict" [type-arg].
[error] 126-126: Missing type parameters for generic type "dict" [type-arg].
[error] 163-163: Missing type parameters for generic type "dict" [type-arg].
backend/app/core/dishka_lifespan.py
[error] 1-1: CI step failed: 'mypy --config-file pyproject.toml --strict .' reported errors (exit code 1).
backend/app/db/repositories/sse_repository.py
[error] 1-1: CI step failed: 'mypy --config-file pyproject.toml --strict .' reported errors (exit code 1).
backend/app/db/repositories/user_settings_repository.py
[error] 1-1: CI step failed: 'mypy --config-file pyproject.toml --strict .' reported errors (exit code 1).
backend/app/db/repositories/user_repository.py
[error] 1-1: CI step failed: 'mypy --config-file pyproject.toml --strict .' reported errors (exit code 1).
backend/app/core/database_context.py
[error] 1-1: CI step failed: 'mypy --config-file pyproject.toml --strict .' reported errors (exit code 1).
backend/app/db/repositories/event_repository.py
[error] 1-1: CI step failed: 'mypy --config-file pyproject.toml --strict .' reported errors (exit code 1).
backend/app/db/repositories/saved_script_repository.py
[error] 1-1: CI step failed: 'mypy --config-file pyproject.toml --strict .' reported errors (exit code 1).
backend/app/events/event_store.py
[error] 1-1: CI step failed: 'mypy --config-file pyproject.toml --strict .' reported errors (exit code 1).
backend/app/db/repositories/admin/admin_events_repository.py
[error] 1-1: CI step failed: 'mypy --config-file pyproject.toml --strict .' reported errors (exit code 1).
backend/workers/dlq_processor.py
[error] 1-1: CI step failed: 'mypy --config-file pyproject.toml --strict .' reported errors (exit code 1).
backend/app/db/repositories/dlq_repository.py
[error] 1-1: CI step failed: 'mypy --config-file pyproject.toml --strict .' reported errors (exit code 1).
backend/app/db/repositories/resource_allocation_repository.py
[error] 1-1: CI step failed: 'mypy --config-file pyproject.toml --strict .' reported errors (exit code 1).
backend/app/core/providers.py
[error] 1-1: CI step failed: 'mypy --config-file pyproject.toml --strict .' reported errors (exit code 1).
[error] 130-130: Call to untyped function "execute_command" in typed context [no-untyped-call].
backend/app/services/coordinator/coordinator.py
[error] 1-1: CI step failed: 'mypy --config-file pyproject.toml --strict .' reported errors (exit code 1).
backend/app/services/k8s_worker/worker.py
[error] 1-1: CI step failed: 'mypy --config-file pyproject.toml --strict .' reported errors (exit code 1).
backend/app/db/repositories/notification_repository.py
[error] 1-1: CI step failed: 'mypy --config-file pyproject.toml --strict .' reported errors (exit code 1).
backend/app/db/repositories/admin/admin_user_repository.py
[error] 1-1: CI step failed: 'mypy --config-file pyproject.toml --strict .' reported errors (exit code 1).
🪛 GitHub Actions: Ruff Linting
backend/workers/run_event_replay.py
[error] 1-1: I001: Import block is un-sorted or un-formatted. Organize imports.
backend/workers/run_saga_orchestrator.py
[error] 1-1: I001: Import block is un-sorted or un-formatted. Organize imports.
backend/workers/dlq_processor.py
[error] 1-1: I001: Import block is un-sorted or un-formatted. Organize imports.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Integration Tests
🔇 Additional comments (28)
backend/app/core/database_context.py (1)
111-122: LGTM! Type-safe client instantiation with explicit event loop binding.The explicit type annotation and event loop binding at line 121 help prevent common asyncio pitfalls in multi-threaded environments.
backend/app/db/schema/schema_manager.py (1)
8-18: Migration to Database abstraction looks good.The constructor signature change maintains backward compatibility at the runtime level while improving type safety.
backend/workers/run_event_replay.py (1)
40-44: Type annotation migration looks good.The DBClient type alias correctly replaces AsyncIOMotorClient while maintaining runtime behavior.
backend/workers/dlq_processor.py (1)
101-109: Type annotations correctly migrated to Database abstractions.Both
DBClientandDatabasetype aliases are properly applied, maintaining runtime behavior while improving type safety.backend/app/db/repositories/saved_script_repository.py (1)
1-15: Clean migration to Database and Collection abstractions.The repository correctly adopts the new type aliases, maintaining consistency with the broader migration effort across the codebase.
backend/app/services/coordinator/coordinator.py (1)
11-11: DBClient type alias correctly applied.The type annotation properly uses the new abstraction while maintaining the existing instantiation pattern.
Also applies to: 553-559
backend/workers/run_saga_orchestrator.py (1)
31-37: Type annotation correctly updated to DBClient.The DBClient type alias properly replaces AsyncIOMotorClient while preserving runtime behavior.
backend/app/core/dishka_lifespan.py (1)
8-8: Database abstraction properly integrated into DI configuration.The migration from
AsyncIOMotorDatabaseto theDatabasetype alias in the dependency injection container is correct and complete. The Dishka container correctly retrieves dependencies by calling .get(SomeType), and theDatabaseprovider is properly configured inproviders.py(lines 108-109) to supply theDatabasetype. All repositories and services that depend onDatabaseresolve correctly through the DI system.backend/app/db/repositories/event_repository.py (1)
8-8: LGTM! Clean migration to database context abstractions.The migration from Motor-specific types to the internal
DatabaseandCollectionabstractions is well-executed and consistent. This aligns with the PR's goal of introducing stricter type checking.Also applies to: 28-31
backend/app/db/repositories/user_repository.py (1)
5-5: LGTM! Consistent type abstraction migration.The changes properly adopt the project's
DatabaseandCollectionabstractions, maintaining consistency with the broader codebase refactoring.Also applies to: 15-17
backend/app/db/repositories/resource_allocation_repository.py (1)
3-3: LGTM! Type abstractions properly applied.The migration to
DatabaseandCollectiontypes is clean and follows the established pattern across the codebase.Also applies to: 10-12
backend/app/db/repositories/execution_repository.py (1)
3-3: LGTM! Database abstraction migration is clean.The migration to
DatabaseandCollectiontypes is properly implemented and consistent with the codebase-wide refactoring.Also applies to: 11-14
backend/app/dlq/manager.py (1)
9-9: LGTM! Database abstraction adoption is correct.The migration to
DatabaseandCollectiontypes, including the updated factory function signature, is clean and aligns with the codebase-wide refactoring.Also applies to: 33-33, 49-49, 398-403
backend/app/db/repositories/dlq_repository.py (1)
4-4: LGTM! Proper database abstraction migration.The changes correctly adopt the
DatabaseandCollectionabstractions, maintaining consistency with the project-wide type refactoring.Also applies to: 26-28
backend/app/db/repositories/sse_repository.py (1)
1-1: LGTM! Database abstraction properly implemented.The migration to
DatabaseandCollectiontypes is clean and consistent, correctly handling multiple collection attributes.Also applies to: 9-12
backend/app/db/repositories/saga_repository.py (1)
5-5: LGTM! Clean database abstraction migration.The migration to
DatabaseandCollectiontypes is properly implemented, correctly handling multiple collection attributes and maintaining the repository pattern.Also applies to: 13-24
backend/app/db/repositories/replay_repository.py (1)
14-17: LGTM! Consistent use of Database abstraction.The constructor and collection initialization correctly use the new
DatabaseandCollectiontype abstractions, with consistent use ofget_collection()throughout the file.backend/app/db/repositories/user_settings_repository.py (1)
18-23: LGTM! Consistent migration to Database abstraction.The repository correctly adopts the
DatabaseandCollectiontype abstractions with consistent use ofget_collection().backend/app/services/k8s_worker/worker.py (2)
59-72: LGTM! Correct use of Database abstraction in worker.The constructor properly accepts
Databaseand assigns it toself._dbwith the correct type annotation.
560-566: Type annotation correctly uses DBClient alias.The
db_client: DBClientannotation paired withAsyncIOMotorClientinstantiation correctly separates the type abstraction from the concrete implementation.backend/app/db/repositories/admin/admin_events_repository.py (1)
46-58: LGTM! Comprehensive migration to Database abstraction.All collection bindings correctly use
get_collection()with the newCollectiontype, maintaining consistency across the repository.backend/app/events/event_store.py (2)
20-34: LGTM with note on collection access pattern.The
db[collection_name]subscript notation is valid for MongoDB databases. Verify theDatabasetype alias includes__getitem__support in its type definition to satisfy mypy strict mode.
310-311: Good: Cursor type correctly applied to internal method.The
_deserialize_cursormethod now uses the abstractCursortype, maintaining consistency with the broader type abstraction.backend/app/db/repositories/admin/admin_user_repository.py (1)
19-31: LGTM! Clean migration to Database abstraction.All collection bindings use
get_collection()consistently with properCollectiontype annotations.backend/app/core/providers.py (3)
107-109: LGTM! Database abstraction correctly returned from connection.The
get_databasemethod properly returns theDatabasetype from the connection abstraction.
176-183: LGTM! DLQ manager provider correctly uses Database type.
210-221: LGTM! Consistent Database type usage across all repository providers.All repository provider methods correctly accept
Databaseand pass it to repository constructors.Also applies to: 332-337, 368-369, 380-381, 384-385, 418-419, 429-431, 441-442, 445-446, 449-450, 485-486, 489-490, 493-494, 497-498, 501-502, 626-627
backend/app/db/repositories/notification_repository.py (1)
19-24: LGTM on changed lines.The initialization correctly uses
get_collection(). The code also uses direct attribute access (self.db.users,self.db.executions) elsewhere, which is idiomatic and fully supported by Motor's AsyncIOMotorDatabase—both patterns are equivalent and type-safe.Likely an incorrect or invalid review comment.
ruff formatting
arg-type fixes
arg-type fixes
|
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.