-
Notifications
You must be signed in to change notification settings - Fork 30
✨ feat: add unit of work abstraction #9600
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
59e8d89
b51e3d1
9a9ea5b
0cecfb4
f93c419
80dbcf8
eb862b4
3325114
a358352
8eec439
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| from abc import ABC, abstractmethod | ||
| from contextlib import AbstractAsyncContextManager | ||
|
|
||
|
|
||
| class ReadUnitOfWork: | ||
| """Opaque persistence scope for sequential reads.""" | ||
|
|
||
|
|
||
| class TransactionalUnitOfWork(ReadUnitOfWork): | ||
| """Opaque persistence scope for sequential reads and writes.""" | ||
|
|
||
|
|
||
| class UnitOfWorkFactory(ABC): | ||
| @abstractmethod | ||
| def read( | ||
| self, | ||
| *, | ||
| existing: ReadUnitOfWork | None = None, | ||
| ) -> AbstractAsyncContextManager[ReadUnitOfWork]: ... | ||
|
|
||
| @abstractmethod | ||
| def transaction( | ||
| self, | ||
| *, | ||
| existing: TransactionalUnitOfWork | None = None, | ||
| ) -> AbstractAsyncContextManager[TransactionalUnitOfWork]: ... |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,75 @@ | ||
| import inspect | ||
| from contextlib import AbstractAsyncContextManager | ||
| from types import TracebackType | ||
|
|
||
| from common_library.unit_of_work import ( | ||
| ReadUnitOfWork, | ||
| TransactionalUnitOfWork, | ||
| UnitOfWorkFactory, | ||
| ) | ||
|
|
||
|
|
||
| class _ReadUnitOfWork(ReadUnitOfWork): ... | ||
|
|
||
|
|
||
| class _TransactionalUnitOfWork(TransactionalUnitOfWork): ... | ||
|
|
||
|
|
||
| class _UnitOfWorkContext[UnitOfWorkT: ReadUnitOfWork]: | ||
| def __init__(self, unit_of_work: UnitOfWorkT) -> None: | ||
| self._unit_of_work = unit_of_work | ||
|
|
||
| async def __aenter__(self) -> UnitOfWorkT: | ||
| return self._unit_of_work | ||
|
|
||
| async def __aexit__( | ||
| self, | ||
| exc_type: type[BaseException] | None, | ||
| exc_value: BaseException | None, | ||
| traceback: TracebackType | None, | ||
| ) -> bool: | ||
| return False | ||
|
|
||
|
|
||
| class _IncompleteUnitOfWorkFactory(UnitOfWorkFactory): ... | ||
|
|
||
|
|
||
| class _UnitOfWorkFactory(UnitOfWorkFactory): | ||
| def read( | ||
| self, | ||
| *, | ||
| existing: ReadUnitOfWork | None = None, | ||
| ) -> AbstractAsyncContextManager[ReadUnitOfWork]: | ||
| self._read_calls += 1 | ||
| return _UnitOfWorkContext(existing or _ReadUnitOfWork()) | ||
|
|
||
| def transaction( | ||
| self, | ||
| *, | ||
| existing: TransactionalUnitOfWork | None = None, | ||
| ) -> AbstractAsyncContextManager[TransactionalUnitOfWork]: | ||
| self._transaction_calls += 1 | ||
| return _UnitOfWorkContext(existing or _TransactionalUnitOfWork()) | ||
|
|
||
| def __init__(self) -> None: | ||
| self._read_calls = 0 | ||
| self._transaction_calls = 0 | ||
|
|
||
|
|
||
| def test_incomplete_unit_of_work_factory_cannot_be_instantiated(): | ||
| assert inspect.isabstract(_IncompleteUnitOfWorkFactory) | ||
|
|
||
|
|
||
| async def test_unit_of_work_factory_contract_supports_new_and_existing_scopes(): | ||
| factory = _UnitOfWorkFactory() | ||
|
|
||
| async with factory.read() as read_uow: | ||
| assert isinstance(read_uow, ReadUnitOfWork) | ||
| async with factory.read(existing=read_uow) as reused_read_uow: | ||
| assert reused_read_uow is read_uow | ||
|
|
||
| async with factory.transaction() as transactional_uow: | ||
| assert isinstance(transactional_uow, TransactionalUnitOfWork) | ||
| assert isinstance(transactional_uow, ReadUnitOfWork) | ||
| async with factory.transaction(existing=transactional_uow) as reused_transactional_uow: | ||
| assert reused_transactional_uow is transactional_uow |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,86 @@ | ||
| from collections.abc import AsyncIterator | ||
| from contextlib import AbstractAsyncContextManager, asynccontextmanager | ||
| from dataclasses import dataclass | ||
|
|
||
| from common_library.unit_of_work import ( | ||
| ReadUnitOfWork, | ||
| TransactionalUnitOfWork, | ||
| UnitOfWorkFactory, | ||
| ) | ||
| from sqlalchemy.ext.asyncio import AsyncConnection, AsyncEngine | ||
|
|
||
|
|
||
| @dataclass(frozen=True, kw_only=True, slots=True) | ||
| class _SqlAlchemyReadUnitOfWork(ReadUnitOfWork): | ||
| connection: AsyncConnection | ||
|
|
||
|
|
||
| @dataclass(frozen=True, kw_only=True, slots=True) | ||
| class _SqlAlchemyTransactionalUnitOfWork(TransactionalUnitOfWork): | ||
| connection: AsyncConnection | ||
|
|
||
|
|
||
| def get_sqlalchemy_connection(unit_of_work: ReadUnitOfWork) -> AsyncConnection: | ||
| if isinstance( | ||
| unit_of_work, | ||
| (_SqlAlchemyReadUnitOfWork, _SqlAlchemyTransactionalUnitOfWork), | ||
| ): | ||
| return unit_of_work.connection | ||
| msg = f"Expected a SQLAlchemy unit of work, got {type(unit_of_work).__name__}" | ||
| raise TypeError(msg) | ||
|
|
||
|
|
||
| def get_sqlalchemy_transaction_connection( | ||
| unit_of_work: TransactionalUnitOfWork, | ||
| ) -> AsyncConnection: | ||
| if isinstance(unit_of_work, _SqlAlchemyTransactionalUnitOfWork): | ||
| return unit_of_work.connection | ||
| msg = f"Expected a SQLAlchemy transactional unit of work, got {type(unit_of_work).__name__}" | ||
| raise TypeError(msg) | ||
|
|
||
|
|
||
| @asynccontextmanager | ||
| async def _read_scope( | ||
| engine: AsyncEngine, | ||
| existing: ReadUnitOfWork | None, | ||
| ) -> AsyncIterator[ReadUnitOfWork]: | ||
| if existing is not None: | ||
| get_sqlalchemy_connection(existing) | ||
| yield existing | ||
| return | ||
|
|
||
| async with engine.connect() as connection: | ||
| yield _SqlAlchemyReadUnitOfWork(connection=connection) | ||
|
|
||
|
|
||
| @asynccontextmanager | ||
| async def _transaction_scope( | ||
| engine: AsyncEngine, | ||
| existing: TransactionalUnitOfWork | None, | ||
| ) -> AsyncIterator[TransactionalUnitOfWork]: | ||
| if existing is not None: | ||
| get_sqlalchemy_transaction_connection(existing) | ||
| yield existing | ||
| return | ||
|
|
||
| async with engine.begin() as connection: | ||
| yield _SqlAlchemyTransactionalUnitOfWork(connection=connection) | ||
|
|
||
|
|
||
| @dataclass(frozen=True, kw_only=True, slots=True) | ||
| class SqlAlchemyUnitOfWorkFactory(UnitOfWorkFactory): | ||
| engine: AsyncEngine | ||
|
|
||
| def read( | ||
| self, | ||
| *, | ||
| existing: ReadUnitOfWork | None = None, | ||
| ) -> AbstractAsyncContextManager[ReadUnitOfWork]: | ||
| return _read_scope(self.engine, existing) | ||
|
|
||
| def transaction( | ||
| self, | ||
| *, | ||
| existing: TransactionalUnitOfWork | None = None, | ||
| ) -> AbstractAsyncContextManager[TransactionalUnitOfWork]: | ||
| return _transaction_scope(self.engine, existing) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same question. what is the difference to using
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's actually a step-down since it will not create a nested transaction if one is already in_transaction. |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,68 @@ | ||
| from dataclasses import dataclass | ||
| from types import TracebackType | ||
| from typing import cast | ||
|
|
||
| import pytest | ||
| from simcore_postgres_database.unit_of_work import SqlAlchemyUnitOfWorkFactory | ||
| from sqlalchemy.ext.asyncio import AsyncConnection, AsyncEngine | ||
|
|
||
|
|
||
| @dataclass | ||
| class _ScopeState: | ||
| closed: bool = False | ||
| committed: bool = False | ||
| rolled_back: bool = False | ||
|
|
||
|
|
||
| class _ConnectionScope: | ||
| def __init__(self, connection: AsyncConnection, state: _ScopeState) -> None: | ||
| self._connection = connection | ||
| self._state = state | ||
|
|
||
| async def __aenter__(self) -> AsyncConnection: | ||
| return self._connection | ||
|
|
||
| async def __aexit__( | ||
| self, | ||
| exc_type: type[BaseException] | None, | ||
| exc_value: BaseException | None, | ||
| traceback: TracebackType | None, | ||
| ) -> bool: | ||
| self._state.closed = True | ||
| return False | ||
|
|
||
|
|
||
| class _TransactionScope(_ConnectionScope): | ||
| async def __aexit__( | ||
| self, | ||
| exc_type: type[BaseException] | None, | ||
| exc_value: BaseException | None, | ||
| traceback: TracebackType | None, | ||
| ) -> bool: | ||
| self._state.closed = True | ||
| self._state.committed = exc_type is None | ||
| self._state.rolled_back = exc_type is not None | ||
| return False | ||
|
|
||
|
|
||
| class _Engine: | ||
| def __init__(self) -> None: | ||
| self.connection = cast(AsyncConnection, object()) | ||
| self.read_scopes: list[_ScopeState] = [] | ||
| self.transaction_scopes: list[_ScopeState] = [] | ||
|
|
||
| def connect(self) -> _ConnectionScope: | ||
| state = _ScopeState() | ||
| self.read_scopes.append(state) | ||
| return _ConnectionScope(self.connection, state) | ||
|
|
||
| def begin(self) -> _TransactionScope: | ||
| state = _ScopeState() | ||
| self.transaction_scopes.append(state) | ||
| return _TransactionScope(self.connection, state) | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def sqlalchemy_uow_factory() -> tuple[SqlAlchemyUnitOfWorkFactory, _Engine]: | ||
| engine = _Engine() | ||
| return SqlAlchemyUnitOfWorkFactory(engine=cast(AsyncEngine, engine)), engine |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
besides the fact that this looks a bit more complex to read, what is the difference with using
pass_or_acquire_connection? frompackages/postgres-database/src/simcore_postgres_database/utils_repos.py?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's totally wrong to bring low-level concepts at service layer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, this is not an answer.
So this is just a wrapper around the async engine used as transaction or as connect to bring it up one level and not see asyncpg anywhere in the imports?
technically speaking this changes nothing else. unless this is applied in a generic manner to other dependencies such as redis, celery, ...
in terms of performance I doubt this will improve it as this creates more classes, calls, etc etc. not sure python excels at that.
a downside is that then if I have code that access the DB to read and optionally need to write I will now need 2 different objects instead of one engine. Why not have just one abstraction then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also please if this gets in, then do a complete migration and not a partial one.