Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 5 commits into from
Nov 26, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 8 additions & 14 deletions Analytics/Classes/Internal/SEGAnalyticsUtils.m
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
101 changes: 77 additions & 24 deletions Analytics/Classes/Internal/SEGFileStorage.m
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't needsConversion always going to be false here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the jsonFromData:needsConversion: method will set it's value if necessary.

[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;
Expand All @@ -163,6 +229,7 @@ - (NSData *_Nullable)dataFromPlist:(nonnull id)plist
return data;
}

/// Deprecated
- (id _Nullable)plistFromData:(NSData *_Nonnull)data
{
NSError *error = nil;
Expand All @@ -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
3 changes: 3 additions & 0 deletions AnalyticsTests/MiddlewareTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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") {
Expand Down
9 changes: 9 additions & 0 deletions AnalyticsTests/TrackingTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}

}
6 changes: 3 additions & 3 deletions Podfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -40,6 +40,6 @@ SPEC CHECKSUMS:
Quick: 58d203b1c5e27fff7229c4c1ae445ad7069a7a08
SwiftTryCatch: 2f4ef36cf5396bdb450006b70633dbce5260d3b3

PODFILE CHECKSUM: 96037d813a3e1239ea354ede97146038cd4a31bb
PODFILE CHECKSUM: 6c11ce6879d40225a5ec2b9b5ac275626edbeeb6

COCOAPODS: 1.7.4
COCOAPODS: 1.8.4