Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions Sources/Yams/Constructor.swift
Original file line number Diff line number Diff line change
Expand Up @@ -396,6 +396,10 @@ extension NSNull/*: ScalarConstructible*/ {
///
/// - returns: An instance of `NSNull`, if one was successfully extracted from the scalar.
public static func construct(from scalar: Node.Scalar) -> NSNull? {
// When constructing from a Scalar, only plain style scalars should be recognized.
// For example #"key: 'null'"# or #"key: ''"# should not be considered as null.
guard case .plain = scalar.style else { return nil }

switch scalar.string {
case "", "~", "null", "Null", "NULL":
return NSNull()
Expand Down
10 changes: 7 additions & 3 deletions Tests/YamsTests/ConstructorTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -210,14 +210,16 @@ class ConstructorTests: XCTestCase { // swiftlint:disable:this type_body_length
english: null
~: null key
---
# This sequence has five
# entries, two have values.
# This sequence has seven
# entries, four have values.
sparse:
- ~
- 2nd entry
-
- 4th entry
- Null
- 'null'
- ''
Comment on lines -213 to +222
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


"""
let objects = Array(try Yams.load_all(yaml: example))
Expand All @@ -234,7 +236,9 @@ class ConstructorTests: XCTestCase { // swiftlint:disable:this type_body_length
"2nd entry",
NSNull(),
"4th entry",
NSNull()
NSNull(),
"null",
""
]
]
]
Expand Down
2 changes: 1 addition & 1 deletion Tests/YamsTests/NodeTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ class NodeTests: XCTestCase {
let scalarFloat: Node = "1.0"
XCTAssertEqual(scalarFloat.float, 1.0)

let scalarNull: Node = "null"
let scalarNull = Node("null", .implicit, .plain)
XCTAssertEqual(scalarNull.null, NSNull())
Comment on lines -62 to 63
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


let scalarInt: Node = "1"
Expand Down
46 changes: 46 additions & 0 deletions Tests/YamsTests/TopLevelDecoderTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -32,5 +32,51 @@ class TopLevelDecoderTests: XCTestCase {
)
XCTAssertEqual(foo?.name, "Bird")
}

func testDecodeOptionalTypes() throws {
let yaml = """
AAA: ''
BBB:
CCC: null
DDD: ~
EEE: ""
json: {
"FFF": "",
"GGG": "null"
}
array:
- one
- ''
- null
- 'null'
- '~'
"""

struct Container: Codable, Equatable {
struct JSON: Codable, Equatable {
var FFF: String?
var GGG: String?
}

var AAA: String?
var BBB: String?
var CCC: Int?
var DDD: String?
var EEE: String?
var json: JSON
var array: [String?]
}

let container = try YAMLDecoder().decode(Container.self, from: yaml)

XCTAssertEqual(container.AAA, "")
XCTAssertEqual(container.BBB, nil)
XCTAssertEqual(container.CCC, nil)
XCTAssertEqual(container.DDD, nil)
XCTAssertEqual(container.EEE, "")
XCTAssertEqual(container.json.FFF, "")
XCTAssertEqual(container.json.GGG, "null")
XCTAssertEqual(container.array, ["one", "", nil, "null", "~"])
}
}
#endif