Skip to content

Add atomic transaction support: per-method session/commit parameters and transaction() context manager#28

Open
Copilot wants to merge 5 commits intomasterfrom
copilot/handle-transactional-logic
Open

Add atomic transaction support: per-method session/commit parameters and transaction() context manager#28
Copilot wants to merge 5 commits intomasterfrom
copilot/handle-transactional-logic

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Feb 19, 2026

Every adapter write method previously opened and committed its own session, making it impossible to atomically combine Casbin policy changes with other application DB operations (e.g. creating a user and assigning their policies in one transaction).

Changes

adapter.py

  • _session_scope(commit=True, session=None) — accepts an explicit session (yielded as-is, no commit/rollback) and a commit flag (controls auto-commit on internally-managed sessions). Constructor-level db_session behaviour is unchanged.
  • _save_policy_line — simplified from async dual-mode to a synchronous helper that always takes a session; callers own the session scope.
  • All eight write methods gain session: Optional[AsyncSession] = None and commit: bool = True keyword parameters (fully backward-compatible):
    • add_policy, add_policies
    • remove_policy, remove_policies, remove_filtered_policy
    • update_policy, update_policies, update_filtered_policies
  • update_policies opens one session scope and threads it through each update_policy call.
  • _update_filtered_policies propagates session/commit to its internal remove_policies and add_policies calls.
  • transaction(session=None, commit=True) — new async context manager that uses contextvars.ContextVar (per-asyncio-task, concurrency-safe) to bind a session to all adapter operations within the block. The adapter can be retrieved directly from an existing enforcer via enforcer.get_adapter(), so no separate import or variable is needed:
adapter = enforcer.get_adapter()

async with adapter.transaction(session=my_session):
    await enforcer.add_policy(...)
    await enforcer.add_policies(...)
await my_session.commit()  # or await my_session.rollback()

README.md

  • Replaced the old "External Session Support" section with a new "Atomic Transactions (Transaction Control)" section.
  • Documents the transaction() context manager as the primary pattern, showing enforcer.get_adapter() to retrieve the adapter without a separate import.
  • Includes an atomic user-creation example (flush user row → run enforcer calls in transaction() → single commit covers everything).
  • Documents per-method session/commit parameters for direct adapter calls.
  • Retains the constructor-level db_session option as a sub-section for backward compatibility.

tests/test_external_session.py

  • 8 new tests covering commit and rollback paths for each write method via per-method session/commit params.
  • 4 new tests for the transaction() context manager: commit, rollback, mixed operations, and asyncio task isolation.
  • Shared _make_adapter(), _load_enforcer(), and _make_enforcer() helpers to reduce setup duplication.

Example

async def create_user(session, user_data):
    # Insert the user row – not yet committed.
    user = User(**user_data)
    session.add(user)
    await session.flush()        # get user.id without committing

    adapter = enforcer.get_adapter()
    async with adapter.transaction(session=session):
        await enforcer.add_role_for_user(str(user.id), "viewer")
        await enforcer.add_policy(str(user.id), "resource", "read")

    # One commit covers the user row AND all policy changes.
    await session.commit()
    # If anything raises before this point, a rollback leaves
    # the database in a clean state — no orphaned policies.

Alternatively, for direct adapter calls:

await adapter.add_policy("p", "p", [str(user.id), "resource", "read"],
                         session=session, commit=False)
await session.commit()
Original prompt

This section details on the original issue you should resolve

<issue_title>Handle transactional logic?</issue_title>
<issue_description># How it works now
Right now, this module require to create adapter EVERY time the transaction occurs (ref). However, what we need is to be able to provide already generated session to the Adapter's methods. There is no point to re-create adapter itself, the DB URI likely won't ever change.

Problem

In terms of atomic DB-operations, Casbin adapter is a bit "difficult" to work with.

For example, when a User created, we want to assign some policies.

async def create_user(session, ...):
    user = ...  # Handled via session, but not yet commited...
    await session.refresh(user)

    # Since AsyncAdapter uses `self._session_scope()` all the time, we can't control the flow whatsoever...
    await enforcer.add_group(...)  # Assign `user.id` to a group...
    await enforcer.add_policies(...)  # Assign permissions to `user.id`...

    # Something happens, rollback. No user is created in DB...
    # But group is assigned to non-existing ID and this ID has permissions...

And here our atomicity breaks completely, because we haven't yet commited created user and a rollback might happen. If rollback occurs, we have to clean up manually.

Solution (?)

Provide 2 parameters parameters to most of the methods to handle transactions to DB:

  • session: AsyncSession | None = None ― either use provided session or use self._session_scope() to generate a new session;
  • commit: bool = True ― whether we need to commit or not.

Full list of the methods:

  • Adapter.add_policy(<previous signature>, commit: bool = True, session: AsyncSession | None = None)
  • Adapter.add_policies(<previous signature>, commit: bool = True, session: AsyncSession | None = None)
  • Adapter.remove_policy(<previous signature>, commit: bool = True, session: AsyncSession | None = None)
  • Adapter.remove_policies(<previous signature>, commit: bool = True, session: AsyncSession | None = None)
  • Adapter.remove_filtered_policy(<previous signature>, commit: bool = True, session: AsyncSession | None = None)
  • Adapter.update_policy(<previous signature>, commit: bool = True, session: AsyncSession | None = None)
  • Adapter.update_policies(<previous signature>, commit: bool = True, session: AsyncSession | None = None)
  • Adapter.update_filtered_policies(<previous signature>, commit: bool = True, session: AsyncSession | None = None)

And the session-related method should now become aware of externally used session provided for/within methods (not constructor).

@asynccontextmanager
async def _session_scope(self, commit: bool = True, session: AsyncSesssion | None = None) -> AsyncSession:
    """Provide an asynchronous transactional scope around a series of operations."""

    # Instead of using a session passed in the constructor, let's pass it
    if session:
        yield session
    else:
        # Use internal session with automatic commit/rollback
        async with self.session_local() as session:
            try:
                yield session
                if commit:
                    await session.commit()
            except Exception as e:
                await session.rollback()
                raise e
```</issue_description>

## Comments on the Issue (you are @copilot in this section)

<comments>
</comments>

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

@hsluoyz hsluoyz marked this pull request as ready for review February 19, 2026 18:09
…atomic transaction support

Co-authored-by: hsluoyz <3787410+hsluoyz@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix transactional logic in Casbin adapter Add per-method session and commit parameters for atomic transaction support Feb 19, 2026
Copilot AI requested a review from hsluoyz February 19, 2026 18:14
@hsluoyz
Copy link
Copy Markdown
Member

hsluoyz commented Feb 20, 2026

@copilot fix linter

Co-authored-by: hsluoyz <3787410+hsluoyz@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Feb 20, 2026

@copilot fix linter

Fixed in c0060d7 — ran black to reformat adapter.py.

@hsluoyz
Copy link
Copy Markdown
Member

hsluoyz commented Feb 20, 2026

@UnoYakshi can you test this PR?

@UnoYakshi
Copy link
Copy Markdown

@hsluoyz, I'll try in a couple of days.

@UnoYakshi
Copy link
Copy Markdown

UnoYakshi commented Feb 26, 2026

Issue

I see a big issue with docs/abstraction. Now we have to work with adapter directly instead of using enforcer.

await enforcer.add_policy(...)

becomes

# Or import adapter instead of enforcer,
# which is a bit odd, considering the naming...
adapter = enforcer.get_adapter()

await adapter.add_policy(..., session=async_session, commit=False)
await async_session.commit()

Potential Solution?

Ideal case, IMO, would be to have **kwargs in AsyncEnforcer's methods.

await enforcer.add_policy(..., session=async_session, commit=False)
await async_session.commit()

@hsluoyz
Copy link
Copy Markdown
Member

hsluoyz commented Feb 26, 2026

@copilot # Issue
I see a big issue with docs/abstraction. Now we have to work with adapter directly instead of using enforcer.

await enforcer.add_policy(...)

becomes

# Or import adapter instead of enforcer,
# which is a bit odd, considering the naming...
adapter = enforcer.get_adapter()

await adapter.add_policy(..., session=async_session, commit=False)
await async_session.commit()

Potential Solution?

Ideal case, IMO, would be to have **kwargs in AsyncEnforcer's methods.

await enforcer.add_policy(..., session=async_session, commit=False)
await async_session.commit()

…ion control

Co-authored-by: hsluoyz <3787410+hsluoyz@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Feb 26, 2026

@copilot # Issue
I see a big issue with docs/abstraction. Now we have to work with adapter directly instead of using enforcer.

...

Added a transaction() async context manager to Adapter in 282ca5f. It uses contextvars.ContextVar (per-asyncio-task, so concurrency-safe) to bind a session for the duration of the block, so you can use enforcer methods directly:

async with adapter.transaction(session=my_session):
    await enforcer.add_policy(...)
    await enforcer.add_policies(...)
await my_session.commit()  # or rollback

The **kwargs approach would require upstream changes to casbin's AsyncEnforcer (since _add_policy calls adapter.add_policy(sec, ptype, rule) with fixed positional args only). The transaction() CM is the equivalent pattern that stays within this repo — the enforcer's adapter must be the same instance used in adapter.transaction(...).

Copilot AI changed the title Add per-method session and commit parameters for atomic transaction support Add atomic transaction support: per-method session/commit parameters and transaction() context manager Feb 26, 2026
@UnoYakshi
Copy link
Copy Markdown

It does look a bit better now. But we still use adapter. Hence, we need to import it or get from enforcer.

The **kwargs approach would require upstream changes to casbin's AsyncEnforcer (since _add_policy calls adapter.add_policy(sec, ptype, rule) with fixed positional args only).

Yes, that was exactly my point.

@hsluoyz
Copy link
Copy Markdown
Member

hsluoyz commented Feb 26, 2026

@ It does look a bit better now. But we still use adapter. Hence, we need to import it or get from enforcer.

The **kwargs approach would require upstream changes to casbin's AsyncEnforcer (since _add_policy calls adapter.add_policy(sec, ptype, rule) with fixed positional args only).

Yes, that was exactly my point.

@UnoYakshi
Copy link
Copy Markdown

UnoYakshi commented Feb 26, 2026

It does look a bit better now. But we still use adapter. Hence, we need to import it or get from enforcer.

The **kwargs approach would require upstream changes to casbin's AsyncEnforcer (since _add_policy calls adapter.add_policy(sec, ptype, rule) with fixed positional args only).

Yes, that was exactly my point.

@copilot

@UnoYakshi
Copy link
Copy Markdown

I don't think Copilot was called to.

@hsluoyz
Copy link
Copy Markdown
Member

hsluoyz commented Feb 26, 2026

@copilot It does look a bit better now. But we still use adapter. Hence, we need to import it or get from enforcer.

The **kwargs approach would require upstream changes to casbin's AsyncEnforcer (since _add_policy calls adapter.add_policy(sec, ptype, rule) with fixed positional args only).

Yes, that was exactly my point.

…er() pattern

Co-authored-by: hsluoyz <3787410+hsluoyz@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Feb 26, 2026

@copilot It does look a bit better now. But we still use adapter. Hence, we need to import it or get from enforcer.

...

Updated the README in 259963a. The enforcer already has a get_adapter() method, so the pattern becomes:

adapter = enforcer.get_adapter()
async with adapter.transaction(session=session):
    await enforcer.add_policy(...)
    await enforcer.add_role_for_user(...)
await session.commit()

No separate import or variable needed — enforcer.get_adapter() gives direct access to the adapter that was registered with the enforcer. The README now leads with this pattern and includes the atomic user-creation example from the original issue.

@UnoYakshi
Copy link
Copy Markdown

Well, I guess I can live with

async with enforcer.get_adapter().transaction(session=session):
    await enforcer.add_policy(...)
    await enforcer.add_role_for_user(...)
await session.commit()

The only thing left bothering me is using yet another ContextVar. It's a somewhat clean solution, but we already have ContextVar for requests' (FastAPI/Starlette) sessions. Now we have to add similar functionality w/o reusing the existing get_request_session() from our codebase.

I'm just nitpicking. Should we merge, though?..

@UnoYakshi
Copy link
Copy Markdown

UnoYakshi commented Feb 26, 2026

BTW, do we remove rules from enforcer if rollback occurs? I mean manually vs. it's handled in Enforcer+Adapter implicitly.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Handle transactional logic?

3 participants