worker-kv does not serialize struct metadata

[crestonbunch]

Is there an existing issue for this?

• I have searched the existing issues

What version of workers-rs are you using?

main branch

What version of wrangler are you using?

3.50.0

Describe the bug

There seems to be a bad serialization step when executing a put_with_metadata(). Specifically, this will serialize any metadata Value (that is an object, and not a primitive) as JsValue(Object({})) instead of the expected value.

I haven't dug into why serde_wasm_bindgen seems to be unable to serialize serde_json::Value::Object types, but I figure if I post it here someone might know something I don't.

Steps To Reproduce

Here is a test case you can add to worker_kv_test/src/lib.rs to reproduce the issue. Add the corresponding route and try hitting it:

diff --git a/worker-kv/tests/worker_kv_test/src/lib.rs b/worker-kv/tests/worker_kv_test/src/lib.rs
index 8567fc4..f3f1ad2 100644
--- a/worker-kv/tests/worker_kv_test/src/lib.rs
+++ b/worker-kv/tests/worker_kv_test/src/lib.rs
@@ -1,5 +1,6 @@
 use std::future::Future;
 
+use serde::{Deserialize, Serialize};
 use worker::*;
 use worker_kv::{KvError, KvStore};
 
@@ -33,6 +34,9 @@ pub async fn main(req: Request, env: Env, _ctx: Context) -> Result<Response> {
         .get_async("/list-keys", |req, ctx| wrap(req, ctx, list_keys))
         .get_async("/put-simple", |req, ctx| wrap(req, ctx, put_simple))
         .get_async("/put-metadata", |req, ctx| wrap(req, ctx, put_metadata))
+        .get_async("/put-metadata-struct", |req, ctx| {
+            wrap(req, ctx, put_metadata_struct)
+        })
         .get_async("/put-expiration", |req, ctx| wrap(req, ctx, put_expiration))
         .run(req, env)
         .await
@@ -92,6 +96,31 @@ async fn put_metadata(_: Request, ctx: RouteContext<KvStore>) -> TestResult {
     Ok("passed".into())
 }
 
+async fn put_metadata_struct(_: Request, ctx: RouteContext<KvStore>) -> TestResult {
+    #[derive(Debug, Default, Deserialize, Serialize, PartialEq)]
+    pub struct TestStruct {
+        pub a: String,
+        pub b: usize,
+    }
+
+    let store = ctx.data;
+    store
+        .put("put_b", "test")?
+        .metadata(TestStruct::default())?
+        .execute()
+        .await?;
+
+    let (val, meta) = store
+        .get("put_b")
+        .text_with_metadata::<TestStruct>()
+        .await?;
+
+    kv_assert_eq!(val.unwrap(), "test")?;
+    kv_assert_eq!(meta.unwrap(), TestStruct::default())?;
+
+    Ok("passed".into())
+}
+
 async fn put_expiration(_: Request, ctx: RouteContext<KvStore>) -> TestResult {
     const EXPIRATION: u64 = 2000000000;
     let store = ctx.data;

Example output:

# Terminal 1
$ wrangler dev --port 8787

# Terminal 2
$ curl http://localhost:8787/put-metadata-struct
js error: JsValue(Error: missing field `a`
Error: missing field `a`
    at at (file:///Users/creston/Projects/workers-rs/worker-kv/tests/worker_kv_test/.wrangler/tmp/dev-XPWiSs/shim.js:385:11)
    at wasm://wasm/001c85da:wasm-function[517]:0x2f6f1
    at wasm://wasm/001c85da:wasm-function[336]:0x26b66
    at wasm://wasm/001c85da:wasm-function[97]:0x60db
    at wasm://wasm/001c85da:wasm-function[96]:0x4e5b
    at wasm://wasm/001c85da:wasm-function[170]:0x1c96b
    at wasm://wasm/001c85da:wasm-function[661]:0x31b6d
    at K (file:///Users/creston/Projects/workers-rs/worker-kv/tests/worker_kv_test/.wrangler/tmp/dev-XPWiSs/shim.js:166:5)
    at u (file:///Users/creston/Projects/workers-rs/worker-kv/tests/worker_kv_test/.wrangler/tmp/dev-XPWiSs/shim.js:158:14))% 

[kflansburg]

Interesting, it looks to me like metadata is never actually passed when PutOptionsBuilder.execute is called.

workers-rs/worker-kv/src/builder.rs

Line 52 in c76fdc4

.call3(&self.this, &self.name, &self.value, &options_object)?

[kflansburg]

Although, it looks like options_object is the whole PutOptionsBuilder serialized, but that is a strange pattern. I will take a look.

[kflansburg]

@crestonbunch I think I found the problem, seems that serde_wasm_bindgen has a custom Serializer that is intended to produce JSON-like behavior, which I think is what we want here:

Serializer::json_compatible()

With this change, I'm able to verify that the metadata round trips successfully. Unfortunately I don't know the implications of all of this or where else we should be using it :/.