-
Notifications
You must be signed in to change notification settings - Fork 26
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
Add max message size #759
Add max message size #759
Changes from all commits
5d50aef
3a947a7
369d6f9
768cdbd
0fb93ab
654d441
06a0ead
5c1b13f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,6 +15,7 @@ | |
#import "ARTBaseMessage+Private.h" | ||
#import "ARTDataQuery.h" | ||
#import "ARTRest+Private.h" | ||
#import "ARTDefault.h" | ||
|
||
@implementation ARTChannel { | ||
__weak ARTRest *_rest; | ||
|
@@ -73,6 +74,17 @@ - (void)publish:(art_nullable NSString *)name data:(art_nullable id)data extras: | |
if (callback) callback([ARTErrorInfo createFromNSError:error]); | ||
return; | ||
} | ||
|
||
// Checked after encoding, so that the client can receive callback with encoding errors | ||
if ([self exceedMaxSize:@[message]]) { | ||
ARTErrorInfo *sizeError = [ARTErrorInfo createWithCode:40009 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this a common error between all libs? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should add those error codes in an enum or at least have it in a global&static var, even though, I know there are a bunch of hard coded codes in the source. Maybe raising an issue is appropriated. What do you think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Make sense. Issue here: #760 |
||
message:@"maximum message length exceeded"]; | ||
if (callback) { | ||
callback(sizeError); | ||
} | ||
return; | ||
} | ||
|
||
[self internalPostMessages:messagesWithDataEncoded callback:callback]; | ||
} | ||
|
||
|
@@ -97,6 +109,17 @@ - (void)publish:(NSString *)name data:(id)data clientId:(NSString *)clientId ext | |
if (callback) callback([ARTErrorInfo createFromNSError:error]); | ||
return; | ||
} | ||
|
||
// Checked after encoding, so that the client can receive callback with encoding errors | ||
if ([self exceedMaxSize:@[message]]) { | ||
ARTErrorInfo *sizeError = [ARTErrorInfo createWithCode:40009 | ||
message:@"maximum message length exceeded"]; | ||
if (callback) { | ||
callback(sizeError); | ||
} | ||
return; | ||
} | ||
|
||
[self internalPostMessages:messagesWithDataEncoded callback:callback]; | ||
} | ||
|
||
|
@@ -106,6 +129,7 @@ - (void)publish:(NSArray<ARTMessage *> *)messages { | |
|
||
- (void)publish:(__GENERIC(NSArray, ARTMessage *) *)messages callback:(art_nullable void (^)(ARTErrorInfo *__art_nullable error))callback { | ||
NSError *error = nil; | ||
|
||
NSMutableArray<ARTMessage *> *messagesWithDataEncoded = [NSMutableArray new]; | ||
for (ARTMessage *message in messages) { | ||
[messagesWithDataEncoded addObject:[self encodeMessageIfNeeded:message error:&error]]; | ||
|
@@ -114,9 +138,28 @@ - (void)publish:(__GENERIC(NSArray, ARTMessage *) *)messages callback:(art_nulla | |
callback([ARTErrorInfo createFromNSError:error]); | ||
return; | ||
} | ||
|
||
// Checked after encoding, so that the client can receive callback with encoding errors | ||
if ([self exceedMaxSize:messages]) { | ||
ARTErrorInfo *sizeError = [ARTErrorInfo createWithCode:40009 | ||
message:@"maximum message length exceeded"]; | ||
if (callback) { | ||
callback(sizeError); | ||
} | ||
return; | ||
} | ||
|
||
[self internalPostMessages:messagesWithDataEncoded callback:callback]; | ||
} | ||
|
||
- (BOOL)exceedMaxSize:(NSArray<ARTBaseMessage *> *)messages { | ||
NSInteger size = 0; | ||
for (ARTMessage *message in messages) { | ||
size += [message messageSize]; | ||
} | ||
return size > [ARTDefault maxMessageSize]; | ||
} | ||
|
||
- (ARTMessage *)encodeMessageIfNeeded:(ARTMessage *)message error:(NSError **)error { | ||
if (!self.dataEncoder) { | ||
return message; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -175,6 +175,10 @@ - (NSDictionary *)toJSON:(NSError *_Nullable *_Nullable)error { | |
return (NSDictionary *)json; | ||
} | ||
|
||
- (NSString *)toJSONString { | ||
return self; | ||
} | ||
|
||
@end | ||
|
||
@implementation NSDictionary (ARTJsonCompatible) | ||
|
@@ -186,6 +190,16 @@ - (NSDictionary *)toJSON:(NSError *_Nullable *_Nullable)error { | |
return self; | ||
} | ||
|
||
- (NSString *)toJSONString { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand why this was added. We already serialised JSON message payloads didn't we? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We needed a way to have a JSON-stringified representation of a dictionary, which is not a built-in function. |
||
NSError *err = nil; | ||
NSData *jsonData = [NSJSONSerialization dataWithJSONObject:self options:0 error:&err]; | ||
if (err) { | ||
return nil; | ||
} | ||
NSString *jsonString = [[NSString alloc] initWithData:jsonData encoding:NSUTF8StringEncoding]; | ||
return jsonString; | ||
} | ||
|
||
@end | ||
|
||
@implementation NSURL (ARTLog) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why
name
is moved. Is the idea to includename
inPresenceMessage
? That shouldn't happen.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@paddybyers
ARTBaseMessage
is a basic superclass. Given the way classes are structured now, this change allows to limit the number of implementations of the method that calculates if a bunch of messages exceedsmaxMessageSize
. Yes, technically that means that anARTPresenceMessage
can now have aname
property.If you like, I can address this in a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So is it not easy to do:
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's one way to do it. I was caught by other aspects of this PR and I didn't think of it :)
Issue here: #762