Fix UnexpectedNullError when deserializing (NULL,NotNull) into Option

[Ten0]

Resolves #2274

I'm not sure if this was originally a bug or not, but in any case as described on the issue, often the NotNull field (say, zzz::some_nullable_thing.is_not_null()) doesn't have any meaning if we couldn't left_join on the zzz table and select the NULL field (say, zzz::id).

This is why I'm very happy to not have access to the second field if the first field is NULL.

However I can't think of a scenario where I'd prefer a runtime error: cases where I could unintentionally "forget" a field should be covered by the typing after the query: "Why can't I get the value of this field? Oh it's because it was packed in this Option...)

This is why I beleive that this behaviour that ends up giving Ok(None) instead of Err(DeserializationError(UnexpectedNullError)) is better.

Accorging to the tests this implementation seems to work properly, though I'm not sure why recovering from the error skips the proper amount of fields... Can anyone confirm that (and why) this indeed behaves as expected ? :)
EDIT: This indeed did not skip the proper amount of fields, and is now fixed.

Also open to any comments :)

[weiznich]

Could you add a testcase that would trigger the bad behaviour to the test suite? Otherwise this looks fine 👍

[Ten0]

Hello,

I've added the test case (it indeed did not work where I expected it to not work) and fixed it :)

Comments (or merge ^^) welcome. :)

[weiznich]

As this is a public API I would prefer not to add this method here, because it only has an internal meaning. That said I'm not sure what would be the right approach here.

[Ten0]

If Row has to be public for others to be able to connect new backends I feel like this is close to being the only alternative. Another way I see would be to make sure that when the error is thrown all the fields have been consumed, but that is useless for every other error case and that possibly means a lot of places where we would have to take care of this and possibly make a mistake (enums, structs with Queryable derive,...) .

Given the other functions that this trait requires, this one doesn't seem too absurd to me, especially since the field is already required by most impls.

[Ten0]

Since it is consistent with advance, the meaning is not only internal.

[weiznich]

I've not forgot this issue, it should definitively be fixed with the diesel 2.0 release, so I've added it the the corresponding mile stone. That said, unfortunately I was pretty occupied with other things in the last time, so I had not really the bandwidth to think much about that. It will be done, but takes probably some time.

[Ten0]

Hello,

I keep hitting this as a runtime error regularly, and code that makes requests work is far from being as clear as the one this PR enables.

I understand this changes the API a bit so it requires a thorough examination.
Is there anything I can do to help this advance or should we just wait until more reviewers happen to pass by ?

[weiznich]

I have currently rather limited bandwidth, so I was not able to think about a better API yet, so this needs to wait till I find some time or any of the other reviewers comments. In general we should try to unify the change to the Row trait with that one done in #2181

[Ten0]

Hello,

Just an up, as I keep hitting this error/having to write and maintain intermediate structures when fetching from the database due to this, and would really benefit from this moving forward.

[weiznich]

There has nothing changed from my side. I do currently not have the bandwidth to work on so much things in parallel due to my real life engagements, a fix for this is scheduled to be included in a 2.0 release (I see it as blocker for 2.0) and I want to see some experimentation about how to unify that with the changes required in #2182 (Especially about how it interacts with FIELDS_NEEDED for dynamic clauses where this is not a static member and how that could be integrated into each other)

EDIT: To add something that is actually actionable for others here: I someone has the time to figure out how to make this interact in a nice way with the changes in #2182 I can try to find some time to review such a change. For a backport to the 1.4.x branch I would require a non-breaking way to implement this change. Adding a non-default method to Row is a breaking change.

[Ten0]

This is eventually being solved in #2182 and there's probably a problem in the implementation of this PR that triggers when nullable booleans come into play (due to #104).
=> closing