feat(graphs): pick a database when creating a graph#171
Conversation
Replaces the confusing "schema vs database" storage-mode toggle with a single Database picker. Schema is now the only isolation strategy: every non-default graph gets its own schema in some database. The user's only choice is *which* database. To get DB-level isolation, just don't put another graph in the same connection. Fixes the bug where selecting any database in the new graph UI silently created the schema in the system DB instead of the chosen one. Addresses PR #171 review feedback (#1, #2, #4, #5, #6, nits) — supersedes that PR with a much smaller diff that builds on the upstream multi-graph foundation that landed since. Backend - GET /api/v1/graphs/database-connections now prepends a synthetic ``default`` entry (id=null, config_key="default") so the system DB is selectable from the dropdown. - DatabaseConnectionResponse: id and created_at are nullable to carry the synthetic entry. - CreateGraphRequest: drop ``storage_mode`` field. Server hardcodes storage_mode="schema" (the column stays for backward compat with old rows but is no longer load-bearing at the API). - create_graph handler: treat ``database_connection_config_key`` of None or "default" as the system DB; any other key resolves to a row. - _provision_graph: previously hardcoded the system graph-db / write-db / Qdrant. Now resolves the target URLs from ``settings.graph_databases[config_key]`` based on ``graph.database_connection_id``, creates schemas via one-off engines pointed at the correct DBs, runs alembic with DATABASE_URL / WRITE_DATABASE_URL env overrides (Pydantic Settings re-reads them in the subprocess), and provisions Qdrant collections against the per-graph URL when it differs. - graph_sessions.py: collapse the "schema vs database" branch in _build_and_cache; non-default graphs now route by ``database_connection_id IS NULL`` only. - New ``make_qdrant_client(url, timeout)`` factory in kt-qdrant for non-singleton clients pointed at arbitrary URLs. - GraphRepository.create_database_connection now rejects the reserved config_key "default" — without this, an admin could insert a row that silently shadows the synthetic entry. - GraphDatabaseConfig: add a Pydantic field validator that normalizes ``postgresql://...`` to ``postgresql+asyncpg://...`` so plain URLs from EXTRA_DB_* env vars or YAML don't blow up create_async_engine. Frontend - Drop the Storage Mode <select> entirely. Database <select> is always visible, populated from listDatabaseConnections() (which now includes the synthetic "default" first), default value "default". - On submit, omit ``database_connection_config_key`` when "default" so the backend treats it as the system DB. - Drop the legacy "Separate DB / Shared DB" badge fallback in the graph card and detail page; render ``g.database_connection_name ?? "default"``. - Filter dropdown: rename the "schema mode" option to "default". Tests - libs/kt-config/tests/test_graph_databases.py — covers the new asyncpg URL validator. - services/api/tests/test_graph_schemas.py — drop the storage_mode- specific cases; add test_default_connection_key_accepted. - services/api/tests/integration/test_database_connections_endpoint.py — TestClient-based coverage of the synthetic-default ordering, real-row ordering, admin-only auth (403 for non-superuser), and the reserved "default" rejection in the repository. Out of scope (follow-ups) - Dropping the ``storage_mode`` column entirely (would need a migration). - Per-graph runtime use of qdrant_url in worker code (the resolver in graph_sessions.py already plumbs it through; no consumer reads it yet). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
9b2e7a9 to
879573c
Compare
|
Pushed a fresh take addressing all review feedback. Force-pushed because the previous branch was diverged from main and the rebase had multiple conflicts — easier to start clean from the latest Changes vs the original PR
Review feedback addressed
Nits
Test plan
|
- Fix Backend Lint failure: hoist DatabaseConnection/delete imports out of
the cleanup_connections fixture body so ruff I001 (import-block sort)
passes (services/api/tests/integration/test_database_connections_endpoint.py).
- Provisioning engines: tag them with application_name="kt-provision-{slug}"
via connect_args.server_settings, and use poolclass=NullPool since these
are single-shot connections. Connections are now visible in
pg_stat_activity and don't sit in a pool waiting for nothing.
- Frontend create form: disable the submit button when dbError is set so a
user whose database-connections fetch failed can't silently fall through
to the system DB by accident.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Round 2 fixes pushed (e07b6ca): Lint failureFixed — moved Review feedback addressed
Test status
|
Picks up the actionable items from the round-3 review. #1 Orphan-schema risk — defended in docstring + recovery path documented The Graph row is committed in "provisioning" status BEFORE _provision_graph runs (graphs.py:313). All provisioning steps are idempotent (CREATE SCHEMA IF NOT EXISTS, alembic upgrade head, ensure_collection), so a partial failure marks the row "error" and /retry-provision is the recovery path. Added a "Failure recovery" section to the _provision_graph docstring explaining why we deliberately don't drop schemas on failure. #3 Fragile qdrant_url comparison — strip trailing slashes when comparing so http://h:6333 and http://h:6333/ are treated as the same target and we don't needlessly spawn+close a fresh Qdrant client. #4 "default" magic string — extracted DEFAULT_DB_CONFIG_KEY constant in kt_config.settings and used everywhere (API handler, repo guard, list endpoint, startup check). Added a startup check in kt_api.main._assert_default_db_key_unreserved() that logs an error if a real database_connections row holds the reserved key — catches anything that may have slipped in via raw SQL or a previous version that lacked the repo guard. #6 Test coverage gap — added two new test files: - services/api/tests/test_provision_graph_routing.py: 3 tests that mock create_async_engine, subprocess.run, and the Qdrant client factories to assert _provision_graph routes to the EXTERNAL DB URLs when database_connection_id is set, routes to the SYSTEM DB URLs when it's null, and raises a clear error when the config_key is missing from settings.graph_databases. - test_database_connections_endpoint.py: new test_create_graph_with_default_key_uses_system_db that POSTs a graph with database_connection_config_key="default" and asserts the resulting row has database_connection_id=None. Added a stub_users_in_db fixture that inserts the test users into the User table to satisfy the FK constraint on graphs.created_by. #7 Frontend grep — confirmed nothing references storage_mode on CreateGraphRequest in frontend/. The only remaining reference is on GraphResponse, which is the read-side type (kept for backward compat). Skipped per reviewer note: #2 (alembic env override — reviewer self-resolved), #5 (cosmetic _admin rename). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Round 3 fixes pushed (7b21f76): Items addressed#1 — Orphan-schema risk (defended + documented) #3 — Fragile qdrant_url comparison ✅ #4 — "default" magic string ✅
#6 — Test coverage gap ✅
#7 — Frontend Skipped per reviewer note
Test results
|
CI runs ``ruff format --check`` and the new test file had a long assertion line that needed reformatting. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
I have read the CLA Document and I hereby sign the CLA You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
When the create-graph handler runs alembic against an external DB whose
CNPG-generated password contains characters URL-encoded as ``%XX`` (e.g.
``+`` → ``%2B``), ``alembic/env.py`` crashes:
ValueError: invalid interpolation syntax in
'postgresql+asyncpg://kt:%2BzYZK...@host:5432/db' at position 24
Python's ``configparser`` interprets ``%(...)s`` as interpolation tokens.
``set_main_option`` validates the value at write time and rejects any
``%`` that isn't part of a valid interpolation reference. The fix is to
escape ``%`` to ``%%`` before calling ``set_main_option`` — the
unescape happens automatically at ``.get()`` time, so SQLAlchemy still
receives the original URL.
Same fix applied to ``alembic_write/env.py`` for write-db.
Reproducer + fix verified by ``test_alembic_env_url_escape.py``.
Discovered while running PR #171's _provision_graph against the prod
shared graph-db: the password starts with ``+`` and broke alembic.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
Drops the confusing "schema vs database" storage-mode toggle in the graph creation UI and replaces it with a single Database picker. The first option is always
default(the system DB); other options come from a newGET /api/v1/graphs/database-connectionsendpoint that lists rows from thedatabase_connectionstable.Conceptually: every non-default graph is a schema in some database. The only choice the user makes is which database. Multiple graphs in the same DB are schema-isolated by convention.
Closes the gap reported on prod where the new graph UI had no database options to pick from.
What changes for users
Before — confusing dropdown:
…and even when "Database" was selected, there was no way to pick which DB.
After — clear single dropdown:
Backend
GET /api/v1/graphs/database-connections(admin-only). Syntheticdefaultentry is always first; rows fromdatabase_connectionsfollow in name order.storage_modefromCreateGraphRequest. Server derives it fromdatabase_connection_config_key(Noneor"default"→ schema mode in system DB; otherwise → database mode in the referenced external DB)._provision_graphto honordatabase_connection_id: resolves the graph/write/qdrant URLs fromsettings.graph_databases[config_key], creates schemas via one-off engines pointed at the correct DBs, runs alembic withDATABASE_URL/WRITE_DATABASE_URLenv overrides (Pydantic Settings re-reads them in the subprocess), and provisions Qdrant collections against the per-graph URL when it differs.make_qdrant_client()factory inkt-qdrantfor non-singleton clients pointed at arbitrary URLs.Frontend
DatabaseConnectionResponsetype andlistDatabaseConnections()API helper.frontend/src/app/graphs/page.tsx: Storage Mode<select>replaced with Database<select>populated from the new endpoint; default selection is"default".frontend/src/app/graphs/[slug]/page.tsx: "Storage" field renamed to "Database", showing the connection name (looked up bydatabase_connection_id), falling back todefault/external.Tests
services/api/tests/test_graph_schemas.py— drop thestorage_mode-specific cases; add atest_default_connection_key_acceptedfor the magic string.services/api/tests/test_database_connections_endpoint.py— covers synthetic-default ordering, name-ordered rows, and string-UUIDidfield. All 3 tests pass locally against real Postgres.kt-config(26),kt-qdrant(64),services/apigraph schemas (15), and frontend (123) tests all green.Out of scope (follow-ups)
storage_modecolumn entirely (would need an alembic migration; column kept for now)qdrant_url(the resolver ingraph_sessions.pyalready plumbs it through; no consumer reads it yet)Test plan
GET /api/v1/graphs/database-connectionsas admin → expect[{default}, {shared}]defaultandSharedprov_testagainstSharedknowledge-tree-shared-graph-db(NOT in the system graph-db)knowledge-tree-shared-qdrantDB: Sharedinstead ofSeparate DB / Shared DBRisks
_provision_graphexternal-DB code path is exercised in prod for the first time. If alembic migrations fail against a fresh external DB it likely meanssearch_pathisn't being honored — fixable, may need a follow-up.CREATE SCHEMA. If it fails, plan B is to point provisioning at the underlying CNPG-rwservice directly.🤖 Generated with Claude Code