Coerce user/database/server_settings to plain str before the wire - #1343
Open
pranjalm37 wants to merge 1 commit into
Open
Coerce user/database/server_settings to plain str before the wire#1343pranjalm37 wants to merge 1 commit into
pranjalm37 wants to merge 1 commit into
Conversation
WriteBuffer.write_str() (in the vendored pgproto submodule) is typed
to accept exactly str and does not accept str subclasses -- e.g.
enum.StrEnum members, or third-party string-like values such as
tomlkit's -- even though isinstance(x, str) is True for them. When one
of these reaches the startup packet via user/database/server_settings,
building it raises a TypeError deep inside the compiled protocol
layer, which in turn triggers a secondary AttributeError ('Protocol'
object has no attribute '_on_error') while trying to report the
original failure, masking the real cause entirely.
Coerce user, database, and server_settings keys/values to plain str
right after they're resolved/validated in
_parse_connect_dsn_and_args(), before they ever reach the protocol
layer.
Verified against a real PostgreSQL server (Docker) with an
enum.StrEnum user and server_settings entry: TypeError/AttributeError
before the fix, successful connection after.
Fixes MagicStack#1340.
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.
Fixes #1340.
Root cause
WriteBuffer.write_str(), which builds the startup packet, lives in the vendoredpgprotosubmodule and is declared as:That
str-typed parameter rejectsstrsubclasses at the C level —enum.StrEnummembers, or third-party string-like values liketomlkit.items.String— even thoughisinstance(x, str)isTruefor them (confirmed in the issue). When one of these reaches the startup packet viauser,database, or aserver_settingskey/value, building it raises aTypeErrordeep inside the compiled protocol layer. That in turn triggers a second failure —AttributeError: 'Protocol' object has no attribute '_on_error'— while trying to report the original error, which is why the issue's traceback shows the confusingAttributeErrorrather than the realTypeError.The actual type check lives in the separate
MagicStack/py-pgprotorepo (asyncpg's git submodule), so I fixed this at the asyncpg call site instead — coercinguser,database, andserver_settingskeys/values to plainstrright after they're resolved/validated in_parse_connect_dsn_and_args(), before they ever reach the protocol layer. This keeps the fix in one repo and doesn't require asyncpg to bump its pinned pgproto submodule version.hostdoesn't need the same treatment — it's consumed for the socket connection itself, not written viawrite_str()in the startup packet, and the issue's own repro confirmshost=-only StrEnum values work fine (the crash only happens onceuseris also aStrEnum).Verification
I built the Cython extension locally (
pip install --no-build-isolation -e ., submodule initialized) and reproduced the exact reported failure end-to-end against a real PostgreSQL server (Docker,postgres:16-alpine):asyncpg.connect(user=SomeStrEnum.USER, ...)→AttributeError: 'Protocol' object has no attribute '_on_error', matching the issue's traceback exactly.SELECT current_userround-trips correctly.I also verified with a minimal in-process fake TCP server (accepts the connection, replies
Nto the SSL negotiation probe, doesn't require a real PostgreSQL) that the client-side startup-packet construction itself succeeds post-fix (67-byte packet sent, then a clean timeout waiting on the fake server — no moreTypeError/AttributeError).Changes
asyncpg/connect_utils.py: coerceuser/databasetostrright after theirNone-checks in_parse_connect_dsn_and_args(); coerceserver_settingsdict keys/values tostrright after its existingisinstancevalidation.tests/test_connect.py: addedtest_connect_params_coerces_str_subclasses, which passesstr-subclassuser/database/server_settingsvalues (custom subclasses, not relying onenum.StrEnum) and assertstype(...) is stron the resulting_ConnectionParameters— confirmed this fails without the fix (AssertionError: <class '...SUser'> != <class 'str'>) and passes with it.Testing
python -m unittest tests.test_connect.TestConnectParams— all 12 tests pass (this class unit-tests_parse_connect_dsn_and_args()directly, no live cluster needed).flake8on both changed files — clean (the one pre-existingE731inconnect_utils.pyis on an unrelated, untouched line).tests.test_connectmodule also spins up a local Postgres cluster viapg_config/initdbfor other test classes, which isn't available in my environment — those failures are unrelated to this change (I confirmed the affected functionality directly against a real Dockerized PostgreSQL instead, as described above).🤖 Generated with Claude Code