Replace `CodableValue` with `AnyCodable` #1081

maxep · 2022-12-08T14:24:11Z

What and why?

Share AnyCodable logic in DatadogInternal.

While migrating the WebView tracking to v2, I realise that we were doing such heavy transformation:

let jsonData = try JSONSerialization.data(withJSONObject: event, options: [])
let encodableEvent = try JSONDecoder().decode(AnyCodable.self, from: jsonData)
writer.write(value: encodableEvent)

The Flight-School/AnyCodable, which was already inspiring our DatadogObjc.AnyEncodable, is capable of serialising Any and so we could use that implementation to write [String: Any] directly.

Moreover, CodableValue is a similar type-erasure than Flight-School/AnyCodable that can be use to decode JSON into [String: Encodable].

For those 2 reasons, I would like to consider the AnyCodable as a solution for [String: Any] serialisation and deserialisation.

How?

Replace CodableValue and DatadogObjc.AnyEncodable with implementation from Flight-School/AnyCodable.

Extensions for ExpressibleBy*Literal and Hashable have been stripped out, but the Equatable extensions remains as it can be useful for data comparison on the message bus.

Model Generator

The generator now uses the shared AnyCodable and DynamicCodingKey.

Review checklist

Feature or bugfix MUST have appropriate tests (unit, integration)
Make sure each commit and the PR mention the Issue number or JIRA reference
Add CHANGELOG entry for user facing changes

Custom CI job configuration (optional)

Run unit tests
Run integration tests
Run smoke tests

datadog-datadog-prod-us1 · 2022-12-09T10:38:46Z

Datadog Report

Branch report: maxep/RUMM-2745/any-codable-erasure
Commit report: 362fa40

✅ dd-sdk-ios 0 Failed, 0 New Flaky, 58 Passed, 0 Skipped, 6.85s Wall Time

ncreated

@maxep could you provide more context on "Why?" I get the idea, but don't fully understand why we need this new vs existing code.

maxep · 2022-12-12T12:56:16Z

@ncreated Indeed, it was missing! I've updated the description.
I'm currently testing this code in the web-view tracking migration to v2, it's ready for review but not yet validated on my side. I might make further modification, or even close it if that doesn't fir our need.

maxep · 2022-12-13T09:52:25Z

Sources/Datadog/DatadogInternal/Codable/AnyEncodable.swift

+        // The list of NSNumber encoding value can be found in the following links:
+        // - https://developer.apple.com/documentation/foundation/nsnumber
+        // - https://github.com/gnustep/libs-base/blob/master/Source/NSNumber.m
+        switch Character(Unicode.Scalar(UInt8(nsnumber.objCType.pointee))) {


This works best than CFNumberGetType(nsnumber) when using Swift value:

let nsnumber = UInt64.max as NSNumber CFNumberGetType(nsnumber) == .sInt64Type // should be .longLongType Character(Unicode.Scalar(UInt8(nsnumber.objCType.pointee))) == "Q" // correct

And this relates to your other mention @maxep, right?

The reason being that the webview bridge returns a JSON as objc dictionary with NSNumber, in Swift a NSNumber with value 1 or 0 can be cast to Bool which would be encoded as true or false and lead to a type mismatch

If so, could we annotate this nsnumber.objCType.pointee code with relevant comment? It seems more important than it looks.

Yes, so an unsigned int does not get encoded as signed! I can improve the comment 👍

Sources/Datadog/DatadogInternal/Codable/AnyEncodable.swift

maxep · 2022-12-13T11:18:26Z

This is ready for review, it works well for our different use-cases 👍 We can now encode dictionary coming from WebView tracking directly. The models have been updated to use the interface shared in DatadogInternal.

Some changes in AnyEncodable were necessary to be usable in both Swift and Objc: In both cases, a value is first cast to a NSNumber so it can infer the right type using the objCType. The reason being that the webview bridge returns a JSON as objc dictionary with NSNumber, in Swift a NSNumber with value 1 or 0 can be cast to Bool which would be encoded as true or false and lead to a type mismatch:

let value: Any = NSNumber(value: 1)
switch value {
case let bool as Bool:
    encode(bool) // will encode 'true'
case let nsnumber as NSNumber:
    encode(nsnumber) // won't reach :(
default:
    break
}

Co-Authored-By: Maciek Grzybowski <maciek.grzybowski@datadoghq.com>

ncreated

All looks good with only minor suggestions. Yes, it makes total sense to have more versatile support for any en-, de-, and codable 👍. Well done 💪.

Sources/Datadog/DatadogInternal/Codable/AnyEncodable.swift

Sources/Datadog/FeaturesIntegration/WebView/WebRUMEventConsumer.swift

ncreated · 2022-12-15T12:38:08Z

tools/rum-models-generator/Tests/CodeGenerationTests/Print/ObjcInteropPrinterTests.swift

-            internal let swiftModel: Foo.Bar
+            internal var swiftModel: Foo.Bar


Looks unrelated - does it mean rum-models-generator tests were red 😥?

Yes.. It's not being tested in CI, which is ok imo, or maybe we could have a conditional workflow!

We have the run_tools_tests lane which looks perfect candidate for it and can be easily conditioned. How about creating separate task to tackle it?

@ncreated I'm adding this to CI in this PR: 362fa40

Sources/Datadog/DatadogInternal/Codable/AnyCodable.swift

ncreated · 2022-12-15T13:10:36Z

Sources/Datadog/DatadogInternal/Codable/AnyEncodable.swift

+        // The list of NSNumber encoding value can be found in the following links:
+        // - https://developer.apple.com/documentation/foundation/nsnumber
+        // - https://github.com/gnustep/libs-base/blob/master/Source/NSNumber.m
+        switch Character(Unicode.Scalar(UInt8(nsnumber.objCType.pointee))) {


And this relates to your other mention @maxep, right?

The reason being that the webview bridge returns a JSON as objc dictionary with NSNumber, in Swift a NSNumber with value 1 or 0 can be cast to Bool which would be encoded as true or false and lead to a type mismatch

If so, could we annotate this nsnumber.objCType.pointee code with relevant comment? It seems more important than it looks.

datadog-datadog-prod-us1 · 2022-12-16T10:46:04Z

Datadog Report

Branch report: maxep/RUMM-2745/any-codable-erasure
Commit report: 362fa40

✅ dd-sdk-ios 0 Failed, 0 New Flaky, 58 Passed, 0 Skipped, 6.85s Wall Time

maxep self-assigned this Dec 8, 2022

maxep force-pushed the maxep/RUMM-2745/crash-reporting-context branch 2 times, most recently from 153e0e8 to ee76468 Compare December 9, 2022 09:52

maxep force-pushed the maxep/RUMM-2745/any-codable-erasure branch 2 times, most recently from 01d7afc to 59bd2c9 Compare December 9, 2022 10:00

Base automatically changed from maxep/RUMM-2745/crash-reporting-context to develop December 9, 2022 13:18

ncreated reviewed Dec 12, 2022

View reviewed changes

maxep commented Dec 13, 2022

View reviewed changes

Sources/Datadog/DatadogInternal/Codable/AnyEncodable.swift Outdated Show resolved Hide resolved

maxep marked this pull request as ready for review December 13, 2022 10:16

maxep requested a review from a team as a code owner December 13, 2022 10:16

maxep force-pushed the maxep/RUMM-2745/any-codable-erasure branch 2 times, most recently from 907fda8 to 618ec72 Compare December 13, 2022 14:05

Replace CodableValue with AnyCodable

cf8c6ae

Co-Authored-By: Maciek Grzybowski <maciek.grzybowski@datadoghq.com>

maxep force-pushed the maxep/RUMM-2745/any-codable-erasure branch from 618ec72 to cf8c6ae Compare December 15, 2022 09:54

ncreated approved these changes Dec 15, 2022

View reviewed changes

maxep added 2 commits December 15, 2022 17:24

Update LICENSE-3rdparty.csv

3e6bdc4

Update AnyEncodable.swift

d81bc1b

maxep force-pushed the maxep/RUMM-2745/any-codable-erasure branch from a561d21 to d81bc1b Compare December 15, 2022 17:23

Run unit tests for rum-models-generator

362fa40

maxep merged commit 8a7c25a into develop Dec 16, 2022

maxep deleted the maxep/RUMM-2745/any-codable-erasure branch December 16, 2022 11:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace `CodableValue` with `AnyCodable` #1081

Replace `CodableValue` with `AnyCodable` #1081

maxep commented Dec 8, 2022 •

edited

Loading

datadog-datadog-prod-us1 bot commented Dec 9, 2022 •

edited

Loading

ncreated left a comment

maxep commented Dec 12, 2022

maxep Dec 13, 2022

ncreated Dec 15, 2022

maxep Dec 15, 2022

maxep commented Dec 13, 2022

ncreated left a comment

ncreated Dec 15, 2022

maxep Dec 15, 2022

ncreated Dec 15, 2022 •

edited

Loading

maxep Dec 16, 2022

ncreated Dec 15, 2022

datadog-datadog-prod-us1 bot commented Dec 16, 2022

		internal let swiftModel: Foo.Bar
		internal var swiftModel: Foo.Bar

Replace CodableValue with AnyCodable #1081

Replace CodableValue with AnyCodable #1081

Conversation

maxep commented Dec 8, 2022 • edited Loading

What and why?

How?

Model Generator

Review checklist

Custom CI job configuration (optional)

datadog-datadog-prod-us1 bot commented Dec 9, 2022 • edited Loading

Datadog Report

ncreated left a comment

Choose a reason for hiding this comment

maxep commented Dec 12, 2022

maxep Dec 13, 2022

Choose a reason for hiding this comment

ncreated Dec 15, 2022

Choose a reason for hiding this comment

maxep Dec 15, 2022

Choose a reason for hiding this comment

maxep commented Dec 13, 2022

ncreated left a comment

Choose a reason for hiding this comment

ncreated Dec 15, 2022

Choose a reason for hiding this comment

maxep Dec 15, 2022

Choose a reason for hiding this comment

ncreated Dec 15, 2022 • edited Loading

Choose a reason for hiding this comment

maxep Dec 16, 2022

Choose a reason for hiding this comment

ncreated Dec 15, 2022

Choose a reason for hiding this comment

datadog-datadog-prod-us1 bot commented Dec 16, 2022

Datadog Report

Replace `CodableValue` with `AnyCodable` #1081

Replace `CodableValue` with `AnyCodable` #1081

maxep commented Dec 8, 2022 •

edited

Loading

datadog-datadog-prod-us1 bot commented Dec 9, 2022 •

edited

Loading

ncreated Dec 15, 2022 •

edited

Loading