From 29d66231c9aa432ab0bbad8c5c1141940f5521e5 Mon Sep 17 00:00:00 2001 From: Natalie Bunduwongse Date: Wed, 22 Jul 2026 22:24:30 +1200 Subject: [PATCH] feat(audience): validate required properties for reserved events at runtime Track(string, Dictionary) let a caller send purchase/progression/resource/ achievement_unlocked/milestone_reached with no properties at all, bypassing every check the typed IEvent classes enforce. It now validates the same required properties those classes do, throwing ArgumentException the same way an empty event name already does in that method. Also fixes an inconsistency this change would otherwise have made worse: Track(IEvent) caught every ToProperties() exception and warned instead of throwing, including for Immutable's own built-in events. That catch exists to stop a buggy third-party IEvent from crashing the game, but a caller forgetting a required field on Purchase/Progression/etc is a caller bug like any other, and should throw the same way Identify()/Alias() already do for equivalent mistakes. A new internal IBuiltInEvent marker lets Track(IEvent) tell the two cases apart: built-in events now throw straight through, a consumer's own custom IEvent implementation is still caught and dropped with a warning. A new test reflects over every IBuiltInEvent implementation to guard against a future one being added without a matching ReservedEvents.RequiredProperties entry. Purchase.Value changes from decimal to string: a decimal can't precisely hold very large or high-precision amounts, and this keeps it consistent with the TS SDK's equivalent change. Co-Authored-By: Claude Sonnet 5 --- .../Scripts/AudienceSample.Events.cs | 4 +- src/Packages/Audience/README.md | 2 +- .../Audience/Runtime/Events/IEvent.cs | 21 ++++- .../Audience/Runtime/Events/ReservedEvents.cs | 27 ++++++ .../Runtime/Events/ReservedEvents.cs.meta | 11 +++ .../Audience/Runtime/Events/TypedEvents.cs | 20 ++-- .../Audience/Runtime/ImmutableAudience.cs | 27 +++++- src/Packages/Audience/Runtime/Utility/Log.cs | 5 + .../Tests/Runtime/Events/TypedEventTests.cs | 27 +++++- .../Tests/Runtime/ImmutableAudienceTests.cs | 92 ++++++++++++++++--- 10 files changed, 204 insertions(+), 32 deletions(-) create mode 100644 src/Packages/Audience/Runtime/Events/ReservedEvents.cs create mode 100644 src/Packages/Audience/Runtime/Events/ReservedEvents.cs.meta diff --git a/examples/audience/Assets/SampleApp/Scripts/AudienceSample.Events.cs b/examples/audience/Assets/SampleApp/Scripts/AudienceSample.Events.cs index f9c625fba..fa0f0d418 100644 --- a/examples/audience/Assets/SampleApp/Scripts/AudienceSample.Events.cs +++ b/examples/audience/Assets/SampleApp/Scripts/AudienceSample.Events.cs @@ -53,7 +53,7 @@ internal readonly struct EventSpec new EventSpec("wishlist_remove", new[] { EventField.Text("gameId") }), new EventSpec("purchase", new[] { EventField.Text("currency"), - EventField.Number("value"), + EventField.Text("value"), EventField.Text("itemId", optional: true), EventField.Text("itemName", optional: true), EventField.Number("quantity", optional: true), @@ -129,7 +129,7 @@ internal readonly struct EventSpec return new Purchase { Currency = OptionalString(props, "currency") ?? "", - Value = OptionalDecimal(props, "value") ?? 0m, + Value = OptionalString(props, "value") ?? "0", ItemId = OptionalString(props, "itemId"), ItemName = OptionalString(props, "itemName"), Quantity = OptionalInt(props, "quantity"), diff --git a/src/Packages/Audience/README.md b/src/Packages/Audience/README.md index 25e1877df..bf6a7f779 100644 --- a/src/Packages/Audience/README.md +++ b/src/Packages/Audience/README.md @@ -35,7 +35,7 @@ public static class Analytics Debug = true, }); - ImmutableAudience.Track(new Purchase { Currency = "USD", Value = 9.99m }); + ImmutableAudience.Track(new Purchase { Currency = "USD", Value = "9.99" }); } } ``` diff --git a/src/Packages/Audience/Runtime/Events/IEvent.cs b/src/Packages/Audience/Runtime/Events/IEvent.cs index c4fbfdea1..241a6af0e 100644 --- a/src/Packages/Audience/Runtime/Events/IEvent.cs +++ b/src/Packages/Audience/Runtime/Events/IEvent.cs @@ -10,10 +10,10 @@ namespace Immutable.Audience /// . /// /// - /// Implementations validate required fields inside - /// ; - /// catches the throw and - /// drops the event with a warning. + /// Implementations validate required fields inside . + /// catches the throw and warns + /// for a consumer-authored ; see + /// for the exception to that. /// public interface IEvent { @@ -32,4 +32,17 @@ public interface IEvent /// Dictionary ToProperties(); } + + /// + /// Marks an implementation as one of Immutable's own + /// built-in event types (, , + /// , , + /// ). Not implementable outside this + /// assembly: it exists only so + /// can tell a validation failure in one of these apart from an exception + /// thrown by a consumer's own custom . + /// + internal interface IBuiltInEvent : IEvent + { + } } diff --git a/src/Packages/Audience/Runtime/Events/ReservedEvents.cs b/src/Packages/Audience/Runtime/Events/ReservedEvents.cs new file mode 100644 index 000000000..91bf8583f --- /dev/null +++ b/src/Packages/Audience/Runtime/Events/ReservedEvents.cs @@ -0,0 +1,27 @@ +#nullable enable + +using System.Collections.Generic; + +namespace Immutable.Audience +{ + /// + /// Required property names per reserved event, enforced at runtime by + /// . + /// The typed classes enforce the same rules + /// at compile time (constructor/property level) and again in + /// ToProperties; this closes the gap for callers who use the + /// string overload instead and so bypass all of that. + /// + internal static class ReservedEvents + { + internal static readonly IReadOnlyDictionary RequiredProperties = + new Dictionary + { + ["purchase"] = new[] { "currency", "value" }, + ["progression"] = new[] { "status" }, + ["resource"] = new[] { "flow", "currency", "amount" }, + ["achievement_unlocked"] = new[] { "achievement_id", "achievement_name" }, + ["milestone_reached"] = new[] { "name" }, + }; + } +} diff --git a/src/Packages/Audience/Runtime/Events/ReservedEvents.cs.meta b/src/Packages/Audience/Runtime/Events/ReservedEvents.cs.meta new file mode 100644 index 000000000..1dc4e5607 --- /dev/null +++ b/src/Packages/Audience/Runtime/Events/ReservedEvents.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: 11f4ec3e32e164c5fb21fdf8fbdbdce1 +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: diff --git a/src/Packages/Audience/Runtime/Events/TypedEvents.cs b/src/Packages/Audience/Runtime/Events/TypedEvents.cs index 52474e01b..b422499da 100644 --- a/src/Packages/Audience/Runtime/Events/TypedEvents.cs +++ b/src/Packages/Audience/Runtime/Events/TypedEvents.cs @@ -44,7 +44,7 @@ internal static class ProgressionStatusExtensions /// Player progressing through a world / level / stage. Track via /// . /// - public class Progression : IEvent + public class Progression : IBuiltInEvent { /// /// Required. Where the player is in the progression flow. @@ -134,7 +134,7 @@ internal static class ResourceFlowExtensions /// In-game currency earned or spent. Track via /// . /// - public class Resource : IEvent + public class Resource : IBuiltInEvent { /// /// Required. Whether this is a gain or a spend. @@ -193,7 +193,7 @@ public Dictionary ToProperties() /// Real-money transaction. Track via /// . /// - public class Purchase : IEvent + public class Purchase : IBuiltInEvent { /// /// Required. ISO 4217 three-letter uppercase currency code (for @@ -202,9 +202,11 @@ public class Purchase : IEvent public string? Currency { get; set; } /// - /// Required. The transaction amount in . + /// Required. The transaction amount in , as a + /// numeric-looking string (e.g. "9.99"), not a number, to + /// avoid floating-point precision loss. /// - public decimal? Value { get; set; } + public string? Value { get; set; } /// /// Optional. Stable identifier of the item purchased. @@ -247,13 +249,13 @@ public Dictionary ToProperties() if (Currency == null || !IsIso4217(Currency)) throw new ArgumentException( $"Purchase.Currency '{Currency}' must be a three-letter uppercase ISO 4217 code"); - if (Value is null) + if (string.IsNullOrEmpty(Value)) throw new ArgumentException("Purchase.Value is required. Set it before calling Track(IEvent)."); var props = new Dictionary { ["currency"] = Currency, - ["value"] = Value.Value + ["value"] = Value }; if (ItemId != null) props["item_id"] = ItemId; @@ -316,7 +318,7 @@ internal static class AchievementTypeExtensions /// Player unlocked an achievement. Track via /// . /// - public class AchievementUnlocked : IEvent + public class AchievementUnlocked : IBuiltInEvent { /// /// Required. Stable identifier for the achievement (for example, @@ -362,7 +364,7 @@ public Dictionary ToProperties() /// Named milestone or achievement reached by the player. Track via /// . /// - public class MilestoneReached : IEvent + public class MilestoneReached : IBuiltInEvent { /// /// Required. The milestone identifier (for example, diff --git a/src/Packages/Audience/Runtime/ImmutableAudience.cs b/src/Packages/Audience/Runtime/ImmutableAudience.cs index 7826c21cf..5420610e6 100644 --- a/src/Packages/Audience/Runtime/ImmutableAudience.cs +++ b/src/Packages/Audience/Runtime/ImmutableAudience.cs @@ -328,6 +328,10 @@ internal static void OnResume() /// /// The event to send. Must not be null, and /// must not be empty (both throw). + /// + /// One of Immutable's own built-in events (, + /// , etc.) is missing a required field. + /// public static void Track(IEvent evt) { if (!_initialized) return; @@ -339,7 +343,6 @@ public static void Track(IEvent evt) var config = _config; if (config == null) return; - // Consumer-supplied impl; catch so a buggy IEvent cannot crash the game. string eventName; Dictionary properties; try @@ -349,6 +352,8 @@ public static void Track(IEvent evt) } catch (Exception ex) { + // See IBuiltInEvent: built-in events throw straight through. + if (evt is IBuiltInEvent) throw; Log.Warn(AudienceLogs.TrackIEventThrew(evt.GetType().Name, ex)); return; } @@ -367,11 +372,17 @@ public static void Track(IEvent evt) /// /// The wire-format event name. Must not be empty (throws otherwise). /// Optional event properties. + /// + /// is empty, or is a reserved event + /// (purchase, progression, resource, + /// achievement_unlocked) missing one of its required properties. + /// public static void Track(string eventName, Dictionary? properties = null) { if (!_initialized) return; if (string.IsNullOrEmpty(eventName)) throw new ArgumentException(AudienceLogs.TrackStringEmptyName, nameof(eventName)); + ValidateReservedEventProperties(eventName, properties); var state = _state; if (!state.Level.CanTrack()) return; @@ -382,6 +393,20 @@ public static void Track(string eventName, Dictionary? propertie EnqueueTrackedEvent(eventName, SnapshotCallerDict(properties), _session?.SessionId, state, config); } + // See ReservedEvents. Unrecognised event names are never checked here. + private static void ValidateReservedEventProperties(string eventName, Dictionary? properties) + { + if (!ReservedEvents.RequiredProperties.TryGetValue(eventName, out var required)) return; + + var missing = new List(); + foreach (var key in required) + { + if (properties == null || !properties.ContainsKey(key)) missing.Add(key); + } + if (missing.Count > 0) + throw new ArgumentException(AudienceLogs.TrackStringMissingRequiredProps(eventName, missing), nameof(properties)); + } + // Shared tail for both Track overloads and TrackFromSession. private static void EnqueueTrackedEvent( string eventName, diff --git a/src/Packages/Audience/Runtime/Utility/Log.cs b/src/Packages/Audience/Runtime/Utility/Log.cs index eef021aa5..90b3f0f86 100644 --- a/src/Packages/Audience/Runtime/Utility/Log.cs +++ b/src/Packages/Audience/Runtime/Utility/Log.cs @@ -1,6 +1,7 @@ #nullable enable using System; +using System.Collections.Generic; namespace Immutable.Audience { @@ -77,6 +78,10 @@ internal static string TrackIEventThrew(string evtTypeName, Exception ex) => internal static string TrackIEventEmptyName(string evtTypeName) => $"Track(IEvent): {evtTypeName}.EventName returned null or empty."; + internal static string TrackStringMissingRequiredProps(string eventName, IReadOnlyList missing) => + $"Track(\"{eventName}\", ...) is missing required propert{(missing.Count > 1 ? "ies" : "y")}: " + + $"{string.Join(", ", missing)}."; + // ---- Identify / Alias ---- // Empty/malformed ids are caller bugs: thrown as ArgumentException, not // logged (see ImmutableAudience.Identify/Alias). IdentifyDiscarded/ diff --git a/src/Packages/Audience/Tests/Runtime/Events/TypedEventTests.cs b/src/Packages/Audience/Tests/Runtime/Events/TypedEventTests.cs index c59527143..f5df6fccf 100644 --- a/src/Packages/Audience/Tests/Runtime/Events/TypedEventTests.cs +++ b/src/Packages/Audience/Tests/Runtime/Events/TypedEventTests.cs @@ -1,4 +1,5 @@ using System; +using System.Linq; using NUnit.Framework; namespace Immutable.Audience.Tests @@ -6,6 +7,24 @@ namespace Immutable.Audience.Tests [TestFixture] internal class TypedEventTests { + // Guards against a new built-in typed event being added without a + // matching ReservedEvents entry, which would leave it silently + // unvalidated on the Track(string, Dictionary) path. + [Test] + public void EveryBuiltInEventHasAReservedEventsEntry() + { + var builtInTypes = typeof(IBuiltInEvent).Assembly.GetTypes() + .Where(t => typeof(IBuiltInEvent).IsAssignableFrom(t) && !t.IsInterface && !t.IsAbstract); + + foreach (var type in builtInTypes) + { + var evt = (IEvent)Activator.CreateInstance(type)!; + Assert.That(ReservedEvents.RequiredProperties.ContainsKey(evt.EventName), Is.True, + $"{type.Name} implements IBuiltInEvent but ReservedEvents.RequiredProperties has no entry " + + $"for \"{evt.EventName}\" (add one, even [])."); + } + } + [Test] public void Progression_EventName_IsProgression() { @@ -115,7 +134,7 @@ public void Purchase_ProducesCorrectProperties() var evt = new Purchase { Currency = "USD", - Value = 9.99m, + Value = "9.99", ItemId = "gem_pack_01", ItemName = "Starter Gem Pack", Quantity = 1, @@ -125,7 +144,7 @@ public void Purchase_ProducesCorrectProperties() var props = evt.ToProperties(); Assert.AreEqual("USD", props["currency"]); - Assert.AreEqual(9.99m, props["value"]); + Assert.AreEqual("9.99", props["value"]); Assert.AreEqual("gem_pack_01", props["item_id"]); Assert.AreEqual("Starter Gem Pack", props["item_name"]); Assert.AreEqual(1, props["quantity"]); @@ -135,7 +154,7 @@ public void Purchase_ProducesCorrectProperties() [Test] public void Purchase_OptionalFieldsOmitted_WhenNull() { - var props = new Purchase { Currency = "EUR", Value = 5.00m }.ToProperties(); + var props = new Purchase { Currency = "EUR", Value = "5.00" }.ToProperties(); Assert.IsTrue(props.ContainsKey("currency")); Assert.IsTrue(props.ContainsKey("value")); @@ -154,7 +173,7 @@ public void Purchase_EventName_IsPurchase() [Test] public void Purchase_WithoutCurrency_ThrowsOnToProperties() { - var evt = new Purchase { Value = 9.99m }; + var evt = new Purchase { Value = "9.99" }; var ex = Assert.Throws(() => evt.ToProperties()); Assert.That(ex!.Message, Does.Contain("Currency")); diff --git a/src/Packages/Audience/Tests/Runtime/ImmutableAudienceTests.cs b/src/Packages/Audience/Tests/Runtime/ImmutableAudienceTests.cs index f32634db3..14797c934 100644 --- a/src/Packages/Audience/Tests/Runtime/ImmutableAudienceTests.cs +++ b/src/Packages/Audience/Tests/Runtime/ImmutableAudienceTests.cs @@ -405,7 +405,32 @@ public void Track_NullEvent_ThrowsEvenAtNoneConsent() } [Test] - public void Track_IEventMissingRequiredField_DropsWithWarn() + public void Track_BuiltInEventMissingRequiredField_Throws() + { + ImmutableAudience.Init(MakeConfig()); + + // Purchase with no Value set: ToProperties throws. Purchase is one of + // Immutable's own built-in events, so that's a caller bug and must + // throw straight through, the same as Identify()/Alias() do for an + // equivalent mistake, rather than ship an incomplete event silently. + Assert.Throws(() => ImmutableAudience.Track(new Purchase { Currency = "USD" })); + + ImmutableAudience.Shutdown(); + var queueDir = AudiencePaths.QueueDir(_testDir); + var contents = Directory.GetFiles(queueDir, "*.json") + .Select(File.ReadAllText).ToList(); + Assert.IsFalse(contents.Any(c => c.Contains("\"purchase\"")), + "purchase event with missing required Value must never reach the queue"); + } + + private class ThrowingCustomEvent : IEvent + { + public string EventName => "custom_event"; + public Dictionary ToProperties() => throw new InvalidOperationException("boom"); + } + + [Test] + public void Track_CustomEventThrows_DropsWithWarn() { ImmutableAudience.Init(MakeConfig()); @@ -413,12 +438,12 @@ public void Track_IEventMissingRequiredField_DropsWithWarn() Log.Writer = lines.Add; try { - // Purchase with no Value set: ToProperties throws; Track must - // catch, warn, and drop rather than ship an incomplete event. - Assert.DoesNotThrow(() => ImmutableAudience.Track(new Purchase { Currency = "USD" })); - // Assert the stable parts (event-type name and trailing "Dropping") - // so the test survives any change to the exception type or message. - Assert.That(lines, Has.Some.Contains(nameof(Purchase))); + // A consumer's own custom IEvent (not one of Immutable's built-in + // types) is not held to the same "throw straight through" standard: + // we can't predict what a third-party implementation might throw, + // so it's caught, warned, and dropped instead of crashing the game. + Assert.DoesNotThrow(() => ImmutableAudience.Track(new ThrowingCustomEvent())); + Assert.That(lines, Has.Some.Contains(nameof(ThrowingCustomEvent))); Assert.That(lines, Has.Some.Contains("Dropping")); } finally { Log.Writer = null; } @@ -427,8 +452,8 @@ public void Track_IEventMissingRequiredField_DropsWithWarn() var queueDir = AudiencePaths.QueueDir(_testDir); var contents = Directory.GetFiles(queueDir, "*.json") .Select(File.ReadAllText).ToList(); - Assert.IsFalse(contents.Any(c => c.Contains("\"purchase\"")), - "purchase event with missing required Value must be dropped, not enqueued"); + Assert.IsFalse(contents.Any(c => c.Contains("custom_event")), + "custom event whose ToProperties() throws must be dropped, not enqueued"); } [Test] @@ -450,6 +475,51 @@ public void Track_NullOrEmptyEventName_ThrowsEvenAtNoneConsent() Assert.Throws(() => ImmutableAudience.Track("")); } + [Test] + public void Track_StringOverload_ReservedEventMissingRequiredProps_Throws() + { + // The typed IEvent classes can't stop a caller from bypassing them + // entirely and calling the string overload directly for a reserved + // event name; this closes that gap. + ImmutableAudience.Init(MakeConfig()); + + Assert.Throws( + () => ImmutableAudience.Track("purchase", new Dictionary { ["currency"] = "USD" }), + "purchase is missing required 'value'"); + Assert.Throws( + () => ImmutableAudience.Track("progression"), + "progression is missing required 'status'"); + Assert.Throws( + () => ImmutableAudience.Track("resource", new Dictionary { ["flow"] = "source" }), + "resource is missing required 'currency' and 'amount'"); + Assert.Throws( + () => ImmutableAudience.Track("achievement_unlocked", new Dictionary { ["achievement_id"] = "a1" }), + "achievement_unlocked is missing required 'achievement_name'"); + } + + [Test] + public void Track_StringOverload_ReservedEventWithAllRequiredProps_DoesNotThrow() + { + ImmutableAudience.Init(MakeConfig()); + + Assert.DoesNotThrow(() => ImmutableAudience.Track("purchase", + new Dictionary { ["currency"] = "USD", ["value"] = 9.99m })); + Assert.DoesNotThrow(() => ImmutableAudience.Track("progression", + new Dictionary { ["status"] = "start" })); + Assert.DoesNotThrow(() => ImmutableAudience.Track("resource", + new Dictionary { ["flow"] = "source", ["currency"] = "gold", ["amount"] = 10f })); + Assert.DoesNotThrow(() => ImmutableAudience.Track("achievement_unlocked", + new Dictionary { ["achievement_id"] = "a1", ["achievement_name"] = "First Steps" })); + } + + [Test] + public void Track_StringOverload_UnrecognisedEventName_DoesNotValidate() + { + ImmutableAudience.Init(MakeConfig()); + + Assert.DoesNotThrow(() => ImmutableAudience.Track("some_custom_event")); + } + [Test] public void Identify_NullOrEmptyUserId_Throws() { @@ -674,7 +744,7 @@ public void Track_TypedPurchase_WritesCorrectEventName() ImmutableAudience.Track(new Purchase { Currency = "USD", - Value = 9.99m + Value = "9.99" }); ImmutableAudience.Shutdown(); @@ -958,7 +1028,7 @@ public void Track_IEvent_ActiveSession_EnvelopeIncludesSessionId() var sessionId = ImmutableAudience.SessionId; Assert.IsFalse(string.IsNullOrEmpty(sessionId), "sanity: a session should be active after Init"); - ImmutableAudience.Track(new Purchase { Currency = "USD", Value = 9.99m }); + ImmutableAudience.Track(new Purchase { Currency = "USD", Value = "9.99" }); ImmutableAudience.FlushQueueToDiskForTesting(); var queueDir = AudiencePaths.QueueDir(_testDir);