fix(storage): reindex FTS on tags/summary; default a stable calendar raw_uid
Two bugs, one file: - Search: reindex_fts now also indexes summary_text and tags_text, and runs on every set_tags call (it previously only fired from finalize_meeting/update_notes, so tag and summary changes never touched the index at all). Promoted from a private helper to a Store trait method so commands.rs can call it after summary.json is written (summary lives only on disk, not through a store write). - Calendar dedup: import_calendar_events used to insert whatever raw_uid the caller passed, including None -- which the (source, raw_uid) unique index explicitly exempts from dedup, so any event without a captured UID duplicated on every re-import forever (the reported 1762 -> 11,764 row bug). Defaults to a deterministic content-derived key (calendar::content_uid) when raw_uid is missing, and adds a regression test that re-importing the same batch repeatedly doesn't grow the table.
This commit is contained in:
+222
-45
@@ -254,6 +254,13 @@ pub trait Store: Send + Sync {
|
||||
async fn get_meeting(&self, id: &MeetingId) -> Result<Meeting, StoreError>;
|
||||
async fn delete_meeting(&self, id: &MeetingId) -> Result<(), StoreError>;
|
||||
async fn update_notes(&self, id: &MeetingId, markdown: &str) -> Result<(), StoreError>;
|
||||
/// (Re)builds this meeting's FTS index row from the current title and
|
||||
/// whatever's on disk/in the DB for transcript/notes/summary/tags (Phase
|
||||
/// 8, FR-SEARCH-1). `finalize_meeting`/`update_notes`/`set_tags` already
|
||||
/// call this themselves; callers that write searchable content some
|
||||
/// other way (e.g. `generate_summary` sealing `summary.json` straight to
|
||||
/// disk) must call it afterward so search doesn't silently miss it.
|
||||
async fn reindex_fts(&self, id: &MeetingId) -> Result<(), StoreError>;
|
||||
/// Set (or create) a speaker's display name; works whether or not the
|
||||
/// meeting has finalized yet (T4.4, FR-SPK-2/5).
|
||||
async fn rename_speaker(
|
||||
@@ -576,49 +583,6 @@ impl SqliteStore {
|
||||
.fetch_all(&self.pool)
|
||||
.await?)
|
||||
}
|
||||
|
||||
/// (Re)builds this meeting's `meeting_fts` row from the current title and
|
||||
/// whatever's on disk for transcript/notes (Phase 8, FR-SEARCH-1).
|
||||
/// `meeting_fts` is a plain (non-`content=`) FTS5 table, so nothing keeps
|
||||
/// it in sync automatically — called from `finalize_meeting`/
|
||||
/// `update_notes` so the index can't drift from what's actually stored.
|
||||
/// Deletes-then-inserts rather than `INSERT OR REPLACE`: FTS5 has no
|
||||
/// unique constraint to conflict on.
|
||||
async fn reindex_fts(&self, id: &MeetingId) -> Result<(), StoreError> {
|
||||
let title: String = sqlx::query_scalar("SELECT title FROM meetings WHERE id = ?")
|
||||
.bind(id)
|
||||
.fetch_one(&self.pool)
|
||||
.await?;
|
||||
|
||||
let transcript_text = read_artifact(&paths::meeting_dir(id).join("transcript.json"))
|
||||
.and_then(|s| serde_json::from_str::<TranscriptFile>(&s).ok())
|
||||
.map(|t| {
|
||||
t.segments
|
||||
.iter()
|
||||
.map(|s| s.text.as_str())
|
||||
.collect::<Vec<_>>()
|
||||
.join(" ")
|
||||
})
|
||||
.unwrap_or_default();
|
||||
|
||||
let notes_text =
|
||||
read_artifact(&paths::meeting_dir(id).join("notes.md")).unwrap_or_default();
|
||||
|
||||
sqlx::query("DELETE FROM meeting_fts WHERE meeting_id = ?")
|
||||
.bind(id)
|
||||
.execute(&self.pool)
|
||||
.await?;
|
||||
sqlx::query(
|
||||
"INSERT INTO meeting_fts (meeting_id, title, transcript_text, notes_text) VALUES (?, ?, ?, ?)",
|
||||
)
|
||||
.bind(id)
|
||||
.bind(&title)
|
||||
.bind(&transcript_text)
|
||||
.bind(¬es_text)
|
||||
.execute(&self.pool)
|
||||
.await?;
|
||||
Ok(())
|
||||
}
|
||||
}
|
||||
|
||||
/// Follows a `label -> merged_into` chain to its canonical label. Capped at 8
|
||||
@@ -963,6 +927,71 @@ impl Store for SqliteStore {
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// `meeting_fts` is a plain (non-`content=`) FTS5 table, so nothing keeps
|
||||
/// it in sync automatically — every write path that changes title,
|
||||
/// transcript, notes, summary, or tags calls this afterward so search
|
||||
/// (FR-SEARCH-1) can't drift from what's actually stored. Deletes-then-
|
||||
/// inserts rather than `INSERT OR REPLACE`: FTS5 has no unique
|
||||
/// constraint to conflict on.
|
||||
async fn reindex_fts(&self, id: &MeetingId) -> Result<(), StoreError> {
|
||||
let title: String = sqlx::query_scalar("SELECT title FROM meetings WHERE id = ?")
|
||||
.bind(id)
|
||||
.fetch_one(&self.pool)
|
||||
.await?;
|
||||
|
||||
let transcript_text = read_artifact(&paths::meeting_dir(id).join("transcript.json"))
|
||||
.and_then(|s| serde_json::from_str::<TranscriptFile>(&s).ok())
|
||||
.map(|t| {
|
||||
t.segments
|
||||
.iter()
|
||||
.map(|s| s.text.as_str())
|
||||
.collect::<Vec<_>>()
|
||||
.join(" ")
|
||||
})
|
||||
.unwrap_or_default();
|
||||
|
||||
let notes_text =
|
||||
read_artifact(&paths::meeting_dir(id).join("notes.md")).unwrap_or_default();
|
||||
|
||||
// `read_artifact` already unseals (T8.8's vault, passthrough when
|
||||
// locked/plaintext), same as the transcript/notes reads above.
|
||||
let summary_text = read_artifact(&paths::meeting_dir(id).join("summary.json"))
|
||||
.and_then(|s| serde_json::from_str::<SummaryFile>(&s).ok())
|
||||
.map(|s| {
|
||||
let mut text = s.summary_md;
|
||||
for decision in &s.decisions {
|
||||
text.push(' ');
|
||||
text.push_str(decision);
|
||||
}
|
||||
for item in &s.action_items {
|
||||
text.push(' ');
|
||||
text.push_str(&item.text);
|
||||
}
|
||||
text
|
||||
})
|
||||
.unwrap_or_default();
|
||||
|
||||
let tags_text = self.tags_for_meeting(id).await?.join(" ");
|
||||
|
||||
sqlx::query("DELETE FROM meeting_fts WHERE meeting_id = ?")
|
||||
.bind(id)
|
||||
.execute(&self.pool)
|
||||
.await?;
|
||||
sqlx::query(
|
||||
"INSERT INTO meeting_fts (meeting_id, title, transcript_text, notes_text, summary_text, tags_text)
|
||||
VALUES (?, ?, ?, ?, ?, ?)",
|
||||
)
|
||||
.bind(id)
|
||||
.bind(&title)
|
||||
.bind(&transcript_text)
|
||||
.bind(¬es_text)
|
||||
.bind(&summary_text)
|
||||
.bind(&tags_text)
|
||||
.execute(&self.pool)
|
||||
.await?;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
async fn rename_speaker(
|
||||
&self,
|
||||
id: &MeetingId,
|
||||
@@ -1201,6 +1230,19 @@ impl Store for SqliteStore {
|
||||
async fn import_calendar_events(&self, events: Vec<ImportedEvent>) -> Result<u32, StoreError> {
|
||||
let mut imported = 0u32;
|
||||
for ImportedEvent { event, attendees } in events {
|
||||
// Bug fix: `raw_uid = NULL` is explicitly exempt from the
|
||||
// (source, raw_uid) unique index below, so any caller that
|
||||
// passes one through duplicated that event on every re-import
|
||||
// forever. `parse_vevents` no longer produces one, but default
|
||||
// here too — defense in depth for any other/future import path.
|
||||
let raw_uid = event.raw_uid.clone().or_else(|| {
|
||||
Some(crate::calendar::content_uid(
|
||||
&event.subject,
|
||||
&event.organizer,
|
||||
event.starts_at,
|
||||
event.ends_at,
|
||||
))
|
||||
});
|
||||
sqlx::query(
|
||||
"INSERT INTO calendar_events (id, source, subject, organizer, starts_at, ends_at, description, raw_uid)
|
||||
VALUES (?, ?, ?, ?, ?, ?, ?, ?)
|
||||
@@ -1218,14 +1260,14 @@ impl Store for SqliteStore {
|
||||
.bind(event.starts_at)
|
||||
.bind(event.ends_at)
|
||||
.bind(&event.description)
|
||||
.bind(&event.raw_uid)
|
||||
.bind(&raw_uid)
|
||||
.execute(&self.pool)
|
||||
.await?;
|
||||
|
||||
// A re-import may have updated an *existing* row rather than
|
||||
// inserting `event.id` — resolve the row that's actually there
|
||||
// before linking attendees to it.
|
||||
let row_id: String = match &event.raw_uid {
|
||||
let row_id: String = match &raw_uid {
|
||||
Some(raw_uid) => {
|
||||
sqlx::query("SELECT id FROM calendar_events WHERE source = ? AND raw_uid = ?")
|
||||
.bind(&event.source)
|
||||
@@ -1308,6 +1350,7 @@ impl Store for SqliteStore {
|
||||
.execute(&self.pool)
|
||||
.await?;
|
||||
}
|
||||
self.reindex_fts(meeting_id).await?;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
@@ -1846,6 +1889,84 @@ mod tests {
|
||||
assert_eq!(meeting.language.as_deref(), Some("fr"));
|
||||
}
|
||||
|
||||
/// End-to-end regression for the search bug: `meeting_fts` originally had
|
||||
/// no summary/tags columns at all and nothing reindexed on those writes,
|
||||
/// so search silently missed anything that wasn't in the title,
|
||||
/// transcript, or notes (FR-SEARCH-1).
|
||||
#[tokio::test]
|
||||
async fn search_finds_hits_via_transcript_notes_summary_and_tags() {
|
||||
let store = SqliteStore::connect_in_memory().await.unwrap();
|
||||
let id = store
|
||||
.create_meeting(NewMeeting {
|
||||
title: "Weekly sync".to_string(),
|
||||
calendar_event_id: None,
|
||||
template_id: None,
|
||||
language: None,
|
||||
})
|
||||
.await
|
||||
.unwrap();
|
||||
store
|
||||
.finalize_meeting(
|
||||
&id,
|
||||
FinalizeMeeting {
|
||||
segments: vec![TranscriptSegment {
|
||||
id: 0,
|
||||
start_ms: 0,
|
||||
end_ms: 1000,
|
||||
speaker: "S1".to_string(),
|
||||
text: "let's discuss the transcriptword rollout".to_string(),
|
||||
confidence: None,
|
||||
interim: false,
|
||||
}],
|
||||
speakers: Vec::new(),
|
||||
duration_secs: 60,
|
||||
recorded: false,
|
||||
language: Some("en".to_string()),
|
||||
backend_used: Some("cpu".to_string()),
|
||||
model_used: Some("small-q5_1".to_string()),
|
||||
},
|
||||
)
|
||||
.await
|
||||
.unwrap();
|
||||
store
|
||||
.update_notes(&id, "action: follow up on notesword")
|
||||
.await
|
||||
.unwrap();
|
||||
store.set_tags(&id, &["tagword".to_string()]).await.unwrap();
|
||||
|
||||
// summary.json isn't written through the store (generate_summary
|
||||
// seals it straight to disk in commands.rs), so reindex_fts must
|
||||
// pick it up when explicitly told to, same as that command does.
|
||||
let summary = SummaryFile {
|
||||
schema: 1,
|
||||
generated_at: 0,
|
||||
provider: "test".to_string(),
|
||||
model: "test".to_string(),
|
||||
summary_md: "summaryword recap".to_string(),
|
||||
decisions: Vec::new(),
|
||||
action_items: Vec::new(),
|
||||
};
|
||||
write_artifact(
|
||||
&paths::meeting_dir(&id).join("summary.json"),
|
||||
serde_json::to_string(&summary).unwrap().as_bytes(),
|
||||
)
|
||||
.unwrap();
|
||||
store.reindex_fts(&id).await.unwrap();
|
||||
|
||||
for (query, source) in [
|
||||
("transcriptword", "transcript"),
|
||||
("notesword", "notes"),
|
||||
("summaryword", "summary"),
|
||||
("tagword", "tags"),
|
||||
] {
|
||||
let hits = store.search(query).await.unwrap();
|
||||
assert!(
|
||||
hits.iter().any(|h| h.id == id),
|
||||
"expected a hit from {source} for query {query:?}, got {hits:?}"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn rename_meeting_updates_the_title() {
|
||||
let store = SqliteStore::connect_in_memory().await.unwrap();
|
||||
@@ -1960,6 +2081,62 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
/// Regression for the calendar duplicate-import bug: re-importing the
|
||||
/// exact same events (same `(source, raw_uid)`) must update the existing
|
||||
/// rows in place, not add new ones -- whether or not the source
|
||||
/// supplied a `raw_uid` at all.
|
||||
#[tokio::test]
|
||||
async fn reimporting_the_same_events_does_not_duplicate_rows() {
|
||||
let store = SqliteStore::connect_in_memory().await.unwrap();
|
||||
let events = || {
|
||||
vec![
|
||||
ImportedEvent {
|
||||
event: CalendarEvent {
|
||||
id: "ev1".to_string(),
|
||||
source: "pst".to_string(),
|
||||
subject: Some("Weekly 1:1".to_string()),
|
||||
organizer: None,
|
||||
starts_at: Some(1_000),
|
||||
ends_at: Some(2_000),
|
||||
description: None,
|
||||
raw_uid: Some("uid-1".to_string()),
|
||||
},
|
||||
attendees: vec![],
|
||||
},
|
||||
// No raw_uid at all -- the case that used to duplicate on
|
||||
// every re-import (exempt from the unique index).
|
||||
ImportedEvent {
|
||||
event: CalendarEvent {
|
||||
id: "ev2".to_string(),
|
||||
source: "pst".to_string(),
|
||||
subject: Some("No UID event".to_string()),
|
||||
organizer: None,
|
||||
starts_at: Some(3_000),
|
||||
ends_at: Some(4_000),
|
||||
description: None,
|
||||
raw_uid: None,
|
||||
},
|
||||
attendees: vec![],
|
||||
},
|
||||
]
|
||||
};
|
||||
store.import_calendar_events(events()).await.unwrap();
|
||||
assert_eq!(
|
||||
store.list_calendar_events(None, None).await.unwrap().len(),
|
||||
2
|
||||
);
|
||||
|
||||
// Re-import the identical batch a few times, as a user retrying an
|
||||
// import would.
|
||||
for _ in 0..3 {
|
||||
store.import_calendar_events(events()).await.unwrap();
|
||||
}
|
||||
assert_eq!(
|
||||
store.list_calendar_events(None, None).await.unwrap().len(),
|
||||
2
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn sync_target_crud_round_trips() {
|
||||
let store = SqliteStore::connect_in_memory().await.unwrap();
|
||||
|
||||
Reference in New Issue
Block a user