Made sure node encoding strategy is considered for values inside unkeyed containers

[regexident]

This fixes a bug that would make XMLEncoder ignore the node-encoding strategy for values contained in unkeyed collections.

Given a set of types like this …

struct Foo: Codable {
  var bar = Bar()
  var unkeyedBars = [Bar()]
  var keyedBars = ["foo": Bar()]
}

struct Bar: Codable {
  let value = "baz"
  
  static func nodeEncoding(forKey codingKey: CodingKey) -> XMLEncoder.NodeEncoding {
    return .attribute
  }
}

… and an encoder configured like this …

let encoder = XMLEncoder()
encoder.outputFormatting = .prettyPrinted
encoder.keyEncodingStrategy = .convertToSnakeCase
encoder.nodeEncodingStrategy = .custom { codableType, encoder in
  guard let barType = codableType as? Bar.Type else {
    return { _ in return .default }
  }
  return barType.nodeEncoding(forKey:)
}

… produces output that looks like this …

<foo>
    <bar value="baz" />
    <unkeyed_bars>
        <value>baz</value>
    </unkeyed_bars>
    <keyed_bars>
        <foo value="baz" />
    </keyed_bars>
</foo>

… while the correct output would look like this …

<foo>
    <unkeyed_bars value="baz" />
    <bar value="baz" />
    <keyed_bars>
        <foo value="baz" />
    </keyed_bars>
</foo>

[codecov]

Codecov Report

Merging #2 into master will increase coverage by 12.75%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           master       #2       +/-   ##
===========================================
+ Coverage   11.82%   24.58%   +12.75%     
===========================================
  Files          12       12               
  Lines        1547     1554        +7     
===========================================
+ Hits          183      382      +199     
+ Misses       1364     1172      -192

Impacted Files	Coverage Δ
Sources/XMLCoder/Encoder/XMLEncoder.swift	22.88% <100%> (+22.88%)	⬆️
Sources/XMLCoder/XMLStackParser.swift	72.47% <0%> (+35.39%)	⬆️
Sources/XMLCoder/Encoder/XMLEncodingStorage.swift	87.5% <0%> (+87.5%)	⬆️

---

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 27e8d96...ca2d711. Read the comment docs.

[MaxDesiatov]

Thank you @regexident, I'm pretty sure that codecov will pass after I merge #1 that gives us correct test coverage. In general I think that the library shouldn't use print for logging. Currently there is only one use of print in the library codebase and I expect even that one to go away once #3 is fixed. Long term it might make sense to implement some general logging settings available to users of XMLCoder.

[MaxDesiatov]

Thanks for the quick update @regexident, would you mind also adding a separate test case with inline XML similar to what is done here? Looks like you already have test XML and Swift code that reproduce this.

This new test should probably be added in a new separate file, as XMLCoderTests file is renamed in #1, which would cause conflicts if new tests were added to that existing test case.

[regexident]

I actually did try adding test cases for this, alas XMLEncoder has non-deterministic node ordering (as doe JSONEncoder for that matter), due to using unordered dictionaries as internal storage, which these days make use of randomly salted seeds.

As such I'm somewhat unsure how to test this properly, other than e.g. doing xml.contains("key=\"value\"") for each expected key on the resulting XML string, which feels wrong to me.

[MaxDesiatov]

Wouldn't it be possible to test this with a single node? I hope the bug is still reproducible in that case. I guess we would always get the same result for a single node even with random ordering. So we only need to test that we don't get this result

<foo>
    <unkeyed_bars>
        <value>baz</value>
    </unkeyed_bars>
</foo>

instead of the correct one

<foo>
    <unkeyed_bars value="baz" />
</foo>

[regexident]

That should work, yes.

For what it's worth I'm currently hacking on a refactor that would enable order-preservation, as XML often times expects elements to appear in a certain order. This should then also make testing much easier.

[MaxDesiatov]

great stuff, thanks again!

[regexident]

That should work, yes.

Narrator: "It did."