Yams icon indicating copy to clipboard operation
Yams copied to clipboard

Fix `YAMLDecoder`'s handling when decoding empty strings into optional types

Open liamnichols opened this issue 3 years ago • 2 comments

Background

I was checking the feasibility of using YAMLDecoder to decode a simple JSON file instead of having to switch between JSONDecoder, but I noticed a discrepancy in a really simple example:

let json = """
    {
      "access": ""
    }
    """

struct Container: Decodable {
    let access: String?
}

let decoded = try YAMLDecoder().decode(Container.self, from: json)
XCTAssertEqual(decoded.access, "") // failed: ("nil") is not equal to ("Optional("")")

This was odd, because the problem does not surface when using Yams.load(yaml:):

let json = """
    {
      "access": ""
    }
    """

let decoded = try XCTUnwrap(try Yams.load(yaml: json) as? NSDictionary)
XCTAssertEqual(decoded, ["access": ""]) // success

It was then where I discovered #301.

Description

Disclaimer: This is my first time ever looking at this codebase, so I likely will have missed something šŸ™

Following the hints from @JensAyton, I could see in NSNull.construct(from:) that we weren't factoring in the style of the input scalar, which meant that technically this method would treat 'null' or '' as null. This however never seemed to surface as an issue when using Yams.load(yaml:) since the calling methods would perform additional checks on things like the node tag. _Decoder.decodeNil() on the other hand does not.

While I could have implemented a fix in _Decoder.decodeNil(), I felt that it might be more appropriate to put it in NSNull.construct(from:), which I did here.

In addition to the fix, I added extra test coverage to reproduce the issue and I also had to make one small adjustment to NodeTests due to this change.. I'll add a comment inline.

Finally, I also want to reiterate another comment from @JensAyton:

However, I’m leery of making a PR with this change since it may impact existing code with unfortunate dependencies on the current behaviour.

I feel the same way. I'd very much appreciate thorough eyes on this. I've added what I think is appropriate test coverage, but if you think I need to do more, or if there is a better approach to fix this, please let me know šŸ™

liamnichols avatar Apr 06 '22 11:04 liamnichols

I checked out the CI failures and corrected some SwiftLint violations that it was erroring on, but I'm not too sure why the rest of the CI jobs failed. If you have any suggestions on what I can do to debug/replicate this locally that will help. Thanks

liamnichols avatar Apr 11 '22 08:04 liamnichols

@jpsim I've just rebased this PR with the latest changes from main and I was just wondering if you could please trigger the CI checks again? šŸ™

liamnichols avatar Jun 09 '22 20:06 liamnichols