Async Types#364
Open
BrianLusina wants to merge 12 commits into
Open
Conversation
BrianLusina
added
documentation
📝 WalkthroughWalkthroughAdds asynchronous session transactions, repository CRUD and query operations, soft deletion, commit callbacks, shared typing definitions, JSON serialization normalization, package-level exports, and typing-tooling updates. ChangesAsync database support
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ServiceMethod
participant async_transaction
participant AsyncSession
participant Database
ServiceMethod->>async_transaction: invoke decorated method
async_transaction->>AsyncSession: begin transaction
AsyncSession->>Database: execute database work
Database-->>AsyncSession: return result
AsyncSession-->>ServiceMethod: complete transaction
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 6
🧹 Nitpick comments (4)
sanctumlabs_dbkit/sql/repository/types.py (1)
1-10: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a module docstring to satisfy pylint.
Pipeline reports
C0114: missing-module-docstringfor this new file.📝 Suggested fix
+""" +Shared type definitions for repository models. +""" + from typing import ( TypeVar, Union, )🤖 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 `@sanctumlabs_dbkit/sql/repository/types.py` around lines 1 - 10, Add a descriptive module docstring at the beginning of types.py, before the imports, documenting the purpose of RepositoryBaseModel and the T type variable while leaving their existing definitions unchanged.Source: Pipeline failures
sanctumlabs_dbkit/sql/repository/async_repository.py (2)
104-116: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
deletesilently no-ops when entity is not found.
await self.find(pk)returnsNonefor missing or already soft-deleted entities (sinceinclude_deleteddefaults toFalse), and the method silently returns without raising. Consider usingfind_or_raiseor at least documenting the idempotent behavior, as callers may expect an error for non-existent keys.🤖 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 `@sanctumlabs_dbkit/sql/repository/async_repository.py` around lines 104 - 116, Update AsyncRepository.delete to explicitly handle a missing entity returned by find, using find_or_raise if deletion should reject non-existent or already soft-deleted records; otherwise document the intentional idempotent no-op behavior in the method contract. Preserve the existing soft-deletion validation and deleted_at assignment.
25-130: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffAsyncRepository largely duplicates the sync Repository.
Pylint
R0801flags duplicate code across the sync and async repositories. Consider extracting shared query-building logic (e.g.,query,_supports_soft_deletion, soft-delete filtering) into a shared base or mixin to prevent drift when one version is updated.🤖 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 `@sanctumlabs_dbkit/sql/repository/async_repository.py` around lines 25 - 130, Extract the duplicated query-building and soft-deletion logic from AsyncRepository, including _supports_soft_deletion and query, into a shared base class or mixin used by both repository implementations. Update AsyncRepository and the sync Repository to reuse these shared symbols while preserving their existing async and sync database operations and filtering behavior.Source: Pipeline failures
sanctumlabs_dbkit/sql/session/types.py (1)
1-3: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a module docstring to satisfy pylint.
Pipeline reports
C0114: missing-module-docstringfor this new file.📝 Suggested fix
+""" +Shared callable typing definitions for session decorators. +""" + from typing import Any, Callable, TypeVar🤖 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 `@sanctumlabs_dbkit/sql/session/types.py` around lines 1 - 3, Add a concise module-level docstring at the beginning of types.py before the typing imports, describing the module’s type definitions or session-related typing helpers; leave FuncT unchanged.Source: Pipeline failures
🤖 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 `@sanctumlabs_dbkit/sql/repository/__init__.py`:
- Around line 1-7: Add a concise module-level docstring at the beginning of the
repository package’s __init__ module, before the Repository and AsyncRepository
imports, to resolve pylint C0114 while preserving the existing exports.
In `@sanctumlabs_dbkit/sql/repository/async_repository.py`:
- Around line 48-64: Update the repository create method to be async and await
both AsyncSession operations in its refresh branch: change create to async def,
await self.session.flush(), and await self.session.refresh(model_instance),
while preserving model construction, session.add, return behavior, and the
refresh flag semantics.
In `@sanctumlabs_dbkit/sql/session/__init__.py`:
- Around line 1-15: Add a concise module-level docstring at the start of
sanctumlabs_dbkit.sql.session’s __init__.py, before the imports, to satisfy
pylint C0114 while preserving the existing exports and import structure.
In `@sanctumlabs_dbkit/sql/session/async_session.py`:
- Around line 22-26: Update AsyncSession.begin so it passes nested=True to the
superclass whenever nested is explicitly requested or self.in_transaction() is
already true, matching the synchronous Session.begin behavior and the
transaction decorator contract.
- Around line 59-100: Update async_transaction to be a regular def that returns
the async wrapper, so decorator application produces a callable. Inside wrapper,
await func(self, *args, **kwargs) within the session transaction context, and
return the wrapper cast to FuncT to satisfy typing.
- Around line 51-56: Update the async transaction decorator’s wrapper to await
the coroutine returned by func before returning its result, and return the
wrapper through the appropriate cast to FuncT, matching the sync
Session.transaction implementation and satisfying mypy.
---
Nitpick comments:
In `@sanctumlabs_dbkit/sql/repository/async_repository.py`:
- Around line 104-116: Update AsyncRepository.delete to explicitly handle a
missing entity returned by find, using find_or_raise if deletion should reject
non-existent or already soft-deleted records; otherwise document the intentional
idempotent no-op behavior in the method contract. Preserve the existing
soft-deletion validation and deleted_at assignment.
- Around line 25-130: Extract the duplicated query-building and soft-deletion
logic from AsyncRepository, including _supports_soft_deletion and query, into a
shared base class or mixin used by both repository implementations. Update
AsyncRepository and the sync Repository to reuse these shared symbols while
preserving their existing async and sync database operations and filtering
behavior.
In `@sanctumlabs_dbkit/sql/repository/types.py`:
- Around line 1-10: Add a descriptive module docstring at the beginning of
types.py, before the imports, documenting the purpose of RepositoryBaseModel and
the T type variable while leaving their existing definitions unchanged.
In `@sanctumlabs_dbkit/sql/session/types.py`:
- Around line 1-3: Add a concise module-level docstring at the beginning of
types.py before the typing imports, describing the module’s type definitions or
session-related typing helpers; leave FuncT unchanged.
🪄 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: 2f9c3669-61aa-4734-9b02-e9b899f6403f
📒 Files selected for processing (10)
sanctumlabs_dbkit/sql/callbacks.pysanctumlabs_dbkit/sql/repository/__init__.pysanctumlabs_dbkit/sql/repository/async_repository.pysanctumlabs_dbkit/sql/repository/repository.pysanctumlabs_dbkit/sql/repository/types.pysanctumlabs_dbkit/sql/session/__init__.pysanctumlabs_dbkit/sql/session/async_session.pysanctumlabs_dbkit/sql/session/session.pysanctumlabs_dbkit/sql/session/types.pysanctumlabs_dbkit/sql/types.py
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Lusina <12752833+BrianLusina@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Lusina <12752833+BrianLusina@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Lusina <12752833+BrianLusina@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Lusina <12752833+BrianLusina@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Lusina <12752833+BrianLusina@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Lusina <12752833+BrianLusina@users.noreply.github.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
sanctumlabs_dbkit/sql/types.py (1)
87-111: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDecimal normalization logic is correct and well-structured.
The
to_integral_value()comparison correctly distinguishes integral from fractional Decimals, and the recursive traversal ofdict/list/tuplecontainers is sound.One minor observation: the function name
_normalise_json_compatible_valueimplies broad JSON compatibility, but onlyDecimalis normalized. Other non-JSON-native types thatmodel_dump(mode='python')can produce (e.g.,datetime,UUID,bytes) pass through unchanged. This is not a regression from the prior code, but if such fields exist on models usingColumnUsesPydanticModelsMixin, they could still cause serialization issues downstream. Worth keeping in mind if the scope of normalization expands in the future.🤖 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 `@sanctumlabs_dbkit/sql/types.py` around lines 87 - 111, The current Decimal normalization and recursive traversal in _normalise_json_compatible_value are correct; no code changes are required for this review comment. Keep the existing scope unchanged and treat support for datetime, UUID, bytes, or other non-JSON-native values as future work only if normalization requirements expand.
🤖 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.
Nitpick comments:
In `@sanctumlabs_dbkit/sql/types.py`:
- Around line 87-111: The current Decimal normalization and recursive traversal
in _normalise_json_compatible_value are correct; no code changes are required
for this review comment. Keep the existing scope unchanged and treat support for
datetime, UUID, bytes, or other non-JSON-native values as future work only if
normalization requirements expand.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 963cb392-8d1a-4e07-ab28-01371615939f
📒 Files selected for processing (6)
sanctumlabs_dbkit/sql/repository/__init__.pysanctumlabs_dbkit/sql/repository/async_repository.pysanctumlabs_dbkit/sql/session/__init__.pysanctumlabs_dbkit/sql/session/async_session.pysanctumlabs_dbkit/sql/types.pytests/sql/conftest.py
🚧 Files skipped from review as they are similar to previous changes (4)
- sanctumlabs_dbkit/sql/session/init.py
- sanctumlabs_dbkit/sql/repository/init.py
- sanctumlabs_dbkit/sql/session/async_session.py
- sanctumlabs_dbkit/sql/repository/async_repository.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@sanctumlabs_dbkit/sql/repository/async_repository.py`:
- Around line 168-215: Move the shared find and find_or_raise implementations
into AsyncBaseRepository so AsyncWriteRepository.delete can resolve
self.find(pk) when used standalone. Remove the duplicate methods from
AsyncReadRepository, preserve their existing behavior, and keep the
AsyncWriteRepository docstring’s find usage valid.
🪄 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: 8d21e3f4-b7d4-481b-ba06-c330855c1cf9
📒 Files selected for processing (4)
sanctumlabs_dbkit/sql/repository/__init__.pysanctumlabs_dbkit/sql/repository/async_repository.pysanctumlabs_dbkit/sql/repository/repository.pytests/sql/conftest.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/sql/conftest.py
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
sanctumlabs_dbkit/sql/repository/async_repository.py (1)
187-191: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove redundant
__init__method.The constructor redundantly sets
self.modelandself.sessionafter callingsuper().__init__, which already performs these exact assignments. You can remove this method entirely to inherit the parent class's constructor.♻️ Proposed fix
- def __init__(self, model: Type[T], session: AsyncSession) -> None: - """Creates an instance of the Repository""" - super().__init__(model, session) - self.model = model - self.session = session -🤖 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 `@sanctumlabs_dbkit/sql/repository/async_repository.py` around lines 187 - 191, Remove the redundant __init__ method from the repository class so it inherits the parent constructor. The parent initialization already assigns model and session, so retain the class behavior without duplicating those assignments.
🤖 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 `@Makefile`:
- Around line 69-72: Update the aggregate lint target to depend on lint-ty, and
add the lint-ty invocation to the relevant jobs in .github/workflows/lint.yml
and .gitlab-ci.yml so type checking runs in both local and CI lint workflows.
In `@sanctumlabs_dbkit/sql/repository/async_repository.py`:
- Line 177: Update the AsyncWriteRepository declaration to inherit from
AsyncBaseRepository[T] instead of the unparameterized AsyncBaseRepository,
preserving the existing T generic context for inherited methods such as find.
Remove the redundant Generic[T] base if parameterizing AsyncBaseRepository[T]
makes it unnecessary.
---
Nitpick comments:
In `@sanctumlabs_dbkit/sql/repository/async_repository.py`:
- Around line 187-191: Remove the redundant __init__ method from the repository
class so it inherits the parent constructor. The parent initialization already
assigns model and session, so retain the class behavior without duplicating
those assignments.
🪄 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: f7468637-4563-492b-8db7-fed311ed7c1a
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
Makefilepyproject.tomlsanctumlabs_dbkit/sql/mixins.pysanctumlabs_dbkit/sql/repository/async_repository.pysanctumlabs_dbkit/sql/repository/repository.pysanctumlabs_dbkit/sql/types.py
🚧 Files skipped from review as they are similar to previous changes (2)
- sanctumlabs_dbkit/sql/repository/repository.py
- sanctumlabs_dbkit/sql/types.py
Comment on lines
+69
to
+72
| .PHONY: lint-ty | ||
| lint-ty: ## Runs type checking with ty | ||
| poetry run ty check | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Wire lint-ty into the enforced lint workflow.
This target is currently opt-in: make lint omits it, and the supplied .github/workflows/lint.yml and .gitlab-ci.yml jobs do not invoke it. Add lint-ty to the aggregate target and add it to CI; otherwise the new type checker will not gate changes.
Proposed Makefile change
-lint: format-black lint-flake8 lint-mypy lint-pylint
+lint: format-black lint-flake8 lint-mypy lint-pylint lint-ty🤖 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 `@Makefile` around lines 69 - 72, Update the aggregate lint target to depend on
lint-ty, and add the lint-ty invocation to the relevant jobs in
.github/workflows/lint.yml and .gitlab-ci.yml so type checking runs in both
local and CI lint workflows.
| return scalars.first() | ||
|
|
||
|
|
||
| class AsyncWriteRepository(AsyncBaseRepository, Generic[T]): |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Parameterize the generic base class directly.
Inheriting from AsyncBaseRepository without specifying its type parameter makes it fall back to AsyncBaseRepository[Any], causing inherited methods like find to lose their type context (Optional[Any]). Inheriting from AsyncBaseRepository[T] explicitly propagates the type and implicitly applies Generic[T].
♻️ Proposed fix
-class AsyncWriteRepository(AsyncBaseRepository, Generic[T]):
+class AsyncWriteRepository(AsyncBaseRepository[T]):📝 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
| class AsyncWriteRepository(AsyncBaseRepository, Generic[T]): | |
| class AsyncWriteRepository(AsyncBaseRepository[T]): |
🤖 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 `@sanctumlabs_dbkit/sql/repository/async_repository.py` at line 177, Update the
AsyncWriteRepository declaration to inherit from AsyncBaseRepository[T] instead
of the unparameterized AsyncBaseRepository, preserving the existing T generic
context for inherited methods such as find. Remove the redundant Generic[T] base
if parameterizing AsyncBaseRepository[T] makes it unnecessary.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Adds async types to session and repository implementations
How can we test this?
make testChecklist:
Summary by CodeRabbit
New Features
Improvements
Decimalvalues to JSON-friendly numeric forms.Tests
Decimal-aware JSON serializer.