tcolo commented on PR #2374: URL: https://github.com/apache/age/pull/2374#issuecomment-4189372226
@uesleilima thanks a lot for your patch. I let Claude code review run over the patch. Can you please address the issues ? ## Code Review ### Summary Adds two useful features to the Python driver: a `configure_connection()` function for externally-managed connection pools, and `to_dict()` serialization methods on graph model classes. The overall approach is sound and the test coverage is solid. A few correctness and consistency issues are worth addressing before merging. --- ### 🔴 Critical Issues | # | File | Issue | |---|------|-------| | 1 | `age/age.py` | **`TypeInfo.fetch()` is called outside the `with conn.cursor()` block.** In `setUpAge` it lives inside the block. If the connection is in `autocommit=False` mode the `SET search_path` change may not be visible to `TypeInfo.fetch` depending on transaction isolation. Move `TypeInfo.fetch` inside the cursor block (or at least after `conn.commit()`). | | 2 | `age/age.py` | **`load_from_plugins=True` without `load=True` is silently ignored.** A caller who passes `load_from_plugins=True` expecting plugins-path loading will get no LOAD at all and no warning. Raise a `ValueError` or at least emit a warning. | --- ### 🟡 Suggestions | # | File | Suggestion | Category | |---|------|------------|----------| | 1 | `age/age.py` | Add type annotations consistent with the rest of the file (`conn: psycopg.connection`, `graph_name: Optional[str] = None`, etc.). | Maintainability | | 2 | `age/age.py` | Consider adding `__all__` to `age.py` to cleanly control the public surface instead of double-importing in `__init__.py`. | Maintainability | | 3 | `age/models.py` | `Path.to_dict()` falls back to `else e` for non-`AGObj` entities — this may return non-JSON-serializable objects. Add a comment or use `str(e)` as a safe fallback. | Correctness | | 4 | `age/models.py` | `dict(self.properties)` is a shallow copy. Nested mutable values will still be shared references. Document this or use `copy.deepcopy` if full JSON safety is the goal. | Correctness | | 5 | `test_age_py.py` | Missing test: `AgeNotSet` is raised when `TypeInfo.fetch` returns `None`. This is a key code path in `configure_connection` with no coverage. | Testing | | 6 | `test_age_py.py` | No test asserting that `load_from_plugins=True, load=False` does **not** execute any LOAD. Ties directly to the silent-ignore issue above. | Testing | | 7 | `test_age_py.py` | Asserting `"LOAD" not in str(call_args)` is fragile — depends on Python's internal repr. Prefer explicit `assert_called_with(...)` checks. | Testing | --- ### ✅ What Looks Good - The `configure_connection` docstring is clear and explicitly explains the managed-PostgreSQL use case (Azure, AWS RDS) — genuinely useful. - Default `load=False` is the right choice for a connection-pool scenario and the rationale is well communicated. - `to_dict()` implementations are clean, simple, and consistent across `Vertex`, `Edge`, and `Path`. - `TestConfigureConnection` correctly uses `unittest.mock` to test without a live database. - `test_registers_agtype_adapters` asserts adapter registration by OID — precise and correct. - `TestPublicImports` is a lightweight but effective guard against accidental API regressions. --- **Verdict: Request Changes** — The silent `load_from_plugins` ignore and the `TypeInfo.fetch` cursor-scope issue are real correctness risks worth fixing. The feature direction is right and most of the code is clean. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
