Rename payload -> data, refactor message data encoding. #171

tcard · 2016-01-28T05:05:36Z

Well, this took a bit of time to get right. It started just
wanting to comply with the spec, and access message data just by
doing message.data instead of message.payload.payload. So
I wanted to remove just the amount of code necessary to do that,
but ended up opening a can of worms and didn't stop until it
kind of made sense.

It seems like the whole ARTPayload business was designed with a
use case which didn't really fit the spec at all, so it was then
hacked here and there to kind of work in some (not all) cases
by bypassing the design. It was also really overengineered, with
a whole class hierarchy thingy just for a few predefined cases.
I simplified the whole thing a lot and made it fit the spec.

I could've just set up a custom message.data getter method that
extracted the data from the ARTPayload instance to make it comply
with the spec, but well, this would've hit us down the road
anyway.

ricardopereira · 2016-01-28T07:35:17Z

#130

ricardopereira · 2016-01-28T07:37:57Z

ably-ios/ARTAuthTokenParams.m

@@ -130,7 +129,7 @@ - (NSDictionary *)toDictionaryWithUnion:(NSArray *)items {

    CCHmac(kCCHmacAlgSHA256, cKey, keyLen, cData, dataLen, hmac);
    NSData *mac = [[NSData alloc] initWithBytes:hmac length:sizeof(hmac)];
-    NSString *str = [ARTBase64PayloadEncoder toBase64:mac];
+    NSString *str = [mac base64EncodedStringWithOptions:0];


What option is 0?

Sorry, I thought it was something else. Even so, can you use the enum NSDataBase64Encoding64CharacterLineLength instead of 0?

Fixed at fb8158fbe96882208af51a935998b481084e5da0.

ricardopereira · 2016-01-28T08:27:46Z

I did a quick look and it seems really good 👍.
It is a big refactor but definitely a needed one.

ricardopereira · 2016-01-28T08:32:36Z

I run the tests and I got this one:

ARTRealtimePresenceTest
testPresenceWithData, (([m0 content]) equal to (dataPayload)) failed: ("c29tZURhdGFQYXlsb2Fk") is not equal to ("<736f6d65 44617461 5061796c 6f6164>")
/Users/ricardopereira/Repositories/Whitesmith/ably-ios/ably-iosTests/ARTRealtimePresenceTest.m:1199

                XCTAssertEqualObjects(m0.clientId, [self getClientId]);
                XCTAssertEqualObjects([m0 content], dataPayload);
                [exp fulfill];

And again, I have 2 errors related with the token initialiser:

RestClient__initializer__should_accept_a_token_UsersricardopereiraRepositoriesWhitesmithablyiosablySpecRestClientswift_48, failed - Got error 'Error Domain=io.ably.cocoa Code=40100 "" UserInfo={ARTErrorStatusCode=401, NSLocalizedDescription=, NSLocalizedFailureReason=invalid credentials}'
/Users/ricardopereira/Repositories/Whitesmith/ably-ios/ablySpec/TestUtilities.swift:215

            else if failOnError, let e = error {
                XCTFail("Got error '\(e)'")
            }

RestClient__initializer__should_accept_a_token_UsersricardopereiraRepositoriesWhitesmithablyiosablySpecRestClientswift_48, failed - expected to eventually be nil, got <Error Domain=io.ably.cocoa Code=40100 "" UserInfo={ARTErrorStatusCode=401, NSLocalizedDescription=, NSLocalizedFailureReason=invalid credentials}>
/Users/ricardopereira/Repositories/Whitesmith/ably-ios/ablySpec/RestClient.swift:51

                    let publishTask = publishTestMessage(client)
                    expect(publishTask.error).toEventually(beNil(), timeout: testTimeout)
                }

@tcard Do you run the tests from Xcode directly or do you run them with scan?
I run them with scan.

tcard · 2016-01-28T12:13:40Z

@ricardopereira The first test is indeed my fault, I run the test suite, fixed something but then only ran it partially. Will fix later.

I don't know about the other two, they are passing on my machine. It's that "invalid credentials" thing again.

mattheworiordan · 2016-01-28T13:19:56Z

"invalid credentials"
That was a bug from around 8am this morning https://status.ably.io/incidents/52

Can you check if it passes now?

ricardopereira · 2016-01-28T18:05:51Z

@tcard I run the test and I got:

ablySpec.RestChannel
**presence__get__should_return_presence_fixture_data_UsersricardopereiraRepositoriesWhitesmithablyiosablySpecRestChannelswift_199**, failed - expected to equal <{"example":{"json":"Object"}}>, got <{example =     {json = Object;};}>
/Users/ricardopereira/Repositories/Whitesmith/ably-ios/ablySpec/RestChannel.swift:221

ablySpec.RestChannel
**presence__get__should_return_presence_fixture_data_UsersricardopereiraRepositoriesWhitesmithablyiosablySpecRestChannelswift_199**, failed - expected to not be nil, got <nil>
/Users/ricardopereira/Repositories/Whitesmith/ably-ios/ablySpec/RestChannel.swift:216

ablySpec.RestClient
**RestClient__initializer__should_accept_a_token_UsersricardopereiraRepositoriesWhitesmithablyiosablySpecRestClientswift_48**, failed - Got error 'Error Domain=io.ably.cocoa Code=40100 "" UserInfo={ARTErrorStatusCode=401, NSLocalizedDescription=, NSLocalizedFailureReason=invalid credentials}'

ablySpec.RestClient
**RestClient__initializer__should_accept_a_token_UsersricardopereiraRepositoriesWhitesmithablyiosablySpecRestClientswift_48**, failed - expected to eventually be nil, got <Error Domain=io.ably.cocoa Code=40100 "" UserInfo={ARTErrorStatusCode=401, NSLocalizedDescription=, NSLocalizedFailureReason=invalid credentials}>

About the invalid credentials ¯_(ツ)_/¯

tcard · 2016-01-29T03:29:06Z

OK, that last commit finally fixed everything.

There's only one test missing, "RestClient initializer should accept a token". That's weird, I run tests before pushing, but maybe I missed something on that one... I've taken a quick glance and can't figure out what is wrong with it. Anyway, that's a matter for another pull request, so I'm marking it as pending (also on the sheet).

tcard · 2016-01-29T03:45:27Z

Oh, I forgot:

ricardopereira · 2016-01-29T09:11:12Z

👍

tcard · 2016-01-29T19:07:34Z

@mattheworiordan PTAL

mattheworiordan · 2016-02-01T16:59:50Z

This looks good, however I do not understand why the Cipher tests were removed. Do you plan on reintroducing the cipher tests on the fixture data later?

tcard · 2016-02-02T00:33:16Z

@mattheworiordan I've added an issue (#181) to bring this back. I've taken a look, but I don't think I can just do this right away without changing a few things, and I'd rather focus on getting the API on shape now.

mattheworiordan · 2016-02-02T09:54:21Z

@mattheworiordan I've added an issue (#181) to bring this back. I've taken a look, but I don't think I can just do this right away without changing a few things, and I'd rather focus on getting the API on shape now.

Ok, sure

Well, this took a bit of time to get right. It started just wanting to comply with the spec, and access message data just by doing `message.data` instead of `message.payload.payload`. So I wanted to remove just the amount of code necessary to do that, but ended up opening a can of worms and didn't stop until it kind of made sense. It seems like the whole ARTPayload business was designed with a use case which didn't really fit the spec at all, so it was then hacked here and there to kind of work in some (not all) cases by bypassing the design. It was also really overengineered, with a whole class hierarchy thingy just for a few predefined cases. I simplified the whole thing a lot and made it fit the spec. I could've just set up a custom `message.data` getter method that extracted the data from the ARTPayload instance to make it comply with the spec, but well, this would've hit us down the road anyway.

The test was expecting PresenceMessages _not_ to be decoded, which is the wrong behavior. Plus, DataEncoder has been refactored so that it doesn't take out arguments but return a custom DataEncoderOutput instance. Calling Objective-C functions with out arguments is a real pain and really unsafe. Encrypted test fixtures for presence messages are now working too.

tcard · 2016-02-02T15:18:49Z

Rebased with no code conflicts.

Rename payload -> data, refactor message data encoding.

RTN14d says that a transport error should be considered recoverable if it is > a network failure, a timeout such as RTN14c, or a disconnected > response, other than a token failure RTN14b) However, it does not define what it means by a "network failure", leading to each platform having to provide its own interpretation. In particular, a client recently told us that they’ve been seeing lots of OSStatus 9806 (errSSLClosedAbort) errors. This appears to indicate some sort of failure to perform an SSL handshake. We don’t understand the cause of this issue but we noticed that it was causing the client to transition to the FAILED state. Speaking to Paddy, he said that this error should be provoking a connection retry, not failure. And more broadly, he indicated that, basically, _all_ transport errors should be considered recoverable. So, that’s what we do here. He’s raised specification issue #171 for us to properly specify this and to decide if there are any nuances to consider, but is keen for us to implement this broad behaviour in ably-cocoa ASAP to help the customer experiencing the errSSLClosedAbort errors. Resolves #1817.

ricardopereira reviewed Jan 28, 2016
View reviewed changes

tcard force-pushed the refactor-payload branch 2 times, most recently from 45e46a2 to fb8158f Compare January 28, 2016 12:10

tcard force-pushed the refactor-payload branch from fb8158f to 7e0be9c Compare January 28, 2016 15:51

ricardopereira mentioned this pull request Jan 28, 2016

Travis CI is stalling in some builds #143

Closed

tcard mentioned this pull request Feb 2, 2016

Test ably-common/test-resources/crypto-data-*.json. #181

Closed

tcard added 4 commits February 2, 2016 16:17

Remove leftover variable.

5524878

Fix: decode PresenceMessages when received.

588b9e0

tcard force-pushed the refactor-payload branch from 129e16c to d5719b3 Compare February 2, 2016 15:18

tcard added a commit that referenced this pull request Feb 2, 2016

Merge pull request #171 from ably/refactor-payload

724d035

Rename payload -> data, refactor message data encoding.

tcard merged commit 724d035 into master Feb 2, 2016

tcard deleted the refactor-payload branch February 2, 2016 15:19

tcard mentioned this pull request Feb 4, 2016

Rest: message encoding #95

Closed

ricardopereira mentioned this pull request Feb 4, 2016

RTL6a #193

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename payload -> data, refactor message data encoding. #171

Rename payload -> data, refactor message data encoding. #171

tcard commented Jan 28, 2016

ricardopereira commented Jan 28, 2016

ricardopereira Jan 28, 2016

ricardopereira Jan 28, 2016

tcard Jan 28, 2016

ricardopereira commented Jan 28, 2016

ricardopereira commented Jan 28, 2016

tcard commented Jan 28, 2016

mattheworiordan commented Jan 28, 2016

ricardopereira commented Jan 28, 2016

tcard commented Jan 29, 2016

tcard commented Jan 29, 2016

ricardopereira commented Jan 29, 2016

tcard commented Jan 29, 2016

mattheworiordan commented Feb 1, 2016

tcard commented Feb 2, 2016

mattheworiordan commented Feb 2, 2016

tcard commented Feb 2, 2016

Rename payload -> data, refactor message data encoding. #171

Rename payload -> data, refactor message data encoding. #171

Conversation

tcard commented Jan 28, 2016

ricardopereira commented Jan 28, 2016

ricardopereira Jan 28, 2016

Choose a reason for hiding this comment

ricardopereira Jan 28, 2016

Choose a reason for hiding this comment

tcard Jan 28, 2016

Choose a reason for hiding this comment

ricardopereira commented Jan 28, 2016

ricardopereira commented Jan 28, 2016

tcard commented Jan 28, 2016

mattheworiordan commented Jan 28, 2016

ricardopereira commented Jan 28, 2016

tcard commented Jan 29, 2016

tcard commented Jan 29, 2016

ricardopereira commented Jan 29, 2016

tcard commented Jan 29, 2016

mattheworiordan commented Feb 1, 2016

tcard commented Feb 2, 2016

mattheworiordan commented Feb 2, 2016

tcard commented Feb 2, 2016