Skip to content

Commit 3e27ef1

Browse files
github-actions[bot]claude
authored andcommitted
refactor(sqlite): harden FTS5 fulltext path
Address review findings on the FTS5 implementation: - Guard createFulltextIndex against empty attributes (would emit invalid CREATE VIRTUAL TABLE SQL). - Cache findFulltextTableForAttribute results so a single search query does one sqlite_master scan and one PRAGMA pass per attribute instead of N+1 per condition. Cache invalidated on FTS create/drop. - Wrap dropFulltextIndexById trigger/table drops in a transaction so a partial failure can't leave orphaned triggers writing to a missing FTS5 table. - Extract FTS_TABLE_SUFFIX and FTS_TRIGGER_{INSERT,DELETE,UPDATE} constants in place of the magic strings duplicated across create, drop, and lookup paths. - Extract getFulltextTablePrefix() so the LIKE-prefix construction isn't duplicated between dropFulltextIndexById and findFulltextTableForAttribute. Filters the collection name as defense-in-depth. - Escape the trailing _fts in the LIKE pattern via :_suffix so the underscore can't act as a single-char wildcard against unrelated tables (parity with the existing _prefix escape comment in getSizeOfCollection). - Extract buildSearchLikeFallback() to remove the duplicated LIKE fallback in getSQLCondition (no-collection-context vs no-FTS-match). - Close the cursor on the existence-check fetch in createFulltextIndex so subsequent DDL doesn't trip "database table is locked". Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
1 parent 6508c9d commit 3e27ef1

1 file changed

Lines changed: 116 additions & 37 deletions

File tree

src/Database/Adapter/SQLite.php

Lines changed: 116 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,28 @@ class SQLite extends MariaDB
4545
private const MARIADB_MEDIUMTEXT_BYTES = '16777215';
4646
private const MARIADB_LONGTEXT_BYTES = '4294967295';
4747

48+
/** Suffix appended to every FTS5 virtual table name created by this adapter. */
49+
private const FTS_TABLE_SUFFIX = '_fts';
50+
51+
/** AFTER INSERT trigger suffix on the parent collection. */
52+
private const FTS_TRIGGER_INSERT = 'ai';
53+
54+
/** AFTER DELETE trigger suffix on the parent collection. */
55+
private const FTS_TRIGGER_DELETE = 'ad';
56+
57+
/** AFTER UPDATE trigger suffix on the parent collection. */
58+
private const FTS_TRIGGER_UPDATE = 'au';
59+
60+
/**
61+
* Memo of FTS5 table resolution keyed by `<collection>::<attribute>`,
62+
* value is the matching FTS5 table name or null when no match exists.
63+
* Avoids re-scanning sqlite_master + PRAGMA table_info on every search
64+
* condition. Cleared whenever an FTS5 table is created or dropped.
65+
*
66+
* @var array<string, ?string>
67+
*/
68+
private array $ftsTableCache = [];
69+
4870
/**
4971
* When enabled, the adapter reports MariaDB-shaped column metadata,
5072
* advertises MariaDB-only capabilities (upserts, attribute resizing,
@@ -615,6 +637,10 @@ public function createIndex(string $collection, string $id, string $type, array
615637
*/
616638
protected function createFulltextIndex(string $collection, string $id, array $attributes): bool
617639
{
640+
if (empty($attributes)) {
641+
throw new DatabaseException('Fulltext index requires at least one attribute');
642+
}
643+
618644
$attributes = \array_map(fn (string $a) => $this->getInternalKeyForAttribute($a), $attributes);
619645
$ftsTable = $this->getFulltextTableName($collection, $attributes);
620646
$parentTable = "{$this->getNamespace()}_{$collection}";
@@ -626,7 +652,9 @@ protected function createFulltextIndex(string $collection, string $id, array $at
626652
");
627653
$stmt->bindValue(':_table', $ftsTable);
628654
$stmt->execute();
629-
if (!empty($stmt->fetch())) {
655+
$exists = !empty($stmt->fetch());
656+
$stmt->closeCursor();
657+
if ($exists) {
630658
return true;
631659
}
632660

@@ -650,22 +678,25 @@ protected function createFulltextIndex(string $collection, string $id, array $at
650678
$createSql = $this->trigger(Database::EVENT_INDEX_CREATE, $createSql);
651679
$this->getPDO()->prepare($createSql)->execute();
652680

681+
$insertSuffix = self::FTS_TRIGGER_INSERT;
653682
$insertTrigger = "
654-
CREATE TRIGGER `{$ftsTable}_ai` AFTER INSERT ON `{$parentTable}` BEGIN
683+
CREATE TRIGGER `{$ftsTable}_{$insertSuffix}` AFTER INSERT ON `{$parentTable}` BEGIN
655684
INSERT INTO `{$ftsTable}` (rowid, {$columnList}) VALUES (NEW.`_id`, {$newColumnList});
656685
END
657686
";
658687
$this->getPDO()->prepare($insertTrigger)->execute();
659688

689+
$deleteSuffix = self::FTS_TRIGGER_DELETE;
660690
$deleteTrigger = "
661-
CREATE TRIGGER `{$ftsTable}_ad` AFTER DELETE ON `{$parentTable}` BEGIN
691+
CREATE TRIGGER `{$ftsTable}_{$deleteSuffix}` AFTER DELETE ON `{$parentTable}` BEGIN
662692
INSERT INTO `{$ftsTable}` (`{$ftsTable}`, rowid, {$columnList}) VALUES ('delete', OLD.`_id`, {$oldColumnList});
663693
END
664694
";
665695
$this->getPDO()->prepare($deleteTrigger)->execute();
666696

697+
$updateSuffix = self::FTS_TRIGGER_UPDATE;
667698
$updateTrigger = "
668-
CREATE TRIGGER `{$ftsTable}_au` AFTER UPDATE ON `{$parentTable}` BEGIN
699+
CREATE TRIGGER `{$ftsTable}_{$updateSuffix}` AFTER UPDATE ON `{$parentTable}` BEGIN
669700
INSERT INTO `{$ftsTable}` (`{$ftsTable}`, rowid, {$columnList}) VALUES ('delete', OLD.`_id`, {$oldColumnList});
670701
INSERT INTO `{$ftsTable}` (rowid, {$columnList}) VALUES (NEW.`_id`, {$newColumnList});
671702
END
@@ -681,6 +712,8 @@ protected function createFulltextIndex(string $collection, string $id, array $at
681712
throw $e;
682713
}
683714

715+
$this->ftsTableCache = [];
716+
684717
return true;
685718
}
686719

@@ -698,7 +731,16 @@ protected function getFulltextTableName(string $collection, array|string $attrib
698731
\sort($attrs);
699732
$key = \implode('_', $attrs);
700733

701-
return "{$this->getNamespace()}_{$this->tenant}_{$collection}_{$key}_fts";
734+
return $this->getFulltextTablePrefix($collection) . $key . self::FTS_TABLE_SUFFIX;
735+
}
736+
737+
/**
738+
* Common prefix for every FTS5 table belonging to `$collection`. Used by
739+
* lookup helpers that scan sqlite_master with a LIKE pattern.
740+
*/
741+
protected function getFulltextTablePrefix(string $collection): string
742+
{
743+
return "{$this->getNamespace()}_{$this->tenant}_{$this->filter($collection)}_";
702744
}
703745

704746
/**
@@ -759,12 +801,12 @@ public function deleteIndex(string $collection, string $id): bool
759801
*/
760802
protected function dropFulltextIndexById(string $collection, string $id): bool
761803
{
762-
$prefix = "{$this->getNamespace()}_{$this->tenant}_{$collection}_";
763804
$stmt = $this->getPDO()->prepare("
764805
SELECT name FROM sqlite_master
765-
WHERE type='table' AND name LIKE :_prefix AND name LIKE '%_fts'
806+
WHERE type='table' AND name LIKE :_prefix AND name LIKE :_suffix
766807
");
767-
$stmt->bindValue(':_prefix', $prefix . '%');
808+
$stmt->bindValue(':_prefix', $this->getFulltextTablePrefix($collection) . '%');
809+
$stmt->bindValue(':_suffix', '%' . self::FTS_TABLE_SUFFIX);
768810
$stmt->execute();
769811
$tables = $stmt->fetchAll(PDO::FETCH_COLUMN);
770812
$stmt->closeCursor();
@@ -773,23 +815,39 @@ protected function dropFulltextIndexById(string $collection, string $id): bool
773815
return false;
774816
}
775817

776-
// The index id isn't encoded in the FTS5 table name, so we can't
777-
// match a single table from the id alone. Probe the index map: the
778-
// caller has to pass the column set when creating, but on delete we
779-
// only get the id. Fall back to dropping every matching FTS5 table
780-
// when only one exists; otherwise leave the regular DROP INDEX
781-
// path to error.
818+
// The index id isn't encoded in the FTS5 table name (the name is
819+
// keyed off the sorted attribute set). When a single FTS5 table
820+
// exists for this collection, treat it as the unambiguous target;
821+
// otherwise refuse and let the caller surface a clearer error.
782822
if (\count($tables) !== 1) {
783823
return false;
784824
}
785825

786826
$ftsTable = $tables[0];
787-
foreach (['ai', 'ad', 'au'] as $suffix) {
788-
$this->getPDO()->prepare("DROP TRIGGER IF EXISTS `{$ftsTable}_{$suffix}`")->execute();
827+
$triggerSuffixes = [
828+
self::FTS_TRIGGER_INSERT,
829+
self::FTS_TRIGGER_DELETE,
830+
self::FTS_TRIGGER_UPDATE,
831+
];
832+
833+
// Atomic teardown: orphaned triggers without their FTS5 table will
834+
// start failing on every parent write, so do not commit a partial
835+
// drop if any step fails midway.
836+
$this->startTransaction();
837+
try {
838+
foreach ($triggerSuffixes as $suffix) {
839+
$this->getPDO()->prepare("DROP TRIGGER IF EXISTS `{$ftsTable}_{$suffix}`")->execute();
840+
}
841+
$sql = "DROP TABLE IF EXISTS `{$ftsTable}`";
842+
$sql = $this->trigger(Database::EVENT_INDEX_DELETE, $sql);
843+
$this->getPDO()->prepare($sql)->execute();
844+
$this->commitTransaction();
845+
} catch (PDOException $e) {
846+
$this->rollbackTransaction();
847+
throw $e;
789848
}
790-
$sql = "DROP TABLE IF EXISTS `{$ftsTable}`";
791-
$sql = $this->trigger(Database::EVENT_INDEX_DELETE, $sql);
792-
$this->getPDO()->prepare($sql)->execute();
849+
850+
$this->ftsTableCache = [];
793851

794852
return true;
795853
}
@@ -2776,25 +2834,20 @@ protected function getSQLCondition(Query $query, array &$binds): string
27762834
}
27772835

27782836
$collection = $this->currentQueryCollection;
2779-
if ($collection === null) {
2780-
// No collection context — fall back to a LIKE scan so the query
2781-
// still returns plausible results instead of erroring out.
2782-
$binds[":{$placeholder}_0"] = '%' . $value . '%';
2783-
$sql = "{$alias}.{$this->quote($attribute)} LIKE :{$placeholder}_0";
2784-
2785-
return $method === Query::TYPE_SEARCH ? $sql : "NOT ({$sql})";
2786-
}
27872837

27882838
// Look the FTS5 table up by attribute rather than computing the
27892839
// name from the attribute alone — multi-column fulltext indexes
27902840
// encode their full sorted attribute set in the table name, so
27912841
// single-attribute searches against them would otherwise miss.
2792-
$ftsTable = $this->findFulltextTableForAttribute($collection, $attribute);
2793-
if ($ftsTable === null) {
2794-
$binds[":{$placeholder}_0"] = '%' . $value . '%';
2795-
$sql = "{$alias}.{$this->quote($attribute)} LIKE :{$placeholder}_0";
2842+
$ftsTable = $collection === null
2843+
? null
2844+
: $this->findFulltextTableForAttribute($collection, $attribute);
27962845

2797-
return $method === Query::TYPE_SEARCH ? $sql : "NOT ({$sql})";
2846+
if ($ftsTable === null) {
2847+
// No collection context or no matching FTS5 table — fall back
2848+
// to a LIKE scan so the query still returns plausible results
2849+
// instead of erroring out.
2850+
return $this->buildSearchLikeFallback($attribute, $value, $alias, $placeholder, $method, $binds);
27982851
}
27992852

28002853
$binds[":{$placeholder}_0"] = $value;
@@ -2804,6 +2857,27 @@ protected function getSQLCondition(Query $query, array &$binds): string
28042857
return $method === Query::TYPE_SEARCH ? $subquery : "NOT ({$subquery})";
28052858
}
28062859

2860+
/**
2861+
* Wrap a SEARCH/NOT_SEARCH query as a LIKE scan when no FTS5 table is
2862+
* available. Extracted so the no-collection-context and no-matching-FTS
2863+
* branches share the same shape.
2864+
*
2865+
* @param array<string,mixed> $binds
2866+
*/
2867+
private function buildSearchLikeFallback(
2868+
string $attribute,
2869+
string $value,
2870+
string $alias,
2871+
string $placeholder,
2872+
string $method,
2873+
array &$binds,
2874+
): string {
2875+
$binds[":{$placeholder}_0"] = '%' . $value . '%';
2876+
$sql = "{$alias}.{$this->quote($attribute)} LIKE :{$placeholder}_0";
2877+
2878+
return $method === Query::TYPE_SEARCH ? $sql : "NOT ({$sql})";
2879+
}
2880+
28072881
/**
28082882
* Find the FTS5 virtual table on `$collection` that covers `$attribute`,
28092883
* or null if none exists. Resolves the multi-column case where the
@@ -2812,14 +2886,19 @@ protected function getSQLCondition(Query $query, array &$binds): string
28122886
*/
28132887
protected function findFulltextTableForAttribute(string $collection, string $attribute): ?string
28142888
{
2815-
$prefix = "{$this->getNamespace()}_{$this->tenant}_{$collection}_";
2889+
$cacheKey = "{$collection}::{$attribute}";
2890+
if (\array_key_exists($cacheKey, $this->ftsTableCache)) {
2891+
return $this->ftsTableCache[$cacheKey];
2892+
}
2893+
28162894
$stmt = $this->getPDO()->prepare("
28172895
SELECT name FROM sqlite_master
28182896
WHERE type='table'
28192897
AND name LIKE :_prefix
2820-
AND name LIKE '%_fts'
2898+
AND name LIKE :_suffix
28212899
");
2822-
$stmt->bindValue(':_prefix', $prefix . '%');
2900+
$stmt->bindValue(':_prefix', $this->getFulltextTablePrefix($collection) . '%');
2901+
$stmt->bindValue(':_suffix', '%' . self::FTS_TABLE_SUFFIX);
28232902
$stmt->execute();
28242903
$tables = $stmt->fetchAll(PDO::FETCH_COLUMN);
28252904
$stmt->closeCursor();
@@ -2831,12 +2910,12 @@ protected function findFulltextTableForAttribute(string $collection, string $att
28312910
$info->closeCursor();
28322911
foreach ($cols as $col) {
28332912
if (($col['name'] ?? null) === $attribute) {
2834-
return $table;
2913+
return $this->ftsTableCache[$cacheKey] = $table;
28352914
}
28362915
}
28372916
}
28382917

2839-
return null;
2918+
return $this->ftsTableCache[$cacheKey] = null;
28402919
}
28412920

28422921
/**

0 commit comments

Comments
 (0)