sqlitebrowser / dbhub.io

A "Cloud" for SQLite databases. Collaborative development for your data. 😊
https://dbhub.io
GNU Affero General Public License v3.0
372 stars 39 forks source link

Minor bug when downloading files through DB4S #15

Closed justinclift closed 7 years ago

justinclift commented 7 years ago

When someone downloads a file from DBHub.io using DB4S, this is printed on the server side stdout (eg from the DB4S end point):

2017/03/16 19:56:39 Invalid database version number: strconv.ParseInt: parsing "": invalid syntax
2017/03/16 19:56:39 GET request handler:retrieveDatabase(): 'issue946.db' downloaded by user 'mkleusberg', 8192 bytes

strconv.ParseInt() isn't present anywhere in the DB4S end point code, so this is likely something in our shared library. At a guess, the end point code is probably expecting a version number for the database file, and getting it wrong somewhere from there.

Shouldn't be too hard to track down. :smile:

MKleusberg commented 7 years ago

Oh yeah that's probably because I've been requesting databases like this https://dev2.dbhub.io:5550/mkleusberg/issue946.db, without the ?version=1 bit. So it's probably this line causing the problem: https://github.com/sqlitebrowser/dbhub.io/blob/master/common/userinput.go#L258. It probably needs to get split up like this (pseudo code):

dbVersionString = r.FormValue("version")
if dbVersionString is not an empty string or an error or whatever happens {
    dbVersion, err := strconv.ParseInt(dbVersionString, 10, 0) // This also validates the version input
    if err != nil {
    log.Printf("Invalid database version number: %v\n", err)
        return 0, errors.New("Invalid database version number")
    }
} else {
    return 0, nil
}

In the DB4S end point code we should then check for version == 0 and handle that case gracefully because while we can adjust the code in DB4S we never know what other request people might send using their own software. I think when the version bit is missing in the request we should probably automatically serve the latest version of the file.

justinclift commented 7 years ago

Ahhh, yep, that totally makes sense.

The fix is pretty simple too. Coming up in a bit. :smile:

justinclift commented 7 years ago

For the case where a version number isn't specified, I'm thinking we should probably use the highest version number available to the requesting user.

That specific wording "available to the user" is for when there's a mix of public and private versions of a database. Some (eg private ones) might not be available to any random user. But they would be available to the person who owns them.

justinclift commented 7 years ago

And done. dev1 is now running this updated code too.

MKleusberg commented 7 years ago

Good thinking with the "available to the user" bit :+1: