RSL6b test, log errors#215
Conversation
…oblems, added test for inconsistent message encoding
paddybyers
left a comment
paddybyers
left a comment
There was a problem hiding this comment.
LGTM but pls seem comments
| data = Base64Coder.decode((String) data); | ||
| } | ||
| catch (IllegalArgumentException e) { | ||
| throw AblyException.fromThrowable(e); |
There was a problem hiding this comment.
What's going to happen to this exception? I don't think it will propagate to anywhere useful. In this case I think we should do the same as in the case of being unable to decrypt - ie leave the data and encoding unchanged, and log a message saying that the base64 decoding step failed.
There was a problem hiding this comment.
Without rethrowing the lib would crash on IllegalArgumentException. Exception is caught in Channel.onMessage or Channel.onPresense and logged. Rethrowing is used in the same method for JSON case.
| data = opts.getCipher().decrypt((byte[]) data); | ||
| continue; | ||
| } | ||
| else |
There was a problem hiding this comment.
If we have a compound if/then/else then it either needs braces everywhere, or nowhere, but not mixed like this. In in doublt, use braces.
There was a problem hiding this comment.
Sure, do you have a doc on code formatting style?
| continue; | ||
| } | ||
| else | ||
| Log.e(TAG, "Encrypted message received but encryption is not set up"); |
There was a problem hiding this comment.
I'm not sure this is an error - I would make this Log.i I think.
Also lets say ".. encryption is not enabled on channel", and also include the channel name in the message pls.
There was a problem hiding this comment.
I would if it was easy to get channel name at this point. Most I can do is throw custom AblyException, catch it in Channel.onMessage and log it there. But it seems like a lot of effort for a minor thing
No, without catching the lib would crash on IllegalArgumentException.
Ok, well we definitely need to do the same thing in each case. If we throw here, then the client doesn't see the event; only a log message. If we catch and do nothing except log it, the client will get an event, with an |
No we don't. Use the same coding style as is there already pls to the extent that it's obvious what that is :) |
|
By rethrowing I mean of course throwing it as a different exception. I just saw a bug (crash in case of wrong base64 data) and fixed it in a way similar to what was used in a nearby statement in the same method. |
|
@psolstice RSL6b says that invalid Base64 does not prevent the event being delivered to the listener. So I think throwing here is wrong; the decode exception should caught, and a log message written out, and the I think we should make the JSON decode failure do the same. |
|
I think you're right, I'll change the behavior |
| data = Base64Coder.decode((String) data); | ||
| } | ||
| catch (IllegalArgumentException e) { throw AblyException.fromThrowable(e); } | ||
| catch (IllegalArgumentException e) { break; } |
There was a problem hiding this comment.
Sorry, I should have been clearer. I think we need a log message (Log.e() I guess) in this case as well.
|
Thanks |
2 participants
Fixed decoding message bugs, added logging for decoding/decryption problems, added test for inconsistent message encoding