feat: add support for mysql and sqlite #100

Frank-III · 2024-09-24T00:50:33Z

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 · 2024-09-24T01:51:57Z

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

src/app.rs

src/cli.rs

src/main.rs

src/database/postgresql.rs

src/components/data.rs

src/components/editor.rs

achristmascarl · 2024-09-24T20:44:29Z

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

Frank-III · 2024-09-24T23:06:08Z

@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 · 2024-09-27T00:35:48Z

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!

Frank-III · 2024-09-27T00:39:03Z

src/database.rs

  let mut query_rows = vec![];
  let mut query_rows_affected: Option<u64> = None;
  let mut headers: Headers = vec![];
-  while !query_finished {


I have to change this part as it hangs the MySQL connection. if I use 3 maximum connections, the third query will hang

achristmascarl · 2024-09-27T03:23:39Z

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 · 2024-09-27T05:18:52Z

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:

init db, start app with mysql
drop table robot_parts
[N] to rollback
go to menu and [R] to refresh
robot_parts was dropped despite rolling back
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
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 · 2024-09-27T15:45:32Z

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 · 2024-09-27T15:49:07Z

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

Frank-III · 2024-09-27T22:21:08Z

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.

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 · 2024-09-28T00:43:50Z

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 · 2024-09-28T00:47:14Z

I think no, thank you for your prompt reply!

achristmascarl · 2024-09-28T00:52:27Z

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

greptile-apps

PR Summary

This PR adds support for MySQL and SQLite databases to the rainfrog project, significantly expanding its database compatibility. The implementation involves refactoring the codebase to use generic database types and introducing new traits to handle different database operations.

Key changes and considerations:

Generic implementation: The codebase now uses generics extensively, allowing for flexible support of different database types.
New database modules: Separate files for MySQL, PostgreSQL, and SQLite have been added in the src/database/ directory.
CLI improvements: The cli.rs file now includes a Driver enum to handle different database drivers.
Component refactoring: All components have been updated to work with generic database types.
Query abstraction: Database-specific queries have been replaced with more generic, abstracted methods.

Security implications:

The use of generics improves type safety, potentially reducing runtime errors related to database operations.
Care should be taken to ensure proper database access controls are maintained across different database types.
Input validation and sanitization should be reviewed for each database type to prevent SQL injection vulnerabilities.

Performance considerations:

The added abstraction layer may have some performance implications, particularly in hot paths.
Using generics for database types allows for compile-time optimizations specific to each database backend.
Connection pooling and query execution strategies may need to be optimized for each database type.

Potential improvements:

Implement more robust error handling, especially for database-specific errors.
Consider adding integration tests for each supported database type to ensure compatibility.
Review and optimize the generic implementations for potential performance bottlenecks.
Implement database-specific features where appropriate, while maintaining a common interface.

_{13 file(s) reviewed, 16 comment(s)}
_{Edit PR Review Bot Settings}

src/cli.rs

src/components/scroll_table.rs

src/database.rs

src/database/postgresql.rs

src/database/sqlite.rs

src/main.rs

github-actions · 2024-09-29T17:27:15Z

Included in release v0.2.6

Frank-III added 4 commits September 23, 2024 18:09

add all db supports

9329951

clean up

8d312d5

clean up

293217f

more clean up

2030d27