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.

Sign up for GitHub

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

Jump to bottom

Fix Decoding of Empty String #145

Merged
merged 23 commits into from Nov 13, 2019
Merged

Fix Decoding of Empty String #145

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
f2ce406
Treat corner case of empty string value intrinsic
jsbean Sep 29, 2019
3adaa40
Remove xcodeproj junk
jsbean Sep 29, 2019
e544f76
Add some logging to assess where we're at
jsbean Sep 29, 2019
5ea4fa7
Add cases for empty string as null element decoding
bwetherfield Sep 30, 2019
f9fc2f4
Swiftformat
bwetherfield Sep 30, 2019
2e40e6a
Merge pull request #47 from bwetherfield/wetherfield/empty-element-em…
jsbean Sep 30, 2019
3ccbfb3
Transform precondition to where clause in switch statement
jsbean Sep 30, 2019
800a26a
Remove print statements
jsbean Sep 30, 2019
c4dabba
Remove nested syntax test
bwetherfield Oct 16, 2019
64f016c
Merge branch 'master' into fix-empty-string-empty-elt
bwetherfield Oct 16, 2019
29ee5d9
Merge branch 'master' into fix-empty-string-empty-elt
MaxDesiatov Oct 17, 2019
4bac491
Fix forcecast
bwetherfield Nov 2, 2019
882eca2
Merge branch 'master' into fix-empty-string-empty-elt
bwetherfield Nov 2, 2019
e14594b
Fix formatting
bwetherfield Nov 2, 2019
a966be6
Merge branch 'fix-empty-string-empty-elt' of https://github.com/bweth…
bwetherfield Nov 2, 2019
8b99000
Update LinuxMain
bwetherfield Nov 2, 2019
0222d24
Remove warnings
bwetherfield Nov 2, 2019
023dc0d
Merge branch 'master' into fix-empty-string-empty-elt
bwetherfield Nov 11, 2019
27cbb5d
Add fix for new empty string issue
bwetherfield Nov 11, 2019
085a12b
Add back missing missingTest - remove "value" special casing from imp…
bwetherfield Nov 11, 2019
8691214
Fix formatting
bwetherfield Nov 11, 2019
f4209dd
Recover missing test!
bwetherfield Nov 13, 2019
e7347b3
Update linuxmain
bwetherfield Nov 13, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 10 additions & 0 deletions Sources/XMLCoder/Decoder/XMLDecoderImplementation.swift
View file
Open in desktop
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([("value", 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
24 changes: 21 additions & 3 deletions Sources/XMLCoder/Decoder/XMLKeyedDecodingContainer.swift
View file
Open in desktop
Expand Up @@ -249,9 +249,12 @@ extension XMLKeyedDecodingContainer {
return []
}
} else {
#warning("TODO: just return keyedBox.elements[key.stringValue]")
return keyedBox.elements[key.stringValue].map {
if let singleKeyed = $0 as? SingleKeyedBox {
return singleKeyed.element
#warning("Don't get rid of key info just yet!")
// return singleKeyed.element
return singleKeyed
} else {
return $0
}
Expand Down Expand Up @@ -282,6 +285,13 @@ 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 == "value" || key.stringValue == "" {
return "" as! T
MaxDesiatov marked this conversation as resolved.
Show resolved Hide resolved
}

switch strategy(key) {
case .attribute:
guard
Expand Down Expand Up @@ -317,7 +327,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 @@ -334,7 +353,6 @@ extension XMLKeyedDecodingContainer {
"Expected \(type) value but found null instead."
))
}

return unwrapped
}

Expand Down
66 changes: 66 additions & 0 deletions Tests/XMLCoderTests/EmptyElementEmptyStringTests.swift
View file
Open in desktop
@@ -0,0 +1,66 @@
//
// 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
}

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)
}
}
9 changes: 0 additions & 9 deletions Tests/XMLCoderTests/Minimal/StringTests.swift
View file
Open in desktop
Expand Up @@ -25,15 +25,6 @@ class StringTests: XCTestCase {
("foobar", "foobar"),
]

func testMissing() {
let decoder = XMLDecoder()

let xmlString = "<container />"
let xmlData = xmlString.data(using: .utf8)!

XCTAssertThrowsError(try decoder.decode(Container.self, from: xmlData))
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this test pass? If so, I'd prefer to keep it in the test suite

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've played around with this, and it does work if the element is renamed to, say, notValue, but I'm encountering an ambiguity with the treatment of the value intrinsic. Would you be content with this if we pushed through the proposed change from value to xmlValue as the labeling of the intrinsic?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(or even limiting the intrinsic to "")

Copy link
Collaborator

Choose a reason for hiding this comment

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

"" and "xmlValue" are both fine, "xmlValue" (or everything that starts with "xml" for that matter) is not a valid element or attribute, at least according to my understanding of the XML standard:

Names beginning with the string "xml", or with any string which would match (('X'|'x') ('M'|'m') ('L'|'l')), are reserved for standardization in this or future versions of this specification.

Copy link
Collaborator

Choose a reason for hiding this comment

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

On the other hand, isn't decoding the value of <container /> element to an empty string is the expected behaviour here? I think I also relied on this in CoreXLSX in some of its behavior if I remember correctly, but this needs to be confirmed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This might seem a bit self-centered, but if it doesn't break CoreXLSX, I'm probably fine with the change, hope MusicXML test suite would pass as well. Just wondering how many other apps and libraries we could break with this change and what would be possible workarounds in that case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hoping we can perhaps circumvent these concerns by changing the use of the "value" coding key, per my latest PR! It does mean downstream users would have to introduce = "" additions, but since this change is probably on the way anyway, I wonder if now is as good a time as any...?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@bwetherfield Will this test be added back?

func testAttribute() throws {
let decoder = XMLDecoder()
let encoder = XMLEncoder()
Expand Down
4 changes: 4 additions & 0 deletions XMLCoder.xcodeproj/project.pbxproj
View file
Open in desktop
Expand Up @@ -21,6 +21,7 @@
/* End PBXAggregateTarget section */

/* Begin PBXBuildFile section */
07E441BA2340F14B00890F46 /* EmptyElementEmptyStringTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 07E441B92340F14B00890F46 /* EmptyElementEmptyStringTests.swift */; };
OBJ_148 /* BoolBox.swift in Sources */ = {isa = PBXBuildFile; fileRef = OBJ_12 /* BoolBox.swift */; };
OBJ_149 /* Box.swift in Sources */ = {isa = PBXBuildFile; fileRef = OBJ_13 /* Box.swift */; };
OBJ_150 /* ChoiceBox.swift in Sources */ = {isa = PBXBuildFile; fileRef = OBJ_14 /* ChoiceBox.swift */; };
Expand Down Expand Up @@ -152,6 +153,7 @@
/* End PBXContainerItemProxy section */

/* Begin PBXFileReference section */
07E441B92340F14B00890F46 /* EmptyElementEmptyStringTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = EmptyElementEmptyStringTests.swift; sourceTree = "<group>"; };
OBJ_100 /* DecimalTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = DecimalTests.swift; sourceTree = "<group>"; };
OBJ_101 /* EmptyTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = EmptyTests.swift; sourceTree = "<group>"; };
OBJ_102 /* FloatTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = FloatTests.swift; sourceTree = "<group>"; };
Expand Down Expand Up @@ -440,6 +442,7 @@
OBJ_119 /* NoteTest.swift */,
OBJ_120 /* PlantCatalog.swift */,
OBJ_121 /* PlantTest.swift */,
07E441B92340F14B00890F46 /* EmptyElementEmptyStringTests.swift */,
OBJ_122 /* RJISample.swift */,
OBJ_123 /* RJITest.swift */,
OBJ_124 /* RelationshipsTest.swift */,
Expand Down Expand Up @@ -716,6 +719,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 */,
OBJ_255 /* UIntTests.swift in Sources */,
OBJ_256 /* URLTests.swift in Sources */,
Expand Down
4 changes: 0 additions & 4 deletions XMLCoder.xcodeproj/xcshareddata/xcschemes/XMLCoder.xcscheme
View file
Open in desktop
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