Removed the force unwrap in the FailableIterator extension for the ne…

[denis-couras]

…xt () method

Although the method accepts an optional return, the function forces unwrap

[jberkel]

As it has been pointed out in the apollographql/apollo-ios#1658, this is a substantial change which has consequences for existing users, so this behaviour change should be documented (and perhaps reverted, if it is not wanted).

[groue]

Thanks for noticing, @jberkel. Let's give some time to the Apollo team to understand the consequences (cc @designatednerd), and act accordingly if they think this is necessary (EDIT: they are on the way 👍).

But basically, yes, this PR has prepare() return an untrustable sequence.

• Did you just fetch 10 rows? Well, who knows, maybe there were 100.
• Did you setup a foreign key and a not null constraint so that you are sure all books link to their author? Well, you may now load a nil author for some books nevertheless, without any error reporting.
• Generally speaking, many database invariants are no longer reliable

In my opinion, this change is really concerning, due to the many apps and other libraries out there that use SQLite.swift. I'm not sure it is reasonable to expect all of them to 1. learn about the change, 2. audit the consequences and 3. switch to failableNext() if necessary. Instead, many app and other libraries now suffer from latent bugs that are just waiting to express.

Before this PR was merged and shipped on 0.13.0, apps and libraries would crash. prepare() did not hide errors. People had an incentive to switch to failableNext(). This was, arguably, a better situation.

[jberkel]

@groue Agreed. Hiding errors this way is a bad solution, and the errors aren't even logged and disappear completely in the ether. I'm not sure why this was merged without discussion. We'll revert it in the next release.

[groue]

Thank you for this, @jberkel 👍

[nathanfallet]

@jberkel Keeping a try! statement makes it crash when it fails. Using a try? instead will return nil if it fails, preventing from crashes. Then, the user can handle the optional. If you want to know about the error, the correct way would be to use a fonction with a throws, and a try.

[groue]

@NathanFallet:

Then, the user can handle the optional.

This is, unfortunately, not true. We're talking about a standard IteratorProtocol, where nil means "end of the sequence". Overloading nil with "an error occurred" is not possible:

let sequence = ...prepare()
var iterator = sequence.makeIterator()
if let v = iterator.next() {
    // Handle value
) else {
    // What? End of sequence, or error? Impossible to tell
}

Then, the user can handle the optional.

Another problem is that "the user" can be many things. There are a lot of them that already exist, and they all already interpret nil as "end of the sequence", beyond the control of any developer:

let sequence = ...prepare()

// "User" is the Swift language
// With this PR, iteration can stop too soon and nobody knows there was an error.
for value in sequence { ... )

// "User" is the standard Array.init initializer
// With this PR, array can be too short and nobody knows there was an error.
let values = Array(sequence)

// "User" is the standard Sequence.map method
// With this PR, array can be too short and nobody knows there was an error.
let values = sequence.map { ... }

// etc

None of those "users" think about errors. None of the many Sequence apis think about errors. The documented behaviors of Sequence and IteratorProtocol do not support iteration errors.

If you want to know about the error, the correct way would be to use a fonction with a throws, and a try.

Correct. A crashing API is correct as well, because it won't let "the user" process invalid data, with unknown consequences. You never know what SQLite.swift is used for, and some users rely very strongly on correct data, and prefer a crash over awful damage - think about apps for firemen, banks, doctors, autonomous vehicles, or the health app on the wrist of your fragile relative.

A crashing API is not ideal, of course, but whenever someone complains about a crash, the proper action is to tell that person to change their program and start using a correct and throwing api.

[groue]

That topic was already discussed 5 five years ago: https://github.com/stephencelis/SQLite.swift/pull/569

[nathanfallet]

@groue Thanks for your explanation, I didn't thought about this. Then, we should revert this change, mark this method as deprecated, and make a new throwing one like I suggested before. (I think it's the best thing to do)

[groue]

Thanks as well, @NathanFallet. It's better when everyone shares the same goal, and is aware of the reasons why. Many people will profit when SQLite.swift stops crashing with iteration errors (eventually), and stops ignoring them as in v0.13.0 (as soon as possible).

[jberkel]

That topic was already discussed 5 five years ago

Yes, feels like going around in circles :) We already have the throwing API, so no new code is needed.

[nathanfallet]

Note: we need to explain how to let people handle errors using failableNext directly:

do {
    while let row = try iterator.failableNext() {
        // Handle row
    }
} catch {
    // Handle error
}

@groue @jberkel

[groue]

Updating the documentation would indeed be quite useful. People do not know how to handle errors properly, even when they think they do (part of https://github.com/apollographql/apollo-ios/pull/1917)

EDIT: I was wrong: the linked commit is correct, since it uses this specific map overload.

[nathanfallet]

If one of you wants to update the documentation, I let you do. Then, we will release a 0.13.1, and it will be okay (with #1075)

[groue]

If one of you wants to update the documentation, I let you do.

My job is done, now that #1075 has been merged. I do not know SQLite.swift enough in order to update the doc. I still get puzzled and I don't know how to write idiomatic SQLite.swift. I also did not check how many methods here need specific error handling and an updated documentation: prepare, prepareRowIterator, pluck, scalar, raw sql support... ? This is beyond my abilities.

Then, we will release a 0.13.1, and it will be okay (with #1075)

Yes, please!

[jberkel]

Let's do a larger doc overhaul in another release. There are some new features which aren't documented yet. #1076