feat: add support for mysql and sqlite

[Frank-III]

hopefully resolve: #67

Hi there! this is a simple hack that adds support for MySQL and sqlite, should be easy to implement for other dbs that sqlx has support for. I haven't finished it yet, would like to discuss it before I continue working on it.

[achristmascarl]

hey @Frank-III, thanks for the PR and taking this on! i wasn't sure when i'd get to it, so this is really helpful 🙏

structure-wise, I think everything makes sense so far. there's a few specific things i'll leave comments for, but overall it makes sense.

i'll also open a separate PR for mysql and sqlite containers/test data to make the development experience smoother, hopefully in time for you to make use of them lol

[achristmascarl]

@Frank-III i just merged https://github.com/achristmascarl/rainfrog/pull/101, hopefully that will be helpful for you during development

[Frank-III]

@Frank-III i just merged #101, hopefully that will be helpful for you during development

Thank you that would be very helpful, I would try to address all the issues and refer to the contribution guide!

[Frank-III]

Hi @achristmascarl, some updates on this PR. The parser for MySQL and SQLite are almost working: For MySQL:

• Geometry type cannot be parsed correctly, we may need to parse it as binary and use crate like geozero to parse it and represent it as String.

For SQLite:

• types besides INTEGER, REAL, TEXT, BLOB would be interpreted as NULL by sqlx and just cast as String for representation(like DECIMAL(10,2), JSON)
• We store geometry data as binary so casting it as binary and parsing it using geozero seems to be the solution as well.

Feel free to give it a try, and provide feedback! Thank you again for adding test data for MySQL and SQLite, super helpful!

[achristmascarl]

thanks @Frank-III, will take a look! we can hold off on the geometry types for now, i think i'll need to add either geozero or georust for #66 to support postgis, so we can wait until then and add it in for all the drivers together

[achristmascarl]

great work, the parsers and drivers set ups look good! two main points of feedback:

1. i don't think the mysql transactions work on drop statements

to repro:

1. init db, start app with mysql
2. drop table robot_parts
3. [N] to rollback
4. go to menu and [R] to refresh
5. robot_parts was dropped despite rolling back
6. however, rolling back seems to work for delete statements. if i try to delete all the rows and roll it back, the rows are not deleted
7. rolling back the drop statement in postgres still works

i'm not sure this is actually because of your code, it could be a quirk with sqlx and mysql. i'll do some more research tmrw as well

2. we should add some tests to the mysql and sqlite drivers, similar to what's there for postgres

other

a minor suggestion, not blocking for the PR though, is to make the driver optional and get the user to input if it's missing

[achristmascarl]

it looks like drop statements can't be rolled back in mysql: https://dev.mysql.com/doc/refman/8.4/en/cannot-roll-back.html

i'll need to change the confirmation popup so that it can optionally show up before executing the query as well, you can skip this for now

[achristmascarl]

probably 'DROP' statements for all drivers can be "confirm before executing" instead of "execute in transaction"

[Frank-III]

it looks like drop statements can't be rolled back in mysql: https://dev.mysql.com/doc/refman/8.4/en/cannot-roll-back.html probably 'DROP' statements for all drivers can be "confirm before executing" instead of "execute in transaction"

Yeah, that makes sense, I would skip this part for now then.

2. we should add some tests to the mysql and sqlite drivers, similar to what's there for Postgres

I add the test for SQLite and MySQL, there is one SQLite query that could not get properly parsed by the sqlparser crate, and the drop table test for MySQL would fail when testing if we should use tx.

a minor suggestion, not blocking for the PR though, is to make the driver optional and get the user to input if it's missing

I thought about this too! Also, if the user passes the URL directly we don't need the driver option

[achristmascarl]

thanks for adding in the driver implementation, and the tests look good! we can leave the other tests for now, i'll first release with a warning that mysql/sqlite are unstable and i'll revisit them later.

@Frank-III was there anything else you wanted to add or is the PR finished from your end? if it's finished, i can run the tests and add a few small things for Docker before merging :)

[Frank-III]

I think no, thank you for your prompt reply!

[achristmascarl]

great, i'm also going to have an automated review bot, but you can ignore it's feedback for now; it's just to surface things that i might need to take a second look at

@greptileai

[github-actions[bot]]

Included in release v0.2.6