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

encoding object with / field should drop other feilds #86

Open
Gozala opened this issue Dec 12, 2022 · 1 comment
Open

encoding object with / field should drop other feilds #86

Gozala opened this issue Dec 12, 2022 · 1 comment

Comments

@Gozala
Copy link
Contributor

Gozala commented Dec 12, 2022

Here is an example code to reproduce the problem:

const block = json.encode({
   hello: {
     '/': 'baguqeeratmwuhl736sndm4bi34xbifhyjqhatgwjrq6vjkfiafl7253rv4sq',
     message: 'hello'
   }
})
   
const data = json.decode(block) // Error: Invalid encoded CID form

I would expect encoder to just ignore message field of the hello and encode it as link as opposed to encoding it into an invalid block.

@rvagg
Copy link
Member

rvagg commented Dec 19, 2022

Background re decoding, see https://ipld.io/specs/codecs/dag-json/spec/#parse-rejection-modes-in-the-reserved-namespace - one of the main reasons for the strictness around the "/" reserved space is the difficulty in tokenized parsing and needing to buffer tokens in order to make decisions about what you're dealing with.

Re encoding .. yeah that's an interesting case and you're right that we probably should be not allowing encoding of invalid forms!

I'm not in favour of silently dropping fields, however. I think in this case it should error on encode rather than apparently succeeding but losing some of your data. The "this is not right" signal should bubble up from the encoder to be dealt with by a different layer accordingly. If you want a layer to massage invalid data into an encodable form then that's fine but I don't think it belongs directly in the encode() of the codec - something like dag-pb's prepare() might be OK for that. Similar to the discussion in ipld/js-dag-cbor#57.

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