From 827ffa26a0b1e00a9df5701281ce067c2ffb4127 Mon Sep 17 00:00:00 2001 From: Fabrizio Demaria Date: Tue, 28 May 2024 11:24:19 +0200 Subject: [PATCH 1/4] refactor: context container in payload --- Sources/Confidence/PayloadMerger.swift | 11 +++-- .../EventSenderEngineTest.swift | 15 ++++-- .../ConfidenceTests/PayloadMergerTests.swift | 47 +++++++++++++++++-- 3 files changed, 64 insertions(+), 9 deletions(-) diff --git a/Sources/Confidence/PayloadMerger.swift b/Sources/Confidence/PayloadMerger.swift index 978e5742..21c09636 100644 --- a/Sources/Confidence/PayloadMerger.swift +++ b/Sources/Confidence/PayloadMerger.swift @@ -6,9 +6,14 @@ internal protocol PayloadMerger { internal struct PayloadMergerImpl: PayloadMerger { func merge(context: ConfidenceStruct, message: ConfidenceStruct) -> ConfidenceStruct { - var map: ConfidenceStruct = context - map += message - return map + let messageContextStruct = message["context"]?.asStructure() ?? [:] + var mutableContext = context + messageContextStruct.forEach { entry in + mutableContext.updateValue(entry.value, forKey: entry.key) + } + var mutablePayload = message + mutablePayload["context"] = .init(structure: mutableContext) + return mutablePayload } } diff --git a/Tests/ConfidenceTests/EventSenderEngineTest.swift b/Tests/ConfidenceTests/EventSenderEngineTest.swift index 9979e200..dc56a9cb 100644 --- a/Tests/ConfidenceTests/EventSenderEngineTest.swift +++ b/Tests/ConfidenceTests/EventSenderEngineTest.swift @@ -70,11 +70,14 @@ final class EventSenderEngineTest: XCTestCase { eventName: "my_event", message: [ "a": .init(integer: 0), - "message": .init(integer: 1), + "context": .init(structure: [ + "a": .init(string: "test1"), + "b": .init(string: "test2"), + ]), ], context: [ "a": .init(integer: 2), - "message": .init(integer: 3) // the root "message" overrides this + "d": .init(integer: 3) ]) @@ -82,7 +85,13 @@ 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": .string("test1"), + "b": .string("test2"), + "d": .number(3) + ]) + ) ])) cancellable.cancel() } diff --git a/Tests/ConfidenceTests/PayloadMergerTests.swift b/Tests/ConfidenceTests/PayloadMergerTests.swift index 5e63c55e..408cd8c6 100644 --- a/Tests/ConfidenceTests/PayloadMergerTests.swift +++ b/Tests/ConfidenceTests/PayloadMergerTests.swift @@ -2,13 +2,54 @@ import XCTest @testable import Confidence class PayloadMergerTests: XCTestCase { - func testMerge() { + func testSimpleMerge() { 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) + XCTAssertEqual(merged, expected) + } + + func testOverlapNoStruct() { + 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 + ] + let expected = [ + "b": ConfidenceValue(string: "west"), + "context": ConfidenceValue(structure: [ + "a": ConfidenceValue(string: "hello"), + "b": ConfidenceValue(string: "world"), + ]) + ] + let merged = PayloadMergerImpl().merge(context: context, message: message) + XCTAssertEqual(merged, expected) + } + + func testOverlap() { + let context = ["a": ConfidenceValue(string: "hello"), "b": ConfidenceValue(string: "world")] + let message = [ + "b": ConfidenceValue(string: "west"), + "context": ConfidenceValue(structure: [ + "a": ConfidenceValue(double: 2.0), + "d": ConfidenceValue(string: "inner") + ]) + ] + let expected = [ + "b": ConfidenceValue(string: "west"), + "context": ConfidenceValue(structure: [ + "a": ConfidenceValue(double: 2.0), + "b": ConfidenceValue(string: "world"), + "d": ConfidenceValue(string: "inner") + ]) ] let merged = PayloadMergerImpl().merge(context: context, message: message) XCTAssertEqual(merged, expected) From 8781fa2f25b24b175e9617dca4bd9e604bf38b40 Mon Sep 17 00:00:00 2001 From: Fabrizio Demaria Date: Tue, 28 May 2024 16:36:31 +0200 Subject: [PATCH 2/4] refactor: Throw with context field in message --- Sources/Confidence/Confidence.swift | 16 +++++--- Sources/Confidence/ConfidenceError.swift | 3 ++ .../Confidence/ConfidenceEventSender.swift | 4 +- Sources/Confidence/EventSenderEngine.swift | 6 +-- Sources/Confidence/PayloadMerger.swift | 16 ++++---- .../EventSenderEngineTest.swift | 25 +++++------ .../ConfidenceTests/PayloadMergerTests.swift | 41 ++++--------------- 7 files changed, 44 insertions(+), 67 deletions(-) diff --git a/Sources/Confidence/Confidence.swift b/Sources/Confidence/Confidence.swift index ff4554bf..e9f3e1e0 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").debug( + "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 21c09636..840bcf0d 100644 --- a/Sources/Confidence/PayloadMerger.swift +++ b/Sources/Confidence/PayloadMerger.swift @@ -1,19 +1,17 @@ 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 { - let messageContextStruct = message["context"]?.asStructure() ?? [:] - var mutableContext = context - messageContextStruct.forEach { entry in - mutableContext.updateValue(entry.value, forKey: entry.key) + func merge(context: ConfidenceStruct, message: ConfidenceStruct) throws -> ConfidenceStruct { + guard message["context"] == nil else { + throw ConfidenceError.invalidContextInMessage } - var mutablePayload = message - mutablePayload["context"] = .init(structure: mutableContext) - return mutablePayload + var map: ConfidenceStruct = message + map["context"] = ConfidenceValue.init(structure: context) + return map } } diff --git a/Tests/ConfidenceTests/EventSenderEngineTest.swift b/Tests/ConfidenceTests/EventSenderEngineTest.swift index dc56a9cb..ec571dfd 100644 --- a/Tests/ConfidenceTests/EventSenderEngineTest.swift +++ b/Tests/ConfidenceTests/EventSenderEngineTest.swift @@ -66,14 +66,10 @@ 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), - "context": .init(structure: [ - "a": .init(string: "test1"), - "b": .init(string: "test2"), - ]), + "a": .init(integer: 0) ], context: [ "a": .init(integer: 2), @@ -87,8 +83,7 @@ final class EventSenderEngineTest: XCTestCase { "a": .number(0.0), "context": .structure( .init(fields: [ - "a": .string("test1"), - "b": .string("test2"), + "a": .number(2), "d": .number(3) ]) ) @@ -105,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) } @@ -124,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() @@ -149,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) @@ -166,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 408cd8c6..29e59ac0 100644 --- a/Tests/ConfidenceTests/PayloadMergerTests.swift +++ b/Tests/ConfidenceTests/PayloadMergerTests.swift @@ -2,7 +2,7 @@ import XCTest @testable import Confidence class PayloadMergerTests: XCTestCase { - func testSimpleMerge() { + 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 = [ @@ -13,45 +13,20 @@ class PayloadMergerTests: XCTestCase { "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 testOverlapNoStruct() { + 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 ] - let expected = [ - "b": ConfidenceValue(string: "west"), - "context": ConfidenceValue(structure: [ - "a": ConfidenceValue(string: "hello"), - "b": ConfidenceValue(string: "world"), - ]) - ] - let merged = PayloadMergerImpl().merge(context: context, message: message) - XCTAssertEqual(merged, expected) - } - - func testOverlap() { - let context = ["a": ConfidenceValue(string: "hello"), "b": ConfidenceValue(string: "world")] - let message = [ - "b": ConfidenceValue(string: "west"), - "context": ConfidenceValue(structure: [ - "a": ConfidenceValue(double: 2.0), - "d": ConfidenceValue(string: "inner") - ]) - ] - let expected = [ - "b": ConfidenceValue(string: "west"), - "context": ConfidenceValue(structure: [ - "a": ConfidenceValue(double: 2.0), - "b": ConfidenceValue(string: "world"), - "d": ConfidenceValue(string: "inner") - ]) - ] - let merged = PayloadMergerImpl().merge(context: context, message: message) - XCTAssertEqual(merged, expected) + XCTAssertThrowsError( + try PayloadMergerImpl().merge(context: context, message: message) + ) { error in + XCTAssertEqual(error as? ConfidenceError, ConfidenceError.invalidContextInMessage) + } } } From 038a4a9cf497b9cdd05d8c1dfe38c0e1e48d99af Mon Sep 17 00:00:00 2001 From: Fabrizio Demaria Date: Tue, 28 May 2024 16:58:18 +0200 Subject: [PATCH 3/4] test: User-facing error test --- .../ConfidenceFeatureProviderTest.swift | 11 +++++++++++ 1 file changed, 11 insertions(+) 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 { From e52a79b1806ccda9e18be9c0f2eb93b3efd3a03d Mon Sep 17 00:00:00 2001 From: Fabrizio Demaria Date: Wed, 29 May 2024 09:30:57 +0200 Subject: [PATCH 4/4] refactor: warrning level Co-authored-by: Nicklas Lundin --- Sources/Confidence/Confidence.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Sources/Confidence/Confidence.swift b/Sources/Confidence/Confidence.swift index e9f3e1e0..ace0e236 100644 --- a/Sources/Confidence/Confidence.swift +++ b/Sources/Confidence/Confidence.swift @@ -135,7 +135,7 @@ public class Confidence: ConfidenceEventSender { eventSenderEngine.flush() } } catch { - Logger(subsystem: "com.confidence", category: "track").debug( + Logger(subsystem: "com.confidence", category: "track").warning( "Error from EventProducer, failed to track event: \(event.name)") } }