feat: unify database_url config for Postgres and SQLite #540
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a27f5e31ac
ℹ️ 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".
src/basic_memory/db.py
Outdated
| "application_name": "basic-memory", | ||
| # Statement timeout for queries (30s to allow for cold start) | ||
| "statement_timeout": "30s", | ||
| # Schema isolation via search_path (extracted from URL or default "public") | ||
| "search_path": search_path, |
There was a problem hiding this comment.
When the URL has no search_path query parameter, extract_search_path_from_url() defaults to "public" and _create_postgres_engine() always sets server_settings.search_path accordingly. That overrides any database- or role-level default search_path (e.g., a tenant schema configured on the role), so existing deployments that relied on non-public defaults will suddenly run migrations/queries in public instead of their intended schema. This is a regression introduced by the new defaulting behavior; consider only setting server_settings.search_path when a search_path was explicitly provided.
Useful? React with 👍 / 👎.
|
Thanks for the review feedback. Fixed in fed645c - now This preserves database/role-level default schemas for existing deployments. |
Extend BASIC_MEMORY_DATABASE_URL to support: - SQLite URLs with custom paths for project-local databases - Postgres URLs with ?search_path= for schema isolation Changes: - config.py: Parse SQLite URLs in app_database_path property - db.py: Extract search_path from Postgres URL, pass to server_settings - db.py: Add ensure_schema_exists() for auto-creating schemas - alembic/env.py: Pass version_table_schema for migration tracking - cli/commands/db.py: Handle Postgres case in reset command Closes basicmachines-co#539 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> Signed-off-by: Cedric Hurst <cedric@spantree.net>
- Use SQLAlchemy DDL (CreateSchema/DropSchema) instead of raw SQL to eliminate SQL injection risk in schema operations - DRY up alembic/env.py by reusing extract_search_path_from_url - Fix type errors by propagating Optional[Path] through call chain - Implement reset_postgres_database() for actual Postgres reset - Handle None db_path in project_service.get_system_status() Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> Signed-off-by: Cedric Hurst <cedric@spantree.net>
Only set search_path in server_settings when explicitly provided in the database URL. This preserves database/role-level default schemas for deployments that rely on them. Addresses review feedback from Codex. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> Signed-off-by: Cedric Hurst <cedric@spantree.net>
divideby0
force-pushed
the
feat/539-database-url-flexibility
branch
from
fed645c to
9c7ff14
Compare
1 participant
Summary
Extends
BASIC_MEMORY_DATABASE_URLto accept:sqlite+aiosqlite:///path/to/db.sqlite)search_pathfor schema isolation (e.g.,postgresql+asyncpg://...?options=-c%20search_path%3Dmyschema)Closes #539
Changes
search_pathfrom Postgres URLs (asyncpg rejects this param directly)Nonefromapp_database_pathfor Postgres (no local file)reset_postgres_database()for properbm resetsupportTest Plan
Security
CreateSchema/DropSchemaDDL instead oftext()🤖 Generated with Claude Code