From 47c372054d562b49f97c6ecbac2d8c439813e34b Mon Sep 17 00:00:00 2001 From: Joseph Heck Date: Sat, 29 Jul 2023 16:57:25 -0700 Subject: [PATCH] test that illustrates failure scenario for issue #54 (#55) * test that illustrates failure scenario for issue #54 * establishing a tree of AutomergeEncoderImpl during encoding to be able to later walk and clean up where needed * tracking keys written, highest index written, and objectIds for each container when found * removing extraneous elements from tree, test to verify tree integrity * verifying ObjectId found for each relevant container * enables post-encoding cleanup using details from captured encode-tree --- .../xcshareddata/xcschemes/Automerge.xcscheme | 14 ++++ .../Codable/Encoding/AutomergeEncoder.swift | 2 + .../Encoding/AutomergeEncoderImpl.swift | 65 ++++++++++++++++++- .../AutomergeKeyedEncodingContainer.swift | 45 ++++++++++--- ...utomergeSingleValueEncodingContainer.swift | 1 + .../AutomergeUnkeyedEncodingContainer.swift | 33 +++++++--- .../AutomergeArrayEncodeDecodeTests.swift | 38 +++++++++++ .../AutomergeKeyEncoderImplTests.swift | 55 ++++++++++++++++ 8 files changed, 232 insertions(+), 21 deletions(-) create mode 100644 Tests/AutomergeTests/CodableTests/AutomergeArrayEncodeDecodeTests.swift diff --git a/.swiftpm/xcode/xcshareddata/xcschemes/Automerge.xcscheme b/.swiftpm/xcode/xcshareddata/xcschemes/Automerge.xcscheme index 82a1b900..3c578c5a 100644 --- a/.swiftpm/xcode/xcshareddata/xcschemes/Automerge.xcscheme +++ b/.swiftpm/xcode/xcshareddata/xcschemes/Automerge.xcscheme @@ -34,6 +34,20 @@ ReferencedContainer = "container:"> + + + + (_ value: T, at path: [CodingKey]) throws { @@ -39,5 +40,6 @@ public struct AutomergeEncoder { cautiousWrite: cautiousWrite ) try value.encode(to: encoder) + encoder.postencodeCleanup() } } diff --git a/Sources/Automerge/Codable/Encoding/AutomergeEncoderImpl.swift b/Sources/Automerge/Codable/Encoding/AutomergeEncoderImpl.swift index b17be8f5..00448b7d 100644 --- a/Sources/Automerge/Codable/Encoding/AutomergeEncoderImpl.swift +++ b/Sources/Automerge/Codable/Encoding/AutomergeEncoderImpl.swift @@ -1,3 +1,5 @@ +/// Convenience marker within AutomergeEncoderImpl to indicate the kind of associated container + /// The internal implementation of AutomergeEncoder. /// /// Instances of the class capture one of the various kinds of schema value types - single value, array, or object. @@ -12,6 +14,16 @@ final class AutomergeEncoderImpl { // indicator that the singleValue has written a value var singleValueWritten: Bool = false + // Tracking details of what was written by Codable implementations + // working with the container encode() calls. The details captured + // in these variables allow us to "clean up" anything extraneous + // in the data store that wasn't overwritten by an encode. + var containerType: EncodingContainerType? + var childEncoders: [AutomergeEncoderImpl] = [] + var highestUnkeyedIndexWritten: UInt64? + var mapKeysWritten: [String] = [] + var objectIdForContainer: ObjId? + init( userInfo: [CodingUserInfoKey: Any], codingPath: [CodingKey], @@ -25,12 +37,60 @@ final class AutomergeEncoderImpl { schemaStrategy = strategy self.cautiousWrite = cautiousWrite } + + func postencodeCleanup() { + precondition(objectIdForContainer != nil) + precondition(containerType != nil) + guard let objectIdForContainer, let containerType else { + return + } + switch containerType { + case .Key: + precondition(!mapKeysWritten.isEmpty) + // Remove keys that exist in this objectId that weren't + // written during encode. (clean up 'dead' keys from maps) + let extraAutomergeKeys = document.keys(obj: objectIdForContainer) + .filter { keyValue in + !mapKeysWritten.contains(keyValue) + } + for extraKey in extraAutomergeKeys { + do { + try document.delete(obj: objectIdForContainer, key: extraKey) + } catch { + fatalError("Unable to delete extra key \(extraKey) during post-encode cleanup: \(error)") + } + } + case .Index: + precondition(highestUnkeyedIndexWritten != nil) + guard let highestUnkeyedIndexWritten else { + return + } + // Remove index elements that exist in this objectId beyond + // the max written during encode. (allow arrays to 'shrink') + var highestAutomergeIndex = document.length(obj: objectIdForContainer) - 1 + while highestAutomergeIndex > highestUnkeyedIndexWritten { + do { + try document.delete(obj: objectIdForContainer, index: highestAutomergeIndex) + highestAutomergeIndex -= 1 + } catch { + fatalError( + "Unable to delete index position \(highestAutomergeIndex) during post-encode cleanup: \(error)" + ) + } + } + case .Value: + return + } + // Recursively walk the encoded tree doing "cleanup". + for child in childEncoders { + child.postencodeCleanup() + } + } } // A bit of example code that someone might implement to provide Encodable conformance // for their own type. // -// // extension Coordinate: Encodable { // func encode(to encoder: Encoder) throws { // var container = encoder.container(keyedBy: CodingKeys.self) @@ -60,6 +120,7 @@ extension AutomergeEncoderImpl: Encoder { codingPath: codingPath, doc: document ) + containerType = .Key return KeyedEncodingContainer(container) } @@ -74,6 +135,7 @@ extension AutomergeEncoderImpl: Encoder { preconditionFailure() } + containerType = .Index return AutomergeUnkeyedEncodingContainer( impl: self, codingPath: codingPath, @@ -92,6 +154,7 @@ extension AutomergeEncoderImpl: Encoder { preconditionFailure() } + containerType = .Value return AutomergeSingleValueEncodingContainer( impl: self, codingPath: codingPath, diff --git a/Sources/Automerge/Codable/Encoding/AutomergeKeyedEncodingContainer.swift b/Sources/Automerge/Codable/Encoding/AutomergeKeyedEncodingContainer.swift index a9ca6a6c..0c526323 100644 --- a/Sources/Automerge/Codable/Encoding/AutomergeKeyedEncodingContainer.swift +++ b/Sources/Automerge/Codable/Encoding/AutomergeKeyedEncodingContainer.swift @@ -52,6 +52,7 @@ struct AutomergeKeyedEncodingContainer: KeyedEncodingContainerProt ) { case let .success(objId): objectId = objId + impl.objectIdForContainer = objId lookupError = nil case let .failure(capturedError): objectId = nil @@ -98,6 +99,7 @@ struct AutomergeKeyedEncodingContainer: KeyedEncodingContainerProt throw reportBestError() } try document.put(obj: objectId, key: key.stringValue, value: .Null) + impl.mapKeysWritten.append(key.stringValue) } mutating func encode(_ value: Bool, forKey key: Self.Key) throws { @@ -108,6 +110,7 @@ struct AutomergeKeyedEncodingContainer: KeyedEncodingContainerProt try checkTypeMatch(value: value, objectId: objectId, key: key, type: .bool) } try document.put(obj: objectId, key: key.stringValue, value: .Boolean(value)) + impl.mapKeysWritten.append(key.stringValue) } mutating func encode(_ value: String, forKey key: Self.Key) throws { @@ -118,6 +121,7 @@ struct AutomergeKeyedEncodingContainer: KeyedEncodingContainerProt try checkTypeMatch(value: value, objectId: objectId, key: key, type: .string) } try document.put(obj: objectId, key: key.stringValue, value: .String(value)) + impl.mapKeysWritten.append(key.stringValue) } mutating func encode(_ value: Double, forKey key: Self.Key) throws { @@ -134,6 +138,7 @@ struct AutomergeKeyedEncodingContainer: KeyedEncodingContainerProt try checkTypeMatch(value: value, objectId: objectId, key: key, type: .double) } try document.put(obj: objectId, key: key.stringValue, value: value.toScalarValue()) + impl.mapKeysWritten.append(key.stringValue) } mutating func encode(_ value: Float, forKey key: Self.Key) throws { @@ -150,6 +155,7 @@ struct AutomergeKeyedEncodingContainer: KeyedEncodingContainerProt try checkTypeMatch(value: value, objectId: objectId, key: key, type: .double) } try document.put(obj: objectId, key: key.stringValue, value: value.toScalarValue()) + impl.mapKeysWritten.append(key.stringValue) } mutating func encode(_ value: Int, forKey key: Self.Key) throws { @@ -160,6 +166,7 @@ struct AutomergeKeyedEncodingContainer: KeyedEncodingContainerProt try checkTypeMatch(value: value, objectId: objectId, key: key, type: .int) } try document.put(obj: objectId, key: key.stringValue, value: value.toScalarValue()) + impl.mapKeysWritten.append(key.stringValue) } mutating func encode(_ value: Int8, forKey key: Self.Key) throws { @@ -170,6 +177,7 @@ struct AutomergeKeyedEncodingContainer: KeyedEncodingContainerProt try checkTypeMatch(value: value, objectId: objectId, key: key, type: .int) } try document.put(obj: objectId, key: key.stringValue, value: value.toScalarValue()) + impl.mapKeysWritten.append(key.stringValue) } mutating func encode(_ value: Int16, forKey key: Self.Key) throws { @@ -180,6 +188,7 @@ struct AutomergeKeyedEncodingContainer: KeyedEncodingContainerProt try checkTypeMatch(value: value, objectId: objectId, key: key, type: .int) } try document.put(obj: objectId, key: key.stringValue, value: value.toScalarValue()) + impl.mapKeysWritten.append(key.stringValue) } mutating func encode(_ value: Int32, forKey key: Self.Key) throws { @@ -190,6 +199,7 @@ struct AutomergeKeyedEncodingContainer: KeyedEncodingContainerProt try checkTypeMatch(value: value, objectId: objectId, key: key, type: .int) } try document.put(obj: objectId, key: key.stringValue, value: value.toScalarValue()) + impl.mapKeysWritten.append(key.stringValue) } mutating func encode(_ value: Int64, forKey key: Self.Key) throws { @@ -200,6 +210,7 @@ struct AutomergeKeyedEncodingContainer: KeyedEncodingContainerProt try checkTypeMatch(value: value, objectId: objectId, key: key, type: .int) } try document.put(obj: objectId, key: key.stringValue, value: value.toScalarValue()) + impl.mapKeysWritten.append(key.stringValue) } mutating func encode(_ value: UInt, forKey key: Self.Key) throws { @@ -210,6 +221,7 @@ struct AutomergeKeyedEncodingContainer: KeyedEncodingContainerProt try checkTypeMatch(value: value, objectId: objectId, key: key, type: .uint) } try document.put(obj: objectId, key: key.stringValue, value: value.toScalarValue()) + impl.mapKeysWritten.append(key.stringValue) } mutating func encode(_ value: UInt8, forKey key: Self.Key) throws { @@ -220,6 +232,7 @@ struct AutomergeKeyedEncodingContainer: KeyedEncodingContainerProt try checkTypeMatch(value: value, objectId: objectId, key: key, type: .uint) } try document.put(obj: objectId, key: key.stringValue, value: value.toScalarValue()) + impl.mapKeysWritten.append(key.stringValue) } mutating func encode(_ value: UInt16, forKey key: Self.Key) throws { @@ -230,6 +243,7 @@ struct AutomergeKeyedEncodingContainer: KeyedEncodingContainerProt try checkTypeMatch(value: value, objectId: objectId, key: key, type: .uint) } try document.put(obj: objectId, key: key.stringValue, value: value.toScalarValue()) + impl.mapKeysWritten.append(key.stringValue) } mutating func encode(_ value: UInt32, forKey key: Self.Key) throws { @@ -240,6 +254,7 @@ struct AutomergeKeyedEncodingContainer: KeyedEncodingContainerProt try checkTypeMatch(value: value, objectId: objectId, key: key, type: .uint) } try document.put(obj: objectId, key: key.stringValue, value: value.toScalarValue()) + impl.mapKeysWritten.append(key.stringValue) } mutating func encode(_ value: UInt64, forKey key: Self.Key) throws { @@ -250,9 +265,13 @@ struct AutomergeKeyedEncodingContainer: KeyedEncodingContainerProt try checkTypeMatch(value: value, objectId: objectId, key: key, type: .uint) } try document.put(obj: objectId, key: key.stringValue, value: value.toScalarValue()) + impl.mapKeysWritten.append(key.stringValue) } mutating func encode(_ value: T, forKey key: Self.Key) throws { + guard let objectId = objectId else { + throw reportBestError() + } let newPath = impl.codingPath + [key] // this is where we need to figure out what the encodable type is in order to create // the correct Automerge objectType underneath the covers. @@ -270,17 +289,7 @@ struct AutomergeKeyedEncodingContainer: KeyedEncodingContainerProt // the Codable method `encode` is called - because that's where a container is created. So while we // can set this "newPath", we don't have the deets to create (if needed) a new objectId until we // initialize a specific container type. - guard let objectId = objectId else { - throw reportBestError() - } - let newEncoder = AutomergeEncoderImpl( - userInfo: impl.userInfo, - codingPath: newPath, - doc: document, - strategy: impl.schemaStrategy, - cautiousWrite: impl.cautiousWrite - ) switch T.self { case is Date.Type: // Capture and override the default encodable pathing for Date since @@ -290,6 +299,7 @@ struct AutomergeKeyedEncodingContainer: KeyedEncodingContainerProt try checkTypeMatch(value: value, objectId: objectId, key: key, type: .timestamp) } try document.put(obj: objectId, key: key.stringValue, value: downcastDate.toScalarValue()) + impl.mapKeysWritten.append(key.stringValue) case is Data.Type: // Capture and override the default encodable pathing for Data since // Automerge supports it as a primitive value type. @@ -298,6 +308,7 @@ struct AutomergeKeyedEncodingContainer: KeyedEncodingContainerProt try checkTypeMatch(value: value, objectId: objectId, key: key, type: .bytes) } try document.put(obj: objectId, key: key.stringValue, value: downcastData.toScalarValue()) + impl.mapKeysWritten.append(key.stringValue) case is Counter.Type: // Capture and override the default encodable pathing for Counter since // Automerge supports it as a primitive value type. @@ -306,6 +317,7 @@ struct AutomergeKeyedEncodingContainer: KeyedEncodingContainerProt try checkTypeMatch(value: value, objectId: objectId, key: key, type: .counter) } try document.put(obj: objectId, key: key.stringValue, value: downcastCounter.toScalarValue()) + impl.mapKeysWritten.append(key.stringValue) case is Text.Type: // Capture and override the default encodable pathing for Counter since // Automerge supports it as a primitive value type. @@ -336,8 +348,21 @@ struct AutomergeKeyedEncodingContainer: KeyedEncodingContainerProt try document.spliceText(obj: textNodeId, start: UInt64(offset), delete: 1) } } + impl.mapKeysWritten.append(key.stringValue) default: + let newEncoder = AutomergeEncoderImpl( + userInfo: impl.userInfo, + codingPath: newPath, + doc: document, + strategy: impl.schemaStrategy, + cautiousWrite: impl.cautiousWrite + ) + // Create a link from the current AutomergeEncoderImpl to the child, which + // will be referenced from future containers and updated with status. + impl.childEncoders.append(newEncoder) + try value.encode(to: newEncoder) + impl.mapKeysWritten.append(key.stringValue) } } diff --git a/Sources/Automerge/Codable/Encoding/AutomergeSingleValueEncodingContainer.swift b/Sources/Automerge/Codable/Encoding/AutomergeSingleValueEncodingContainer.swift index 5b21e97d..e2bc0a53 100644 --- a/Sources/Automerge/Codable/Encoding/AutomergeSingleValueEncodingContainer.swift +++ b/Sources/Automerge/Codable/Encoding/AutomergeSingleValueEncodingContainer.swift @@ -26,6 +26,7 @@ struct AutomergeSingleValueEncodingContainer: SingleValueEncodingContainer { case let .success(objId): if let lastCodingKey = codingPath.last { objectId = objId + impl.objectIdForContainer = objId codingkey = AnyCodingKey(lastCodingKey) lookupError = nil } else { diff --git a/Sources/Automerge/Codable/Encoding/AutomergeUnkeyedEncodingContainer.swift b/Sources/Automerge/Codable/Encoding/AutomergeUnkeyedEncodingContainer.swift index f9dc5742..e057a5b1 100644 --- a/Sources/Automerge/Codable/Encoding/AutomergeUnkeyedEncodingContainer.swift +++ b/Sources/Automerge/Codable/Encoding/AutomergeUnkeyedEncodingContainer.swift @@ -18,7 +18,6 @@ struct AutomergeUnkeyedEncodingContainer: UnkeyedEncodingContainer { init(impl: AutomergeEncoderImpl, codingPath: [CodingKey], doc: Document) { self.impl = impl - // array = impl.array! self.codingPath = codingPath document = doc switch doc.retrieveObjectId( @@ -28,14 +27,19 @@ struct AutomergeUnkeyedEncodingContainer: UnkeyedEncodingContainer { ) { case let .success(objId): objectId = objId + impl.objectIdForContainer = objId lookupError = nil case let .failure(capturedError): objectId = nil lookupError = capturedError } + // Fix for issue #54 looks best here, but I'm not happy with it. + // We don't know the length of what's going to be encoded, nor do we get any + // signal when we're done, so the only path I can see is to pro-actively wipe + // out the extent of any array *before* we start writing back into it. if #available(macOS 11, iOS 14, *) { let logger = Logger(subsystem: "Automerge", category: "AutomergeEncoder") - logger.debug("Establishing Unkeyed Encoding Container for path \(codingPath.map { AnyCodingKey($0) })") + logger.debug("Established Unkeyed Encoding Container for path \(codingPath.map { AnyCodingKey($0) })") } } @@ -56,14 +60,6 @@ struct AutomergeUnkeyedEncodingContainer: UnkeyedEncodingContainer { mutating func encodeNil() throws {} mutating func encode(_ value: T) throws where T: Encodable { - let newPath = impl.codingPath + [AnyCodingKey(UInt64(count))] - let newEncoder = AutomergeEncoderImpl( - userInfo: impl.userInfo, - codingPath: newPath, - doc: document, - strategy: impl.schemaStrategy, - cautiousWrite: impl.cautiousWrite - ) guard let objectId = objectId else { throw reportBestError() } @@ -88,6 +84,7 @@ struct AutomergeUnkeyedEncodingContainer: UnkeyedEncodingContainer { ) } try document.insert(obj: objectId, index: UInt64(count), value: valueToWrite) + impl.highestUnkeyedIndexWritten = UInt64(count) case is Data.Type: // Capture and override the default encodable pathing for Data since // Automerge supports it as a primitive value type. @@ -108,6 +105,7 @@ struct AutomergeUnkeyedEncodingContainer: UnkeyedEncodingContainer { } try document.insert(obj: objectId, index: UInt64(count), value: valueToWrite) + impl.highestUnkeyedIndexWritten = UInt64(count) case is Counter.Type: // Capture and override the default encodable pathing for Counter since // Automerge supports it as a primitive value type. @@ -127,6 +125,7 @@ struct AutomergeUnkeyedEncodingContainer: UnkeyedEncodingContainer { ) } try document.insert(obj: objectId, index: UInt64(count), value: valueToWrite) + impl.highestUnkeyedIndexWritten = UInt64(count) case is Text.Type: // Capture and override the default encodable pathing for Counter since // Automerge supports it as a primitive value type. @@ -157,8 +156,22 @@ struct AutomergeUnkeyedEncodingContainer: UnkeyedEncodingContainer { try document.spliceText(obj: textNodeId, start: UInt64(offset), delete: 1) } } + impl.highestUnkeyedIndexWritten = UInt64(count) default: + let newPath = impl.codingPath + [AnyCodingKey(UInt64(count))] + let newEncoder = AutomergeEncoderImpl( + userInfo: impl.userInfo, + codingPath: newPath, + doc: document, + strategy: impl.schemaStrategy, + cautiousWrite: impl.cautiousWrite + ) + // Create a link from the current AutomergeEncoderImpl to the child, which + // will be referenced from future containers and updated with status. + impl.childEncoders.append(newEncoder) + try value.encode(to: newEncoder) + impl.highestUnkeyedIndexWritten = UInt64(count) } count += 1 } diff --git a/Tests/AutomergeTests/CodableTests/AutomergeArrayEncodeDecodeTests.swift b/Tests/AutomergeTests/CodableTests/AutomergeArrayEncodeDecodeTests.swift new file mode 100644 index 00000000..83bb8625 --- /dev/null +++ b/Tests/AutomergeTests/CodableTests/AutomergeArrayEncodeDecodeTests.swift @@ -0,0 +1,38 @@ +import Automerge +import XCTest + +final class AutomergeArrayEncodeDecodeTests: XCTestCase { + var doc: Document! + var setupCache: [String: ObjId] = [:] + + override func setUp() { + setupCache = [:] + doc = Document() + } + + func testArrayShrinkingEncode() throws { + // illustrates https://github.com/automerge/automerge-swift/issues/54 + + struct StructWithArray: Codable, Equatable { + var names: [String] = [] + } + + let encoder = AutomergeEncoder(doc: doc) + let decoder = AutomergeDecoder(doc: doc) + var sample = StructWithArray() + sample.names.append("one") + sample.names.append("two") + + try encoder.encode(sample) + let replica = try decoder.decode(StructWithArray.self) + XCTAssertEqual(replica, sample) + + _ = sample.names.popLast() + try encoder.encode(sample) + let secondReplica = try decoder.decode(StructWithArray.self) + XCTAssertEqual(secondReplica, sample) + // XCTAssertEqual failed: + // ("StructWithArray(names: ["one", "one", "two"])") is not equal to + // ("StructWithArray(names: ["one"])") + } +} diff --git a/Tests/AutomergeTests/CodableTests/AutomergeKeyEncoderImplTests.swift b/Tests/AutomergeTests/CodableTests/AutomergeKeyEncoderImplTests.swift index f5a27caf..b0536818 100644 --- a/Tests/AutomergeTests/CodableTests/AutomergeKeyEncoderImplTests.swift +++ b/Tests/AutomergeTests/CodableTests/AutomergeKeyEncoderImplTests.swift @@ -422,4 +422,59 @@ final class AutomergeKeyEncoderImplTests: XCTestCase { let enc = rootKeyedContainer.superEncoder(forKey: .value) XCTAssertEqual(enc.codingPath.count, 0) } + + func testEncoderImplTreeBuilding() throws { + let impl = AutomergeEncoderImpl( + userInfo: [:], + codingPath: [], + doc: doc, + strategy: .createWhenNeeded, + cautiousWrite: false + ) + // hand off to synthesized or provided logic + let thingToEncode = Samples.layered + try thingToEncode.encode(to: impl) + // And we're back - see what we've got... + + // NOTE: change 'shouldPrint' to true if you want to see the + // detail of the tree. + walk(encoderImpl: impl, indent: 0, shouldPrint: false) + + func walk(encoderImpl: AutomergeEncoderImpl, indent: Int, shouldPrint: Bool) { + let prefix = String(repeating: " ", count: indent) + // let compactCodingPath = encoderImpl.codingPath.stringPath() + + let compactCodingPath = encoderImpl.codingPath + .map { pathElement in + AnyCodingKey(pathElement).description + } + .joined(separator: ".") + + switch encoderImpl.containerType { + case .none: + if shouldPrint { print("\(prefix)*NIL* - \(compactCodingPath)") } + XCTFail("encountered encoderImpl without a defined type") + case .some(.Index): + if shouldPrint { + print( + "\(prefix)INDEX -> \(compactCodingPath) maxIndex: \(String(describing: encoderImpl.highestUnkeyedIndexWritten))" + ) + } + XCTAssertNotNil(encoderImpl.highestUnkeyedIndexWritten) + XCTAssertTrue(encoderImpl.mapKeysWritten.isEmpty) + XCTAssertNotNil(encoderImpl.objectIdForContainer) + case .some(.Key): + if shouldPrint { print("\(prefix)KEY ---> \(compactCodingPath) keys: \(encoderImpl.mapKeysWritten)") } + XCTAssertNil(encoderImpl.highestUnkeyedIndexWritten) + XCTAssertFalse(encoderImpl.mapKeysWritten.isEmpty) + XCTAssertNotNil(encoderImpl.objectIdForContainer) + case .some(.Value): + if shouldPrint { print("\(prefix)VALUE -> \(compactCodingPath)") } + XCTAssertNotNil(encoderImpl.objectIdForContainer) + } + for child in encoderImpl.childEncoders { + walk(encoderImpl: child, indent: indent + 1, shouldPrint: shouldPrint) + } + } + } }