diff --git a/src/history/base.rs b/src/history/base.rs index 6b76a2415..683eb5bc7 100644 --- a/src/history/base.rs +++ b/src/history/base.rs @@ -37,6 +37,26 @@ pub enum SearchDirection { Forward, } +/// A typed expected value for [`SearchFilter::more_info_json`] filters. +/// +/// Each variant maps to a JSON scalar type and produces a type-precise SQL comparison +/// using `json_type()` to avoid collisions between JSON booleans, integers, and strings. +/// Arrays and objects are intentionally unsupported. +#[derive(Clone, Debug, PartialEq)] +#[non_exhaustive] +pub enum JsonFilterValue { + /// Match a JSON `null` at the given path. + Null, + /// Match a JSON boolean (`true` or `false`) at the given path. + Bool(bool), + /// Match a JSON integer at the given path. Does not match JSON `true`/`false` or strings. + Integer(i64), + /// Match a JSON floating-point number at the given path. + Real(f64), + /// Match a JSON string at the given path. Does not match numbers or booleans. + Text(String), +} + /// Defines additional filters for querying the [`History`] pub struct SearchFilter { /// Query for the command line content @@ -53,6 +73,19 @@ pub struct SearchFilter { pub exit_successful: Option, /// Filter on the session id pub session: Option, + /// Filter by JSON path in the `more_info` column using SQLite's `json_type()` / + /// `json_extract()`. + /// + /// Each entry is `(json_path, expected_value)`. The [`JsonFilterValue`] variant + /// determines the exact SQL comparison used, preventing collisions between JSON + /// booleans, integers, and strings (e.g. `Bool(true)` will not match integer `1` + /// or string `"true"`). + /// + /// Rows where `more_info` is SQL `NULL` or the JSON path does not exist will not match. + /// Only evaluated by [`crate::SqliteBackedHistory`] typed search methods + /// (`search_with_extra`); the non-typed [`History`] trait methods and + /// [`crate::FileBackedHistory`] ignore this field. + pub more_info_json: Option>, } impl SearchFilter { @@ -88,6 +121,7 @@ impl SearchFilter { cwd_prefix: None, exit_successful: None, session, + more_info_json: None, } } } diff --git a/src/history/mod.rs b/src/history/mod.rs index 9e798862d..281081324 100644 --- a/src/history/mod.rs +++ b/src/history/mod.rs @@ -8,7 +8,8 @@ mod sqlite_backed; pub use sqlite_backed::SqliteBackedHistory; pub use base::{ - CommandLineSearch, History, HistoryNavigationQuery, SearchDirection, SearchFilter, SearchQuery, + CommandLineSearch, History, HistoryNavigationQuery, JsonFilterValue, SearchDirection, + SearchFilter, SearchQuery, }; pub use cursor::HistoryCursor; pub use item::{ diff --git a/src/history/sqlite_backed.rs b/src/history/sqlite_backed.rs index 830f1a60e..5cfb674f8 100644 --- a/src/history/sqlite_backed.rs +++ b/src/history/sqlite_backed.rs @@ -1,14 +1,15 @@ use super::{ - base::{CommandLineSearch, SearchDirection, SearchQuery}, - History, HistoryItem, HistoryItemId, HistorySessionId, + base::{CommandLineSearch, JsonFilterValue, SearchDirection, SearchQuery}, + History, HistoryItem, HistoryItemExtraInfo, HistoryItemId, HistorySessionId, + IgnoreAllExtraInfo, }; use crate::{ result::{ReedlineError, ReedlineErrorVariants}, Result, }; use chrono::{TimeZone, Utc}; -use rusqlite::{named_params, params, Connection, ToSql}; -use std::{path::PathBuf, time::Duration}; +use rusqlite::{named_params, params, Connection, ToSql, TransactionBehavior}; +use std::{fmt::Write, path::PathBuf, time::Duration}; const SQLITE_APPLICATION_ID: i32 = 1151497937; /// A history that stores the values to an SQLite database. @@ -23,7 +24,9 @@ pub struct SqliteBackedHistory { session_timestamp: Option>, } -fn deserialize_history_item(row: &rusqlite::Row) -> rusqlite::Result { +fn deserialize_history_item( + row: &rusqlite::Row, +) -> rusqlite::Result> { let x: Option = row.get("more_info")?; Ok(HistoryItem { id: Some(HistoryItemId::new(row.get("id")?)), @@ -45,7 +48,7 @@ fn deserialize_history_item(row: &rusqlite::Row) -> rusqlite::Result(&x).map_err(|e| { // hack rusqlite::Error::InvalidColumnType( 0, @@ -58,43 +61,71 @@ fn deserialize_history_item(row: &rusqlite::Row) -> rusqlite::Result( + conn: &Connection, + id: HistoryItemId, +) -> Result> { + conn.prepare("select * from history where id = :id") + .map_err(map_sqlite_err)? + .query_row(named_params! { ":id": id.0 }, deserialize_history_item::) + .map_err(map_sqlite_err) +} + +/// Shared implementation of [`SqliteBackedHistory::save_with_extra`], generic over +/// anything that derefs to a [`Connection`] so it can run against either the history's +/// own connection or an in-flight [`rusqlite::Transaction`]. +fn save_with_extra_conn( + conn: &Connection, + mut entry: HistoryItem, +) -> Result> { + let more_info_serialized = entry + .more_info + .as_ref() + .map(serde_json::to_string) + .transpose() + .map_err(map_json_err)?; + let ret: i64 = conn + .prepare( + "insert into history + (id, start_timestamp, command_line, session_id, hostname, cwd, duration_ms, exit_status, more_info) + values (:id, :start_timestamp, :command_line, :session_id, :hostname, :cwd, :duration_ms, :exit_status, :more_info) + on conflict (history.id) do update set + start_timestamp = excluded.start_timestamp, + command_line = excluded.command_line, + session_id = excluded.session_id, + hostname = excluded.hostname, + cwd = excluded.cwd, + duration_ms = excluded.duration_ms, + exit_status = excluded.exit_status, + more_info = excluded.more_info + returning id", + ) + .map_err(map_sqlite_err)? + .query_row( + named_params! { + ":id": entry.id.map(|id| id.0), + ":start_timestamp": entry.start_timestamp.map(|e| e.timestamp_millis()), + ":command_line": entry.command_line, + ":session_id": entry.session_id.map(|e| e.0), + ":hostname": entry.hostname, + ":cwd": entry.cwd, + ":duration_ms": entry.duration.map(|e| e.as_millis() as i64), + ":exit_status": entry.exit_status, + ":more_info": more_info_serialized, + }, + |row| row.get(0), + ) + .map_err(map_sqlite_err)?; + entry.id = Some(HistoryItemId::new(ret)); + Ok(entry) +} + impl History for SqliteBackedHistory { - fn save(&mut self, mut entry: HistoryItem) -> Result { - let ret: i64 = self - .db - .prepare( - "insert into history - (id, start_timestamp, command_line, session_id, hostname, cwd, duration_ms, exit_status, more_info) - values (:id, :start_timestamp, :command_line, :session_id, :hostname, :cwd, :duration_ms, :exit_status, :more_info) - on conflict (history.id) do update set - start_timestamp = excluded.start_timestamp, - command_line = excluded.command_line, - session_id = excluded.session_id, - hostname = excluded.hostname, - cwd = excluded.cwd, - duration_ms = excluded.duration_ms, - exit_status = excluded.exit_status, - more_info = excluded.more_info - returning id", - ) - .map_err(map_sqlite_err)? - .query_row( - named_params! { - ":id": entry.id.map(|id| id.0), - ":start_timestamp": entry.start_timestamp.map(|e| e.timestamp_millis()), - ":command_line": entry.command_line, - ":session_id": entry.session_id.map(|e| e.0), - ":hostname": entry.hostname, - ":cwd": entry.cwd, - ":duration_ms": entry.duration.map(|e| e.as_millis() as i64), - ":exit_status": entry.exit_status, - ":more_info": entry.more_info.as_ref().map(|e| serde_json::to_string(e).unwrap()) - }, - |row| row.get(0), - ) - .map_err(map_sqlite_err)?; - entry.id = Some(HistoryItemId::new(ret)); - Ok(entry) + fn save(&mut self, entry: HistoryItem) -> Result { + self.save_with_extra(entry) } fn load(&self, id: HistoryItemId) -> Result { @@ -102,14 +133,20 @@ impl History for SqliteBackedHistory { .db .prepare("select * from history where id = :id") .map_err(map_sqlite_err)? - .query_row(named_params! { ":id": id.0 }, deserialize_history_item) + .query_row( + named_params! { ":id": id.0 }, + deserialize_history_item::, + ) .map_err(map_sqlite_err)?; Ok(entry) } - fn count(&self, query: SearchQuery) -> Result { + fn count(&self, mut query: SearchQuery) -> Result { + // more_info_json is only evaluated by search_with_extra; ignore it here + query.filter.more_info_json = None; let (query, params) = self.construct_query(&query, "coalesce(count(*), 0)"); - let params_borrow: Vec<(&str, &dyn ToSql)> = params.iter().map(|e| (e.0, &*e.1)).collect(); + let params_borrow: Vec<(&str, &dyn ToSql)> = + params.iter().map(|e| (e.0.as_str(), &*e.1)).collect(); let result: i64 = self .db .prepare(&query) @@ -119,14 +156,20 @@ impl History for SqliteBackedHistory { Ok(result) } - fn search(&self, query: SearchQuery) -> Result> { + fn search(&self, mut query: SearchQuery) -> Result> { + // more_info_json is only evaluated by search_with_extra; ignore it here + query.filter.more_info_json = None; let (query, params) = self.construct_query(&query, "*"); - let params_borrow: Vec<(&str, &dyn ToSql)> = params.iter().map(|e| (e.0, &*e.1)).collect(); + let params_borrow: Vec<(&str, &dyn ToSql)> = + params.iter().map(|e| (e.0.as_str(), &*e.1)).collect(); let results: Vec = self .db .prepare(&query) .unwrap() - .query_map(¶ms_borrow[..], deserialize_history_item) + .query_map( + ¶ms_borrow[..], + deserialize_history_item::, + ) .map_err(map_sqlite_err)? .collect::>>() .map_err(map_sqlite_err)?; @@ -140,8 +183,13 @@ impl History for SqliteBackedHistory { ) -> Result<()> { // in theory this should run in a transaction let item = self.load(id)?; - self.save(updater(item))?; - Ok(()) + // `HistoryItem` here is always `HistoryItem`, which cannot + // carry a meaningful `more_info` value (it serializes to `null` regardless of + // what was loaded). So this type-erased path updates every column except + // `more_info`, leaving whatever a typed caller wrote via `save_with_extra` + // intact. Callers that need to read/modify typed `more_info` should use + // [`Self::update_with_extra`] instead. + self.update_preserving_more_info(updater(item)) } fn clear(&mut self) -> Result<()> { @@ -187,7 +235,14 @@ fn map_sqlite_err(err: rusqlite::Error) -> ReedlineError { ))) } +fn map_json_err(err: serde_json::Error) -> ReedlineError { + ReedlineError(ReedlineErrorVariants::HistoryDatabaseError(format!( + "could not serialize more_info: {err}" + ))) +} + type BoxedNamedParams<'a> = Vec<(&'static str, Box)>; +type OwnedNamedParams<'a> = Vec<(String, Box)>; impl SqliteBackedHistory { /// Creates a new history with an associated history file. @@ -278,7 +333,7 @@ impl SqliteBackedHistory { &self, query: &'a SearchQuery, select_expression: &str, - ) -> (String, BoxedNamedParams<'a>) { + ) -> (String, OwnedNamedParams<'a>) { // TODO: this whole function could be done with less allocs let (is_asc, asc) = match query.direction { SearchDirection::Forward => (true, "asc"), @@ -379,17 +434,792 @@ impl SqliteBackedHistory { Box::new(session_timestamp.timestamp_millis()), )); } - let mut wheres = wheres.join(" and "); - if wheres.is_empty() { - wheres = "true".to_string(); + + // Build WHERE string, appending dynamic json_type/json_extract conditions last. + // Each JsonFilterValue variant generates type-precise SQL so that JSON booleans, + // integers, and strings cannot collide with each other. + // Dynamic parameter names (":json_path_N", ":json_val_N") are stored separately + // since they cannot be &'static str. + let mut where_string = wheres.join(" and "); + let mut json_params: Vec<(String, Box)> = Vec::new(); + if let Some(filters) = &query.filter.more_info_json { + for (i, (path, value)) in filters.iter().enumerate() { + if !where_string.is_empty() { + where_string.push_str(" and "); + } + match value { + JsonFilterValue::Null => { + write!( + where_string, + "json_type(more_info, :json_path_{i}) = 'null'" + ) + .unwrap(); + } + JsonFilterValue::Bool(b) => { + let type_str = if *b { "true" } else { "false" }; + write!( + where_string, + "json_type(more_info, :json_path_{i}) = '{type_str}'" + ) + .unwrap(); + } + JsonFilterValue::Integer(n) => { + write!( + where_string, + "json_type(more_info, :json_path_{i}) = 'integer' \ + AND json_extract(more_info, :json_path_{i}) = :json_val_{i}" + ) + .unwrap(); + json_params.push((format!(":json_val_{i}"), Box::new(*n))); + } + JsonFilterValue::Real(f) => { + write!( + where_string, + "json_type(more_info, :json_path_{i}) = 'real' \ + AND json_extract(more_info, :json_path_{i}) = :json_val_{i}" + ) + .unwrap(); + json_params.push((format!(":json_val_{i}"), Box::new(*f))); + } + JsonFilterValue::Text(s) => { + write!( + where_string, + "json_type(more_info, :json_path_{i}) = 'text' \ + AND json_extract(more_info, :json_path_{i}) = :json_val_{i}" + ) + .unwrap(); + json_params.push((format!(":json_val_{i}"), Box::new(s.clone()))); + } + } + json_params.push((format!(":json_path_{i}"), Box::new(path.clone()))); + } + } + if where_string.is_empty() { + where_string = "true".to_string(); } let query = format!( "SELECT {select_expression} \ FROM history \ - WHERE ({wheres}) \ + WHERE ({where_string}) \ ORDER BY id {asc} \ {limit}" ); - (query, params) + let mut all_params: OwnedNamedParams = params + .into_iter() + .map(|(k, v)| (k.to_string(), v)) + .collect(); + all_params.extend(json_params); + (query, all_params) + } + + /// Save a history item with typed `more_info`. + /// + /// Unlike [`History::save`], this method preserves the full `more_info` type `E` + /// by serializing it to JSON and storing it in the `more_info` column. + /// + /// Note: this method is specific to [`SqliteBackedHistory`]. The [`History`] trait + /// methods use [`IgnoreAllExtraInfo`] and do not roundtrip custom `more_info`. + pub fn save_with_extra( + &mut self, + entry: HistoryItem, + ) -> Result> { + save_with_extra_conn(&self.db, entry) + } + + /// Load a history item by ID with typed `more_info`. + /// + /// Unlike [`History::load`], this method deserializes `more_info` into the concrete + /// type `E` instead of discarding it. + /// + /// Note: this method is specific to [`SqliteBackedHistory`]. The [`History`] trait + /// methods use [`IgnoreAllExtraInfo`] and do not roundtrip custom `more_info`. + pub fn load_with_extra( + &self, + id: HistoryItemId, + ) -> Result> { + load_with_extra_conn(&self.db, id) + } + + /// Update every column of an existing row except `more_info`, leaving it as-is. + /// + /// Used by the type-erased [`History::update`], which cannot express a meaningful + /// change to `more_info` (see its implementation for why). + fn update_preserving_more_info(&mut self, entry: HistoryItem) -> Result<()> { + let changed = self + .db + .prepare( + "update history set + start_timestamp = :start_timestamp, + command_line = :command_line, + session_id = :session_id, + hostname = :hostname, + cwd = :cwd, + duration_ms = :duration_ms, + exit_status = :exit_status + where id = :id", + ) + .map_err(map_sqlite_err)? + .execute(named_params! { + ":id": entry.id.map(|id| id.0), + ":start_timestamp": entry.start_timestamp.map(|e| e.timestamp_millis()), + ":command_line": entry.command_line, + ":session_id": entry.session_id.map(|e| e.0), + ":hostname": entry.hostname, + ":cwd": entry.cwd, + ":duration_ms": entry.duration.map(|e| e.as_millis() as i64), + ":exit_status": entry.exit_status, + }) + .map_err(map_sqlite_err)?; + if changed == 0 { + return Err(ReedlineError(ReedlineErrorVariants::HistoryDatabaseError( + "Could not find item".to_string(), + ))); + } + Ok(()) + } + + /// Update a history item atomically while preserving typed `more_info`. + /// + /// Unlike [`History::update`], this loads and saves through [`Self::load_with_extra`] + /// and [`Self::save_with_extra`], so `updater` can read and change the typed + /// `more_info` and have the change persisted. + /// + /// Note: this method is specific to [`SqliteBackedHistory`]. It is not part of the + /// [`History`] trait and so is unavailable through `&dyn History`. + pub fn update_with_extra( + &mut self, + id: HistoryItemId, + updater: &dyn Fn(HistoryItem) -> HistoryItem, + ) -> Result<()> { + // `Immediate` acquires the write lock up front, so no other connection can + // interleave a write between the load and the save below. + let tx = self + .db + .transaction_with_behavior(TransactionBehavior::Immediate) + .map_err(map_sqlite_err)?; + let item = load_with_extra_conn::(&tx, id)?; + save_with_extra_conn(&tx, updater(item))?; + tx.commit().map_err(map_sqlite_err)?; + Ok(()) + } + + /// Search history items with typed `more_info`. + /// + /// Unlike [`History::search`], this method deserializes `more_info` into the concrete + /// type `E`. It also evaluates [`SearchFilter::more_info_json`] conditions using + /// SQLite's `json_extract()`. + /// + /// Note: this method is specific to [`SqliteBackedHistory`]. The [`History`] trait + /// methods use [`IgnoreAllExtraInfo`] and ignore [`SearchFilter::more_info_json`]. + pub fn search_with_extra( + &self, + query: SearchQuery, + ) -> Result>> { + let (sql, params) = self.construct_query(&query, "*"); + let params_borrow: Vec<(&str, &dyn ToSql)> = + params.iter().map(|e| (e.0.as_str(), &*e.1)).collect(); + self.db + .prepare(&sql) + .map_err(map_sqlite_err)? + .query_map(¶ms_borrow[..], deserialize_history_item::) + .map_err(map_sqlite_err)? + .collect::>>>() + .map_err(map_sqlite_err) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::history::base::{JsonFilterValue, SearchDirection, SearchFilter}; + use serde::{Deserialize, Serialize}; + + #[derive(Clone, Debug, Default, PartialEq, Eq, Serialize, Deserialize)] + struct TestExtra { + meta_command: bool, + tag: String, + count: i64, + } + impl HistoryItemExtraInfo for TestExtra {} + + fn item_with_extra(cmd: &str, extra: TestExtra) -> HistoryItem { + HistoryItem { + id: None, + start_timestamp: None, + command_line: cmd.to_string(), + session_id: None, + hostname: None, + cwd: None, + duration: None, + exit_status: None, + more_info: Some(extra), + } + } + + fn item_no_extra(cmd: &str) -> HistoryItem { + HistoryItem { + id: None, + start_timestamp: None, + command_line: cmd.to_string(), + session_id: None, + hostname: None, + cwd: None, + duration: None, + exit_status: None, + more_info: None, + } + } + + #[cfg(any(feature = "sqlite", feature = "sqlite-dynlib"))] + #[test] + fn save_and_load_with_extra() -> crate::Result<()> { + let mut db = SqliteBackedHistory::in_memory()?; + let item = item_with_extra( + "ls -la", + TestExtra { + meta_command: false, + tag: "test".into(), + ..Default::default() + }, + ); + let saved = db.save_with_extra(item.clone())?; + assert!(saved.id.is_some()); + assert_eq!(saved.more_info, item.more_info); + + let loaded = db.load_with_extra::(saved.id.unwrap())?; + assert_eq!(loaded, saved); + Ok(()) + } + + #[cfg(any(feature = "sqlite", feature = "sqlite-dynlib"))] + #[test] + fn save_with_extra_null_more_info_roundtrips() -> crate::Result<()> { + let mut db = SqliteBackedHistory::in_memory()?; + let item = item_no_extra("pwd"); + let saved = db.save_with_extra(item)?; + assert!(saved.id.is_some()); + + let loaded = db.load_with_extra::(saved.id.unwrap())?; + assert_eq!(loaded.more_info, None); + Ok(()) + } + + #[cfg(any(feature = "sqlite", feature = "sqlite-dynlib"))] + #[test] + fn search_with_extra_more_info_json_matches() -> crate::Result<()> { + let mut db = SqliteBackedHistory::in_memory()?; + + let saved_meta = db.save_with_extra(item_with_extra( + ":help", + TestExtra { + meta_command: true, + tag: "meta".into(), + ..Default::default() + }, + ))?; + let saved_normal = db.save_with_extra(item_with_extra( + "ls", + TestExtra { + meta_command: false, + tag: "normal".into(), + ..Default::default() + }, + ))?; + db.save_with_extra(item_no_extra("pwd"))?; + + let filter = SearchFilter { + more_info_json: Some(vec![( + "$.meta_command".to_string(), + JsonFilterValue::Bool(true), + )]), + ..SearchFilter::anything(None) + }; + let results = db.search_with_extra::(SearchQuery { + filter, + ..SearchQuery::everything(SearchDirection::Forward, None) + })?; + + assert_eq!(results.len(), 1); + assert_eq!(results[0].id, saved_meta.id); + + let filter2 = SearchFilter { + more_info_json: Some(vec![( + "$.meta_command".to_string(), + JsonFilterValue::Bool(false), + )]), + ..SearchFilter::anything(None) + }; + let results2 = db.search_with_extra::(SearchQuery { + filter: filter2, + ..SearchQuery::everything(SearchDirection::Forward, None) + })?; + + // Only "ls" matches (meta_command: false); "pwd" with NULL more_info does not match + assert_eq!(results2.len(), 1); + assert_eq!(results2[0].id, saved_normal.id); + Ok(()) + } + + #[cfg(any(feature = "sqlite", feature = "sqlite-dynlib"))] + #[test] + fn search_with_extra_null_more_info_not_matched() -> crate::Result<()> { + let mut db = SqliteBackedHistory::in_memory()?; + db.save_with_extra(item_no_extra("pwd"))?; + + let filter = SearchFilter { + more_info_json: Some(vec![( + "$.meta_command".to_string(), + JsonFilterValue::Bool(true), + )]), + ..SearchFilter::anything(None) + }; + let results = db.search_with_extra::(SearchQuery { + filter, + ..SearchQuery::everything(SearchDirection::Forward, None) + })?; + + assert_eq!(results, vec![]); + Ok(()) + } + + /// Verify that Bool, Integer, and Text filters do not collide with each other + /// even when the raw JSON bytes look similar (e.g. `true` vs `1` vs `"1"`). + #[cfg(any(feature = "sqlite", feature = "sqlite-dynlib"))] + #[test] + fn json_filter_value_no_type_collisions() -> crate::Result<()> { + let mut db = SqliteBackedHistory::in_memory()?; + + // Row A: meta_command=true (JSON boolean), count=1 (JSON integer), tag="1" (JSON string) + db.save_with_extra(item_with_extra( + "cmd_a", + TestExtra { + meta_command: true, + tag: "1".into(), + count: 1, + }, + ))?; + // Row B: meta_command=false, count=0, tag="true" + db.save_with_extra(item_with_extra( + "cmd_b", + TestExtra { + meta_command: false, + tag: "true".into(), + count: 0, + }, + ))?; + + let search = |val: JsonFilterValue| -> crate::Result { + let filter = SearchFilter { + more_info_json: Some(vec![("$.meta_command".to_string(), val)]), + ..SearchFilter::anything(None) + }; + Ok(db + .search_with_extra::(SearchQuery { + filter, + ..SearchQuery::everything(SearchDirection::Forward, None) + })? + .len()) + }; + + // Bool(true) must match only row A (meta_command: true), not integer 1 or string "1" + assert_eq!(search(JsonFilterValue::Bool(true))?, 1); + // Bool(false) must match only row B (meta_command: false) + assert_eq!(search(JsonFilterValue::Bool(false))?, 1); + // Integer(1) must not match the boolean true in meta_command + assert_eq!( + search(JsonFilterValue::Integer(1))?, + 0, + "Integer(1) must not match JSON boolean true" + ); + // Text("true") must not match the boolean true in meta_command + assert_eq!( + search(JsonFilterValue::Text("true".into()))?, + 0, + r#"Text("true") must not match JSON boolean true"# + ); + + let search_count = |val: JsonFilterValue| -> crate::Result { + let filter = SearchFilter { + more_info_json: Some(vec![("$.count".to_string(), val)]), + ..SearchFilter::anything(None) + }; + Ok(db + .search_with_extra::(SearchQuery { + filter, + ..SearchQuery::everything(SearchDirection::Forward, None) + })? + .len()) + }; + + // Integer(1) must match row A (count: 1) but not row B (count: 0) + assert_eq!(search_count(JsonFilterValue::Integer(1))?, 1); + // Bool(true) must not match integer 1 in count + assert_eq!( + search_count(JsonFilterValue::Bool(true))?, + 0, + "Bool(true) must not match JSON integer 1" + ); + // Text("1") must not match JSON integer 1 + assert_eq!( + search_count(JsonFilterValue::Text("1".into()))?, + 0, + r#"Text("1") must not match JSON integer 1"# + ); + + let search_tag = |val: JsonFilterValue| -> crate::Result { + let filter = SearchFilter { + more_info_json: Some(vec![("$.tag".to_string(), val)]), + ..SearchFilter::anything(None) + }; + Ok(db + .search_with_extra::(SearchQuery { + filter, + ..SearchQuery::everything(SearchDirection::Forward, None) + })? + .len()) + }; + + // Text("1") must match row A (tag: "1") but not row B (tag: "true") + assert_eq!(search_tag(JsonFilterValue::Text("1".into()))?, 1); + // Text("true") must match row B (tag: "true") but not row A + assert_eq!(search_tag(JsonFilterValue::Text("true".into()))?, 1); + // Bool(true) must not match the string "true" in tag + assert_eq!( + search_tag(JsonFilterValue::Bool(true))?, + 0, + r#"Bool(true) must not match JSON string "true""# + ); + // Integer(1) must not match the string "1" in tag + assert_eq!( + search_tag(JsonFilterValue::Integer(1))?, + 0, + r#"Integer(1) must not match JSON string "1""# + ); + + Ok(()) + } + + /// Verify that Real(f) does not collide with Integer values stored at the same path. + #[cfg(any(feature = "sqlite", feature = "sqlite-dynlib"))] + #[test] + fn json_filter_value_real_vs_integer() -> crate::Result<()> { + // Two structs so we can store the same path ($.score) as either real or integer. + #[derive(Clone, Debug, Default, PartialEq, Serialize, Deserialize)] + struct WithRealScore { + score: f64, + } + impl HistoryItemExtraInfo for WithRealScore {} + + #[derive(Clone, Debug, Default, PartialEq, Serialize, Deserialize)] + struct WithIntScore { + score: i64, + } + impl HistoryItemExtraInfo for WithIntScore {} + + let mut db = SqliteBackedHistory::in_memory()?; + let make_real = |cmd: &str, score: f64| HistoryItem { + id: None, + start_timestamp: None, + command_line: cmd.to_string(), + session_id: None, + hostname: None, + cwd: None, + duration: None, + exit_status: None, + more_info: Some(WithRealScore { score }), + }; + let make_int = |cmd: &str, score: i64| HistoryItem { + id: None, + start_timestamp: None, + command_line: cmd.to_string(), + session_id: None, + hostname: None, + cwd: None, + duration: None, + exit_status: None, + more_info: Some(WithIntScore { score }), + }; + // Row A: $.score = 1.5 (JSON real) + db.save_with_extra(make_real("real_a", 1.5))?; + // Row B: $.score = 1.0 (JSON real — serde_json always emits decimal point for f64) + db.save_with_extra(make_real("real_b", 1.0))?; + // Row C: $.score = 1 (JSON integer — stored via i64 struct) + db.save_with_extra(make_int("int_c", 1))?; + + let search_score = |val: JsonFilterValue| -> crate::Result> { + let filter = SearchFilter { + more_info_json: Some(vec![("$.score".to_string(), val)]), + ..SearchFilter::anything(None) + }; + Ok(db + .search_with_extra::(SearchQuery { + filter, + ..SearchQuery::everything(SearchDirection::Forward, None) + })? + .into_iter() + .map(|h| h.command_line) + .collect()) + }; + + // Real(1.5) matches only real_a + assert_eq!(search_score(JsonFilterValue::Real(1.5))?, vec!["real_a"]); + // Real(1.0) matches real_b but NOT int_c (json_type for int_c is 'integer', not 'real') + assert_eq!(search_score(JsonFilterValue::Real(1.0))?, vec!["real_b"]); + // Integer(1) matches int_c but NOT real_a or real_b + assert_eq!( + search_score(JsonFilterValue::Integer(1))?, + vec!["int_c"], + "Integer(1) must match JSON integer 1 but not JSON reals" + ); + + Ok(()) + } + + /// Verify that Null matches JSON null at a path but not SQL-NULL more_info + /// or a path that is simply absent from the JSON object. + #[cfg(any(feature = "sqlite", feature = "sqlite-dynlib"))] + #[test] + fn json_filter_value_null_vs_missing_path() -> crate::Result<()> { + #[derive(Clone, Debug, Default, PartialEq, Serialize, Deserialize)] + struct WithOptVal { + val: Option, + } + impl HistoryItemExtraInfo for WithOptVal {} + + // A separate struct without a `val` field to produce JSON that omits $.val entirely. + #[derive(Clone, Debug, Default, PartialEq, Serialize, Deserialize)] + struct WithoutVal { + other: String, + } + impl HistoryItemExtraInfo for WithoutVal {} + + let mut db = SqliteBackedHistory::in_memory()?; + let make_opt = |cmd: &str, val: Option| HistoryItem { + id: None, + start_timestamp: None, + command_line: cmd.to_string(), + session_id: None, + hostname: None, + cwd: None, + duration: None, + exit_status: None, + more_info: Some(WithOptVal { val }), + }; + // Row A: val=null in JSON (serde serializes None as JSON null) + db.save_with_extra(make_opt("null_row", None))?; + // Row B: val=42 in JSON + db.save_with_extra(make_opt("int_row", Some(42)))?; + // Row C: SQL NULL more_info — $.val is absent because more_info IS NULL + db.save_with_extra::(HistoryItem { + id: None, + start_timestamp: None, + command_line: "sql_null_row".to_string(), + session_id: None, + hostname: None, + cwd: None, + duration: None, + exit_status: None, + more_info: None, + })?; + // Row D: non-null JSON that simply has no `val` key — $.val path is missing + db.save_with_extra(HistoryItem { + id: None, + start_timestamp: None, + command_line: "missing_path_row".to_string(), + session_id: None, + hostname: None, + cwd: None, + duration: None, + exit_status: None, + more_info: Some(WithoutVal { other: "x".into() }), + })?; + + let search_val = |val: JsonFilterValue| -> crate::Result> { + let filter = SearchFilter { + more_info_json: Some(vec![("$.val".to_string(), val)]), + ..SearchFilter::anything(None) + }; + Ok(db + .search_with_extra::(SearchQuery { + filter, + ..SearchQuery::everything(SearchDirection::Forward, None) + })? + .into_iter() + .map(|h| h.command_line) + .collect()) + }; + + // Null matches row A (json_type = 'null') only; rows B, C, and D are excluded + assert_eq!(search_val(JsonFilterValue::Null)?, vec!["null_row"]); + // Integer(42) matches row B only + assert_eq!(search_val(JsonFilterValue::Integer(42))?, vec!["int_row"]); + + Ok(()) + } + + #[cfg(any(feature = "sqlite", feature = "sqlite-dynlib"))] + #[test] + fn typed_and_untyped_interop() -> crate::Result<()> { + let mut db = SqliteBackedHistory::in_memory()?; + let item = item_with_extra( + ":cd /tmp", + TestExtra { + meta_command: true, + ..Default::default() + }, + ); + let saved = db.save_with_extra(item)?; + + // Load as untyped (IgnoreAllExtraInfo): more_info column is non-NULL but deserialized + // as Some(IgnoreAllExtraInfo) since IgnoreAllExtraInfo accepts any JSON value + let untyped = db.load(saved.id.unwrap())?; + assert_eq!(untyped.command_line, ":cd /tmp"); + assert_eq!(untyped.more_info, Some(IgnoreAllExtraInfo)); + Ok(()) + } + + #[cfg(any(feature = "sqlite", feature = "sqlite-dynlib"))] + #[test] + fn trait_update_preserves_typed_more_info() -> crate::Result<()> { + let mut db = SqliteBackedHistory::in_memory()?; + let saved = db.save_with_extra(item_with_extra( + "cmd", + TestExtra { + meta_command: true, + tag: "keep-me".into(), + count: 7, + }, + ))?; + let id = saved.id.unwrap(); + + // Go through the type-erased `History::update`, e.g. as + // `Reedline::update_last_command_context` does for post-command bookkeeping. + History::update(&mut db, id, &|mut e| { + e.exit_status = Some(0); + e + })?; + + let reloaded = db.load_with_extra::(id)?; + assert_eq!(reloaded.exit_status, Some(0)); + assert_eq!( + reloaded.more_info, + Some(TestExtra { + meta_command: true, + tag: "keep-me".into(), + count: 7, + }) + ); + Ok(()) + } + + #[cfg(any(feature = "sqlite", feature = "sqlite-dynlib"))] + #[test] + fn trait_update_missing_id_errors() { + let mut db = SqliteBackedHistory::in_memory().unwrap(); + let result = History::update(&mut db, HistoryItemId::new(999), &|e| e); + assert!(result.is_err()); + } + + #[cfg(any(feature = "sqlite", feature = "sqlite-dynlib"))] + #[test] + fn update_with_extra_modifies_typed_more_info() -> crate::Result<()> { + let mut db = SqliteBackedHistory::in_memory()?; + let saved = db.save_with_extra(item_with_extra( + "cmd", + TestExtra { + meta_command: false, + tag: "before".into(), + count: 1, + }, + ))?; + let id = saved.id.unwrap(); + + db.update_with_extra::(id, &|mut e| { + if let Some(extra) = e.more_info.as_mut() { + extra.tag = "after".into(); + extra.count += 1; + } + e + })?; + + let reloaded = db.load_with_extra::(id)?; + assert_eq!( + reloaded.more_info, + Some(TestExtra { + meta_command: false, + tag: "after".into(), + count: 2, + }) + ); + Ok(()) + } + + #[test] + fn update_with_extra_blocks_concurrent_writer() -> crate::Result<()> { + use std::cell::{Cell, RefCell}; + use tempfile::tempdir; + + let tmp = tempdir().unwrap(); + let histfile = tmp.path().join("history.sqlite3"); + + let mut writer = SqliteBackedHistory::with_file(histfile.clone(), None, None)?; + let saved = writer.save_with_extra(item_with_extra( + "cmd", + TestExtra { + meta_command: false, + tag: "before".into(), + count: 1, + }, + ))?; + let id = saved.id.unwrap(); + + // A second connection to the same file. Its busy timeout is set to 0 so a + // lock conflict surfaces immediately as an error instead of blocking for + // rusqlite's default 5-second busy handler, keeping this test fast. + let other = SqliteBackedHistory::with_file(histfile, None, None)?; + other + .db + .busy_timeout(Duration::from_millis(0)) + .map_err(map_sqlite_err)?; + let other = RefCell::new(other); + + let concurrent_write_blocked = Cell::new(false); + writer.update_with_extra::(id, &|mut e| { + // Runs while `writer`'s immediate transaction still holds the write + // lock. Without it, this competing write would succeed. + let result = other.borrow_mut().save_with_extra(item_with_extra( + "competing", + TestExtra { + meta_command: false, + tag: "concurrent".into(), + count: 0, + }, + )); + let is_locked_error = + matches!(&result, Err(e) if e.to_string().contains("database is locked")); + concurrent_write_blocked.set(is_locked_error); + + if let Some(extra) = e.more_info.as_mut() { + extra.tag = "after".into(); + extra.count += 1; + } + e + })?; + + assert!( + concurrent_write_blocked.get(), + "expected the competing writer to fail with a database-locked error while update_with_extra's immediate transaction was held" + ); + + let reloaded = writer.load_with_extra::(id)?; + assert_eq!( + reloaded.more_info, + Some(TestExtra { + meta_command: false, + tag: "after".into(), + count: 2, + }) + ); + Ok(()) } } diff --git a/src/lib.rs b/src/lib.rs index 997b56393..dbdfc1bc7 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -252,8 +252,8 @@ mod history; pub use history::SqliteBackedHistory; pub use history::{ CommandLineSearch, FileBackedHistory, History, HistoryItem, HistoryItemExtraInfo, - HistoryItemId, HistoryNavigationQuery, HistorySessionId, IgnoreAllExtraInfo, SearchDirection, - SearchFilter, SearchQuery, HISTORY_SIZE, + HistoryItemId, HistoryNavigationQuery, HistorySessionId, IgnoreAllExtraInfo, JsonFilterValue, + SearchDirection, SearchFilter, SearchQuery, HISTORY_SIZE, }; mod prompt;