Missing timestamps #19 #22
Conversation
This will be needed to deduce what unit timestamps use in next commit.
Since sqlite3 schemaversion 31, timestamps are in milliseconds (see 56f686f7 in quassel). As a result, timestamps were broken on sqlite3 databases >= 31. This fixes issue fish-face#19.
There was a problem hiding this comment.
OUTDATED - see follow-up review!
Summary
- ❌ Crash issue due to
strvsint, fix provided in comments - 🏁 Works with fix! Same time before/after upgrade.
Test results with fix applied
These test results are with the commented fix applied. Without the fix, quasselgrep instead crashes.
Quassel 0.10 database (schema version 17)
An SQLite database backup from before I experimented with Unicode channel names.
(Quassel handled it just fine, and the later switch to UTF-8 by default for IRC servers fixed the underlying issue.)
$ sqlite3 ../quassel-storage-2014-7-22th-disconnected.sqlite 'SELECT value FROM coreinfo WHERE key="schemaversion"'
17
$ python3 -m quasselgrep --dbname ../quassel-storage-2014-7-22th-disconnected.sqlite --limit 1 "Quassel"
(buffer_name) [2014-07-22 05:01:33] <digitalcircuit> Though, honestly, if it breaks Quassel, I will be very disappointed. […]
Quassel 0.13.1 database (schema version 31)
Same database, merely upgraded by running a newer Quassel core against it.
$ sqlite3 ../quassel-storage-2014-7-22th-disconnected-0.13.1.sqlite 'SELECT value FROM coreinfo WHERE key="schemaversion"'
31
$ python3 -m quasselgrep --dbname ../quassel-storage-2014-7-22th-disconnected-0.13.1.sqlite --limit 1 "Quassel"
(buffer_name) [2014-07-22 05:01:33] <digitalcircuit> Though, honestly, if it breaks Quassel, I will be very disappointed. […]
Results
Success! Same timestamp.
quasselgrep/db.py
Outdated
| results = cursor.fetchall() | ||
| if len(results) != 1: | ||
| raise ValueError('Incorrect sqlite schemaversion format') | ||
| options.sqlite_version = results[0][0] |
There was a problem hiding this comment.
This results in comparing a str with an int later, for example…
File "$HOME/Desktop/fdsa/quasselgrep-sqlite3_ts_ms/quasselgrep/query.py", line 145, in columns
if self.options.sqlite_version >= 31:
TypeError: '>=' not supported between instances of 'str' and 'int'
NOTE: I would suggest providing some form of fallback or catch in case the results cannot be casted to an integer. Quassel should keep schema versions as whole numbers, but guarding against future changes with an informative error is probably a good idea.
| options.sqlite_version = results[0][0] | |
| try: | |
| # Schema version should be an integer, but this isn't guaranteed | |
| options.sqlite_version = int(results[0][0]) | |
| except ValueError as ex: | |
| raise ValueError('Unexpected sqlite schemaversion "{0}", not an integer'.format(results[0][0])) from ex |
With this fix, your changes work on an old and new SQLite database!
Hypothetical robustness test
Replacing the result with the string test-unexpected confirms that this error handling works.
Error connecting to database: Unexpected sqlite schemaversion "test-unexpected", not an integer
There was a problem hiding this comment.
Thanks for testing! I run quasselgrep with python2 (because this is what the shebang points to) where the crash doesn't happen. Changing the shebang to python3 doesn't work on my distro (pkg_resources.DistributionNotFound).
Anyhow, the "from ex" syntax isn't python2 compatible. Therefore I've slightly modified your patch by dropping "from ex" and adapting your ValueError string to match the global coding style (e instead of ex, '%' instead of format()). I've also tried to replace the result with the string '1.0.x' and it works:
Error connecting to database: Unexpected sqlite schemaversion 1.0.x, not an integer: invalid literal for int() with base 10: '1.0.x'
Perhaps you can retest with python3 to make sure I didn't break anything?
There was a problem hiding this comment.
I missed this reply until now - that makes sense! I overlooked Python 2 support, thank you for catching that, too.
As retested earlier, this looks good!
Python3 requires a cast of options.sqlite_version to int, otherwise the following
crash happens:
File "$HOME/Desktop/fdsa/quasselgrep-sqlite3_ts_ms/quasselgrep/query.py", line 145, in columns
if self.options.sqlite_version >= 31:
TypeError: '>=' not supported between instances of 'str' and 'int'
Also raise an exception with an explicit error message in case quassel ever decides
to introduce characters in its schema version.
There was a problem hiding this comment.
Summary
- 🏁 Works! Same time before/after upgrade.
Test results
Quassel 0.10 database (schema version 17)
$ sqlite3 ../quassel-storage-2014-7-22th-disconnected.sqlite 'SELECT value FROM coreinfo WHERE key="schemaversion"'
17
$ python3 -m quasselgrep --dbname ../quassel-storage-2014-7-22th-disconnected.sqlite --limit 1 "Quassel"
(buffer_name) [2014-07-22 05:01:33] <digitalcircuit> Though, honestly, if it breaks Quassel, I will be very disappointed. […]
Quassel 0.14.0rc1 database (schema version 32)
Same database, merely upgraded by running a newer Quassel core against it.
$ sqlite3 ../quassel-storage-2014-7-22th-disconnected-0.14.0rc1.sqlite 'SELECT value FROM coreinfo WHERE key="schemaversion"'
32
$ python3 -m quasselgrep --dbname ../quassel-storage-2014-7-22th-disconnected-0.14.0rc1.sqlite --limit 1 "Quassel"
(buffer_name) [2014-07-22 05:01:33] <digitalcircuit> Though, honestly, if it breaks Quassel, I will be very disappointed. […]
Results
Success! Same timestamp.
|
Hello, anything preventing the merge :)? |
This PR fixes issue #19:
For sqlite3 databases, we read schemaversion on connect and store it into the options object. That way, we can deduce the timestamp unit (milliseconds or seconds) from other files. This approach was suggested by @digitalcircuit in #19 [1].
I've just started using quasselgrep, so perhaps I missed something. Let me know if that's the case. Also, I cannot test with databases < 31, but old behavior should be left untouched in that case.
[1] #19 (comment)