1226461 - Use IN when checking for tables

Status: RESOLVED FIXED

(Firefox for iOS :: Data Storage, defect)

Description

[Richard Newman [:rnewman]]

extension SQLiteDBConnection {
    func tablesExist(names: Args) -> Bool {
        let count = names.count
        let orClause = Array(count: count, repeatedValue: "name = ?").joinWithSeparator(" OR ")
        let tablesSQL = "SELECT name FROM sqlite_master WHERE type = 'table' AND (\(orClause))"

        let res = self.executeQuery(tablesSQL, factory: StringFactory, withArgs: names)
        log.debug("\(res.count) tables exist. Expected \(count)")
        return res.count > 0
    }
}

Running query "SELECT name FROM sqlite_master WHERE type = 'table' AND (name = ? OR name = ? OR name = ? OR name = ? OR name = ? OR name = ? OR name = ? OR name = ? OR name = ? OR name = ? OR name = ? OR name = ? OR name = ?);

This is what SQL 'IN' is for. There are great examples elsewhere in the codebase:

https://github.com/mozilla/firefox-ios/blob/1e57eac39b9488c3747a76dc63f8b0c58f74c007/Storage/SQL/SQLiteHistory.swift#L903

Comment 1

[Roland Demeter]

I was not sure if the same "BrowserDB" will be used in this case. I hope it helps.


extension SQLiteDBConnection {
    func tablesExist(names: Args) -> Bool {
        let count = names.count
        let orClause = BrowserDB.varlist(names.count)
        let tablesSQL = "SELECT name FROM sqlite_master WHERE type = 'table' AND name IN (\(orClause))"

        let res = self.executeQuery(tablesSQL, factory: StringFactory, withArgs: names)
        log.debug("\(res.count) tables exist. Expected \(count)")
        return res.count > 0
    }
}

Comment 2

[Roland Demeter]

These orClauses could be also written as inClauses in this case to keep it clean, which I forgot.

extension SQLiteDBConnection {
    func tablesExist(names: Args) -> Bool {
        let count = names.count
        let inClause = BrowserDB.varlist(names.count)
        let tablesSQL = "SELECT name FROM sqlite_master WHERE type = 'table' AND name IN (\(inClause))"

        let res = self.executeQuery(tablesSQL, factory: StringFactory, withArgs: names)
        log.debug("\(res.count) tables exist. Expected \(count)")
        return res.count > 0
    }
}

Comment 3

[Richard Newman [:rnewman]]

Roland, please take a look at <https://github.com/mozilla/firefox-ios#creating-a-pull-request> and submit a pull request with your changes.

You should also take that opportunity to make sure the project still builds and tests still pass. Thanks!

Comment 4

[Roland Demeter]

Hello Richard! I am not sure how to proceed with the pull request. I am not sure what the base and compare option is under "Compare changes".

Comment 5

[Richard Newman [:rnewman]]

One side is 'master' in the main repo.

The other is your feature branch in your fork. So you take the usual GitHub approach of:

* Fork the repo.
* Clone it.
* Check out a branch to work in: `git checkout -b roland/bug-1226461`
* Make your changes and commit your work: `git commit -m "Bug 1226461 - Use IN when checking for tables."`
* Push your branch to GitHub: `git push origin roland/bug-1226461`
* Create the PR back to our `master` branch from your feature branch.

The GitHub docs cover this in more detail:

https://help.github.com/articles/fork-a-repo/

Comment 6

[Roland Demeter]

Thank you Richard.

I have made the changes and created a PR.
https://github.com/roliusz/firefox-ios/pull/1

Thanks!

Comment 7

[Richard Newman [:rnewman]]

Attachment: Pull req.

Comment 8

[Richard Newman [:rnewman]]

(Throwing this at Steph to review so he has an opportunity to get more familiar with this code.)

Comment 9

[Stephan Leroux [:sleroux]]

Comment on attachment 8697591 [details] [review]
Pull req.

Looks good - thanks for the patch!

Comment 10

[Richard Newman [:rnewman]]

Steph, could you rebase and land this?

Comment 11

[Stephan Leroux [:sleroux]]

master 83e21f15a3784253715d6acaa7d64ee241a84137