From a9d41fae6646a1381874f46e8708b2475c6b026e Mon Sep 17 00:00:00 2001 From: Fabrizio Demaria Date: Wed, 29 May 2024 11:12:00 +0200 Subject: [PATCH] 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 --------- Co-authored-by: Nicklas Lundin --- Sources/Confidence/Confidence.swift | 16 +++++++---- Sources/Confidence/ConfidenceError.swift | 3 ++ .../Confidence/ConfidenceEventSender.swift | 4 +-- Sources/Confidence/EventSenderEngine.swift | 6 ++-- Sources/Confidence/PayloadMerger.swift | 11 +++++--- .../ConfidenceFeatureProviderTest.swift | 11 ++++++++ .../EventSenderEngineTest.swift | 28 +++++++++++-------- .../ConfidenceTests/PayloadMergerTests.swift | 24 +++++++++++++--- 8 files changed, 73 insertions(+), 30 deletions(-) diff --git a/Sources/Confidence/Confidence.swift b/Sources/Confidence/Confidence.swift index ff4554bf..ace0e236 100644 --- 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 index e26ebda9..6dee3c3a 100644 --- 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 index 911f2d7e..e5a84f80 100644 --- 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 index 394c08c2..4fa8f98a 100644 --- 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 index 978e5742..840bcf0d 100644 --- 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 index ea666157..e167a886 100644 --- 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 index 9979e200..ec571dfd 100644 --- a/Tests/ConfidenceTests/EventSenderEngineTest.swift +++ b/Tests/ConfidenceTests/EventSenderEngineTest.swift @@ -66,15 +66,14 @@ 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) ]) @@ -82,7 +81,12 @@ final class EventSenderEngineTest: XCTestCase { 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 index 5e63c55e..29e59ac0 100644 --- 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) + } + } }