Encoder: handle invalid types gracefully #592

ricardopereira · 2017-04-11T18:55:15Z

ricardopereira · 2017-04-11T18:59:59Z

Source/ARTJsonEncoder.m

+    }
+    @catch (NSException *exception) {
+        if (error) {
+            *error = [[NSError alloc] initWithDomain:ARTAblyErrorDomain code:ARTClientCodeErrorInvalidType userInfo:@{NSLocalizedDescriptionKey: exception.reason}];


@mattheworiordan I decided to not use the isValidJSONObject method because it doesn't tell us why it isn't valid. On the other hand, catching the exception gives us more details about the failure.

tcard · 2017-04-11T19:41:40Z

Source/ARTChannel.m

-        [self.logger error:@"ARTChannel: error encoding data: %@", error];
-        [NSException raise:NSInvalidArgumentException format:@"ARTChannel: error encoding data: %@", error];
+    message = [message encodeWithEncoder:self.dataEncoder error:error];
+    if (error) {


error && *error; error alone will be true just by passing a non-null NSError **.

In fact, so that we always log this, and not only when error is passed, we should do something like:

NSError *e = nil; message = [message encodeWithEncoder:self.dataEncoder error:&e]; if (e) { [self.logger error:@"ARTChannel: error encoding data: %@", e]; } if (error) { *error = e; }

Done f4619aa

tcard · 2017-04-11T19:45:14Z

Source/ARTEncoder.h

@@ -43,8 +43,8 @@ ART_ASSUME_NONNULL_BEGIN
 - (art_nullable NSData *)encodePresenceMessage:(ARTPresenceMessage *)message;
 - (art_nullable NSData *)encodePresenceMessages:(NSArray *)messages;

- (art_nullable NSData *)encodeProtocolMessage:(ARTProtocolMessage *)message;
- (art_nullable ARTProtocolMessage *)decodeProtocolMessage:(NSData *)data;
+- (art_nullable NSData *)encodeProtocolMessage:(ARTProtocolMessage *)message error:(NSError * __autoreleasing *)error;


I'm not sure __autoreleasing is necessary, but I think __Nullable is: NSError *_Nullable *_Nullable

I did that because I took a look at the NSJSONSerialization header file that returns errors as well 😅 [NSJSONSerialization JSONObjectWithData:<#(nonnull NSData *)#> options:<#(NSJSONReadingOptions)#> error:<#(NSError * _Nullable __autoreleasing * _Nullable)#>]

tcard · 2017-04-11T19:49:00Z

Source/ARTRealtime.m

@@ -810,12 +811,28 @@ - (BOOL)isActive {
 - (void)sendImpl:(ARTProtocolMessage *)msg callback:(void (^)(ARTStatus *))cb {
    if (msg.ackRequired) {
        msg.msgSerial = [NSNumber numberWithLongLong:self.msgSerial++];


This should be moved below the error handling, so that errors don't increase msgSerial.

@tcard Ah you're right. So I need to encode it twice?

What about decrementing the msgSerial if the encode fails?

Ah right, I didn't notice we need the updated value. I think msg.msgSerial = [NSNumber numberWithLongLong:self.msgSerial + 1] here and then below just self.msgSerial++; would be the simplest.

Done f4619aa. Thanks

tcard · 2017-04-11T19:50:18Z

Spec/Utilities.swift

+                            done()
+                        }
+                    }
+                }


Missing a test for incoming malformed JSON.

(Also for malformed MsgPack, BTW.)

Done: c4de4b9

mattheworiordan · 2017-04-12T14:06:34Z

Ok, so we would like to extend these tests so that they additionally cover incoming invalid/non-encodeable payloads for both binary (msgpack) and text (JSON) transports i.e. if the client library receives a payload that causes a native encoding fault / exception, then the library must handle that in a way that does not cause it to crash. We need to test that both for REST and Realtime.

* Test: JSON encoder using invalid types * Test: should handle and emit the invalid data error * Fix: handle and emit the invalid data encoder error * Update existing tests * Test: should decode data with malformed JSON * fixup! Fix: handle and emit the invalid data encoder error * Test: should decode data with malformed MsgPack * Test: incoming invalid/non-encodeable payloads and messages * Add error argument on every encode/decode method * Update tests * fixup! Add error argument on every encode/decode method * TestProxyTransport: sendWithData needs to capture protocol messages * Add missing authDetailsFromDictionary * fixup! Add error argument on every encode/decode method * Fix: invalid type cast * Fix: bad access crash * Fix test * fixup! Fix: bad access crash * Fix: missing encoder fields * If message error is empty then use the reason prop * Keep the original error domain to give more context * Transport: use only one way to send data

ricardopereira added 4 commits April 11, 2017 15:38

Test: JSON encoder using invalid types

5153d2a

Test: should handle and emit the invalid data error

d5bc0f5

Fix: handle and emit the invalid data encoder error

a889cc4

Update existing tests

659fc60

ricardopereira requested review from mattheworiordan and tcard April 11, 2017 18:55

ricardopereira commented Apr 11, 2017

View reviewed changes

tcard suggested changes Apr 11, 2017

View reviewed changes

ricardopereira added 3 commits April 11, 2017 23:52

Test: should decode data with malformed JSON

c4de4b9

fixup! Fix: handle and emit the invalid data encoder error

f4619aa

Test: should decode data with malformed MsgPack

4c29e26

ricardopereira added 3 commits April 12, 2017 18:32

Test: incoming invalid/non-encodeable payloads and messages

657f82c

Add error argument on every encode/decode method

9423f23

Update tests

aa9b4ce

tcard approved these changes Apr 12, 2017

View reviewed changes

ricardopereira added 5 commits April 12, 2017 20:43

fixup! Add error argument on every encode/decode method

dbcf66a

TestProxyTransport: sendWithData needs to capture protocol messages

29a7a59

Add missing authDetailsFromDictionary

df0990c

fixup! Add error argument on every encode/decode method

6981ade

Fix: invalid type cast

7288dce

ricardopereira force-pushed the fix-json-encoder branch from 3f74c2e to 7288dce Compare April 13, 2017 00:21

ricardopereira added 6 commits April 13, 2017 01:34

Fix: bad access crash

cab47d2

Fix test

8c6e232

fixup! Fix: bad access crash

09b7e8e

Fix: missing encoder fields

f3eb7ac

If message error is empty then use the reason prop

843b2de

Keep the original error domain to give more context

72239d8

ricardopereira force-pushed the fix-json-encoder branch from 05c17c1 to 7073b04 Compare April 13, 2017 10:15

Transport: use only one way to send data

05cc875

ricardopereira force-pushed the fix-json-encoder branch from 7073b04 to 05cc875 Compare April 13, 2017 10:52

ricardopereira merged commit 32918ed into master Apr 13, 2017

ricardopereira deleted the fix-json-encoder branch April 13, 2017 12:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Encoder: handle invalid types gracefully #592

Encoder: handle invalid types gracefully #592

ricardopereira commented Apr 11, 2017

ricardopereira Apr 11, 2017

tcard Apr 11, 2017

ricardopereira Apr 11, 2017

tcard Apr 11, 2017 •

edited

Loading

ricardopereira Apr 11, 2017

tcard Apr 11, 2017

ricardopereira Apr 11, 2017

ricardopereira Apr 11, 2017

tcard Apr 11, 2017

ricardopereira Apr 11, 2017

tcard Apr 11, 2017

tcard Apr 11, 2017

ricardopereira Apr 11, 2017

mattheworiordan commented Apr 12, 2017

Encoder: handle invalid types gracefully #592

Encoder: handle invalid types gracefully #592

Conversation

ricardopereira commented Apr 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcard Apr 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattheworiordan commented Apr 12, 2017

tcard Apr 11, 2017 •

edited

Loading