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

Dynamic node encoding + new formatters + various fixes #70

Merged
merged 20 commits into from Feb 9, 2019

Conversation

JoeMatt
Copy link
Contributor

@JoeMatt JoeMatt commented Jan 31, 2019

Dynamic node encoding

Introduction

The main part of this PR adds a protocol DynamicNodeEncoding that attempts to improve the use of custom encoding strategies.

This solution is related to #45 , not too dissimilar to @MaxDesiatov RFC in the comments Link. Though this PR is only attempting to resolve the use of node encoding.

This PR includes some commits beyond which are discussed later. Most of those commits could be cherry picked out if rejected though the reasoning for them is hopefully thoroughly explained.

Purpose

The current major shortcoming in XMLEncoder is that the default encoding strategy is to always encode nodes as Elements. In order to override this default, a closure must be passed to each new instance of XMLEncoder, which is only provided the CodingKey path and Encoder instance.

Deep knowledge of all the CodingKey's for each possible encountered struct of class is required in a monolothic switch statement, which makes for difficult refactoring and mostly impossible for dealing with dynamically generated structures or structures with private or otherwise unknown CodingKeys.

This protocol moves the ability for models themselves to determine how they should be translated into XML.

Code

bce416a

Example

    <book id="123">
        <id>123</id>
        <title>Cat in the Hat</title>
        <category main="Y">Kids</category>
        <category main="N">Wildlife</category>
    </book>
private struct Book: Codable, Equatable, DynamicNodeEncoding {
    let id: UInt
    let title: String
    let categories: [Category]

    private enum CodingKeys: String, CodingKey {
        case id
        case title
        case categories = "category"
    }

    static func nodeEncoding(forKey key: CodingKey) -> XMLEncoder.NodeEncoding {
        switch key {
        case Book.CodingKeys.id: return .both
        default: return .element
        }
    }
}

Additional changes

Bool encoding

Commit: 05e3c2d

Purpose

Currently bools are only generated from string of exactly 1, 0, true, false.
These are technically the only valid values according to the XML spec, but in Cocoa values of Yes and NO are commonly encountered. The XML spec is trying to dictate type inference, but since Swift is statically typed, and we're only matching to a known key type, it's of my opinion that it's within reason to allow a programmer to convert non-conforming Boolean like String values to Swift Bool's without requiring custom encode functions.

This commit allows for:

  • Case insensitivity
  • Addition of yes, no, y, 'n

Additional default key encodings

5dbe1d7

Purpose

Default convenience of common XML key casings in addition to the current snake case key conversion.

  • lowercased
  • UPPERCASED
  • Capitalized (common in SOAP-XML)

This commit also changes the extensions on String to StringProtocol to allow for more flexibility on code reuse.

SharedBoxProtocol: Generalize for any Box inheritance

9ae94dc

Purpose

SharedBox has a generic type Unboxed:Boxed but the default implementation of func unbox() returns the concrete protocol type Box, instead of the generic inherited type of Unboxed. This requires some additional type checking in other parts of the code.

It additionally adds a type removed protocol with a default implementation of func unbox() that retains the current behavior of returning the non-generic protocol, to avoid if lets that don't work with protocols with associated types, but where simply conforming to Box protocol is all that is required.

This overall cleans up some the code and removes some fail() calls and type checking in a later commit.

Empty string key value encoding fix

551d484

Purpose

In the case where a codable provides an empty string for the codable string value for an instance variable an empty bracket was inserted which is invalid XML.

let attr = "bar"
let value = "FOO"
enum CodingKeys : String, CodingKey {
    case attr
    case value = ""
}

Will be useful for unkeyed objects that contain only attributes eg;

<box attr="bar"><>FOO</></box>
<!-- Would now correctly become -->
<box attr="bar">FOO</box>

This is related to #12 , this is the encoding case. I'm still working on another PR for the decoding of un-keyed intrinsics with attributes

Additional considerations

In the event that more than 1 element exists, this will still produce what is most likely invalid XML. It's possible that this solution would need to be extended to throw if any other elements exist in past or future element encodings.

XMLEncoder: Add both option to value encoding, refactor encoder

bd24a08

Purpose

Similar to the case of #45, where an attribute and element have the same key value, this allows encoding the same value in both places.

This is an kind of an odd case, but in real world use of XMLCoder, I've run into a SOAP specification where XML elements in the payload have the same value in both an attribute and element position. Adding a both optional allowed using the DynamicNodeEncoding protocols still instead of requiring a custom encoding function for the model with duplicated attribute/element.

It refactors the complex switch statement slightly as well to use local closures for better readability and reusing the same code for the both case. It also adds some type aliases on the closures used in attribute encoding to make editing and reading a little easier.

Fix warnings

5eedcea

Purpose

Swift lint was spitting out a lot of warnings presently.

Resolved:

  • Long files were refactored into new files to split up different structures
  • Init's moved to extensions.
    This allows for 1, the Swift compiler to auto-generate the default initializer (all custom init's and conformance to protocols that auto-generate inits such as Codable need to be in extensions in order for the Swift compiler to auto-generate the default initializers. That default initializer can then be called in additional initializers in extensions which helps reduce complexity in refactoring, adding, removing instance variables to those structs.

Unresolved:

  • TODOs were left
  • XMLCoderElement.flatten() still warns about complexity, but it was slightly improved by converting a mutable array builder to a immutable reduce() function that's more Swifty. It should be a little easier now to refactor the reduce closure to something less complex.

@MaxDesiatov
Copy link
Collaborator

Fantastic stuff, thank you very much @JoeMatt! Would it be hard to make this pass CI? Please let me know if you need any help with that.

There's also a discussion about enabling XPath queries in #71 , possibly passed in CodingKey.rawValue, which could also help with this problem. I wonder if it would make sense to have 2 APIs for this, one for XPath and another like DynamicNodeEncoding from this PR with no runtime overhead for parsing XPath. So users could go with XPath for concise code, but no compile time validation of XPath queries and some runtime overhead. Or they could use DynamicNodeEncoding which is proper Swift with strong typing, a bit more verbose but faster at run time.

@JoeMatt
Copy link
Contributor Author

JoeMatt commented Jan 31, 2019

@MaxDesiatov Sure no prob, I'll take a look at the CI logs now that the report is generated.

I also experimented a bit with using Swift KeyPaths or extensions to CodingKey to make something similar to the XPath solution for nested containers but without the performance issues of XPath like you stated.

I'll see what I can come up with since I may need something similar to finish the project I'm using XMLCoder on.

@MaxDesiatov
Copy link
Collaborator

Interesting to see how KeyPath solution would look like. I'm not sure I understand how those could be a good fit as you'd need to express a path within an XML, which most of the time doesn't have corresponding properties on Swift side to get a key path formed.

@codecov
Copy link

codecov bot commented Jan 31, 2019

Codecov Report

Merging #70 into master will decrease coverage by 1.22%.
The diff coverage is 72.95%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #70      +/-   ##
==========================================
- Coverage   75.65%   74.42%   -1.23%     
==========================================
  Files          31       37       +6     
  Lines        1491     1736     +245     
==========================================
+ Hits         1128     1292     +164     
- Misses        363      444      +81
Impacted Files Coverage Δ
Sources/XMLCoder/Decoder/XMLDecoder.swift 76.03% <ø> (+7.39%) ⬆️
Sources/XMLCoder/Auxiliaries/Box/BoolBox.swift 100% <100%> (ø) ⬆️
Sources/XMLCoder/Auxiliaries/Box/SharedBox.swift 100% <100%> (ø) ⬆️
...urces/XMLCoder/Auxiliaries/String+Extensions.swift 100% <100%> (ø) ⬆️
Sources/XMLCoder/Auxiliaries/Box/Box.swift 100% <100%> (ø)
...rImplementation+SingleValueEncodingContainer.swift 100% <100%> (ø)
Sources/XMLCoder/Auxiliaries/Box/KeyedBox.swift 100% <100%> (ø) ⬆️
Sources/XMLCoder/Encoder/XMLEncoder.swift 79.2% <45.45%> (-2.95%) ⬇️
Sources/XMLCoder/Encoder/DynamicNodeEncoding.swift 50% <50%> (ø)
...s/XMLCoder/Encoder/XMLKeyedEncodingContainer.swift 40% <50%> (+3.63%) ⬆️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bd22c20...32f58ee. Read the comment docs.

…percased

Some helpers to deal with converting auto-generated codable String values for instance variables to match some common XML key coding standards to the commonly used Swift camel casing

- capitalzied: convert first letter to uppercase
- uppercased: All letters uppercased
- lowercased: All letters lowercased

Support all types that conform to StringProtocol rather than just String
Use a type erased protocl inheritance strategy commonly used to provide default implimentation to avaoid issues with as? checks in generic protocols, while retaining reuse benefits of generic protocols
…y string

In the case where a codable provides an empty string for the codable string value for an instance variable an empty bracket was inserted which is invalid XML.

```
let attr = "bar"
let value = "FOO"
enum CodingKeys : String, CodingKey {
    case attr
    case value = ""
}
```

Will be useful for unkeyed objects that contain only attributes eg;
```xml
<box attr="bar"><>FOO</></box>
<!-- Would now correctly become -->
<box attr="bar">FOO</box>
```
DynamicNodeEncoding allows easily adding the ability to choose if iVars should be attribute or element encoded by inheriting DynamicNodeEncoding and implimenting a single static function in any Codable class or struct.

This is simpler than the current method that requires a global dynamic encoding closure for every XMLEncoder instance. This allows changing the encoding where the data models live, rather than the creator of the XMLEncoder instance needing to have knowledge of all the possible structs and classes that the encoder might encounter at init time.
- refactor element and attribute encoders to closures for easier code reuse
- Added type alias for encoding closures for clarity
Had removed them since I was testing intrinsics with attributes. Needed to wrap cateogy value in an element tag again.

Also appears the hack for decoding nested arrays is no longer required so removed the complex decoding of Category
Previous version of this test techncially passed on Encdode/Decode comparision sinve the structure values were the same, but the encoding make Book structs id an element, so the strings weren't equal.

Modified the simplier single book test to check that the attributes are encoded to XML and match the original string (minus white space formatting)
@JoeMatt
Copy link
Contributor Author

JoeMatt commented Feb 6, 2019

Just rebased from master and fixed failing tests. This PR branch should be complete as far as my work unless feedback or changes are requested.

MaxDesiatov
MaxDesiatov previously approved these changes Feb 9, 2019
@MaxDesiatov MaxDesiatov self-assigned this Feb 9, 2019
@MaxDesiatov MaxDesiatov merged commit cf516de into CoreOffice:master Feb 9, 2019
@JoeMatt
Copy link
Contributor Author

JoeMatt commented Feb 11, 2019

Hey @MaxDesiatov , I found in my testing that I needed the code that did seem redundant as certain tests were failing if I removed the same code as d29ce8f. Maybe this is why you were seeing failing unit tests in #73

MaxDesiatov pushed a commit that referenced this pull request Feb 22, 2019
## Overview

This PR fixes #12: decoding of unkeyed single value elements that contain attributes as reported in that issue.

## Example

```xml
<?xml version="1.0" encoding="UTF-8"?>
<foo id="123">456</foo>
``` 

```swift
private struct Foo: Codable, DynamicNodeEncoding {
    let id: String
    let value: String

    enum CodingKeys: String, CodingKey {
        case id
        case value
        // case value = "" would also work
    }

    static func nodeEncoding(forKey key: CodingKey) -> XMLEncoder.NodeEncoding {
        switch key {
        case CodingKeys.id:
            return .attribute
        default:
            return .element
        }
    }
}
```

Previously this XML example would fail to decode. This PR allows two different methods of decoding discussed in usage.

## Usage

This PR will support decoding the example XML in two cases as long as the prerequisite cases are matched.

### Prerequisites

1. No keyed child elements exist
2. Any keyed child nodes are supported Attribute types and are indicated to decode as such

### Supported cases

1. An instance var with the key `value` of any decodable type.
2. An instance var of any key that has a Decoding key of String value "value" or "".

The decoder will look for the case where an element was keyed with either "value" or "", but not both, and only one of those values (ie; no other keyed elements). It will automatically find the correct value based on the CodingKey supplied.

## Other considerations

The choice to decode as either "value" or "" keys was purely to try to support the inverse to XML version which would only work if an instance var specifically has a `CodingKey` with associated value type `String` that returns an empty string, if PR #70 is commited as-is, which adds XML coding support for unkeyed attributed value elements.

The 'value' variant was added as a simpler means to support decoding a nested unkeyed element without having to provide custom CodingKey enum for a struct. Something needed to be provided since Swift doesn't have empty string iVars `let "" : String`, isn't a valid iVar token for example, so `value` was chosen as a logical default.

## Notes

This PR is an extension of #70 , though it could be recoded to work off of `master`. The last commit in this PR is the only commit specific to this feature, though #70 provides the inverse solution of creating XML from an attributed value wrapping struct.

Coding and decoding unit tests of String and Int values are included.
MaxDesiatov added a commit that referenced this pull request Mar 25, 2019
This adds new `DynamicNodeDecoding` protocol similar to `DynamicNodeEncoding` introduced in #70 

* Add DynamicNodeDecoding protocol
* Remove NodeDecoding from XMLEncoder
* Add NodeDecoding to XMLDecoder
* Fix class name in DynamicNodeDecoding.swift
* Implement DynamicNodeDecoding with tests
* Improve test coverage
* Add more example code to README
* Fix wording in README
* Fix typos, cleanup example code
* Cleanup example code in README
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