Fail on duplicate keys in a mapping #416
Conversation
|
|
||
| ##### Bug Fixes | ||
|
|
||
| * Yams will now correctly error when it tries to decode a mapping with duplicate keys. |
There was a problem hiding this comment.
This is technically a bug fix but it may be breaking if people have malformed yaml they are trying to decode with this. imo this is the correct spot for this but happy to move it to "Breaking" instead if you feel that would be more apt
|
Thanks for the PR! This is a welcome change, and doing this at the Swift layer is fine. Based on some GitHub discussions, it does look like the C API can detect duplicate keys. There's also this issue with relevant discussion: yaml/pyyaml#41 However, I think it's important that if we start to error in these cases, that 1) it be recoverable (e.g. we don't fail the whole decoding) and 2) the error is reported at the correct location (mark), right now 1:1 is being hardcoded as the location. If you rebase or merge main you should have CI in a healthier state to run again. |
|
hmmm ok, I'll need a bit of support on both of those due to my lack of familiarity here:
|
# By JP Simard (3) and others # Via GitHub * upstream/main: Require Swift 5.7 or later (jpsim#424) Add empty changelog section Release 5.1.3 add support for riscv64 (jpsim#419) Add support for Android (jpsim#421) GitHub Actions workflow fixes (jpsim#422) add os(visionOS) support (jpsim#418) Bazel: support rules_swift 2.x (jpsim#420) Bump rexml from 3.2.5 to 3.2.8 # Conflicts: # CHANGELOG.md
|
Updated the error to notify of all duplicated keys and their locations by creating a new custom error type that can give you actionable feedback for duplicated keys, no matter how many times a key is duplicated/how many keys are duplicates. I haven't updated it to be recoverable bc personally I don't believe it should recover - imo it should just throw immediately (but happy to make updates to make it recoverable if you give me guidance on how you think that should be done) |
| let node = Node.mapping(.init(pairs, tag(firstEvent.mappingTag), event.mappingStyle, firstEvent.startMark)) | ||
| if let anchor = firstEvent.mappingAnchor { | ||
| anchors[anchor] = node | ||
| } | ||
| return node | ||
| } | ||
|
|
||
| private func checkDuplicates(mappingKeys: [Node]) throws { | ||
| var duplicates: [String: [Node]] = [:] | ||
| for key in mappingKeys { |
There was a problem hiding this comment.
lines 368-377 (define var, for loop, filter) could just be
let duplicateKeys = Dictionary(grouping: keys, by: { $0.string! }).filter { $1.count > 1 }
but I wanted to avoid the force unwrap on $0.string. if we're confident that these keys will always be scalars (which I think they should be but I'm not sure about that) then I can just switch to that simpler form
|
hey @jpsim, apologies for the tag but I'd love another review here when you have a minute 😅 |
This fixes #415 but it is a slightly "naive" implementation. It's possible that the "correct" place to throw an error from would be the C code rather than the swift code, but I do not have the knowledge to make those changes 😓
Please let me know what you think!