When using `.optional()`, convert a `QueryBuilderError` into `None` #4019

dessalines · 2024-05-07T21:54:02Z

This is my first diesel PR, I apologize in advance for any issues. Let me know of any documentation I missed, tests I should add, or anything I can do to improve it.

I also had to comment a few tests in bin/test to get this to pass, which leads me to believe the main branch has some unaddressed errors which might fail CI before reaching the test I added.

Related discussion: #4018

- Related discussion: diesel-rs#4018

dessalines · 2024-05-07T22:00:38Z

diesel_tests/tests/update.rs

@@ -234,6 +234,28 @@ fn update_with_no_changes() {
        .unwrap();
 }

+#[test]


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.

It's better to have a separate test for this. These tests are quite fast so there is no point to worry about anything.

weiznich

The structure of the change looks ok, although I'm quite unhappy with the new matching there as it allows a potentially large number of different error. That's not something I would like to support there.

weiznich · 2024-05-08T15:20:57Z

diesel/src/result.rs

@@ -284,6 +292,7 @@ impl<T> OptionalExtension<T> for QueryResult<T> {
        match self {
            Ok(value) => Ok(Some(value)),
            Err(Error::NotFound) => Ok(None),
+            Err(Error::QueryBuilderError(_)) => Ok(None),


I wouldn't want to allow such a wide match here. We should only allow that one specific error that happens for empty updates.

Since this is a String, should I match that exact string then?

If we want to make that into an error that we may match on, then we probably want to turn it into a some form of typed error: it's less error prone (typos, error message updates...), and does not imply the additional runtime cost that string comparisons would imply.

The type already allows that:

So if it's currently a String (or &'static str rather) that we then turn into a Box<dyn StdError + ...>, we probably want to create a dedicated type for it (with the same message as before in its Display impl) and match it with .is::<_>(), as is done with UnexpectedNullError in the QueryableByName code for Option.

diesel/diesel/src/result.rs

Lines 393 to 403 in df89687

/// An unexpected `NULL` was encountered during deserialization

#[derive(Debug, Clone, Copy)]

pub struct UnexpectedNullError;

impl fmt::Display for UnexpectedNullError {

fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {

write!(f, "Unexpected null for non-null column")

}

}

impl StdError for UnexpectedNullError {}

diesel/diesel/src/type_impls/option.rs

Line 94 in df89687

Err(e) if e.is::<crate::result::UnexpectedNullError>() => Ok(None),

I was thinking the same, I'd be glad to do this if its desired.

Ten0

Thanks for tackling this topic and opening this PR! 😊
It does indeed look like it would be useful to be able to catch this error in an ergonomic manner. 🙂

However mixing it within the existing .optional() extension feels odd to me: it feels like it might make it harder to use .optional() without mixing in potential unrelated/unexpected scenarios into None.
I find that it is typically better to avoid falling back from larger error categories than may really happen, as this may turn into silent fails, resulting in hard-to-debug issues.

As far as I'm concerned we have many use-cases where we use the update(table.find(id)).filter(make_sure_that_state_is_ok()).optional() combination, where make_sure_that_state_is_ok() is for us to get single-query happy path, and then if None re-query to know whether the ID was wrong or the state was wrong and return the appropriate user error.

However we have no use-case such as the one described by this issue.

This tends to make me believe that it's better to be able to guarantee that .optional really only catches the scenario I'm personally always looking for where make_sure_that_state_is_ok() is FALSE, to avoid potentially enforcing upon users the unexpected silent fail scenario described above, so I'd probably prefer if it stayed dedicated to the current meaning it has. I think a design that wouldn't have this downside of too-large-matching, but would still have the same ergonomics as the solution currently proposed by this PR, would be introducing a separate function similar to .optional() that catches precisely that empty changeset scenario. (.optional_empty_changeset()?)

However as weiznich pointed out here, .optional() is just an extension trait that is reasonably easy to re-create outside of Diesel. (We do have a few similar extension traits ourselves, such as .get_only_result() which is a get_result that errors if there are multiple results, which is an extremely common case for us, prevents bugs where one would forget to write the appropriate .find or .filter clause (again because hard errors are preferable to silent fails that would cause incorrect behavior)... and have found that having that in our diesel_helpers crate doesn't generate any friction.)
So overall I have no strong opinion on whether the extension trait itself should even live within Diesel itself - I guess it depends on really how common this use-case is, as this is a compromise between providing the feature and API&Doc size/maintenance cost, and all I can say is I personally haven't encountered this use-case.

However what seems pretty clear that we should do within Diesel is adding a typed error for this and exporting it in a similar way to UnexpectedNullError, so that users can actually match this error and construct any extension trait related to this that they see fit, in an idiomatic and ~typed manner, (that is, not by matching on a string which the API does not guarantee may not change and is slow).

dessalines · 2024-05-10T15:54:25Z

.optional_empty_changeset()

I'd be fine with this also, although I'd probably prefer .optional() catching this error case already.

guess it depends on really how common this use-case is,

Its at least common in our case, to build form large form objects without knowing beforehand which fields we're receiving are updated, because they come from data sources outside our control. We've added some hacky code to check individually that all fields are not None to avoid this error (and you're right also that we could make an additional extension on top of optional. So making sure this case is accounted for in .optional(), rather than throwing an error, is much safer for us than doing a .ok() on the result.

Ten0 · 2024-05-10T16:22:24Z

Ok so next steps for this PR:

Create the new error type and have the relevant code export it
Get opinion from @diesel-rs/reviewers on whether it's worth including the extension trait in Diesel itself
If so, add that as well

Ten0 · 2024-05-11T22:25:52Z

Sorry, missclick

…ilder_error

diesel/src/result.rs

dessalines · 2024-05-14T17:19:23Z

diesel/src/result.rs

+
+/// Expected when an update has no changes to save
+#[derive(Debug, Clone, Copy)]
+pub struct EmptyChangeset;


Not sure if set should be capitalized or not.

dessalines · 2024-05-14T17:20:13Z

diesel_tests/tests/update.rs

@@ -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());


Changed this from a panic check, to an is_err check, since this now returns QueryBuilder(EmptyChangeset)

weiznich

Looks fine for me beside the comment about the breaking change to the OptionalExtension trait. I think it's safer to just introduce a new trait for that + reexport that from the prelude.

diesel/src/result.rs

weiznich · 2024-05-17T11:21:43Z

diesel_tests/tests/update.rs

@@ -234,6 +234,28 @@ fn update_with_no_changes() {
        .unwrap();
 }

+#[test]


It's better to have a separate test for this. These tests are quite fast so there is no point to worry about anything.

…ilder_error

weiznich

Looks good for me with the small documentation fix applied.

weiznich · 2024-05-17T19:15:43Z

diesel/src/result.rs

@@ -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`.


Minor note: That comment is not correct anymore. It might be a got idea to move this comment to the new error type.

K, will do.

diesel/src/result.rs

JMLX42 · 2024-05-22T07:31:45Z

@weiznich can we please have a minor release for this?

weiznich · 2024-05-22T07:50:45Z

@JMLX42 As this is a new feature it needs to wait until the next diesel feature release happens. As for the timeline for such a release: This is a free time project, so that will happen as soon as it is ready. We generally do not give ETA's there. You can contribute or sponsor the development to make such a release happen faster.

When using .optional(), convert a QueryBuilderError into None

8532355

- Related discussion: diesel-rs#4018

dessalines commented May 7, 2024

View reviewed changes

dessalines added 4 commits May 7, 2024 18:08

Fixing doc test.

8efba72

Adding message to doc result.

ccb6adf

Adding into

cfc5078

Another doc comment.

234f369

weiznich requested changes May 8, 2024

View reviewed changes

weiznich requested review from Ten0 and a team May 8, 2024 18:28

Ten0 reviewed May 9, 2024

View reviewed changes

Ten0 closed this May 11, 2024

Ten0 reopened this May 11, 2024

dessalines marked this pull request as draft May 14, 2024 14:18

dessalines added 5 commits May 14, 2024 12:57

Adding a separate optional_empty_changeset error.

d1dc8a8

Merge remote-tracking branch 'diesel-rs/master' into optional_querybu…

82b9808

…ilder_error

Fixing doc for QueryBuilderError.

cc455ed

Fixing pointless doc.

c65dd31

Fixing test.

d6f1b7b

dessalines marked this pull request as ready for review May 14, 2024 17:17

dessalines requested review from weiznich and Ten0 May 14, 2024 17:17

dessalines commented May 14, 2024

View reviewed changes

diesel/src/result.rs Show resolved Hide resolved

dessalines commented May 14, 2024

View reviewed changes

Fixing doctest.

0edee8f

weiznich reviewed May 17, 2024

View reviewed changes

dessalines added 3 commits May 17, 2024 12:03

Use a separate trait extension for optional_empty_changeset

eda8628

Merge remote-tracking branch 'diesel-rs/master' into optional_querybu…

707c4f5

…ilder_error

Fix doctest.

01ea5c3

weiznich approved these changes May 17, 2024

View reviewed changes

Moving error doc.

6b225d3

weiznich approved these changes May 18, 2024

View reviewed changes

diesel/src/result.rs Outdated Show resolved Hide resolved

Update result.rs

1062278

weiznich enabled auto-merge May 18, 2024 05:53

weiznich added this pull request to the merge queue May 18, 2024

Merged via the queue into diesel-rs:master with commit 3c7b7c4 May 18, 2024
48 checks passed

BrewTestBot mentioned this pull request May 31, 2024

diesel 2.2.0 Homebrew/homebrew-core#173312

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

When using `.optional()`, convert a `QueryBuilderError` into `None` #4019

When using `.optional()`, convert a `QueryBuilderError` into `None` #4019

dessalines commented May 7, 2024 •

edited

Loading

dessalines May 7, 2024

weiznich May 17, 2024

weiznich left a comment

weiznich May 8, 2024

dessalines May 9, 2024

Ten0 May 9, 2024 •

edited

Loading

dessalines May 10, 2024

Ten0 left a comment •

edited

Loading

dessalines commented May 10, 2024

Ten0 commented May 10, 2024

Ten0 commented May 11, 2024

dessalines May 14, 2024

dessalines May 14, 2024

weiznich left a comment

weiznich May 17, 2024

weiznich left a comment

weiznich May 17, 2024

dessalines May 17, 2024

JMLX42 commented May 22, 2024

weiznich commented May 22, 2024

	/// An unexpected `NULL` was encountered during deserialization
	#[derive(Debug, Clone, Copy)]
	pub struct UnexpectedNullError;

	impl fmt::Display for UnexpectedNullError {
	fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
	write!(f, "Unexpected null for non-null column")
	}
	}

	impl StdError for UnexpectedNullError {}

When using .optional(), convert a QueryBuilderError into None #4019

When using .optional(), convert a QueryBuilderError into None #4019

Conversation

dessalines commented May 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weiznich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ten0 May 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ten0 left a comment • edited Loading

Choose a reason for hiding this comment

dessalines commented May 10, 2024

Ten0 commented May 10, 2024

Ten0 commented May 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weiznich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weiznich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JMLX42 commented May 22, 2024

weiznich commented May 22, 2024

When using `.optional()`, convert a `QueryBuilderError` into `None` #4019

When using `.optional()`, convert a `QueryBuilderError` into `None` #4019

dessalines commented May 7, 2024 •

edited

Loading

Ten0 May 9, 2024 •

edited

Loading

Ten0 left a comment •

edited

Loading