Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RUMM-987 Prepare code generation for RUM @objc models #382

Merged
merged 8 commits into from
Jan 26, 2021

Conversation

ncreated
Copy link
Member

@ncreated ncreated commented Jan 25, 2021

What and why?

📦 This PR updates the rum-models-generator tool to generate @objc RUM data models. This is required to further expose scrubbing API in DatadogObjc as it was done for Swift (#367).

How?

I leveraged the existing architecture of rum-models-generator:

  • read → transform → print

by implementing another Printer:

  • read → transform → SwiftPrinter
  • read → transform → ObjcInteropPrinter

The SwiftType schema generated and transformed for Swift code generation is now used by ObjcInteropReader to generate ObjcInteropType schema. The schema is later passed to the ObjcInteropPrinter for printing the actual code.

Swift / Obj-c interoperability for RUM

@objc interoperability is done by wrapping Swift structs in @objc classes and exposing Swift enums as @objc enum .

Simple example can be illustrated as:

// MARK: - Swift

public struct Foo {
    public let immutableString: String
    public let optionalImmutableString: String?
    public var mutableString: String
    public var optionalMutableString: String?
}

// MARK: - ObjcInterop

@objc
public class DDFoo: NSObject {
    internal var swiftModel: Foo
    internal var root: DDFoo { self }

    internal init(swiftModel: Foo) {
        self.swiftModel = swiftModel
    }

    @objc public var immutableString: String {
        root.swiftModel.immutableString
    }

    @objc public var optionalImmutableString: String? {
        root.swiftModel.optionalImmutableString
    }

    @objc public var mutableString: String {
        set { root.swiftModel.mutableString = newValue }
        get { root.swiftModel.mutableString }
    }

    @objc public var optionalMutableString: String? {
        set { root.swiftModel.optionalMutableString = newValue }
        get { root.swiftModel.optionalMutableString }
    }
}

Nested structs are managed by transitive @objc classes:

// MARK: - Swift

public struct Foo {
    public var bar: Bar

    public struct Bar {
        public let property: Int
    }
}

// MARK: - ObjcInterop

@objc
public class DDFoo: NSObject {
    internal var swiftModel: Foo
    internal var root: DDFoo { self }

    internal init(swiftModel: Foo) {
        self.swiftModel = swiftModel
    }

    @objc public var bar: DDFooBar {
        DDFooBar(root: root)
    }
}

@objc
public class DDFooBar: NSObject {
    internal let root: DDFoo

    internal init(root: DDFoo) {
        self.root = root
    }

    @objc public var property: NSNumber {
        root.swiftModel.bar.property as NSNumber
    }
}

Code Reviewing

🙏 Please, carefully follow all the code snippets generated in ObjcInteropPrinterTests. They showcase solutions to all edge-cases and Swift <> Obj-c interoperability limitations that I struggled with. Better alternatives are welcome 🙌.

Especially, I want to draw your attention to two things:

1/ Some Swift value types are incompatible with @objc

Types like Int64, Bool? or Enumeration? cannot be directly exposed. Numeric types are represented as NSNumber. Optional enums required adding extra case none to represent nil.

2/ Building key-paths to optional properties requires ! unwrapping

Some property wrappers in transitive @objc classes require force unwrapping of the Swift struct's key path, e.g.:

// Property wrapper defined in transitive @objc class `DDFooBar`:
@objc public var property: String {
    // Because `bar` is an optional property on `Foo`, its key-path requires force unwrapping
    root.swiftModel.bar!.property
}

The safety of this construct is guaranteed by the != nil check in the property wrapper providing the access to DDFooBar:

// Property wrapper defined in root @objc class `DDFoo`:
@objc public var bar: DDFooBar? {
    root.swiftModel.bar != nil ? DDFooBar(root: root) : nil
}

Coming in next PRs:

Next PRs will focus on:

  • configuring CI job for @objc RUM models validation,
  • exposing scrubbing API in DatadogObjc.

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

@ncreated ncreated requested a review from a team as a code owner January 25, 2021 20:54
@ncreated ncreated self-assigned this Jan 25, 2021
Copy link
Contributor

@buranmert buranmert left a comment

Choose a reason for hiding this comment

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

that should work ✅

by adding missing `import Foundation`
Copy link
Contributor

@acgh acgh left a comment

Choose a reason for hiding this comment

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

As discussed there is some potential confusion between Reader/Transformer to clarify/refactor, but that will fit better in a follow-up PR. Looks great anyway!

@ncreated ncreated merged commit c5dfbe7 into master Jan 26, 2021
@ncreated ncreated deleted the ncreated/RUMM-987-expose-scrubbing-API-for-objc branch January 26, 2021 16:25
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.

3 participants