Repository navigation
fix(postgres): forward SSL/TLS config to psycopg - #60
Merged
Merged
Conversation
…paces Every BaseExpression subclass must live under the backend's expression namespace, and the name `types` should mean exactly one thing per layer. Two problems are fixed here. First, PostgresEnumType is an expression (it renders a type reference for casts and column definitions, and its own docstring says it is not for DDL), but it was defined in types/enum.py alongside a manager. Move it to expression/enum_.py, matching the enum_.py naming already used by the core expression types package. EnumTypeManager is a DDL facade, so it moves to a top-level enum_type_manager.py, alongside transaction.py and its TransactionManager. Second, types/ held 44 domain value types (PostgresVector, PostgresMoney, TsQueryNode, ...) that have nothing to do with expressions. Renaming the package to type_values/ frees `types` to mean DataType expressions, so the boundary becomes checkable. The full impl.postgres.<Name> API is unchanged; only the impl.postgres.types.* path moves, with no compatibility shim. Imports are rewritten by resolving each relative import against the filesystem, so the 17 sibling `from .types import` sites in expression/, protocols/, mixins/ and explain/ - which resolve to their own DataType, Protocol, mixin and DTO modules - are deliberately left untouched. Adds a guardrail test that imports every backend module and asserts that no BaseExpression subclass is defined outside the expression namespace.
The guardrail collected every transitive BaseExpression subclass in the process, which includes the core expressions in rhosocial.activerecord.backend.expression. Those are legitimate, so the assertion flagged all of them and failed. Constrain the check to classes actually defined inside this backend's package: a class under impl.postgres must be under impl.postgres.expression. Classes from the core and from other backends are out of scope.
…without a driver Every backend __init__ imported all of its submodules eagerly, and both .backend and .adapters import psycopg. Python executes a parent package's __init__ before any submodule, so `impl.postgres.expression` could not be imported without psycopg present - even though no expression class touches it, and type_values/range.py documents that PostgresRange works without psycopg installed. The eager adapters import defeated that intent. Public names are now resolved on first access through a PEP 562 module __getattr__, with the name -> (module, attribute) map derived from the existing import statements. Also repairs two names that __all__ has promised since v1.0.0.dev12 but that no import provides: gen_random_uuid and uuid_default_generator, whose imports were dropped while the __all__ entries were left behind. Both exist in .functions, so `from impl.postgres import *` raised AttributeError on the release branch and now succeeds. The exported surface is otherwise unchanged: attribute access, __all__, dir() and `from impl.postgres import *` all behave as before, and the previous eager imports move into an `if TYPE_CHECKING:` block so mypy and IDEs still resolve them. 296 public names verified to resolve.
Drop the top-level impl/<backend>/__init__.py so the directory becomes a PEP 420 namespace portion and no longer re-exports every public name. Each name is now imported from the sub-package that defines it, so a symbol has one public path instead of two. The facade also made "expression is importable on its own" unenforceable: importing impl.<backend>.expression executed the parent __init__, which pulled in the driver. With the file gone, the sub-packages stand alone by construction rather than by a lazy __getattr__ that each backend had to maintain separately. This is a breaking change for anyone importing `from rhosocial.activerecord.backend.impl.<backend> import X`; the replacement is `...impl.<backend>.<module> import X`.
Move the sync and async classes into a backend/ package with the fixed layout: backend/backend.py and backend/async_backend.py. Eight backends kept them as two modules beside backend/, and sqlite and postgres used backend/sync.py, so the class locations differed three ways between backends. With the layout fixed the worker tasks derive the async class from the sync module's own package instead of searching for it, so the _find_async_backend helper is gone.
The move into backend/ shifted every relative import by one level, and the function-local ones were missed, so impl.bigquery could not reach its adapters module. Resolve each target against the filesystem instead of counting dots.
impl.<backend>.backend already re-exported the sync class, so leaving the async one out made the two classes reachable by different rules. Export both. The eight backends whose async driver is a hard requirement import it eagerly. SQLite and SQL Server keep a PEP 562 __getattr__, because aiosqlite and aioodbc ship as the [async] extra and importing either eagerly would make the async class a requirement for sync-only users.
The test checked that impl.postgres re-exported PostgresStorageParameter, which is a property of the flat facade that has been removed. Assert it on impl.postgres.storage_parameters instead.
The worker tasks resolve the backend class by importing backend_module and reading the attribute off it. Pointing that at impl.postgres, now a bare namespace, made every worker task fail with "module has no attribute 'PostgresBackend'". The package that re-exports the class is impl.postgres.backend.
The CLI resolves AsyncPostgresBackend inside backend_async_factory by importing impl.postgres.backend.async_backend, so patching the bare impl.postgres namespace left the real class in place. Point the patch at the defining module.
…d/base Constructing either backend raised ModuleNotFoundError: No module named 'rhosocial.activerecord.backend.impl.adapters'. The adapter registration in backend_base.py reached for ..adapters, but backend_base.py sits directly in impl/postgres, so the second dot climbed to impl and adapters resolved one level too high. The identical text in backend/base.py is correct only because that file sits one directory deeper. backend_base.py was a byte-for-byte copy of backend/base.py, kept alive only by two stale modules: backend/sync.py, itself a copy of backend/backend.py differing in nothing but that same import, and the pre-restructure top-level async_backend.py. Deleting backend_base.py without those would trade one breakage for another, so all three import sites now name backend/base.py. Both classes now construct and build their dialect, verified against PostgresConnectionConfig. Also corrects backend/base.py's file header, which still named the deleted path.
The remaining pre-restructure copy of async_backend.py, left over from moving both classes into the backend package. It differed from backend/async_backend.py only in its import depths, and those depths were wrong: ..expression and ..introspection climbed to impl.expression and impl.introspection, one level short of backend.expression and impl.postgres.introspection. They sit inside function bodies, so nothing failed until the code path ran. The canonical backend/async_backend.py resolves both correctly from its deeper position. Deleting the stale copy rather than correcting it also keeps a single definition per class; the two introspection docs now name the canonical path. Verified both classes still construct, and that resolving every relative import across all ten backend repositories and the core now finds no unresolvable module, down from 19.
The convention is that the first line names the module's own path, so a stale name sends the next reader to a file that is not there. These were left behind by earlier renames, when table.py became ddl_table.py and show_types.py became show/types.py. Comment only; no behaviour changes.
The README's 15-row version table described PostgreSQL's own feature history rather than what this backend gates on, and several thresholds were off by a minor release (RETURNING 8.2 vs 8.4, JSONB 9.4 vs 9.4, JSON_TABLE 12 vs 13, MERGE 15 vs 16). Replaced with a table generated from the dialect's supports_* switches. The core dependency was documented as >=1.0.0, which cannot resolve against the 1.0.0.devN pre-release line; pyproject had the correct value. In the agent docs: the architecture file imported from a nonexistent impl.postgresql package and named the class PostgreSQLBackend (it is PostgresBackend), listed a CTEQueryMixin that does not exist in the ActiveQuery composition, showed __init__.py for packages that are PEP 420 namespace packages, inverted the IActiveRecord relationship to BaseActiveRecord, and mislabelled ValidationError's base. Also corrected the core dependency and extras listings, and the PYTHONPATH guidance, which omitted tests -- without it the shared testsuite cannot resolve its provider registry.
psycopg 3.3.6 declares requires-python >=3.10 while 3.2.13 supports >=3.8, so a single floor of >=3.2.13 leaves 3.10 and above on the older line. The backend is tested against 3.8 through 3.15, and the driver gate belongs with the other version-dependent behaviour rather than being left to resolve to whatever is newest. Both pyproject.toml and requirements.txt carry the same two markers, since CI installs the backend with ".[test]" and the requirements file is used for developer setup. Verified the markers evaluate as intended: 3.8 and 3.9 select >=3.2.13, 3.10 through 3.15 select >=3.3.6, and both bounds match the published metadata.
PostgresConnectionConfig declares the libpq-named PostgresSSLMixin (sslmode/sslcert/sslkey/sslrootcert/sslcrl/sslcompression) and does not inherit the generic SSLMixin, so it has no ssl_ca/ssl_cert/ssl_key/ssl_mode attributes at all. Both connect() implementations nevertheless guarded on those four names with hasattr(), so every check was False and no TLS setting ever reached psycopg.connect(). A connection configured with sslmode="verify-full" ran in plaintext, and an unreadable sslrootcert was silently ignored instead of failing the handshake. Collect the configured libpq keywords once in PostgresConnectionConfig.get_ssl_connection_params() and call it from the sync and async connect paths, so the two cannot drift apart again. The kwargs extraction lists in both backends now accept the libpq names. Verified against a PostgreSQL 19beta4 server: pg_stat_ssl.ssl is now False for sslmode=disable and True for sslmode=require, and verify-full with a missing root CA is rejected at connect time.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release/v1.0.0.dev17 #60 +/- ##
========================================================
- Coverage 76.92% 75.06% -1.86%
========================================================
Files 330 338 +8
Lines 15360 15807 +447
Branches 2606 2680 +74
========================================================
+ Hits 11816 11866 +50
- Misses 2580 2986 +406
+ Partials 964 955 -9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Replace the six divergent changelog-check variants with one implementation
that encodes the documented merge rules.
feature/*|fix/* -> release/*|maint/*|main:
- the PR must add at least one changelog.d/<number>.<type>.md
- every added fragment's <number> must equal the PR number
- fragments already present from previously merged branches are ignored,
since only files ADDED by this PR are considered
- escape hatches ([trivial] title, skip-changelog label, docs: update
CHANGELOG) waive only the "at least one" requirement; the
number-must-match rule is always enforced on fragments that were added
release/* -> main:
- CHANGELOG.md must have changed (content is not inspected)
- changelog.d/ must hold no fragments at the merge commit
- no escape hatches apply to this path
The number-must-match rule was the one documented requirement that none of
the previous variants implemented. Two latent bugs are also fixed: the
paths-ignore of "**.md" meant a CHANGELOG.md-only push never ran the check at
all, and three backends skipped release merges outright instead of applying
the release rules.
A feature/fix branch merges into a release or maintenance branch; only release/* merges into main. Opening a second PR at main from the same branch is a duplicate, and it also breaks the changelog fragment, which is numbered for exactly one PR. This pattern already cost a wasted PR in six repositories (a ->release PR merged alongside a ->main PR that was then closed), and the fragment numbering check only exposed it indirectly, by failing the leftover PR. hotfix/* -> main stays allowed; the version control policy calls for hotfixes to be merged back to main and the active release branch.
|
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.
Description
Fixes # (issue)
PostgresConnectionConfigdeclares the libpq-namedPostgresSSLMixin(
sslmode/sslcert/sslkey/sslrootcert/sslcrl/sslcompression) and does notinherit the generic
SSLMixin, so it has nossl_ca/ssl_cert/ssl_key/ssl_modeattributes at all.
Both
connect()implementations nevertheless guarded on those four names withhasattr():Every check was therefore
Falseand no TLS setting ever reachedpsycopg.connect().The mapping was also wrong on its own terms:
ssl_cawas assigned tosslcertratherthan
sslrootcert, andsslcertwas assigned twice.Security impact
Certificate verification was silently disabled. With no
sslmodepassed at all,libpq falls back to its own default of
prefer: TLS is still negotiated, but theserver certificate is not verified.
So a connection configured with
sslmode="verify-full"orsslmode="verify-ca"behaved exactly like
sslmode="prefer"— encryption without authentication, leavingit open to an active man-in-the-middle. The traffic was not plaintext on a
TLS-capable server, but it was not verified either, and nothing in the application
indicated the difference. Certificates supplied via
sslrootcert/sslcert/sslkeywere ignored outright, so a broken or wrong CA path never surfaced as an error.
Measured on a PostgreSQL 19beta4 server with SSL enabled, before the fix:
Anyone who relied on
sslmodeto protect credentials or data in transit shouldtreat the affected versions as not providing certificate verification and
re-verify their deployments after upgrading.
Reproduction Steps
Configure a connection that requires verified TLS, then inspect the session:
Expected: connection refused —
/nonexistent/ca.crtcannot be loaded, soverify-fullmust fail.Actual: the session connects with TLS, ignoring both
verify-fulland theunreadable CA path. Passing the same config straight to
psycopg.connect()does failwith
root certificate file "/nonexistent/ca.crt" does not exist, which confirms theconnection-string fields themselves are fine and only the driver call was wrong.
The CLI's
--ssl verify-fullmaps toconfig.sslmodeand is lost at the same point.Type of change
fix: A bug fixrefactor: A code change that neither fixes a bug nor adds a feature (collecting the libpq keywords in one place, shared by the sync and async connect paths)test: Adding tests for the fixBreaking Change
Applicable
rhosocial-activerecordVersion Range:>=1.0.0,<2.0.0Related Repositories/Packages
This is one of five coordinated fixes. The same class of defect — SSL parameters
collected on the config but never handed to the driver — was found and fixed
alongside in
python-activerecord-mariadb,python-activerecord-mysql,python-activerecord-oracleandpython-activerecord-sqlserver. No sharedpython-activerecordchange is required; each backend translates the genericSSLMixinnames to its own driver's keywords.Testing
Test Plan:
New
tests/rhosocial/activerecord_postgres_test/feature/backend/test_ssl_connection_params.pyasserts the libpq keywords reach
psycopg.connect()on both the sync and the asyncpath, that the generic SSL names are absent from the config, and that no stray
ssl*key is emitted when nothing is configured. The sync test fails against the pre-fix code.
Verified against a live PostgreSQL 19beta4 server, after the fix:
pg_stat_sslsslmode=disable(False, None, None)sslmode=require(True, 'TLSv1.3', 'TLS_AES_256_GCM_SHA384')verify-full+ missing CAverify-full+ bogus client certsslmode=disableandsslmode=requirenow produce distinguishable sessions, which isthe observable proof that the setting reaches the driver at all.
Full
tests/rhosocial/activerecord_postgres_test/feature/backend/suite passes(excluding the pre-existing
test_old_types_package_is_gonefailure, which failsidentically on the unmodified branch). Lint and mypy show no new findings.
Checklist
README.md, etc.).changelog.d/<issue_number>.<type>.md). — will be added aschangelog.d/<pr_number>.fixed.mdonce CI passes.Additional Notes
This branch also carries accumulated work that predates this fix: the
impl.postgres.backendpackage reorganisation, thetype_values/expressionnamespace split, lazy backend exports, and the README/
.claudedocumentationrewrite (16 commits ahead of
release/v1.0.0.dev17in total).Worth flagging separately:
backend/sync.pyis a byte-identical duplicate ofbackend/backend.py(only the header comment differs) and nothing imports it.Both are patched here so the dead copy cannot be resurrected with the bug intact,
but collapsing the duplication would be a worthwhile follow-up.