-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
When using .optional()
, convert a QueryBuilderError
into None
#4019
Changes from 11 commits
8532355
8efba72
ccb6adf
cfc5078
234f369
d1dc8a8
82b9808
cc455ed
c65dd31
d6f1b7b
0edee8f
eda8628
707c4f5
01ea5c3
6b225d3
1062278
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -45,6 +45,8 @@ pub enum Error { | |
/// An example of when this error could occur is if you are attempting to | ||
/// construct an update statement with no changes (e.g. all fields on the | ||
/// struct are `None`). | ||
/// | ||
/// When using `optional_empty_changeset`, this error is turned into `None`. | ||
QueryBuilderError(Box<dyn StdError + Send + Sync>), | ||
|
||
/// An error occurred deserializing the data being sent to the database. | ||
|
@@ -268,7 +270,7 @@ pub trait OptionalExtension<T> { | |
/// # Example | ||
/// | ||
/// ```rust | ||
/// use diesel::{QueryResult, NotFound, OptionalExtension}; | ||
/// use diesel::{QueryResult, NotFound, OptionalExtension, result::Error::QueryBuilderError}; | ||
weiznich marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/// | ||
/// let result: QueryResult<i32> = Ok(1); | ||
/// assert_eq!(Ok(Some(1)), result.optional()); | ||
|
@@ -277,6 +279,18 @@ pub trait OptionalExtension<T> { | |
/// assert_eq!(Ok(None), result.optional()); | ||
/// ``` | ||
fn optional(self) -> Result<Option<T>, Error>; | ||
|
||
/// By default, Diesel treats an empty update as a `QueryBuilderError`. This method will | ||
/// convert that error into `None`. | ||
/// | ||
/// # Example | ||
/// | ||
/// ```rust | ||
/// use diesel::{QueryResult, OptionalExtension, result::Error::QueryBuilderError, result::EmptyChangeset}; | ||
/// let result: QueryResult<i32> = Err(QueryBuilderError(Box::new(EmptyChangeset))); | ||
/// assert_eq!(Ok(None), result.optional_empty_changeset()); | ||
/// ``` | ||
fn optional_empty_changeset(self) -> Result<Option<T>, Error>; | ||
} | ||
|
||
impl<T> OptionalExtension<T> for QueryResult<T> { | ||
|
@@ -287,6 +301,13 @@ impl<T> OptionalExtension<T> for QueryResult<T> { | |
Err(e) => Err(e), | ||
} | ||
} | ||
fn optional_empty_changeset(self) -> Result<Option<T>, Error> { | ||
dessalines marked this conversation as resolved.
Show resolved
Hide resolved
|
||
match self { | ||
Ok(value) => Ok(Some(value)), | ||
Err(Error::QueryBuilderError(e)) if e.is::<EmptyChangeset>() => Ok(None), | ||
Err(e) => Err(e), | ||
} | ||
} | ||
} | ||
|
||
impl From<NulError> for ConnectionError { | ||
|
@@ -413,3 +434,18 @@ impl fmt::Display for UnexpectedEndOfRow { | |
} | ||
|
||
impl StdError for UnexpectedEndOfRow {} | ||
|
||
/// Expected when an update has no changes to save | ||
#[derive(Debug, Clone, Copy)] | ||
pub struct EmptyChangeset; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure if set should be capitalized or not. |
||
|
||
impl fmt::Display for EmptyChangeset { | ||
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
write!( | ||
f, | ||
"There are no changes to save. This query cannot be built" | ||
) | ||
} | ||
} | ||
|
||
impl StdError for EmptyChangeset {} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -214,7 +214,6 @@ fn sql_syntax_is_correct_when_option_field_comes_mixed_with_non_option() { | |
} | ||
|
||
#[test] | ||
#[should_panic(expected = "There are no changes to save.")] | ||
fn update_with_no_changes() { | ||
#[derive(AsChangeset)] | ||
#[diesel(table_name = users)] | ||
|
@@ -228,10 +227,30 @@ fn update_with_no_changes() { | |
name: None, | ||
hair_color: None, | ||
}; | ||
update(users::table) | ||
let update_result = update(users::table).set(&changes).execute(connection); | ||
assert!(update_result.is_err()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changed this from a panic check, to an |
||
} | ||
|
||
#[test] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since this test is just copied from the one above, it might make sense to add the new line just above the panic one above. Or just keep it as its own test, up to you. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's better to have a separate test for this. These tests are quite fast so there is no point to worry about anything. |
||
fn update_with_optional_empty_changeset() { | ||
#[derive(AsChangeset)] | ||
#[diesel(table_name = users)] | ||
struct Changes { | ||
name: Option<String>, | ||
hair_color: Option<String>, | ||
} | ||
|
||
let connection = &mut connection(); | ||
let changes = Changes { | ||
name: None, | ||
hair_color: None, | ||
}; | ||
let update_result = update(users::table) | ||
.set(&changes) | ||
.execute(connection) | ||
.optional_empty_changeset() | ||
.unwrap(); | ||
assert_eq!(None, update_result); | ||
} | ||
|
||
#[test] | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor note: That comment is not correct anymore. It might be a got idea to move this comment to the new error type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
K, will do.