Skip to content
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

Reset behavior is dangerous and should not be default #1810

Open
barakmichaely opened this issue Jul 3, 2024 · 5 comments
Open

Reset behavior is dangerous and should not be default #1810

barakmichaely opened this issue Jul 3, 2024 · 5 comments

Comments

@barakmichaely
Copy link

barakmichaely commented Jul 3, 2024

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 radex changed the title ⚠️ Reset behavior is dangerous and should not be default ⚠️ Reset behavior is dangerous and should not be default Jul 4, 2024
@radex
Copy link
Collaborator

radex commented Jul 4, 2024

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

@barakmichaely
Copy link
Author

barakmichaely commented Jul 8, 2024

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
Copy link
Contributor

KrisLau commented Jul 21, 2024

@radex I believe this issue is somewhat related to this one which I opened awhile back: #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
Copy link

GitMurf commented Sep 14, 2024

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
Copy link

GitMurf commented Sep 14, 2024

unsafeDestroyEverything(): void {
// Deleting files by default because it seems simpler, more reliable
// And we have a weird problem with sqlite code 6 (database busy) in sync mode
// But sadly this won't work for in-memory (shared) databases, so in those cases,
// drop all tables, indexes, and reset user version to 0

@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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants