Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for ecto_sqlite3_extras #400

Merged
merged 8 commits into from
Feb 10, 2023

Conversation

orsinium
Copy link
Contributor

@orsinium orsinium commented Feb 2, 2023

I've created ecto_sqlite3_extras, a package designed specifically to show stats for SQLite3 databases in Phoenix Live Dashboard. This PR integrates the package with the live dashboard. I mimicked the API and implementation of ecto_psql_extras (and ecto_mysql_extras did the same), so the integration was quite straightforward.

Moving forward, some code and documentation can be cleaned up a bit. It was ok to copy-paste the same text twice but not that ok when you do it 3 times, and maybe more in the future. However, for now, I decided not to be creative and do the same as what ecto_mysql_extras team did.

image

@orsinium
Copy link
Contributor Author

orsinium commented Feb 2, 2023

The tests require PostgreSQL database with a specific username and password, but I don't see docker-compose or similar. Is there a simple way to run tests locally?

Copy link
Member

@alexcastano alexcastano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @orsinium!

Thanks for your PR. I miss the configuration in the file dev.exs that allows us to try the dashboard in development mode. Right now, we can give two arguments --postgres and --mysql. It would be nice to have the same for sqlite3.

A docker compose for the databases to be used in dev and test environments would be interesting too. For example, I execute the following command for MySQL:

docker run --restart=unless-stopped --name mariadb -e MARIADB_ALLOW_EMPTY_ROOT_PASSWORD=yes -e MARIADB_DATABASE=phx_dashboard_test -p 3306:3306 -d mariadb:latest

@orsinium
Copy link
Contributor Author

orsinium commented Feb 3, 2023

Thank you for the review! All good comments, I'll make sure to fix it all. I also want to provide some more tests, similar to what we have to MySQL. I'll ping you when it's done.

@orsinium orsinium changed the title Add support for ecto_sqlite3_extras WIP: Add support for ecto_sqlite3_extras Feb 3, 2023
@orsinium orsinium marked this pull request as draft February 3, 2023 09:01
@orsinium orsinium changed the title WIP: Add support for ecto_sqlite3_extras Add support for ecto_sqlite3_extras Feb 3, 2023
@orsinium orsinium marked this pull request as ready for review February 3, 2023 11:05
@orsinium orsinium requested a review from alexcastano February 3, 2023 11:05
@alexcastano
Copy link
Member

Hi @orsinium ,

Thanks again for your job. I will review it again when I have more time, but I see this error when clicking on Sequence number:

image

@orsinium
Copy link
Contributor Author

orsinium commented Feb 4, 2023

but I see this error when clicking on Sequence number:

Yep, I know. The issue is on the library side, and I don't know yet how to resolve it:
https://github.com/orsinium-labs/ecto_sqlite3_extras/blob/master/lib/queries/sequence_number.ex#L19-L21

Suggestions are welcome. People usually solve it by creating and then dropping a temporary table with an autoincrement column, but I'd like to have all queries read-only.

@mindreframer
Copy link

@orsinium It seems this is the only way:

https://renenyffenegger.ch/notes/development/databases/SQLite/internals/schema-objects/sqlite_sequence

The structure of sqlite_sequence can be displayed with the SQLite shell command .schema sqlite_sequence. Because sqlite_sequence is only created when the first table with an autoincrement column exists, we first need to create such a table.

One can not create this table up-front:

sqlite> CREATE TABLE IF NOT EXISTS sqlite_sequence(name,seq);
Run Time: real 0.000 user 0.000060 sys 0.000060
Parse error: object name reserved for internal use: sqlite_sequence

So the best way would be to use a migration to create a dummy table + drop it, as you have already mentioned.

create table dummy (id integer primary key autoincrement);
drop table dummy; 
SELECT name AS table_name, seq AS sequence_number FROM sqlite_sequence ORDER BY sequence_number DESC;

Maybe it is possible to have an init callback with the repo, and then one could create this table on demand:

SELECT name FROM sqlite_master WHERE type='table' AND name='sqlite_sequence';

Also: SQLite queries are fast, so performance wont be a concern here. I would suggest to get it done in a non-clean way, just to get it done. And if there is a better, cleaner way, improve it later.

Thanks for the ecto_sqlite3_extras-package, looks nice!

@alexcastano
Copy link
Member

but I see this error when clicking on Sequence number:

Yep, I know. The issue is on the library side, and I don't know yet how to resolve it: https://github.com/orsinium-labs/ecto_sqlite3_extras/blob/master/lib/queries/sequence_number.ex#L19-L21

Suggestions are welcome. People usually solve it by creating and then dropping a temporary table with an autoincrement column, but I'd like to have all queries read-only.

I don't use SQLite, but could you capture this specific error to return an empty result? I think the library shouldn't create any table; it can cause problems in future versions. The way should be to capture the no table exception and return an empty result.

@orsinium
Copy link
Contributor Author

orsinium commented Feb 6, 2023

Fixed! Thank you all for your help :)

image

@alexcastano alexcastano merged commit 419e256 into phoenixframework:master Feb 10, 2023
@alexcastano
Copy link
Member

🎉 🎉 🎉 🎉 🎉

@orsinium
Copy link
Contributor Author

Thank you all! ❤️

@orsinium orsinium deleted the sqlite3 branch February 11, 2023 07:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants