CoreOffice / XMLCoder

Easy XML parsing using Codable protocols in Swift
https://coreoffice.github.io/XMLCoder/
MIT License
801 stars 112 forks source link

Dynamic node encoding + new formatters + various fixes #70

Closed JoeMatt closed 5 years ago

JoeMatt commented 5 years ago

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

bce416aa2d295f2f5e0d7ac53cc1aa11d0675162

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: 05e3c2dd60048a6f5a86408fd7a266a1f32b3e87

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:

Additional default key encodings

5dbe1d7482f30c928ef0e6eb6d4ea3f8506a0df9

Purpose

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

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

SharedBoxProtocol: Generalize for any Box inheritance

9ae94dc91b5326ee51049421867f22dc01abc6c3

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

551d4843c5eb58a83f8fbfe274961bcb0249a0d4

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

bd24a08b17d8305a0b4a617adc9f93c3a9ad37e9

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

5eedcea50d4a952177631bd7f92f6515dba1525a

Purpose

Swift lint was spitting out a lot of warnings presently.

Resolved:

Unresolved:

MaxDesiatov commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

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[bot] commented 5 years ago

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%) :arrow_up:
Sources/XMLCoder/Auxiliaries/Box/BoolBox.swift 100% <100%> (ø) :arrow_up:
Sources/XMLCoder/Auxiliaries/Box/SharedBox.swift 100% <100%> (ø) :arrow_up:
...urces/XMLCoder/Auxiliaries/String+Extensions.swift 100% <100%> (ø) :arrow_up:
Sources/XMLCoder/Auxiliaries/Box/Box.swift 100% <100%> (ø)
...rImplementation+SingleValueEncodingContainer.swift 100% <100%> (ø)
Sources/XMLCoder/Auxiliaries/Box/KeyedBox.swift 100% <100%> (ø) :arrow_up:
Sources/XMLCoder/Encoder/XMLEncoder.swift 79.2% <45.45%> (-2.95%) :arrow_down:
Sources/XMLCoder/Encoder/DynamicNodeEncoding.swift 50% <50%> (ø)
...s/XMLCoder/Encoder/XMLKeyedEncodingContainer.swift 40% <50%> (+3.63%) :arrow_up:
... 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.

JoeMatt commented 5 years ago

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.

JoeMatt commented 5 years ago

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