Map decode will fail for some stored values

[ghost]

The decode logic for a map will test for nil values (192) to determine if a map entry was removed. However, a map which has a int32 type, for example, can have the first byte with a value of 192. For example, the number 3520 will be encoded as 192, 13, 0, 0 but will be decoded as a nil value (i.e. the entry was removed). This will happen for any encoded value where 192 is a valid first byte (uint8, int8, uint16, etc.)

This logic tests for the nil values

schema/src/Schema.ts

Line 356 in 76a8cac

if (decode.nilCheck(bytes, it)) {

and calls the onRemove event. Removed values should be encoded in a different way, maybe we could add a byte to indicate the removal or not of the key before the new value, such that this byte can then have a magic value and the actual value of the field can be anything?

[endel]

Wow, that's a nice catch, thanks for the detailed report, and diving deep into the problem. I'm going to check this as soon as possible!

[ghost]

I made some changes to the tests that catch this problem. In order to do this, I created a helper function that will convert the encoded array to a "byte" array (where all values are in the [0,255] range), like the websocket would do in a normal client/server interaction.
While this works, making sure the result of encoding primitives is always a byte array might be a better alternative, as there is no risk of forgetting to convert values for testing.
You can take a look at the changes here. If this is fine I will continue to test my solution for this problem like this, as changing the encoding logic should probably be a different issue.

[endel]

Nice, thanks @colombotapps, I've been experimenting with your proposal and made sure we use bytes from 0 to 255 internally during encoding. I've adapted the tests with the expected bytes for encoding, which is on master now.

I've also added a failing test for the current issue here: https://github.com/colyseus/schema/blob/master/test/EdgeCasesTest.ts#L9-L39

Once this is fixed, I'm going to push a version 0.5.0 of colyseus/schema, and bump the Colyseus version itself, since this going to be a breaking change for everyone.

Cheers!

[endel]

Hi @colombotapps, I've just created a PR to fix this (#45). Next step is to apply the patch on all decoders to use the new NIL order

(Note that the decoder implementations for other languages hosted on this repository are outdated, the most up-to-date ones are on each Colyseus client repository)

[endel]

Hi @colombotapps, you mind checking if colyseus@0.11.17 + latest C# client fixes this issue for you? 😇

@Wenish has set-up a nice pipeline to auto-generate a ZIP file containing the Unity plugin files in the releases now: https://github.com/colyseus/colyseus-unity3d/releases/tag/0.11.2

There's a plugins-0.11.2.zip file available for download there :)

Cheers!