feat: added D1 integration #246

KernelFreeze · 2022-11-21T17:33:40Z

Added a Cloudflare D1 integration using wasm_bindgen.

Pending work:

Add tests to Worker Sandbox.
Add meta field.
Cleanup.

zebp

This looks like a really good start, but I feel that it won't be rust-y enough if we just return JsValues. I think the path forward is to create a D1Deserializer that implements Deserializer. This guide by the Serde developers should be super useful in implementing that, but it still might not be trivial. If you'd like me write that deserializer I'm more than happy to get this PR landed :)

zebp · 2022-11-22T20:59:01Z

worker/src/d1.rs

+        self.0.error()
+    }
+
+    pub fn results(&self) -> Vec<JsValue> {


We should try to avoid returning regular JsValues where possible. Perhaps we can use a serde Deserializer with D1's implicit type conversion.

That's a good idea

zebp · 2022-11-22T20:59:22Z

worker/src/d1.rs

+        Ok(D1Result(result.into()))
+    }
+
+    pub async fn raw(&self) -> Result<Vec<JsValue>> {


Same thing, let's avoid JsValue.

zebp · 2022-11-22T20:59:34Z

worker/src/d1.rs

+        Ok(())
+    }
+
+    pub async fn first(&self, col_name: Option<&str>) -> Result<JsValue> {


We should avoid JsValue.

…ize D1 data.

KernelFreeze · 2022-11-23T13:08:13Z

I applied your suggestion using Cloudflare's serde_wasm_bindgen, is that ok?

zebp · 2022-11-27T20:37:04Z

I applied your suggestion using Cloudflare's serde_wasm_bindgen, is that ok?

Yeah that seems fine! When you think this PR is ready, remove the draft status and give me a ping.

KernelFreeze · 2022-12-02T20:12:59Z

I'm now working on improving the error handling. Some users reported to me that they have problems with some queries and they can't know what is the exact problem because right now it only returns "D1Error".

FlareLine · 2022-12-19T04:05:57Z

worker/src/d1.rs

+pub struct D1PreparedStatement(D1PreparedStatementSys);
+
+impl D1PreparedStatement {
+    pub fn bind<T>(&self, value: &T) -> Result<()>


Should this be some variation of pub fn bind<T>(&self, value: &T) -> Result<Result<D1PreparedStatement, JsValue>>? Please correct me if I'm wrong but it looks like we're throwing away the returned statement / error?

Ideally replacing JSValue with a more appropriate 'Rust-y' type.

Yes, I need to rework the error handling

FlareLine · 2022-12-19T09:42:40Z

Just playing around with this one (as I'm extremely interested in getting in across the line):

Looks like I'm having trouble using parameterized queries. A simple code example:

// ...
.get_async("/endpoint/:id", |_, ctx| async move {
let default = String::from("A");
let id = ctx.param("id").unwrap_or(&default).clone();
let d1 = ctx.env.d1("mydb")?;
let stmt = match d1
  .prepare("SELECT * FROM things WHERE thing_id = ?1")
  .bind(&id)
  {
    Ok(r) => r,
    Err(e) => return Response::error(format!("{:?}", e), 500),
  };

#[derive(Serialize, Deserialize)]
struct Thing {
  thing_id: String,
  desc: String,
  num: u16,
}

Using the above code (and some modifications to the workers create to allow for deeper debugging provides me with this error, irrespective of calls to run()/all()/raw():

D1_ERROR
Error: D1_ERROR
at D1Database._send (d1-beta-facade.entry.js:682:13)
at async D1PreparedStatement.all (d1-beta-facade.entry.js:720:7))")

Using the same structure but with a non-parameterized query doesn't seem to error in the same way

.prepare("SELECT * FROM things WHERE thing_id = 'A'")
.bind(&id)

seems to return the expected result of

> 'Hello world!'

I've taken a look at serde_wasm_bindgen and it doesn't look like there's anything out of the ordinary here. I've also tried several other different commands / structures and the errors I'm getting are super inconsistent.

The errors provided by the binding with JS seem really obscure 🤔

Alex1607 · 2022-12-19T11:42:48Z

Having the same problem as @FlareLine when trying to bind a parameter.
Funnily enough, the binding works fine when I added quotes around it. Like this: [...] WHERE something = "?";
However, it doesn't work, obviously. I just found that a bit strange.

FlareLine · 2022-12-20T05:53:16Z

It does appear that most of the unexpected errors occur when you attempt to use parameterized queries. Ruling out all known issues I still get errors when binding a simple string to a parameter in a query.

To get around this issue, I could use simple interpolation instead of parameter binding in my query string, as below:

.get_async("/db/:id", |_, ctx| async move {
  let default = String::from("A");
  let id = ctx.param("id").unwrap_or(&default);
  let d1 = ctx.env.d1("my-db")?;
  let query = format!("SELECT * FROM things WHERE thing_id = '{}'", id);
  let stmt = d1.prepare(&query);
  let result = stmt.all().await?;
  let things = result.results::<Thing>()?;
  Response::from_json::<Vec<Thing>>(&things)
})
.run(request, env)
.await

But this opens up the application to SQLi vulnerabilities and is a 'hacky' workaround.

Upon attempting to diagnose further, it looks like there's a lack of information coming back from the wasm_bindgen call - I'm way out of my depth to know how to diagnose past the Rust code, but does anyone have any doco on the process?

FlareLine · 2023-01-08T10:56:23Z

Documenting my findings for reference

I've forked workers-rs and wrangler locally to allow me to debug and make changes to the underlying JS.

I've changed a significant amount of the error handling within the local wrangler fork, in the D1 JS facade (d1-beta-facade.js) to facilitate better error visibility during debugging. This isn't something that we'd do in the JS layer to allow for this Rust D1 functionality, but we would have to work out the binding process to allow this information to be bound and discoverable at the Rust layer.

I've worked out that one of the underlying errors is actually a SQL 9008 Invalid type error as per this statement and response combination:

JS query payload: "{\"sql\":\"SELECT * FROM things WHERE thing_id = ?1\",\"params\":[[\"A\"]]}"
JS query response: {\"error\":\"ERROR 9008: SQL query error: Invalid type\",\"success\":false,\"serve...

From this, I've noticed that the params field actually contains a nested array? I'm not sure if this is intentional, but performing that same query without using binding results in a successful response.

I'll continue debugging here to see if I can get a consistent pattern working.

FlareLine · 2023-01-08T11:32:38Z

worker-sys/src/d1.rs

+    #[derive(Debug, Clone, PartialEq, Eq)]
+    pub type D1PreparedStatement;
+
+    #[wasm_bindgen(structural, method, js_class=D1PreparedStatement, js_name=bind, catch)]


This fixes one of the issues documented in #246 (comment). 😄

Suggested change

#[wasm_bindgen(structural, method, js_class=D1PreparedStatement, js_name=bind, catch)]

#[wasm_bindgen(structural, method, catch, variadic, js_class=D1PreparedStatement, js_name=bind)]

FlareLine · 2023-01-08T21:37:47Z

worker/src/d1.rs

+        T: serde::ser::Serialize + ?Sized,
+    {
+        let value = serde_wasm_bindgen::to_value(value)?;
+        let array = Array::of1(&value);


This will most likely cause issues documented in #246 (comment). We should allow bind<T> to take in an Iterator of T and convert this to an Array instance for use in our self.0.bind(array) call. Despite the variadic fix in the aforementioned comment, I can still only bind a single parameter to my query, and any attempts to call bind again in some chained fashion will fail.

FlareLine · 2023-01-16T00:11:11Z

@KernelFreeze are you still happy to work on this PR? I'd be keen to get this across the line myself - happy to attribute your contributions thus far? 😄

KernelFreeze · 2023-01-19T22:32:28Z

@KernelFreeze are you still happy to work on this PR? I'd be keen to get this across the line myself - happy to attribute your contributions thus far? 😄

Hey! I don't have too much time right now to finish this, so sure, go ahead ^^ If you want to, I can give you access to my repo or if you prefer you can open your own PR.

FlareLine · 2023-01-26T23:47:14Z

@zebp @Alex1607 I've created #270 to supersede this PR as per guidance from @KernelFreeze.

Alex1607 · 2023-01-27T00:02:55Z

@zebp @Alex1607 I've created #270 to supersede this PR as per guidance from @KernelFreeze.

Thank you! I will try it out as soon as I got some time :D

KernelFreeze added 7 commits November 21, 2022 12:27

feat: added D1 integration

b15562b

refactor: removed unused type annotations

613a54a

feat: re-export D1 structs

0dfbda9

fix: cast to the right type

72cae5d

fix: remove getter from most bindings

b938d40

fix: use different result struct for exec

d33d671

fix: use D1Result without casting

24cfa33

zebp reviewed Nov 22, 2022

View reviewed changes

feat: use Cloudflare's serde_wasm_bindgen to serialize and deserial…

1fe6b52

…ize D1 data.

FlareLine reviewed Dec 19, 2022

View reviewed changes

KianNH mentioned this pull request Jan 5, 2023

[Feature] D1 Support #221

Open

1 task

FlareLine suggested changes Jan 9, 2023

View reviewed changes

KernelFreeze closed this Feb 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: added D1 integration #246

feat: added D1 integration #246

KernelFreeze commented Nov 21, 2022 •

edited

Loading

zebp left a comment

zebp Nov 22, 2022

KernelFreeze Nov 23, 2022

zebp Nov 22, 2022

zebp Nov 22, 2022

KernelFreeze commented Nov 23, 2022

zebp commented Nov 27, 2022

KernelFreeze commented Dec 2, 2022

FlareLine Dec 19, 2022

FlareLine Dec 19, 2022 •

edited

Loading

KernelFreeze Dec 19, 2022

FlareLine commented Dec 19, 2022 •

edited

Loading

Alex1607 commented Dec 19, 2022

FlareLine commented Dec 20, 2022

FlareLine commented Jan 8, 2023

FlareLine Jan 8, 2023

FlareLine Jan 8, 2023

FlareLine commented Jan 16, 2023

KernelFreeze commented Jan 19, 2023 •

edited

Loading

FlareLine commented Jan 26, 2023

Alex1607 commented Jan 27, 2023

	#[wasm_bindgen(structural, method, js_class=D1PreparedStatement, js_name=bind, catch)]
	#[wasm_bindgen(structural, method, catch, variadic, js_class=D1PreparedStatement, js_name=bind)]

feat: added D1 integration #246

feat: added D1 integration #246

Conversation

KernelFreeze commented Nov 21, 2022 • edited Loading

zebp 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

KernelFreeze commented Nov 23, 2022

zebp commented Nov 27, 2022

KernelFreeze commented Dec 2, 2022

Choose a reason for hiding this comment

FlareLine Dec 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FlareLine commented Dec 19, 2022 • edited Loading

Alex1607 commented Dec 19, 2022

FlareLine commented Dec 20, 2022

FlareLine commented Jan 8, 2023

Documenting my findings for reference

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FlareLine commented Jan 16, 2023

KernelFreeze commented Jan 19, 2023 • edited Loading

FlareLine commented Jan 26, 2023

Alex1607 commented Jan 27, 2023

KernelFreeze commented Nov 21, 2022 •

edited

Loading

FlareLine Dec 19, 2022 •

edited

Loading

FlareLine commented Dec 19, 2022 •

edited

Loading

KernelFreeze commented Jan 19, 2023 •

edited

Loading