From d124ef5ddc20492383d260f9c1f109521c81fedb Mon Sep 17 00:00:00 2001 From: Maxteabag <18162810+Maxteabag@users.noreply.github.com> Date: Sun, 24 May 2026 11:54:47 +0200 Subject: [PATCH 1/9] Store query history as per-query .sql files instead of one JSON History moves from a single `query_history.json` to per-query `.sql` files under `/queries//_.sql`, each with a small `-- sqlit:history` header (connection, database, timestamp). Files sort chronologically by name and grep cleanly. Re-running an existing query (same content after `strip()`) deletes the old file and writes a fresh one with the new timestamp, so dedup remains content-based and the most-recent timestamp wins. The 100- entry-per-connection cap is enforced by lex-order eviction. Public API (`HistoryStore` methods) is unchanged so existing callers in `query_execution.py` and `services.py` keep working. A one-time migration on first construct fans out any legacy `query_history.json` into the new tree and renames it to `.migrated`. --- sqlit/domains/query/store/history.py | 413 +++++++++++++------ tests/unit/test_history_store_file_backed.py | 230 +++++++++++ 2 files changed, 522 insertions(+), 121 deletions(-) create mode 100644 tests/unit/test_history_store_file_backed.py diff --git a/sqlit/domains/query/store/history.py b/sqlit/domains/query/store/history.py index f3622934..af22d779 100644 --- a/sqlit/domains/query/store/history.py +++ b/sqlit/domains/query/store/history.py @@ -1,11 +1,30 @@ -"""History store for managing query history per connection.""" +"""File-backed query history store. + +Each query is a `.sql` file under `CONFIG_DIR/queries//` +with a small comment header carrying connection name, database, and +timestamp. Files are sortable lexicographically by their timestamp prefix. + +Re-running the same query (exact text after `strip()`) updates the +existing entry by deleting the old file and writing a new one with the +current timestamp, so most-recent always sorts last (and reverse-sorts +first for the UI). +""" from __future__ import annotations +import hashlib +import os +import re +import tempfile from dataclasses import dataclass from datetime import datetime +from pathlib import Path -from sqlit.shared.core.store import CONFIG_DIR, JSONFileStore +from sqlit.shared.core.store import CONFIG_DIR + +_HEADER_MARKER = "-- sqlit:history" +_HEADER_LINE = re.compile(r"^--\s*([A-Za-z_][A-Za-z0-9_-]*)\s*:\s*(.*?)\s*$") +_SAFE_NAME = re.compile(r"[^A-Za-z0-9._-]") @dataclass @@ -15,12 +34,11 @@ class QueryHistoryEntry: query: str timestamp: str # ISO format connection_name: str - database: str = "" # Active database when query was executed + database: str = "" is_starred: bool = False # Computed at load time, not persisted is_starred_only: bool = False # True if only in starred store, not in history def to_dict(self) -> dict: - """Convert to dictionary for JSON serialization.""" d: dict = { "query": self.query, "timestamp": self.timestamp, @@ -32,7 +50,6 @@ def to_dict(self) -> dict: @classmethod def from_dict(cls, data: dict) -> QueryHistoryEntry: - """Create from dictionary.""" return cls( query=data["query"], timestamp=data["timestamp"], @@ -41,138 +58,292 @@ def from_dict(cls, data: dict) -> QueryHistoryEntry: ) -class HistoryStore(JSONFileStore): - """Store for managing query history. - - History is stored as query_history.json in the sqlit config directory. - Each entry includes query text, timestamp, and connection name. +def _query_hash(query: str) -> str: + return hashlib.sha256(query.strip().encode("utf-8")).hexdigest()[:8] + + +def _connection_dir_name(connection_name: str) -> str: + """Sanitize connection name + append short hash for collision-free uniqueness.""" + safe = _SAFE_NAME.sub("_", connection_name)[:40] or "_" + short = hashlib.sha256(connection_name.encode("utf-8")).hexdigest()[:8] + return f"{safe}_{short}" + + +def _timestamp_to_filename(iso_ts: str) -> str: + """Turn an ISO timestamp into a filesystem-safe sortable prefix.""" + return iso_ts.replace(":", "-") + + +def _filename_to_timestamp(stem_prefix: str) -> str: + """Inverse of _timestamp_to_filename. Used only as a fallback when a + stored file is missing a header.""" + if "T" not in stem_prefix: + return stem_prefix + date_part, _, time_part = stem_prefix.partition("T") + return f"{date_part}T{time_part.replace('-', ':', 2)}" + + +def _format_entry(entry: QueryHistoryEntry) -> str: + lines = [ + _HEADER_MARKER, + f"-- connection: {entry.connection_name}", + ] + if entry.database: + lines.append(f"-- database: {entry.database}") + lines.append(f"-- ran: {entry.timestamp}") + lines.append("") + lines.append(entry.query) + if not entry.query.endswith("\n"): + lines.append("") + return "\n".join(lines) + + +def _parse_entry( + text: str, *, fallback_connection: str, fallback_timestamp: str +) -> QueryHistoryEntry | None: + """Parse a stored .sql file. Header is the leading run of comment + lines, terminated by the first blank line. Anything below is the + query body.""" + lines = text.splitlines() + metadata: dict[str, str] = {} + body_start = 0 + for i, line in enumerate(lines): + stripped = line.rstrip() + if stripped == _HEADER_MARKER: + body_start = i + 1 + continue + if stripped == "": + body_start = i + 1 + break + m = _HEADER_LINE.match(stripped) + if m: + key, value = m.group(1).lower(), m.group(2) + metadata[key] = value + body_start = i + 1 + continue + body_start = i + break + + query = "\n".join(lines[body_start:]).strip("\n") + if not query: + return None + + return QueryHistoryEntry( + query=query, + timestamp=metadata.get("ran") or fallback_timestamp, + connection_name=metadata.get("connection") or fallback_connection, + database=metadata.get("database", ""), + ) + + +class HistoryStore: + """File-backed query history. + + Layout:: + + CONFIG_DIR/queries//_.sql + + Each file holds the query prefixed by an SQL-comment header + (``-- sqlit:history``, ``-- connection:``, ``-- database:``, + ``-- ran:``) terminated by a blank line. """ MAX_ENTRIES_PER_CONNECTION = 100 - def __init__(self) -> None: - super().__init__(CONFIG_DIR / "query_history.json") + def __init__(self, base_dir: Path | None = None) -> None: + self._base_dir = base_dir if base_dir is not None else CONFIG_DIR / "queries" + self._migrated = False - def _load_all_entries(self) -> list[dict]: - """Load all history entries as raw dictionaries.""" - data = self._read_json() - return data if isinstance(data, list) else [] + @property + def base_dir(self) -> Path: + return self._base_dir - def load_for_connection(self, connection_name: str) -> list[QueryHistoryEntry]: - """Load query history for a specific connection. - - Args: - connection_name: Name of connection to load history for. - - Returns: - List of QueryHistoryEntry objects, sorted by most recent first. - """ - all_entries = self._load_all_entries() + def _ensure_dir(self, path: Path) -> None: + path.mkdir(parents=True, exist_ok=True) + try: + os.chmod(path, 0o700) + except OSError: + pass + + def _connection_dir(self, connection_name: str) -> Path: + return self._base_dir / _connection_dir_name(connection_name) + + def _maybe_migrate(self) -> None: + if self._migrated: + return + self._migrated = True + legacy = self._base_dir.parent / "query_history.json" + if not legacy.exists(): + return try: - entries = [ - QueryHistoryEntry.from_dict(entry) - for entry in all_entries - if entry.get("connection_name") == connection_name - ] - entries.sort(key=lambda e: e.timestamp, reverse=True) - return entries - except (KeyError, TypeError): + import json + with legacy.open("r", encoding="utf-8") as f: + data = json.load(f) + except (OSError, json.JSONDecodeError): + return + if not isinstance(data, list): + return + for raw in data: + if not isinstance(raw, dict): + continue + try: + entry = QueryHistoryEntry.from_dict(raw) + except (KeyError, TypeError): + continue + self._write_entry(entry) + try: + legacy.replace(legacy.with_suffix(".json.migrated")) + except OSError: + pass + + def _write_entry(self, entry: QueryHistoryEntry) -> Path: + """Write one entry to disk, replacing any prior file with the + same query hash for the same connection. Atomic per-file.""" + conn_dir = self._connection_dir(entry.connection_name) + self._ensure_dir(conn_dir) + + qhash = _query_hash(entry.query) + for existing in conn_dir.glob(f"*_{qhash}.sql"): + try: + existing.unlink() + except OSError: + pass + + filename = f"{_timestamp_to_filename(entry.timestamp)}_{qhash}.sql" + dest = conn_dir / filename + body = _format_entry(entry) + + fd, tmp_path = tempfile.mkstemp(dir=conn_dir, prefix=".tmp_", suffix=".sql") + try: + with os.fdopen(fd, "w", encoding="utf-8") as f: + f.write(body) + try: + os.chmod(tmp_path, 0o600) + except OSError: + pass + os.replace(tmp_path, dest) + except Exception: + try: + os.unlink(tmp_path) + except OSError: + pass + raise + return dest + + def _evict(self, connection_name: str) -> None: + conn_dir = self._connection_dir(connection_name) + if not conn_dir.is_dir(): + return + files = sorted(p for p in conn_dir.glob("*.sql") if p.is_file()) + excess = len(files) - self.MAX_ENTRIES_PER_CONNECTION + if excess <= 0: + return + for path in files[:excess]: + try: + path.unlink() + except OSError: + pass + + def _load_dir(self, conn_dir: Path) -> list[QueryHistoryEntry]: + if not conn_dir.is_dir(): return [] + dir_name = conn_dir.name + # Dir names end with `_<8-hex-chars>`. Strip that for fallback. + fallback_connection = ( + dir_name[:-9] + if len(dir_name) > 9 and dir_name[-9] == "_" + else dir_name + ) + entries: list[QueryHistoryEntry] = [] + for path in conn_dir.glob("*.sql"): + try: + text = path.read_text(encoding="utf-8") + except OSError: + continue + stem = path.stem + fallback_ts_raw = stem.rsplit("_", 1)[0] if "_" in stem else stem + fallback_ts = _filename_to_timestamp(fallback_ts_raw) + entry = _parse_entry( + text, + fallback_connection=fallback_connection, + fallback_timestamp=fallback_ts, + ) + if entry is not None: + entries.append(entry) + entries.sort(key=lambda e: e.timestamp, reverse=True) + return entries - def load_all(self) -> list[QueryHistoryEntry]: - """Load query history for all connections. + # ----- public API (matches HistoryStoreProtocol) ----- - Returns: - List of QueryHistoryEntry objects, sorted by most recent first. - """ - all_entries = self._load_all_entries() - try: - entries = [QueryHistoryEntry.from_dict(entry) for entry in all_entries] - entries.sort(key=lambda e: e.timestamp, reverse=True) - return entries - except (KeyError, TypeError): + def load_for_connection(self, connection_name: str) -> list[QueryHistoryEntry]: + self._maybe_migrate() + return self._load_dir(self._connection_dir(connection_name)) + + def load_all(self) -> list[QueryHistoryEntry]: + self._maybe_migrate() + if not self._base_dir.is_dir(): return [] + entries: list[QueryHistoryEntry] = [] + for child in self._base_dir.iterdir(): + if child.is_dir(): + entries.extend(self._load_dir(child)) + entries.sort(key=lambda e: e.timestamp, reverse=True) + return entries def save_query(self, connection_name: str, query: str, database: str = "") -> None: - """Save a query to history. - - If the exact query already exists for this connection, updates its timestamp. - Otherwise adds a new entry. Keeps only MAX_ENTRIES_PER_CONNECTION entries. - - Args: - connection_name: Name of the connection. - query: SQL query text. - database: Active database when the query was executed. - """ - all_entries = self._load_all_entries() + self._maybe_migrate() query_stripped = query.strip() - now = datetime.now().isoformat() - - # Check if query already exists - for entry in all_entries: - if entry.get("connection_name") == connection_name and entry.get("query", "").strip() == query_stripped: - entry["timestamp"] = now - if database: - entry["database"] = database - break - else: - # Add new entry - new_entry = QueryHistoryEntry( - query=query_stripped, - timestamp=now, - connection_name=connection_name, - database=database, - ) - all_entries.append(new_entry.to_dict()) - - # Limit entries per connection - connection_entries = [e for e in all_entries if e.get("connection_name") == connection_name] - other_entries = [e for e in all_entries if e.get("connection_name") != connection_name] - - connection_entries.sort(key=lambda e: e.get("timestamp", ""), reverse=True) - connection_entries = connection_entries[: self.MAX_ENTRIES_PER_CONNECTION] - - self._write_json(other_entries + connection_entries) + if not query_stripped: + return + entry = QueryHistoryEntry( + query=query_stripped, + timestamp=datetime.now().isoformat(), + connection_name=connection_name, + database=database, + ) + self._write_entry(entry) + self._evict(connection_name) def delete_entry(self, connection_name: str, timestamp: str) -> bool: - """Delete a specific history entry. - - Args: - connection_name: Name of the connection. - timestamp: ISO timestamp of the entry to delete. - - Returns: - True if an entry was deleted, False otherwise. - """ - all_entries = self._load_all_entries() - original_count = len(all_entries) - - all_entries = [ - e - for e in all_entries - if not (e.get("timestamp") == timestamp and e.get("connection_name") == connection_name) - ] - - if len(all_entries) < original_count: - self._write_json(all_entries) - return True + self._maybe_migrate() + conn_dir = self._connection_dir(connection_name) + if not conn_dir.is_dir(): + return False + filename_prefix = _timestamp_to_filename(timestamp) + for path in conn_dir.glob("*.sql"): + try: + text = path.read_text(encoding="utf-8") + except OSError: + continue + entry = _parse_entry( + text, + fallback_connection=connection_name, + fallback_timestamp=_filename_to_timestamp(path.stem.rsplit("_", 1)[0]), + ) + if entry is None: + continue + if entry.timestamp == timestamp or path.name.startswith(filename_prefix): + try: + path.unlink() + return True + except OSError: + return False return False def clear_for_connection(self, connection_name: str) -> int: - """Clear all history for a connection. - - Args: - connection_name: Name of the connection. - - Returns: - Number of entries deleted. - """ - all_entries = self._load_all_entries() - original_count = len(all_entries) - - all_entries = [e for e in all_entries if e.get("connection_name") != connection_name] - - deleted = original_count - len(all_entries) - if deleted > 0: - self._write_json(all_entries) - return deleted + self._maybe_migrate() + conn_dir = self._connection_dir(connection_name) + if not conn_dir.is_dir(): + return 0 + count = 0 + for path in conn_dir.glob("*.sql"): + try: + path.unlink() + count += 1 + except OSError: + pass + try: + conn_dir.rmdir() + except OSError: + pass + return count diff --git a/tests/unit/test_history_store_file_backed.py b/tests/unit/test_history_store_file_backed.py new file mode 100644 index 00000000..51aec507 --- /dev/null +++ b/tests/unit/test_history_store_file_backed.py @@ -0,0 +1,230 @@ +"""Tests for the file-backed HistoryStore.""" + +from __future__ import annotations + +import json +from pathlib import Path + +import pytest + +from sqlit.domains.query.store.history import ( + HistoryStore, + _connection_dir_name, +) + + +@pytest.fixture +def store(tmp_path: Path) -> HistoryStore: + return HistoryStore(base_dir=tmp_path / "queries") + + +def _read_file_text(store: HistoryStore, connection: str) -> list[str]: + conn_dir = store.base_dir / _connection_dir_name(connection) + return [p.read_text(encoding="utf-8") for p in sorted(conn_dir.glob("*.sql"))] + + +class TestRoundTrip: + def test_save_and_load_one_query(self, store: HistoryStore) -> None: + store.save_query("postgres-prod", "SELECT 1", database="myapp") + entries = store.load_for_connection("postgres-prod") + assert len(entries) == 1 + assert entries[0].query == "SELECT 1" + assert entries[0].connection_name == "postgres-prod" + assert entries[0].database == "myapp" + assert entries[0].timestamp # ISO string set by store + + def test_load_returns_most_recent_first(self, store: HistoryStore) -> None: + store.save_query("c", "SELECT 1") + store.save_query("c", "SELECT 2") + store.save_query("c", "SELECT 3") + entries = store.load_for_connection("c") + assert [e.query for e in entries] == ["SELECT 3", "SELECT 2", "SELECT 1"] + + def test_load_all_spans_connections(self, store: HistoryStore) -> None: + store.save_query("a", "SELECT 1") + store.save_query("b", "SELECT 2") + names = {e.connection_name for e in store.load_all()} + assert names == {"a", "b"} + + def test_blank_query_is_ignored(self, store: HistoryStore) -> None: + store.save_query("c", " \n \n") + assert store.load_for_connection("c") == [] + + def test_query_text_is_stripped(self, store: HistoryStore) -> None: + store.save_query("c", " SELECT 1 \n ") + entries = store.load_for_connection("c") + assert entries[0].query == "SELECT 1" + + +class TestDedup: + def test_same_query_updates_in_place(self, store: HistoryStore) -> None: + store.save_query("c", "SELECT 1") + first_ts = store.load_for_connection("c")[0].timestamp + # Force a different timestamp on the second save. + import time + time.sleep(0.01) + store.save_query("c", "SELECT 1") + entries = store.load_for_connection("c") + assert len(entries) == 1 + assert entries[0].timestamp != first_ts + + def test_same_query_only_one_file_on_disk(self, store: HistoryStore) -> None: + store.save_query("c", "SELECT 1") + store.save_query("c", "SELECT 1") + store.save_query("c", "SELECT 1") + assert len(_read_file_text(store, "c")) == 1 + + def test_whitespace_variants_dedup(self, store: HistoryStore) -> None: + store.save_query("c", "SELECT 1") + store.save_query("c", " SELECT 1 ") + assert len(store.load_for_connection("c")) == 1 + + +class TestHeaderFormat: + def test_header_contains_expected_fields(self, store: HistoryStore) -> None: + store.save_query("postgres-prod", "SELECT 1", database="myapp") + text = _read_file_text(store, "postgres-prod")[0] + assert "-- sqlit:history" in text + assert "-- connection: postgres-prod" in text + assert "-- database: myapp" in text + assert "-- ran:" in text + + def test_database_omitted_when_empty(self, store: HistoryStore) -> None: + store.save_query("c", "SELECT 1") + text = _read_file_text(store, "c")[0] + assert "-- database:" not in text + + def test_query_body_preserved_verbatim(self, store: HistoryStore) -> None: + body = "-- user's own comment\nSELECT 1\nFROM users" + store.save_query("c", body) + entries = store.load_for_connection("c") + assert entries[0].query == body + + +class TestConnectionDirNaming: + def test_slashes_are_sanitized(self) -> None: + name = _connection_dir_name("server/database") + assert "/" not in name + assert name.startswith("server_database_") + + def test_two_similar_names_get_distinct_dirs(self) -> None: + a = _connection_dir_name("a/b") + b = _connection_dir_name("a_b") + # Sanitization alone would collide; hash suffix saves us. + assert a != b + + def test_unicode_name_is_handled(self, store: HistoryStore) -> None: + store.save_query("databäs-prøduktiøn", "SELECT 1") + entries = store.load_for_connection("databäs-prøduktiøn") + assert len(entries) == 1 + assert entries[0].connection_name == "databäs-prøduktiøn" + + +class TestFilenameOrdering: + def test_filenames_sort_chronologically(self, store: HistoryStore) -> None: + import time + for i in range(5): + store.save_query("c", f"SELECT {i}") + time.sleep(0.005) + conn_dir = store.base_dir / _connection_dir_name("c") + names = sorted(p.name for p in conn_dir.glob("*.sql")) + # Last (lex-max) name must be the most recent (SELECT 4). + last_text = (conn_dir / names[-1]).read_text(encoding="utf-8") + assert "SELECT 4" in last_text + + +class TestDeleteAndClear: + def test_delete_entry_by_timestamp(self, store: HistoryStore) -> None: + store.save_query("c", "SELECT 1") + ts = store.load_for_connection("c")[0].timestamp + assert store.delete_entry("c", ts) is True + assert store.load_for_connection("c") == [] + + def test_delete_entry_missing_returns_false(self, store: HistoryStore) -> None: + assert store.delete_entry("c", "2026-01-01T00:00:00") is False + + def test_clear_for_connection(self, store: HistoryStore) -> None: + store.save_query("c", "SELECT 1") + store.save_query("c", "SELECT 2") + assert store.clear_for_connection("c") == 2 + assert store.load_for_connection("c") == [] + + +class TestEviction: + def test_max_entries_enforced( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + monkeypatch.setattr(HistoryStore, "MAX_ENTRIES_PER_CONNECTION", 3) + s = HistoryStore(base_dir=tmp_path / "queries") + import time + for i in range(5): + s.save_query("c", f"SELECT {i}") + time.sleep(0.005) + entries = s.load_for_connection("c") + # Oldest two (SELECT 0, SELECT 1) should have been evicted. + assert len(entries) == 3 + assert {e.query for e in entries} == {"SELECT 2", "SELECT 3", "SELECT 4"} + + +class TestLegacyMigration: + def test_migrates_json_on_first_use(self, tmp_path: Path) -> None: + config = tmp_path / "config" + config.mkdir() + legacy = config / "query_history.json" + legacy.write_text( + json.dumps( + [ + { + "query": "SELECT 1", + "timestamp": "2026-01-01T00:00:00", + "connection_name": "old", + }, + { + "query": "SELECT 2", + "timestamp": "2026-01-02T00:00:00", + "connection_name": "old", + "database": "myapp", + }, + ] + ) + ) + + store = HistoryStore(base_dir=config / "queries") + entries = store.load_for_connection("old") + assert [e.query for e in entries] == ["SELECT 2", "SELECT 1"] + assert entries[0].database == "myapp" + + # JSON has been renamed so we don't re-import on next launch. + assert not legacy.exists() + assert (config / "query_history.json.migrated").exists() + + def test_migration_handles_missing_file(self, tmp_path: Path) -> None: + store = HistoryStore(base_dir=tmp_path / "queries") + # Should not crash on first load. + assert store.load_all() == [] + + def test_migration_handles_malformed_file(self, tmp_path: Path) -> None: + config = tmp_path / "config" + config.mkdir() + (config / "query_history.json").write_text("not valid json") + store = HistoryStore(base_dir=config / "queries") + assert store.load_all() == [] + # Malformed file is left alone (not renamed). + assert (config / "query_history.json").exists() + + +class TestFallbackParsing: + def test_file_without_header_uses_filename_timestamp( + self, store: HistoryStore + ) -> None: + # User dropped a raw .sql file into the connection dir. + conn_dir = store.base_dir / _connection_dir_name("c") + conn_dir.mkdir(parents=True) + (conn_dir / "2026-05-23T14-30-15_deadbeef.sql").write_text( + "SELECT raw_user_file\n", encoding="utf-8" + ) + entries = store.load_for_connection("c") + assert len(entries) == 1 + assert entries[0].query == "SELECT raw_user_file" + assert entries[0].timestamp == "2026-05-23T14:30:15" + assert entries[0].connection_name == "c" From a3fd5f0935d7784ef2e7106a7c805bf84be76ac8 Mon Sep 17 00:00:00 2001 From: Maxteabag <18162810+Maxteabag@users.noreply.github.com> Date: Sun, 24 May 2026 11:54:57 +0200 Subject: [PATCH 2/9] Add project-scoped config via `sqlit ` Passing a directory as the first positional arg routes connections, query history, and starred queries into `/.sqlit/` instead of the global config. Theme, settings, and credentials (keyring) stay global. `sqlit .` resolves to the current shell directory; a missing directory errors out cleanly with exit code 1. Use case: a team can commit `/.sqlit/connections.json` and useful saved queries to share project-specific db setup, while each developer keeps their personal theme/keyring out of the repo. `ConnectionStore` and `StarredStore` gain a `file_path` constructor arg; `HistoryStore` already accepts `base_dir`. `RuntimeConfig` gains `project_dir` plus a `project_config_dir` helper. `build_app_services` threads these into the stores when `project_dir` is set. --- sqlit/cli.py | 82 +++++++++- .../domains/connections/store/connections.py | 11 +- sqlit/domains/query/store/starred.py | 9 +- sqlit/shared/app/runtime.py | 6 + sqlit/shared/app/services.py | 20 ++- tests/unit/test_project_dir_routing.py | 153 ++++++++++++++++++ 6 files changed, 270 insertions(+), 11 deletions(-) create mode 100644 tests/unit/test_project_dir_routing.py diff --git a/sqlit/cli.py b/sqlit/cli.py index fefa33be..d6933b69 100644 --- a/sqlit/cli.py +++ b/sqlit/cli.py @@ -37,6 +37,64 @@ def _get_schema_value_flags() -> set[str]: return flags +def _looks_like_project_path(arg: str) -> bool: + """Conservative test for a positional project-dir argument. + + Only matches things that are obviously paths: `.`, `..`, anything + starting with `./`, `../`, `/`, `~`, or anything ending in `/`. + """ + if arg in {".", ".."}: + return True + if arg.startswith(("./", "../", "/", "~")): + return True + if arg.endswith("/"): + return True + return False + + +def _extract_project_dir(argv: list[str]) -> tuple[Path | None, list[str]]: + """Extract a positional project-dir argument from argv if present. + + Returns (project_dir, remaining_argv). Exits with an error if the + user passed a path that doesn't resolve to an existing directory. + """ + subcommands = {"connections", "connection", "connect", "query", "docker"} + result_argv: list[str] = [] + project_dir: Path | None = None + + i = 0 + while i < len(argv): + arg = argv[i] + if i == 0: + result_argv.append(arg) + i += 1 + continue + # Flags pass straight through (let argparse handle them). + if arg.startswith("-"): + result_argv.append(arg) + i += 1 + continue + # First subcommand: copy the rest verbatim. + if arg in subcommands: + result_argv.extend(argv[i:]) + break + if project_dir is None and _looks_like_project_path(arg): + resolved = Path(arg).expanduser().resolve() + if not resolved.is_dir(): + print( + f"Error: project directory does not exist: {arg}", + file=sys.stderr, + ) + sys.exit(1) + project_dir = resolved + i += 1 + continue + result_argv.append(arg) + i += 1 + + return project_dir, result_argv + + def _extract_connection_url(argv: list[str]) -> tuple[str | None, list[str]]: """Extract a connection URL from argv if present. @@ -255,7 +313,12 @@ def _exec_restart(argv: list[str]) -> None: os.execvp(exe, argv) -def _build_runtime(args: argparse.Namespace, startup_mark: float) -> RuntimeConfig: +def _build_runtime( + args: argparse.Namespace, + startup_mark: float, + *, + project_dir: Path | None = None, +) -> RuntimeConfig: settings_path = Path(args.settings).expanduser() if args.settings else None max_rows = args.max_rows if args.max_rows and args.max_rows > 0 else None mock_install = args.mock_install if args.mock_install != "real" else None @@ -297,6 +360,7 @@ def _build_runtime(args: argparse.Namespace, startup_mark: float) -> RuntimeConf return RuntimeConfig( settings_path=settings_path, + project_dir=project_dir, theme=getattr(args, "theme", None), max_rows=max_rows, debug_mode=bool(args.debug), @@ -328,15 +392,25 @@ def main() -> int: ) log_startup_step("cli_start") + # Extract positional project-dir before argparse so we can route + # connections/history/starred into /.sqlit/. + with startup_span("cli_extract_project_dir"): + project_dir, filtered_argv = _extract_project_dir(sys.argv) # Extract connection URL before argparse (URLs conflict with subcommands) with startup_span("cli_extract_connection_url"): - connection_url, filtered_argv = _extract_connection_url(sys.argv) + connection_url, filtered_argv = _extract_connection_url(filtered_argv) log_startup_step("cli_parser_start") parser = argparse.ArgumentParser( prog="sqlit", description="A terminal UI for SQL databases", - epilog="Connect via URL: sqlit mysql://user:pass@host/db, sqlit sqlite:///path/to/db.sqlite", + epilog=( + "Connect via URL: sqlit mysql://user:pass@host/db, " + "sqlit sqlite:///path/to/db.sqlite\n" + "Project mode: sqlit . or sqlit /path/to/project — " + "connections, history, and starred queries live in " + "/.sqlit/ instead of the global config." + ), formatter_class=argparse.RawDescriptionHelpFormatter, ) @@ -581,7 +655,7 @@ def main() -> int: log_startup_step("cli_parse_end") with startup_span("runtime_build"): - runtime = _build_runtime(args, startup_mark) + runtime = _build_runtime(args, startup_mark, project_dir=project_dir) with startup_span("services_build"): services = build_app_services(runtime) diff --git a/sqlit/domains/connections/store/connections.py b/sqlit/domains/connections/store/connections.py index 18d52a54..3de9d208 100644 --- a/sqlit/domains/connections/store/connections.py +++ b/sqlit/domains/connections/store/connections.py @@ -8,6 +8,8 @@ from sqlit.shared.core.store import CONFIG_DIR, JSONFileStore if TYPE_CHECKING: + from pathlib import Path + from sqlit.domains.connections.app.credentials import CredentialsService from sqlit.domains.connections.domain.config import ConnectionConfig @@ -26,8 +28,13 @@ class ConnectionStore(JSONFileStore): _instance: ConnectionStore | None = None is_persistent: bool = True - def __init__(self, credentials_service: CredentialsService | None = None) -> None: - super().__init__(CONFIG_DIR / "connections.json") + def __init__( + self, + credentials_service: CredentialsService | None = None, + *, + file_path: Path | None = None, + ) -> None: + super().__init__(file_path if file_path is not None else CONFIG_DIR / "connections.json") self._credentials_service = credentials_service @property diff --git a/sqlit/domains/query/store/starred.py b/sqlit/domains/query/store/starred.py index dfd17591..0e6853cf 100644 --- a/sqlit/domains/query/store/starred.py +++ b/sqlit/domains/query/store/starred.py @@ -2,8 +2,13 @@ from __future__ import annotations +from typing import TYPE_CHECKING + from sqlit.shared.core.store import CONFIG_DIR, JSONFileStore +if TYPE_CHECKING: + from pathlib import Path + class StarredStore(JSONFileStore): """Store for managing starred queries. @@ -17,8 +22,8 @@ class StarredStore(JSONFileStore): _instance: StarredStore | None = None - def __init__(self) -> None: - super().__init__(CONFIG_DIR / "starred_queries.json") + def __init__(self, *, file_path: Path | None = None) -> None: + super().__init__(file_path if file_path is not None else CONFIG_DIR / "starred_queries.json") @classmethod def get_instance(cls) -> StarredStore: diff --git a/sqlit/shared/app/runtime.py b/sqlit/shared/app/runtime.py index 7109e23f..aa8a41ea 100644 --- a/sqlit/shared/app/runtime.py +++ b/sqlit/shared/app/runtime.py @@ -30,6 +30,7 @@ class RuntimeConfig: """Runtime configuration provided by CLI or tests.""" settings_path: Path | None = None + project_dir: Path | None = None theme: str | None = None max_rows: int | None = None debug_mode: bool = False @@ -47,6 +48,11 @@ class RuntimeConfig: query_alert_mode: int = 0 mock: MockConfig = field(default_factory=MockConfig) + @property + def project_config_dir(self) -> Path | None: + """The `.sqlit/` directory inside the project, if project_dir is set.""" + return self.project_dir / ".sqlit" if self.project_dir is not None else None + @classmethod def from_env(cls) -> RuntimeConfig: def _parse_startup_mark(value: str | None) -> float | None: diff --git a/sqlit/shared/app/services.py b/sqlit/shared/app/services.py index 1c4b7c97..c17ddbb7 100644 --- a/sqlit/shared/app/services.py +++ b/sqlit/shared/app/services.py @@ -264,12 +264,26 @@ def build_app_services( credentials_service = credentials_service or build_credentials_service(settings_store) if credentials_service is None: raise RuntimeError("Credentials service is not available.") + + # Project-scoped paths: when runtime.project_dir is set, connections, + # history, and starred queries live in /.sqlit/ instead of + # the global CONFIG_DIR. Settings, credentials, and theme stay global. + project_config = runtime.project_config_dir + if project_config is not None: + project_config.mkdir(parents=True, exist_ok=True) + project_connections_path = project_config / "connections.json" if project_config else None + project_queries_dir = project_config / "queries" if project_config else None + project_starred_path = project_config / "starred_queries.json" if project_config else None + with startup_span("build_connection_store"): - connection_store = connection_store or ConnectionStore(credentials_service=credentials_service) + connection_store = connection_store or ConnectionStore( + credentials_service=credentials_service, + file_path=project_connections_path, + ) with startup_span("build_history_store"): - history_store = history_store or HistoryStore() + history_store = history_store or HistoryStore(base_dir=project_queries_dir) with startup_span("build_starred_store"): - starred_store = starred_store or StarredStore() + starred_store = starred_store or StarredStore(file_path=project_starred_path) if hasattr(connection_store, "set_credentials_service"): connection_store.set_credentials_service(credentials_service) diff --git a/tests/unit/test_project_dir_routing.py b/tests/unit/test_project_dir_routing.py new file mode 100644 index 00000000..0f435ed8 --- /dev/null +++ b/tests/unit/test_project_dir_routing.py @@ -0,0 +1,153 @@ +"""Tests for project-scoped config routing via runtime.project_dir.""" + +from __future__ import annotations + +import json +from pathlib import Path + +import pytest + +from sqlit.cli import _extract_project_dir, _looks_like_project_path +from sqlit.domains.connections.store.connections import ConnectionStore +from sqlit.domains.query.store.history import HistoryStore +from sqlit.domains.query.store.starred import StarredStore +from sqlit.shared.app.runtime import RuntimeConfig + + +class TestLooksLikePath: + @pytest.mark.parametrize( + "arg", + [ + ".", + "..", + "./foo", + "../bar", + "/absolute/path", + "~/home-path", + "trailing/", + ], + ) + def test_path_like(self, arg: str) -> None: + assert _looks_like_project_path(arg) is True + + @pytest.mark.parametrize( + "arg", + [ + "connections", + "query", + "mysql://user@host/db", + "testdb", + "some-name", + ], + ) + def test_not_path_like(self, arg: str) -> None: + assert _looks_like_project_path(arg) is False + + +class TestExtractProjectDir: + def test_extracts_dot(self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.chdir(tmp_path) + project_dir, remaining = _extract_project_dir(["sqlit", "."]) + assert project_dir == tmp_path.resolve() + assert remaining == ["sqlit"] + + def test_extracts_absolute_path(self, tmp_path: Path) -> None: + project_dir, remaining = _extract_project_dir(["sqlit", str(tmp_path)]) + # str(tmp_path) starts with `/` so it's path-like. + assert project_dir == tmp_path.resolve() + assert remaining == ["sqlit"] + + def test_missing_dir_errors( + self, tmp_path: Path, capsys: pytest.CaptureFixture + ) -> None: + bogus = tmp_path / "does-not-exist" + with pytest.raises(SystemExit) as exc: + _extract_project_dir(["sqlit", str(bogus)]) + assert exc.value.code == 1 + captured = capsys.readouterr() + assert "does not exist" in captured.err + + def test_no_path_arg(self) -> None: + project_dir, remaining = _extract_project_dir(["sqlit", "connections", "list"]) + assert project_dir is None + assert remaining == ["sqlit", "connections", "list"] + + def test_subcommand_after_path( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + monkeypatch.chdir(tmp_path) + project_dir, remaining = _extract_project_dir( + ["sqlit", ".", "connections", "list"] + ) + assert project_dir == tmp_path.resolve() + assert remaining == ["sqlit", "connections", "list"] + + def test_flags_passthrough(self, tmp_path: Path) -> None: + project_dir, remaining = _extract_project_dir( + ["sqlit", "--theme", "dracula", str(tmp_path)] + ) + assert project_dir == tmp_path.resolve() + assert remaining == ["sqlit", "--theme", "dracula"] + + +class TestRuntimeProjectConfigDir: + def test_project_config_dir_appends_dot_sqlit(self, tmp_path: Path) -> None: + runtime = RuntimeConfig(project_dir=tmp_path) + assert runtime.project_config_dir == tmp_path / ".sqlit" + + def test_project_config_dir_none_when_unset(self) -> None: + runtime = RuntimeConfig() + assert runtime.project_config_dir is None + + +class TestStoreOverrides: + def test_history_store_uses_base_dir(self, tmp_path: Path) -> None: + s = HistoryStore(base_dir=tmp_path / "queries") + s.save_query("c", "SELECT 1") + files = list((tmp_path / "queries").rglob("*.sql")) + assert len(files) == 1 + + def test_starred_store_uses_file_path(self, tmp_path: Path) -> None: + s = StarredStore(file_path=tmp_path / "starred.json") + s.star_query("c", "SELECT 1") + assert (tmp_path / "starred.json").exists() + data = json.loads((tmp_path / "starred.json").read_text()) + assert data == {"c": ["SELECT 1"]} + + def test_connection_store_uses_file_path(self, tmp_path: Path) -> None: + s = ConnectionStore(file_path=tmp_path / "conns.json") + assert s.file_path == tmp_path / "conns.json" + + +class TestBuildAppServicesProjectDir: + def test_project_dir_routes_history_and_starred( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + # Keep global config out of the way. + global_config = tmp_path / "global-config" + monkeypatch.setenv("SQLIT_CONFIG_DIR", str(global_config)) + + # Reset the module-level CONFIG_DIR which was resolved at import time. + # We do this by directly patching the stores' constructed paths via + # the explicit overrides — that's the contract under test. + project = tmp_path / "project" + project.mkdir() + + runtime = RuntimeConfig(project_dir=project) + project_config = runtime.project_config_dir + assert project_config is not None + project_config.mkdir(parents=True, exist_ok=True) + + history = HistoryStore(base_dir=project_config / "queries") + starred = StarredStore(file_path=project_config / "starred_queries.json") + connections = ConnectionStore(file_path=project_config / "connections.json") + + history.save_query("conn-a", "SELECT 1") + starred.star_query("conn-a", "SELECT 1") + + # All artifacts ended up in /.sqlit/, not in global. + assert (project / ".sqlit" / "queries").is_dir() + assert (project / ".sqlit" / "starred_queries.json").is_file() + assert connections.file_path.parent == project / ".sqlit" + # Global config was not touched. + assert not global_config.exists() or not any(global_config.iterdir()) From 9c3bf952f328223b950990aec81026a43370c382 Mon Sep 17 00:00:00 2001 From: Maxteabag <18162810+Maxteabag@users.noreply.github.com> Date: Sun, 24 May 2026 11:55:09 +0200 Subject: [PATCH 3/9] Add `space + E` to edit current query in a terminal editor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When the query editor has focus, the leader menu picks up a new "Edit in editor" entry. On first use (or when the saved preference is no longer installed) a picker pops up listing detected terminal editors (nvim, vim, helix, kakoune, micro, nano, emacs, vi). Missing ones are grayed out. The selection is saved to settings as `preferred_editor`. The action writes the current query buffer to a temp `.sql` file with a meaningful name (`sqlit-edit--XXXX.sql`), suspends the TUI via `app.suspend()`, runs the editor blocking on that file, then loads the result back into the buffer. When the connection is saved, the edited text is also saved to history as a new entry — so edits land in the file-backed history regardless of whether they were executed. No-op edits short-circuit. --- sqlit/core/keymap.py | 7 + sqlit/core/leader_commands.py | 1 + sqlit/domains/query/app/editor.py | 72 +++++++++++ sqlit/domains/query/state/query_normal.py | 4 + .../query/ui/mixins/query_execution.py | 85 ++++++++++++ sqlit/domains/query/ui/screens/__init__.py | 2 + .../domains/query/ui/screens/editor_picker.py | 111 ++++++++++++++++ sqlit/domains/shell/ui/mixins/ui_leader.py | 3 + tests/ui/keybindings/test_state_machine.py | 31 +++++ tests/ui/test_edit_query_in_editor.py | 122 ++++++++++++++++++ tests/unit/test_editor_detection.py | 112 ++++++++++++++++ 11 files changed, 550 insertions(+) create mode 100644 sqlit/domains/query/app/editor.py create mode 100644 sqlit/domains/query/ui/screens/editor_picker.py create mode 100644 tests/ui/test_edit_query_in_editor.py create mode 100644 tests/unit/test_editor_detection.py diff --git a/sqlit/core/keymap.py b/sqlit/core/keymap.py index bcd4aed3..5fbd9943 100644 --- a/sqlit/core/keymap.py +++ b/sqlit/core/keymap.py @@ -196,6 +196,13 @@ def _build_leader_commands(self) -> list[LeaderCommandDef]: # Actions LeaderCommandDef("z", "cancel_operation", "Cancel", "Actions", guard="query_executing"), LeaderCommandDef("t", "change_theme", "Change Theme", "Actions"), + LeaderCommandDef( + "E", + "edit_query_in_editor", + "Edit in editor", + "Actions", + guard="query_focused", + ), LeaderCommandDef("h", "show_help", "Help", "Actions"), LeaderCommandDef("space", "telescope", "Telescope", "Actions"), LeaderCommandDef("slash", "telescope_filter", "Telescope Search", "Actions"), diff --git a/sqlit/core/leader_commands.py b/sqlit/core/leader_commands.py index be378bdb..a735c1ae 100644 --- a/sqlit/core/leader_commands.py +++ b/sqlit/core/leader_commands.py @@ -11,6 +11,7 @@ LEADER_GUARDS: dict[str, Callable[[InputContext], bool]] = { "has_connection": lambda ctx: ctx.has_connection, "query_executing": lambda ctx: ctx.query_executing, + "query_focused": lambda ctx: ctx.focus == "query", } diff --git a/sqlit/domains/query/app/editor.py b/sqlit/domains/query/app/editor.py new file mode 100644 index 00000000..aee547ce --- /dev/null +++ b/sqlit/domains/query/app/editor.py @@ -0,0 +1,72 @@ +"""Detect terminal editors and resolve the user's editor preference.""" + +from __future__ import annotations + +import os +import shutil +from dataclasses import dataclass + + +# Terminal editors we know about, ordered by typical preference. +# The order is what surfaces in the picker top-down. +_KNOWN_EDITORS: tuple[tuple[str, str], ...] = ( + ("nvim", "Neovim"), + ("vim", "Vim"), + ("hx", "Helix"), + ("kak", "Kakoune"), + ("micro", "Micro"), + ("nano", "Nano"), + ("emacs", "Emacs"), + ("vi", "Vi"), +) + + +@dataclass(frozen=True) +class EditorEntry: + """A terminal editor candidate.""" + + command: str + display_name: str + path: str | None # None when the binary isn't on PATH. + + @property + def is_installed(self) -> bool: + return self.path is not None + + +def detect_editors() -> list[EditorEntry]: + """Return all known editors, with each entry's resolved path or None.""" + return [ + EditorEntry(command=cmd, display_name=name, path=shutil.which(cmd)) + for cmd, name in _KNOWN_EDITORS + ] + + +def env_default_editor() -> str | None: + """Return the first usable editor command from $VISUAL or $EDITOR.""" + for var in ("VISUAL", "EDITOR"): + value = os.environ.get(var, "").strip() + if not value: + continue + # $EDITOR can include args (e.g. "code -w"). Take the binary. + cmd = value.split()[0] + if shutil.which(cmd): + return cmd + return None + + +def resolve_editor(preferred: str | None) -> str | None: + """Pick the editor to use: preferred (if installed) else $VISUAL/$EDITOR.""" + if preferred and shutil.which(preferred): + return preferred + return env_default_editor() + + +def build_editor_argv(editor_cmd: str) -> list[str]: + """Build the argv prefix for invoking the editor on a file. + + `emacs` is special-cased to use `-nw` so it stays in the terminal. + """ + if editor_cmd == "emacs": + return ["emacs", "-nw"] + return [editor_cmd] diff --git a/sqlit/domains/query/state/query_normal.py b/sqlit/domains/query/state/query_normal.py index afbc9e7e..2f2f6ae6 100644 --- a/sqlit/domains/query/state/query_normal.py +++ b/sqlit/domains/query/state/query_normal.py @@ -22,6 +22,10 @@ def _setup_actions(self) -> None: self.allows("g_leader_key", label="Go", help="Go motions (menu)") self.allows("new_query", label="New", help="New query (clear all)") self.allows("show_history", label="History", help="Query history") + self.allows( + "edit_query_in_editor", + help="Open current query in your terminal editor", + ) # Vim cursor movement self.allows("cursor_left", help="Move cursor left") self.allows("cursor_right", help="Move cursor right") diff --git a/sqlit/domains/query/ui/mixins/query_execution.py b/sqlit/domains/query/ui/mixins/query_execution.py index 08ba29fe..4390ed9c 100644 --- a/sqlit/domains/query/ui/mixins/query_execution.py +++ b/sqlit/domains/query/ui/mixins/query_execution.py @@ -754,6 +754,91 @@ def _apply_history_query(self: QueryMixinHost, query: str) -> None: # Focus query input - this triggers on_descendant_focus which updates footer bindings self.query_input.focus() + PREFERRED_EDITOR_SETTING = "preferred_editor" + + def action_edit_query_in_editor(self: QueryMixinHost) -> None: + """Open the current query in the user's terminal editor. + + Pops up an editor picker on first use (or whenever the saved + preference is missing from PATH). On editor exit, replaces the + query buffer and — when the connection is saved — saves the + edited text to history regardless of execution. + """ + from sqlit.domains.query.app.editor import resolve_editor + + settings = self.services.settings_store.load_all() + preferred = settings.get(self.PREFERRED_EDITOR_SETTING) + editor_cmd = resolve_editor(preferred) + if editor_cmd is None: + self._open_editor_picker() + return + self._run_external_editor(editor_cmd) + + def _open_editor_picker(self: QueryMixinHost) -> None: + from ..screens import EditorPickerScreen + + def on_pick(result: str | None) -> None: + if not result: + return + settings = self.services.settings_store.load_all() + settings[self.PREFERRED_EDITOR_SETTING] = result + self.services.settings_store.save_all(settings) + self._run_external_editor(result) + + self.push_screen(EditorPickerScreen(), on_pick) + + def _run_external_editor(self: QueryMixinHost, editor_cmd: str) -> None: + import os + import subprocess + import tempfile + from datetime import datetime + + from sqlit.domains.query.app.editor import build_editor_argv + + original = self.query_input.text + timestamp = datetime.now().strftime("%Y%m%dT%H%M%S") + fd, path_str = tempfile.mkstemp( + prefix=f"sqlit-edit-{timestamp}-", + suffix=".sql", + ) + try: + with os.fdopen(fd, "w", encoding="utf-8") as f: + f.write(original) + argv = build_editor_argv(editor_cmd) + [path_str] + try: + with self.suspend(): + subprocess.run(argv, check=False) + except Exception as exc: + self.notify(f"Failed to launch editor '{editor_cmd}': {exc}", severity="error") + return + + try: + with open(path_str, encoding="utf-8") as f: + edited = f.read() + except OSError as exc: + self.notify(f"Could not read edited file: {exc}", severity="error") + return + finally: + try: + os.unlink(path_str) + except OSError: + pass + + # Trim a single trailing newline that most editors add on save. + if edited.endswith("\n"): + edited = edited[:-1] + + if edited == original: + return + + self._apply_history_query(edited) + + if self.current_config and self._should_save_query_history(self.current_config): + database = getattr(self, "_active_database", None) or "" + self._get_history_store().save_query( + self.current_config.name, edited, database=database + ) + def action_show_history(self: QueryMixinHost) -> None: """Show query history for the current connection.""" if not self.current_config: diff --git a/sqlit/domains/query/ui/screens/__init__.py b/sqlit/domains/query/ui/screens/__init__.py index 9408a7d5..39f58255 100644 --- a/sqlit/domains/query/ui/screens/__init__.py +++ b/sqlit/domains/query/ui/screens/__init__.py @@ -1,11 +1,13 @@ """Query UI screens.""" from .char_pending_menu import CharPendingMenuScreen +from .editor_picker import EditorPickerScreen from .query_history import QueryHistoryScreen from .text_object_menu import TextObjectMenuScreen __all__ = [ "CharPendingMenuScreen", + "EditorPickerScreen", "QueryHistoryScreen", "TextObjectMenuScreen", ] diff --git a/sqlit/domains/query/ui/screens/editor_picker.py b/sqlit/domains/query/ui/screens/editor_picker.py new file mode 100644 index 00000000..87fd9672 --- /dev/null +++ b/sqlit/domains/query/ui/screens/editor_picker.py @@ -0,0 +1,111 @@ +"""Pick a terminal editor for 'Edit query in editor'. + +Shown the first time the action is invoked, or whenever the saved +preference is missing from the filesystem. The result is the editor +command string (e.g. ``"nvim"``); the caller persists it to settings. +""" + +from __future__ import annotations + +from textual.app import ComposeResult +from textual.binding import Binding +from textual.screen import ModalScreen +from textual.widgets import OptionList, Static +from textual.widgets.option_list import Option + +from sqlit.domains.query.app.editor import EditorEntry, detect_editors +from sqlit.shared.ui.widgets import Dialog + + +class EditorPickerScreen(ModalScreen[str | None]): + """Modal list of terminal editors. Installed ones are selectable; + missing ones render disabled.""" + + BINDINGS = [ + Binding("escape", "cancel", "Cancel", priority=True), + Binding("enter", "select", "Select"), + ] + + CSS = """ + EditorPickerScreen { + align: center middle; + background: transparent; + } + + #editor-picker-dialog { + width: 60; + max-width: 90%; + height: auto; + max-height: 24; + } + + #editor-picker-description { + margin-bottom: 1; + color: $text-muted; + height: auto; + } + + #editor-picker-list { + background: $surface; + border: none; + height: auto; + max-height: 16; + } + """ + + def __init__(self, *, entries: list[EditorEntry] | None = None) -> None: + super().__init__() + self._entries = entries if entries is not None else detect_editors() + + def compose(self) -> ComposeResult: + shortcuts = [("Select", ""), ("Cancel", "")] + with Dialog( + id="editor-picker-dialog", + title="Pick a terminal editor", + shortcuts=shortcuts, + ): + yield Static( + "Choose your preferred terminal editor. Missing entries are grayed out.", + id="editor-picker-description", + ) + yield OptionList(id="editor-picker-list") + + def on_mount(self) -> None: + option_list = self.query_one("#editor-picker-list", OptionList) + first_installed: int | None = None + for idx, entry in enumerate(self._entries): + label = self._format_label(entry) + option = Option(label, id=entry.command, disabled=not entry.is_installed) + option_list.add_option(option) + if first_installed is None and entry.is_installed: + first_installed = idx + if first_installed is not None: + option_list.highlighted = first_installed + option_list.focus() + + @staticmethod + def _format_label(entry: EditorEntry) -> str: + if entry.is_installed: + return f"{entry.display_name} [dim]({entry.path})[/]" + return f"[dim]{entry.display_name} (not installed)[/]" + + def action_select(self) -> None: + option_list = self.query_one("#editor-picker-list", OptionList) + idx = option_list.highlighted + if idx is None: + return + try: + option = option_list.get_option_at_index(idx) + except Exception: + return + if option is None or option.disabled or option.id is None: + return + self.dismiss(option.id) + + def on_option_list_option_selected(self, event: OptionList.OptionSelected) -> None: + if event.option.disabled or event.option.id is None: + return + self.dismiss(event.option.id) + + def action_cancel(self) -> None: + self.dismiss(None) diff --git a/sqlit/domains/shell/ui/mixins/ui_leader.py b/sqlit/domains/shell/ui/mixins/ui_leader.py index 55a225bc..ea99674a 100644 --- a/sqlit/domains/shell/ui/mixins/ui_leader.py +++ b/sqlit/domains/shell/ui/mixins/ui_leader.py @@ -106,6 +106,9 @@ def action_leader_cancel_operation(self: UINavigationMixinHost) -> None: def action_leader_change_theme(self: UINavigationMixinHost) -> None: self._execute_leader_command("change_theme") + def action_leader_edit_query_in_editor(self: UINavigationMixinHost) -> None: + self._execute_leader_command("edit_query_in_editor") + def action_leader_toggle_process_worker(self: UINavigationMixinHost) -> None: self._execute_leader_command("toggle_process_worker") diff --git a/tests/ui/keybindings/test_state_machine.py b/tests/ui/keybindings/test_state_machine.py index 72569100..9f2ed417 100644 --- a/tests/ui/keybindings/test_state_machine.py +++ b/tests/ui/keybindings/test_state_machine.py @@ -213,3 +213,34 @@ def test_visual_mode_shown_in_footer_when_node_highlighted(self): left, _right = sm.get_display_bindings(ctx) actions = [b.action for b in left] assert "enter_tree_visual_mode" in actions + + +class TestEditQueryInEditorLeaderCommand: + """The space+E leader command is gated on the query editor having focus.""" + + def test_allowed_when_query_focused(self): + sm = UIStateMachine() + ctx = make_context( + focus="query", + leader_pending=True, + leader_menu="leader", + ) + assert sm.check_action(ctx, "leader_edit_query_in_editor") is True + + def test_forbidden_when_explorer_focused(self): + sm = UIStateMachine() + ctx = make_context( + focus="explorer", + leader_pending=True, + leader_menu="leader", + ) + assert sm.check_action(ctx, "leader_edit_query_in_editor") is False + + def test_forbidden_when_results_focused(self): + sm = UIStateMachine() + ctx = make_context( + focus="results", + leader_pending=True, + leader_menu="leader", + ) + assert sm.check_action(ctx, "leader_edit_query_in_editor") is False diff --git a/tests/ui/test_edit_query_in_editor.py b/tests/ui/test_edit_query_in_editor.py new file mode 100644 index 00000000..bb2f995e --- /dev/null +++ b/tests/ui/test_edit_query_in_editor.py @@ -0,0 +1,122 @@ +"""End-to-end tests for the 'Edit query in editor' leader command.""" + +from __future__ import annotations + +from contextlib import contextmanager +from pathlib import Path +from unittest.mock import patch + +import pytest + +from sqlit.domains.shell.app.main import SSMSTUI + + +@contextmanager +def _noop_suspend(*_args, **_kwargs): + yield + +from .mocks import ( + MockConnectionStore, + MockHistoryStore, + MockSettingsStore, + build_test_services, + create_test_connection, +) + + +def _make_app(settings: dict | None = None) -> SSMSTUI: + conn = create_test_connection("test-conn", "sqlite") + services = build_test_services( + connection_store=MockConnectionStore([conn]), + settings_store=MockSettingsStore(settings or {"theme": "tokyo-night"}), + history_store=MockHistoryStore(), + ) + return SSMSTUI(services=services) + + +def _fake_editor_writes(new_text: str): + """Return a subprocess.run replacement that overwrites the temp file's + contents with `new_text`, simulating an editor session.""" + + def _run(argv, *args, **kwargs): + path = Path(argv[-1]) + path.write_text(new_text, encoding="utf-8") + + class _Result: + returncode = 0 + + return _Result() + + return _run + + +class TestEditQueryInEditor: + @pytest.mark.asyncio + async def test_invokes_preferred_editor_and_updates_buffer(self): + """With a saved preference + installed editor, run subprocess and + load the edited content back into the query buffer.""" + app = _make_app(settings={"theme": "tokyo-night", "preferred_editor": "nvim"}) + + async with app.run_test(size=(120, 40)) as pilot: + app.query_input.text = "SELECT 1" + await pilot.pause() + + with patch( + "sqlit.domains.query.app.editor.shutil.which", + side_effect=lambda cmd: "/usr/bin/nvim" if cmd == "nvim" else None, + ), patch( + "subprocess.run", + side_effect=_fake_editor_writes("SELECT 2 FROM users"), + ), patch.object(app, "suspend", _noop_suspend): + app.action_edit_query_in_editor() + await pilot.pause() + + assert app.query_input.text == "SELECT 2 FROM users" + + @pytest.mark.asyncio + async def test_picker_opens_when_no_preference_set(self): + """Empty settings + no detected env editor → picker pops up.""" + from sqlit.domains.query.ui.screens import EditorPickerScreen + + app = _make_app(settings={"theme": "tokyo-night"}) + + async with app.run_test(size=(120, 40)) as pilot: + app.query_input.text = "SELECT 1" + await pilot.pause() + + with patch( + "sqlit.domains.query.app.editor.shutil.which", + return_value=None, + ), patch.dict("os.environ", {}, clear=False) as env: + env.pop("VISUAL", None) + env.pop("EDITOR", None) + app.action_edit_query_in_editor() + await pilot.pause() + + assert any( + isinstance(screen, EditorPickerScreen) + for screen in app.screen_stack + ) + + @pytest.mark.asyncio + async def test_no_change_when_buffer_text_unchanged(self): + """If the editor closes without modifying the file, nothing happens + (history isn't touched, buffer is untouched).""" + app = _make_app(settings={"theme": "tokyo-night", "preferred_editor": "nvim"}) + + async with app.run_test(size=(120, 40)) as pilot: + app.query_input.text = "SELECT same" + await pilot.pause() + + with patch( + "sqlit.domains.query.app.editor.shutil.which", + side_effect=lambda cmd: "/usr/bin/nvim" if cmd == "nvim" else None, + ), patch( + "subprocess.run", side_effect=_fake_editor_writes("SELECT same") + ), patch.object(app, "suspend", _noop_suspend): + app.action_edit_query_in_editor() + await pilot.pause() + + assert app.query_input.text == "SELECT same" + # MockHistoryStore tracks save_query calls via its entries dict. + assert app.services.history_store.entries == {} diff --git a/tests/unit/test_editor_detection.py b/tests/unit/test_editor_detection.py new file mode 100644 index 00000000..fae7df8f --- /dev/null +++ b/tests/unit/test_editor_detection.py @@ -0,0 +1,112 @@ +"""Tests for terminal editor detection and preference resolution.""" + +from __future__ import annotations + +from unittest.mock import patch + +import pytest + +from sqlit.domains.query.app.editor import ( + build_editor_argv, + detect_editors, + env_default_editor, + resolve_editor, +) + + +class TestDetectEditors: + def test_returns_known_editors_with_resolved_paths(self) -> None: + with patch("sqlit.domains.query.app.editor.shutil.which") as which: + which.side_effect = lambda cmd: f"/usr/bin/{cmd}" if cmd in {"nvim", "nano"} else None + entries = detect_editors() + + names = [e.command for e in entries] + assert "nvim" in names + assert "vim" in names + + nvim = next(e for e in entries if e.command == "nvim") + vim = next(e for e in entries if e.command == "vim") + assert nvim.is_installed + assert nvim.path == "/usr/bin/nvim" + assert not vim.is_installed + assert vim.path is None + + +class TestEnvDefault: + def test_uses_visual_first(self, monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv("VISUAL", "nvim") + monkeypatch.setenv("EDITOR", "vim") + with patch( + "sqlit.domains.query.app.editor.shutil.which", + side_effect=lambda cmd: "/usr/bin/nvim" if cmd == "nvim" else None, + ): + assert env_default_editor() == "nvim" + + def test_falls_back_to_editor(self, monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.delenv("VISUAL", raising=False) + monkeypatch.setenv("EDITOR", "vim") + with patch( + "sqlit.domains.query.app.editor.shutil.which", + side_effect=lambda cmd: "/usr/bin/vim" if cmd == "vim" else None, + ): + assert env_default_editor() == "vim" + + def test_strips_args_from_env(self, monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.delenv("VISUAL", raising=False) + monkeypatch.setenv("EDITOR", "code -w") + with patch( + "sqlit.domains.query.app.editor.shutil.which", + side_effect=lambda cmd: "/usr/local/bin/code" if cmd == "code" else None, + ): + assert env_default_editor() == "code" + + def test_none_when_unset(self, monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.delenv("VISUAL", raising=False) + monkeypatch.delenv("EDITOR", raising=False) + assert env_default_editor() is None + + def test_none_when_env_editor_missing_on_path( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + monkeypatch.setenv("EDITOR", "made-up-editor") + with patch("sqlit.domains.query.app.editor.shutil.which", return_value=None): + assert env_default_editor() is None + + +class TestResolveEditor: + def test_uses_preferred_when_installed(self, monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.delenv("VISUAL", raising=False) + monkeypatch.delenv("EDITOR", raising=False) + with patch( + "sqlit.domains.query.app.editor.shutil.which", + side_effect=lambda cmd: "/usr/bin/hx" if cmd == "hx" else None, + ): + assert resolve_editor("hx") == "hx" + + def test_falls_back_to_env_when_preferred_missing( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + monkeypatch.setenv("EDITOR", "vim") + + def which(cmd: str) -> str | None: + return "/usr/bin/vim" if cmd == "vim" else None + + with patch("sqlit.domains.query.app.editor.shutil.which", side_effect=which): + # preferred "hx" not installed → falls back to $EDITOR + assert resolve_editor("hx") == "vim" + + def test_none_when_nothing_works(self, monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.delenv("VISUAL", raising=False) + monkeypatch.delenv("EDITOR", raising=False) + with patch("sqlit.domains.query.app.editor.shutil.which", return_value=None): + assert resolve_editor(None) is None + + +class TestBuildEditorArgv: + def test_default_is_just_command(self) -> None: + assert build_editor_argv("nvim") == ["nvim"] + assert build_editor_argv("vim") == ["vim"] + + def test_emacs_gets_nw_flag(self) -> None: + # emacs without -nw spawns a GUI window — we want terminal mode. + assert build_editor_argv("emacs") == ["emacs", "-nw"] From 3efece18afd0270b8139fe89c0291601ad7e20ab Mon Sep 17 00:00:00 2001 From: Maxteabag <18162810+Maxteabag@users.noreply.github.com> Date: Sun, 24 May 2026 12:17:45 +0200 Subject: [PATCH 4/9] Bind editor leader to lowercase `o` (Open in editor) Capital `E` required Shift; `o` is unused in the global leader menu (e/f/c/x/z/t/h/space/slash/q are taken). Action and behavior unchanged. --- sqlit/core/keymap.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sqlit/core/keymap.py b/sqlit/core/keymap.py index 5fbd9943..75883f0e 100644 --- a/sqlit/core/keymap.py +++ b/sqlit/core/keymap.py @@ -197,9 +197,9 @@ def _build_leader_commands(self) -> list[LeaderCommandDef]: LeaderCommandDef("z", "cancel_operation", "Cancel", "Actions", guard="query_executing"), LeaderCommandDef("t", "change_theme", "Change Theme", "Actions"), LeaderCommandDef( - "E", + "o", "edit_query_in_editor", - "Edit in editor", + "Open in editor", "Actions", guard="query_focused", ), From 5553301abc915c44efdc3e717ff03c935363d2d0 Mon Sep 17 00:00:00 2001 From: Maxteabag <18162810+Maxteabag@users.noreply.github.com> Date: Sun, 24 May 2026 12:26:27 +0200 Subject: [PATCH 5/9] Notify on editor picker open / no editor detected / edit result MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The action was silent in the empty-state cases. Add user-visible notifications: - No installed editor anywhere → error notification with install hint; the picker no longer opens (all options would be disabled anyway). - Picker about to open → info notification explaining first-time setup. - Picker cancelled → warning notification. - Editor closed with no changes → info notification. - Buffer updated from editor → info notification. --- .../query/ui/mixins/query_execution.py | 20 +++++++++++ tests/ui/test_edit_query_in_editor.py | 33 +++++++++++++++++-- 2 files changed, 50 insertions(+), 3 deletions(-) diff --git a/sqlit/domains/query/ui/mixins/query_execution.py b/sqlit/domains/query/ui/mixins/query_execution.py index 4390ed9c..293f9ef9 100644 --- a/sqlit/domains/query/ui/mixins/query_execution.py +++ b/sqlit/domains/query/ui/mixins/query_execution.py @@ -775,14 +775,32 @@ def action_edit_query_in_editor(self: QueryMixinHost) -> None: self._run_external_editor(editor_cmd) def _open_editor_picker(self: QueryMixinHost) -> None: + from sqlit.domains.query.app.editor import detect_editors + from ..screens import EditorPickerScreen + if not any(e.is_installed for e in detect_editors()): + self.notify( + "No terminal editor detected. Install nvim, vim, hx, micro, or " + "nano and try again (or set $EDITOR).", + severity="error", + timeout=8, + ) + return + + self.notify( + "No preferred editor set — pick one (saved for next time).", + severity="information", + ) + def on_pick(result: str | None) -> None: if not result: + self.notify("Editor selection cancelled.", severity="warning") return settings = self.services.settings_store.load_all() settings[self.PREFERRED_EDITOR_SETTING] = result self.services.settings_store.save_all(settings) + self.notify(f"Editor set to '{result}'. Launching…", severity="information") self._run_external_editor(result) self.push_screen(EditorPickerScreen(), on_pick) @@ -829,9 +847,11 @@ def _run_external_editor(self: QueryMixinHost, editor_cmd: str) -> None: edited = edited[:-1] if edited == original: + self.notify("Editor closed — no changes.", severity="information") return self._apply_history_query(edited) + self.notify("Query updated from editor.", severity="information") if self.current_config and self._should_save_query_history(self.current_config): database = getattr(self, "_active_database", None) or "" diff --git a/tests/ui/test_edit_query_in_editor.py b/tests/ui/test_edit_query_in_editor.py index bb2f995e..5fcbc33e 100644 --- a/tests/ui/test_edit_query_in_editor.py +++ b/tests/ui/test_edit_query_in_editor.py @@ -74,8 +74,8 @@ async def test_invokes_preferred_editor_and_updates_buffer(self): assert app.query_input.text == "SELECT 2 FROM users" @pytest.mark.asyncio - async def test_picker_opens_when_no_preference_set(self): - """Empty settings + no detected env editor → picker pops up.""" + async def test_picker_opens_when_no_preference_but_editor_detected(self): + """Empty settings + no env editor + at least one installed editor → picker pops up.""" from sqlit.domains.query.ui.screens import EditorPickerScreen app = _make_app(settings={"theme": "tokyo-night"}) @@ -86,7 +86,7 @@ async def test_picker_opens_when_no_preference_set(self): with patch( "sqlit.domains.query.app.editor.shutil.which", - return_value=None, + side_effect=lambda cmd: "/usr/bin/nvim" if cmd == "nvim" else None, ), patch.dict("os.environ", {}, clear=False) as env: env.pop("VISUAL", None) env.pop("EDITOR", None) @@ -98,6 +98,33 @@ async def test_picker_opens_when_no_preference_set(self): for screen in app.screen_stack ) + @pytest.mark.asyncio + async def test_no_picker_when_no_editor_detected(self): + """Nothing installed at all → notify with install hint, no picker.""" + from sqlit.domains.query.ui.screens import EditorPickerScreen + + app = _make_app(settings={"theme": "tokyo-night"}) + notifications: list[str] = [] + original_notify = app.notify + app.notify = lambda msg, **kw: (notifications.append(msg), original_notify(msg, **kw))[1] # type: ignore[assignment] + + async with app.run_test(size=(120, 40)) as pilot: + app.query_input.text = "SELECT 1" + await pilot.pause() + + with patch( + "sqlit.domains.query.app.editor.shutil.which", return_value=None + ), patch.dict("os.environ", {}, clear=False) as env: + env.pop("VISUAL", None) + env.pop("EDITOR", None) + app.action_edit_query_in_editor() + await pilot.pause() + + assert not any( + isinstance(screen, EditorPickerScreen) for screen in app.screen_stack + ) + assert any("No terminal editor detected" in n for n in notifications) + @pytest.mark.asyncio async def test_no_change_when_buffer_text_unchanged(self): """If the editor closes without modifying the file, nothing happens From e89210ef4d006a03e7a73dd484634b5e1c9501ae Mon Sep 17 00:00:00 2001 From: Maxteabag <18162810+Maxteabag@users.noreply.github.com> Date: Sun, 24 May 2026 12:33:30 +0200 Subject: [PATCH 6/9] Drop query_focused guard from space+o editor command MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When the guard failed (focus ≠ "query"), `resolve_action` returned None for the leader resolution, then `on_key` cancelled leader-pending and retried the raw key. In query NORMAL mode that retry resolved `o` to vim's `open_line_below` action, which silently inserted a blank line — looking like "nothing happened" to the user. The action handles an empty buffer fine, so the guard isn't needed. Drop it so space+o always reaches the editor regardless of which pane has focus. --- sqlit/core/keymap.py | 1 - tests/ui/keybindings/test_state_machine.py | 11 ++++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/sqlit/core/keymap.py b/sqlit/core/keymap.py index 75883f0e..5e93e0f4 100644 --- a/sqlit/core/keymap.py +++ b/sqlit/core/keymap.py @@ -201,7 +201,6 @@ def _build_leader_commands(self) -> list[LeaderCommandDef]: "edit_query_in_editor", "Open in editor", "Actions", - guard="query_focused", ), LeaderCommandDef("h", "show_help", "Help", "Actions"), LeaderCommandDef("space", "telescope", "Telescope", "Actions"), diff --git a/tests/ui/keybindings/test_state_machine.py b/tests/ui/keybindings/test_state_machine.py index 9f2ed417..8f29bad6 100644 --- a/tests/ui/keybindings/test_state_machine.py +++ b/tests/ui/keybindings/test_state_machine.py @@ -216,7 +216,8 @@ def test_visual_mode_shown_in_footer_when_node_highlighted(self): class TestEditQueryInEditorLeaderCommand: - """The space+E leader command is gated on the query editor having focus.""" + """The space+o leader command is unguarded so it never falls through + to vim `o = open_line_below` regardless of which pane has focus.""" def test_allowed_when_query_focused(self): sm = UIStateMachine() @@ -227,20 +228,20 @@ def test_allowed_when_query_focused(self): ) assert sm.check_action(ctx, "leader_edit_query_in_editor") is True - def test_forbidden_when_explorer_focused(self): + def test_allowed_when_explorer_focused(self): sm = UIStateMachine() ctx = make_context( focus="explorer", leader_pending=True, leader_menu="leader", ) - assert sm.check_action(ctx, "leader_edit_query_in_editor") is False + assert sm.check_action(ctx, "leader_edit_query_in_editor") is True - def test_forbidden_when_results_focused(self): + def test_allowed_when_results_focused(self): sm = UIStateMachine() ctx = make_context( focus="results", leader_pending=True, leader_menu="leader", ) - assert sm.check_action(ctx, "leader_edit_query_in_editor") is False + assert sm.check_action(ctx, "leader_edit_query_in_editor") is True From ded5dacb8c7c71c0e8c02268ffba75af74f2fd21 Mon Sep 17 00:00:00 2001 From: Maxteabag <18162810+Maxteabag@users.noreply.github.com> Date: Sun, 24 May 2026 13:28:18 +0200 Subject: [PATCH 7/9] Move database into path, drop redundant header lines MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `-- database:` and `-- ran:` header lines were carrying data that already had a natural home in the filesystem: - `database` → subdirectory under the connection dir. Multi-database connections (Postgres, MSSQL, etc.) get one subdir per db; single- database engines (SQLite, DuckDB) keep files flat under the connection dir. Loading walks both levels. - `ran:` timestamp → already in the filename prefix (`YYYY-MM-DDTHH-MM-SS_.sql`). Header collapses to two lines: -- sqlit:history -- connection: postgres-prod SELECT * FROM users Eviction still operates per-connection (summing files across db subdirs); re-running an existing query continues to dedup by query hash even when the database moves between executions. Files written under the previous v1 layout are migrated in-place: each file's `-- database:` header is read, the file moves into the appropriate subdir, and the header is rewritten in the trimmed form. --- sqlit/domains/query/store/history.py | 270 ++++++++++++++----- tests/unit/test_history_store_file_backed.py | 151 +++++++++-- 2 files changed, 330 insertions(+), 91 deletions(-) diff --git a/sqlit/domains/query/store/history.py b/sqlit/domains/query/store/history.py index af22d779..2d47a094 100644 --- a/sqlit/domains/query/store/history.py +++ b/sqlit/domains/query/store/history.py @@ -1,13 +1,31 @@ """File-backed query history store. -Each query is a `.sql` file under `CONFIG_DIR/queries//` -with a small comment header carrying connection name, database, and -timestamp. Files are sortable lexicographically by their timestamp prefix. - -Re-running the same query (exact text after `strip()`) updates the -existing entry by deleting the old file and writing a new one with the -current timestamp, so most-recent always sorts last (and reverse-sorts -first for the UI). +Each query is a `.sql` file under +``CONFIG_DIR/queries//[/]_.sql``. + +The connection always becomes a directory level; the database becomes a +second directory level when the connection runs against a named +database (MSSQL/Postgres/etc.). For file-based engines (SQLite, DuckDB) +where there's no database concept, files sit directly under the +connection dir. + +Each file holds an SQL-comment header followed by a blank line and the +query body:: + + -- sqlit:history + -- connection: postgres-prod + + SELECT * FROM users + +The connection name in the header is authoritative — the directory +name is sanitized + hashed for filesystem safety and is not reversible. +Timestamps and the database are encoded structurally in the path, so +they don't need to be repeated in the header. + +Re-running the same query (exact text after `strip()`) deletes the old +file and writes a new one with the current timestamp, so the most- +recent timestamp always wins and the directory listing stays +chronologically sortable. """ from __future__ import annotations @@ -62,21 +80,29 @@ def _query_hash(query: str) -> str: return hashlib.sha256(query.strip().encode("utf-8")).hexdigest()[:8] -def _connection_dir_name(connection_name: str) -> str: - """Sanitize connection name + append short hash for collision-free uniqueness.""" - safe = _SAFE_NAME.sub("_", connection_name)[:40] or "_" - short = hashlib.sha256(connection_name.encode("utf-8")).hexdigest()[:8] +def _safe_dir_name(name: str) -> str: + """Sanitize + append short hash so distinct names always get distinct dirs.""" + safe = _SAFE_NAME.sub("_", name)[:40] or "_" + short = hashlib.sha256(name.encode("utf-8")).hexdigest()[:8] return f"{safe}_{short}" +def _connection_dir_name(connection_name: str) -> str: + return _safe_dir_name(connection_name) + + +def _database_dir_name(database: str) -> str: + return _safe_dir_name(database) + + def _timestamp_to_filename(iso_ts: str) -> str: """Turn an ISO timestamp into a filesystem-safe sortable prefix.""" return iso_ts.replace(":", "-") def _filename_to_timestamp(stem_prefix: str) -> str: - """Inverse of _timestamp_to_filename. Used only as a fallback when a - stored file is missing a header.""" + """Inverse of _timestamp_to_filename, used to derive the canonical + ISO timestamp from a stored filename.""" if "T" not in stem_prefix: return stem_prefix date_part, _, time_part = stem_prefix.partition("T") @@ -87,23 +113,24 @@ def _format_entry(entry: QueryHistoryEntry) -> str: lines = [ _HEADER_MARKER, f"-- connection: {entry.connection_name}", + "", + entry.query, ] - if entry.database: - lines.append(f"-- database: {entry.database}") - lines.append(f"-- ran: {entry.timestamp}") - lines.append("") - lines.append(entry.query) if not entry.query.endswith("\n"): lines.append("") return "\n".join(lines) def _parse_entry( - text: str, *, fallback_connection: str, fallback_timestamp: str + text: str, + *, + fallback_connection: str, + fallback_database: str, + fallback_timestamp: str, ) -> QueryHistoryEntry | None: - """Parse a stored .sql file. Header is the leading run of comment - lines, terminated by the first blank line. Anything below is the - query body.""" + """Parse a stored .sql file. The header is the leading run of `--` + comment lines, terminated by the first blank line. Anything after + that is the query body.""" lines = text.splitlines() metadata: dict[str, str] = {} body_start = 0 @@ -132,7 +159,7 @@ def _parse_entry( query=query, timestamp=metadata.get("ran") or fallback_timestamp, connection_name=metadata.get("connection") or fallback_connection, - database=metadata.get("database", ""), + database=metadata.get("database", fallback_database), ) @@ -141,11 +168,12 @@ class HistoryStore: Layout:: - CONFIG_DIR/queries//_.sql + CONFIG_DIR/queries///_.sql + CONFIG_DIR/queries//_.sql # db empty - Each file holds the query prefixed by an SQL-comment header - (``-- sqlit:history``, ``-- connection:``, ``-- database:``, - ``-- ran:``) terminated by a blank line. + Each file holds the query prefixed by a small SQL-comment header + (``-- sqlit:history`` and ``-- connection:``), terminated by a + blank line. """ MAX_ENTRIES_PER_CONNECTION = 100 @@ -168,10 +196,21 @@ def _ensure_dir(self, path: Path) -> None: def _connection_dir(self, connection_name: str) -> Path: return self._base_dir / _connection_dir_name(connection_name) + def _entry_dir(self, connection_name: str, database: str) -> Path: + """Where a given (connection, database) pair's files live.""" + conn_dir = self._connection_dir(connection_name) + if database: + return conn_dir / _database_dir_name(database) + return conn_dir + def _maybe_migrate(self) -> None: if self._migrated: return self._migrated = True + self._migrate_from_legacy_json() + self._migrate_layout_v1_to_v2() + + def _migrate_from_legacy_json(self) -> None: legacy = self._base_dir.parent / "query_history.json" if not legacy.exists(): return @@ -196,24 +235,75 @@ def _maybe_migrate(self) -> None: except OSError: pass + def _migrate_layout_v1_to_v2(self) -> None: + """Move existing files written under v1 layout + (`/.sql` with `-- database:` in the header) into the + v2 layout (`//.sql`, header trimmed).""" + if not self._base_dir.is_dir(): + return + for conn_dir in self._base_dir.iterdir(): + if not conn_dir.is_dir(): + continue + for path in list(conn_dir.glob("*.sql")): + try: + text = path.read_text(encoding="utf-8") + except OSError: + continue + stem = path.stem + fallback_ts_raw = stem.rsplit("_", 1)[0] if "_" in stem else stem + fallback_ts = _filename_to_timestamp(fallback_ts_raw) + entry = _parse_entry( + text, + fallback_connection=conn_dir.name, + fallback_database="", + fallback_timestamp=fallback_ts, + ) + if entry is None: + continue + # If body matches what we'd write now (no db and no ran/database + # in the header), nothing to migrate for this file. + if not entry.database and not self._header_has_dropped_fields(text): + continue + # Rewrite into the correct location with the trimmed header. + self._write_entry(entry) + if path.exists(): + try: + path.unlink() + except OSError: + pass + + @staticmethod + def _header_has_dropped_fields(text: str) -> bool: + for line in text.splitlines(): + stripped = line.rstrip() + if stripped == "": + return False + m = _HEADER_LINE.match(stripped) + if m and m.group(1).lower() in {"ran", "database"}: + return True + return False + def _write_entry(self, entry: QueryHistoryEntry) -> Path: """Write one entry to disk, replacing any prior file with the - same query hash for the same connection. Atomic per-file.""" - conn_dir = self._connection_dir(entry.connection_name) - self._ensure_dir(conn_dir) + same query hash for this (connection, database). Atomic per-file.""" + target_dir = self._entry_dir(entry.connection_name, entry.database) + self._ensure_dir(target_dir) qhash = _query_hash(entry.query) - for existing in conn_dir.glob(f"*_{qhash}.sql"): - try: - existing.unlink() - except OSError: - pass + # Drop any same-hash file across all (conn, db) pairs so a query + # that moves between databases doesn't leave a stale duplicate. + for existing in self._all_files(entry.connection_name): + if existing.name.endswith(f"_{qhash}.sql"): + try: + existing.unlink() + except OSError: + pass filename = f"{_timestamp_to_filename(entry.timestamp)}_{qhash}.sql" - dest = conn_dir / filename + dest = target_dir / filename body = _format_entry(entry) - fd, tmp_path = tempfile.mkstemp(dir=conn_dir, prefix=".tmp_", suffix=".sql") + fd, tmp_path = tempfile.mkstemp(dir=target_dir, prefix=".tmp_", suffix=".sql") try: with os.fdopen(fd, "w", encoding="utf-8") as f: f.write(body) @@ -230,11 +320,27 @@ def _write_entry(self, entry: QueryHistoryEntry) -> Path: raise return dest - def _evict(self, connection_name: str) -> None: + def _all_files(self, connection_name: str) -> list[Path]: + """Every `.sql` file for a connection, across all database subdirs.""" conn_dir = self._connection_dir(connection_name) if not conn_dir.is_dir(): - return - files = sorted(p for p in conn_dir.glob("*.sql") if p.is_file()) + return [] + files: list[Path] = [] + for path in conn_dir.glob("*.sql"): + if path.is_file(): + files.append(path) + for child in conn_dir.iterdir(): + if child.is_dir(): + for path in child.glob("*.sql"): + if path.is_file(): + files.append(path) + return files + + def _evict(self, connection_name: str) -> None: + # Sort by filename only: timestamps live in the filename prefix, + # so lex-ordering filenames = chronological ordering. Sorting by + # full path would group by db subdir first, which is wrong. + files = sorted(self._all_files(connection_name), key=lambda p: p.name) excess = len(files) - self.MAX_ENTRIES_PER_CONNECTION if excess <= 0: return @@ -244,18 +350,11 @@ def _evict(self, connection_name: str) -> None: except OSError: pass - def _load_dir(self, conn_dir: Path) -> list[QueryHistoryEntry]: - if not conn_dir.is_dir(): - return [] - dir_name = conn_dir.name - # Dir names end with `_<8-hex-chars>`. Strip that for fallback. - fallback_connection = ( - dir_name[:-9] - if len(dir_name) > 9 and dir_name[-9] == "_" - else dir_name - ) + def _entries_from_files( + self, files: list[Path], *, fallback_connection: str + ) -> list[QueryHistoryEntry]: entries: list[QueryHistoryEntry] = [] - for path in conn_dir.glob("*.sql"): + for path in files: try: text = path.read_text(encoding="utf-8") except OSError: @@ -263,9 +362,16 @@ def _load_dir(self, conn_dir: Path) -> list[QueryHistoryEntry]: stem = path.stem fallback_ts_raw = stem.rsplit("_", 1)[0] if "_" in stem else stem fallback_ts = _filename_to_timestamp(fallback_ts_raw) + # If the file lives in a db subdir, derive db from the dir name + # (sanitized — only useful for fallback; header trumps when present). + parent = path.parent + grandparent = parent.parent + in_db_subdir = grandparent != self._base_dir and grandparent.is_dir() + fallback_db = parent.name[:-9] if in_db_subdir and len(parent.name) > 9 and parent.name[-9] == "_" else "" entry = _parse_entry( text, fallback_connection=fallback_connection, + fallback_database=fallback_db, fallback_timestamp=fallback_ts, ) if entry is not None: @@ -273,20 +379,43 @@ def _load_dir(self, conn_dir: Path) -> list[QueryHistoryEntry]: entries.sort(key=lambda e: e.timestamp, reverse=True) return entries + def _fallback_connection_from_dir(self, conn_dir: Path) -> str: + dir_name = conn_dir.name + return ( + dir_name[:-9] + if len(dir_name) > 9 and dir_name[-9] == "_" + else dir_name + ) + # ----- public API (matches HistoryStoreProtocol) ----- def load_for_connection(self, connection_name: str) -> list[QueryHistoryEntry]: self._maybe_migrate() - return self._load_dir(self._connection_dir(connection_name)) + conn_dir = self._connection_dir(connection_name) + return self._entries_from_files( + self._all_files(connection_name), + fallback_connection=self._fallback_connection_from_dir(conn_dir), + ) def load_all(self) -> list[QueryHistoryEntry]: self._maybe_migrate() if not self._base_dir.is_dir(): return [] entries: list[QueryHistoryEntry] = [] - for child in self._base_dir.iterdir(): - if child.is_dir(): - entries.extend(self._load_dir(child)) + for conn_dir in self._base_dir.iterdir(): + if not conn_dir.is_dir(): + continue + fallback_connection = self._fallback_connection_from_dir(conn_dir) + files: list[Path] = list(conn_dir.glob("*.sql")) + for db_dir in conn_dir.iterdir(): + if db_dir.is_dir(): + files.extend(db_dir.glob("*.sql")) + entries.extend( + self._entries_from_files( + [p for p in files if p.is_file()], + fallback_connection=fallback_connection, + ) + ) entries.sort(key=lambda e: e.timestamp, reverse=True) return entries @@ -306,11 +435,8 @@ def save_query(self, connection_name: str, query: str, database: str = "") -> No def delete_entry(self, connection_name: str, timestamp: str) -> bool: self._maybe_migrate() - conn_dir = self._connection_dir(connection_name) - if not conn_dir.is_dir(): - return False filename_prefix = _timestamp_to_filename(timestamp) - for path in conn_dir.glob("*.sql"): + for path in self._all_files(connection_name): try: text = path.read_text(encoding="utf-8") except OSError: @@ -318,6 +444,7 @@ def delete_entry(self, connection_name: str, timestamp: str) -> bool: entry = _parse_entry( text, fallback_connection=connection_name, + fallback_database="", fallback_timestamp=_filename_to_timestamp(path.stem.rsplit("_", 1)[0]), ) if entry is None: @@ -332,18 +459,25 @@ def delete_entry(self, connection_name: str, timestamp: str) -> bool: def clear_for_connection(self, connection_name: str) -> int: self._maybe_migrate() - conn_dir = self._connection_dir(connection_name) - if not conn_dir.is_dir(): - return 0 + files = self._all_files(connection_name) count = 0 - for path in conn_dir.glob("*.sql"): + for path in files: try: path.unlink() count += 1 except OSError: pass - try: - conn_dir.rmdir() - except OSError: - pass + # Drop now-empty database subdirs and the connection dir itself. + conn_dir = self._connection_dir(connection_name) + if conn_dir.is_dir(): + for child in conn_dir.iterdir(): + if child.is_dir(): + try: + child.rmdir() + except OSError: + pass + try: + conn_dir.rmdir() + except OSError: + pass return count diff --git a/tests/unit/test_history_store_file_backed.py b/tests/unit/test_history_store_file_backed.py index 51aec507..aa76726f 100644 --- a/tests/unit/test_history_store_file_backed.py +++ b/tests/unit/test_history_store_file_backed.py @@ -10,6 +10,7 @@ from sqlit.domains.query.store.history import ( HistoryStore, _connection_dir_name, + _database_dir_name, ) @@ -18,9 +19,20 @@ def store(tmp_path: Path) -> HistoryStore: return HistoryStore(base_dir=tmp_path / "queries") -def _read_file_text(store: HistoryStore, connection: str) -> list[str]: +def _all_files_for(store: HistoryStore, connection: str) -> list[Path]: + """List every .sql file for a connection across all db subdirs.""" conn_dir = store.base_dir / _connection_dir_name(connection) - return [p.read_text(encoding="utf-8") for p in sorted(conn_dir.glob("*.sql"))] + if not conn_dir.is_dir(): + return [] + files = [p for p in conn_dir.glob("*.sql") if p.is_file()] + for child in conn_dir.iterdir(): + if child.is_dir(): + files.extend(p for p in child.glob("*.sql") if p.is_file()) + return sorted(files) + + +def _read_file_text(store: HistoryStore, connection: str) -> list[str]: + return [p.read_text(encoding="utf-8") for p in _all_files_for(store, connection)] class TestRoundTrip: @@ -60,7 +72,6 @@ class TestDedup: def test_same_query_updates_in_place(self, store: HistoryStore) -> None: store.save_query("c", "SELECT 1") first_ts = store.load_for_connection("c")[0].timestamp - # Force a different timestamp on the second save. import time time.sleep(0.01) store.save_query("c", "SELECT 1") @@ -79,20 +90,45 @@ def test_whitespace_variants_dedup(self, store: HistoryStore) -> None: store.save_query("c", " SELECT 1 ") assert len(store.load_for_connection("c")) == 1 + def test_dedup_across_db_subdirs(self, store: HistoryStore) -> None: + """Same query on different DBs of the same connection — last write wins.""" + store.save_query("c", "SELECT 1", database="db_a") + store.save_query("c", "SELECT 1", database="db_b") + # Single file total (the second save deletes the first). + assert len(_all_files_for(store, "c")) == 1 + entries = store.load_for_connection("c") + assert len(entries) == 1 + assert entries[0].database == "db_b" + + +class TestPathLayout: + def test_database_becomes_subdir(self, store: HistoryStore) -> None: + store.save_query("postgres-prod", "SELECT 1", database="myapp") + conn_dir = store.base_dir / _connection_dir_name("postgres-prod") + db_dir = conn_dir / _database_dir_name("myapp") + assert db_dir.is_dir() + assert any(db_dir.glob("*.sql")) + # Nothing directly under the connection dir. + assert not any(p.is_file() for p in conn_dir.glob("*.sql")) + + def test_no_database_keeps_files_flat(self, store: HistoryStore) -> None: + store.save_query("local-sqlite", "SELECT 1") + conn_dir = store.base_dir / _connection_dir_name("local-sqlite") + assert any(conn_dir.glob("*.sql")) + # No subdirs created when database is empty. + assert not any(child.is_dir() for child in conn_dir.iterdir()) + class TestHeaderFormat: - def test_header_contains_expected_fields(self, store: HistoryStore) -> None: + def test_header_carries_only_marker_and_connection(self, store: HistoryStore) -> None: store.save_query("postgres-prod", "SELECT 1", database="myapp") text = _read_file_text(store, "postgres-prod")[0] assert "-- sqlit:history" in text assert "-- connection: postgres-prod" in text - assert "-- database: myapp" in text - assert "-- ran:" in text - - def test_database_omitted_when_empty(self, store: HistoryStore) -> None: - store.save_query("c", "SELECT 1") - text = _read_file_text(store, "c")[0] + # database is now structural (in the path), not in the header. assert "-- database:" not in text + # timestamp is in the filename, not the header. + assert "-- ran:" not in text def test_query_body_preserved_verbatim(self, store: HistoryStore) -> None: body = "-- user's own comment\nSELECT 1\nFROM users" @@ -110,7 +146,6 @@ def test_slashes_are_sanitized(self) -> None: def test_two_similar_names_get_distinct_dirs(self) -> None: a = _connection_dir_name("a/b") b = _connection_dir_name("a_b") - # Sanitization alone would collide; hash suffix saves us. assert a != b def test_unicode_name_is_handled(self, store: HistoryStore) -> None: @@ -126,10 +161,8 @@ def test_filenames_sort_chronologically(self, store: HistoryStore) -> None: for i in range(5): store.save_query("c", f"SELECT {i}") time.sleep(0.005) - conn_dir = store.base_dir / _connection_dir_name("c") - names = sorted(p.name for p in conn_dir.glob("*.sql")) - # Last (lex-max) name must be the most recent (SELECT 4). - last_text = (conn_dir / names[-1]).read_text(encoding="utf-8") + names = sorted(p.name for p in _all_files_for(store, "c")) + last_text = next(p for p in _all_files_for(store, "c") if p.name == names[-1]).read_text() assert "SELECT 4" in last_text @@ -145,23 +178,30 @@ def test_delete_entry_missing_returns_false(self, store: HistoryStore) -> None: def test_clear_for_connection(self, store: HistoryStore) -> None: store.save_query("c", "SELECT 1") - store.save_query("c", "SELECT 2") + store.save_query("c", "SELECT 2", database="db_a") assert store.clear_for_connection("c") == 2 assert store.load_for_connection("c") == [] + def test_clear_drops_empty_db_subdirs(self, store: HistoryStore) -> None: + store.save_query("c", "SELECT 1", database="db_a") + store.clear_for_connection("c") + conn_dir = store.base_dir / _connection_dir_name("c") + assert not conn_dir.exists() + class TestEviction: - def test_max_entries_enforced( + def test_max_entries_enforced_across_databases( self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: + """Eviction is per-connection, summing across all database subdirs.""" monkeypatch.setattr(HistoryStore, "MAX_ENTRIES_PER_CONNECTION", 3) s = HistoryStore(base_dir=tmp_path / "queries") import time + # Alternate databases — oldest two should still be the ones evicted. for i in range(5): - s.save_query("c", f"SELECT {i}") + s.save_query("c", f"SELECT {i}", database="db_a" if i % 2 else "db_b") time.sleep(0.005) entries = s.load_for_connection("c") - # Oldest two (SELECT 0, SELECT 1) should have been evicted. assert len(entries) == 3 assert {e.query for e in entries} == {"SELECT 2", "SELECT 3", "SELECT 4"} @@ -193,14 +233,18 @@ def test_migrates_json_on_first_use(self, tmp_path: Path) -> None: entries = store.load_for_connection("old") assert [e.query for e in entries] == ["SELECT 2", "SELECT 1"] assert entries[0].database == "myapp" + # Database-bearing entry now lives in a db subdir. + old_dir = config / "queries" / _connection_dir_name("old") + myapp_dir = old_dir / _database_dir_name("myapp") + assert any(myapp_dir.glob("*.sql")) + # No-database entry stays flat. + assert any(p.is_file() for p in old_dir.glob("*.sql")) - # JSON has been renamed so we don't re-import on next launch. assert not legacy.exists() assert (config / "query_history.json.migrated").exists() def test_migration_handles_missing_file(self, tmp_path: Path) -> None: store = HistoryStore(base_dir=tmp_path / "queries") - # Should not crash on first load. assert store.load_all() == [] def test_migration_handles_malformed_file(self, tmp_path: Path) -> None: @@ -209,15 +253,76 @@ def test_migration_handles_malformed_file(self, tmp_path: Path) -> None: (config / "query_history.json").write_text("not valid json") store = HistoryStore(base_dir=config / "queries") assert store.load_all() == [] - # Malformed file is left alone (not renamed). assert (config / "query_history.json").exists() +class TestV1ToV2LayoutMigration: + """Files written under the v1 layout (flat under conn dir, database + in the header) should be moved into v2 (db subdir, trimmed header).""" + + def test_migrates_v1_files_with_database_to_subdir(self, tmp_path: Path) -> None: + config = tmp_path / "config" + queries = config / "queries" + conn_dir = queries / _connection_dir_name("postgres-prod") + conn_dir.mkdir(parents=True) + v1_text = ( + "-- sqlit:history\n" + "-- connection: postgres-prod\n" + "-- database: myapp\n" + "-- ran: 2025-12-01T10:00:00\n" + "\n" + "SELECT * FROM users\n" + ) + (conn_dir / "2025-12-01T10-00-00_deadbeef.sql").write_text(v1_text) + + store = HistoryStore(base_dir=queries) + entries = store.load_for_connection("postgres-prod") + assert len(entries) == 1 + assert entries[0].database == "myapp" + + # File should now live in the db subdir, not flat. + flat = list(conn_dir.glob("*.sql")) + assert not [p for p in flat if p.is_file()] + db_dir = conn_dir / _database_dir_name("myapp") + assert any(db_dir.glob("*.sql")) + + # Header is trimmed — no ran/database lines. + migrated_text = next(db_dir.glob("*.sql")).read_text(encoding="utf-8") + assert "-- database:" not in migrated_text + assert "-- ran:" not in migrated_text + assert "-- connection: postgres-prod" in migrated_text + + def test_migrates_v1_files_without_database_just_trims_header( + self, tmp_path: Path + ) -> None: + config = tmp_path / "config" + queries = config / "queries" + conn_dir = queries / _connection_dir_name("local-sqlite") + conn_dir.mkdir(parents=True) + v1_text = ( + "-- sqlit:history\n" + "-- connection: local-sqlite\n" + "-- ran: 2025-12-01T10:00:00\n" + "\n" + "SELECT 1\n" + ) + (conn_dir / "2025-12-01T10-00-00_deadbeef.sql").write_text(v1_text) + + store = HistoryStore(base_dir=queries) + entries = store.load_for_connection("local-sqlite") + assert len(entries) == 1 + assert entries[0].database == "" + + # Stays flat under the connection dir; header trimmed. + files = [p for p in conn_dir.glob("*.sql") if p.is_file()] + assert len(files) == 1 + assert "-- ran:" not in files[0].read_text(encoding="utf-8") + + class TestFallbackParsing: def test_file_without_header_uses_filename_timestamp( self, store: HistoryStore ) -> None: - # User dropped a raw .sql file into the connection dir. conn_dir = store.base_dir / _connection_dir_name("c") conn_dir.mkdir(parents=True) (conn_dir / "2026-05-23T14-30-15_deadbeef.sql").write_text( From f4d6ae284f1c42238bf87f81cfd19befe747311b Mon Sep 17 00:00:00 2001 From: Maxteabag <18162810+Maxteabag@users.noreply.github.com> Date: Sun, 24 May 2026 13:30:52 +0200 Subject: [PATCH 8/9] =?UTF-8?q?Drop=20v1=E2=86=92v2=20layout=20migration?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Nobody has v1 files in the wild — that layout only existed in the earlier commits of this PR. Removing the migration code and its tests. The legacy JSON migration is unrelated and stays. --- sqlit/domains/query/store/history.py | 54 +---------------- tests/unit/test_history_store_file_backed.py | 63 -------------------- 2 files changed, 2 insertions(+), 115 deletions(-) diff --git a/sqlit/domains/query/store/history.py b/sqlit/domains/query/store/history.py index 2d47a094..07cfe7ae 100644 --- a/sqlit/domains/query/store/history.py +++ b/sqlit/domains/query/store/history.py @@ -204,13 +204,11 @@ def _entry_dir(self, connection_name: str, database: str) -> Path: return conn_dir def _maybe_migrate(self) -> None: + """One-time migration from the legacy `query_history.json` store. + Runs lazily on the first public-API call.""" if self._migrated: return self._migrated = True - self._migrate_from_legacy_json() - self._migrate_layout_v1_to_v2() - - def _migrate_from_legacy_json(self) -> None: legacy = self._base_dir.parent / "query_history.json" if not legacy.exists(): return @@ -235,54 +233,6 @@ def _migrate_from_legacy_json(self) -> None: except OSError: pass - def _migrate_layout_v1_to_v2(self) -> None: - """Move existing files written under v1 layout - (`/.sql` with `-- database:` in the header) into the - v2 layout (`//.sql`, header trimmed).""" - if not self._base_dir.is_dir(): - return - for conn_dir in self._base_dir.iterdir(): - if not conn_dir.is_dir(): - continue - for path in list(conn_dir.glob("*.sql")): - try: - text = path.read_text(encoding="utf-8") - except OSError: - continue - stem = path.stem - fallback_ts_raw = stem.rsplit("_", 1)[0] if "_" in stem else stem - fallback_ts = _filename_to_timestamp(fallback_ts_raw) - entry = _parse_entry( - text, - fallback_connection=conn_dir.name, - fallback_database="", - fallback_timestamp=fallback_ts, - ) - if entry is None: - continue - # If body matches what we'd write now (no db and no ran/database - # in the header), nothing to migrate for this file. - if not entry.database and not self._header_has_dropped_fields(text): - continue - # Rewrite into the correct location with the trimmed header. - self._write_entry(entry) - if path.exists(): - try: - path.unlink() - except OSError: - pass - - @staticmethod - def _header_has_dropped_fields(text: str) -> bool: - for line in text.splitlines(): - stripped = line.rstrip() - if stripped == "": - return False - m = _HEADER_LINE.match(stripped) - if m and m.group(1).lower() in {"ran", "database"}: - return True - return False - def _write_entry(self, entry: QueryHistoryEntry) -> Path: """Write one entry to disk, replacing any prior file with the same query hash for this (connection, database). Atomic per-file.""" diff --git a/tests/unit/test_history_store_file_backed.py b/tests/unit/test_history_store_file_backed.py index aa76726f..5a58b51a 100644 --- a/tests/unit/test_history_store_file_backed.py +++ b/tests/unit/test_history_store_file_backed.py @@ -256,69 +256,6 @@ def test_migration_handles_malformed_file(self, tmp_path: Path) -> None: assert (config / "query_history.json").exists() -class TestV1ToV2LayoutMigration: - """Files written under the v1 layout (flat under conn dir, database - in the header) should be moved into v2 (db subdir, trimmed header).""" - - def test_migrates_v1_files_with_database_to_subdir(self, tmp_path: Path) -> None: - config = tmp_path / "config" - queries = config / "queries" - conn_dir = queries / _connection_dir_name("postgres-prod") - conn_dir.mkdir(parents=True) - v1_text = ( - "-- sqlit:history\n" - "-- connection: postgres-prod\n" - "-- database: myapp\n" - "-- ran: 2025-12-01T10:00:00\n" - "\n" - "SELECT * FROM users\n" - ) - (conn_dir / "2025-12-01T10-00-00_deadbeef.sql").write_text(v1_text) - - store = HistoryStore(base_dir=queries) - entries = store.load_for_connection("postgres-prod") - assert len(entries) == 1 - assert entries[0].database == "myapp" - - # File should now live in the db subdir, not flat. - flat = list(conn_dir.glob("*.sql")) - assert not [p for p in flat if p.is_file()] - db_dir = conn_dir / _database_dir_name("myapp") - assert any(db_dir.glob("*.sql")) - - # Header is trimmed — no ran/database lines. - migrated_text = next(db_dir.glob("*.sql")).read_text(encoding="utf-8") - assert "-- database:" not in migrated_text - assert "-- ran:" not in migrated_text - assert "-- connection: postgres-prod" in migrated_text - - def test_migrates_v1_files_without_database_just_trims_header( - self, tmp_path: Path - ) -> None: - config = tmp_path / "config" - queries = config / "queries" - conn_dir = queries / _connection_dir_name("local-sqlite") - conn_dir.mkdir(parents=True) - v1_text = ( - "-- sqlit:history\n" - "-- connection: local-sqlite\n" - "-- ran: 2025-12-01T10:00:00\n" - "\n" - "SELECT 1\n" - ) - (conn_dir / "2025-12-01T10-00-00_deadbeef.sql").write_text(v1_text) - - store = HistoryStore(base_dir=queries) - entries = store.load_for_connection("local-sqlite") - assert len(entries) == 1 - assert entries[0].database == "" - - # Stays flat under the connection dir; header trimmed. - files = [p for p in conn_dir.glob("*.sql") if p.is_file()] - assert len(files) == 1 - assert "-- ran:" not in files[0].read_text(encoding="utf-8") - - class TestFallbackParsing: def test_file_without_header_uses_filename_timestamp( self, store: HistoryStore From 6aeb40f317f10375a4dceb2921c849fe0cede2cf Mon Sep 17 00:00:00 2001 From: Maxteabag <18162810+Maxteabag@users.noreply.github.com> Date: Sun, 24 May 2026 18:16:34 +0200 Subject: [PATCH 9/9] Scope query-hash dedup to (connection, database) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous logic deleted any same-hash file across every database subdir on the connection, so running the same query against two databases silently destroyed the older entry. With database as a structural part of the path, the same query against different databases is two legitimately distinct events — dedup should be per (conn, db), matching the path layout. The test that locked in the wrong behavior is replaced with one asserting both entries are preserved, plus a complementary test that re-running the same query against the same database still collapses to a single file. --- sqlit/domains/query/store/history.py | 16 +++++++-------- tests/unit/test_history_store_file_backed.py | 21 ++++++++++++++------ 2 files changed, 23 insertions(+), 14 deletions(-) diff --git a/sqlit/domains/query/store/history.py b/sqlit/domains/query/store/history.py index 07cfe7ae..e3338748 100644 --- a/sqlit/domains/query/store/history.py +++ b/sqlit/domains/query/store/history.py @@ -240,14 +240,14 @@ def _write_entry(self, entry: QueryHistoryEntry) -> Path: self._ensure_dir(target_dir) qhash = _query_hash(entry.query) - # Drop any same-hash file across all (conn, db) pairs so a query - # that moves between databases doesn't leave a stale duplicate. - for existing in self._all_files(entry.connection_name): - if existing.name.endswith(f"_{qhash}.sql"): - try: - existing.unlink() - except OSError: - pass + # Dedup is per (connection, database): running the same query + # against two different databases preserves both as separate + # history entries. + for existing in target_dir.glob(f"*_{qhash}.sql"): + try: + existing.unlink() + except OSError: + pass filename = f"{_timestamp_to_filename(entry.timestamp)}_{qhash}.sql" dest = target_dir / filename diff --git a/tests/unit/test_history_store_file_backed.py b/tests/unit/test_history_store_file_backed.py index 5a58b51a..a716696a 100644 --- a/tests/unit/test_history_store_file_backed.py +++ b/tests/unit/test_history_store_file_backed.py @@ -90,15 +90,24 @@ def test_whitespace_variants_dedup(self, store: HistoryStore) -> None: store.save_query("c", " SELECT 1 ") assert len(store.load_for_connection("c")) == 1 - def test_dedup_across_db_subdirs(self, store: HistoryStore) -> None: - """Same query on different DBs of the same connection — last write wins.""" + def test_same_query_on_different_dbs_is_preserved(self, store: HistoryStore) -> None: + """Same SQL against different databases on one connection is two + legitimately distinct events — dedup is scoped to (conn, db).""" store.save_query("c", "SELECT 1", database="db_a") store.save_query("c", "SELECT 1", database="db_b") - # Single file total (the second save deletes the first). - assert len(_all_files_for(store, "c")) == 1 + assert len(_all_files_for(store, "c")) == 2 entries = store.load_for_connection("c") - assert len(entries) == 1 - assert entries[0].database == "db_b" + assert len(entries) == 2 + assert {e.database for e in entries} == {"db_a", "db_b"} + + def test_dedup_within_same_db_replaces_existing(self, store: HistoryStore) -> None: + """Re-running the same query against the same database still + collapses to one file with the newer timestamp.""" + store.save_query("c", "SELECT 1", database="db_a") + import time + time.sleep(0.01) + store.save_query("c", "SELECT 1", database="db_a") + assert len(_all_files_for(store, "c")) == 1 class TestPathLayout: