Correctly encode Duration and optional fields #72

uhoreg · 2023-12-14T22:51:46Z

fixes #56, #57, and (hopefully) element-hq/element-web#26566

I ported matrix-org/matrix-rust-sdk#817 over, and added another parameter to mark fields as optional if they're Option<...>, to avoid sticking null in responses. It's kind of ugly, so suggestions welcome.

I'm not sure what's the best way to test this. All the existing tests are in JS, and as far as I can tell, from JS the only way to get these structs is to get them from OlmMachine, which means coercing OlmMachine to spit out the right requests. Otherwise, I could write a test in Rust that creates the ruma request struct and test that it transforms into the wasm request struct correctly, but that would mean adding another testing system. That's not too bad since Rust has its own testing built-in, but I don't know if we care about keeping the tests in JS.

uhoreg · 2023-12-14T22:54:35Z

src/requests.rs

@@ -316,7 +318,7 @@ macro_rules! request {
    (
        $destination_request:ident from $source_request:ident
        $( extracts $( $field_name:ident : $field_type:tt ),+ $(,)? )?
-        $( $( and )? groups $( $grouped_field_name:ident ),+ $(,)? )?
+        $( $( and )? groups $( $grouped_field_name:ident $( { $transformation:expr } )? $( $optional:literal )? ),+ $(,)? )?


I'm adding "optional" to flag things as optional. But the match will match any literal, which is suboptimal. I'd prefer to match just optional, but I don't see any way of doing that. If I use $( optional ), then that would match just optional, but then I don't have a way to expand that back out it the transcription below, because it isn't assigned to a metavariable.

Could we check at runtime that the token is optional? It would be nice to have some kind of check.

Or I guess you could have two alternatives, one with optional and one without, and give the metavariables different names? I've not done many of these macros so I'm just guessing.

@Hywan as original author of the request! macro, do you have any suggestions for this?

What about simply optional?

$( $( and )? groups $( $grouped_field_name:ident $( { $transformation:expr } )? $( optional )? ),+ $(,)? )?

Yup, I tried doing $( optional )?, but I couldn't figure out how to handle the expansion so that it does something different depending on whether it's specified or not, because it isn't bound to a metavariable.

Let's keep things as they are now, I may update this later.

uhoreg · 2023-12-15T18:18:15Z

Otherwise, I could write a test in Rust that creates the ruma request struct and test that it transforms into the wasm request struct correctly, but that would mean adding another testing system. That's not too bad since Rust has its own testing built-in, but I don't know if we care about keeping the tests in JS.

Just for fun, I decided to see what would happen if I ran cargo test, and I got

    Finished test [unoptimized + debuginfo] target(s) in 56.12s
     Running unittests src/lib.rs (target/wasm32-unknown-unknown/debug/deps/matrix_sdk_crypto_wasm-a46182b35ca3089b.wasm)
error: test failed, to rerun pass `--lib`

Caused by:
  could not execute process `/home/hubert/Projects/matrix/matrix-rust-sdk-crypto-wasm/target/wasm32-unknown-unknown/debug/deps/matrix_sdk_crypto_wasm-a46182b35ca3089b.wasm` (never executed)

Caused by:
  Exec format error (os error 8)

which suggests that doing Rust tests will not be as trivial as I thought it would be.

uhoreg · 2023-12-15T22:45:51Z

I figured out a way to test the conversions, and it's only moderately ugly!

uhoreg · 2023-12-15T22:49:15Z

src/requests.rs

+
+    use super::{KeysClaimRequest, KeysQueryRequest, KeysUploadRequest};
+
+    #[wasm_bindgen(js_name = "_test_make_keys_claim_request")]


I deliberately used ugly names here since they're only intended for testing

andybalaam

I found these macros very difficult to understand, but as far as I did understand, this looks sensible, and the tests appear to demonstrate it works :-)

andybalaam · 2023-12-20T15:37:54Z

src/requests.rs

@@ -316,7 +318,7 @@ macro_rules! request {
    (
        $destination_request:ident from $source_request:ident
        $( extracts $( $field_name:ident : $field_type:tt ),+ $(,)? )?
-        $( $( and )? groups $( $grouped_field_name:ident ),+ $(,)? )?
+        $( $( and )? groups $( $grouped_field_name:ident $( { $transformation:expr } )? $( $optional:literal )? ),+ $(,)? )?


Could we check at runtime that the token is optional? It would be nice to have some kind of check.

andybalaam · 2023-12-20T15:39:23Z

src/requests.rs

@@ -316,7 +318,7 @@ macro_rules! request {
    (
        $destination_request:ident from $source_request:ident
        $( extracts $( $field_name:ident : $field_type:tt ),+ $(,)? )?
-        $( $( and )? groups $( $grouped_field_name:ident ),+ $(,)? )?
+        $( $( and )? groups $( $grouped_field_name:ident $( { $transformation:expr } )? $( $optional:literal )? ),+ $(,)? )?


Or I guess you could have two alternatives, one with optional and one without, and give the metavariables different names? I've not done many of these macros so I'm just guessing.

src/requests.rs

Hywan · 2024-01-08T16:22:09Z

src/requests.rs

@@ -316,7 +318,7 @@ macro_rules! request {
    (
        $destination_request:ident from $source_request:ident
        $( extracts $( $field_name:ident : $field_type:tt ),+ $(,)? )?
-        $( $( and )? groups $( $grouped_field_name:ident ),+ $(,)? )?
+        $( $( and )? groups $( $grouped_field_name:ident $( { $transformation:expr } )? $( $optional:literal )? ),+ $(,)? )?


What about simply optional?

$( $( and )? groups $( $grouped_field_name:ident $( { $transformation:expr } )? $( optional )? ),+ $(,)? )?

Cargo.toml

Hywan · 2024-01-11T13:16:09Z

src/requests.rs

@@ -316,7 +318,7 @@ macro_rules! request {
    (
        $destination_request:ident from $source_request:ident
        $( extracts $( $field_name:ident : $field_type:tt ),+ $(,)? )?
-        $( $( and )? groups $( $grouped_field_name:ident ),+ $(,)? )?
+        $( $( and )? groups $( $grouped_field_name:ident $( { $transformation:expr } )? $( $optional:literal )? ),+ $(,)? )?


Let's keep things as they are now, I may update this later.

PR #72 landed in 4.0.1, not 4.0.0.

PR #72 landed in 4.0.1, not 4.0.0. Also: add UNRELEASED section

correctly encode Duration and optional fields

70f9e0c

uhoreg commented Dec 14, 2023

View reviewed changes

uhoreg added 2 commits December 15, 2023 17:39

add tests and mark timeout as optional

8e4df86

run cargo fmt

0c956bf

make eslint happy too

8d80d97

uhoreg commented Dec 15, 2023

View reviewed changes

make prettier happy too

743d507

uhoreg marked this pull request as ready for review December 15, 2023 23:05

uhoreg requested a review from a team as a code owner December 15, 2023 23:05

uhoreg requested a review from andybalaam December 15, 2023 23:05

andybalaam approved these changes Dec 20, 2023

View reviewed changes

Hywan requested changes Jan 8, 2024

View reviewed changes

uhoreg added 2 commits January 10, 2024 13:31

use wasm_bindgen_test to test requests

d0ff8bd

cargo fmt

846fb69

uhoreg requested a review from Hywan January 10, 2024 18:57

Hywan requested changes Jan 11, 2024

View reviewed changes

uhoreg added 4 commits January 12, 2024 17:40

Merge branch 'main' into request_transform

93fca09

add changelog

e381fcd

Merge branch 'main' into request_transform

823e6cb

bump wasm-bindgen-test version

8cff979

uhoreg requested a review from Hywan January 20, 2024 19:30

Hywan approved these changes Jan 22, 2024

View reviewed changes

Hywan merged commit 33d531a into matrix-org:main Jan 22, 2024
3 checks passed

uhoreg mentioned this pull request Jan 23, 2024

KeysUploadRequest generates malformed bodies #57

Closed

richvdh added a commit that referenced this pull request Jan 23, 2024

Fix changelog for 4.0.1

4c3d0f1

PR #72 landed in 4.0.1, not 4.0.0.

This was referenced Jan 23, 2024

Fix changelog for 4.0.1 #93

Merged

Element Web R can't send encrypted messages on dendrite/conduit element-hq/element-web#26566

Closed

richvdh added a commit that referenced this pull request Jan 23, 2024

Fix changelog for 4.0.1 (#93)

45e8ea3

PR #72 landed in 4.0.1, not 4.0.0. Also: add UNRELEASED section

uhoreg mentioned this pull request Jan 25, 2024

Element R: Add test that requests are encoded properly matrix-org/matrix-js-sdk#4033

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correctly encode Duration and optional fields #72

Correctly encode Duration and optional fields #72

uhoreg commented Dec 14, 2023

uhoreg Dec 14, 2023

andybalaam Dec 20, 2023

andybalaam Dec 20, 2023

uhoreg Dec 21, 2023

Hywan Jan 8, 2024

uhoreg Jan 8, 2024

Hywan Jan 11, 2024

uhoreg commented Dec 15, 2023

uhoreg commented Dec 15, 2023

uhoreg Dec 15, 2023

andybalaam left a comment

andybalaam Dec 20, 2023

andybalaam Dec 20, 2023

Hywan Jan 8, 2024

Hywan Jan 11, 2024


		use super::{KeysClaimRequest, KeysQueryRequest, KeysUploadRequest};

		#[wasm_bindgen(js_name = "_test_make_keys_claim_request")]

Correctly encode Duration and optional fields #72

Correctly encode Duration and optional fields #72

Conversation

uhoreg commented Dec 14, 2023

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

uhoreg commented Dec 15, 2023

uhoreg commented Dec 15, 2023

Choose a reason for hiding this comment

andybalaam 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