Skip to content

✨ feat: add unit of work abstraction - #9600

Open
giancarloromeo wants to merge 10 commits into
ITISFoundation:masterfrom
giancarloromeo:feature/service-library-unit-of-work
Open

✨ feat: add unit of work abstraction#9600
giancarloromeo wants to merge 10 commits into
ITISFoundation:masterfrom
giancarloromeo:feature/service-library-unit-of-work

Conversation

@giancarloromeo

@giancarloromeo giancarloromeo commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

What do these changes do?

This pull request introduces a new, abstract unit-of-work (UoW) contract for managing database read and transactional scopes, and provides a concrete implementation for asyncpg/Postgres using SQLAlchemy.

Usage example

Construct the concrete factory at the application composition boundary:

from simcore_postgres_database.unit_of_work import SqlAlchemyUnitOfWorkFactory

unit_of_work_factory = SqlAlchemyUnitOfWorkFactory(engine=asyncpg_engine)

Keep orchestration backend-neutral and open the scope only around contiguous database work:

from common_library.unit_of_work import ReadUnitOfWork, UnitOfWorkFactory

async def list_conversations(
    unit_of_work_factory: UnitOfWorkFactory,
) -> list[Conversation]:
    async with unit_of_work_factory.read() as unit_of_work:
        user = await users_service.get_current_user(
            unit_of_work=unit_of_work,
        )
        return await conversations_repository.list_for_user(
            user.id,
            unit_of_work=unit_of_work,
        )

async def get_current_user(
    *,
    unit_of_work: ReadUnitOfWork,
) -> User:
    # Nested database calls reuse the active scope instead of checking out
    # another connection.
    return await users_repository.get_current_user(
        unit_of_work=unit_of_work,
    )

Only the postgres repository unwraps the backend-specific connection:

from common_library.unit_of_work import ReadUnitOfWork
from simcore_postgres_database.unit_of_work import get_sqlalchemy_connection

async def list_for_user(
    user_id: int,
    *,
    unit_of_work: ReadUnitOfWork,
) -> list[Conversation]:
    connection = get_sqlalchemy_connection(unit_of_work)
    result = await connection.execute(query.where(conversations.c.user_id == user_id))
    return list(result.mappings())

For writes, use unit_of_work_factory.transaction(). Nested write operations receive and reuse the resulting TransactionalUnitOfWork; the outer scope owns commit or rollback.

Related issue/s

How to test

Dev-ops

  • No changes.

@giancarloromeo giancarloromeo self-assigned this Aug 27, 2026
@giancarloromeo giancarloromeo added this to the War Pigs milestone Aug 27, 2026
@github-actions github-actions Bot added the a:services-library issues on packages/service-libs label Aug 27, 2026
@giancarloromeo giancarloromeo added t:maintenance Maintenance work; used to filter tasks for end-of-sprint reporting in Review (Agreed July 3, Retro) and removed a:services-library issues on packages/service-libs labels Aug 27, 2026
@codecov

codecov Bot commented Aug 27, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.77%. Comparing base (469c84e) to head (8eec439).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #9600      +/-   ##
==========================================
+ Coverage   88.16%   89.77%   +1.60%     
==========================================
  Files        1563     1241     -322     
  Lines       60730    48047   -12683     
  Branches     1586      684     -902     
==========================================
- Hits        53541    43132   -10409     
+ Misses       6767     4759    -2008     
+ Partials      422      156     -266     
Flag Coverage Δ
integrationtests 71.88% <ø> (-0.02%) ⬇️
unittests 90.07% <100.00%> (+3.07%) ⬆️
Components Coverage Δ
pkg_aws_library ∅ <ø> (∅)
pkg_celery_library ∅ <ø> (∅)
pkg_dask_task_models_library ∅ <ø> (∅)
pkg_models_library 92.62% <ø> (ø)
pkg_notifications_library ∅ <ø> (∅)
pkg_postgres_database 90.35% <100.00%> (+0.21%) ⬆️
pkg_service_integration ∅ <ø> (∅)
pkg_service_library ∅ <ø> (∅)
pkg_settings_library ∅ <ø> (∅)
pkg_simcore_sdk 86.37% <ø> (-0.05%) ⬇️
agent 93.91% <ø> (ø)
api_server 92.97% <ø> (ø)
autoscaling 95.21% <ø> (ø)
catalog 92.47% <ø> (ø)
clusters_keeper 98.61% <ø> (ø)
dask_sidecar 93.45% <ø> (+0.15%) ⬆️
datcore_adapter 98.08% <ø> (ø)
director 79.01% <ø> (ø)
director_v2 91.92% <ø> (-0.12%) ⬇️
dynamic_scheduler 96.17% <ø> (+0.19%) ⬆️
dynamic_sidecar 72.55% <ø> (-16.11%) ⬇️
efs_guardian 89.40% <ø> (ø)
invitations 91.63% <ø> (ø)
payments 92.49% <ø> (ø)
resource_usage_tracker 91.78% <ø> (+0.15%) ⬆️
storage 88.03% <ø> (+0.03%) ⬆️
webclient ∅ <ø> (∅)
webserver ∅ <ø> (∅)

Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 469c84e...8eec439. Read the comment docs.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@giancarloromeo giancarloromeo changed the title ✨ feat(service-library): add unit of work abstraction ✨ feat: add unit of work abstraction Aug 27, 2026
@github-actions github-actions Bot added a:api framework api, data schemas, a:database associated to postgres service and postgres-database package labels Aug 27, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Introduces a backend-neutral unit-of-work contract and a SQLAlchemy/PostgreSQL implementation.

Changes:

  • Adds read and transactional UoW abstractions.
  • Implements connection acquisition, reuse, commit, and rollback.
  • Adds focused contract and lifecycle tests.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file
File Description
packages/common-library/src/common_library/unit_of_work.py Defines UoW contracts.
packages/common-library/tests/test_unit_of_work.py Tests abstract contracts and scope reuse.
packages/postgres-database/src/simcore_postgres_database/unit_of_work.py Implements SQLAlchemy-backed scopes.
packages/postgres-database/tests/unit_of_work/conftest.py Provides test fixtures and scope fakes.
packages/postgres-database/tests/unit_of_work/test_sqlalchemy_unit_of_work.py Tests lifecycle, reuse, validation, and rollback.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@giancarloromeo
giancarloromeo marked this pull request as ready for review August 27, 2026 09:26

@sanderegg sanderegg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure I see the advantage over packages/postgres-database/src/simcore_postgres_database/utils_repos.py

I find it:

  • less readable

if you want to pass around the connection, now instead of passing the engine you pass the unitofwork + you need to have the right one I guess depending if you are using read or write.

does this command pattern only apply to sql? or you plan to use that elsewhere? and if yes to what?

*,
existing: ReadUnitOfWork | None = None,
) -> AbstractAsyncContextManager[ReadUnitOfWork]:
return _read_scope(self.engine, existing)

Copy link
Copy Markdown
Member

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 ? from packages/postgres-database/src/simcore_postgres_database/utils_repos.py ?

Copy link
Copy Markdown
Contributor Author

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.

Copy link
Copy Markdown
Member

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?

Copy link
Copy Markdown
Member

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.

*,
existing: TransactionalUnitOfWork | None = None,
) -> AbstractAsyncContextManager[TransactionalUnitOfWork]:
return _transaction_scope(self.engine, existing)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same question. what is the difference to using
transaction_context from packages/postgres-database/src/simcore_postgres_database/utils_repos.py?

@pcrespov pcrespov left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you know, this is an abstraction I'm quite fond of. I like abstractions, but if we only have ONE implementation, we risk designing it to fit that single case rather than remaining truly abstract.

Could you create a Unit of Work implementation for redis and one for filesystem?

Also, could you clarify how utilities like pass_or_acquire_connection and transaction_context fit into this? Does this mean we deprecate them, or can they be reused within the UoW?

@giancarloromeo

Copy link
Copy Markdown
Contributor Author

So, pass_or_acquire_connection and his brother transaction_context are low-level utils.
Distributed transactions should be managed at service layer, not inside single repository calls!
Usage of pass_or_acquire_connection at upper levels would couple low-level concepts THERE (e.g. sqlalchemy.ext.asyncio.{AsyncConnection, AsyncEngine}).

@sonarqubecloud

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a:api framework api, data schemas, a:database associated to postgres service and postgres-database package t:maintenance Maintenance work; used to filter tasks for end-of-sprint reporting in Review (Agreed July 3, Retro)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants