Swapped JSON in for the storage format instead of plists. (#854)

* Converted file storage to JSON from plist. * Updated string storage to account for JSON vs plist differences. * update podfile lock. * Remove some NSNull hacks. * Updated tests to validate null values.
segmentio · Nov 26, 2019 · 2c8971c · 2c8971c
1 parent 19d6c56
commit 2c8971c
Show file tree

Hide file tree

Showing 5 changed files with 100 additions and 41 deletions.
diff --git a/Analytics/Classes/Internal/SEGAnalyticsUtils.m b/Analytics/Classes/Internal/SEGAnalyticsUtils.m
@@ -107,42 +107,36 @@ void SEGLog(NSString *format, ...)
 
 static id SEGCoerceJSONObject(id obj)
 {
-    // Hotfix: Storage format should support NSNull instead
-    if ([obj isKindOfClass:[NSNull class]]) {
-        return @"<null>";
-    }
     // if the object is a NSString, NSNumber
     // then we're good
     if ([obj isKindOfClass:[NSString class]] ||
-        [obj isKindOfClass:[NSNumber class]]) {
+        [obj isKindOfClass:[NSNumber class]] ||
+        [obj isKindOfClass:[NSNull class]]) {
         return obj;
     }
 
     if ([obj isKindOfClass:[NSArray class]]) {
         NSMutableArray *array = [NSMutableArray array];
         for (id i in obj) {
+            NSObject *value = i;
             // Hotfix: Storage format should support NSNull instead
-            if ([i isKindOfClass:[NSNull class]]) {
-                continue;
+            if ([value isKindOfClass:[NSNull class]]) {
+                value = [NSData data];
             }
-            [array addObject:SEGCoerceJSONObject(i)];
+            [array addObject:SEGCoerceJSONObject(value)];
         }
         return array;
     }
 
     if ([obj isKindOfClass:[NSDictionary class]]) {
         NSMutableDictionary *dict = [NSMutableDictionary dictionary];
         for (NSString *key in obj) {
-            // Hotfix for issue where SEGFileStorage uses plist which does NOT support NSNull
-            // So when `[NSNull null]` gets passed in as track property values the queue serialization fails
-            if ([obj[key] isKindOfClass:[NSNull class]]) {
-                continue;
-            }
+            NSObject *value = obj[key];
             if (![key isKindOfClass:[NSString class]])
                 SEGLog(@"warning: dictionary keys should be strings. got: %@. coercing "
                        @"to: %@",
                        [key class], [key description]);
-            dict[key.description] = SEGCoerceJSONObject(obj[key]);
+            dict[key.description] = SEGCoerceJSONObject(value);
         }
         return dict;
     }

diff --git a/Analytics/Classes/Internal/SEGFileStorage.m b/Analytics/Classes/Internal/SEGFileStorage.m
@@ -87,32 +87,38 @@ - (NSData *)dataForKey:(NSString *)key
 
 - (NSDictionary *)dictionaryForKey:(NSString *)key
 {
-    return [self plistForKey:key];
+    return [self jsonForKey:key];
 }
 
 - (void)setDictionary:(NSDictionary *)dictionary forKey:(NSString *)key
 {
-    [self setPlist:dictionary forKey:key];
+    [self setJSON:dictionary forKey:key];
 }
 
 - (NSArray *)arrayForKey:(NSString *)key
 {
-    return [self plistForKey:key];
+    return [self jsonForKey:key];
 }
 
 - (void)setArray:(NSArray *)array forKey:(NSString *)key
 {
-    [self setPlist:array forKey:key];
+    [self setJSON:array forKey:key];
 }
 
 - (NSString *)stringForKey:(NSString *)key
 {
-    return [self plistForKey:key];
+    // unlike plists, we can't have stray values, they need to be
+    // in a dictionary or array container.
+    NSDictionary *data = [self jsonForKey:key];
+    return data[key];
 }
 
 - (void)setString:(NSString *)string forKey:(NSString *)key
 {
-    [self setPlist:string forKey:key];
+    // unlike plists, we can't have stray values, they need to be
+    // in a dictionary or array container.
+    NSDictionary *data = @{key: string};
+    [self setJSON:data forKey:key];
 }
 
 + (NSURL *)applicationSupportDirectoryURL
@@ -129,20 +135,80 @@ - (NSURL *)urlForKey:(NSString *)key
 
 #pragma mark - Helpers
 
-- (id _Nullable)plistForKey:(NSString *)key
+- (id _Nullable)jsonForKey:(NSString *)key
 {
+    id result = nil;
+
     NSData *data = [self dataForKey:key];
-    return data ? [self plistFromData:data] : nil;
+    if (data) {
+        BOOL needsConversion = NO;
+        result = [self jsonFromData:data needsConversion:&needsConversion];
+        if (needsConversion) {
+            [self setJSON:result forKey:key];
+        }
+    }
+    return result;
 }
 
-- (void)setPlist:(id _Nonnull)plist forKey:(NSString *)key
+- (void)setJSON:(id _Nonnull)plist forKey:(NSString *)key
 {
-    NSData *data = [self dataFromPlist:plist];
+    NSDictionary *dict = nil;
+
+    // json doesn't allow stand alone values like plist (previous storage format) does so
+    // we need to massage it a little.
+    if ([plist isKindOfClass:[NSDictionary class]] || [plist isKindOfClass:[NSArray class]]) {
+        dict = plist;
+    } else {
+        dict = @{key: plist};
+    }
+
+    NSData *data = [self dataFromJSON:dict];
     if (data) {
         [self setData:data forKey:key];
     }
 }
 
+- (NSData *_Nullable)dataFromJSON:(id)json
+{
+    NSError *error = nil;
+    NSData *data = [NSJSONSerialization dataWithJSONObject:json options:0 error:&error];
+    if (error) {
+        SEGLog(@"Unable to serialize data from json object; %@, %@", error, json);
+    }
+    return data;
+}
+
+- (id _Nullable)jsonFromData:(NSData *_Nonnull)data needsConversion:(BOOL *)needsConversion
+{
+    NSError *error = nil;
+    id result = [NSJSONSerialization JSONObjectWithData:data options:0 error:&error];
+    if (error) {
+        // maybe it's a plist and needs to be converted.
+        result = [self plistFromData:data];
+        if (result != nil) {
+            *needsConversion = YES;
+        } else {
+            SEGLog(@"Unable to parse json from data %@", error);
+        }
+    }
+    return result;
+}
+
+- (void)createDirectoryAtURLIfNeeded:(NSURL *)url
+{
+    if (![[NSFileManager defaultManager] fileExistsAtPath:url.path
+                                              isDirectory:NULL]) {
+        NSError *error = nil;
+        if (![[NSFileManager defaultManager] createDirectoryAtPath:url.path
+                                       withIntermediateDirectories:YES
+                                                        attributes:nil
+                                                             error:&error]) {
+            SEGLog(@"error: %@", error.localizedDescription);
+        }
+    }
+}
+
+/// Deprecated
 - (NSData *_Nullable)dataFromPlist:(nonnull id)plist
 {
     NSError *error = nil;
@@ -163,6 +229,7 @@ - (NSData *_Nullable)dataFromPlist:(nonnull id)plist
     return data;
 }
 
+/// Deprecated
 - (id _Nullable)plistFromData:(NSData *_Nonnull)data
 {
     NSError *error = nil;
@@ -176,18 +243,4 @@ - (id _Nullable)plistFromData:(NSData *_Nonnull)data
     return plist;
 }
 
-- (void)createDirectoryAtURLIfNeeded:(NSURL *)url
-{
-    if (![[NSFileManager defaultManager] fileExistsAtPath:url.path
-                                              isDirectory:NULL]) {
-        NSError *error = nil;
-        if (![[NSFileManager defaultManager] createDirectoryAtPath:url.path
-                                       withIntermediateDirectories:YES
-                                                        attributes:nil
-                                                             error:&error]) {
-            SEGLog(@"error: %@", error.localizedDescription);
-        }
-    }
-}
-
 @end
diff --git a/AnalyticsTests/MiddlewareTests.swift b/AnalyticsTests/MiddlewareTests.swift
@@ -21,6 +21,7 @@ let customizeAllTrackCalls = SEGBlockMiddleware { (context, next) in
       let newEvent = "[New] \(track.event)"
       var newProps = track.properties ?? [:]
       newProps["customAttribute"] = "Hello"
+      newProps["nullTest"] = NSNull()
       ctx.payload = SEGTrackPayload(
         event: newEvent,
         properties: newProps,
@@ -65,6 +66,8 @@ class MiddlewareTests: QuickSpec {
       let track = passthrough.lastContext?.payload as? SEGTrackPayload
       expect(track?.event) == "[New] Purchase Success"
       expect(track?.properties?["customAttribute"] as? String) == "Hello"
+      let isNull = (track?.properties?["nullTest"] is NSNull)
+      expect(isNull) == true
     }
 
     it("expects event to be swallowed if next is not called") {

diff --git a/AnalyticsTests/TrackingTests.swift b/AnalyticsTests/TrackingTests.swift
@@ -89,6 +89,15 @@ class TrackingTests: QuickSpec {
       expect(payload?.groupId) == "acme-company"
       expect(payload?.traits?["employees"] as? Int) == 2333
     }
+
+    it("handles null values") {
+      analytics.track("null test", properties: [
+        "nullTest": NSNull()
+        ])
+      let payload = passthrough.lastContext?.payload as? SEGTrackPayload
+      let isNull = (payload?.properties?["nullTest"] is NSNull)
+      expect(isNull) == true
+    }
   }
 
 }
diff --git a/Podfile.lock b/Podfile.lock
@@ -16,7 +16,7 @@ DEPENDENCIES:
   - SwiftTryCatch (from `https://github.com/segmentio/SwiftTryCatch.git`)
 
 SPEC REPOS:
-  https://github.com/cocoapods/specs.git:
+  https://github.com/CocoaPods/Specs.git:
     - Alamofire
     - Alamofire-Synchronous
     - Nimble
@@ -40,6 +40,6 @@ SPEC CHECKSUMS:
   Quick: 58d203b1c5e27fff7229c4c1ae445ad7069a7a08
   SwiftTryCatch: 2f4ef36cf5396bdb450006b70633dbce5260d3b3
 
-PODFILE CHECKSUM: 96037d813a3e1239ea354ede97146038cd4a31bb
+PODFILE CHECKSUM: 6c11ce6879d40225a5ec2b9b5ac275626edbeeb6
 
-COCOAPODS: 1.7.4
+COCOAPODS: 1.8.4