feat(python-driver): add public API for connection pooling and model dict conversion#2374
feat(python-driver): add public API for connection pooling and model dict conversion#2374uesleilima wants to merge 5 commits intoapache:masterfrom
Conversation
…dict conversion Add two enhancements to the Python driver's public API: 1. Add configure_connection() function that registers AGE agtype adapters on an existing psycopg connection without creating a new one. This enables use with external connection pools (e.g. psycopg_pool) and managed PostgreSQL services where LOAD 'age' may be restricted. Also explicitly export AgeLoader and ClientCursor as public symbols in age/__init__.py. (apache#2369) 2. Add to_dict() methods to Vertex, Edge, and Path model classes for conversion to plain Python dicts. This enables direct JSON serialization with json.dumps() without requiring custom conversion logic. (apache#2371) - Vertex.to_dict() returns {id, label, properties} - Edge.to_dict() returns {id, label, start_id, end_id, properties} - Path.to_dict() returns a list of to_dict() results Closes apache#2369 Closes apache#2371
…plugins - Replace confusing `skip_load` (double-negative) with `load` (positive boolean, default False). The default now correctly matches the intent: no LOAD by default for connection pool / managed PostgreSQL use cases. - Add `load_from_plugins` parameter for parity with setUpAge(). - Fix docstring to accurately describe parameter behavior. - Add 6 unit tests for configure_connection covering: default no-load, explicit load, load_from_plugins, search_path always set, adapter registration, and graph_name check delegation. Made-with: Cursor
|
@uesleilima thanks a lot for your patch. I let Claude code review run over the patch. Can you please address the issues ? Code ReviewSummaryAdds two useful features to the Python driver: a 🔴 Critical Issues
🟡 Suggestions
✅ What Looks Good
Verdict: Request Changes — The silent |
- Move TypeInfo.fetch() inside cursor block so search_path change is visible regardless of transaction isolation mode - Raise ValueError when load_from_plugins=True but load=False - Add type annotations to configure_connection signature - Document shallow-copy semantics in Vertex/Edge to_dict() - Path.to_dict() uses str() fallback for non-AGObj entities to guarantee JSON-serializable output - Add test for AgeNotSet when TypeInfo.fetch returns None - Add test for load_from_plugins=True without load=True - Replace fragile string assertions with assert_called_with/assert_any_call Made-with: Cursor
There was a problem hiding this comment.
Pull request overview
Adds new public APIs to the Python driver to better support external connection management (e.g., pooled connections) and to make model objects easier to serialize/consume in downstream apps.
Changes:
- Introduces
configure_connection()as a public API to configure an existing psycopg connection for AGE (search_path, agtype loaders, optional LOAD, optional graph check). - Adds
to_dict()helpers toVertex,Edge, andPathfor plain-Python dict/list conversion. - Exports
AgeLoader,ClientCursor, andconfigure_connectionfromage/__init__.pyand adds accompanying tests.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| drivers/python/age/age.py | Adds the new configure_connection() API for externally-managed connections. |
| drivers/python/age/models.py | Adds to_dict() methods for Path, Vertex, and Edge. |
| drivers/python/age/init.py | Exposes new public symbols at the package top-level. |
| drivers/python/test_age_py.py | Adds unit tests for to_dict() and configure_connection() and importability checks. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Path.to_dict(): leave dict/list/str/int/float/bool/None unchanged instead of converting to str(); handle entities=None safely - Add TestModelToDict, TestPublicImports, TestConfigureConnection to the __main__ suite so they run via direct script execution Made-with: Cursor
|
All review feedback has been addressed: tcolo's review:
Copilot review:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def configure_connection( | ||
| conn: psycopg.connection, | ||
| graph_name: str | None = None, | ||
| load: bool = False, | ||
| load_from_plugins: bool = False, |
There was a problem hiding this comment.
configure_connection() uses PEP 604 union syntax (str | None) in its public signature. This is a syntax error on Python 3.9, but the package metadata declares requires-python = ">=3.9" (drivers/python/pyproject.toml:25). Either adjust the type hints to be 3.9-compatible (e.g., Optional[str] / Union[...]) or bump the declared minimum Python version to 3.10+ so users on 3.9 don’t break at import time.
There was a problem hiding this comment.
Fixed in 1d92c31: graph_name is now Optional[str] with from typing import Optional so the module parses on Python 3.9 while keeping requires-python >= 3.9. Also aligned SET search_path with "$user" and updated tests.
|
@uesleilima Please see Copilot's last one. |
…_path - Use Optional[str] for configure_connection graph_name (PEP 604 unions are invalid on Python 3.9). - Import Any/Optional from typing for annotations. - Quote $user in SET search_path; align unit test expectations. Made-with: Cursor
Summary
Two enhancements to the Python driver's public API:
1. Public API for connection pooling / external connection management (#2369)
The
Ageclass creates and owns a single connection internally. There's no supported way to register agtype adapters on externally-managed connections (e.g., frompsycopg_pool.ConnectionPool).Solution: Add
configure_connection(conn, graph_name=None, skip_load=False)that:search_pathtoag_catalogAgeLoaderLOAD 'age'by default (suitable for managed PostgreSQL services)Also explicitly exports
AgeLoader,ClientCursor, andconfigure_connectionas public symbols inage/__init__.py, eliminating the need for# type: ignorewhen accessing driver internals.Usage with connection pool:
2. Vertex/Edge/Path model classes dict/JSON conversion (#2371)
The driver's model classes lack conversion to plain Python data structures, forcing every consumer to write custom normalization.
Solution: Add
to_dict()methods:Vertex.to_dict()→{"id", "label", "properties"}Edge.to_dict()→{"id", "label", "start_id", "end_id", "properties"}Path.to_dict()→ list ofto_dict()resultsAll return plain dicts/lists that work directly with
json.dumps().Tests
Added 8 new unit tests (no DB required):
TestModelToDict(5 tests): vertex, edge, pathto_dict(), empty properties, JSON serializabilityTestPublicImports(3 tests): verifyconfigure_connection,AgeLoader,ClientCursorare importableCloses