Repository navigation
Python: Validate PostgreSQL collection field types - #14611
Jose Alvarez (jpalvarezl) merged 4 commits into
Conversation
Python Test Coverage Report •
Python Unit Test Overview
|
||||||||||||||||||||||||||||||
There was a problem hiding this comment.
🟡 Changes recommended
Dictionary matching accepts malformed annotations and silently converts dictionary arrays into scalar JSONB columns.
1 open finding
What changed in this PR
Restricts PostgreSQL automatic collection creation to trusted type expressions while preserving custom-type usage through pre-created tables.
Changes:
- Adds canonical type mappings and recursive array handling.
- Rejects unsupported field types before database access.
- Adds exact-DDL, rejection, and sentinel integration coverage.
| File | Description |
|---|---|
| python/tests/unit/connectors/memory/test_postgres_store.py | Tests supported aliases and early rejection. |
| python/tests/integration/memory/postgres/test_postgres_int.py | Adds sentinel-table protection coverage. |
| python/semantic_kernel/connectors/postgres.py | Validates type expressions and documents custom-type limitations. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
I ran this on Python 3.13 with psycopg 3.3.6, comparing main (7a92cd4) with the PR head.
Tests: tests/unit/connectors/memory/test_postgres_store.py gives 97 passed on main and 110 passed on the PR. With the PR's test file on main, 13 tests fail and 97 pass, so the new tests do cover the change. I did not run the integration test, which needs a database.
The injection fix works. I called the type function directly with TEXT); DROP TABLE x; --, list[int and INTEGER[] trailing. Main turns them into SQL text (TEXT); DROP TABLE X; --, LIST[INT, INTEGER[] TRAILING). The PR rejects all three with VectorStoreModelValidationError.
The part I would change. The old code passed any other type name through as upper case SQL, and that path was used by ordinary models. I took decorated dataclass models with @vectorstoremodel and called ensure_collection_exists() with the repo's unit test fixtures (mock cursor). The decorator stores the type as __name__, so these were the type strings:
| model field | main | PR |
|---|---|---|
datetime.datetime |
TIMESTAMP |
TIMESTAMP |
list[float] | None |
DOUBLE PRECISION[] |
DOUBLE PRECISION[] |
str | None |
TEXT |
TEXT |
datetime.date |
DATE |
VectorStoreModelValidationError |
uuid.UUID data field |
UUID |
VectorStoreModelValidationError |
decimal.Decimal |
DECIMAL |
VectorStoreModelValidationError |
date, UUID and Decimal were fields of the same model in my run, so I did not test them one by one. I also called the type function with strings like bigint, varchar(255), numeric(10,2) and timestamptz. Main turns each into the same name in upper case, which is valid PostgreSQL (I did not run it against a database), and the PR rejects them. (A uuid.UUID key field is rejected by the model validator on both main and PR, so that is unchanged.)
So an app that has a date, UUID or Decimal field and relies on ensure_collection_exists() will start to fail after this change. The description says advanced types should use a pre-created table, but date and UUID are not advanced. I would add the common built-in names (date, uuid, decimal, bigint, smallint, real, timestamptz) to POSTGRES_TYPE_ALIASES. Allowing a length or precision in parentheses with a strict pattern like \(\d+(,\d+)?\) would keep varchar(255) working without opening the injection path.
I think this is worth fixing before merge, so I am requesting changes. I like the approach (an allow list and sql.SQL composition) and the tests are good.
|
PRABHU KIRAN VANDRANKI (@VANDRANKI) Thanks for the detailed compatibility check. I added the common decorator-inferred types ( |

Motivation and Context
PostgreSQL automatic collection creation previously treated an unrecognized field type string as a complete SQL type expression. Automatic schema creation should accept only the field types it can safely and predictably generate.
Description
date,UUID,Decimal,BIGINT,SMALLINT,REAL, andTIMESTAMPTZ.list[...]and PostgreSQL...[]notation.VARCHAR/CHARACTER VARYINGandNUMERIC/DECIMAL.ensure_collection_exists().Validation:
uv sync --all-extras --dev --python 3.12— passeduv run --python 3.12 pytest tests/unit/connectors/memory/test_postgres_store.py— 117 passeduv run --python 3.12 pre-commit run --files semantic_kernel/connectors/postgres.py tests/unit/connectors/memory/test_postgres_store.py tests/integration/memory/postgres/test_postgres_int.py— passedContribution Checklist