search

yahoo / squidb

SquiDB is a SQLite database library for Android and iOS
https://github.com/yahoo/squidb/wiki
Apache License 2.0
1.31k stars 132 forks source link

Model clone method is not working correctly #251

Closed MFlisar closed 7 years ago

MFlisar commented 7 years ago

I think here's a small bug:

https://github.com/yahoo/squidb/blob/cf35046974e2cf7ef6d1f727c9aaf72d9780275f/squidb/src/com/yahoo/squidb/data/AbstractModel.java#L171

The problem is, this clone method does not create a new transitoryData, so the clone and the base Model operate on the same HashMap object...

sbosley commented 7 years ago

Yep you're absolutely right. Thanks for reporting!

sbosley commented 7 years ago

My inclination for this is to make cloned models get a new transitory map with a shallow copy of all current transitory values by default. If the transitory values themselves needed to be deep-copied, that detail should be left to the user (since SquiDB doesn't know what types of transitories might be stored or how to deep copy them). Alternatively, if users prefers that cloned models not carry over any transitory values, they can implement custom cloning logic to clear the transitory map after the default clone has taken place. Any disagreement or other thoughts?

MFlisar commented 7 years ago

I would think not copying transitory data would be more intuitive but I would be fine with your suggestion as well. This is more or less a question of what someone prefers, don't think any decision is (thinking generic) better than the other...

If you make shallow copies offering a clearTransitoryData() function or similar would be helpful I think. Currently you can't access the HashMap directly and would have to clear values one by one with the correct key (if I remember correctly)

sbosley commented 7 years ago

@MFlisar yes you're right about clearing values, and in fact on my branch to fix this I've already added a clearAllTransitory() method.

Can I ask why you feel that not copying transitory data would be more intuitive? In my experience, transitory data is mainly used for one of two things: keeping track of some relation to another model (which should not be lost after a clone), and keeping a parsed non-primitive value attached to the model for tracking some non-primitive serialized property (which might as well be shared to save on performance). Do you have other use cases that are more common?

MFlisar commented 7 years ago

You're right. I'm using it for this use case as well.

In my use case I clone an object and assign it to a new parent and this is the case in all my use cases... That's why I thought it would be more intuitive first.

But thinking about it again I think your suggestion is better. Cloning should clone as much data as possible and the user should get the responsibility to clear the cloned data if necessary...

sbosley commented 7 years ago

Ok sounds good, thank you for the feedback! I'll have a fix pushed up soon, and we'll try to get a new release out within the next couple days. Thanks again for catching and reporting this bug!

MFlisar commented 7 years ago

Thanks for correcting this so fast. I'm currently simply solving it via a reflection method and will remove it as soon as the new release is out...