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

[liamnichols]

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]

Some inline commentary 🙏

[liamnichols]

Before the change to NSNull.construct(yaml:), these tests passed anyway. This is because of other stuff that is done when calling Yams.load_all(yaml:). I just made the changes here anyway to highlight the expectation incase things change in the future.

[liamnichols]

without the patch, these are the failures:

XCTAssertEqual(container.A, "") // failed: ("nil") is not equal to ("Optional("")")
XCTAssertEqual(container.B, nil)
XCTAssertEqual(container.C, nil)
XCTAssertEqual(container.D, nil)
XCTAssertEqual(container.E, "") // failed: ("nil") is not equal to ("Optional("")")
XCTAssertEqual(container.json.F, "") // failed: ("nil") is not equal to ("Optional("")")
XCTAssertEqual(container.json.G, "null") // failed: ("nil") is not equal to ("Optional("null")") 
XCTAssertEqual(container.array, ["one", "", nil, "null", "~"]) // failed: ("[Optional("one"), nil, nil, nil, nil]") is not equal to ("[Optional("one"), Optional(""), nil, Optional("null"), Optional("~")]")

[liamnichols]

I had to make this change here since the construct method will no longer construct NSNull if the style isn't .plain.

Have I missed anything here, is that ok? I'm not familiar how this is used elsewhere but I think its ok? Do we have to worry about styles like .any? Or is that more only for when using?

This is the only thing that makes me wonder if the patch should have been inside _Decoder.decodeNil() instead as I'm not sure if it was intentional or not?

Let me know if there are better tests I can add to verify this.

[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]

@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? 🙏

[mattpolzin]

@jpsim the issue this PR attempts to fix is a bit tricky to work around downstream of the parsing (sometimes impossible without accepting both things you want to parse successfully and also things you don't want to parse successfully).

If you get the chance to revisit the issue, I'd love to know whether this PR (assuming conflicts were fixed) has some chance of getting merged, or whether you'd prefer a different solution, or whether you don't anticipate accepting a fix for the associated bug now or in the future.

[jpsim]

Hi everyone, really sorry for the long delay here, but I've merged these changes in 0c4ff78 and will be cutting a new major release in the next few days, considering that this may be a breaking change for some users.

Thanks for everyone's contributions and patience here. ❤️

[mattpolzin]

Thanks @jpsim! Really appreciate you looping back on this, not to mention all the other effort you've put into Yams.

[liamnichols]

Thanks @jpsim, much appreciated 🙏