search

liuch / dmarc-srg

A php parser, viewer and summary report generator for incoming DMARC reports.
GNU General Public License v3.0
218 stars 32 forks source link

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

Open AnnoyingTechnology opened 1 year ago

AnnoyingTechnology commented 1 year ago

Draft

AnnoyingTechnology commented 1 year ago

This seems to be working pretty well now.

liuch commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

AnnoyingTechnology commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Okay. So be it.

williamdes commented 4 months ago

bump rebase merge ?