Mdmapp migration#8
Merged
Merged
Conversation
…gration MDMApp is migrating its production Postgres layer and test-DB bootstrap onto pgdevkit; these are the gaps that migration needs closed: - pgdevkit.db.complex_types.ComplexHelper: promote the composite/enum/JSONB adapter out of the skills reference doc into real, importable code, with a `normalizers` hook for project-specific composite value backfilling (e.g. a locale-labels type) without hardcoding that into pgdevkit. - pgdevkit.db.crud: pg_retrieve/pg_retrieve_many now auto-wrap composite/ enum columns in to_jsonb() on SELECT; pg_insert/pg_insert_many/ pg_upsert_dict/pg_upsert_many_dict convert dict/list values destined for those columns via ComplexHelper. Tables with no complex columns keep the existing fast path. - pgdevkit.db.connection.PgPool: credential_kind (default_azure/ managed_identity), max_lifetime, use_null_pool "auto"-detection (with matching prepare_threshold=None) for real Azure Postgres hosts, and dsn_params passthrough — closes the gap with a hand-rolled Azure AD + PgBouncer connection layer. - pgdevkit.testdb.schema: apply_schema now also applies purely-additive `**/migrations/*.sql` files (previously skipped outright) and seeds composite/enum test-data columns correctly instead of naively json.dumps()-ing them. Two real bugs found and fixed along the way: - ComplexHelper.complex_types was a mutable *class* attribute, so a CompositeInfo/EnumInfo (which carries OIDs from one connection/database) leaked across every other ComplexHelper instance that looked up the same type *name* — exactly the case for pgdevkit's own per-worktree isolated test databases, which legitimately reuse type names like "locale_labels" with different OIDs per database. Now instance-scoped. - recursive_convert checked `isinstance(value, list)` before `info == Jsonb`, so a JSONB column holding a JSON array value was wrongly treated as an array-of-composites and split into a list of individually-Jsonb-wrapped scalars instead of one Jsonb(list) value. Also added is_azure_postgres_host() (strict suffix match) alongside the existing detect_provider() (whose "azure_postgres" is a fallback default, not a positive match) — PgPool's null-pool auto-detection needs the strict version so plain localhost/dev hosts aren't mistaken for Azure Postgres. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ad1oGCCUheJVhcmwxHWsdU
…_many Both needed for MDMApp's migration to delegate its write-path CRUD to pgdevkit without behavior loss: - must_exist=True switches pg_upsert_many_dict to a plain UPDATE (no INSERT) matched on primary_keys — used by an existing MDMApp caller that only ever updates pre-existing rows. - pg_insert_many now accepts Sequence[dict | BaseModel] — an existing MDMApp caller streams typed model instances (not dicts) into it directly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ad1oGCCUheJVhcmwxHWsdU
MDMApp's Procrastinate integration needs the underlying psycopg_pool object directly (open_async(pool=...)), not just connection() — expose it as a public property instead of callers reaching into the private _pool attr. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ad1oGCCUheJVhcmwxHWsdU
The json.dumps() fallback for non-complex dict/list values was wrong: a plain Postgres array column (text[] etc.) isn't "complex" (ComplexHelper correctly returns None for it -- psycopg already adapts a Python list to it natively), so json.dumps()-ing its value produced a JSON string like "[]" that psycopg then tried to bind to an array column, failing with "malformed array literal". Found by running ensure_testdb() against MDMApp's real (126-file) database/ tree, which has such columns. Only convert when info is not None (matching the original mdm_common behavior this was ported from, which never had this fallback at all). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ad1oGCCUheJVhcmwxHWsdU
The original mdm_common code this was ported from filters test-data JSON
keys down to columns that actually exist in the table before building the
INSERT (real_cols = {col: ... for col in json_keys if col in all_table_cols})
-- a resilience feature for fixtures left behind after a column was renamed
or removed. My port dropped that filtering, using the JSON keys directly
as the INSERT column list, so a stale key crashed with UndefinedColumn.
Found the same way as the previous fix: running ensure_testdb() against
MDMApp's real database/ tree, where akeneo_attribute.test_data.json has
exactly this (an "options" key from a column that's since been split into
its own table).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Ad1oGCCUheJVhcmwxHWsdU
- complex_types.py: enum_types CTE only matched a column's bare enum type name, never the underscore-prefixed array variant (information_schema reports "_mood" for a mood[] column), so is_enum was always computed False for array-of-enum columns -- load_all_complex_types returned a CompositeInfo instead of EnumInfo, crashing recursive_convert's `assert isinstance(info, EnumInfo)`. Fixed the join condition in both queries; array-of-composite was already unaffected (composite arrays live in a non-pg_catalog schema, a different code path). - testdb/schema.py: _is_additive_migration only checked for ALTER COLUMN (unpaired with ADD COLUMN) and OWNER TO -- DROP COLUMN, DROP TABLE, TRUNCATE, and RENAME COLUMN/TO all passed through as "safe", meaning a destructive migration file would silently replay on every apply_schema() call. Added those patterns to the denylist. - db/crud.py: _select_list called load_all_complex_types with the default include_generated=False, so any GENERATED ALWAYS column was silently dropped from the SELECT list whenever a complex_helper was passed -- unlike the plain `SELECT *` fallback, which always includes them. Also fixed a pre-existing ty check failure in testdb/constants.py's conninfo() (unrelated to the above, from a separate parallel change that landed on this branch via a main merge): params: dict[str, str | int] unpacked via **params could theoretically bind an int to make_conninfo's str-typed `conninfo` positional param. Made port/connect_timeout strings. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ad1oGCCUheJVhcmwxHWsdU
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
1 participant
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.
No description provided.