Check for JSONObject.NULL before casting to String [Fixes #209] #723
Conversation
Codecov Report
@@ Coverage Diff @@
## master #723 +/- ##
============================================
- Coverage 53.43% 53.42% -0.02%
- Complexity 1747 1748 +1
============================================
Files 132 132
Lines 10293 10295 +2
Branches 1427 1428 +1
============================================
Hits 5500 5500
- Misses 4339 4340 +1
- Partials 454 455 +1
Continue to review full report at Codecov.
|
|
I am looking at this and am trying to identify the issue... It would be better to catch the bug rather than do a check because there's a bug somewhere. Session token is read only, if you attempt to call |
|
Well, even ParseObject.mergeFromServer() may blindly insert a non-String object into that key -- and I'm sure there are plenty of other places as well that may do that. We could add some debugging code to
Where do you see that? I don't see any checks for put() where key == sessionToken. |
|
I have a strong feeling that something wrong is happing in this class. But before fixing I am trying to write a failing test. If anyone would like to help investigating, it would be great. |
|
@natario1 OK, I was able to reproduce the issue by using Charles Proxy to rewrite the server response of the I'll build in the put() override mentioned above, so I can trace it back to wherever the value is getting set. Note that I do not think the class you linked to is relevant. The first thing I notice is that ParseCurrentUserCoder's KEY_SESSION_TOKEN key name is |
|
@jhansche that's cool, but I don't think parse-server is ever returning null session tokens. Even automatic users have one, right? |
|
I really think it's the coder I mentioned. This reproduces the error. @Test
public void testEncodeDecodeWithNullSessionToken() throws Exception {
ParseUser.State state = new ParseUser.State.Builder()
.sessionToken(null)
.build();
ParseUserCurrentCoder coder = ParseUserCurrentCoder.get();
JSONObject object = coder.encode(state, null, PointerEncoder.get());
ParseUser.State.Builder builder =
coder.decode(new ParseUser.State.Builder(), object, ParseDecoder.get());
state = builder.build();
assertNull(state.sessionToken());
}Going to set up a fix... Thanks for helping @jhansche |
|
I added the |
|
OK, so you think it's when persisting to disk and then restoring from disk? The docs for ParseCurrentUserCoder suggest it will only be used when LDS is not enabled. I do use LDS, so I'm not sure if that's the same issue that my app is running into. The problem with PCUC that you mentioned, is that it only removes the value if it was non-null, but it will leave the key if it did exist but contained a null-like value, like Similarly, the encode() method lets the super class encode everything into the JSONObject, and then only replaces the session token key if it was non-null, but will not remove the JSONObject.NULL if it had already been encoded that way. |
|
With the same put() override and running your test, this is the stacktrace: But I think the documentation for ParseUserCurrentCoder is incorrect then, because the coder is still used even with local datastore enabled: But OfflineObjectStore seems to only use the passed-in legacy store in order to migrate old data to the new offline store. |
|
I have created #724 . Yes, exactly... So you are experiencing this with LDS enabled? |
|
Yes, my app is seeing the same crash with LDS enabled. |
|
OK, sorry, didn't read. I think this was the root cause of the problem. But the fact that you are experiencing it with LDS enabled is confusing. |
|
Another change that fixes it across all cases, without having to check it on the way out: What's the reason for ever keeping JSONObject.NULL? |
|
@natario1 we're seeing that issue too, randomly. |
|
@flovilmart do you have traces? There's another bug somewhere then. If it's very frequent we could merge this, it's just not elegant at all, like a try catch to hide a broken contract. |
|
Yeah, we’ll investigate more Monday, we don’t have other traces besides what’s there already. I’d like us not to merge this one as it’s pushing the dust under the carpet. |
|
Hi All, I am running into this issue as well with LDS enabled. Currently running Android SDK: 1.16.3 Here is my stack trace: All of this is originating from: ParseQuery.getQuery(XXXXXXXXXX.class).getFirst(); |
|
@jhansche We're jumping back on this one.
I don't believe we should ever have a JSONObject.NULL. How does it handle when you put a null value in the database? does it interpoate to null? |
|
The source of the bug is probably a encode/decode cycle, in which a legit null value gets encoded to JsonObject.NULL for storing (more or less), and then it is not correctly decoded to null again. I fixed the LDS-off decoder in #724 , I would expect a similar issue somewhere in LDS-on classes... But can't help more than this |
|
It would help to be able to track down how that JsonObject.NULL value gets into the map. Given that we see this crash on the way out anyway, is there any objection to crashing loudly in BTW, maybe we should reopen #209 if this still an issue? |
|
@jhansche as it stands now, we could harden the logic around the sessionToken setting, from null to check also for JSONObject.NULL. I'm not a big fan of preventing the put of JSONObject.NULL at large as it hides a bigger issue. I believe we're on the right track, with the serializer into SQLite. Perhaps at one point, because a key is missing (with anon users) the session token is stored as null and not 'undefined'. When unserializing from the LDS' this may cause the JSON.NULL to pop in. |
|
@flovilmart |
|
@flovilmart the reason blocking it at Right now I see a lot of "maybe" and "might" and "perhaps", but no suggestions for proving any of those theories. |
|
Anyway we can instrument the code with a proper printStackTrace if it ever happens? Right now we don’t know what puts it in, wether it’a malformed parse server response or a serialization issue? I’d be comfortable with having it at the put, preventing it, and print a stacktrace that would be easily reported |
|
Right, that's what I'm talking about. My suggestion was to throw an exception (which will of course include the stacktrace), but if you'd rather just log it, that's at least a way to make an informed decision. Throwing the exception will be more noticeable (because it will crash) and it is more likely to get reported here with the stacktrace. Simply logging the exception would require pairing the #209 crash, with an earlier stacktrace that got printed to the logs, and that might make it more difficult to track down the root cause. |
|
Can you direct me to the line number where you're suggesting we put it? I can go ahead and push it through our app (since we haven't been able to repro this locally), and I can attempt to get it logged remotely to see what we come up with. |
|
Same for us (me and @flovilmart). @jhansche I am looking at the SDK right now, but I'm not totally sure where I would put this log/debug/crash to be able to prove who is saving this JSON.Null. Thanks for you help. |
|
From #723 (comment) |
|
I’ll also double check that the server never send out null in any case for that particular key. |
|
I added a stacktrace logger into this method: So here is the stacktrace: |
|
Maybe what I could do is put the stacktrace logging where @jhansche suggested, so I would know who is trying to set a null session in the State. |
|
@mmimeault the session token can be null if the user was never saved. |
|
@flovilmart to be clear, the problem is not whether @mmimeault is that stack trace what you got when encountering the error in your live app environment? |
|
Looking at ParseDecoder.decode I see: And because So the problem is that it doesn't convert ParseEncoder.encode does convert |
|
This is where we converge as for the core reason for the issue, @mmimeault has an app going live soon with extra tooling in there. let's see the results of that and try to release something before next week. |
ParseEncoder encodes (null) => JSONObject.NULL. But ParseDecoder was not doing the inverse conversion. That resulted in JSONObject.NULL values being in the ParseObject.State map, which means any calls to State.get(key) that were casting the result to a specific type (i.e., String) without first checking the Object return type would result in a ClassCastException. ParseObject already accounts for this possibility by checking the Object type before casting it, and returning null if the type did not match. The known case that causes crashes is parse-community#209, where ParseUser.State.sessionToken() casts the result of get(KEY_SESSION_TOKEN) to String. When that was null it would be encoded as JSONObject.NULL due to the ParseEncoder; but after decoding it would not convert back to a native null, leading to the ClassCastException.
|
Ok so well, your solution @jhansche sounds good. I applied the same fixes to a production build with a lot of logging. It seem to be when you logout and then relaunch the app following that logout. The logout persist a null token as I can see. (validate me). I haven't see any other case where the token was set to null from the logs I got during the weekend. The SDK explicitly encode So from what I understand, it is totally possible to have a null session token in a some particular case. Saving them a JSONObject.NULL will always result into a crash when trying to retrieve that particular token. So I'm fine to go with your solution/implementation that is pending since August. @flovilmart --^ |
Check for JSONObject.NULL before casting to String [Fixes parse-community#209]
The same technique is already used in
ParseObject.getString(), but because this is a ParseObject.State subclass, there is no getString() method. The documentation explicitly states that JSONObject.NULL may be contained in the data.Before the change, the unit test fails with: