From 30733a36885602931889da2779a4ed09f7656fe2 Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Wed, 18 May 2016 11:33:58 -0500 Subject: [PATCH] Modify Rows::next to tie its lifetime to the returned Row. This means Rows no longer implements Iterator, but it is no longer possible to misuse it by accessing a stale Row handle. --- src/error.rs | 7 -- src/lib.rs | 162 +++++++++++++------------------------------- src/named_params.rs | 6 +- src/types/mod.rs | 24 ++++--- 4 files changed, 64 insertions(+), 135 deletions(-) diff --git a/src/error.rs b/src/error.rs index f277776..22f79a6 100644 --- a/src/error.rs +++ b/src/error.rs @@ -40,10 +40,6 @@ pub enum Error { /// did not return any. QueryReturnedNoRows, - /// Error when trying to access a `Row` after stepping past it. See the discussion on - /// the `Rows` type for more details. - GetFromStaleRow, - /// Error when the value of a particular column is requested, but the index is out of range /// for the statement. InvalidColumnIndex(c_int), @@ -105,7 +101,6 @@ impl fmt::Display for Error { write!(f, "Execute returned results - did you mean to call query?") } Error::QueryReturnedNoRows => write!(f, "Query returned no rows"), - Error::GetFromStaleRow => write!(f, "Attempted to get a value from a stale row"), Error::InvalidColumnIndex(i) => write!(f, "Invalid column index: {}", i), Error::InvalidColumnName(ref name) => write!(f, "Invalid column name: {}", name), Error::InvalidColumnType => write!(f, "Invalid column type"), @@ -137,7 +132,6 @@ impl error::Error for Error { "execute returned results - did you mean to call query?" } Error::QueryReturnedNoRows => "query returned no rows", - Error::GetFromStaleRow => "attempted to get a value from a stale row", Error::InvalidColumnIndex(_) => "invalid column index", Error::InvalidColumnName(_) => "invalid column name", Error::InvalidColumnType => "invalid column type", @@ -163,7 +157,6 @@ impl error::Error for Error { Error::InvalidParameterName(_) | Error::ExecuteReturnedResults | Error::QueryReturnedNoRows | - Error::GetFromStaleRow | Error::InvalidColumnIndex(_) | Error::InvalidColumnName(_) | Error::InvalidColumnType | diff --git a/src/lib.rs b/src/lib.rs index 8708869..840a60c 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -63,12 +63,12 @@ extern crate lazy_static; use std::default::Default; use std::convert; +use std::marker::PhantomData; use std::mem; use std::ptr; use std::fmt; use std::path::{Path, PathBuf}; -use std::rc::Rc; -use std::cell::{RefCell, Cell}; +use std::cell::RefCell; use std::ffi::{CStr, CString}; use std::result; use std::str; @@ -800,7 +800,11 @@ impl<'conn> Statement<'conn> { } } - /// Execute the prepared statement, returning an iterator over the resulting rows. + /// Execute the prepared statement, returning a handle to the resulting rows. + /// + /// Due to lifetime restricts, the rows handle returned by `query` does not + /// implement the `Iterator` trait. Consider using `query_map` or `query_and_then` + /// instead, which do. /// /// ## Example /// @@ -811,7 +815,7 @@ impl<'conn> Statement<'conn> { /// let mut rows = try!(stmt.query(&[])); /// /// let mut names = Vec::new(); - /// for result_row in rows { + /// while let Some(result_row) = rows.next() { /// let row = try!(result_row); /// names.push(row.get(0)); /// } @@ -831,11 +835,8 @@ impl<'conn> Statement<'conn> { Ok(Rows::new(self)) } - /// Executes the prepared statement and maps a function over the resulting - /// rows. - /// - /// Unlike the iterator produced by `query`, the returned iterator does not expose the possibility - /// for accessing stale rows. + /// Executes the prepared statement and maps a function over the resulting rows, returning + /// an iterator over the mapped function results. /// /// # Failure /// @@ -855,9 +856,6 @@ impl<'conn> Statement<'conn> { /// rows, where the function returns a `Result` with `Error` type implementing /// `std::convert::From` (so errors can be unified). /// - /// Unlike the iterator produced by `query`, the returned iterator does not expose the possibility - /// for accessing stale rows. - /// /// # Failure /// /// Will return `Err` if binding parameters fails. @@ -941,7 +939,8 @@ impl<'stmt, T, F> Iterator for MappedRows<'stmt, F> type Item = Result; fn next(&mut self) -> Option> { - self.rows.next().map(|row_result| row_result.map(|row| (self.map)(&row))) + let map = &mut self.map; + self.rows.next().map(|row_result| row_result.map(|row| (map)(&row))) } } @@ -959,9 +958,10 @@ impl<'stmt, T, E, F> Iterator for AndThenRows<'stmt, F> type Item = result::Result; fn next(&mut self) -> Option { + let map = &mut self.map; self.rows.next().map(|row_result| { row_result.map_err(E::from) - .and_then(|row| (self.map)(&row)) + .and_then(|row| (map)(&row)) }) } } @@ -969,52 +969,19 @@ impl<'stmt, T, E, F> Iterator for AndThenRows<'stmt, F> /// Old name for `Rows`. `SqliteRows` is deprecated. pub type SqliteRows<'stmt> = Rows<'stmt>; -/// An iterator over the resulting rows of a query. -/// -/// ## Warning -/// -/// Strongly consider using `query_map` or `query_and_then` instead of `query`; the former do not -/// suffer from the following problem. -/// -/// Due to the way SQLite returns result rows of a query, it is not safe to attempt to get values -/// from a row after it has become stale (i.e., `next()` has been called again on the `Rows` -/// iterator). For example: -/// -/// ```rust,no_run -/// # use rusqlite::{Connection, Result}; -/// fn bad_function_will_panic(conn: &Connection) -> Result { -/// let mut stmt = try!(conn.prepare("SELECT id FROM my_table")); -/// let mut rows = try!(stmt.query(&[])); -/// -/// let row0 = try!(rows.next().unwrap()); -/// // row 0 is valid for now... -/// -/// let row1 = try!(rows.next().unwrap()); -/// // row 0 is now STALE, and row 1 is valid -/// -/// let my_id = row0.get(0); // WILL PANIC because row 0 is stale -/// Ok(my_id) -/// } -/// ``` -/// -/// Please note that this means some of the methods on `Iterator` are not useful, such as `collect` -/// (which would result in a collection of rows, only the last of which can safely be used) and -/// `min`/`max` (which could return a stale row unless the last row happened to be the min or max, -/// respectively). +/// An handle for the resulting rows of a query. pub struct Rows<'stmt> { stmt: Option<&'stmt Statement<'stmt>>, - current_row: Rc>, } impl<'stmt> Rows<'stmt> { fn new(stmt: &'stmt Statement<'stmt>) -> Rows<'stmt> { Rows { stmt: Some(stmt), - current_row: Rc::new(Cell::new(0)), } } - fn get_expected_row(&mut self) -> Result> { + fn get_expected_row<'a>(&'a mut self) -> Result> { match self.next() { Some(row) => row, None => Err(Error::QueryReturnedNoRows), @@ -1028,21 +995,24 @@ impl<'stmt> Rows<'stmt> { } } } -} -impl<'stmt> Iterator for Rows<'stmt> { - type Item = Result>; - - fn next(&mut self) -> Option>> { + /// Attempt to get the next row from the query. Returns `Some(Ok(Row))` if there + /// is another row, `Some(Err(...))` if there was an error getting the next + /// row, and `None` if all rows have been retrieved. + /// + /// ## Note + /// + /// This interface is not compatible with Rust's `Iterator` trait, because the + /// lifetime of the returned row is tied to the lifetime of `self`. This is a + /// "streaming iterator". For a more natural interface, consider using `query_map` + /// or `query_and_then` instead, which return types that implement `Iterator`. + pub fn next<'a>(&'a mut self) -> Option>> { self.stmt.and_then(|stmt| { match unsafe { ffi::sqlite3_step(stmt.stmt) } { ffi::SQLITE_ROW => { - let current_row = self.current_row.get() + 1; - self.current_row.set(current_row); Some(Ok(Row { stmt: stmt, - current_row: self.current_row.clone(), - row_idx: current_row, + phantom: PhantomData, })) } ffi::SQLITE_DONE => { @@ -1065,46 +1035,22 @@ impl<'stmt> Drop for Rows<'stmt> { } /// Old name for `Row`. `SqliteRow` is deprecated. -pub type SqliteRow<'stmt> = Row<'stmt>; +pub type SqliteRow<'a, 'stmt> = Row<'a, 'stmt>; /// A single result row of a query. -pub struct Row<'stmt> { +pub struct Row<'a, 'stmt> { stmt: &'stmt Statement<'stmt>, - current_row: Rc>, - row_idx: c_int, + phantom: PhantomData<&'a ()>, } -impl<'stmt> Row<'stmt> { +impl<'a, 'stmt> Row<'a, 'stmt> { /// Get the value of a particular column of the result row. /// - /// Note that `Row` can panic at runtime if you use it incorrectly. When you are - /// retrieving the rows of a query, a row becomes stale once you have requested the next row, - /// and the values can no longer be retrieved. In general (when using looping over the rows, - /// for example) this isn't an issue, but it means you cannot do something like this: - /// - /// ```rust,no_run - /// # use rusqlite::{Connection, Result}; - /// fn bad_function_will_panic(conn: &Connection) -> Result { - /// let mut stmt = try!(conn.prepare("SELECT id FROM my_table")); - /// let mut rows = try!(stmt.query(&[])); - /// - /// let row0 = try!(rows.next().unwrap()); - /// // row 0 is value now... - /// - /// let row1 = try!(rows.next().unwrap()); - /// // row 0 is now STALE, and row 1 is valid - /// - /// let my_id = row0.get(0); // WILL PANIC because row 0 is stale - /// Ok(my_id) - /// } - /// ``` - /// /// ## Failure /// /// Panics if the underlying SQLite column type is not a valid type as a source for `T`. /// - /// Panics if `idx` is outside the range of columns in the returned query or if this row - /// is stale. + /// Panics if `idx` is outside the range of columns in the returned query. pub fn get(&self, idx: I) -> T { self.get_checked(idx).unwrap() } @@ -1121,12 +1067,7 @@ impl<'stmt> Row<'stmt> { /// /// Returns an `Error::InvalidColumnName` if `idx` is not a valid column name /// for this row. - /// - /// Returns an `Error::GetFromStaleRow` if this row is stale. pub fn get_checked(&self, idx: I) -> Result { - if self.row_idx != self.current_row.get() { - return Err(Error::GetFromStaleRow); - } unsafe { let idx = try!(idx.idx(self.stmt)); @@ -1317,15 +1258,24 @@ mod test { let mut query = db.prepare("SELECT x FROM foo WHERE x < ? ORDER BY x DESC").unwrap(); { - let rows = query.query(&[&4i32]).unwrap(); - let v: Vec = rows.map(|r| r.unwrap().get(0)).collect(); + let mut rows = query.query(&[&4i32]).unwrap(); + let mut v = Vec::::new(); + + while let Some(row) = rows.next() { + v.push(row.unwrap().get(0)); + } assert_eq!(v, [3i32, 2, 1]); } { - let rows = query.query(&[&3i32]).unwrap(); - let v: Vec = rows.map(|r| r.unwrap().get(0)).collect(); + let mut rows = query.query(&[&3i32]).unwrap(); + let mut v = Vec::::new(); + + while let Some(row) = rows.next() { + v.push(row.unwrap().get(0)); + } + assert_eq!(v, [2i32, 1]); } } @@ -1388,26 +1338,6 @@ mod test { assert!(format!("{}", err).contains("does_not_exist")); } - #[test] - fn test_row_expiration() { - let db = checked_memory_handle(); - db.execute_batch("CREATE TABLE foo(x INTEGER)").unwrap(); - db.execute_batch("INSERT INTO foo(x) VALUES(1)").unwrap(); - db.execute_batch("INSERT INTO foo(x) VALUES(2)").unwrap(); - - let mut stmt = db.prepare("SELECT x FROM foo ORDER BY x").unwrap(); - let mut rows = stmt.query(&[]).unwrap(); - let first = rows.next().unwrap().unwrap(); - let second = rows.next().unwrap().unwrap(); - - assert_eq!(2i32, second.get(0)); - - match first.get_checked::(0).unwrap_err() { - Error::GetFromStaleRow => (), - err => panic!("Unexpected error {}", err), - } - } - #[test] fn test_last_insert_rowid() { let db = checked_memory_handle(); diff --git a/src/named_params.rs b/src/named_params.rs index 3e62610..a6b82bd 100644 --- a/src/named_params.rs +++ b/src/named_params.rs @@ -90,7 +90,7 @@ impl<'conn> Statement<'conn> { unsafe { self.execute_() } } - /// Execute the prepared statement with named parameter(s), returning an iterator over the + /// Execute the prepared statement with named parameter(s), returning a handle for the /// resulting rows. If any parameters that were in the prepared statement are not included in /// `params`, they will continue to use the most-recently bound value from a previous call to /// `query_named`, or `NULL` if they have never been bound. @@ -102,7 +102,7 @@ impl<'conn> Statement<'conn> { /// fn query(conn: &Connection) -> Result<()> { /// let mut stmt = try!(conn.prepare("SELECT * FROM test where name = :name")); /// let mut rows = try!(stmt.query_named(&[(":name", &"one")])); - /// for row in rows { + /// while let Some(row) = rows.next() { /// // ... /// } /// Ok(()) @@ -117,6 +117,8 @@ impl<'conn> Statement<'conn> { Ok(Rows::new(self)) } + // TODO: add query_map_named and query_and_then_named + fn bind_parameters_named(&mut self, params: &[(&str, &ToSql)]) -> Result<()> { for &(name, value) in params { if let Some(i) = try!(self.parameter_index(name)) { diff --git a/src/types/mod.rs b/src/types/mod.rs index 58b4dbd..ba953a1 100644 --- a/src/types/mod.rs +++ b/src/types/mod.rs @@ -353,17 +353,21 @@ mod test { let mut stmt = db.prepare("SELECT t, b FROM foo ORDER BY ROWID ASC").unwrap(); let mut rows = stmt.query(&[]).unwrap(); - let row1 = rows.next().unwrap().unwrap(); - let s1: Option = row1.get(0); - let b1: Option> = row1.get(1); - assert_eq!(s.unwrap(), s1.unwrap()); - assert!(b1.is_none()); + { + let row1 = rows.next().unwrap().unwrap(); + let s1: Option = row1.get(0); + let b1: Option> = row1.get(1); + assert_eq!(s.unwrap(), s1.unwrap()); + assert!(b1.is_none()); + } - let row2 = rows.next().unwrap().unwrap(); - let s2: Option = row2.get(0); - let b2: Option> = row2.get(1); - assert!(s2.is_none()); - assert_eq!(b, b2); + { + let row2 = rows.next().unwrap().unwrap(); + let s2: Option = row2.get(0); + let b2: Option> = row2.get(1); + assert!(s2.is_none()); + assert_eq!(b, b2); + } } #[test]