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 Decoding of Arrays of Empty Elements #152

Merged
merged 41 commits into from Nov 17, 2019

Conversation

bwetherfield
Copy link
Collaborator

@bwetherfield bwetherfield commented Nov 14, 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

<container>
    <empty/>
    <empty/>
    <empty/>
</container>

into the following type

struct EmptyArray: Equatable, Codable {
    enum CodingKeys: String, CodingKey { case empties = "empty" }
    let empties: [Empty]
}

where

struct Empty: Equatable, Codable { }

We can also decode a value of the following type

struct EmptyWrapper {
    let empty: Empty
}

from

<wrapper>
    <empty/>
</wrapper>

Further, following from #145 we can decode arrays of empty strings

<container>
    <string-type/>
    <string-type>My neighbors are empty<string-type>
    <string-type/>
</container>

as

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 and in the following error-throwing test case.

bwetherfield and others added 30 commits August 22, 2019 23:03
…element-empty-string

Final Fix for empty element / empty string !
Transform precondition to where clause in switch statement
…-array-empty-string-test

Empty Strings and Empty Elements Clarification Sweep
Refactor XMLKeyedDecodingContainer.decodeConcrete elements massaging
Copy link
Collaborator

@MaxDesiatov MaxDesiatov left a comment

Choose a reason for hiding this comment

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

Overall LGTM, but with regards to compatibility concerns I wonder if there's a way to distinguish <empty/> from <empty></empty>, so that the former is decoded as nil, but the latter as ""?

@MaxDesiatov MaxDesiatov added the enhancement New feature or request label Nov 14, 2019
@bwetherfield
Copy link
Collaborator Author

I can't quite figure out if

    func parser(_: XMLParser,
                didStartElement elementName: String,
                namespaceURI: String?,
                qualifiedName: String?,
                attributes attributeDict: [String: String] = [:])

and

    func parser(_: XMLParser,
                didEndElement _: String,
                namespaceURI _: String?,
                qualifiedName _: String?)

in XMLStackParser get called differently for these two cases in the XMLParser implementation. If so, then maybe we could tweak the parser delegate implementation, but I'm not too sure. AFAIK, XML implementations wouldn't tend to distinguish in this way as a rule, so would this cause compatibility issues more broadly?

Copy link
Collaborator

@MaxDesiatov MaxDesiatov left a comment

Choose a reason for hiding this comment

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

I've just tested this with CoreXLSX, and while changes were required, they did make sense. Distinguishing between <element/> and <element></element> wasn't required after all. On balance, it's probably worth breaking compatibility here for the sake of general improvements.

@MaxDesiatov
Copy link
Collaborator

@bwetherfield thank you for polishing this and finding new edge cases! 🙏

@MaxDesiatov MaxDesiatov merged commit 3a7fd96 into CoreOffice:master Nov 17, 2019
@MaxDesiatov MaxDesiatov added the breaking change The change is not backward compatible label Nov 17, 2019
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
Labels
breaking change The change is not backward compatible enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Decoding containers with (potentially)-empty elements
3 participants