search

bpsm / edn-java

a reader for extensible data notation
Eclipse Public License 1.0
100 stars 24 forks source link

Duplicate map keys are parsed without error #49

Closed achamberlin-lendup closed 7 years ago

achamberlin-lendup commented 8 years ago

{:a 1 :a 2} parses without error in edn-java

whereas

user=> (clojure.edn/read-string "{:a 1 :a 2}")

IllegalArgumentException Duplicate key: :a  clojure.lang.PersistentArrayMap.createWithCheck (PersistentArrayMap.java:71)

The edn spec states that keys should appear "at most once" so I think an error should be reported in this scenario.

bpsm commented 8 years ago

Thanks for the report (and the patch). You're right, that is a bug. I had no computer with me over the weekend, but I spent some time thinking through the issue and have identified a few things I need to consider before deciding how to proceed. I'd be interested to hear your thoughts on them:

  1. Should the parser error on sets with duplicate elements?
  2. Should the parser error on duplicate keys in maps only if their values differ?
  3. Is DefaultMapFactory's CollectionBuilder the correct place for this fix?

This would mean that only callers using the default map factory will see this change in behavior. Callers who provide their own MapFactory will not benefit from this fix, and indeed, checking for duplicate keys will probably need to become part of the contract of CollectionBuilder.

achamberlin-lendup commented 8 years ago

re: 1 & 2 yes, I think sets should raise an exception if they contain non-unique values, and that maps regardless of their values should only contain a key once.

FYI in my case, we use edn as a configuration file format and duplicate map keys led to an ambiguous configuration that would have been better off failing at parse time.

re: 3 I'm not that familiar with the edn-java codebase. My PR was more of a guess, and if you need to reject it in order to fix the issue in a more general way, no offense taken.

achamberlin-lendup commented 7 years ago

@bpsm is there any line-of-sight to addressing this issue? I see your concern about 3.) above, but having this safety in the DefaultMapFactory is better than none at all.

bpsm commented 7 years ago

I'm hoping to release 0.5.0 this weekend, which will include a fix for this issue.

bpsm commented 7 years ago

fixed in 0.5.0