Skip to content

Commit

Permalink
Fix Decoding of Empty String (#145)
Browse files Browse the repository at this point in the history
Prior to this fix, we found that decoding an empty string (represented as a null element), `<string/>` was confusing the decoder.

* Treat corner case of empty string value intrinsic
* Remove xcodeproj junk
* Add some logging to assess where we're at
* Add cases for empty string as null element decoding
* Swiftformat
* Transform precondition to where clause in switch statement
* Remove print statements
* Remove nested syntax test
* Fix forcecast
(cherry picked from commit e6ec42a)
* Fix formatting
* Update LinuxMain
* Remove warnings
* Add fix for new empty string issue
* Add back missing missingTest - remove "value" special casing from implementation
* Fix formatting
* Recover missing test!
* Update linuxmain
  • Loading branch information
bwetherfield authored and MaxDesiatov committed Nov 13, 2019
1 parent bf09326 commit 0eba4c6
Show file tree
Hide file tree
Showing 6 changed files with 115 additions and 7 deletions.
10 changes: 10 additions & 0 deletions Sources/XMLCoder/Decoder/XMLDecoderImplementation.swift
Expand Up @@ -101,6 +101,12 @@ class XMLDecoderImplementation: Decoder {
attributes: KeyedStorage()
))
))
case let containsEmpty as SingleKeyedBox where containsEmpty.element is NullBox:
return KeyedDecodingContainer(XMLKeyedDecodingContainer<Key>(
referencing: self, wrapping: SharedBox(KeyedBox(
elements: KeyedStorage([("", StringBox(""))]), attributes: KeyedStorage()
))
))
case let keyed as SharedBox<KeyedBox>:
return KeyedDecodingContainer(XMLKeyedDecodingContainer<Key>(
referencing: self,
Expand Down Expand Up @@ -212,6 +218,10 @@ extension XMLDecoderImplementation {
let value = keyedBox.withShared({ $0.value as? B })
else { throw error }
return value
case let singleKeyedBox as SingleKeyedBox:
guard let value = singleKeyedBox.element as? B
else { throw error }
return value
case is NullBox:
throw error
case let keyedBox as KeyedBox:
Expand Down
20 changes: 17 additions & 3 deletions Sources/XMLCoder/Decoder/XMLKeyedDecodingContainer.swift
Expand Up @@ -234,7 +234,7 @@ extension XMLKeyedDecodingContainer {
.withShared { keyedBox -> [KeyedBox.Element] in
keyedBox.elements[key.stringValue].map {
if let singleKeyed = $0 as? SingleKeyedBox {
return singleKeyed.element
return singleKeyed.element.isNull ? singleKeyed : singleKeyed.element
} else {
return $0
}
Expand Down Expand Up @@ -264,6 +264,12 @@ extension XMLKeyedDecodingContainer {
return empty
}

// If we are looking at a coding key value intrinsic where the expected type is `String` and
// the value is empty, return `""`.
if strategy(key) != .attribute, elements.isEmpty, attributes.isEmpty, type == String.self, key.stringValue == "", let emptyString = "" as? T {
return emptyString
}

switch strategy(key) {
case .attribute:
guard
Expand Down Expand Up @@ -299,7 +305,16 @@ extension XMLKeyedDecodingContainer {
let value: T?
if !(type is AnySequence.Type), let unkeyedBox = box as? UnkeyedBox,
let first = unkeyedBox.first {
value = try decoder.unbox(first)
// Handle case where we have held onto a `SingleKeyedBox`
if let singleKeyed = first as? SingleKeyedBox {
if singleKeyed.element.isNull {
value = try decoder.unbox(singleKeyed)
} else {
value = try decoder.unbox(singleKeyed.element)
}
} else {
value = try decoder.unbox(first)
}
} else {
value = try decoder.unbox(box)
}
Expand All @@ -316,7 +331,6 @@ extension XMLKeyedDecodingContainer {
"Expected \(type) value but found null instead."
))
}

return unwrapped
}

Expand Down
71 changes: 71 additions & 0 deletions Tests/XMLCoderTests/EmptyElementEmptyStringTests.swift
@@ -0,0 +1,71 @@
//
// EmptyElementEmptyStringTests.swift
// XMLCoderTests
//
// Created by James Bean on 9/29/19.
//

import XCTest
import XMLCoder

class EmptyElementEmptyStringTests: XCTestCase {
struct Parent: Equatable, Codable {
let thing: Thing
}

struct Thing: Equatable, Codable {
let attribute: String?
let value: String

enum CodingKeys: String, CodingKey {
case attribute
case value = ""
}
}

func testEmptyElementEmptyStringDecoding() throws {
let xml = """
<thing></thing>
"""
let expected = Thing(attribute: nil, value: "")
let result = try XMLDecoder().decode(Thing.self, from: xml.data(using: .utf8)!)
XCTAssertEqual(expected, result)
}

func testEmptyElementEmptyStringWithAttributeDecoding() throws {
let xml = """
<thing attribute="x"></thing>
"""
let expected = Thing(attribute: "x", value: "")
let result = try XMLDecoder().decode(Thing.self, from: xml.data(using: .utf8)!)
XCTAssertEqual(expected, result)
}

func testArrayOfEmptyElementStringDecoding() throws {
let xml = """
<container>
<thing></thing>
<thing attribute="x"></thing>
<thing></thing>
</container>
"""
let expected = [
Thing(attribute: nil, value: ""),
Thing(attribute: "x", value: ""),
Thing(attribute: nil, value: ""),
]
let result = try XMLDecoder().decode([Thing].self, from: xml.data(using: .utf8)!)
XCTAssertEqual(expected, result)
}

func testNestedEmptyElementEmptyStringDecoding() throws {
let xml = """
<parent>
<thing/>
</parent>
"""
let expected = Parent(thing: Thing(attribute: nil, value: ""))
let result = try XMLDecoder().decode(Parent.self, from: xml.data(using: .utf8)!)
XCTAssertEqual(expected, result)
}
}
13 changes: 13 additions & 0 deletions Tests/XMLCoderTests/XCTestManifests.swift
Expand Up @@ -253,6 +253,18 @@ extension DynamicNodeEncodingTest {
]
}

extension EmptyElementEmptyStringTests {
// DO NOT MODIFY: This is autogenerated, use:
// `swift test --generate-linuxmain`
// to regenerate.
static let __allTests__EmptyElementEmptyStringTests = [
("testArrayOfEmptyElementStringDecoding", testArrayOfEmptyElementStringDecoding),
("testEmptyElementEmptyStringDecoding", testEmptyElementEmptyStringDecoding),
("testEmptyElementEmptyStringWithAttributeDecoding", testEmptyElementEmptyStringWithAttributeDecoding),
("testNestedEmptyElementEmptyStringDecoding", testNestedEmptyElementEmptyStringDecoding),
]
}

extension EmptyTests {
// DO NOT MODIFY: This is autogenerated, use:
// `swift test --generate-linuxmain`
Expand Down Expand Up @@ -796,6 +808,7 @@ public func __allTests() -> [XCTestCaseEntry] {
testCase(DecodingContainerTests.__allTests__DecodingContainerTests),
testCase(DynamicNodeDecodingTest.__allTests__DynamicNodeDecodingTest),
testCase(DynamicNodeEncodingTest.__allTests__DynamicNodeEncodingTest),
testCase(EmptyElementEmptyStringTests.__allTests__EmptyElementEmptyStringTests),
testCase(EmptyTests.__allTests__EmptyTests),
testCase(EnumAssociatedValueTestComposite.__allTests__EnumAssociatedValueTestComposite),
testCase(ErrorContextTest.__allTests__ErrorContextTest),
Expand Down
4 changes: 4 additions & 0 deletions XMLCoder.xcodeproj/project.pbxproj
Expand Up @@ -21,6 +21,7 @@
/* End PBXAggregateTarget section */

/* Begin PBXBuildFile section */
07E441BA2340F14B00890F46 /* EmptyElementEmptyStringTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 07E441B92340F14B00890F46 /* EmptyElementEmptyStringTests.swift */; };
4A062D4F2341924E009BCAC1 /* CombineTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 4A062D4E2341924E009BCAC1 /* CombineTests.swift */; };
B5EA3BB6230F237800D8D69B /* NestedChoiceArrayTest.swift in Sources */ = {isa = PBXBuildFile; fileRef = B5EA3BB4230F235C00D8D69B /* NestedChoiceArrayTest.swift */; };
B5F74472233F74E400BBDB15 /* RootLevelAttributeTest.swift in Sources */ = {isa = PBXBuildFile; fileRef = B5F74471233F74E400BBDB15 /* RootLevelAttributeTest.swift */; };
Expand Down Expand Up @@ -158,6 +159,7 @@
/* End PBXContainerItemProxy section */

/* Begin PBXFileReference section */
07E441B92340F14B00890F46 /* EmptyElementEmptyStringTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = EmptyElementEmptyStringTests.swift; sourceTree = "<group>"; };
4A062D4E2341924E009BCAC1 /* CombineTests.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = CombineTests.swift; sourceTree = "<group>"; };
B5EA3BB4230F235C00D8D69B /* NestedChoiceArrayTest.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = NestedChoiceArrayTest.swift; sourceTree = "<group>"; };
B5F74471233F74E400BBDB15 /* RootLevelAttributeTest.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = RootLevelAttributeTest.swift; sourceTree = "<group>"; };
Expand Down Expand Up @@ -456,6 +458,7 @@
OBJ_119 /* NoteTest.swift */,
OBJ_120 /* PlantCatalog.swift */,
OBJ_121 /* PlantTest.swift */,
07E441B92340F14B00890F46 /* EmptyElementEmptyStringTests.swift */,
D18FBFB72348FAE500FA4F65 /* QuoteDecodingTest.swift */,
OBJ_124 /* RelationshipsTest.swift */,
OBJ_122 /* RJISample.swift */,
Expand Down Expand Up @@ -738,6 +741,7 @@
OBJ_251 /* KeyedTests.swift in Sources */,
OBJ_252 /* NullTests.swift in Sources */,
OBJ_253 /* OptionalTests.swift in Sources */,
07E441BA2340F14B00890F46 /* EmptyElementEmptyStringTests.swift in Sources */,
OBJ_254 /* StringTests.swift in Sources */,
B5EA3BB6230F237800D8D69B /* NestedChoiceArrayTest.swift in Sources */,
OBJ_255 /* UIntTests.swift in Sources */,
Expand Down
4 changes: 0 additions & 4 deletions XMLCoder.xcodeproj/xcshareddata/xcschemes/XMLCoder.xcscheme
Expand Up @@ -39,8 +39,6 @@
</BuildableReference>
</TestableReference>
</Testables>
<AdditionalOptions>
</AdditionalOptions>
</TestAction>
<LaunchAction
buildConfiguration = "Debug"
Expand All @@ -61,8 +59,6 @@
ReferencedContainer = "container:XMLCoder.xcodeproj">
</BuildableReference>
</MacroExpansion>
<AdditionalOptions>
</AdditionalOptions>
</LaunchAction>
<ProfileAction
buildConfiguration = "Release"
Expand Down

0 comments on commit 0eba4c6

Please sign in to comment.