Skip to content

Conversation

@ktoso
Copy link
Member

@ktoso ktoso commented Oct 14, 2020

Motivation:

  • remove not needed workaround

Modifications:

  • remove the wrapper type which was needed on 2.5.4

Result:

  • less hacks

Commit 2 e86b29e

Motivation:

Modifications:

  • fix how we handle known "blobs" in our blob coding

Result:

@ktoso ktoso added this to the 0.6.2 milestone Oct 14, 2020
@ktoso ktoso changed the title =serialization Remove workaround, 5.2.4 did land so we dont need it anymore =serialization rm 5.2.4 workaround, fix LWWMap with Codable Value (Codable in Proto nesting) Oct 14, 2020
// return try context.serialization.deserialize(as: T.self, from: .data(data), using: manifest)
// }
//
// }
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not needed, cleanup

// ==== ----------------------------------------------------------------------------------------------------------------
// MARK: Top-Level Bytes-Blob Encoder

// TODO: TopLevelDataEncoder
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this actually kind of implements just that I guess :nod:

func unkeyedContainer() -> UnkeyedEncodingContainer {
fatalErrorBacktrace("Attempted \(#function) in \(self)")
// TopLevelProtobufBlobEncoderContainer(superEncoder: self)
// TopLevelProtobufBlobUnkeyedEncodingContainer(superEncoder: self)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

though we don't actually need it, good

try data.encode(to: self.superEncoder)
try self.superEncoder.store(data: data)
case let bytes as [UInt8]:
try self.superEncoder.store(bytes: bytes)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so we're not really "encoding" we're just using it and that's great 👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the fix™

settings.serialization.register(CRDT.LWWRegister<String?>.self)
settings.serialization.register(CodableExampleValue.self)
settings.serialization.register(CRDT.LWWMap<String, CodableExampleValue>.self)
settings.serialization.register(CRDT.LWWRegister<CodableExampleValue>.self)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a bit annoying - but good reminder, to use a LWWMap one has to use a LWWRegister heh :~

let gossipTwoExpectMap: [String: CodableExampleValue] = ["a": "foo", "aa": "baz", "b": "bar"]
try self.expectMap(probe: p1, expected: gossipTwoExpectMap)
try self.expectMap(probe: p2, expected: gossipTwoExpectMap)
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this would crash hard.

try data.encode(to: self.superEncoder)
try self.superEncoder.store(data: data)
case let bytes as [UInt8]:
try self.superEncoder.store(bytes: bytes)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the fix™

@ktoso
Copy link
Member Author

ktoso commented Oct 14, 2020

I honestly don't understand the 5.2 failure :|
Things work on docker in linux + 5.2.5 for me which CI should be... debugging :\

@ktoso
Copy link
Member Author

ktoso commented Oct 14, 2020

CI is on 5.2.1... 😞 Well, that explains it... this will pass once CI uses 5.2.5 heh -- I could remove the first hack removal commit but we really want to CI on 5.2.5.

@ktoso
Copy link
Member Author

ktoso commented Oct 14, 2020

@swift-server-bot test this please

@ktoso ktoso merged commit cf466ac into apple:main Oct 15, 2020
@ktoso ktoso deleted the wip-crdt-serialization branch October 15, 2020 01:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Nesting a Codable (JSON) value into an LWWMap (Proto) crashes

2 participants