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

Decoding containers with (potentially)-empty elements #123

Closed
jsbean opened this issue Aug 8, 2019 · 5 comments · Fixed by #152
Closed

Decoding containers with (potentially)-empty elements #123

jsbean opened this issue Aug 8, 2019 · 5 comments · Fixed by #152
Assignees

Comments

@jsbean
Copy link
Contributor

jsbean commented Aug 8, 2019

We have a use case where we need to decode an array of potentially empty elements.

Currently, unkeyed and keyed containers holding onto empty (e.g., a struct with no fields) or a potentially empty (e.g., a struct with all optional fields) elements does not decode.

Say we've got a struct like this:

struct Empty: Equatable, Codable { }

If we have some XML like this:

let xml = """
<container>
    <empty/>
    <empty/>
    <empty/>
</container>
"""

It would be nice to decode like this:

let decoded = try XMLDecoder().decode([Empty].self, from: xml.data(using: .utf8)!)
// Throws "Expected Empty but found null instead."

The poetics are strong.

Wrapping it up a bit, consider this:

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

Given the xml from above, we can try to decode it like this:

let decoded = try XMLDecoder().decode(EmptyArray.self, from: xml.data(using: .utf8)!)
// Throws "Expected Array<Empty> value but found null instead."

Lastly, given this container:

struct EmptyWrapper {
    let empty: Empty
}

And we have some XML like this:

let xml = """
<wrapper>
    <empty/>
</wrapper>
"""

And we try to decode it like this:

let decoded = try XMLDecoder().decode(EmptyWrapper.self, from: xml.data(using: .utf8)!)
// Throws "Expected Empty value but found null instead"

We get the same outcomes if you substitute the Empty type with something like this:

struct MaybeEmpty {
    var idunno: Int?
}

If this seems like something that should theoretically be supported, I can give a go at it. It smells like it might touch similar neighborhoods of the codebase as #122.

@MaxDesiatov
Copy link
Collaborator

Thank you for reporting this. I previously also had an attempt at this, or a similar case related to optionals and empty elements, and I wasn't able to fix and at the same time to keep all existing tests passing. If you see a way to make this work well, please go ahead!

@JUSTINMKAUFMAN
Copy link

@MaxDesiatov Any word on this?

I have the same issue decoding the following xml (see the part with <qtcodec/>):

<codec>
    <appspecificdata>
        <appname>Final Cut Pro</appname>
        <appmanufacturer>Apple Inc.</appmanufacturer>
        <data>
            <qtcodec/>
        </data>
    </appspecificdata>
</codec>

If there is no official answer to this as of yet, I'm curious to know what you're doing @jsbean to handle this case in the interim (both for decoding and encoding)?

Thanks, and kudos on a great library!

@MaxDesiatov MaxDesiatov self-assigned this Oct 15, 2019
@jsbean
Copy link
Contributor Author

jsbean commented Oct 15, 2019

Hi @JUSTINMKAUFMAN,

We made a patch for this in this PR on our fork. We haven't submitted it here yet because it changes an expectation in one test (@bwetherfield mentions it in the PR), and because we have been focused on other things in the meantime. Our patch is working great for us, though!

What kinda value is qtcodec supposed to decode into? We have also changed things a little on our fork so that an empty element like <qtcodec/> could represent "".

Perhaps give our fork on master a try. If this works for folks other than us (and @MaxDesiatov accepts a slight change in semantics), we will gladly submit a PR here!

@JUSTINMKAUFMAN
Copy link

Thanks @jsbean that works perfectly!

@MaxDesiatov
Copy link
Collaborator

MaxDesiatov commented Oct 15, 2019

Thanks folks! My main reason for being cautious with this is the effort to maintain widest possible compatibility in CoreXLSX. But I need to double check, maybe the fork works fine in most important cases and slight breakage in backward compatibility would be ok. I'll keep the issue open until that's verified, and as usual I'm open to any possible improvements.

MaxDesiatov pushed a commit that referenced this issue 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 issue 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
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants