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

Merged
merged 8 commits into from Aug 8, 2018

Conversation

3 participants
@funkyboy
Copy link
Collaborator

funkyboy commented Aug 8, 2018

This addresses spec TO3l8* and RSL1i items, aka throw an error before publishing messages when the size exceeds maxMessageSize.

NOTE: queued messages exceeding maxMessageSize (RTL6d1) will be addressed in a separate PR

@funkyboy funkyboy requested review from ricardopereira and paddybyers Aug 8, 2018


// Checked after encoding, so that the client can receive callback with encoding errors
if ([self exceedMaxSize:@[message]]) {
ARTErrorInfo *sizeError = [ARTErrorInfo createWithCode:40009

This comment has been minimized.

@ricardopereira

ricardopereira Aug 8, 2018

Collaborator

Is this a common error between all libs?

This comment has been minimized.

@ricardopereira

ricardopereira Aug 8, 2018

Collaborator

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?

This comment has been minimized.

@funkyboy

funkyboy Aug 8, 2018

Collaborator

Make sense. Issue here: #760

@ricardopereira
Copy link
Collaborator

ricardopereira left a comment

A small comment but looks good 👍

@funkyboy funkyboy merged commit 1399005 into develop Aug 8, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@funkyboy funkyboy deleted the add-maxMessageSize branch Aug 8, 2018

@@ -29,6 +30,10 @@ NS_ASSUME_NONNULL_BEGIN

@property (strong, nonatomic, nullable) id data;

/// The event name, if available
@property (nullable, readwrite, strong, nonatomic) NSString *name;

This comment has been minimized.

@paddybyers

paddybyers Aug 9, 2018

Member

I don't understand why name is moved. Is the idea to include name in PresenceMessage ? That shouldn't happen.

This comment has been minimized.

@funkyboy

funkyboy Aug 9, 2018

Collaborator

@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 exceeds maxMessageSize. Yes, technically that means that an ARTPresenceMessagecan now have a name property.

If you like, I can address this in a separate PR.

This comment has been minimized.

@paddybyers

paddybyers Aug 9, 2018

Member

So is it not easy to do:

ARTMessage.messageSize() {
  return super.messageSize() + name.length;
}

?

This comment has been minimized.

@funkyboy

funkyboy Aug 9, 2018

Collaborator

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

@@ -186,6 +190,16 @@ - (NSDictionary *)toJSON:(NSError *_Nullable *_Nullable)error {
return self;
}

- (NSString *)toJSONString {

This comment has been minimized.

@paddybyers

paddybyers Aug 9, 2018

Member

I don't understand why this was added. We already serialised JSON message payloads didn't we?

This comment has been minimized.

@funkyboy

funkyboy Aug 9, 2018

Collaborator

We needed a way to have a JSON-stringified representation of a dictionary, which is not a built-in function.

expect(err?.message).to(contain("maximum message length exceeded"))
done()
})

This comment has been minimized.

@paddybyers

paddybyers Aug 9, 2018

Member

Extra whitespace?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment