From 95ec8c2ccc44c473c1a09f82cf2102149be8eb26 Mon Sep 17 00:00:00 2001 From: stackotter Date: Tue, 3 Dec 2024 11:02:58 +1000 Subject: [PATCH 1/2] Only propagate not-decoded keys from decoders when they're successful --- .../KeyedDecodingContainerProtocol.swift | 15 +++++- .../SingleValueDecodingContainer.swift | 17 +++++-- .../Decoder/UnkeyedDecodingContainer.swift | 16 ++++-- Tests/TOMLKitTests/TOMLKitTests.swift | 49 +++++++++++++++---- 4 files changed, 80 insertions(+), 17 deletions(-) diff --git a/Sources/TOMLKit/Decoder/KeyedDecodingContainerProtocol.swift b/Sources/TOMLKit/Decoder/KeyedDecodingContainerProtocol.swift index 16671385..d34e7a84 100644 --- a/Sources/TOMLKit/Decoder/KeyedDecodingContainerProtocol.swift +++ b/Sources/TOMLKit/Decoder/KeyedDecodingContainerProtocol.swift @@ -228,13 +228,24 @@ extension InternalTOMLDecoder.KDC { codingPath: self.codingPath + key, dataDecoder: self.dataDecoder, strictDecoding: self.strictDecoding, - notDecodedKeys: self.notDecodedKeys + notDecodedKeys: InternalTOMLDecoder.NotDecodedKeys() ) self.decodedKeys.append(key.stringValue) - return try T(from: decoder) + let decodedValue = try T(from: decoder) + + // Only propagate not-decoded keys if the decoding was successful. + // Otherwise `Decodable` implementations that attempt multiple + // decoding strategies in succession (trying the next if the + // previous one failed), don't work. + for (key, path) in decoder.notDecodedKeys.keys { + self.notDecodedKeys.keys[key] = path + } + + return decodedValue } + } } diff --git a/Sources/TOMLKit/Decoder/SingleValueDecodingContainer.swift b/Sources/TOMLKit/Decoder/SingleValueDecodingContainer.swift index 561b86b1..a5505a3d 100644 --- a/Sources/TOMLKit/Decoder/SingleValueDecodingContainer.swift +++ b/Sources/TOMLKit/Decoder/SingleValueDecodingContainer.swift @@ -213,12 +213,13 @@ extension InternalTOMLDecoder.SVDC { codingPath: self.codingPath, dataDecoder: self.dataDecoder, strictDecoding: self.strictDecoding, - notDecodedKeys: self.notDecodedKeys + notDecodedKeys: InternalTOMLDecoder.NotDecodedKeys() ) + let decodedValue: T if type is Data.Type { if let data = self.dataDecoder(self.tomlValue) { - return data as! T + decodedValue = data as! T } else { throw DecodingError.dataCorrupted(DecodingError.Context( codingPath: self.codingPath, @@ -226,7 +227,17 @@ extension InternalTOMLDecoder.SVDC { )) } } else { - return try T(from: decoder) + decodedValue = try T(from: decoder) } + + // Only propagate not-decoded keys if the decoding was successful. + // Otherwise `Decodable` implementations that attempt multiple + // decoding strategies in succession (trying the next if the + // previous one failed), don't work. + for (key, path) in decoder.notDecodedKeys.keys { + self.notDecodedKeys.keys[key] = path + } + + return decodedValue } } diff --git a/Sources/TOMLKit/Decoder/UnkeyedDecodingContainer.swift b/Sources/TOMLKit/Decoder/UnkeyedDecodingContainer.swift index 85776e6e..14e29374 100644 --- a/Sources/TOMLKit/Decoder/UnkeyedDecodingContainer.swift +++ b/Sources/TOMLKit/Decoder/UnkeyedDecodingContainer.swift @@ -166,11 +166,21 @@ extension InternalTOMLDecoder.UDC { codingPath: self.codingPath + TOMLCodingKey(index: self.currentIndex), dataDecoder: self.dataDecoder, strictDecoding: self.strictDecoding, - notDecodedKeys: self.notDecodedKeys + notDecodedKeys: InternalTOMLDecoder.NotDecodedKeys() ) - let decodable = try T(from: decoder) + + let decodedValue = try T(from: decoder) self.currentIndex += 1 - return decodable + + // Only propagate not-decoded keys if the decoding was successful. + // Otherwise `Decodable` implementations that attempt multiple + // decoding strategies in succession (trying the next if the + // previous one failed), don't work. + for (key, path) in decoder.notDecodedKeys.keys { + self.notDecodedKeys.keys[key] = path + } + + return decodedValue } } diff --git a/Tests/TOMLKitTests/TOMLKitTests.swift b/Tests/TOMLKitTests/TOMLKitTests.swift index 82f6b35d..95d52b87 100644 --- a/Tests/TOMLKitTests/TOMLKitTests.swift +++ b/Tests/TOMLKitTests/TOMLKitTests.swift @@ -19,15 +19,6 @@ enum CodableEnum: String, Codable, Equatable { case abc case def case ghi - - public static func == (lhs: Self, rhs: Self) -> Bool { - switch (lhs, rhs) { - case (.abc, .abc): return true - case (.def, .def): return true - case (.ghi, .ghi): return true - default: return false - } - } } struct CodableStruct: Codable, Equatable { @@ -501,4 +492,44 @@ final class TOMLKitTests: XCTestCase { let _ = try udc.nestedUnkeyedContainer() XCTAssertEqual(udc.currentIndex, 1) } + + // Tests for a bug where not-decoded keys would carry over from unsuccessful + // decoding attempts, breaking the common 'try one thing then retry another' + // decoding pattern. This bug was specific to decoders with + // `strictDecoding: true`. + func testRetry() throws { + struct SimpleCodableStruct: Codable, Equatable { + var value: Int + } + + enum SimpleOrComplex: Decodable, Equatable { + case simple(SimpleCodableStruct) + case complex(CodableStruct) + + init(from decoder: Decoder) throws { + if let container = try? decoder.singleValueContainer() { + if let value = try? container.decode(CodableStruct.self) { + self = .complex(value) + return + } else if let value = try? container.decode(SimpleCodableStruct.self) { + self = .simple(value) + return + } + } + throw DecodingError.dataCorrupted(.init( + codingPath: [], + debugDescription: "Expected CodableStruct or SimpleCodableStruct" + )) + } + } + + let toml = "value = 2" + + // Before the bug got fixed, this line would throw an unused key error. + let value = try TOMLDecoder(strictDecoding: true).decode(SimpleOrComplex.self, from: toml) + + // We don't actually expect this to fail, it's not the point of the test, but + // might as well assert it just in case. + XCTAssertEqual(value, .simple(SimpleCodableStruct(value: 2))) + } } From 3a7f8f91a12c3e60a2f3c3a105d0ef9c9bb7098e Mon Sep 17 00:00:00 2001 From: stackotter Date: Tue, 3 Dec 2024 11:38:46 +1000 Subject: [PATCH 2/2] Fix a different aspect of the not-decoded keys propagation bug When creating multiple containers and only using the last, you could end up propagating not-decoded keys from the original container anyway. I think it's ok to assume that only the last container was used to decode the value? --- .../TOMLKit/Decoder/InternalTOMLDecoder.swift | 17 +++++++- Tests/TOMLKitTests/TOMLKitTests.swift | 41 ++++++++++++++++++- 2 files changed, 56 insertions(+), 2 deletions(-) diff --git a/Sources/TOMLKit/Decoder/InternalTOMLDecoder.swift b/Sources/TOMLKit/Decoder/InternalTOMLDecoder.swift index bcd897e3..aa8e307d 100644 --- a/Sources/TOMLKit/Decoder/InternalTOMLDecoder.swift +++ b/Sources/TOMLKit/Decoder/InternalTOMLDecoder.swift @@ -12,6 +12,7 @@ final class InternalTOMLDecoder: Decoder { var dataDecoder: (TOMLValueConvertible) -> Data? var strictDecoding: Bool = false var notDecodedKeys: NotDecodedKeys + let originalNotDecodedKeys: [String: [CodingKey]] let tomlValue: TOMLValue init( @@ -28,6 +29,7 @@ final class InternalTOMLDecoder: Decoder { self.dataDecoder = dataDecoder self.strictDecoding = strictDecoding self.notDecodedKeys = notDecodedKeys + self.originalNotDecodedKeys = notDecodedKeys.keys } func container(keyedBy type: Key.Type) throws -> KeyedDecodingContainer where Key: CodingKey { @@ -42,6 +44,10 @@ final class InternalTOMLDecoder: Decoder { ) } + // Assume that any previous container creation was related to a failed decoding + // attempt, and reset the not-decoded keys back to the value that the parent + // decoder passed to us. + self.notDecodedKeys.keys = self.originalNotDecodedKeys return KeyedDecodingContainer( KDC( table: table, @@ -64,6 +70,11 @@ final class InternalTOMLDecoder: Decoder { ) ) } + + // Assume that any previous container creation was related to a failed decoding + // attempt, and reset the not-decoded keys back to the value that the parent + // decoder passed to us. + self.notDecodedKeys.keys = self.originalNotDecodedKeys return UDC( array, codingPath: self.codingPath, @@ -75,7 +86,11 @@ final class InternalTOMLDecoder: Decoder { } func singleValueContainer() throws -> SingleValueDecodingContainer { - SVDC( + // Assume that any previous container creation was related to a failed decoding + // attempt, and reset the not-decoded keys back to the value that the parent + // decoder passed to us. + self.notDecodedKeys.keys = self.originalNotDecodedKeys + return SVDC( self.tomlValue, codingPath: self.codingPath, dataDecoder: self.dataDecoder, diff --git a/Tests/TOMLKitTests/TOMLKitTests.swift b/Tests/TOMLKitTests/TOMLKitTests.swift index 95d52b87..d06e5b39 100644 --- a/Tests/TOMLKitTests/TOMLKitTests.swift +++ b/Tests/TOMLKitTests/TOMLKitTests.swift @@ -497,7 +497,46 @@ final class TOMLKitTests: XCTestCase { // decoding attempts, breaking the common 'try one thing then retry another' // decoding pattern. This bug was specific to decoders with // `strictDecoding: true`. - func testRetry() throws { + func testRetryKeyedOrDictionary() throws { + enum StructOrDictionary: Decodable, Equatable { + case `struct`(value: Int) + case dictionary([String: Int]) + + enum CodingKeys: String, CodingKey { + case value + } + + init(from decoder: Decoder) throws { + if let container = try? decoder.container(keyedBy: CodingKeys.self) { + if let value = try? container.decode(Int.self, forKey: .value) { + self = .struct(value: value) + } + } + + if let container = try? decoder.singleValueContainer() { + self = try .dictionary(container.decode([String: Int].self)) + } else { + throw DecodingError.dataCorrupted(.init( + codingPath: [], + debugDescription: "Expected int or keyedInt" + )) + } + } + } + + let toml = "other_value = 1" + + // Before the bug got fixed, this line would throw an unused key error. + let value = try TOMLDecoder(strictDecoding: true).decode(StructOrDictionary.self, from: toml) + + // We don't actually expect this to fail, it's not the point of the test, but + // might as well assert it just in case. + XCTAssertEqual(value, .dictionary(["other_value": 1])) + } + + // This is related to the test above, but tests for a different aspect of the + // bug (I originally fixed one but not the other so they are kind of independent). + func testRetryKeyedOrInt() throws { struct SimpleCodableStruct: Codable, Equatable { var value: Int }