kriszyp / cbor-x

Ultra-fast CBOR encoder/decoder with extensions for records and structural cloning
MIT License
276 stars 32 forks source link

Edge case with nested records sharing the same keys #68

Closed atomictag closed 1 year ago

atomictag commented 1 year ago

Hi there. I am having an issue with encoding custom extensions inside a record when both the wrapping record and the wrapped extension share exactly the same keys. Example (note: standard setup of extension and config of encoder omitted for readability):

class MyExtension {
  constructor(value) {
    this.key = value;
  }
}
const extensionData = new MyExtension("foo");
const recordData = { key : extensionData }
const encoded = encode(recordData); // wrong encoded CBOR: d9dfff8319e00081636b6579d9a606d9e0008163666f6f

In the above example everything works fine if we change the property key of either the record or the extension (say we use key_ext for the extension). Now the encoded byte stream is correct: d9dfff8319e00081636b6579d9a606d9dfff8319e00181676b65795f65787463666f6f

Note that this issue can be reproduced with any number of keys - as long as the enclosing record has the same keys as the enclosed object. Actually this seems to be unrelated to extensions and can be reproduced with plain records and a new Encoder that supports records:

{"key":{"key":"foo"}} => d9dfff8319e00081636b6579d9e0008163666f6f // NOT OK
{"key":{"key_alt":"foo"}} => d9dfff8319e00081636b6579d9dfff8319e00181676b65795f616c7463666f6f // OK

At first glance the issue seem to be rooted in writeObject(...) https://github.com/kriszyp/cbor-x/blob/master/encode.js#L663 where the transition is not renewed unless the enclosed object has different keys (didn't go much farther than that as I am not sure what "transitions" are used for).

Many thanks!

atomictag commented 1 year ago

Here's a reduced case that might help:


    const encoder = new Encoder();
    // { key : { key : "foo" }} => KO
    {
      const r_key = "key";
      const d_key = "key";
      const data = { [r_key]: { [d_key]: "foo" } };
      const enc = encoder.encode(data);
      const dec = encoder.decode(enc);
      expect(dec).not.toEqual(data); // KO  
    }
    // { key : { key_alt : "foo" }} => OK
    {
      const r_key = "key";
      const d_key = "key_alt";
      const data = { [r_key]: { [d_key]: "foo" } };
      const enc = encoder.encode(data);
      const dec = encoder.decode(enc);
      expect(dec).toEqual(data); // OK
    }```
atomictag commented 1 year ago

I've just tried the above test with the configuration

    const encoder = new Encoder({
      structures: [], // <--- initialized structures 
    });

and that seems enough to get rid of the issue, albeit I have to admit I am not sure why

kriszyp commented 1 year ago

Should be fixed in v1.5.1. Thanks for the report!

atomictag commented 1 year ago

Thank you for looking into this. The new release fixes the problem. Closing this issue. Thanks again!