feat(driver-adapters): enable Wasm on `quaint` and related crates #4442

jkomyno · 2023-11-14T10:51:05Z

This PR closes https://github.com/prisma/team-orm/issues/279 and deprecates #4119.

This PR allows quaint to be conditionally compiled to wasm32-unknown-unknown.
The end goal is that query-engine-node-api keeps using quaint's native Rust Drivers + napi.rs-based Driver Adapters, whereas quaint-engine-wasm should use only quaint's definitions (without native Rust Drivers) + Wasm-based Driver Adapters (the latter are NOT part of this PR).

How it works

The default feature of quaint enables the SQL connector definitions into the compiled bundle, but not their drivers
Native Rust drivers are explicitly enabled via the native feature of quaint
The connector module of quaint is refactored in such a way that each SQL database's definition is now in its own folder (e.g., connector/mysql rather than connector/mysql.rs), and each such folder contains the native and wasm subfolders. Intuitively, wasm contains the Wasm-compatible definitions, data structures, and errors needed for that specific connector (e.g., mysql), and native contains extensions and additional definitions for such connector that are only used when native Rust drivers are required.
The connector.rs, by default, imports the Wasm-compatible connector definitions (e.g., via #[cfg(feature = "mysql")]), and optionally imports the native bindings on top of it (e.g., via #[cfg(feature = "mysql-native")]). In other words, in practice, importing e.g. mysql/native implies importing mysql/wasm as well, but not viceversa.

Compilation check

`--target native`

✅ cargo build -p query-engine-node-api

`--target wasm32-unknown-unknown`

✅ cargo build -p quaint --target wasm32-unknown-unknown
✅ cargo build -p prisma-models --target wasm32-unknown-unknown
✅ cargo build -p user-facing-errors --target wasm32-unknown-unknown
❌ cargo build -p query-core --target wasm32-unknown-unknown (https://github.com/prisma/team-orm/issues/280)
❌ cargo build -p request-handlers --target wasm32-unknown-unknown (https://github.com/prisma/team-orm/issues/281)
✅ cargo build -p query-connector --target wasm32-unknown-unknown
❌ cargo build -p sql-query-connector --target wasm32-unknown-unknown (https://github.com/prisma/team-orm/issues/283)

…ls on native

codspeed-hq · 2023-11-14T11:10:39Z

CodSpeed Performance Report

Merging #4442 will not alter performance

_{Comparing feat/quaint-on-wasm32-unknown-unknown (a1c2b40) with main (800a9b4)}

Summary

✅ 11 untouched benchmarks

jkomyno · 2023-11-14T11:34:09Z

EDIT: solved by 5ab6d96.

Current blocker

sqlite's error SqliteError uses rusqlite and libsqlite3-sys under the hood, which are not compatible with wasm32-unknown-unknown no matter which feature flag is used on those crates. This error struct (along with similar ones for postgres and mysql) is used in driver_adapters::result::DriverAdapterError.

Proposed strategy:

copy the code definition needed from libsqlite3-sys

jkomyno · 2023-11-14T12:39:56Z

quaint/src/connector/mysql/wasm/error.rs

+#[derive(Debug, Error)]
+enum MysqlAsyncError {
+    #[error("Server error: `{}'", _0)]
+    Server(#[source] MysqlError),
+}


This is a partial copy of the mysql_async::Error enum.

Importing mysql_async here would break the Wasm compilation.

👍 Could you please add a comment in the source code explaining this comment, and pointing to the source code of the original Enum, where you grab this variant?

Sure: bacb635

jkomyno · 2023-11-14T22:38:34Z

quaint/Cargo.toml

@@ -102,7 +127,7 @@ branch = "vendored-openssl"

 [dependencies.rusqlite]
 version = "0.29"
-features = ["chrono", "bundled", "column_decltype"]
+features = ["chrono", "column_decltype"]


The bundled feature for rusqlite is enabled when the sqlite-native feature of quaint (implied by native and all) is turned on.

miguelff

Alberto and I reviewed this code together. We discussed some of the decisions made, and some potential improvements that are commented below, but are not blocking at all.

One piece of actionable feedback that we talked about is that the wasm folders for each provider should include code that only will be linked when compiling to wasm-unknown-unknown, and native modules should not depend on anything within this folder.

As such we are going to make a small refactoring to move from this set of dependencies:

provider::native -> provider::wasm::common
provider::wasm -> provider::wasm::common

to

provider::native -> provider::common
provider::wasm -> provider::common

Other than that the code looks good enough and further refactorings that have to do with legacy aspects of the code, will be tackled independently (@jkomyno will create them)

miguelff · 2023-11-15T10:29:26Z

quaint/src/connector/mssql/native/mod.rs

@@ -0,0 +1,256 @@
+//! Definitions for the MSSQL connector.


I guess the answer is no, but to double check with you, files within the native directories, don't contain any new definitions, but are only part of the diff, because a slice of it was moved from the previous files at the root of any provider folder. Am I right?

You are right, Miguel.

miguelff · 2023-11-15T10:30:28Z

quaint/src/connector/mssql/native/mod.rs

+use tokio_util::compat::{Compat, TokioAsyncWriteCompatExt};
+
+/// The underlying SQL Server driver. Only available with the `expose-drivers` Cargo feature.
+#[cfg(feature = "expose-drivers")]


Can we remove this feature, given that we don't publish quaint anymore? This is not blocking at all, but if we are not using it, and it's not very difficult to remove it, let's get rid of it.

miguelff · 2023-11-15T10:33:03Z

quaint/src/connector/mysql/wasm/common.rs

+
+                    connect_timeout = match as_int {
+                        0 => None,
+                        _ => Some(Duration::from_secs(as_int)),
+                    };
+                }
+                "pool_timeout" => {
+                    let as_int = v
+                        .parse::<u64>()
+                        .map_err(|_| Error::builder(ErrorKind::InvalidConnectionArguments).build())?;
+
+                    pool_timeout = match as_int {
+                        0 => None,
+                        _ => Some(Duration::from_secs(as_int)),
+                    };
+                }
+                "sslaccept" => {
+                    use_ssl = true;
+                    match v.as_ref() {
+                        "strict" => {
+                            #[cfg(feature = "mysql-native")]
+                            {
+                                ssl_opts = ssl_opts.with_danger_accept_invalid_certs(false);
+                            }
+                        }
+                        "accept_invalid_certs" => {}
+                        _ => {
+                            tracing::debug!(
+                                message = "Unsupported SSL accept mode, defaulting to `accept_invalid_certs`",
+                                mode = &*v
+                            );
+                        }
+                    };
+                }
+                "max_connection_lifetime" => {
+                    let as_int = v
+                        .parse()
+                        .map_err(|_| Error::builder(ErrorKind::InvalidConnectionArguments).build())?;
+
+                    if as_int == 0 {
+                        max_connection_lifetime = None;
+                    } else {
+                        max_connection_lifetime = Some(Duration::from_secs(as_int));
+                    }
+                }
+                "max_idle_connection_lifetime" => {
+                    let as_int = v
+                        .parse()
+                        .map_err(|_| Error::builder(ErrorKind::InvalidConnectionArguments).build())?;
+
+                    if as_int == 0 {
+                        max_idle_connection_lifetime = None;
+                    } else {
+                        max_idle_connection_lifetime = Some(Duration::from_secs(as_int));
+                    }
+                }
+                _ => {
+                    tracing::trace!(message = "Discarding connection string param", param = &*k);
+                }
+            };
+        }
+
+        // Wrapping this in a block, as attributes on expressions are still experimental
+        // See: https://github.com/rust-lang/rust/issues/15701
+        #[cfg(feature = "mysql-native")]
+        {
+            ssl_opts = match identity {
+                Some((Some(path), Some(pw))) => {
+                    let identity = mysql_async::ClientIdentity::new(path).with_password(pw);
+                    ssl_opts.with_client_identity(Some(identity))
+                }
+                Some((Some(path), None)) => {
+                    let identity = mysql_async::ClientIdentity::new(path);
+                    ssl_opts.with_client_identity(Some(identity))
+                }
+                _ => ssl_opts,
+            };
+        }
+
+        Ok(MysqlUrlQueryParams {
+            #[cfg(feature = "mysql-native")]
+            ssl_opts,
+            connection_limit,
+            use_ssl,
+            socket,
+            socket_timeout,
+            connect_timeout,
+            pool_timeout,
+            max_connection_lifetime,
+            max_idle_connection_lifetime,
+            prefer_socket,
+            statement_cache_size,
+        })
+    }
+
+    #[cfg(feature = "pooled")]
+    pub(crate) fn connection_limit(&self) -> Option<usize> {
+        self.query_params.connection_limit
+    }
+}


For other reviewer's interest. Now some implementations of a struct contract are segregated into common and native modules (as it should be) This is the common part of MysqlUrl

miguelff · 2023-11-15T10:34:47Z

quaint/src/connector/mysql/native/mod.rs

+impl MysqlUrl {
+    pub(crate) fn cache(&self) -> LruCache<String, my::Statement> {
+        LruCache::new(self.query_params.statement_cache_size)
+    }
+
+    pub(crate) fn to_opts_builder(&self) -> my::OptsBuilder {
+        let mut config = my::OptsBuilder::default()
+            .stmt_cache_size(Some(0))
+            .user(Some(self.username()))
+            .pass(self.password())
+            .db_name(Some(self.dbname()));
+
+        match self.socket() {
+            Some(ref socket) => {
+                config = config.socket(Some(socket));
+            }
+            None => {
+                config = config.ip_or_hostname(self.host()).tcp_port(self.port());
+            }
+        }
+
+        config = config.conn_ttl(Some(Duration::from_secs(5)));
+
+        if self.query_params.use_ssl {
+            config = config.ssl_opts(Some(self.query_params.ssl_opts.clone()));
+        }
+
+        if self.query_params.prefer_socket.is_some() {
+            config = config.prefer_socket(self.query_params.prefer_socket);
+        }
+
+        config
+    }
+}


And this is the second part of the interface that was segregated as mentioned in https://github.com/prisma/prisma-engines/pull/4442/files#diff-c21c4fed44e88a7b5e9434cc899df264f55d52bb9b2bb654f15c7ecff7540060R301

In here, there are symbols that either are IO (part of the native subject) related. But also others that are only used in the native code paths. Like the cache function.

miguelff · 2023-11-15T10:37:45Z

quaint/src/connector/mysql/native/mod.rs

+pub use mysql_async;
+
+impl MysqlUrl {
+    pub(crate) fn cache(&self) -> LruCache<String, my::Statement> {


Consider inlining in the MySQL constructor -if possible- as this is an implementation detail of the statement cache, that is exposed in the interface.

miguelff · 2023-11-15T10:42:17Z

quaint/src/connector/mysql/native/mod.rs

+        LruCache::new(self.query_params.statement_cache_size)
+    }
+
+    pub(crate) fn to_opts_builder(&self) -> my::OptsBuilder {


I wonder why a URL has to depend on socket at all. 🤷🏾

miguelff · 2023-11-15T10:46:44Z

quaint/src/connector/mysql/native/mod.rs

+impl Mysql {
+    /// Create a new MySQL connection using `OptsBuilder` from the `mysql` crate.
+    pub async fn new(url: MysqlUrl) -> crate::Result<Self> {
+        let conn = timeout::connect(url.connect_timeout(), my::Conn::new(url.to_opts_builder())).await?;


I was wondering why we couldn't just make the MySQL url, 'common' rather than native. @jkomyno pointed me to this code. The reason is that a URL is an object that belongs to the driver-specific crate mysql_async, and it's used at least in here, as an argument for timeout::connect, so the URL cannot return a different object that can polyfill mysql_async::OptsBuilder

This is of course, existing porting code, nothing newly added.

miguelff · 2023-11-15T10:51:52Z

quaint/src/connector/mysql/wasm/error.rs

+#[derive(Debug, Error)]
+enum MysqlAsyncError {
+    #[error("Server error: `{}'", _0)]
+    Server(#[source] MysqlError),
+}


👍 Could you please add a comment in the source code explaining this comment, and pointing to the source code of the original Enum, where you grab this variant?

miguelff · 2023-11-15T10:57:31Z

quaint/src/connector/sqlite/wasm/ffi.rs

@@ -0,0 +1,7 @@
+//! This is a partial copy of `rusqlite::ffi::*`.


A sightly more verbose comment, in the lines of:

"These are only the subset of constants being used by the query-engine to parse the errors into some higher-level, more meaningful error up the stack"

Maybe a "See errors.rs" in the WASM folder -> to be probably moved to a common folder

Sure: 263bab0

miguelff

For the record, we paired on flattening the structure of each of the providers directories, to get rid off wasm specific things.

It's still pending to replace the additional family of feature (-native) which is only dependant on the target architecture with conditional compilation based on whether we are on the wasm32 target or not, which @jkomyno will do for both mysql, and the rest of the providers. After that, this should be ready to be merged.

jkomyno · 2023-11-15T13:04:23Z

I have flattened the modules as agreed in our call with Miguel, but I don’t agree with replacing *-native with #[cfg(not(target_arch = "wasm32"))]. Why? Let's take a look at quaint/Cargo.toml. The *-native feature flag implies additional optional dependencies or dependencies' flags.

E.g., for the mysql provider, the mysql-native feature flag toggles "mysql", "mysql_async", "tokio/time", "lru-cache", which is easily expressed via:

[features]
native = [
  "mysql-native",
  # ...
]

mysql-native = ["mysql", "mysql_async", "tokio/time", "lru-cache"]

There is no equivalently valid Cargo.toml syntax for achieving the same result with such clarity (see rust-lang/cargo#1197).

For this reason, I argue we should keep the -native flags (as we had originally discussed ~ 2 months ago), and import quaint conditionally based on:

whether we need it on query-engine-node-api

[dependencies]
quaint = { path = "../../quaint", features = ["native"] }

whether we need it on query-engine-wasm:
```
quaint = { path = "../../quaint" }
```

In practice, query-engine-* crates don't import quaint directly, but you get the idea.
It's easier and clearer to have other crates importing quaint to conditionally add the native feature when not on wasm32 arch, then to apply this logic in quaint itself.

miguelff · 2023-11-15T14:33:02Z

Ok, to not replace with architecture conditional flags in the light of rust-lang/cargo#1197 👍

jkomyno · 2023-11-15T15:16:51Z

Ok, to not replace with architecture conditional flags in the light of rust-lang/cargo#1197 👍

Great, thanks. This PR is now ready for a final review round.

Weakky · 2023-11-15T15:22:22Z

quaint/src/connector/mysql/native/mod.rs

+    #[test]
+    fn should_parse_socket_url() {
+        let url = MysqlUrl::new(Url::parse("mysql://root@localhost/dbname?socket=(/tmp/mysql.sock)").unwrap()).unwrap();
+        assert_eq!("dbname", url.dbname());
+        assert_eq!(&Some(String::from("/tmp/mysql.sock")), url.socket());
+    }
+
+    #[test]
+    fn should_parse_prefer_socket() {
+        let url =
+            MysqlUrl::new(Url::parse("mysql://root:root@localhost:3307/testdb?prefer_socket=false").unwrap()).unwrap();
+        assert!(!url.prefer_socket().unwrap());
+    }
+
+    #[test]
+    fn should_parse_sslaccept() {
+        let url =
+            MysqlUrl::new(Url::parse("mysql://root:root@localhost:3307/testdb?sslaccept=strict").unwrap()).unwrap();
+        assert!(url.query_params.use_ssl);
+        assert!(!url.query_params.ssl_opts.skip_domain_validation());
+        assert!(!url.query_params.ssl_opts.accept_invalid_certs());
+    }
+
+    #[test]
+    fn should_parse_ipv6_host() {
+        let url = MysqlUrl::new(Url::parse("mysql://[2001:db8:1234::ffff]:5432/testdb").unwrap()).unwrap();
+        assert_eq!("2001:db8:1234::ffff", url.host());
+    }
+
+    #[test]
+    fn should_allow_changing_of_cache_size() {
+        let url = MysqlUrl::new(Url::parse("mysql:///root:root@localhost:3307/foo?statement_cache_size=420").unwrap())
+            .unwrap();
+        assert_eq!(420, url.cache().capacity());
+    }
+
+    #[test]
+    fn should_have_default_cache_size() {
+        let url = MysqlUrl::new(Url::parse("mysql:///root:root@localhost:3307/foo").unwrap()).unwrap();
+        assert_eq!(100, url.cache().capacity());
+    }


These tests should most likely be part of the url.rs crate

It might be the case for other connectors

Thanks, I've addressed this in 95a4e28.

miguelff

Ship it boy! 🚀

miguelff · 2023-11-17T08:21:48Z

new::regressions::prisma_15204::conversion_error::convert_to_bigint_sqlite_js

This started regressing. It might be because of any latest change in driver adapters code, could you please check and fix @aqrln ?

Since #4442

jkomyno added 10 commits November 10, 2023 13:49

feat(quaint): allow wasm32-unknown-unknown compilation; currently fai…

61ef25b

…ls on native

feat(quaint): split postgres connector into native and wasm submodules

055e696

feat(quaint): split mysql connector into native and wasm submodules

12c6ebb

feat(quaint): recover wasm error for mysql

060486d

feat(quaint): split mssql connector into native and wasm submodules

5de1dc0

feat(quaint): split sqlite connector into native and wasm submodules

8ecbc5c

chore(quaint): fix clippy when compiling natively

45df24f

chore(quaint): fix clippy when compiling to wasm32-unknown-unknown

6a1f733

chore(quaint): update README

e61bf75

chore(quaint): rename "*-connector" feature flag to "*-native"

257c4c8

jkomyno added this to the 5.7.0 milestone Nov 14, 2023

jkomyno self-assigned this Nov 14, 2023

jkomyno added 2 commits November 14, 2023 12:37

feat(quaint): enable pure Wasm SqliteError

5ab6d96

feat(query-connect): allow wasm32-unknown-unknown compilation

ab65c95

jkomyno marked this pull request as ready for review November 14, 2023 12:11

jkomyno requested a review from miguelff November 14, 2023 12:11

Merge branch 'main' into feat/quaint-on-wasm32-unknown-unknown

485f6dc

jkomyno commented Nov 14, 2023

View reviewed changes

jkomyno mentioned this pull request Nov 14, 2023

feat(driver-adapters): enable Wasm on sql-query-connector #4444

Merged

janpio added topic: edge functions formerly phase 2 topic: vertical slice 🍰 labels Nov 14, 2023

jkomyno commented Nov 14, 2023

View reviewed changes

miguelff reviewed Nov 15, 2023

View reviewed changes

WIP: refactor mysql module to flatten its structure

c987dce

miguelff reviewed Nov 15, 2023

View reviewed changes

jkomyno added 3 commits November 15, 2023 13:17

feat(quaint): flatten mssql connector module

626bc1e

feat(quaint): flatten postgres connector module

a9f8ba8

feat(quaint): flatten sqlite connector module

3c1a100

chore(quaint): export all public definitions in connector "url" modules

7f4c8f9

jkomyno requested a review from a team as a code owner November 15, 2023 12:53

jkomyno requested review from Weakky and removed request for a team November 15, 2023 12:53

jkomyno requested a review from miguelff November 15, 2023 15:16

Weakky reviewed Nov 15, 2023

View reviewed changes

chore(quaint): refactor tests for connectors, addressing feedback

95a4e28

jkomyno mentioned this pull request Nov 15, 2023

feat(quaint): remove expose-drivers feature #4451

Closed

jkomyno added 2 commits November 15, 2023 17:19

chore: add comment on MysqlAsyncError

bacb635

chore: add comment on ffi.rs for sqlite

263bab0

miguelff approved these changes Nov 16, 2023

View reviewed changes

Merge branch 'main' into feat/quaint-on-wasm32-unknown-unknown

6a754b3

Merge branch 'main' into feat/quaint-on-wasm32-unknown-unknown

a1c2b40

jkomyno merged commit b567896 into main Nov 17, 2023
59 of 60 checks passed

jkomyno deleted the feat/quaint-on-wasm32-unknown-unknown branch November 17, 2023 15:22

jkomyno mentioned this pull request Nov 17, 2023

feat(quaint): enable wasm32-unknown-unknown compilation via slim feature flag #4119

Closed

Jolg42 mentioned this pull request Nov 20, 2023

chore(deps): update engines to 5.7.0-8.ebb702b740d9a49b9d5c1ce78fa735d48576fdcb prisma/prisma#22031

Merged

miguelff added a commit that referenced this pull request Nov 20, 2023

Fix ambiguous imports

7fa8cb9

Since #4442

miguelff mentioned this pull request Nov 20, 2023

Fix ambiguous imports #4468

Merged

miguelff added a commit that referenced this pull request Nov 20, 2023

Fix ambiguous imports

fe3a34c

Since #4442

miguelff pushed a commit that referenced this pull request Nov 20, 2023

Fix ambiguous imports (#4468)

8736624

Since #4442

miguelff added a commit that referenced this pull request Nov 20, 2023

Fix ambiguous imports

0a516f6

Since #4442

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(driver-adapters): enable Wasm on `quaint` and related crates #4442

feat(driver-adapters): enable Wasm on `quaint` and related crates #4442

jkomyno commented Nov 14, 2023 •

edited

Loading

codspeed-hq bot commented Nov 14, 2023 •

edited

Loading

jkomyno commented Nov 14, 2023 •

edited

Loading

jkomyno Nov 14, 2023

miguelff Nov 15, 2023

jkomyno Nov 15, 2023

jkomyno Nov 14, 2023

miguelff left a comment

miguelff Nov 15, 2023

jkomyno Nov 15, 2023

miguelff Nov 15, 2023

jkomyno Nov 15, 2023

miguelff Nov 15, 2023 •

edited

Loading

miguelff Nov 15, 2023

miguelff Nov 15, 2023

miguelff Nov 15, 2023

miguelff Nov 15, 2023

miguelff Nov 15, 2023

miguelff Nov 15, 2023

miguelff Nov 15, 2023

jkomyno Nov 15, 2023

miguelff left a comment

jkomyno commented Nov 15, 2023

miguelff commented Nov 15, 2023

jkomyno commented Nov 15, 2023

Weakky Nov 15, 2023

Weakky Nov 15, 2023

jkomyno Nov 15, 2023

miguelff left a comment •

edited

Loading

miguelff commented Nov 17, 2023

		@@ -0,0 +1,7 @@
		//! This is a partial copy of `rusqlite::ffi::*`.

feat(driver-adapters): enable Wasm on quaint and related crates #4442

feat(driver-adapters): enable Wasm on quaint and related crates #4442

Conversation

jkomyno commented Nov 14, 2023 • edited Loading

How it works

Compilation check

--target native

--target wasm32-unknown-unknown

codspeed-hq bot commented Nov 14, 2023 • edited Loading

Merging #4442 will not alter performance

Summary

jkomyno commented Nov 14, 2023 • edited Loading

Current blocker

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

miguelff 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

miguelff Nov 15, 2023 • 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

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

miguelff left a comment

Choose a reason for hiding this comment

jkomyno commented Nov 15, 2023

miguelff commented Nov 15, 2023

jkomyno commented Nov 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miguelff left a comment • edited Loading

Choose a reason for hiding this comment

miguelff commented Nov 17, 2023

feat(driver-adapters): enable Wasm on `quaint` and related crates #4442

feat(driver-adapters): enable Wasm on `quaint` and related crates #4442

jkomyno commented Nov 14, 2023 •

edited

Loading

`--target native`

`--target wasm32-unknown-unknown`

codspeed-hq bot commented Nov 14, 2023 •

edited

Loading

jkomyno commented Nov 14, 2023 •

edited

Loading

miguelff Nov 15, 2023 •

edited

Loading

miguelff left a comment •

edited

Loading