Nozbe / WatermelonDB

🍉 Reactive & asynchronous database for powerful React and React Native apps ⚡️
https://watermelondb.dev
MIT License
10.62k stars 600 forks source link

Reset behavior is dangerous and should not be default #1810

Open barakmichaely opened 4 months ago

barakmichaely commented 4 months ago

I started using Watermelon in my app and encountered a situation where the db randomly reset (and deleted all data) even though I did not change the schema version. This made me realize that I cannot use Watermelon db in my project, since even the slightest possibility of the app randomly deleting all of the user's data is an absolute deal breaker for me. And since there is no way to prevent the database reset behavior I am forced to use a different db library.

I highly recommend removing the reset behavior as the default, and having the default behavior just throw an error. As a professional app developer, it's better to accidentally release a broken version of the app than accidentally erase all of the user's data. A broken app can be updated and fixed, user data cannot be restored.

radex commented 4 months ago

Can you be more specific? I understand you're frustrated, but please give more details under which conditions does an unexpected reset occur.

barakmichaely commented 4 months ago

Hi, didn't mean to sound frustrated :) I am not sure what triggered the unexpected reset but it could have been due to hot reloading, potentially react native reloading from bundle which had older schema version. However this is not the real problem, the real problem is that if schema version changes without a migration it deletes all user data. Do you see how this can be a problem? Mistakes happen, so it's a likely scenario that someone will forget to do a migration after updating schema, or maybe the migration code has an error, etc. and if the app is released like this by mistake all user data is wiped out.

It would be preferred, and in my opinion required, that you change this behavior to at least be optional, meaning that the data will never be deleted if this option is off. This behavior that deletes data due to lack of migration is a deal breaker for me in my project unfortunately. It expects the coders to always be very responsible, and in a project with multiple people that is not a good idea.

KrisLau commented 3 months ago

@radex I believe this issue is somewhat related to this one which I opened awhile back: https://github.com/Nozbe/WatermelonDB/issues/1609

Ideally it would be nice if we could have an event like onSchemaVersionChange where we could override the default behaviour of the migration or maybe have a migration step where I can put toVersion: * to handle all migrations and add reset and/or sync as a migration step.

For me, I'd personally like to be able to completely reset the database and re-sync when the schema version changes so that I dont have to deal with frontend migrations at all and only need to migrate the backend db.

GitMurf commented 2 months ago

Did this ever get resolved or addressed? I am just starting to test out watermelon and it seems like I hit similar behavior in testing and haven’t been able to figure out why (and not able to fully reproduce). But it feels like it may be related to this.

GitMurf commented 2 months ago

https://github.com/Nozbe/WatermelonDB/blob/a757e646141437ad9a06f7314ad5555a8a4d252e/src/adapters/sqlite/sqlite-node/Database.js#L83-L87

@KrisLau @barakmichaely I believe this is the cause. At face value, the code comments for this section itself seem like maybe there could be a better way than just "its simpler to delete everything". @radex I would argue / request that anytime Watermelon decides it should "delete everything", there should be an option for us to be able to create a backup of the database (maybe a hook for anything unsafeDestroyEverything() wants to run?). In a perfect world we would have a beforeDestroy() and afterDestroy() hook.

By destroying sqlite databases like this, we are 100% relying on the fact that our "backend" remote server that is being synced to is in a reliable state to restore whatever data may be being destroyed on the local device. Even in a perfect world, that still feels dangerous.

In my case I can't even build a new database using the node sqlite adapter because it creates a new database at version 0 and then tries to destroy this new database immediately since schema version 1 does not match database version 0, and on Windows it then throws an error because you are not able to delete the database. See @KrisLau issues / PRs related: #1705, #1706, #1707.

So I am stuck in an endless loop. I have not fully figured out a workaround yet but I think avoiding this unsafeDestroyEverything() is likely going to be the path for me (still debugging and testing).

@radex please let me know if I am mistaken at all but it seems like there are a few opportunities here for improvement. Thanks!