Refactor sqlthread #1794

cis-muzahid · 2021-07-28T08:33:50Z

Refactor sqlthread and add test cases for setting versions and SQL internal function called create_function

src/class_sqlThread.py

PeterSurda · 2021-07-28T08:41:02Z

src/class_sqlThread.py

+        Upgrade Db with respect to versions
+    """
+
+    conn, cur = connection_build()


here allow to call it optionally with a parameter for in memory database

pass arguments with DB file name

PeterSurda · 2021-07-28T08:42:48Z

src/class_sqlThread.py

-        self.conn = sqlite3.connect(state.appdata + 'messages.dat')
-        self.conn.text_factory = str
-        self.cur = self.conn.cursor()
+        self.conn, self.cur = connection_build()


this should be in parent class

Inherit connection vars from UpgradeDB class

Moved in to __init__

PeterSurda · 2021-07-28T08:43:00Z

src/class_sqlThread.py

-            self.conn.commit()
-
-            settingsversion = 4
+        settingsversion = self.earlier_setting_version(settingsversion)


this should be in parent class

src/class_sqlThread.py

PeterSurda · 2021-07-28T08:43:27Z

src/class_sqlThread.py

@@ -465,10 +349,14 @@ def run(self):  # pylint: disable=too-many-locals, too-many-branches, too-many-s
            else:
                logger.error(err)

-        # Let us check to see the last time we vaccumed the messages.dat file.
-        # If it has been more than a month let's do it now.
+    def check_vaccumed(self):  # pylint: disable=too-many-locals, too-many-branches, too-many-statements,


move to parent

vacuum not vaccum

PeterSurda · 2021-07-28T08:43:36Z

src/class_sqlThread.py

@@ -635,8 +523,45 @@ def run(self):  # pylint: disable=too-many-locals, too-many-branches, too-many-s
                helper_sql.sqlReturnQueue.put((self.cur.fetchall(), rowcount))
                # helper_sql.sqlSubmitQueue.task_done()

+    def earlier_setting_version(self, settingsversion):


move to parent

different name, to show it's related to BMConfigParser version rather than sql version

Change name to upgrade_config_parser_setting_version

PeterSurda · 2021-07-28T08:44:24Z

src/class_sqlThread.py


-    def run(self):  # pylint: disable=too-many-locals, too-many-branches, too-many-statements
+    def run(self):  # pylint: disable=too-many-locals, too-many-branches, too-many-statements,


only this should be in this class, the other functionality should be refactored into methods and put into parent.

PeterSurda · 2021-07-28T08:45:55Z

src/class_sqlThread.py

+        self.cur.execute(item, parameters)
+
+
+class sqlThread(threading.Thread, UpgradeDB):


maybe an UpgradeDB object would be an attribute of the sqlThread rarther than sqlThread inheriting it. That way we isolate the DB stuff and can test it without threading, and the thread would need to use the same interfaces as the test.

Changes done,
Refactored code and kept in Upgrade DB,

src/class_sqlThread.py

PeterSurda · 2021-08-05T08:26:23Z

src/class_sqlThread.py

+    def __init__(self):
+        self._current_level = None
+        self.max_level = 11
+        # self.conn, self.cur = connection_build('messages.dat')


make another class which overrides this to point to in-memory database, this can then be used for tests. You can call it something like class MockDB(UpgradeDB)

PeterSurda · 2021-08-05T08:26:37Z

src/class_sqlThread.py


-    def __init__(self):
-        threading.Thread.__init__(self, name="SQL")
+def connection_build(file_name):


Put this inside UpgradeDB

PeterSurda · 2021-08-05T08:27:11Z

src/class_sqlThread.py

+
+    def __get_current_settings_version(self):
+        """
+            Upgrade Db with respect to their versions


update docstring

PeterSurda · 2021-08-05T08:27:31Z

src/class_sqlThread.py


+    def _upgrade_one_level_sql_statement(self, file_name):
+        """
+            Execute SQL files and queries


update docstring

PeterSurda · 2021-08-05T08:43:21Z

src/class_sqlThread.py


-        helper_startup.updateConfig()
+    def embaded_version(self):


should be called something like init_database

Changes are done for that and, but for some query I applied same query script.

let's call it upgrade_schema_if_old_version

Changed def name to upgrade_schema_if_old_version

PeterSurda · 2021-08-05T08:45:12Z

src/class_sqlThread.py

@@ -635,8 +523,45 @@ def run(self):  # pylint: disable=too-many-locals, too-many-branches, too-many-s
                helper_sql.sqlReturnQueue.put((self.cur.fetchall(), rowcount))
                # helper_sql.sqlSubmitQueue.task_done()

+    def earlier_setting_version(self, settingsversion):


different name, to show it's related to BMConfigParser version rather than sql version

PeterSurda · 2021-08-05T08:47:10Z

src/class_sqlThread.py

+        # If the settings version is equal to 2 or 3 then the
+        # sqlThread will modify the pubkeys table and change
+        # the settings version to 4.
+        settingsversion = BMConfigParser().getint(


write getter + setter for this

Applied setter and getter

PeterSurda · 2021-08-05T08:50:14Z

src/class_sqlThread.py

+        # call create_function for encode address
+        self.create_function(self.conn)
+
+        self.initiate_database(self.cur, self.conn)


Something like this to skip init when running tests:

if not self.instanceof(MockDB): self.initiate_database()

managed in earlier logic used in memory for test mocking

src/debug.py

g1itch · 2021-08-09T10:10:25Z

Your sql directories are not python packages and should not contain the empty __init__ modules. If you want to install SQL scripts with the app, put them into the package data: https://github.com/Bitmessage/PyBitmessage/blob/v0.6/setup.py#L137
This doesn't apply to tests, they are not installed, and you may find the files based on os.path.abspath(__file__)

g1itch · 2021-08-09T10:14:24Z

src/tests/sql/init_version_5.sql

@@ -0,0 +1,10 @@
+DROP TABLE objectprocessorqueue;
+
+CREATE TABLE IF NOT EXISTS `knownnodes` (


Cool (; I haven't seen this used

$ sqlite3 .config/PyBitmessage/messages.dat SQLite version 3.34.1 2021-01-20 14:10:07 Enter ".help" for usage hints. sqlite> select * from knownnodes; Error: no such table: knownnodes

I think the table was deleted in a later version.

It looks like some weird leftover of deleted code. Since the current upgrade code deletes it, we should keep it, otherwise we'd have to test against ancient versions.

g1itch · 2021-08-09T10:16:29Z

src/tests/sql/init_version_5.sql

@@ -0,0 +1,10 @@
+DROP TABLE objectprocessorqueue;


Why drop here, if you are going to create it later?

you're right, if it's removed and there is an error, it means there is a bug somewhere

g1itch · 2021-08-09T10:19:59Z

src/tests/sql/init_version_7.sql

+  `payload` blob DEFAULT NULL,
+  `integer` integer NOT NULL,
+  UNIQUE(hash) ON CONFLICT REPLACE
+) ;


I don't see a difference from the previous version - in the init_version_6.sql

I haven't reviewed the SQL files yet, I'm focusing on getting the structure of classes and tests right, it's already at like 3rd iteration.

I looked through the tests and instructed to redesign how they perform setup.

PeterSurda · 2021-08-09T10:26:02Z

@g1itch This PR should among other things separate the SQL execution from threads, so that you can run the tests without launching the SQL thread. There is a lot of work and that's why it's taking long.

g1itch · 2021-08-09T10:43:07Z

@g1itch This PR should among other things separate the SQL execution from threads, so that you can run the tests without launching the SQL thread. There is a lot of work and that's why it's taking long.

I see. I have no problem with the speed yet. I'm glad to see many line of code deleted in this PR. But I'd prefer to report the strange things, I can see in the code before it got merged. Because I believe we can broke it harder than before. More tests will help to avoid such case.

g1itch · 2021-08-09T10:48:06Z

By the way, the core test suite reports an interesting issue here: https://buildbot.bitmessage.org/#builders/25/builds/1515

PeterSurda · 2021-08-18T08:45:55Z

src/class_sqlThread.py

+        self.max_level = 11
+        self.conn, self.cur = self.connection_build('messages.dat')
+
+    def connection_build(self, file_name=None):


def connection_build(self, file_name='messages.dat'):

applied changes method name to __connection_build_internal and pass file_name

PeterSurda · 2021-08-18T08:48:05Z

src/class_sqlThread.py

+        return self.__get_current_settings_version()
+
+    @sql_schema_version.setter
+    def sql_schema_version(self, current_level):


instead of current_level use ignore

removed current_level

PeterSurda · 2021-08-18T08:51:37Z

src/class_sqlThread.py

+        item = '''update settings set value=? WHERE key='version';'''
+        parameters = (current_level + 1,)
+        self.cur.execute(item, parameters)
+        self._current_level = self.__get_current_settings_version()


variable self.current_level shouldn't be needed

Now its

def sql_schema_version(self): """ Update version with one level """ item = '''update settings set value=value+1 WHERE key='version';''' self.cur.execute(item) self._current_level = self.__get_current_settings_version()

Its changed into

self.cur.execute(item, self.__get_current_settings_version() + 1)

Because its not taking value+1

PeterSurda · 2021-08-18T08:56:22Z

src/class_sqlThread.py

+            self.cur.execute(item, parameters)
+            self.conn.commit()
+
+            settingsversion = 3


separate getter setter for settingsversion

PeterSurda · 2021-08-18T08:58:28Z

src/class_sqlThread.py

+        # Redefinition-of-parameters-type-from-tuple-to-str, R0204, line-too-long, E501
+        """Process SQL queries from `.helper_sql.sqlSubmitQueue`"""
+        helper_sql.sql_available = True
+        self.conn, self.cur = UpgradeDB().conn, UpgradeDB().cur


insetad just create one object and save it, like

self.db = UpgradeDB()

and then just reference
self.db.cur.execute(blabla)

passed while initialise

PeterSurda · 2021-08-18T08:59:11Z

src/class_sqlThread.py

+        self.cur.execute('PRAGMA secure_delete = true')
+
+        # call create_function for encode address
+        self.create_function(self.conn)


self.db.create_function(blabla)

PeterSurda · 2021-08-18T08:59:44Z

src/class_sqlThread.py

+        # call create_function for encode address
+        self.create_function(self.conn)
+
+        self.initiate_database(self.cur, self.conn)


self.db.initialize_schema()

Changed into initialize_schema

PeterSurda · 2021-08-18T08:59:58Z

src/class_sqlThread.py

+        # version we are on can stay embedded in the messages.dat file. Let us
+        # check to see if the settings table exists yet.
+
+        self.embaded_version()


changed to upgrade_schema_if_old_version

g1itch

Barely readable ); The tests/sql seems duplicating sql

src/tests/sql/init_version_7.sql

g1itch · 2021-11-24T16:41:02Z

setup.py

@@ -137,7 +137,7 @@ def run(self):
        package_data={'': [
            'bitmessageqt/*.ui', 'bitmsghash/*.cl', 'sslkeys/*.pem',
            'translations/*.ts', 'translations/*.qm',
-            'images/*.png', 'images/*.ico', 'images/*.icns'
+            'images/*.png', 'images/*.ico', 'images/*.icns', 'sql/*.py', 'sql/*.sql'


I don't understand the purpose of this empty sql/__init__.py and especially listing it in the package_data.

The file was removed, but the reference in setup.py still persists. @cis-muzahid please delete this reference, and also delete the file src/tests/sql/__init__.py, that is also unnecessary.

g1itch · 2021-11-24T16:41:19Z

src/class_sqlThread.py

@@ -8,6 +8,7 @@
 import sys
 import threading
 import time
+import logging


g1itch · 2021-11-24T16:42:13Z

src/class_sqlThread.py

+class BitmessageDB(object):
+    """
+        Upgrade Db with respect to versions
+    """


strange formatting

g1itch · 2021-11-24T16:43:11Z

src/class_sqlThread.py

+        """
+            Get current setting Version
+        """
+        item = '''SELECT value FROM settings WHERE key='version';'''


No need for semicolon

There was a problem, we found out what it is, will be fixed in next commit.

g1itch · 2021-11-24T16:54:43Z

src/class_sqlThread.py

+    @property
+    def sql_config_settings_version(self):
+        """
+            Getter for BmConfigparser


BMConfigParser (obj)

g1itch · 2021-11-24T16:57:10Z

src/class_sqlThread.py

+                                "MainWindow",
+                                'Alert: Your disk or data storage volume is full. Bitmessage will now exit.'),
+                            True)))
+                    os._exit(0)


Weird formatting

g1itch · 2021-11-24T16:58:41Z

src/class_sqlThread.py

+                        ' Here is the actual error message thrown by the sqlThread: %s',
+                        str(item),
+                        str(repr(parameters)),
+                        str(err))


These casts seem redundant

I tend to agree. Something like traceback.format_exc would be better.

g1itch · 2021-11-24T17:02:15Z

src/tests/core.py

+        queryreturn = sqlExecute("INSERT INTO testtbl VALUES(101);")
+        self.assertEqual(queryreturn, 1)
+
+        queryreturn = sqlQuery(''' SELECT * FROM testtbl ''')


"SELECT * FROM testtbl"

g1itch · 2021-11-24T17:03:44Z

src/tests/core.py

@@ -409,6 +409,22 @@ def test_adding_two_same_case_sensitive_addresses(self):
        self.delete_address_from_addressbook(address1)
        self.delete_address_from_addressbook(address2)

+    def test_sqlscripts(self):
+        """ Testing sql statements"""


What it tests?

g1itch

I see no point in another review since my previous comments are mostly ignored.

PeterSurda · 2021-12-10T07:26:22Z

src/class_sqlThread.py

+            parameters = helper_sql.sqlSubmitQueue.get()
+            rowcount = 0
+            try:
+                self.db.cur.execute(item, parameters)


item is a bad name. Use query.

PeterSurda

There are still a couple of unresolved conversations.

PeterSurda · 2022-01-18T05:18:47Z

setup.py

@@ -137,7 +137,7 @@ def run(self):
        package_data={'': [
            'bitmessageqt/*.ui', 'bitmsghash/*.cl', 'sslkeys/*.pem',
            'translations/*.ts', 'translations/*.qm',
-            'images/*.png', 'images/*.ico', 'images/*.icns'
+            'images/*.png', 'images/*.ico', 'images/*.icns', 'sql/*.py', 'sql/*.sql'


The file was removed, but the reference in setup.py still persists. @cis-muzahid please delete this reference, and also delete the file src/tests/sql/__init__.py, that is also unnecessary.

PeterSurda · 2022-01-18T05:22:19Z

src/class_sqlThread.py

+                                "MainWindow",
+                                'Alert: Your disk or data storage volume is full. Bitmessage will now exit.'),
+                            True)))
+                    os._exit(0)


PeterSurda

The PR shouldn't remove the *.sql files. It should ignore them.

PeterSurda

unstage the sql files

PeterSurda marked this pull request as draft July 28, 2021 08:35

PeterSurda reviewed Jul 28, 2021

View reviewed changes

src/class_sqlThread.py Outdated Show resolved Hide resolved

PeterSurda reviewed Jul 28, 2021

View reviewed changes

src/class_sqlThread.py Show resolved Hide resolved

PeterSurda reviewed Jul 28, 2021

View reviewed changes

cis-muzahid force-pushed the test_refactor_sqlthread branch from 0f93137 to 8f544a2 Compare July 29, 2021 07:54

cis-muzahid changed the title ~~Test refactor sqlthread~~ Refactor sqlthread Jul 30, 2021

cis-muzahid requested a review from PeterSurda July 30, 2021 19:39

cis-muzahid force-pushed the test_refactor_sqlthread branch from 44654df to f0044d3 Compare August 5, 2021 07:49

PeterSurda requested changes Aug 5, 2021

View reviewed changes

g1itch reviewed Aug 9, 2021

View reviewed changes

PeterSurda reviewed Aug 18, 2021

View reviewed changes

cis-muzahid mentioned this pull request Nov 8, 2021

Refactor sqlthread #1760

Closed

cis-muzahid force-pushed the test_refactor_sqlthread branch from 79ebee5 to 7240658 Compare November 11, 2021 07:22

PeterSurda mentioned this pull request Nov 16, 2021

Added mock code #1883

Draft

cis-muzahid force-pushed the test_refactor_sqlthread branch 2 times, most recently from d5296dd to a55c086 Compare November 24, 2021 09:39

g1itch reviewed Nov 24, 2021

View reviewed changes

cis-muzahid force-pushed the test_refactor_sqlthread branch from a55c086 to b3d6dc4 Compare November 26, 2021 07:37

cis-muzahid requested review from g1itch and PeterSurda December 8, 2021 08:14

g1itch reviewed Dec 9, 2021

View reviewed changes

PeterSurda reviewed Dec 10, 2021

View reviewed changes

cis-muzahid force-pushed the test_refactor_sqlthread branch from dd64925 to bb88554 Compare December 20, 2021 14:49

cis-muzahid requested review from g1itch and PeterSurda December 20, 2021 14:58

PeterSurda requested changes Jan 18, 2022

View reviewed changes

cis-muzahid force-pushed the test_refactor_sqlthread branch from c531e62 to 533df72 Compare January 18, 2022 14:05

cis-muzahid force-pushed the test_refactor_sqlthread branch from 533df72 to 899d28f Compare January 27, 2022 16:44

cis-muzahid force-pushed the test_refactor_sqlthread branch 3 times, most recently from 2b89a05 to d6acbc3 Compare February 23, 2022 10:39

cis-muzahid force-pushed the test_refactor_sqlthread branch 2 times, most recently from c953c10 to de89853 Compare March 16, 2022 09:27

PeterSurda requested changes Mar 16, 2022

View reviewed changes

PeterSurda reviewed Mar 17, 2022

View reviewed changes

kdcis force-pushed the test_refactor_sqlthread branch 2 times, most recently from 676025a to f61b1c1 Compare March 25, 2022 17:00

kdcis force-pushed the test_refactor_sqlthread branch from 4267c44 to a13a37d Compare April 14, 2022 10:20

refactor sqlthread and add create test cases

3720c3f

kdcis force-pushed the test_refactor_sqlthread branch from 38f7628 to 3720c3f Compare May 13, 2022 14:12

PeterSurda mentioned this pull request May 29, 2024

msgid is stored as text type #2247

Open


		def run(self): # pylint: disable=too-many-locals, too-many-branches, too-many-statements
		def run(self): # pylint: disable=too-many-locals, too-many-branches, too-many-statements,

		self.cur.execute(item, parameters)


		class sqlThread(threading.Thread, UpgradeDB):

		@@ -0,0 +1,10 @@
		DROP TABLE objectprocessorqueue;

		CREATE TABLE IF NOT EXISTS `knownnodes` (

Refactor sqlthread #1794

Are you sure you want to change the base?

Refactor sqlthread #1794

Conversation

cis-muzahid commented Jul 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

g1itch commented Aug 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PeterSurda commented Aug 9, 2021

g1itch commented Aug 9, 2021

g1itch commented Aug 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PeterSurda Aug 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

g1itch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cis-muzahid commented Jul 28, 2021 •

edited

Loading

PeterSurda Aug 18, 2021 •

edited

Loading