Allow creating a sea_orm::Databse from sqlx::ConnectOptions #434

05storm26 · 2022-01-07T10:26:56Z

Seems like doing what is requested in #398 is too difficult.

However allowing users to create a sea_orm::Database from sqlx::ConnectOptions implementations like MySqlConnectOptions seems to be doable.

For me this is good enough.

Also I have added a new feature options sqlx-any. If this is enabled the sqlx is any feature is turned on and you can create an sea_orm::Database from AnyConnectOptions as well.

~~However this depends on this PR launchbadge/sqlx#1610 (not yet merged!)~~ MERGED and released

05storm26 · 2022-01-07T12:11:00Z

@billy1624 Can you approve the run now?

billy1624 · 2022-01-07T13:52:43Z

Sure!

05storm26 · 2022-01-07T15:20:22Z

I think the rocket example that failed has beed fixed.

Also I have went through and added Hash, PartialEq, Clone derives on all types where possible, in the second commit.

billy1624

I'm curious about why we need to derive Hash?

billy1624 · 2022-01-14T03:57:29Z

src/database/mod.rs

 /// Defines the configuration options of a database
-#[derive(Debug)]
+#[derive(Debug, Clone)]
 pub struct ConnectOptions {
-    /// The URI of the database
-    pub(crate) url: String,
+    /// The database sqlx::ConnectOptions used to connect to the database.
+    pub(crate) connect_options: SqlxConnectOptions,
+    /// The URI of the database, if this struct was created from an URI string, otherwise None
+    pub(crate) url: Option<String>,


We should make this an enum with two variants

SqlxConnectOptions: wrapper enum of all supported databases' connection option

ConnectionOptions: raw url with some other connection option included in the old ConnectOptions struct

First I removed the url entirely. Then I put it back because of the url() method.

The best solution would be to remove the url string field from here and then have the sqlx::ConnectOptions have a method for getting a string url from them representing their content and then we could remove the url field from here. However this would need to be implemented in sqlx.

This PR has changed the url() in a way that it will only return something if it was created from an url because otherwise we currently cannot get the url from the connect_options.

Do we need to have the method url()? If we could remove it then we can also just get rid of the url field here.

The best option would be to have the sqlx::ConnectOptions trait have a to_url() -> String method that is implemented in sqlx for all the ConnectOptions implmentations IMO.

I'm just too lazy to do that myself that seems like a lot of work :D

This PR has changed the url() in a way that it will only return something if it was created from an url because otherwise we currently cannot get the url from the connect_options.

Which is pretty confusing IMO... :\

What do you think we should do with the url? Should we keep the url() method? Should that only return the url if the sea_orm::ConenctOptions was created from an url string?

I think we can keep the ConnectOptions untouched. And introduce a new trait (e.g. ConnectionOptionTrait) that convert any kinds of DatabaseOptions into Database. Then, Database::connect() method takes any ConnectionOptionTrait.

Make sense? CC @tyt2y3

@billy1624 No this doesn't answer the question about what should we return from the DatabaseConnection when get_url is called if the DatabaseConnection was created from a sqlx::ConnectOptions implementation like SqliteConnectOptions?

About ConnectionOptionTrait we don't need that to be able to implement creating DatabaseConnection from both urls and both sqlx::ConnectOptions implementations. You can just do what I already did in this PR implementing with TryFrom.

@billy1624 any update about this? Specifically about what should get_url return if the DatabaseConnection was not created from an url string but from an sqlx::ConnectOptions e.g.: SqliteConnectOptions?

@billy1624 @tyt2y3 Anyone?

Hey @05storm26, sorry for the delay. I will get back to you ASAP

Hi @billy1624 the required sqlx commits have been merged so this could be merged after the review is completed.

Could you take a look at this conversation again?

05storm26 · 2022-01-14T09:36:10Z

I'm curious about why we need to derive Hash?

I am not sure. Maybe someone needs it. If you want to use ConnectOptions in a hashmap as keys maybe. I don't know why you would do that just an idea.

I am generally on the side that we should implement for all types as much from the traits: Clone, Clone, PartialEq, Hash as possible.

billy1624 · 2022-02-16T09:08:57Z

Will launchbadge/sqlx#1610 be in SQLx 0.6? And when will 0.6 lands?

05storm26 · 2022-02-17T16:43:09Z

Due to changes requested by sqlx owner I had to make a small change in this pr as well.

billy1624 · 2022-03-01T09:48:13Z

Oh, I wonder why the tests failed

05storm26 · 2022-03-01T09:54:49Z

the trait std::hash::Hash is not implemented for sea_query::Value

Seems like maybe there was a sea_query change that removed Hash from Value. No idea why. I think it is useful to have these basic traits implemented like Debug, Clone, PartialEq, Hash for all possible types.

Currently there is two commits in this PR the actual change and also a second one which adds these useful trait derives on multiple types. Probably that is the one breaking.

Should I just remove that? Or should we fix this in sea_query?

05storm26 · 2022-03-01T09:57:51Z

But I am still not sure what to do with the get_url method on DatabaseConnection, when the connection was not created from an url string but a sqlx::ConnectOptions....

billy1624 · 2022-03-02T08:31:13Z

Should I just remove that? Or should we fix this in sea_query?

I think we can remove the Hash derive

billy1624 · 2022-03-02T08:48:40Z

However allowing users to create a sea_orm::Database from sqlx::ConnectOptions implementations like MySqlConnectOptions seems to be doable.

Now that I re-think this. Shouldn't we just add a method to create DatabaseConnection from sqlx::ConnectOptions? Similar to from_sqlx_*_pool methods.

sea-orm/src/driver/sqlx_mysql.rs

Lines 66 to 72 in 483de17

    
               /// Instantiate a sqlx pool connection to a [DatabaseConnection] 
        
               pub fn from_sqlx_mysql_pool(pool: MySqlPool) -> DatabaseConnection { 
        
                   DatabaseConnection::SqlxMySqlPoolConnection(SqlxMySqlPoolConnection { 
        
                       pool, 
        
                       metric_callback: None, 
        
                   }) 
        
               }

05storm26 · 2022-12-03T16:33:13Z

@billy1624 I have rebased this on top of master. Please run the tests for it.

05storm26 · 2022-12-05T16:46:36Z

Now that I re-think this. Shouldn't we just add a method to create DatabaseConnection from sqlx::ConnectOptions? Similar to from_sqlx_*_pool methods.

For me it looks like that should be constructed by calling Database::connect. So I change that in this PR to accept an sqlx::ConnectOptions as well.

05storm26 · 2022-12-05T16:49:16Z

Is clippy supposed to run without warning? Because currently master HEAD has some clippy warnings:

$ cargo clippy --features "sqlx-all macros with-json with-chrono with-uuid sqlx-all runtime-async-std-native-tls"  -- -D warnings
    Checking sea-orm v0.10.3 (/home/adam/sea-orm)
error: redundant closure
   --> src/database/transaction.rs:278:25
    |
278 |             err.map_err(|e| sqlx_error_to_query_err(e))
    |                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace the closure with the function itself: `sqlx_error_to_query_err`
    |
    = note: `-D clippy::redundant-closure` implied by `-D warnings`
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#redundant_closure

error: passing a unit value to a function
   --> src/driver/sqlx_mysql.rs:216:9
    |
216 |         Ok(self.pool.close().await)
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: `-D clippy::unit-arg` implied by `-D warnings`
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unit_arg
help: move the expression in front of the call and replace it with the unit literal `()`
    |
216 ~         self.pool.close().await;
217 +         Ok(())
    |

error: passing a unit value to a function
   --> src/driver/sqlx_postgres.rs:231:9
    |
231 |         Ok(self.pool.close().await)
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unit_arg
help: move the expression in front of the call and replace it with the unit literal `()`
    |
231 ~         self.pool.close().await;
232 +         Ok(())
    |

error: passing a unit value to a function
   --> src/driver/sqlx_sqlite.rs:219:9
    |
219 |         Ok(self.pool.close().await)
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unit_arg
help: move the expression in front of the call and replace it with the unit literal `()`
    |
219 ~         self.pool.close().await;
220 +         Ok(())
    |

05storm26 · 2022-12-09T17:26:36Z

I don't think this is ever going to be merged so I am just gonna let it go.

billy1624 · 2022-12-12T02:29:59Z

Hey @05storm26, sorry and thanks again for the effort!

05storm26 force-pushed the master branch 2 times, most recently from 575c8b4 to 9f4e204 Compare January 7, 2022 12:09

05storm26 force-pushed the master branch 3 times, most recently from 0fce39b to 8711d89 Compare January 7, 2022 15:18

billy1624 linked an issue Jan 14, 2022 that may be closed by this pull request

Support creating a sea_orm::Database from any sqlx::Executor for example from an sqlx::any::AnyPool. #398

Closed

billy1624 reviewed Jan 14, 2022

View reviewed changes

tyt2y3 force-pushed the master branch 2 times, most recently from 7ce941b to 33a87d7 Compare February 6, 2022 06:39

billy1624 force-pushed the master branch from 66fb015 to 4769a3c Compare February 7, 2022 02:47

tyt2y3 force-pushed the master branch from 11b1e4c to 2788833 Compare February 7, 2022 12:55

05storm26 force-pushed the master branch from 8711d89 to e842e8a Compare February 17, 2022 16:41

05storm26 force-pushed the master branch 2 times, most recently from ca64c37 to 0c91a5a Compare February 19, 2022 16:39

billy1624 force-pushed the master branch from af9aa4d to 2d5967d Compare March 27, 2022 09:43

billy1624 assigned 05storm26 Jul 12, 2022

05storm26 force-pushed the master branch 2 times, most recently from 7cd0537 to 4699f7d Compare December 3, 2022 15:54

05storm26 force-pushed the master branch from 4699f7d to 22a2efc Compare December 3, 2022 16:02

05storm26 force-pushed the master branch 2 times, most recently from bedb6e8 to b77cac7 Compare December 5, 2022 16:40

05storm26 force-pushed the master branch 3 times, most recently from b452118 to 4852645 Compare December 5, 2022 19:36

05storm26 added 3 commits December 5, 2022 20:41

Allow creating a sea_orm::Database from sqlx::ConnectOptions

0f4293e

Fix some clippy warnings

ab2f92b

Fix compilation when sqlx-all is enabled

d81ae81

05storm26 force-pushed the master branch from 4852645 to d81ae81 Compare December 5, 2022 19:41

05storm26 closed this Dec 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow creating a sea_orm::Databse from sqlx::ConnectOptions #434

Allow creating a sea_orm::Databse from sqlx::ConnectOptions #434

05storm26 commented Jan 7, 2022 •

edited

Loading

05storm26 commented Jan 7, 2022

billy1624 commented Jan 7, 2022

05storm26 commented Jan 7, 2022

billy1624 left a comment

billy1624 Jan 14, 2022

05storm26 Jan 14, 2022

05storm26 Jan 14, 2022

05storm26 Jan 14, 2022

billy1624 Jan 14, 2022

05storm26 Jan 14, 2022 •

edited

Loading

05storm26 Jan 18, 2022

05storm26 Feb 3, 2022

billy1624 Feb 13, 2022

05storm26 Feb 19, 2022

05storm26 commented Jan 14, 2022

billy1624 commented Feb 16, 2022

05storm26 commented Feb 17, 2022 •

edited

Loading

billy1624 commented Mar 1, 2022

05storm26 commented Mar 1, 2022

05storm26 commented Mar 1, 2022

billy1624 commented Mar 2, 2022

billy1624 commented Mar 2, 2022

05storm26 commented Dec 3, 2022

05storm26 commented Dec 5, 2022 •

edited

Loading

05storm26 commented Dec 5, 2022

05storm26 commented Dec 9, 2022

billy1624 commented Dec 12, 2022

Allow creating a sea_orm::Databse from sqlx::ConnectOptions #434

Allow creating a sea_orm::Databse from sqlx::ConnectOptions #434

Conversation

05storm26 commented Jan 7, 2022 • edited Loading

05storm26 commented Jan 7, 2022

billy1624 commented Jan 7, 2022

05storm26 commented Jan 7, 2022

billy1624 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

05storm26 Jan 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

05storm26 commented Jan 14, 2022

billy1624 commented Feb 16, 2022

05storm26 commented Feb 17, 2022 • edited Loading

billy1624 commented Mar 1, 2022

05storm26 commented Mar 1, 2022

05storm26 commented Mar 1, 2022

billy1624 commented Mar 2, 2022

billy1624 commented Mar 2, 2022

05storm26 commented Dec 3, 2022

05storm26 commented Dec 5, 2022 • edited Loading

05storm26 commented Dec 5, 2022

05storm26 commented Dec 9, 2022

billy1624 commented Dec 12, 2022

05storm26 commented Jan 7, 2022 •

edited

Loading

05storm26 Jan 14, 2022 •

edited

Loading

05storm26 commented Feb 17, 2022 •

edited

Loading

05storm26 commented Dec 5, 2022 •

edited

Loading