Skip to content

fix 2 issues regarding connection#94

Merged
bmichele merged 6 commits into
SkeneTechnologies:mainfrom
theodufort:feat/correct-postgresql-connection-issues
Jul 7, 2026
Merged

fix 2 issues regarding connection#94
bmichele merged 6 commits into
SkeneTechnologies:mainfrom
theodufort:feat/correct-postgresql-connection-issues

Conversation

@theodufort

@theodufort theodufort commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes the PostgreSQL foreign-key introspection query in postgres_live.py which had two compounding bugs:

  1. Cartesian product on composite FKs — the information_schema join on constraint_name alone produced N² rows for N-column FKs, causing phantom cross-matches and incorrect column pairings.

  2. constraint_column_usage lacks ordinal_position — this information_schema view doesn't expose the column position, so even with an ordinal_position guard the join was unreliable across PostgreSQL versions.

How it's fixed: The FK query now queries pg_constraint directly, pairing referencing columns (conkey) with referenced columns (confkey) via generate_subscripts — a version-stable approach that correctly handles single-column and composite FKs alike.

Alongside the fix, live integration tests were added so this behavior can be validated against a real database, and error messages from introspect_db() now redact passwords.

Commit history

Commit Message Notes
362e5c0 fix: correlate FK columns by ordinal_position to prevent Cartesian product Closes #92
d946622 fix: redact password from introspect_db() error messages Closes #93
ce445eb feat(postgres_live): fix FK query and add live integration tests

Test Plan

  • Mocked tests: uv run pytest tests/test_schema_parsers/test_postgres_live.py — all 18 mocked tests pass (single FK, views, multi-schema, composite FK, duplicate constraint names).
  • Live tests: uv run pytest tests/ -m db --db-url "postgresql://user:pass@localhost/db" -v — 5 live tests create their own tables, verify FK pairing (single + composite), multi-schema isolation, and run clean teardown. Skipped automatically without --db-url.
  • Full suite: uv run pytest tests/ — 428 passed, 1 skipped.

Checklist

  • PR is focused on a single change (PostgreSQL FK introspection fix) — no unrelated refactoring
  • Tests added/updated for behavioral changes (5 live tests + composite FK + duplicate constraint name mocks)
  • Documentation updated (no public API or flag changes)
  • Linting and tests pass locally (uv run ruff check && uv run pytest)
  • No merge conflicts

…oduct

The foreign-key introspection query joined information_schema
constraint_column_usage to key_column_usage on constraint_name alone,
producing a Cartesian product for composite FKs (N columns → N² rows)
and phantom cross-matches for same-named constraints on different tables.

Added AND ccu.ordinal_position = kcu.ordinal_position to the join
condition so columns pair correctly by position within the constraint.

Also added tests for composite FKs and duplicate constraint names.
@theodufort
theodufort requested a review from bmichele as a code owner July 5, 2026 21:16

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 362e5c0060

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/skene/analyzers/schema_parsers/postgres_live.py Outdated
Catch psycopg.Error at psycopg.connect() and re-raise with a sanitized
message so the connection string (including password) never appears in
CLI output, CI logs, or shell history.

The existing _redact_db_url() only covered the happy-path status message;
libpq's DSN parser echoes the full connection string in its error for
malformed DSNs, leaking passwords.

Tests: update test_password_not_in_exception to patch psycopg.connect
(where the real leak occurs) with a libpq-style error containing the
password, and assert it is not present in the re-raised exception.
- Rewrite FK query to use pg_constraint directly instead of
  information_schema.constraint_column_usage (which lacks
  ordinal_position in PostgreSQL).  The new query pairs referencing
  columns (conkey) with referenced columns (confkey) via
  generate_subscripts for correct composite-FK support.

- Add --db-url pytest CLI option and pg_conn fixture so live tests
  can connect to a real database.  Tests marked @pytest.mark.db are
  skipped when --db-url is not provided.

- Add TestLivePostgresLive with 5 live tests:
  - test_empty_db_returns_empty_index
  - test_single_table_basic (columns, PK, index)
  - test_foreign_keys_live (single-column FK)
  - test_composite_foreign_key_live (multi-column FK pairing)
  - test_multi_schema_no_collision_live (same table name in different schemas)

- Register 'db' marker in pyproject.toml so tests are properly
  deselected by default.

@bmichele bmichele left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for fixing those issues! 💪

Before merging:

  • I think we should avoid having a detailed docs/guides/testing.md. No one is going to read it anyway. Instead, add a short reference in CONTRIBUTING.md
  • fix linting/formatting issues (see current CONTRIBUTING.md)

I can them merge the PR 😃

Comment thread docs/guides/testing.md Outdated
@bmichele
bmichele merged commit e2173a9 into SkeneTechnologies:main Jul 7, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants