-
Notifications
You must be signed in to change notification settings - Fork 282
feat(core): accept SQLite URLs in database_url (#539) #1551
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,7 @@ | |
| from loguru import logger | ||
| from pydantic import AliasChoices, BaseModel, Field, model_validator | ||
| from pydantic_settings import BaseSettings, SettingsConfigDict | ||
| from sqlalchemy.engine import make_url | ||
|
|
||
| from basic_memory.config_migrations import ( | ||
| migrate_legacy_projects, | ||
|
|
@@ -942,6 +943,50 @@ def model_post_init(self, __context: Any) -> None: | |
| elif self.default_project is not None and self.default_project not in self.projects: | ||
| self.default_project = next(iter(self.projects.keys())) | ||
|
|
||
| def _resolve_sqlite_database_url_path(self) -> Optional[Path]: | ||
| """Derive the SQLite database file path from an explicit ``database_url``. | ||
|
|
||
| Returns ``None`` when ``database_url`` is unset or the backend is not | ||
| SQLite, so ``app_database_path`` falls back to the existing | ||
| ``data_dir_path / APP_DATABASE_NAME`` default (Postgres deployments | ||
| never use this; their ``database_url`` is consumed directly in db.py). | ||
|
|
||
| When a SQLite ``database_url`` is set, this is the single place that | ||
| parses it, following SQLAlchemy's own convention: three slashes is a | ||
| relative path (resolved against the current working directory), four | ||
| is absolute (issue #539). This lets each project/worktree point at | ||
| its own index file — e.g. | ||
| ``BASIC_MEMORY_DATABASE_URL=sqlite+aiosqlite:///.basic-memory/memory.db`` — | ||
| instead of always sharing the global data-dir database. | ||
| """ | ||
| if self.database_backend != DatabaseBackend.SQLITE or not self.database_url: | ||
| return None | ||
|
|
||
| try: | ||
| url = make_url(self.database_url) | ||
| except Exception as error: | ||
| raise ValueError( | ||
| f"Invalid database_url for sqlite backend: {self.database_url!r} ({error})" | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When parsing fails—for example, for a malformed URL such as Useful? React with 👍 / 👎. |
||
| ) from error | ||
|
|
||
| if not url.drivername.startswith("sqlite"): | ||
| raise ValueError( | ||
| "database_url must use a sqlite driver (e.g. " | ||
| "'sqlite+aiosqlite:///path/to.db') when database_backend='sqlite'; " | ||
| f"got {url.drivername!r}. Set database_backend='postgres' for a " | ||
| "Postgres database_url, or unset database_url to use the default " | ||
| "SQLite path." | ||
| ) | ||
|
|
||
| if not url.database: | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When AGENTS.md reference: AGENTS.md:L132-L133 Useful? React with 👍 / 👎. |
||
| raise ValueError( | ||
| "database_url for the sqlite backend must include a file path, e.g. " | ||
| "'sqlite+aiosqlite:///.basic-memory/memory.db' " | ||
| "(in-memory SQLite is not supported for the app database)." | ||
| ) | ||
|
|
||
| return Path(url.database) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
For SQLite URLs with query options—especially file URIs such as AGENTS.md reference: AGENTS.md:L132-L133 Useful? React with 👍 / 👎. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When the configured URL places the database at a visible path inside a watched project—for example, Useful? React with 👍 / 👎. |
||
|
|
||
| @property | ||
| def app_database_path(self) -> Path: | ||
| """Get the path to the app-level database. | ||
|
|
@@ -950,9 +995,13 @@ def app_database_path(self) -> Path: | |
| across all projects. | ||
|
|
||
| Uses BASIC_MEMORY_CONFIG_DIR when set so each process/worktree can | ||
| isolate both config and database state. | ||
| isolate both config and database state. When ``database_url`` is set | ||
| to a SQLite URL, that path is used instead — see | ||
| ``_resolve_sqlite_database_url_path``. | ||
| """ | ||
| database_path = self.data_dir_path / APP_DATABASE_NAME | ||
| database_path = self._resolve_sqlite_database_url_path() or ( | ||
| self.data_dir_path / APP_DATABASE_NAME | ||
| ) | ||
|
Comment on lines
+1002
to
+1004
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a host constructs AGENTS.md reference: AGENTS.md:L265-L269 Useful? React with 👍 / 👎. |
||
| if not database_path.exists(): # pragma: no cover | ||
| database_path.parent.mkdir(parents=True, exist_ok=True) | ||
| database_path.touch() | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,104 @@ | ||
| """End-to-end coverage for issue #539's SQLite half: BASIC_MEMORY_DATABASE_URL / | ||
| config ``database_url`` accepting a ``sqlite+aiosqlite://`` URL and the engine | ||
| actually reading/writing the file it points at (not the default | ||
| ~/.basic-memory/memory.db). | ||
|
|
||
| Config-level URL parsing (relative/absolute, error cases) is covered in | ||
| tests/test_config.py::TestBasicMemoryConfig - this module proves the engine | ||
| layer honors config.app_database_path once it is derived from database_url, | ||
| including the WAL/pragma setup in db.py. | ||
| """ | ||
|
|
||
| from pathlib import Path | ||
|
|
||
| import pytest | ||
| from sqlalchemy import text | ||
|
|
||
| from basic_memory.config import BasicMemoryConfig, DatabaseBackend | ||
| from basic_memory.db import DatabaseType, engine_session_factory | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_engine_creates_sqlite_file_at_custom_relative_database_url(tmp_path, monkeypatch): | ||
| """The engine writes to the custom path derived from a relative database_url.""" | ||
| monkeypatch.chdir(tmp_path) | ||
| monkeypatch.delenv("BASIC_MEMORY_CONFIG_DIR", raising=False) | ||
|
|
||
| config = BasicMemoryConfig( | ||
| env="test", | ||
| database_backend=DatabaseBackend.SQLITE, | ||
| database_url="sqlite+aiosqlite:///.basic-memory/memory.db", | ||
| skip_initialization_sync=True, | ||
| ) | ||
|
|
||
| db_path = config.app_database_path | ||
| assert db_path == Path(".basic-memory/memory.db") | ||
|
|
||
| async with engine_session_factory(db_path, DatabaseType.FILESYSTEM, config) as ( | ||
| engine, | ||
| session_maker, | ||
| ): | ||
| async with session_maker() as session: | ||
| # Force a real connection/write so SQLite actually creates the file | ||
| # and applies the WAL pragma from _configure_sqlite_connection. | ||
| await session.execute(text("CREATE TABLE probe (id INTEGER PRIMARY KEY)")) | ||
| await session.execute(text("INSERT INTO probe (id) VALUES (1)")) | ||
| await session.commit() | ||
|
|
||
| result = await session.execute(text("PRAGMA journal_mode")) | ||
| assert result.scalar() == "wal" | ||
|
|
||
| resolved_db = tmp_path / ".basic-memory" / "memory.db" | ||
| assert resolved_db.exists() | ||
| assert resolved_db.stat().st_size > 0 | ||
|
|
||
| # The default per-user location must never have been created. | ||
| assert not (tmp_path / ".basic-memory-home" / "memory.db").exists() | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_engine_creates_sqlite_file_at_custom_absolute_database_url(tmp_path, monkeypatch): | ||
| """The engine writes to the custom path derived from an absolute database_url.""" | ||
| monkeypatch.delenv("BASIC_MEMORY_CONFIG_DIR", raising=False) | ||
| custom_db = tmp_path / "instance-a" / "index.db" | ||
|
|
||
| config = BasicMemoryConfig( | ||
| env="test", | ||
| database_backend=DatabaseBackend.SQLITE, | ||
| database_url=f"sqlite+aiosqlite:///{custom_db}", | ||
| skip_initialization_sync=True, | ||
| ) | ||
|
|
||
| db_path = config.app_database_path | ||
| assert db_path == custom_db | ||
|
|
||
| async with engine_session_factory(db_path, DatabaseType.FILESYSTEM, config) as ( | ||
| engine, | ||
| session_maker, | ||
| ): | ||
| async with session_maker() as session: | ||
| await session.execute(text("CREATE TABLE probe (id INTEGER PRIMARY KEY)")) | ||
| await session.commit() | ||
|
|
||
| assert custom_db.exists() | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_engine_uses_default_path_when_database_url_unset(tmp_path, monkeypatch): | ||
| """Unset database_url keeps the pre-existing default SQLite path unchanged.""" | ||
| monkeypatch.setenv("BASIC_MEMORY_CONFIG_DIR", str(tmp_path / "state")) | ||
|
|
||
| config = BasicMemoryConfig(env="test", skip_initialization_sync=True) | ||
|
|
||
| db_path = config.app_database_path | ||
| assert db_path == tmp_path / "state" / "memory.db" | ||
|
|
||
| async with engine_session_factory(db_path, DatabaseType.FILESYSTEM, config) as ( | ||
| engine, | ||
| session_maker, | ||
| ): | ||
| async with session_maker() as session: | ||
| await session.execute(text("CREATE TABLE probe (id INTEGER PRIMARY KEY)")) | ||
| await session.commit() | ||
|
|
||
| assert (tmp_path / "state" / "memory.db").exists() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When
database_urlis set throughbasic-memory config setor merely loaded from configuration, this helper is not called: the command only runsBasicMemoryConfig.model_validate()before persisting the candidate. Values this code intends to reject, such as a non-SQLite scheme or a SQLite URL with no path, are therefore reported as successfully saved and fail only later when a database path happens to be requested. Move this validation into a model validator so invalid configuration is rejected before it is persisted.AGENTS.md reference: AGENTS.md:L128-L133
Useful? React with 👍 / 👎.