RUMM-1000 Add support for additional properties to the models generation pipeline #399

acgh · 2021-02-04T10:44:51Z

What and why?

We ultimately want to add support for custom timings. This was previously introduced to the schema in rum-events-format #25 relying on JSON Schema's additionalProperty.

Our Swift/Objc pipeline that generates models from a schema doesn't know how to handle additionalProperties at this point. To prevent from mis-generating the model for custom timings we previously had to put a dedicated filter until we add the support for additionalProperties.

In this PR we add the plumbing needed for a basic support of additionalProperty. It will allow us to remove the filter and generate the model for custom timings.

How?

As of 8e6fbbf the approach has been simplified. To sum-up, we model additionalProperties as dictionaries while adding a relatively restrictive support of SwiftDictionary (only primitive types).
Any future gaps between an improved use of JSON Schema in the input files and the current limitations of the pipeline should be caught by exceptions at runtime, and prior to shipping the generated code inside the SDK.

JSON

We add an additionalProperties property to the JSONSchema. It is optional and of type JSONSchema itself as required by the spec.

We map that schema to a JSONObject.AdditionalProperties struct which is a subset of a JSONObject.Property. It holds a type that is a JSONPrimitive, meaning that we only support bool, double, integer, string at this point. The mapping happens in the JSONSchemaToJSONTypeTransformer.

Swift

To support additional properties in SwiftStruct, we introduce a basic SwiftDictionary. Note that keys are String and values must be SwiftPrimitiveType, i.e. Bool, Int, Int64, String, Double.
From that point we can declare a SwiftStruct.Property of type SwiftDictionary to model JSONSchema's additionalProperties. The mapping from JSONObject to SwiftStruct is done in JSONToSwiftTypeTransformer.

JSONToSwiftTypeTransformer enforces a couple of new rules:

A provided root object cannot map to a Dictionary, especially a root object cannot have additionalProperties that would require a mapping to a dictionary.
A JSON object cannot hold both regular properties and additionalProperties.

Objc Interop

To mirror additions to the Swift type system and transformer, we introduce a ObjcInteropNSDictionary and add the expected mapping methods inside SwiftToObjcInteropTypeTransformer.

RUM

We just add some basic expect methods to allow traversing a SwiftDictionary and transforming any primitive types, e.g. Int -> Int64.

Printers

This has been simplified a lot compared to the initial approach (41a1ac8 and before), they only need to know how to print dictionaries while converting some core Swift <-> Objc types.

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

... RUMDataModels.swift is out of sync with rum-events-format: 8b955a03d0fe0b2f032a02d6800c61ef3fc9fada
Difference was found when comparing it with template file:
>>>
111,113d110
<         /// User custom timings of the view
<         public let customTimings: [String: Int64]?
< 
172d168
<             case customTimings = "custom_timings"
<<<

…ToSwiftTypeTransformer

…iftType transformer

…s in Objc Interop

…dditionalProperties

…e and use it for additionalProperties

…r after adding support for additionalProperties

…additional properties to a later stage in RUMSwiftTypeTransformer

…dicated PR

…we introduce a model with addional properties

acgh · 2021-02-04T13:28:56Z

...tor/Sources/RUMModelsGeneratorCore/Input/ObjcInterop/SwiftToObjcInteropTypeTransformer.swift

+            )
+            propertyWrapper.objcInteropType = try objcInteropType(for: swiftArray)
+            return propertyWrapper
+        case let swiftDictionary as SwiftDictionary where swiftDictionary.value is SwiftPrimitiveType:


The whole function comes from the block of code previously starting at line 35. But I want to draw attention to this case that handles a SwiftDictionary.

acgh · 2021-02-04T13:32:12Z

tools/rum-models-generator/Sources/RUMModelsGeneratorCore/Output/Printing/SwiftPrinter.swift

@@ -89,11 +129,59 @@ public class SwiftPrinter: BasePrinter {
        writeLine("}")
    }

+    private func printCodableMethods(for properties: [SwiftStruct.Property], additionalProperties: SwiftStruct.Property) throws {


Please see the associated tests for a preview of the output:
https://github.com/DataDog/dd-sdk-ios/pull/399/files#diff-fcc08ef045de4c9d692921649cfe668e0900e3900473a1e5702383cc67c37efa

…onalProperties as simple Swift Dictionaries

ncreated

Looks way simpler and much more straightforward now 🚀. I left some comments that we may consider to simplify it even further - let me know WDYT.

ncreated · 2021-02-08T09:34:43Z

tools/rum-models-generator/Sources/RUMModelsGeneratorCore/Input/JSON/JSONType.swift

        let isRequired: Bool
        let isReadOnly: Bool
    }

    let name: String
    let comment: String?
    let properties: [Property]
+    let additionalProperties: Property?


I think that we're using the Property type here with a wrong purpose. Looking at how "additionalProperties" work, only Property.type, Property.isReadOnly and Property.comment apply to it and we force artificial / uncontrolled values for name, defaultValue and isRequired. Maybe it's worth introducing separate type: JSONObject.AdditionalProperties(comment:type:isReadOnly), WDYT?

I think that it's a good point and good idea!

ncreated · 2021-02-08T09:36:45Z

...tor/Sources/RUMModelsGeneratorCore/Input/ObjcInterop/SwiftToObjcInteropTypeTransformer.swift

+                case let swiftDictionary as SwiftDictionary where swiftDictionary.key is SwiftEnum || swiftDictionary.value is SwiftEnum:
+                    throw Exception.unimplemented(
+                        "Objc Interop for `SwiftDictionary`s with an Enum as key or value is not supported."
+                    )


Maybe it's easier to change SwiftDictionary.value to be SwiftPrimitiveType to have compile-time check for this?

That's a good idea, and similarly SwiftDictionary.key should be a SwiftPrimitiveType for now.

ncreated · 2021-02-08T09:38:42Z

tools/rum-models-generator/Sources/RUMModelsGeneratorCore/Input/Swift/SwiftType.swift

+internal struct SwiftDictionary: SwiftType {
+    var key: SwiftType
+    var value: SwiftType
+}


IMO we can save some control flows / tests if being more tailored on that, i.e.:

var key: SwiftPrimitive<String> var value: SwiftPrimitiveType

Yes you're right!

…ect.AdditionalProperties

ncreated

LVG 🎸

Alexandre Costanza added 14 commits February 2, 2021 14:37

RUMM-1000 Fix typo defaultVaule -> defaultValue

e104a4e

RUMM-1000 Add additionalProperties to JSONSchema and JSONObject

4f1e077

RUMM-1000 Add additionalProperties to SwiftType and read them in JSON…

5cacf26

…ToSwiftTypeTransformer

RUMM-1000 Make SwiftStruct.additionalProperties go through the RUM Sw…

ebd5797

…iftType transformer

RUMM-1000 Make SwiftPrinter print SwiftStruct.additionalProperties

ee0d7df

RUMM-1000 Change remaining mentions of managedSwift to bridgedSwift

bd6c2b1

RUMM-1000 Handle PropertyWrappers for SwiftStruct additionalPropertie…

4b02c71

…s in Objc Interop

RUMM-1000 Generate Codable methods in SwiftPrinter for Structs with a…

865c881

…dditionalProperties

RUMM-1000 Add basic support of Dictionary as SwiftType/ObjcInteropTyp…

d6e7c67

…e and use it for additionalProperties

RUMM-1000 Remove exclusion of customTimings in RUMSwiftTypeTransforme…

7c60dcd

…r after adding support for additionalProperties

RUMM-1000 Move mapping from Property to Dictionary of Properties for …

efe10d9

…additional properties to a later stage in RUMSwiftTypeTransformer

RUMM-1000 Add comments and factor some functions

f3bbb6d

RUMM-1000 Factor some more code

a0e6b81

RUMM-1000 Add back exclusion of customTimings to be addressed in a de…

afbf154

…dicated PR

acgh requested a review from a team as a code owner February 4, 2021 10:44

acgh self-assigned this Feb 4, 2021

Alexandre Costanza added 2 commits February 4, 2021 13:33

RUMM-1000 Lint

ab61310

RUMM-1000 Exclude DynamicCodingKeys by default in SwiftPrinter until …

41a1ac8

…we introduce a model with addional properties

acgh commented Feb 4, 2021

View reviewed changes

Alexandre Costanza added 2 commits February 5, 2021 21:58

RUMM-1000 Backtrack on the approach and model JSONSchemas with additi…

8e6fbbf

…onalProperties as simple Swift Dictionaries

RUMM-1000 Lint

e43c911

ncreated reviewed Feb 8, 2021

View reviewed changes

RUMM-1000 Make SwiftDictionary a String: Value type and add a JSONObj…

8c3221e

…ect.AdditionalProperties

ncreated approved these changes Feb 8, 2021

View reviewed changes

acgh merged commit d61fd7b into master Feb 8, 2021

acgh deleted the acgh/rumm-1000-support-custom-timings-json-schema branch February 8, 2021 14:20

acgh mentioned this pull request Feb 9, 2021

RUMM-1000 Use View.customTimings from actual schema instead of RUMEvent #406

Merged

3 tasks

ncreated mentioned this pull request Jun 9, 2021

RUMM-1401 Update RUM models generator for recent event schemas #516

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RUMM-1000 Add support for additional properties to the models generation pipeline #399

RUMM-1000 Add support for additional properties to the models generation pipeline #399

acgh commented Feb 4, 2021 •

edited

Loading

acgh Feb 4, 2021

acgh Feb 4, 2021

ncreated left a comment •

edited

Loading

ncreated Feb 8, 2021

acgh Feb 8, 2021

ncreated Feb 8, 2021

acgh Feb 8, 2021

ncreated Feb 8, 2021

acgh Feb 8, 2021

ncreated left a comment

RUMM-1000 Add support for additional properties to the models generation pipeline #399

RUMM-1000 Add support for additional properties to the models generation pipeline #399

Conversation

acgh commented Feb 4, 2021 • edited Loading

What and why?

How?

JSON

Swift

Objc Interop

RUM

Printers

Review checklist

Next

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncreated left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncreated left a comment

Choose a reason for hiding this comment

acgh commented Feb 4, 2021 •

edited

Loading

ncreated left a comment •

edited

Loading