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

Emptish objects fail to roundtrip cbor marshal & unmarshal #10

Closed
dignifiedquire opened this issue Nov 13, 2017 · 8 comments
Closed

Emptish objects fail to roundtrip cbor marshal & unmarshal #10

dignifiedquire opened this issue Nov 13, 2017 · 8 comments

Comments

@dignifiedquire
Copy link
Contributor

Trying some simple edge cases and got these issues

Emptyish objects that fail to round trip

(fmt.Printf("%s - %s\n", obj, reflect.TypeOf(obj)))

  • [] - interface[]{}: panic: cborEncoder stack overpopped (marshal)
  • [<nil>] - interface[]{}: Invalid majorByte: 0xff (unmarshal)
  • <nil> - %!s(<nil)): panic: cborEncoder stack overpopped (marshal)

(Setup is as described in #9)

warpfork added a commit that referenced this issue Nov 13, 2017
Two sources of problem:

- the wrong byte was being encoded for nil...!  Youch.
- a stack pop was performed as if nils weren't terminals, which could cause panics.

Both should now be fixed.

The fixtures we already had for empty and zero values should've caught this... but unfortunately we hadn't mapped them onto corresponding CBOR byte fixtures.  That oversight is now fixed (and we added more fixtures, to boot), but we should also really fix the root cause of that oversight and write a test that we've got enough tests mapped consistently in all directions >.<

Thanks to dignifiedquire for his stellar bug reporting.  This should fix issue #10.  (The "invalid major byte" from unmarshal was actually a valid complaint; it's also fixed by fixing the wrong byte emitted from the marshal side.)

Signed-off-by: Eric Myhre <hash@exultant.us>
@warpfork
Copy link
Member

Ok, I think all these are fixed on master as well.

Thank you for this heroic and excellent bug reporting!

(I'm particularly embarrassed in this case -- there were fixtures on the token side meant to cover precisely these cases... but no mappings defined on the matching the cbor side. It's to the point I should be writing tests to make sure the texts are fully filling out the $n*m$ matrix...!)

@dignifiedquire
Copy link
Contributor Author

Thanks for the quick turn around, I have some more issues I am afraid

  • [] gets turned into nil if I round trip it
  • {"a":"IPFS","b":null,"c":[1]} gets turned into {"a":"IPFS","b":"IPFS","c":[1]} in a round trip

@dignifiedquire
Copy link
Contributor Author

It would probably be useful to add a bunch of test vectors, making sure things work as expected. Some I have used in the past and might be useful

@warpfork
Copy link
Member

Sorry E_TIME_BUDGET happened a bit... I'll try to allocate some time this evening.

At first glance, these are probably issues with nil-vs-typed-nil being handled inconsistently in the obj mapper package rather than a cbor problem. So I'll be focusing on adding unit tests and fixtures there.

Those borc tools look useful...! Maybe with a little work I can rig them around the refmt CLI tool to get a lot of acceptance test coverage there. Though this might require some rather odd subcommands like refmt cbor=obj=cbor, which would have very little sensible purpose other than testing.

@warpfork
Copy link
Member

warpfork commented Nov 16, 2017

Fixed the issue with nulls, thanks.

I can't reproduce your issue with []. Am I missing something?

@dignifiedquire
Copy link
Contributor Author

Thanks, I'll check it out. There is always the chance my code is the issue :)

@dignifiedquire
Copy link
Contributor Author

Turns out the [] was an issue on our side thanks a lot for the work.

@warpfork
Copy link
Member

Even more test coverage is a very acceptable "problem" to have as a sideeffect :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants