fix: the defects the agents page turned up - #21
Merged
Conversation
AsyncCConnection subclasses asyncpg.Connection at module scope and the package __init__ imports it, so a clean "pip install sqlalchemy-foundation-kit" followed by "import sqlalchemy_foundation_kit" raised ModuleNotFoundError. asyncpg was only listed in the test group. The library is asyncpg-only by design -- the DSN, the connection class and the connect_args in create_async_session_manager are all asyncpg-specific -- so the honest fix is to declare it rather than make the import optional.
AsyncSessionManager.get_transaction() passed execution_options= to the session factory on every call, with or without an isolation level, and Session.__init__ has no such keyword -- so every call raised TypeError before reaching the database. The isolation level now travels with the connection checkout instead: session.begin() does not provision a connection, so the session.connection() call inside the block is the one that checks it out, and it applies the level before the connection begins its transaction. The unit tests only asserted the shape of the call to a mocked sessionmaker, which is why they agreed with the bug. One of them now builds a real session over a real engine -- that path never reaches the database, but it does construct the session, which is where the TypeError came from.
…saction apply_isolation_level() awaited session.connection() -- which checks a connection out and begins its transaction -- and only then set the level on it. PostgreSQL cannot change the isolation level of a running transaction, so SQLAlchemy raised InvalidRequestError. Every isolation_level= argument on transaction(), managed_session() and query() was therefore unusable; only the engine-level setting worked. The level is now handed to the session.connection() call that checks the connection out, which applies it and then begins. That call starts the session's transaction, so transaction() and managed_session() join the transaction that is already open instead of calling session.begin() on top of it -- begin() refuses a second one. Neither the unit tests (mocked sessionmaker) nor the integration tests (which never passed an isolation level) covered this. The integration suite now runs all three methods against PostgreSQL and reads back SHOW transaction_isolation, and gains coverage for AsyncSessionManager.get_transaction(), which had none.
try_advisory_xact_lock() turned a str key into an integer with the built-in hash(), which is salted per interpreter: three fresh processes produced three different keys for the same string. Two replicas of a service asking for the same named lock therefore took different locks and both proceeded -- the exact case the documentation recommends string keys for. BLAKE2b truncated to 64 bits replaces it, so a string is the same lock in every process. Integer keys are untouched. The key an existing process computes for a given string changes with this release. Nothing could depend on the old value across processes, but during a rolling deploy an old replica and a new one hold different locks for the same name until the rollout finishes. PostgresAdvisoryLockMixin.try_advisory_lock now types key as str | int, which is what the guides have always passed it.
Importing contrib.di without dishka failed with "AttributeError: 'NoneType' object has no attribute 'APP'", and contrib.dependency_injector without its package with the same error for 'DeclarativeContainer'. Both packages carry a check that raises a message naming the extra to install, but it never ran: the class bodies read Scope.APP and subclass containers.DeclarativeContainer, which happens before __init_subclass__. The None placeholders are now objects that run that check on first attribute access, so the import fails with the message the code meant to give. Covered by importing each package in a subprocess with its dependency hidden -- the failure is at import time, so patching a flag in an already-imported module cannot reach it.
…nstall
require_optional("orjson", "json") told the user to run
"pip install 'sqlalchemy-foundation-kit[json]'". There is no [json] extra; it
is [orjson].
Everything here was checked against the source or run: - README imported the unit of work from unit_of_work_kit, a package that does not exist here. - session_manager.close(), .close(timeout=...) and .healthcheck() appear on four pages. The only close is aclose(), the timeout is the manager's dispose_timeout, and there is no healthcheck method -- the library ships the query (DEFAULT_HEALTHCHECK_QUERY) and leaves the policy to the caller. - retry_async_connection was shown as a decorator on two pages, with RetryConfig fields (max_attempts, initial_delay, max_delay, exponential_base, jitter) that do not exist. It is a coroutine function taking connect_func, service_name and config, and the fields are max_retries, retry_delay, max_backoff_delay. - PostgresMetrics was documented with service_name and labels arguments it does not take, and with metric names missing the db_ segment they all carry. - The environment variables in the configuration guide were one underscore short: BasePostgresConfig declares no model_config, so the names come from the BaseSettings holding it and every level is a double underscore -- POSTGRES__CONNECTION__HOST. Verified against pydantic-settings. - The API reference listed create_async_session_manager under session.builder; it lives in session.factories. - README and the home page said only sqlalchemy and pydantic are required. docs/agents.md follows the fixes in this pull request: the section that listed the three broken entry points now says they are fixed and keeps the workarounds for anyone still on 0.2.0, and rules 1, 8 and 13 describe the current behaviour.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
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
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.
Writing
docs/agents.mdagainst the source turned up six defects. Every one wasreproduced before it was touched; each is below with the reproduction, the change, and
whether it breaks anything.
Nothing here is a breaking API change. Two behaviours do change for an existing caller:
asyncpgis now installed for you, and a string advisory-lock key hashes to a differentinteger than it did — see finding 4.
1.
import sqlalchemy_foundation_kitfails on a clean installsession/connection.pyimportsasyncpgat module scope (AsyncCConnectionsubclassesasyncpg.Connection), the package__init__imports it, andasyncpgwas declared onlyin the
testdependency group.Changed:
asyncpg>=0.30.0,<1.0.0added todependencies. The library isasyncpg-only by design — the DSN, the connection class and the
connect_argsincreate_async_session_managerare all asyncpg-specific — so declaring it is the honestfix rather than making the import optional. README and the home page said only
sqlalchemy[asyncio]andpydanticwere required; they now say what is true.uv.lockchanges by exactly the two lines this adds.Guarded by
tests/unit/test_distribution.py, which reads the installed metadata.2.
AsyncSessionManager.get_transaction()raisedTypeErroron every callexecution_options=was passed to the session factory unconditionally, andSession.__init__has no such keyword. No database needed:With and without
isolation_level. The unit tests mocked the sessionmaker and assertedthe broken call shape; the integration suite never touched the manager at all.
Changed: the isolation level now travels with the connection checkout.
session.begin()does not provision a connection, so thesession.connection()callinside the block is the one that checks it out, and SQLAlchemy applies execution options
to a fresh connection before beginning its transaction.
Tests: one unit test now builds a session over a real engine — that path never
reaches the database, but it constructs the session, which is where the
TypeErrorcamefrom — and
get_transactiongains integration coverage for commit, rollback and theisolation level, read back with
SHOW transaction_isolation. Against the oldmanager.py: 8 of the new tests fail.3. Every
isolation_level=argument on the unit of work raisedapply_isolation_level()awaitedsession.connection(), which checks a connection outand begins its transaction, and only then set the level on it. PostgreSQL cannot change
the isolation level of a running transaction.
Against PostgreSQL 17:
Changed: the level is handed to the
session.connection()call that checks theconnection out. That call starts the session's transaction, so
transaction()andmanaged_session()join the transaction that is already open instead of callingsession.begin()on top of it —begin()refuses a second one.transaction()drivesits own commit/rollback for the same reason; the flush-before-commit behaviour is
unchanged.
Applying the level inside
open_session()(rather than after the transaction starts) isdeliberate: the documented
open_sessionoverride runs its own statements — a GUC, anRLS context — and those would provision the connection first, at which point SQLAlchemy
would silently ignore the level rather than raise.
Tests: the integration suite now runs all three methods against PostgreSQL and reads
the level back, and covers commit and rollback with a level set. Against the old
uow/sqlalchemy.py: 17 tests fail, including all 7 new integration tests.4. String advisory-lock keys did not lock across processes
try_advisory_xact_lockhashed astrwith the built-inhash(), which is salted perinterpreter:
Two replicas of a service asking for the same named lock took different locks and both
proceeded — the exact case
guide/advanced.mdrecommends string keys for.Changed: BLAKE2b truncated to 64 bits. Integer keys are untouched.
PostgresAdvisoryLockMixin.try_advisory_locknow typeskeyasstr | int, which iswhat the guides have always passed it. The
SupportsAdvisoryLockprotocol still saysint— widening it would break anyone who has implemented it withkey: int.Not breaking, but worth knowing: the integer a given string maps to changes with this
release. Nothing could have depended on the old value across processes, but during a
rolling deploy an old replica and a new one hold different locks for the same name until
the rollout finishes.
Tests: three fresh interpreters must agree on the key, plus the string path through
try_advisory_xact_lock. Withhash()put back behind the same name, the determinismtest fails.
5.
contrib.diandcontrib.dependency_injectorfailed on import withAttributeErrorBoth packages carry a check that raises a message naming the extra to install, and it
never ran:
scope = Scope.APPandclass BaseDIContainer(containers.DeclarativeContainer)are evaluated in class bodies, which happens before
__init_subclass__.Changed: the
Noneplaceholders are now objects that run the check on first attributeaccess, so the import fails with the intended
ImportError.Tests: each package is imported in a subprocess with its dependency hidden by a
meta-path blocker — the failure is at import time, so patching a flag in an
already-imported module cannot reach it. Both fail against the old
_deps.py.6. Documentation
require_optional("orjson", "json")told the user to install[json]. There is nosuch extra; it is
[orjson]. That one is a code fix — the message is what a user sees.unit_of_work_kit.session_manager.close(),.close(timeout=...)and.healthcheck()appear on fourpages. The only close is
aclose(), the timeout is the manager'sdispose_timeout,and there is no healthcheck method — the library ships the query
(
DEFAULT_HEALTHCHECK_QUERY) and leaves the policy to the caller, which is what the DIproviders do. The pages now show that.
retry_async_connectionwas shown as a decorator on two pages, withRetryConfigfields (
max_attempts,initial_delay,max_delay,exponential_base,jitter)that do not exist. It is a coroutine function taking
connect_func,service_nameandconfig; the fields aremax_retries,retry_delay,max_backoff_delay.PostgresMetricswas documented withservice_nameandlabelsarguments it does nottake, and with metric names missing the
db_segment they all carry.BasePostgresConfigdeclares nomodel_config, so the names come from theBaseSettingsthat holds it and every level is a double underscore:POSTGRES__CONNECTION__HOST. Every name in the new block was run againstpydantic-settings, including the
MY_APP_prefix variant andBasePostgresMigrationsConfig.create_async_session_managerundersession.builder; itlives in
session.factories.docs/agents.mdThe page's "Broken at 0.2.0" section listed exactly findings 2, 3 and 5, and rules 1, 8
and 13 stated the buggy behaviour as rules. The section is now "Fixed since 0.2.0" —
same table, reframed, keeping the workarounds for anyone pinned to 0.2.0, since the fixes
are not in a release yet. Rules 1, 8 and 13, the
get_transactionrow, the isolation-levelnote on the unit-of-work table, the advisory-lock paragraph, the errors table and the
string-key entry under "Common mistakes" all follow the code. The
Versionrow no longerclaims 0.2.0 semantics for behaviour that is not 0.2.0's.
Verification
uv sync --frozen --group dev --all-extrasis clean.uv.lockchanged by the two linesfinding 1 requires and nothing else.
Clean-install check after the fixes:
pip install sqlalchemy-foundation-kitpullsasyncpg,
import sqlalchemy_foundation_kitworks, and both contrib DI packages raisetheir intended
ImportError.