db: check info_hash record not found instead dropping all errors

[da2ce7]

No description provided.

[josecelano]

hi @da2ce7, the reason I did it that way is that:

• Generally, I am uncomfortable changing the contracts without tests. So I tried not to change other parts. Your solution changes the contract for get_info_hash_from_whitelist in the SQLite version. It indeed seems nobody else is using that function (only the tracker), and it seems safe to change. In fact, with your change, you make the contract the same for both versions (MySQL and SQLite). Maybe my fear comes from working with dynamic languages :-).
• On the other hand, the method is a query that does not change the state. If it fails for another reason, we can try to insert the torrent in the whitelist. The SQL sentence for the "query" could be wrong, but the "insert" query could be OK. That was my thought (very odd thought since I usually prefer failing asap). Because I did not want to change the uncovered code, I was "forced" to introduce that "unexpected" behavior with code that does not reveal the intention. I apologize.

I think I should first evaluate the impact of changing uncovered code in the future.

I'm going to merge the PR. But I think what we really need to do is to add a new method like this:

async fn is_info_hash_whitelisted(info_hash: &str) -> Result<bool, database::Error>

so we can have this simpler version:

async fn add_torrent_to_database_whitelist(&self, info_hash: &InfoHash) -> Result<(), database::Error> {
    if self.database.is_info_hash_whitelisted(&info_hash.to_owned().to_string()) {
        // We do not need to whitelist it again
        Ok(())
    }
    self.database.add_info_hash_to_whitelist(*info_hash).await?;
    Ok(())
}

We can probably use the current get_info_hash_from_whitelistfuntion to implement the new one.

[josecelano]

ACK 167b749

[josecelano]

One more thing @da2ce7 do not you think we should simply "panic" when the SQL is wrong? I don't think we should try to handle those errors. I have to check that. It's strange that the DB package returns an error when the query does not have any result. I suppose the database trait should always return Option (for a single record result) or collection results.

[da2ce7]

@josecelano The db error InvalidQuery is used incorrectly/inappropriately throughout the entire mysql.rs and sqlite.rs, the error-handling in both of these files really needs a complete overhaul.

You are very correct to suggest that the DB should be rerunning an Optional Value to indicate that there was no record. – The code in this pull request is simply the minimal changes, without changing any of the interface... I just edited the db code because to correct their "unexpected" behavior, i.e. they didn't follow a reasonable interpenetration of their own contracts.