-
Notifications
You must be signed in to change notification settings - Fork 16
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
Reading nested JSON throws TypeError
#45
Comments
Hm, could this be msgpack-related? |
Thanks for the very detailed report, I believe this is a msgpack error. The good news is that I was able to reproduce it so I hope this is an easy fix. Let me look at it |
Hi, just to give a bit of feedback on this, the fix for all storage backends that leverage msgpack is already done, as is for hive which is not using it but with the change I did it will start to use it. This was quite a problem although for the backends supporting msgpack it was quite easy to fix. The same cannot be said of the others as I got mixed scenarios. Some like Hive have this problem internally per my investigations. Other like sembast work perfectly. What is happening in a nutshell is type erasure. Map<String, dynamic> becomes Map<dynamic, dynamic> for inner structures and that's the source of the problem. I've tried for quite some time to avoid using msgpack everywhere but I will now move to that as it gives much more control and avoids backend problems. As I have to change this is more backends I believe it will only be ready on the next weekend |
Yeah, that's the problem with supporting many targets, you end up with the least common denominator of all of them 🙂 I think that maybe the quickest and most honest and/or efficient solution might be to just have the Anyway, thanks for looking into it! |
Hi, it's finally fixed on the latest version. I've added unit tests covering this scenario to make sure it does not happen aggain. Once again, thanks for signalling this. It was quite and important bug and it is now fixed. |
Brilliant! |
Hello. Probably this change causes issues with web version:
|
That's a bit strange to be honest, do you have values of this magnitude ? I only use writeUint64 on messagepack if the value is greater than 0xFFFFFFFF which is quite large. Also I was aware of this limitation on Web but not expecting this to be needed due to the size of the values. Also to confirm:
I separate the unit tests in here explicitly with |
Hum I think they use a dirty trick for this, in a nutshell they use double, not the most optimal thing but I can do something similar on web. I can try once you get back to me on the previous questions |
When I added print() for value:
with our override:
I get this:
So, nothing unusual, simple json. |
Humm, the only place where I write a Uint64 is in here:
I believe this means you are falling in the And if you ended up here your id is higher than 0 because it forked in:
Are you sure the field id is not bigger than 0xFFFFFFFF in some scenario (overflow for example) ? |
Just to make sure this is working, I've tried on web with:
And I had no errors with id 0, 1 or -1 |
I've found the source of an issue, we used DateTime.now().millisecondsSinceEpoch for cache eviction policy and this was the part of a hidden package structure. Thanks for explanation! |
Ok I will still look at this as I think I can improve this corner case for web. Closing this issue |
Describe the bug
So, this one took me quite a bit of digging. It turns out that some backends (stash_hive and stash_isar at least, possibly others) are unable to decode nested JSON structures that where written by a different VM instance (as in, the values have left memory).
It seems that this is due to objects' runtime type of
Map<String, dynamic>
is lost and instead you getMap<dynamic, dynamic>
. The top level is fine because stash casts this, but nested objects are not touched.To Reproduce
Here's a test scenario that can be used to recreate the behavior:
Expected behavior
After writing the entry in step 1, it should be deserializable in step 2, but instead it fails with:
type '_Map<dynamic, dynamic>' is not a subtype of type 'Map<String, dynamic>' in type cast
Version
4.5.2
Additional context
I have been able to work around the issue by changing
PersistenceStore.decodeValue
to this:I'd open a pull request but I've haven't been able to write a testcase which properly sets up the preconditions for the current code to fail. I also have some doubts about the efficiency of the code (Especially since Dart is not tail-recursive :D )
I also just realized that this would probably affect storing of plain maps (without a
fromEncodable
) as well, meaning thatreturn source;
above will return aMap<dynamic, dynamic>
when reading a cached value after a restart. So_toEncodable
may have to be run on that as well.The text was updated successfully, but these errors were encountered: