Skip to content

Commit

Permalink
Make query_row a synonym for query_row_safe.
Browse files Browse the repository at this point in the history 
This is a breaking change for anyone using `query_row`. To update code
that used the old `query_row`, you must now `.unwrap()` the returned
result.
  • Loading branch information
@hydhknn
hydhknn committed May 5, 2015
1 parent 74496cd commit 03be8e0
Show file tree
Hide file tree
Showing 3 changed files with 27 additions and 32 deletions.
47 changes: 21 additions & 26 deletions src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -274,28 +274,27 @@ impl SqliteConnection {
/// ## Example
///
/// ```rust,no_run
/// # use rusqlite::{SqliteConnection};
/// fn preferred_locale(conn: &SqliteConnection) -> String {
/// conn.query_row("SELECT value FROM preferences WHERE name='locale'", &[], |row| {
/// # use rusqlite::{SqliteResult,SqliteConnection};
/// fn preferred_locale(conn: &SqliteConnection) -> SqliteResult<String> {
/// conn.query_row_safe("SELECT value FROM preferences WHERE name='locale'", &[], |row| {
/// row.get(0)
/// })
/// }
/// ```
///
/// ## Failure
///
/// Panics if:
///
/// * Preparing the query fails.
/// * Running the query fails (i.e., calling `query` on the prepared statement).
/// * The query does not successfully return at least one row.
///
/// If the query returns more than one row, all rows except the first are ignored.
pub fn query_row<T, F>(&self, sql: &str, params: &[&ToSql], f: F) -> T
pub fn query_row<T, F>(&self, sql: &str, params: &[&ToSql], f: F) -> SqliteResult<T>
where F: FnOnce(SqliteRow) -> T {
let mut stmt = self.prepare(sql).unwrap();
let mut rows = stmt.query(params).unwrap();
f(rows.next().expect("Query did not return a row").unwrap())
let mut stmt = try!(self.prepare(sql));
let mut rows = try!(stmt.query(params));

match rows.next() {
Some(row) => row.map(f),
None => Err(SqliteError{
code: ffi::SQLITE_NOTICE,
message: "Query did not return a row".to_string(),
})
}
}

/// Convenience method to execute a query that is expected to return a single row.
Expand All @@ -312,18 +311,14 @@ impl SqliteConnection {
/// ```
///
/// If the query returns more than one row, all rows except the first are ignored.
///
/// ## Deprecated
///
/// This method should be considered deprecated. Use `query_row` instead, which now
/// does exactly the same thing.
pub fn query_row_safe<T, F>(&self, sql: &str, params: &[&ToSql], f: F) -> SqliteResult<T>
where F: FnOnce(SqliteRow) -> T {
let mut stmt = try!(self.prepare(sql));
let mut rows = try!(stmt.query(params));

match rows.next() {
Some(row) => row.map(f),
None => Err(SqliteError{
code: ffi::SQLITE_NOTICE,
message: "Query did not return a row".to_string(),
})
}
self.query_row(sql, params, f)
}

/// Prepare a SQL statement for execution.
Expand Down Expand Up @@ -904,7 +899,7 @@ mod test {
assert_eq!(db.execute("INSERT INTO foo(x) VALUES (?)", &[&1i32]).unwrap(), 1);
assert_eq!(db.execute("INSERT INTO foo(x) VALUES (?)", &[&2i32]).unwrap(), 1);

assert_eq!(3i32, db.query_row("SELECT SUM(x) FROM foo", &[], |r| r.get(0)));
assert_eq!(3i32, db.query_row("SELECT SUM(x) FROM foo", &[], |r| r.get(0)).unwrap());
}

#[test]
Expand Down
6 changes: 3 additions & 3 deletions src/transaction.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@ mod test {
}
{
let _tx = db.transaction().unwrap();
assert_eq!(2i32, db.query_row("SELECT SUM(x) FROM foo", &[], |r| r.get(0)));
assert_eq!(2i32, db.query_row("SELECT SUM(x) FROM foo", &[], |r| r.get(0)).unwrap());
}
}

Expand All @@ -200,7 +200,7 @@ mod test {
}
{
let _tx = db.transaction().unwrap();
assert_eq!(2i32, db.query_row("SELECT SUM(x) FROM foo", &[], |r| r.get(0)));
assert_eq!(2i32, db.query_row("SELECT SUM(x) FROM foo", &[], |r| r.get(0)).unwrap());
}
}

Expand Down Expand Up @@ -228,6 +228,6 @@ mod test {
}
}
}
assert_eq!(3i32, db.query_row("SELECT SUM(x) FROM foo", &[], |r| r.get(0)));
assert_eq!(3i32, db.query_row("SELECT SUM(x) FROM foo", &[], |r| r.get(0)).unwrap());
}
}
6 changes: 3 additions & 3 deletions src/types.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,7 @@ mod test {
let v1234 = vec![1u8,2,3,4];
db.execute("INSERT INTO foo(b) VALUES (?)", &[&v1234]).unwrap();

let v: Vec<u8> = db.query_row("SELECT b FROM foo", &[], |r| r.get(0));
let v: Vec<u8> = db.query_row("SELECT b FROM foo", &[], |r| r.get(0)).unwrap();
assert_eq!(v, v1234);
}

Expand All @@ -257,7 +257,7 @@ mod test {
let s = "hello, world!";
db.execute("INSERT INTO foo(t) VALUES (?)", &[&s.to_string()]).unwrap();

let from: String = db.query_row("SELECT t FROM foo", &[], |r| r.get(0));
let from: String = db.query_row("SELECT t FROM foo", &[], |r| r.get(0)).unwrap();
assert_eq!(from, s);
}

Expand All @@ -268,7 +268,7 @@ mod test {
let ts = time::Timespec{sec: 10_000, nsec: 0 };
db.execute("INSERT INTO foo(t) VALUES (?)", &[&ts]).unwrap();

let from: time::Timespec = db.query_row("SELECT t FROM foo", &[], |r| r.get(0));
let from: time::Timespec = db.query_row("SELECT t FROM foo", &[], |r| r.get(0)).unwrap();
assert_eq!(from, ts);
}

Expand Down

5 comments on commit 03be8e0

@marcusklaas
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool! I was just thinking, query_row_safe was never really 'safe', as you could just pass the identity function and get the underlying SqliteRow object. To make it really safe, we should add a T: 'static bound. What do you think?

@jgallagher
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that would be too restrictive, wouldn't it? You wouldn't be able to capture any references in f at all, which would otherwise be fine.

@marcusklaas
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are correct, I was thinking about the more general case. The point is that I don't see why you would ever want to capture a reference to the SqliteRow object anyway.

The idea I'm playing with is having SqliteStatement not return an iterator over SqliteRows, but an iterator over T: 'static (after passing it a map as in query_row_safe). Would that not make the interface panic free, whilst maintaining the full ergonomics of Iterator, including collect!

Am I missing something?

Edit: so this would change the query method's signature on SqliteStatement to

fn query<T, F>(&mut self, params: &[&ToSql], fn: F) -> SomeIterator<T>
    where F: FnOnce(SqliteRow) -> T,
          T: 'static

Edit2: deadly typo: how -> why

@jgallagher
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. So something like this?

fn query_map<'a, T, F>(&'a mut self, params: &[&ToSql], f: F) -> SqliteResult<MappedRows<'a, T>>
    where T: 'static,
          F: Fn(&SqliteRow) -> T,

where MappedRows<T> is an iterator? That would be pretty great.

@jgallagher
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hah, we were close. I think F has to be Fn (so you can call it for each row). Oh duh, you're right that it can just take SqliteRow (not a ref) since SqliteRow is already non-'static.

I'm less sure about replacing query outright. It seems like there might be some uses that are currently possible that this wouldn't cover, but I can't think of any off hand...

Please sign in to comment.