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

Add option to not accept maps with duplicate keys #161

Merged
merged 1 commit into from
Nov 8, 2021
Merged

Add option to not accept maps with duplicate keys #161

merged 1 commit into from
Nov 8, 2021

Conversation

KubqoA
Copy link
Contributor

@KubqoA KubqoA commented Nov 2, 2021

This PR adds a Decoder option to prevent the decoder from parsing maps with duplicate keys, which is one of the valid ways to handle duplicate map keys as described in the CBOR RFC.

Citing from CBOR RFC 8949:
3.1 Major types, Major type 5

A map that has duplicate keys may be well-formed, but it is not valid, and thus it causes indeterminate decoding; see also Section 5.6.

5.6. Specifying Keys for Maps

When processing maps that exhibit entries with duplicate keys, a generic decoder might do one of the following:

  • Not accept maps with duplicate keys ...
  • Pass all map entries to the application ...
  • Lose some entries with duplicate keys, e.g., deliver only the final (or first) entry out of the entries with the same key ...
    Generic decoders need to document which of these three approaches they implement.

I believe this package chose the third option, although not documented, but in my use of CBOR I found it quite useful to have an option to not accept maps with duplicate keys. I decided to implement it via a Decoder option as to not break backwards compatibility.

@hildjj
Copy link
Owner

hildjj commented Nov 2, 2021

I agree this is interesting. I'll get to doing a full review in a day or two. In the meantime, can you add yourself to the contributors in the packages/cbor/package.json file, please?

"contributors": [

@KubqoA
Copy link
Contributor Author

KubqoA commented Nov 3, 2021

Thanks! Added myself to the contributors and also added a few more tests.

@KubqoA
Copy link
Contributor Author

KubqoA commented Nov 8, 2021

Hi, sorry to bother you, just wanted to ask if you had any time to look at the changes? Thanks :)

Copy link
Owner

@hildjj hildjj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nits. Otherwise great patch!
Will merge after nit fix.

packages/cbor/lib/decoder.js Outdated Show resolved Hide resolved
packages/cbor/lib/decoder.js Outdated Show resolved Hide resolved
packages/cbor/lib/decoder.js Outdated Show resolved Hide resolved
packages/cbor/test/decoder.ava.js Outdated Show resolved Hide resolved
@KubqoA
Copy link
Contributor Author

KubqoA commented Nov 8, 2021

Made the changes, also found some eslint errors in my code, so fixed those as well. Hope everything is good now :),
oh and thanks for your time to review this 👍

@hildjj hildjj merged commit caca0bc into hildjj:main Nov 8, 2021
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

Successfully merging this pull request may close these issues.

2 participants