Detect and report all duplicated keys #170
Conversation
| ?.let { throw DuplicateKeysException(it) } | ||
| } | ||
|
|
||
| private tailrec fun accumulateDuplicateKeys( |
There was a problem hiding this comment.
This function and iteration over all nodes could be avoided if something like YamlNodeFactory was added and make it its responsibility to calculate duplicates (and possibly do other stuff for different node types). However, I didn't want to change too much in the code structure.
| @@ -267,7 +267,7 @@ private sealed class YamlMapLikeInputBase(map: YamlMap, context: SerializersModu | |||
| protected fun <T> fromCurrentValue(action: YamlInput.() -> T): T { | |||
| try { | |||
| return action(currentValueDecoder) | |||
| } catch (e: YamlException) { | |||
There was a problem hiding this comment.
Should this stay as YamlException?
There was a problem hiding this comment.
Based on the current code only SinglePathYamlException can be thrown here. It probably would be more future–proof to catch 'em all and do more checks inside. But if you'd go with not traversing the whole tree the root exception could stay the same so this change would be reverted.
| init { | ||
| val keys = entries.keys.sortedWith { a, b -> | ||
| val lineComparison = a.location.line.compareTo(b.location.line) | ||
| private val encounteredKeys = mutableMapOf<String, YamlScalar>() |
There was a problem hiding this comment.
Is there a reason why we wouldn't move these into the init block and just immediately throw the exception in there?
There was a problem hiding this comment.
If exception is thrown immediately there, there is no way to traverse the whole tree and accumulate duplicates.
However, after reading your comment I believe it is something you'd prefer.
| expect({ parser.decodeFromString<MapStructure>(yaml) }).toThrow<DuplicateKeysException> { | ||
| message.toBe( | ||
| """ | ||
| Found duplicates of key 'key1' defined at line 1, column 1: |
There was a problem hiding this comment.
Perhaps the error message would be clearer if it read as something like this:
Key 'key1' is only allowed once, but is defined multiple times:
- at line 1, column 1
- at line 2, column 1
What do you think?
There was a problem hiding this comment.
Yes, this looks better. I didn't have a good idea for the message.
|
Just had an idea that might make this change less invasive - we could continue to use the "old" So if we had this input YAML: some:
thing:
key1: blah
key2: foo
key1: bar
key2: bazWe'd end up with a I don't think we need to report all duplicates in all locations throughout the tree. Doing this would be quite different behaviour to all the other exceptions kaml currently throws, as kaml currently just throws an exception on the first issue, rather than trying to aggregate all of them. How does that sound? |
In that case it is not something I was hoping for so it would be better to close this PR (or adjust it to your needs) as I'll need to fork it or make a local copy in the project I'm currently writing. |
This is binary and source incompatible change as exceptions hierarchy and structure is changed. If library users rely on path properties of
YamlExceptionthey will not find them in that type anymore. I decided to change it this way to make sure that everyone still catches all exceptions asDuplicateKeysExceptiondoes not extend previousYamlException(nowSinglePathYamlException).I created a separate test suite due to #27.
Closes #169