Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Emitting with an array/vector data incorrectly serialized as separate arguments. #225

Open
NessajCN opened this issue Jan 5, 2024 · 14 comments
Labels
bug Something isn't working documentation Improvements or additions to documentation help wanted Extra attention is needed socketio-v4 It is only about the version 4 of the socket.io protocol socketio-v5 It is only about the version 5 of the socket.io protocol

Comments

@NessajCN
Copy link

NessajCN commented Jan 5, 2024

Describe the bug
As described in title, emitting an event with an array or vector like:

let v = vec![1,2,3];
socket.emit("message",  v).ok();

will split elements in the vector and send them as separate arguments.
Clients receive that many arguments instead of the intact array:

To Reproduce
Run a server using socketioxide in rust and start a Javascript or python client.
Emitting an event from server with an array/vector payload.

socket.on("message", (arg)=>{
    // will print 1
    console.log(arg);
    // error
    console.log(arg[1]);
})

socket.on("message", (...arg)=>{
    // will print 
   // 1
   // 2
   // 3 
    for (const a of arg) {
            console.log(a);
    }
})

Expected behavior
The expected resulte would be:

socket.on("message", (arg)=>{
    // will print [1,2,3]
    console.log(arg);
    // will print 2
    console.log(arg[1]);
})

Same behavior for tuple and array.

Versions (please complete the following information):

  • Socketioxide version: 0.10.0
  • Http lib: axum 0.7
  • Socket.io client version js v4.7.2

Additional context
If I wrap the array/vector with an additional array, the results will be fine:

let v = vec![1,2,3];
socket.emit("message",  [v]).ok();
@NessajCN NessajCN added the bug Something isn't working label Jan 5, 2024
@NessajCN NessajCN changed the title Emitting with an array/vector data incorrectly serialized as seperate arguments. Emitting with an array/vector data incorrectly serialized as separate arguments. Jan 5, 2024
@tausifcreates
Copy link
Contributor

That is an interesting find out.

@Totodore
Copy link
Owner

Totodore commented Jan 5, 2024

Unfortunately this is the only way I found to handle multi arguments.
When multi arguments are received they are "compacted" into an array and to send multi arguments you just provide an array or tuple. The best behaviour may be to have tuples for multi-arguments and keep arrays "as-is". But it is impossible to differentiate them when everything is serialized to a serde_json::Value.

We should document that case though.

@Totodore Totodore added documentation Improvements or additions to documentation socketio-v4 It is only about the version 4 of the socket.io protocol socketio-v5 It is only about the version 5 of the socket.io protocol labels Jan 5, 2024
@NessajCN
Copy link
Author

NessajCN commented Jan 5, 2024

Unfortunately this is the only way I found to handle multi arguments.
When multi arguments are received they are "compacted" into an array and to send multi arguments you just provide an array or tuple. The best behaviour may be to have tuples for multi-arguments and keep arrays "as-is". But it is impossible to differentiate them when everything is serialized to a serde_json::Value.

We should document that case though.

Could we just check for vector/tuple payloads and wrap them before sending in the crate?

@Totodore
Copy link
Owner

Totodore commented Jan 5, 2024

serde_json::Value doesn't make the difference (in both case it is a Value::Array variant) and we must support multi arguments to have full compatibility. Solving this issue would require to be able to detect if we provided a tuple or an array either at compile time or at runtime.
We could use a macro fn to check that but then it would not be a method of the socket struct.

@goto-bus-stop
Copy link

goto-bus-stop commented Jan 14, 2024

Perhaps it would make sense to always require a tuple in emit()? You would have to write socket.emit("single", (value,)) to emit a single argument, which is less pretty, but it's more clear and cannot give you an unexpected result. I already write all my emits like that because it already works today 😛 The only problem I see with it is that a variable number of arguments would not be supported, but that could be in a separate socket.emit_variable() method, as it's hopefully uncommon.

@Totodore
Copy link
Owner

Perhaps it would make sense to always require a tuple in emit()? You would have to write socket.emit("single", (value,)) to emit a single argument, which is less pretty, but it's more clear and cannot give you an unexpected result. I already write all my emits like that because it already works today 😛 The only problem I see with it is that a variable number of arguments would not be supported, but that could be in a separate socket.emit_variable() method, as it's hopefully uncommon.

I don't really want to force people to put tuples inside socket.emit for the reasons you gave. However adding a second emit_array or emit_variable (I don't have a precise name for the moment) would be a good idea to support the case of @NessajCN without having to do socket.emit([array]). Internally it would be just an alias of socket.emit([arg]).

If someone want to work on this don't hesitate :). Things to do are :

  • Document this special case in the socket.emit fn and in the Emitting data category in the main lib.rs doc.
  • Add a emit_array fn in Socket and Operators with an explanation on why this is needed and the difference with socket.emit
  • Update examples where the syntax socket.emit([args]) is used.

@Totodore Totodore removed their assignment Jan 14, 2024
@Totodore Totodore added the help wanted Extra attention is needed label Jan 14, 2024
@fundon
Copy link
Contributor

fundon commented Jan 15, 2024

Value::Array(ref mut v) if !v.is_empty() => {
v.insert(0, Value::String((*e).to_string()));
serde_json::to_string(&v)

                    Value::Array(v) if !v.is_empty() => serde_json::to_string(&vec![
                        (*e).to_string(),
                        serde_json::to_string(v).unwrap(),
                    ]),

Maybe we can modify it like this.

@Totodore
Copy link
Owner

Value::Array(ref mut v) if !v.is_empty() => {
v.insert(0, Value::String((*e).to_string()));
serde_json::to_string(&v)

                    Value::Array(v) if !v.is_empty() => serde_json::to_string(&vec![
                        (*e).to_string(),
                        serde_json::to_string(v).unwrap(),
                    ]),

Maybe we can modify it like this.

There are two issues:

  • The inner serde_json::to_string(v) would be escaped and therefore the result woul be [string, string] and not [string, ...T].
  • It would be impossible to do multi-argument emission.

@fundon
Copy link
Contributor

fundon commented Jan 16, 2024

                    Value::Array(v) if !v.is_empty() => {
                        serde_json::to_string(&json!([(*e).to_string(), v]))
                    }
  • It would be impossible to do multi-argument emission.

Yes, there is still a need for a method that supports multiple parameters.

@fundon
Copy link
Contributor

fundon commented Jan 16, 2024

There is another way to solve this problem. We can add a few notes to the document.

let _ = socket
        .within(data.room.clone())
        .emit("message", json!([[1, 2, 3, 4]]));
let _ = socket.within(data.room).emit("message", response);

@goto-bus-stop
Copy link

goto-bus-stop commented Jan 16, 2024

IMHO, whichever way it goes, there should be separate methods for emitting single or multiple values, and emit shouldn't do both like it does today. I think .emit(data) where data always arrives as a single argument, even if it is an array, and an .emit_multiple([1, 2, 3]) that arrives as multiple arguments would also be great. .emit(data) could just be implemented as .emit_multiple([data]).

A small issue with adding more methods is that they need to be added on 3 types and also have ack/non-ack variants. It's OK for this but if there are more variations in the future, you'd end up with lots and lots of emit_x_y_z methods.

@tausifcreates
Copy link
Contributor

What if someone tries to emit multiple arrays in emit_variable()? Then we have to wrap those arrays with additional [] right?
My input is, we should document these cases, and keep the only emit method. Otherwise, imo, it will create inconsistency.

@goto-bus-stop
Copy link

goto-bus-stop commented Jan 16, 2024

If you wanted to emit two arrays as separate arguments, you'd do emit_variable((array_a, array_b)). If you want to emit an array that contains arrays, you'd do emit_variable((array_of_arrays,)) or just emit(array_of_arrays).

The current situation is inconsistent, because if you write emit(data), you never know if it will be emitted as one argument or multiple. It depends on data's serialization implementation. If you have an opaque JSON value already, and you want to emit it as a single argument, you have to do this:

let data: serde_json::Value = todo!("get a value from somewhere");
let Some(array) = data.as_array() {
  socket.emit([array])?
} else {
  socket.emit(data)?
}

Tbh I'd still argue for a single emit() method that takes a tuple. On second thought I think that can actually handle all cases with an IntoArguments trait. See:

// Single argument
socket.emit((1,))?;

// Multiple arguments of same type
socket.emit([1, 2, 3])?;

// Multiple arguments of different types
socket.emit((1, "2", serde_json::json!({ "wrapped": "by tuple" })))?;

// Variable number of arguments
let array = vec![0; random_size];
socket.emit(array);

// Variable number of arguments of different types
let mut array: Vec<serde_json::Value> = vec![];
array.push(1.into());
if some_condition { array.push("2".into()); }
socket.emit(array);

With a trait definition like this:

trait IntoArguments {
    fn into_arguments(self) -> Vec<serde_json::Value>;
}
// Implement this for tuples of many different sizes using a macro
impl <T: Serialize> IntoArguments for (T,) {
    // pseudocode implementation, would need error handling
    fn into_arguments(self) -> Vec<serde_json::Value> { vec![serde_json::to_value(self)] }
}

impl <T: Serialize> IntoArguments for &[T] {
    // pseudocode implementation, would need error handling
    fn into_arguments(self) -> Vec<serde_json::Value> { self.iter().map(serde_json::to_value).collect() }
}
impl SocketIo {
    fn emit(&self, event: &str, args: impl IntoArguments) -> // etc
}

In my opinion writing emit((value,)) is really not that bad (it's 3 extra characters that help clarity), and that would be the only thing that would need to be explicitly documented, and if you omit it you'd get a compile error.

@Totodore
Copy link
Owner

I'm still hesitant on the direction to take to fix this issue.

Tbh I'd still argue for a single emit() method that takes a tuple. On second thought I think that can actually handle all cases with an IntoArguments trait. See:

A solution with an IntoArgument trait is a good idea but I'm not a big fan of this syntax emit((value,)).

For the moment I'm going to simply document this issue on the emit fn.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working documentation Improvements or additions to documentation help wanted Extra attention is needed socketio-v4 It is only about the version 4 of the socket.io protocol socketio-v5 It is only about the version 5 of the socket.io protocol
Projects
None yet
Development

No branches or pull requests

5 participants