Define the public API with __all__ and freeze it - #145
Open
roed-math wants to merge 7 commits into
Open
Conversation
…random edge cases Four independent defects found reviewing rc2, each with regression tests that fail on rc2 and pass here. max_id/min_id formatted their table argument into the statement as text. A name needing quotes was a syntax error and a name carrying its own statement ran it; both compose an Identifier now. While there, the empty sentinel is documented: max_id returns -1, and 0 is a real id, which is what random() got wrong below. _approx_most_common read reltuples from a hard-coded public.nf_fields but read frequencies from the owning table, so every table other than nf_fields got its own frequencies scaled by an unrelated row count. The row count now comes from the table the statistics are about, looked up by name in the current schema, and the column type goes through column_type_sql rather than being concatenated. update_from_file's logging default was a shared dictionary literal that the method wrote logid and aborted into. Consecutive default calls saw the previous call's values and a caller's dictionary came back modified. An AST scan of the package confirms this was the only mutable default anywhere that is actually mutated, so nothing else needed changing. random() raised IndexError from random.choice([]) when pick_first found no values, reported a table whose only row has id 0 as empty, and discarded rows whose projection was falsy -- a table of zeros exhausted maxtries and raised "Random selection failed!". random_sample returned None for an unrecognized mode, which reads like an empty result, and reseeded the global random module when asked for a repeatable sample.
Write paths called _break_stats, but read paths queried the cache tables regardless, so a count cached before a restat=False write kept being served afterwards. Reproduced against PostgreSQL 18: a query counted at 67, every matching row then changed so that none satisfy it, and quick_count still answered 67. Every lookup that would serve a cached answer now goes through one predicate, _may_use_cache, and reports a miss while stats_valid is false -- quick_count, quick_count_distinct, _quick_statistic, _has_stats, _has_numstats and null_counts, which between them are what make count, max, min, sum, column_counts and numstats recompute rather than return a stored value. Three things are deliberately outside the rule, and the predicate's docstring says why: the empty-query total, which is maintained on every write and stays exact; a suffixed table, whose caches are its own and which stats_valid says nothing about; and the _status/status/extra_counts inventory, which reports what the cache contains rather than answering a question about the data -- refresh_stats uses it to discover what to recompute, so gating it would make an invalid table forget what statistics it is supposed to have. The flag had no way back to true. _restore_stats is the counterpart of _break_stats, called at the end of refresh_stats inside the same transaction that rebuilt the caches, so a refresh that fails part-way leaves the table marked invalid rather than claiming a cache it does not have. A suffixed refresh does not touch the live flag. Separately, bulk paths now run PostgreSQL's own ANALYZE, which is a different thing from psycodict's statistics: a bulk-loaded relation has none until autovacuum reaches it and the planner costs it as though it were tiny. The _tmp copies are analyzed before the swap and outside its transaction, since the catalog entry follows the relation through a rename -- one call in _swap_in_tmp covers reload, rewrite, non-inplace update_from_file and staged commits, which all funnel through it. copy_from analyzes the live table. The existing statistics fixtures insert rows, which invalidates, and then assume a usable cache; they now say so with _restore_stats, which is true of them (nothing is cached yet and the total is maintained by the insert) and is the state a freshly loaded table is in.
Unqualified DDL and DML went wherever search_path happened to point, while catalog inspection was a mixture of hard-coded 'public' and no filter at all. With a relation of the same name in two schemas the answers came from both: _column_types unioned their columns (or raised "Type mismatch"), an index or constraint in the other schema counted as present, _all_tablenames listed the name twice, and table_sizes reported only public whatever the session was using. PostgresDatabase now takes schema="public", validates it as an identifier once in the constructor, and pins search_path to it in _configure_session -- which runs for the first connection and for every replacement, so a reconnect cannot come back pointing somewhere else. It is deliberately not part of _connect_kwargs: psycopg.connect has no such parameter, and passing it there would reach the driver. Every catalog query is then filtered to that schema, binding it as a value rather than interpolating it: _table_exists, _all_tablenames, _index_exists, _list_indexes, _relation_exists, _constraint_exists, _list_constraints, _column_types, _relation_columns, refresh_tables' column discovery, the read-only and knowls capability probes, _grantees, the legacy-extras check, table_sizes, tablespaces, _check_tmp_leftovers' two probes, the metadata bootstrap's existing-table set, _approx_most_common and dbdiff's column reader. _schema_relations and _approx_most_common previously asked the server with current_schema(); they now bind the same value as everything else, so there is one notion of which schema this is. The userdb.users grant probe keeps its own schema: that one is deliberately about a different schema, not about this database's.
…ize_changes resort() was a disabled no-op: the old implementation renumbered every id with an in-place UPDATE, which stalls replication and leaves the rows in their old physical order, so the point of id-ordering -- sequential disk reads -- was never achieved. It now rebuilds. The table is dumped without ids and reloaded; reload assigns ids 1..N in sort order via a new private _generate_sorted_ids, which is a physical rebuild (INSERT ... SELECT ... ORDER BY into a fresh table that replaces the original) rather than an in-place UPDATE. Everything else -- the primary key, indexes, constraints, grants, counts/stats companions, ANALYZE and the _oldN backup -- comes from reload's one replacement path, so resort() adds almost no orchestration of its own. reload and non-inplace update_from_file call _generate_sorted_ids directly rather than public resort(), so there is no recursion, and the renumber now runs before the keys are rebuilt (it replaces the table, so the table must have no pkey/indexes at that point). Because a resort is a full-table rebuild, it is no longer a side effect of a small write. resort=True on insert_many, update, copy_from and in-place update_from_file raises, pointing at resort(); resort=False is unaffected. rewrite defaults resort to the replacement path only, and a staged table's in-place writes drop the request. An in-place update that changes a sort key now records out_of_order rather than pretending it could reorder. finalize_changes() was a documented public no-op; it is removed. The write methods already leave total, the order flag and stats_valid correct on return, which test_write_invariants now checks for every path. scripts/audit_id_order.py is a read-only check that streams each id_ordered table in sort order (server-side cursor, constant client memory) and reports whether the ids actually increase, since the flag can drift and must not be trusted blindly during the production audit.
Track E: psycopg becomes a plain dependency, so `pip install psycodict` gets a working package (pure-Python driver on the system libpq). pgbinary adds the bundled binary build and pgc a locally compiled one; the pgsource extra is removed, since plain install replaces it, and a <4 ceiling keeps an unreviewed driver major out of a 1.x environment. CI's "import must fail without psycopg" step is inverted to "plain install must import", and both smoke installs now assert importlib.metadata.version == __version__ and run pip check. Track F: search-data export files gain an optional `# psycodict-export-format: N` marker. One shared reader (_read_header_lines) accepts a marked file, accepts an unmarked file as format 0 so every older export still loads, and refuses a version it does not understand before loading any data; the writers (copy_to via _write_header_lines, and rewrite's inline header) emit the current marker. This is the data-file format, kept distinct from the meta_* metadata format; Versioning.md states the realistic promise and decouples both format numbers from the package major. Track G: the default database name is the generic `postgres` rather than `lmfdb` (LMFDB names its own), and the unused `secretsfile` argument to PostgresDatabase is removed -- no caller in LMFDB or seminars passes it. Track H: MANIFEST.in makes the sdist a complete, testable checkout (tests with conftest.py, scripts, guides, metadata), and CI unpacks it and runs its database-free tests so a dropped file fails the build. The release workflow's third-party actions are pinned to commit SHAs.
The versioning policy said "documented non-underscore names are public", while Sphinx ran with undoc-members, so any helper that happened to have a docstring became part of the permanent 1.x surface. Each module now declares __all__ -- the curated set of names psycodict actually promises -- and the API reference documents exactly those (undoc-members is off). A non-__all__ name, docstring or not, is implementation: still importable, so nothing downstream breaks, but not promised. tests/test_public_api.py freezes every module's __all__, checks each exported name resolves, checks `from psycodict import *` binds exactly the root set, and checks the specific names LMFDB and seminars import still resolve -- including private ones (seminars' _counts_cols, _meta_*_cols) and a couple kept importable but unpromised (range_formatter, KeyedDefaultDict), since __all__ governs `import *` and the docs, not explicit imports. Versioning.md is rewritten around this: public = the __all__ names and their documented behavior; db[name] is the canonical table lookup while db.<name> is convenience a real database attribute wins over, so adding a method in a minor release never makes a table unreachable through db[name].
Documentation build overview
36 files changed ·
|
Four review points on the __all__ freeze, all in the contract and its documentation rather than the declarations themselves. 1. __version__ is unambiguously public. Versioning.md said membership in __all__ decides, then separately excluded every underscore-prefixed name with no exception, which classified the exported __version__ both ways. Both passages now apply the single test: a name is public exactly when it appears in its module's __all__, dunder or not. 2. The API reference covers the package root. docs/api/package.md documents psycodict itself; seven of its eight exports are imported members and __version__ is a special data member, so the page carries local imported-members/special-members options (the module pages keep documenting exactly their own __all__ -- verified: every page's top-level anchors now equal that module's __all__, and range_formatter, KeyedDefaultDict and number_types remain absent). __version__ gets a #: doc-comment so it renders with a description rather than str's docstring. Versioning.md's API-reference link becomes the absolute Read the Docs URL, since the canonical file is read on GitHub where docs/api/index.md does not exist. test_public_api.py gains a guard that every module in the frozen surface has an automodule page, and one that every exported callable has a docstring (without undoc-members, an undocumented export silently leaves the reference). The conf.py comment is corrected to claim only the upper bound that turning undoc-members off actually gives. 3. The changelog no longer implies __all__ is behavior-neutral. Explicit imports of a non-public name still resolve; `from ... import *` now binds __all__ and nothing else, which is a real behavior change and is described as one. test_star_import_gives_exactly_all is parametrized over every module in EXPECTED rather than the package root alone. 4. The API overview matches the collision-safe lookup policy: db[name] is canonical and db.<name> is shorthand for when the name is not shadowed. Searching.md's "equivalently" claim gets the same correction. Full suite 1439 passed, 36 skipped, 1 xfailed; ruff and the -W docs build clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Author
|
All four required changes are in 1.
|
Owner
|
GPT signed off. |
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
2 participants
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.
Sixth of the rc3 stabilization PRs: Track D.
Stacked on #140–#144.
The problem
Versioning.md said "documented non-underscore names are public", while Sphinx
ran with
undoc-members. So any helper that happened to carry a docstringbecame part of the permanent 1.x surface —
range_formatter,numeric_converter,normalize_query, the internal validators, and so on.The fix
Each module declares a curated
__all__— the names psycodict actually promises— and the API reference documents exactly those (
undoc-membersis off). Anon-
__all__name, docstring or not, is implementation: still importable, sonothing downstream breaks, but not promised.
I audited what LMFDB and seminars actually import before choosing the sets. The
curated surface, per your "curated core" preference:
SQL/Identifier/Placeholder/Literal/Composable/Composed,DelayCommit,__version__PostgresDatabase,PostgresTable,PostgresSearchTable,PostgresStatsTable,PostgresBaseConfiguration;DelayCommit,IdentifierWrapper,LockError,SearchParsingErrorJson,Array,copy_dumps;GrantPolicy,LMFDBGrantPolicyNotificationListener;compare_databases/format_differences;parse_slow_log/slow_query_report/show_slow_reportInvalidColumnTypeError,InvalidDefinitionErrorWhat that cuts from the previously-documented surface (all still importable,
just no longer promised) is listed in the PR discussion below — the notable ones
being
utils.range_formatterandutils.KeyedDefaultDict(both imported byLMFDB), the
encodingdumper classes andnumeric_converter,config'shelper functions,
table.StagedWriteContext, and the internalvalidationvalidators.
Freezing it
tests/test_public_api.py: freezes every module's__all__, checks each nameresolves, checks
from psycodict import *binds exactly the root set, andchecks the specific names LMFDB and seminars import still resolve — including
seminars' private
_counts_cols/_meta_*_colsand the kept-but-unpromisedrange_formatter/KeyedDefaultDict, since__all__governsimport *andthe docs, not explicit imports.
Versioning.md is rewritten around this, and now also states that
db[name]isthe canonical table lookup while
db.<name>is convenience a real attributewins over — so adding a method in a minor release never makes a table
unreachable through
db[name].Full suite: 1398 passed, 36 skipped, 1 xfailed. Ruff and the
-Wdocs build clean.🤖 Generated with Claude Code