refactor: context container in event payload (#130)

* refactor: context container in payload * refactor: Throw with context field in message * test: User-facing error test * refactor: warrning level Co-authored-by: Nicklas Lundin <nicklasl@spotify.com> --------- Co-authored-by: Nicklas Lundin <nicklasl@spotify.com>
spotify · May 29, 2024 · a9d41fa · a9d41fa
1 parent accaaa3
commit a9d41fa
Show file tree

Hide file tree

Showing 8 changed files with 73 additions and 30 deletions.
diff --git a/Sources/Confidence/Confidence.swift b/Sources/Confidence/Confidence.swift
@@ -1,5 +1,6 @@
 import Foundation
 import Combine
+import os
 
 public class Confidence: ConfidenceEventSender {
     public let clientSecret: String
@@ -113,8 +114,8 @@ public class Confidence: ConfidenceEventSender {
             .eraseToAnyPublisher()
     }
 
-    public func track(eventName: String, message: ConfidenceStruct) {
-        eventSenderEngine.emit(
+    public func track(eventName: String, message: ConfidenceStruct) throws {
+        try eventSenderEngine.emit(
             eventName: eventName,
             message: message,
             context: getContext()
@@ -128,9 +129,14 @@ public class Confidence: ConfidenceEventSender {
                     guard let self = self else {
                         return
                     }
-                    self.track(eventName: event.name, message: event.message)
-                    if event.shouldFlush {
-                        eventSenderEngine.flush()
+                    do {
+                        try self.track(eventName: event.name, message: event.message)
+                        if event.shouldFlush {
+                            eventSenderEngine.flush()
+                        }
+                    } catch {
+                        Logger(subsystem: "com.confidence", category: "track").warning(
+                            "Error from EventProducer, failed to track event: \(event.name)")
                     }
                 }
                 .store(in: &cancellables)

diff --git a/Sources/Confidence/ConfidenceError.swift b/Sources/Confidence/ConfidenceError.swift
@@ -26,6 +26,7 @@ public enum ConfidenceError: Error, Equatable {
     case internalError(message: String)
     case parseError(message: String)
     case invalidContextError
+    case invalidContextInMessage
 }
 
 extension ConfidenceError: CustomStringConvertible {
@@ -62,6 +63,8 @@ extension ConfidenceError: CustomStringConvertible {
             return "Flag not found for key \(key)"
         case .invalidContextError:
             return "Invalid context error"
+        case .invalidContextInMessage:
+            return "Field 'context' is not allowed in event's data"
         }
     }
 }
diff --git a/Sources/Confidence/ConfidenceEventSender.swift b/Sources/Confidence/ConfidenceEventSender.swift
@@ -8,11 +8,11 @@ public protocol ConfidenceEventSender: Contextual {
     Upon return, the event has been correctly stored and will be emitted to the backend
     according to the configured flushing logic
     */
-    func track(eventName: String, message: ConfidenceStruct)
+    func track(eventName: String, message: ConfidenceStruct) throws
     /**
     The ConfidenceProducer can be used to push context changes or event tracking
     */
-    func track(producer: ConfidenceProducer)
+    func track(producer: ConfidenceProducer) throws
 
     /**
     Schedule a manual flush of the event data currently stored on disk

diff --git a/Sources/Confidence/EventSenderEngine.swift b/Sources/Confidence/EventSenderEngine.swift
@@ -8,7 +8,7 @@ protocol FlushPolicy {
 }
 
 protocol EventSenderEngine {
-    func emit(eventName: String, message: ConfidenceStruct, context: ConfidenceStruct)
+    func emit(eventName: String, message: ConfidenceStruct, context: ConfidenceStruct) throws
     func shutdown()
     func flush()
 }
@@ -119,10 +119,10 @@ final class EventSenderEngineImpl: EventSenderEngine {
         semaphore.signal()
     }
 
-    func emit(eventName: String, message: ConfidenceStruct, context: ConfidenceStruct) {
+    func emit(eventName: String, message: ConfidenceStruct, context: ConfidenceStruct) throws {
         writeReqChannel.send(ConfidenceEvent(
             name: eventName,
-            payload: payloadMerger.merge(context: context, message: message),
+            payload: try payloadMerger.merge(context: context, message: message),
             eventTime: Date.backport.now)
         )
     }

diff --git a/Sources/Confidence/PayloadMerger.swift b/Sources/Confidence/PayloadMerger.swift
@@ -1,13 +1,16 @@
 import Foundation
 
 internal protocol PayloadMerger {
-    func merge(context: ConfidenceStruct, message: ConfidenceStruct) -> ConfidenceStruct
+    func merge(context: ConfidenceStruct, message: ConfidenceStruct) throws -> ConfidenceStruct
 }
 
 internal struct PayloadMergerImpl: PayloadMerger {
-    func merge(context: ConfidenceStruct, message: ConfidenceStruct) -> ConfidenceStruct {
-        var map: ConfidenceStruct = context
-        map += message
+    func merge(context: ConfidenceStruct, message: ConfidenceStruct) throws -> ConfidenceStruct {
+        guard message["context"] == nil else {
+            throw ConfidenceError.invalidContextInMessage
+        }
+        var map: ConfidenceStruct = message
+        map["context"] = ConfidenceValue.init(structure: context)
         return map
     }
 }

diff --git a/Tests/ConfidenceTests/ConfidenceFeatureProviderTest.swift b/Tests/ConfidenceTests/ConfidenceFeatureProviderTest.swift
@@ -570,6 +570,17 @@ class ConfidenceFeatureProviderTest: XCTestCase {
         XCTAssertEqual(client.resolveStats, 1)
         XCTAssertEqual(flagApplier.applyCallCount, 0)
     }
+
+    func testInvalidContextInMessage() async throws {
+        let confidence = Confidence.Builder(clientSecret: "test")
+            .build()
+
+        XCTAssertThrowsError(
+            try confidence.track(eventName: "test", message: ["context": ConfidenceValue(string: "test")])
+        ) { error in
+            XCTAssertEqual(error as? ConfidenceError, ConfidenceError.invalidContextInMessage)
+        }
+    }
 }
 
 final class DispatchQueueFake: DispatchQueueType {

diff --git a/Tests/ConfidenceTests/EventSenderEngineTest.swift b/Tests/ConfidenceTests/EventSenderEngineTest.swift
@@ -66,23 +66,27 @@ final class EventSenderEngineTest: XCTestCase {
         let cancellable = uploaderMock.subject.sink { _ in
             expectation.fulfill()
         }
-        eventSenderEngine.emit(
+        try eventSenderEngine.emit(
             eventName: "my_event",
             message: [
-                "a": .init(integer: 0),
-                "message": .init(integer: 1),
+                "a": .init(integer: 0)
             ],
             context: [
                 "a": .init(integer: 2),
-                "message": .init(integer: 3) // the root "message" overrides this
+                "d": .init(integer: 3)
             ])
 
 
         wait(for: [expectation], timeout: 5)
         XCTAssertEqual(try XCTUnwrap(uploaderMock.calledRequest)[0].eventDefinition, "my_event")
         XCTAssertEqual(try XCTUnwrap(uploaderMock.calledRequest)[0].payload, NetworkStruct(fields: [
             "a": .number(0.0),
-            "message": .number(1.0)
+            "context": .structure(
+                .init(fields: [
+                    "a": .number(2),
+                    "d": .number(3)
+                ])
+            )
         ]))
         cancellable.cancel()
     }
@@ -96,7 +100,7 @@ final class EventSenderEngineTest: XCTestCase {
             writeQueue: writeQueue
         )
 
-        eventSenderEngine.emit(eventName: "Hello", message: [:], context: [:])
+        try eventSenderEngine.emit(eventName: "Hello", message: [:], context: [:])
         // TODO: We need to wait for writeReqChannel to complete to make this test meaningful
         XCTAssertNil(uploaderMock.calledRequest)
     }
@@ -115,7 +119,7 @@ final class EventSenderEngineTest: XCTestCase {
             flushPolicies: [ImmidiateFlushPolicy()],
             writeQueue: writeQueue
         )
-        eventSenderEngine.emit(eventName: "testEvent", message: ConfidenceStruct(), context: ConfidenceStruct())
+        try eventSenderEngine.emit(eventName: "testEvent", message: ConfidenceStruct(), context: ConfidenceStruct())
         let expectation = expectation(description: "events batched")
         storageMock.eventsRemoved{
             expectation.fulfill()
@@ -140,7 +144,7 @@ final class EventSenderEngineTest: XCTestCase {
             writeQueue: writeQueue
         )
 
-        eventSenderEngine.emit(eventName: "testEvent", message: ConfidenceStruct(), context: ConfidenceStruct())
+        try eventSenderEngine.emit(eventName: "testEvent", message: ConfidenceStruct(), context: ConfidenceStruct())
 
         writeQueue.sync {
             XCTAssertEqual(storageMock.isEmpty(), false)
@@ -157,10 +161,10 @@ final class EventSenderEngineTest: XCTestCase {
             writeQueue: writeQueue
         )
 
-        eventSenderEngine.emit(eventName: "Hello", message: [:], context: [:])
-        eventSenderEngine.emit(eventName: "Hello", message: [:], context: [:])
-        eventSenderEngine.emit(eventName: "Hello", message: [:], context: [:])
-        eventSenderEngine.emit(eventName: "Hello", message: [:], context: [:])
+        try eventSenderEngine.emit(eventName: "Hello", message: [:], context: [:])
+        try eventSenderEngine.emit(eventName: "Hello", message: [:], context: [:])
+        try eventSenderEngine.emit(eventName: "Hello", message: [:], context: [:])
+        try eventSenderEngine.emit(eventName: "Hello", message: [:], context: [:])
 
 
         writeQueue.sync {

diff --git a/Tests/ConfidenceTests/PayloadMergerTests.swift b/Tests/ConfidenceTests/PayloadMergerTests.swift
@@ -2,15 +2,31 @@ import XCTest
 @testable import Confidence
 
 class PayloadMergerTests: XCTestCase {
-    func testMerge() {
+    func testMerge() throws {
         let context = ["a": ConfidenceValue(string: "hello"), "b": ConfidenceValue(string: "world")]
         let message = ["b": ConfidenceValue(string: "west"), "c": ConfidenceValue(string: "world")]
         let expected = [
-            "a": ConfidenceValue(string: "hello"),
             "b": ConfidenceValue(string: "west"),
-            "c": ConfidenceValue(string: "world")
+            "c": ConfidenceValue(string: "world"),
+            "context": ConfidenceValue(structure: [
+                "a": ConfidenceValue(string: "hello"),
+                "b": ConfidenceValue(string: "world"),
+            ])
         ]
-        let merged = PayloadMergerImpl().merge(context: context, message: message)
+        let merged = try PayloadMergerImpl().merge(context: context, message: message)
         XCTAssertEqual(merged, expected)
     }
+
+    func testInvalidMessage() throws {
+        let context = ["a": ConfidenceValue(string: "hello"), "b": ConfidenceValue(string: "world")]
+        let message = [
+            "b": ConfidenceValue(string: "west"),
+            "context": ConfidenceValue(string: "world")  // simple value context is lost
+        ]
+        XCTAssertThrowsError(
+            try PayloadMergerImpl().merge(context: context, message: message)
+        ) { error in
+            XCTAssertEqual(error as? ConfidenceError, ConfidenceError.invalidContextInMessage)
+        }
+    }
 }