Decoding fails for keyed structs in a singleValueContainer

[mrackwitz]

This is for now mostly a bug report, adding a failing test reproducing the issue at hand, as I don't have a solution to this without better understanding the expected behavior.

🐛 Problem

Assume I have the following structs:

struct Wrapper: Codable, Equatable {
    let wrapped: Wrapped

    init(wrapped: Wrapped) {
        self.wrapped = wrapped
    }

    init(from decoder: Decoder) throws {
        let container = try decoder.singleValueContainer()
        self.wrapped = try container.decode(Wrapped.self)
    }

    func encode(to encoder: Encoder) throws {
        var container = encoder.singleValueContainer()
        try container.encode(wrapped)
    }
}
struct Wrapped: Codable, Equatable {
    let val: String
}

I presume the values of both structs are expected to encode and decode the same binary format, as the type Wrapper acts a transparent wrapper from an encoding/decoding perspective. That at least is the current behavior, encoding-wise. So while they encode indeed to the same data, the decoding of Wrapper however does always (!) fail.

failed: caught error: "dataCorrupted(Swift.DecodingError.Context(codingPath: [], debugDescription: "Premature end of data", underlyingError: nil))

In a more complex situation, I also saw this failing with:

failed: caught error: "dataCorrupted(Swift.DecodingError.Context(codingPath: [], debugDescription: "Unknown data type 4", underlyingError: nil))

🔍 Solution Explored

To me, the problem seemed to be that DecodingNode.singleValueContainer() turns its storage into an indexed BinaryStreamProvider, which will be eventually turned back into Data. While converting back and forth here seemed innocuous at first glance, I presumed they inadvertently consume the count byte(s) in this case. However when I attempted to fix this myself (by making ValueDecoder operate directly on the Storage instead), I found out that while this does indeed fix the new test, it does break several other tests which seem to rely on exactly this behavior of singleValueContainer. (testDecodingAsDifferentType, testStringEnumEncoding, testStringEncoding, UUIDEncodingTests)

🛟 Help Required

I'm not quite sure on the design of the binary format and whether this is supposed to work as is or the wrapped struct should be rather prepended by a count byte.

[christophhagen]

Thanks for the notification of the issue, and the thorough investigation and test case.

• You're right that the encoding and decoding should work as your test case specifies, and the Wrapper should be transparent as far as encoding goes.
• Normally, the binary format only prepends a count if the data is variable-length and contained in another object (keyed or unkeyed container), otherwise the whole data is used during decoding. I think the additional wrapper prevents this from being detected correctly.
• I think this may be related to a bug fix for unkeyed containers I pushed a while back (28804e7).
  I'll have a closer look
• FYI: The compare() function used in your test actually performs a full encode-decode, I should probably rename that to be more clear.

I'll report back once I had time to look at it more closely.

[mrackwitz]

Thanks for the feedback and confirming some of my assumptions.

I actually had another go at my attempted fix and managed to do in such way that it does resolve the new test without introducing any regressions. I pushed it up to this branch, so it's part of this PR. Please have a look if you see any potential issues with this solution.

FYI: The compare() function used in your test actually performs a full encode-decode, I should probably rename that to be more clear.

Yeah, I did see this as well, but as it was consistently used in all StructEncodingTests, it seemed consistent to stick with it here as well.

[christophhagen]

Thanks for the update! It seems to be working (although the pipeline fails due to an unrelated error), and I'll check the code later to see before merging this.

[mrackwitz]

Just addressed the CI failure as well to get this out of the way.

[christophhagen]

Some minor issues with the test, but I'll merge this and clean up when I'll do some refactoring.

[christophhagen]

Should be changed to compare(wrapped, to: expected).
The function encodes wrapped and compares it to expected. That it works for encodedWrapped as well is just due to the fact that [UInt8] encodes to itself.

[christophhagen]

Would change this to XCTAssertEqual(encodedWrapper, Data(expected))

[mrackwitz]

Thanks for swiftly merging and releasing this! 🙏

Legit feedback on code review. I think there are a few more places in StructEncodingTests which should only rely on a subset of what compare actually does. I did avoid doing any of these refactorings myself to keep this PR small and focused, but happy to follow up with refactorings.