search

k-paxian / dart-json-mapper

Serialize / Deserialize Dart Objects to / from JSON
https://pub.dev/packages/dart_json_mapper
Other
399 stars 33 forks source link

`fromMap` and `toMap` inefficiency due to string serialisation/deserialisation #168

Closed buzzware closed 2 years ago

buzzware commented 2 years ago

I'm using Firebase Firestore and model classes marked with @jsonSerializable.

I want to read and write my models to and from Firestore. For reading, the firestore sdk returns a Map<String,dynamic> that I want to create a model instance from. To become a Map, there must be deserialization step inside of the firestore sdk. I'm using JsonMapper.fromMap, which adds serialization & deserialization steps internally, and leads to confusing bugs due to serialization failing when you think you're only deserializing. It also seems like a lot of waste of memory and processing time, which probably becomes significant with a lot of records. In general, maps are very useful for manipulating unknown data structures, and are often used as a step in-between class instances and json strings (including by de/serialization libraries), so I think good map support is relevant to this library.

Please consider implementing fromMap and toMap(model) as generated code that assigns and converts fields, without the json serialization & deserialization.

k-paxian commented 2 years ago

Dear Gary,

Thank you so much for pointing that, indeed there was a redundancy and overhead related to enforcing bidirectional consistency. I've simplified those methods, hope this helps a bit.

On the other hand over-generation of code following this pattern: ... code that assigns and converts fields ... will lead to a completely different class of issues like this, this, etc.

buzzware commented 2 years ago

I don't really understand the change, but I don't need to either. Fundamentally I would like to see fromMap(map) be much more efficient than JsonMapper.deserialize(jsonEncode(map)) and thats what I expected before I looked at the source. Even if it isn't at present, I would still keep fromMap/toMap and I understand that this is open source etc and it might happen one day without affecting dependent apps.

k-paxian commented 2 years ago

Fundamentally I would like to see fromMap(map) be much more efficient than JsonMapper.deserialize(jsonEncode(map)) and thats what I expected before I looked at the source.

That's exact the plot of the change being made, from now on fromMap complexity is equal to JsonMapper.deserialize complexity, and fromMap became an alias for JsonMapper.deserialize method.

Even if it isn't at present, I would still keep fromMap/toMap and I understand that this is open source etc and it might happen one day without affecting dependent apps.

Sure, that's just a convenience methods part of the public facing API, so they will remain available in future.