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 choice elements that can hold empty structs #120

Merged
merged 3 commits into from Jul 31, 2019

Conversation

bwetherfield
Copy link
Collaborator

Introduction

In merging in #119, we fixed most but not quite all of #91! Decoding of null choice elements (represented as enums with empty struct associated values on the Swift side) still results in errors. This PR adds a test to demonstrate and fixes this issue by wrapping each NullBox inside of a SingleKeyedBox at the merge phase (called by transformToBoxTree).

Motivation

One of the main lessons from #119 was that we have to wrap choice elements in the decoding phase to hold onto their keys. The keys are needed for directing us to the correct branch of the do-catch pyramid used for decoding.

private enum Entry: Equatable {
    case run(Run)
    case properties(Properties)
    case br(Break)
}

extension Entry: Decodable {
    private enum CodingKeys: String, XMLChoiceCodingKey {
        case run, properties, br
    }

    public init(from decoder: Decoder) throws {
        let container = try decoder.container(keyedBy: CodingKeys.self)
        do {
            self = .run(try container.decode(Run.self, forKey: .run))
        } catch {
            do {
                self = .properties(try container.decode(Properties.self, forKey: .properties))
            } catch {
                self = .br(try container.decode(Break.self, forKey: .br))
            }
        }
    }
}

where one of the associated values could be an empty struct (represented by null):

private struct Break: Decodable {}

Although we can throw out keys for non-choice null elements, a mechanism is needed for holding onto the keys while transforming from the XMLCoderElement tree to the boxTree. Only later will we know if the key is needed (if this wrapped element is transformed to a ChoiceBox); if not, we will be able to throw out the key.

Proposed solution

The Public API is unchanged. On the implementation side, we catch NullBox values in merge and wrap them in SingleKeyedBox instances.

Detailed Design

In merge, we wrap each NullBox in a SingleKeyedBox with the appropriate key bundled in. An XMLChoiceDecodingContainer can be constructed from the SingleKeyedBox by converting it to a ChoiceBox (just transferring over the contents) - as normal. In XMLKeyedDecodingContainer, when preparing the elements for concrete decoding, we unwrap all SingleKeyedBox values that may be contained therein, as any choice elements contained would have already been transformed to a ChoiceBox by this point in decoding: any stray SingleKeyedBox wrappers can thus be thrown out.

Source compatibility

This is purely an additive change.

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.

Fantastic, thank you @bwetherfield!

@MaxDesiatov MaxDesiatov merged commit 4ce74e2 into CoreOffice:master Jul 31, 2019
@bwetherfield
Copy link
Collaborator Author

So quick! Thank you, @MaxDesiatov

arjungupta0107 pushed a commit to salido/XMLCoder that referenced this pull request Jun 26, 2020
## Introduction

In merging in CoreOffice#119, we fixed most but not quite all of CoreOffice#91! Decoding of _null_ choice elements (represented as enums with empty struct associated values on the Swift side) still results in errors. This PR adds a test to demonstrate and fixes this issue by wrapping each `NullBox` inside of a `SingleKeyedBox` at the `merge` phase (called by `transformToBoxTree`).

## Motivation

One of the main lessons from CoreOffice#119 was that we have to wrap choice elements in the decoding phase to hold onto their keys. The keys are needed for directing us to the correct branch of the do-catch pyramid used for decoding. 

```swift
private enum Entry: Equatable {
    case run(Run)
    case properties(Properties)
    case br(Break)
}

extension Entry: Decodable {
    private enum CodingKeys: String, XMLChoiceCodingKey {
        case run, properties, br
    }

    public init(from decoder: Decoder) throws {
        let container = try decoder.container(keyedBy: CodingKeys.self)
        do {
            self = .run(try container.decode(Run.self, forKey: .run))
        } catch {
            do {
                self = .properties(try container.decode(Properties.self, forKey: .properties))
            } catch {
                self = .br(try container.decode(Break.self, forKey: .br))
            }
        }
    }
}
```
where one of the associated values could be an empty struct (represented by null):

```swift
private struct Break: Decodable {}
```

Although we _can_ throw out keys for non-choice null elements, a mechanism is needed for holding onto the keys while transforming from the `XMLCoderElement` tree to the `boxTree`. Only later will we know if the key is needed (if this wrapped element is transformed to a `ChoiceBox`); if not, we will be able to throw out the key. 

## Proposed solution

The Public API is unchanged. On the implementation side, we catch `NullBox` values in `merge` and wrap them in `SingleKeyedBox` instances. 

## Detailed Design

In `merge`, we wrap each `NullBox` in a `SingleKeyedBox` with the appropriate key bundled in. An `XMLChoiceDecodingContainer` can be constructed from the `SingleKeyedBox` by converting it to a `ChoiceBox` (just transferring over the contents) - as normal. In `XMLKeyedDecodingContainer`, when preparing the `elements` for concrete decoding, we unwrap all `SingleKeyedBox` values that may be contained therein, as any choice elements contained would have already been transformed to a `ChoiceBox` by this point in decoding: any stray `SingleKeyedBox` wrappers can thus be thrown out. 

## Source compatibility

This is purely an additive change.
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 this pull request may close these issues.

None yet

2 participants