Fix Decoding of Empty String #145
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…element-empty-string Final Fix for empty element / empty string !
houndci-bot
reviewed
Oct 16, 2019
bwetherfield
changed the title
(cherry picked from commit e6ec42a)
…erfield/XMLCoder into fix-empty-string-empty-elt
bwetherfield
changed the title
|
This one is now ready for your review, @MaxDesiatov ! Thanks |
MaxDesiatov
reviewed
Nov 2, 2019
|
|
||
| XCTAssertThrowsError(try decoder.decode(Container.self, from: xmlData)) | ||
| } | ||
|
|
There was a problem hiding this comment.
Does this test pass? If so, I'd prefer to keep it in the test suite
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
(or even limiting the intrinsic to "")
There was a problem hiding this comment.
"" 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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...?
There was a problem hiding this comment.
@bwetherfield Will this test be added back?
MaxDesiatov
reviewed
Nov 2, 2019
There was a problem hiding this comment.
This is good, thanks for discovering this edge case @bwetherfield! I'm a bit worried about the removed test function though, hope it's easy to add it back and then it's ready to be merged 👍
MaxDesiatov
changed the title
MaxDesiatov
pushed a commit
that referenced
this pull request
Nov 11, 2019
## Overview Restricts the special case usage of `"value"` OR `""` as a coding key, as implemented in #73 to just `""`. `"value"` resumes functioning as a normal coding key. ## Example As noted in #145, `"value"` is starting to collide awkwardly in one or two cases where it may be _implicit_ per the special case usage ```swift <value-containing>I am implicitly labeled</value-containing> ``` or _explicit_, as in ```swift <value-containing> <value>I am explicitly labeled</value> </value-containing> ``` With the change proposed, the latter example will be unambiguously captured by ```swift struct ValueContaining: Codable { let value: String } ``` while the former is equivalent to ```swift struct ValueContaining: Codable { let value: String enum CodingKeys: String, CodingKey { case value = "" } } ``` ## Source Compatability This is a **breaking change** for downstream users of the special `"value"` coding key. All such uses should be replaced by `case value = ""` subject to this PR's inclusion.
|
@bwetherfield I'm ready to review this again when conflicts are resolved 🙂 |
|
Thanks, @MaxDesiatov ! I also need to add back the missing test :) Will keep you posted |
bwetherfield
force-pushed
the
fix-empty-string-empty-elt
branch
from
2a840ef to
0222d24
Compare
houndci-bot
reviewed
Nov 11, 2019
|
OK - should be ready for review! Thanks, @MaxDesiatov |
MaxDesiatov
reviewed
Nov 12, 2019
|
My apologies, @MaxDesiatov - should be in now! |
MaxDesiatov
approved these changes
Nov 13, 2019
MaxDesiatov
pushed a commit
that referenced
this pull request
Nov 17, 2019
## Overview Fixes #123 and extends #145 to accommodate decoding of arrays of empty strings or mixed arrays of non-empty and empty strings. ## Example We may now decode ```xml <container> <empty/> <empty/> <empty/> </container> ``` into the following type ```swift struct EmptyArray: Equatable, Codable { enum CodingKeys: String, CodingKey { case empties = "empty" } let empties: [Empty] } ``` where ```swift struct Empty: Equatable, Codable { } ``` We can also decode a value of the following type ```swift struct EmptyWrapper { let empty: Empty } ``` from ```xml <wrapper> <empty/> </wrapper> ``` Further, following from #145 we can decode arrays of empty strings ```xml <container> <string-type/> <string-type>My neighbors are empty<string-type> <string-type/> </container> ``` as ```swift struct EmptyArray: Equatable, Codable { enum CodingKeys: String, CodingKey { case stringType = "string-type" } let stringType: [String] } ``` and variants. ## Source Compatibility In cases where we decode an array of type `[String?]`, an empty element is now decoded as `""` rather than `nil`, the justification being that `String` can itself take empty values. We use the convention that `nil` signifies the absence of an element (if 0 or 1 of the element are allowed) as in the updated [BreakfastTest](https://github.com/bwetherfield/XMLCoder/blob/0d20614e47df98d1a10174e992c585edf629c9b9/Tests/XMLCoderTests/BreakfastTest.swift) and in the following error-throwing [test case](https://github.com/MaxDesiatov/XMLCoder/blob/2855777ff868e8a4c1d944c7da0ddb49a8b3893e/Tests/XMLCoderTests/Minimal/NullTests.swift#L56-L68). * Add nested choice unkeyed container decoding test * Fix nested unkeyed container implementation for nested keyed box * Clean up unwrapping syntax * 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 * Add failing test for a nested array of empty-string value intrinsic elements * Do a little cleanup of single keyed box passing around * Refactor XMLKeyedDecodingContainer.decodeConcrete elements massaging * Remove xcscheme junk * Add fix for empty arrays, wrapped empties etc * Clean up * Revert singleKeyed dive change * Accommodate singleKeyed reading options * Alter Border Test * Add test case that returns [nil] (exists a non-optional property) * Eliminate possibly empty Int from Breakfast test * Fix formatting * Fix forcecast * Fix formatting * Update LinuxMain * Fix tests such that null elements read as empty strings * Fix linux main * Add nested array of empty strings decoding in the explicit style * Add mixed case empty and non-empty string cases * Reinstate missing test * Add test for decoding a null element into an optional type
arjungupta0107
pushed a commit
to salido/XMLCoder
that referenced
this pull request
Jun 26, 2020
## Overview Restricts the special case usage of `"value"` OR `""` as a coding key, as implemented in CoreOffice#73 to just `""`. `"value"` resumes functioning as a normal coding key. ## Example As noted in CoreOffice#145, `"value"` is starting to collide awkwardly in one or two cases where it may be _implicit_ per the special case usage ```swift <value-containing>I am implicitly labeled</value-containing> ``` or _explicit_, as in ```swift <value-containing> <value>I am explicitly labeled</value> </value-containing> ``` With the change proposed, the latter example will be unambiguously captured by ```swift struct ValueContaining: Codable { let value: String } ``` while the former is equivalent to ```swift struct ValueContaining: Codable { let value: String enum CodingKeys: String, CodingKey { case value = "" } } ``` ## Source Compatability This is a **breaking change** for downstream users of the special `"value"` coding key. All such uses should be replaced by `case value = ""` subject to this PR's inclusion.
arjungupta0107
pushed a commit
to salido/XMLCoder
that referenced
this pull request
Jun 26, 2020
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
arjungupta0107
pushed a commit
to salido/XMLCoder
that referenced
this pull request
Jun 26, 2020
## Overview Fixes CoreOffice#123 and extends CoreOffice#145 to accommodate decoding of arrays of empty strings or mixed arrays of non-empty and empty strings. ## Example We may now decode ```xml <container> <empty/> <empty/> <empty/> </container> ``` into the following type ```swift struct EmptyArray: Equatable, Codable { enum CodingKeys: String, CodingKey { case empties = "empty" } let empties: [Empty] } ``` where ```swift struct Empty: Equatable, Codable { } ``` We can also decode a value of the following type ```swift struct EmptyWrapper { let empty: Empty } ``` from ```xml <wrapper> <empty/> </wrapper> ``` Further, following from CoreOffice#145 we can decode arrays of empty strings ```xml <container> <string-type/> <string-type>My neighbors are empty<string-type> <string-type/> </container> ``` as ```swift struct EmptyArray: Equatable, Codable { enum CodingKeys: String, CodingKey { case stringType = "string-type" } let stringType: [String] } ``` and variants. ## Source Compatibility In cases where we decode an array of type `[String?]`, an empty element is now decoded as `""` rather than `nil`, the justification being that `String` can itself take empty values. We use the convention that `nil` signifies the absence of an element (if 0 or 1 of the element are allowed) as in the updated [BreakfastTest](https://github.com/bwetherfield/XMLCoder/blob/0d20614e47df98d1a10174e992c585edf629c9b9/Tests/XMLCoderTests/BreakfastTest.swift) and in the following error-throwing [test case](https://github.com/MaxDesiatov/XMLCoder/blob/2855777ff868e8a4c1d944c7da0ddb49a8b3893e/Tests/XMLCoderTests/Minimal/NullTests.swift#L56-L68). * Add nested choice unkeyed container decoding test * Fix nested unkeyed container implementation for nested keyed box * Clean up unwrapping syntax * 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 * Add failing test for a nested array of empty-string value intrinsic elements * Do a little cleanup of single keyed box passing around * Refactor XMLKeyedDecodingContainer.decodeConcrete elements massaging * Remove xcscheme junk * Add fix for empty arrays, wrapped empties etc * Clean up * Revert singleKeyed dive change * Accommodate singleKeyed reading options * Alter Border Test * Add test case that returns [nil] (exists a non-optional property) * Eliminate possibly empty Int from Breakfast test * Fix formatting * Fix forcecast * Fix formatting * Update LinuxMain * Fix tests such that null elements read as empty strings * Fix linux main * Add nested array of empty strings decoding in the explicit style * Add mixed case empty and non-empty string cases * Reinstate missing test * Add test for decoding a null element into an optional type
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Prior to this fix, we found that decoding an empty string (represented as a null element),
<string/>was confusing the decoder. I can't seem to find any issues logged. Perhaps @jsbean has an idea where these might be hiding...?I'm also building up to a PR that addresses the array-of-empty-elements issue, but I am trying to disentangle the changes we merged into a separate branch!