Initial support for SQLite database backend, fixes #56 #57

AnnoyingTechnology · 2023-03-21T14:28:54Z

Draft

AnnoyingTechnology · 2023-03-21T15:46:56Z

This seems to be working pretty well now.

liuch · 2023-03-23T05:58:02Z

Hello @AnnoyingTechnology, Sorry for the delay in reply. I'm a bit busy right now, but I promise, I'll take a look at your PR soon.

AnnoyingTechnology · 2023-03-23T06:28:44Z

Hello @AnnoyingTechnology, Sorry for the delay in reply. I'm a bit busy right now, but I promise, I'll take a look at your PR soon.

no worries

liuch · 2023-03-27T19:57:57Z

I don't think you've tested your code with the table prefix option. I'm right?

AnnoyingTechnology · 2023-03-27T21:53:06Z

I don't think you've tested your code with the table prefix option. I'm right?

Correct. As SQLite isn't a DBMS the file is the database and the need for prefix to avoid collisions when sharing a single database for multiple uses doesn't exist.

There is still a real issue though, I removed an order by somewhere that should have been kept. But at the time I couldn't make it work properly.

liuch

You've done a great job. Thank you for the PR. I have a few comments besides ones I left in your code:

Your code needs to be tested with the table prefix option enabled with one or more other tables without the prefix.
You have used spaces and tabs for indentation in your php code. Could you use spaces instead of tabs, please? I am sticking to PSR-2 in the php code of this project.
In all the new files that I saw, you have not been mentioned as the author of the code. Maybe you forgot to change it?

I haven't tested your code on a server with reports yet. Perhaps after these tests I will have some questions and comments.

liuch · 2023-03-27T16:56:49Z

classes/Database/DatabaseController.php

+                    break;
+				case 'sqlite':
+                    $type = 'sqlite';


Suggested change

break;

case 'sqlite':

$type = 'sqlite';

break;

case 'sqlite':

$type = 'sqlite';

liuch · 2023-03-27T20:00:34Z

classes/Database/Sqlite/Connector.php

+            ],
+            'additional' => '',
+            'table_options' => '',
+			'sub_queries' => []


You may have forgotten to create an unique index on the fqdn field.

liuch · 2023-03-27T20:02:24Z

classes/Database/Sqlite/Connector.php

+            );
+            while ($row = $st->fetch(\PDO::FETCH_ASSOC)) {
+                $tname = $row['name'];
+                $rcnt  = $this->dbh->query('SELECT COUNT(*) FROM ' . $tname . '')->fetch(\PDO::FETCH_NUM)[0];


Suggested change

$rcnt = $this->dbh->query('SELECT COUNT(*) FROM ' . $tname . '')->fetch(\PDO::FETCH_NUM)[0];

$rcnt = $this->dbh->query('SELECT COUNT(*) FROM ' . $tname)->fetch(\PDO::FETCH_NUM)[0];

liuch · 2023-03-27T20:03:12Z

classes/Database/Sqlite/Connector.php

+            $db->query('PRAGMA FOREIGN_KEYS = OFF');
+            $st = $db->query($this->sqlShowTablesQuery());
+            while ($table = $st->fetchColumn(0)) {
+                $db->query('DROP TABLE ' . $table . '');


Suggested change

$db->query('DROP TABLE ' . $table . '');

$db->query('DROP TABLE ' . $table);

liuch · 2023-03-27T20:04:23Z

classes/Database/Sqlite/Connector.php

+            if ($col_num > 0) {
+                $query .= ', ';
+            }
+            $query .= '' . $column['name'] . ' ' . $column['definition'];


Suggested change

$query .= '' . $column['name'] . ' ' . $column['definition'];

$query .= $column['name'] . ' ' . $column['definition'];

liuch · 2023-03-27T21:20:01Z

classes/Database/Sqlite/ReportMapper.php

+        try {
+
+            $st = $db->prepare(
+				'SELECT org, begin_time, end_time, fqdn, external_id, seen, SUM(rcount) AS rcount,'
+				. ' MIN(dkim_align) AS dkim_align, MIN(spf_align) AS spf_align,'
+				. ' MIN(disposition) AS disposition FROM ' . $this->connector->tablePrefix('reports')
+				. ' AS rp LEFT JOIN ' . $this->connector->tablePrefix('rptrecords')
+				. ' AS rr ON rp.id = rr.report_id'
+				. ' INNER JOIN ' . $this->connector->tablePrefix('domains')
+				. ' AS d ON d.id = rp.domain_id'
+				. ' GROUP BY rp.id'
+				. $cond_str1 . $order_str . $limit_str
+			);


Suggested change

try {

$st = $db->prepare(

'SELECT org, begin_time, end_time, fqdn, external_id, seen, SUM(rcount) AS rcount,'

. ' MIN(dkim_align) AS dkim_align, MIN(spf_align) AS spf_align,'

. ' MIN(disposition) AS disposition FROM ' . $this->connector->tablePrefix('reports')

. ' AS rp LEFT JOIN ' . $this->connector->tablePrefix('rptrecords')

. ' AS rr ON rp.id = rr.report_id'

. ' INNER JOIN ' . $this->connector->tablePrefix('domains')

. ' AS d ON d.id = rp.domain_id'

. ' GROUP BY rp.id'

. $cond_str1 . $order_str . $limit_str

);

try {

$st = $db->prepare(

'SELECT org, begin_time, end_time, fqdn, external_id, seen, SUM(rcount) AS rcount,'

. ' MIN(dkim_align) AS dkim_align, MIN(spf_align) AS spf_align,'

. ' MIN(disposition) AS disposition FROM ' . $this->connector->tablePrefix('reports')

. ' AS rp LEFT JOIN ' . $this->connector->tablePrefix('rptrecords')

. ' AS rr ON rp.id = rr.report_id'

. ' INNER JOIN ' . $this->connector->tablePrefix('domains')

. ' AS d ON d.id = rp.domain_id'

. ' GROUP BY rp.id'

. $cond_str1 . $order_str . $limit_str

);

liuch · 2023-03-27T21:22:20Z

classes/Database/Sqlite/ReportMapper.php

+		    $st = $this->connector->dbh()->query(
+				'SELECT DISTINCT strftime("%Y-%m", date) AS month FROM'
+				. ' (SELECT DISTINCT begin_time AS date FROM ' . $rep_tn
+				. ' UNION SELECT DISTINCT end_time AS date FROM ' . $rep_tn
+				. ') AS r ORDER BY month DESC'
+			);


Suggested change

$st = $this->connector->dbh()->query(

'SELECT DISTINCT strftime("%Y-%m", date) AS month FROM'

. ' (SELECT DISTINCT begin_time AS date FROM ' . $rep_tn

. ' UNION SELECT DISTINCT end_time AS date FROM ' . $rep_tn

. ') AS r ORDER BY month DESC'

);

$st = $this->connector->dbh()->query(

'SELECT DISTINCT strftime("%Y-%m", date) AS month FROM'

. ' (SELECT DISTINCT begin_time AS date FROM ' . $rep_tn

. ' UNION SELECT DISTINCT end_time AS date FROM ' . $rep_tn

. ') AS r ORDER BY month DESC'

);

liuch · 2023-03-27T21:34:53Z

classes/Database/Sqlite/StatisticsMapper.php

+            $st->closeCursor();
+
+            $st = $db->prepare(
+                'SELECT COUNT(*) FROM (SELECT org FROM ' . $this->connector->tablePrefix('reports') . ''


Suggested change

'SELECT COUNT(*) FROM (SELECT org FROM ' . $this->connector->tablePrefix('reports') . ''

'SELECT COUNT(*) FROM (SELECT org FROM ' . $this->connector->tablePrefix('reports')

liuch · 2023-03-27T21:39:22Z

classes/Database/Sqlite/StatisticsMapper.php

+				'SELECT ip, SUM(rcount) AS rcount, SUM(CASE WHEN dkim_align = 2 THEN rcount ELSE 0 END) AS dkim_aligned,'
+				. ' SUM(CASE WHEN spf_align = 2 THEN rcount ELSE 0 END) AS spf_aligned'
+				. ' FROM ' . $this->connector->tablePrefix('rptrecords') . ' AS rr'
+				. ' INNER JOIN ' . $this->connector->tablePrefix('reports')
+				. ' AS rp ON rr.report_id = rp.id'
+				. $this->sqlCondition($domain ? true : false) . ' GROUP BY ip ORDER BY rcount DESC'
+			);


Suggested change

'SELECT ip, SUM(rcount) AS rcount, SUM(CASE WHEN dkim_align = 2 THEN rcount ELSE 0 END) AS dkim_aligned,'

. ' SUM(CASE WHEN spf_align = 2 THEN rcount ELSE 0 END) AS spf_aligned'

. ' FROM ' . $this->connector->tablePrefix('rptrecords') . ' AS rr'

. ' INNER JOIN ' . $this->connector->tablePrefix('reports')

. ' AS rp ON rr.report_id = rp.id'

. $this->sqlCondition($domain ? true : false) . ' GROUP BY ip ORDER BY rcount DESC'

);

'SELECT ip, SUM(rcount) AS rcount, SUM(CASE WHEN dkim_align = 2 THEN rcount ELSE 0 END) AS dkim_aligned,'

. ' SUM(CASE WHEN spf_align = 2 THEN rcount ELSE 0 END) AS spf_aligned'

. ' FROM ' . $this->connector->tablePrefix('rptrecords') . ' AS rr'

. ' INNER JOIN ' . $this->connector->tablePrefix('reports')

. ' AS rp ON rr.report_id = rp.id'

. $this->sqlCondition($domain ? true : false) . ' GROUP BY ip ORDER BY rcount DESC'

);

liuch · 2023-03-27T22:07:02Z

classes/Database/Sqlite/UpgraderMapper.php

+    private function columnExists($db, string $table, string $column): bool
+    {
+        $st = $db->prepare(
+            'PRAGMA table_info(?)'
+        );
+        $st->bindValue(2, $table, \PDO::PARAM_STR);
+        $st->execute();
+        $res = $st->fetch(\PDO::FETCH_ASSOC);
+        $st->closeCursor();
+        return $res ? true : false;
+    }


How is the column existence tested here?

liuch · 2023-03-27T22:30:59Z

Correct. As SQLite isn't a DBMS the file is the database and the need for prefix to avoid collisions when sharing a single database for multiple uses doesn't exist.

Yes, I understand you. But different prefixes can be used within the same project. For example, for tests or quick reports. The current code may delete tables beyond the prefix.

Added: What about Write-Ahead Logging: https://www.sqlite.org/wal.html ?

AnnoyingTechnology · 2023-03-31T07:03:31Z

Thanks for the feedback, I'll fix all of this.

Added: What about Write-Ahead Logging: https://www.sqlite.org/wal.html ?

What about it ? This is a performance/concurrency per-database setting.
You you're concerned about what/where it is stored, it is comprised of the database name, with -wal suffix and (if enabled) lives in the same folder as the database.

But different prefixes can be used within the same project. For example, for tests or quick reports

The simplicity of changing the database file path, or copying a database file generally renders this moot. But I'll bring prefixes back.

liuch · 2023-04-04T22:55:21Z

At least there is support for prefixes, there is support for sqlite, but nowhere is it stated that the prefixes will not work with sqlite. Your code only creates the appearance of working with prefixes. That's the main problem. If you don't want prefixes to be used with sqlite, it makes sense to EXPLICITLY disable running this configuration.

AnnoyingTechnology · 2023-04-05T06:28:31Z

If you don't want prefixes to be used with sqlite, it makes sense to EXPLICITLY disable running this configuration.

If you're OK with that, I'm fine with the application dying with Prefixes aren't supported with SQLite if the parameter isn't left empty.

liuch · 2023-04-06T12:40:05Z

Okay. So be it.

williamdes · 2024-04-17T18:07:19Z

bump rebase merge ?

Initial support for SQLite database backend

fb69ffa

AnnoyingTechnology marked this pull request as draft March 21, 2023 14:44

SQLite compatibility improved

f0e96c9

AnnoyingTechnology changed the title ~~Initial support for SQLite database backend~~ Initial support for SQLite database backend, fixes #56 Mar 23, 2023

liuch marked this pull request as ready for review March 27, 2023 16:35

liuch reviewed Mar 27, 2023

View reviewed changes

	$rcnt = $this->dbh->query('SELECT COUNT(*) FROM ' . $tname . '')->fetch(\PDO::FETCH_NUM)[0];
	$rcnt = $this->dbh->query('SELECT COUNT(*) FROM ' . $tname)->fetch(\PDO::FETCH_NUM)[0];

	$db->query('DROP TABLE ' . $table . '');
	$db->query('DROP TABLE ' . $table);

	$query .= '' . $column['name'] . ' ' . $column['definition'];
	$query .= $column['name'] . ' ' . $column['definition'];

	'SELECT COUNT(*) FROM (SELECT org FROM ' . $this->connector->tablePrefix('reports') . ''
	'SELECT COUNT(*) FROM (SELECT org FROM ' . $this->connector->tablePrefix('reports')

Initial support for SQLite database backend, fixes #56 #57

Are you sure you want to change the base?

Initial support for SQLite database backend, fixes #56 #57

Uh oh!

Conversation

AnnoyingTechnology commented Mar 21, 2023

Uh oh!

AnnoyingTechnology commented Mar 21, 2023

Uh oh!

liuch commented Mar 23, 2023

Uh oh!

AnnoyingTechnology commented Mar 23, 2023

Uh oh!

liuch commented Mar 27, 2023

Uh oh!

AnnoyingTechnology commented Mar 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liuch commented Mar 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AnnoyingTechnology commented Mar 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liuch commented Apr 4, 2023

Uh oh!

AnnoyingTechnology commented Apr 5, 2023

Uh oh!

liuch commented Apr 6, 2023

Uh oh!

williamdes commented Apr 17, 2024

Uh oh!

Uh oh!

AnnoyingTechnology commented Mar 27, 2023 •

edited

Loading

liuch commented Mar 27, 2023 •

edited

Loading

AnnoyingTechnology commented Mar 31, 2023 •

edited

Loading