Skip to content

Commit

Permalink
Bug 1582379 - Include more context in Rust mozStorage errors. r=tcsc
Browse files Browse the repository at this point in the history 
A low-frequency error that showed up in telemetry for new bookmark
sync is "Storage operation failed with NS_ERROR_CANNOT_CONVERT_DATA",
which occurs when we try to convert a variant to the wrong type. It
would be helpful to include the type and column names for this case;
for example, "Can't get i32 for column lastModified". Neither the type
nor column name are PII; we want them to show up in telemetry.

Differential Revision: https://phabricator.services.mozilla.com/D46433

--HG--
extra : moz-landing-system : lando
  • Loading branch information
@linabutler
linabutler committed Sep 19, 2019
1 parent 959ff7f commit 065c7d6
Show file tree
Hide file tree
Showing 2 changed files with 154 additions and 36 deletions.
170 changes: 135 additions & 35 deletions storage/rust/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
#![allow(non_snake_case)]

use std::{error, fmt, ops::Deref, result};
use std::{borrow::Cow, error, fmt, ops::Deref, result};

use nserror::{nsresult, NS_ERROR_NO_INTERFACE};
use nsstring::nsCString;
Expand Down Expand Up @@ -82,7 +82,7 @@ impl Conn {
/// If `query` contains multiple statements, only the first will be prepared,
/// and the rest will be ignored.
pub fn prepare<Q: AsRef<str>>(&self, query: Q) -> Result<Statement> {
let statement = self.call_and_wrap_error(|| {
let statement = self.call_and_wrap_error(DatabaseOp::Prepare, || {
getter_addrefs(|p| unsafe {
self.handle
.CreateStatement(&*nsCString::from(query.as_ref()), p)
Expand All @@ -97,7 +97,7 @@ impl Conn {
/// Executes a SQL statement. `query` may contain one or more
/// semicolon-separated SQL statements.
pub fn exec<Q: AsRef<str>>(&self, query: Q) -> Result<()> {
self.call_and_wrap_error(|| {
self.call_and_wrap_error(DatabaseOp::Exec, || {
unsafe {
self.handle
.ExecuteSimpleSQL(&*nsCString::from(query.as_ref()))
Expand Down Expand Up @@ -148,6 +148,7 @@ impl Conn {
/// uses.
fn call_and_wrap_error<T>(
&self,
op: DatabaseOp,
func: impl FnOnce() -> result::Result<T, nsresult>,
) -> Result<T> {
func().or_else(|rv| -> Result<T> {
Expand All @@ -156,7 +157,12 @@ impl Conn {
Err(if code != SQLITE_OK {
let mut message = nsCString::new();
unsafe { self.handle.GetLastErrorString(&mut *message) }.to_result()?;
Error::Database(rv, code, String::from_utf8_lossy(&message).into_owned())
Error::Database {
rv,
op,
code,
message,
}
} else {
rv.into()
})
Expand Down Expand Up @@ -235,23 +241,32 @@ impl<'c> Statement<'c> {
let variant = value.into_variant();
unsafe { self.handle.BindByIndex(index as u32, variant.coerce()) }
.to_result()
.map_err(|rv| Error::BindByIndex(rv, index))
.map_err(|rv| Error::BindByIndex {
rv,
data_type: V::type_name(),
index,
})
}

/// Binds a parameter with the given `name` to the prepared statement.
pub fn bind_by_name<N: AsRef<str>, V: VariantType>(&mut self, name: N, value: V) -> Result<()> {
let name = name.as_ref();
let variant = value.into_variant();
unsafe {
self.handle
.BindByName(&*nsCString::from(name.as_ref()), variant.coerce())
.BindByName(&*nsCString::from(name), variant.coerce())
}
.to_result()
.map_err(|rv| Error::BindByName(rv, name.as_ref().into()))
.map_err(|rv| Error::BindByName {
rv,
data_type: V::type_name(),
name: name.into(),
})
}

/// Executes the statement and returns the next row of data.
pub fn step<'s>(&'s mut self) -> Result<Option<Step<'c, 's>>> {
let has_more = self.conn.call_and_wrap_error(|| {
let has_more = self.conn.call_and_wrap_error(DatabaseOp::Step, || {
let mut has_more = false;
unsafe { self.handle.ExecuteStep(&mut has_more) }.to_result()?;
Ok(has_more)
Expand All @@ -262,8 +277,9 @@ impl<'c> Statement<'c> {
/// Executes the statement once, discards any data, and resets the
/// statement.
pub fn execute(&mut self) -> Result<()> {
self.conn
.call_and_wrap_error(|| unsafe { self.handle.Execute() }.to_result())
self.conn.call_and_wrap_error(DatabaseOp::Execute, || {
unsafe { self.handle.Execute() }.to_result()
})
}

/// Resets the prepared statement so that it's ready to be executed
Expand All @@ -273,20 +289,23 @@ impl<'c> Statement<'c> {
Ok(())
}

fn get_column_index<N: AsRef<str>>(&self, name: N) -> Result<u32> {
fn get_column_index(&self, name: &str) -> Result<u32> {
let mut index = 0u32;
let rv = unsafe {
self.handle
.GetColumnIndex(&*nsCString::from(name.as_ref()), &mut index)
.GetColumnIndex(&*nsCString::from(name), &mut index)
};
if rv.succeeded() {
Ok(index)
} else {
Err(Error::InvalidColumn(rv, name.as_ref().into()))
Err(Error::InvalidColumn {
rv,
name: name.into(),
})
}
}

fn get_column_value<T: VariantType>(&self, index: u32) -> Result<T> {
fn get_column_value<T: VariantType>(&self, index: u32) -> result::Result<T, nsresult> {
let variant = getter_addrefs(|p| unsafe { self.handle.GetVariant(index, p) })?;
let value = T::from_variant(variant.coerce())?;
Ok(value)
Expand All @@ -305,7 +324,13 @@ pub struct Step<'c, 's>(&'s mut Statement<'c>);
impl<'c, 's> Step<'c, 's> {
/// Returns the value of the column at `index` for the current row.
pub fn get_by_index<T: VariantType>(&self, index: u32) -> Result<T> {
self.0.get_column_value(index)
self.0
.get_column_value(index)
.map_err(|rv| Error::GetByIndex {
rv,
data_type: T::type_name(),
index,
})
}

/// A convenience wrapper that returns the default value for the column
Expand All @@ -316,8 +341,15 @@ impl<'c, 's> Step<'c, 's> {

/// Returns the value of the column specified by `name` for the current row.
pub fn get_by_name<N: AsRef<str>, T: VariantType>(&self, name: N) -> Result<T> {
let name = name.as_ref();
let index = self.0.get_column_index(name)?;
self.0.get_column_value(index)
self.0
.get_column_value(index)
.map_err(|rv| Error::GetByName {
rv,
data_type: T::type_name(),
name: name.into(),
})
}

/// Returns the default value for the column with the given `name`, or the
Expand All @@ -327,6 +359,27 @@ impl<'c, 's> Step<'c, 's> {
}
}

/// A database operation, included for better context in error messages.
#[derive(Clone, Copy, Debug, Eq, PartialEq)]
pub enum DatabaseOp {
Exec,
Prepare,
Step,
Execute,
}

impl DatabaseOp {
/// Returns a description of the operation to include in an error message.
pub fn what(&self) -> &'static str {
match self {
DatabaseOp::Exec => "execute SQL string",
DatabaseOp::Prepare => "prepare statement",
DatabaseOp::Step => "step statement",
DatabaseOp::Execute => "execute statement",
}
}
}

/// Storage errors.
#[derive(Debug)]
pub enum Error {
Expand All @@ -336,18 +389,49 @@ pub enum Error {

/// A database operation failed. The error includes a SQLite result code,
/// and an explanation string.
Database(nsresult, i32, String),
Database {
rv: nsresult,
op: DatabaseOp,
code: i32,
message: nsCString,
},

/// A parameter couldn't be bound at this index, likely because the index
/// is out of range.
BindByIndex(nsresult, u32),
/// A parameter with the given data type couldn't be bound at this index,
/// likely because the index is out of range.
BindByIndex {
rv: nsresult,
data_type: Cow<'static, str>,
index: u32,
},

/// A parameter couldn't be bound to this name, likely because the statement
/// doesn't have a matching `:`-prefixed parameter with the name.
BindByName(nsresult, String),
/// A parameter with the given type couldn't be bound to this name, likely
/// because the statement doesn't have a matching `:`-prefixed parameter
/// with the name.
BindByName {
rv: nsresult,
data_type: Cow<'static, str>,
name: String,
},

/// A column with this name doesn't exist.
InvalidColumn(nsresult, String),
InvalidColumn { rv: nsresult, name: String },

/// A value of the given type couldn't be accessed at this index. This is
/// the error returned when a type conversion fails; for example, requesting
/// an `nsString` instead of an `Option<nsString>` when the column is `NULL`.
GetByIndex {
rv: nsresult,
data_type: Cow<'static, str>,
index: u32,
},

/// A value of the given type couldn't be accessed for the column with
/// this name.
GetByName {
rv: nsresult,
data_type: Cow<'static, str>,
name: String,
},

/// A storage operation failed for other reasons.
Other(nsresult),
Expand All @@ -369,10 +453,12 @@ impl From<Error> for nsresult {
fn from(err: Error) -> nsresult {
match err {
Error::NoThread => NS_ERROR_NO_INTERFACE,
Error::Database(rv, _, _)
| Error::BindByIndex(rv, _)
| Error::BindByName(rv, _)
| Error::InvalidColumn(rv, _)
Error::Database { rv, .. }
| Error::BindByIndex { rv, .. }
| Error::BindByName { rv, .. }
| Error::InvalidColumn { rv, .. }
| Error::GetByIndex { rv, .. }
| Error::GetByName { rv, .. }
| Error::Other(rv) => rv,
}
}
Expand All @@ -382,20 +468,34 @@ impl fmt::Display for Error {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
match self {
Error::NoThread => f.write_str("Async thread unavailable for storage connection"),
Error::Database(_, code, message) => {
Error::Database {
op, code, message, ..
} => {
if message.is_empty() {
write!(f, "Database operation failed with code {}", code)
write!(f, "Failed to {} with code {}", op.what(), code)
} else {
write!(
f,
"Database operation failed with code {} ({})",
code, message
"Failed to {} with code {} ({})",
op.what(),
code,
message
)
}
}
Error::BindByIndex(_, index) => write!(f, "Can't bind parameter at {}", index),
Error::BindByName(_, name) => write!(f, "Can't bind named parameter {}", name),
Error::InvalidColumn(_, name) => write!(f, "Column {} doesn't exist", name),
Error::BindByIndex {
data_type, index, ..
} => write!(f, "Can't bind {} at {}", data_type, index),
Error::BindByName {
data_type, name, ..
} => write!(f, "Can't bind {} to named parameter {}", data_type, name),
Error::InvalidColumn { name, .. } => write!(f, "Column {} doesn't exist", name),
Error::GetByIndex {
data_type, index, ..
} => write!(f, "Can't get {} at {}", data_type, index),
Error::GetByName {
data_type, name, ..
} => write!(f, "Can't get {} for column {}", data_type, name),
Error::Other(rv) => write!(f, "Storage operation failed with {}", rv.error_name()),
}
}
Expand Down
20 changes: 19 additions & 1 deletion storage/variant/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ extern crate xpcom;

mod bag;

use std::borrow::Cow;

use libc::c_double;
use nserror::{nsresult, NS_OK};
use nsstring::{nsACString, nsAString, nsCString, nsString};
Expand Down Expand Up @@ -70,6 +72,7 @@ impl GetDataType for nsIVariant {
}

pub trait VariantType {
fn type_name() -> Cow<'static, str>;
fn into_variant(self) -> RefPtr<nsIVariant>;
fn from_variant(variant: &nsIVariant) -> Result<Self, nsresult>
where
Expand All @@ -80,6 +83,9 @@ pub trait VariantType {
macro_rules! variant {
($typ:ident, $constructor:ident, $getter:ident) => {
impl VariantType for $typ {
fn type_name() -> Cow<'static, str> {
stringify!($typ).into()
}
fn into_variant(self) -> RefPtr<nsIVariant> {
// getter_addrefs returns a Result<RefPtr<T>, nsresult>,
// but we know that our $constructor is infallible, so we can
Expand All @@ -102,6 +108,9 @@ macro_rules! variant {
};
(* $typ:ident, $constructor:ident, $getter:ident) => {
impl VariantType for $typ {
fn type_name() -> Cow<'static, str> {
stringify!($typ).into()
}
fn into_variant(self) -> RefPtr<nsIVariant> {
// getter_addrefs returns a Result<RefPtr<T>, nsresult>,
// but we know that our $constructor is infallible, so we can
Expand All @@ -128,6 +137,9 @@ macro_rules! variant {
// The macro can't produce its implementations of VariantType, however,
// so we implement them concretely.
impl VariantType for () {
fn type_name() -> Cow<'static, str> {
"()".into()
}
fn into_variant(self) -> RefPtr<nsIVariant> {
// getter_addrefs returns a Result<RefPtr<T>, nsresult>,
// but we know that NS_NewStorageNullVariant is infallible, so we can
Expand All @@ -142,7 +154,13 @@ impl VariantType for () {
}
}

impl<T> VariantType for Option<T> where T: VariantType {
impl<T> VariantType for Option<T>
where
T: VariantType,
{
fn type_name() -> Cow<'static, str> {
format!("Option<{}>", T::type_name()).into()
}
fn into_variant(self) -> RefPtr<nsIVariant> {
match self {
Some(v) => v.into_variant(),
Expand Down

0 comments on commit 065c7d6

Please sign in to comment.