Autobahn Python/JS Interoperability fails due to handling of msgpack bintype

[pramukta]

This is an issue I talked (too much) about in #9. It can be observed by trying out the basic arguments autobahn example and removing any of the u"..." prefixes in the responses. This is most relevant in python 2.7 though will likely arise in 3.x in b"..." prefixed strings. A small discussion of the underlying problem (and a kindof fix) is present in the Autobahn|Python source, here (the docstring).

I was able to fix the issue for the referenced example by uncommenting the from_bin function in json_msgpack_sax.hpp included from tplgy/json-msgpack

The diff is small and there may be a good reason for the original commented out version so will just post it here:

--- a/include/json_msgpack/json_msgpack_sax.hpp
+++ b/include/json_msgpack/json_msgpack_sax.hpp
@@ -71,10 +71,10 @@ struct no_custom_string_conversion
     }

     template <typename Writer>
-    inline static bool from_bin(Writer&, const msgpack::object&)
+    inline static bool from_bin(Writer& writer, const msgpack::object& o)
     {
-        //return writer.String(o.via.bin.ptr, o.via.bin.size);
-        return false;
+        return writer.String(o.via.bin.ptr, o.via.bin.size);
+        //return false;
     }

     template <typename Writer>

Does anyone know if that change safe to make / merge? or would it break bin types being translated from msgpack<->msgpack based WAMP clients. Maybe the case should be handled differently?

[jpetso]

You can fix a problem by using the no_custom_string_conversion string converter? We shouldn't even be using that, we should be using the WAMP-specific one.

[jpetso]

I also just noticed that we write encoded.at(1) = '\0'; here: https://github.com/tplgy/bonefish/blob/master/src/bonefish/serialization/json_serializer.cpp#L151

I feel like there's a good chance it should be encoded.at(0) instead, since the binary data starts at encoded.data() + 1 and will overwrite that \0, in addition to a potentially incorrect value at the start of the buffer.

[pramukta]

Not sure, but yeah, that change worked. Could it be that this line should be something different? It looks fine to me but not sure.

[pramukta]

Okay, so I figured out why the no_custom_string_conversion code was being called. Inside the write_json method of json_msgpack_sax.hpp on for example lines 312, 327, 330, there are non-templated recursive calls to write_json which means things default to the no_custom_string_conversion code.

This still doesn't make the interop work but at least it runs the WAMP specific code.

[jpetso]

Ha! Excellent find, thanks. I'll merge it in a second. Let's try changing encoded.at(1) to encoded.at(0) and see if that makes the interop work?

[pramukta]

Cool, tried to make that change but it doesn't seem to fix things. It looks like the issue at this point is that the BIN type records are getting base64 encoded on the way in but not converted back on the way out. Will continue to look.

[pramukta]

I'm at a bit of a block. Basically, if the BIN type payload bytes are base64 encoded before being sent out as JSON the reader on the other end would need to know that when decoding the JSON string, which it doesn't seem like the WAMP client can know. Could that section be replaced with just writing the bytes out and letting rapidjson handle the escaping? It amounts to making this function look more like the bit commented out of this function.

[jpetso]

The other side can figure it out because WAMP specifies a null character at the beginning of a binary string. i.e.:

• begins with \0: base64-encoded, maps to BIN
• anything else: not encoded, maps to STR

[pramukta]

oh ok, didn't know that. I guess it's Autobahn|JS that doesn't respect that part of the spec yet.

[jpetso]

Yeah, hm, I don't know how JavaScript WAMP clients deal with this in general - JSON doesn't have a binary type, so I wonder if they just leave it up to the user to detect and handle these cases correctly.

Either way for bonefish it should amount to a simple passthrough. Are you aware of a (relatively) straightforward test to check whether that behaviour works in a standard-compliant way, independently of how client APIs might handle BIN vs STR?

[jrogers]

It seems like many of the other wamp implementations don't support the spec in this case...

On Fri, Feb 5, 2016 at 2:54 PM, Jakob Petsovits notifications@github.com wrote:

Yeah, hm, I don't know how JavaScript WAMP clients deal with this in general - JSON doesn't have a binary type, so I wonder if they just leave it up to the user to detect and handle these cases correctly.

Either way for bonefish it should amount to a simple passthrough. Are you aware of a (relatively) straightforward test to check whether that behaviour works in a standard-compliant way, independently of how client APIs might handle BIN vs STR?

— Reply to this email directly or view it on GitHub https://github.com/tplgy/bonefish/issues/33#issuecomment-180529103.

[jpetso]

It seems like the merged commit should fix the bonefish side of the issue. Please reopen if you find any remaining/new shortcomings in the JSON/msgpack conversion code.

[pramukta]

Cool, thanks, will do.