nowinandroid icon indicating copy to clipboard operation
nowinandroid copied to clipboard

Exception handling when reading datastore.

Open nur-shuvo opened this issue 1 year ago • 11 comments

What I have done and why

Datastore file can be corrupted in cases, or simply invalid. So there needs to handle runtime Fatal exception like I found:

FATAL EXCEPTION: main Process: com.google.samples.apps.nowinandroid.demo.debug, PID: 10565 androidx.datastore.core.CorruptionException: Cannot read proto. ...

So exception handling is needed, and replaced with UserPreferences.getDefaultInstance() when there occurs such exception

nur-shuvo avatar Nov 24 '24 15:11 nur-shuvo

It would be great to add tests for this new code. Also, using the Android Log here means we won't be able to run standard JVM unit test 😕

SimonMarquis avatar Dec 10 '24 06:12 SimonMarquis

It would be great to add tests for this new code. Also, using the Android Log here means we won't be able to run standard JVM unit test 😕

I will try to add tests, though it seems that corrupting the file in unit test environment is challenging, maybe I might have to write androidTest. I will look into this. Thank you for your review. @SimonMarquis

nur-shuvo avatar Dec 10 '24 08:12 nur-shuvo

I'm not sure exactly how for now but I think you can fake the datastore and throw the IOException from the loading method.

SimonMarquis avatar Dec 10 '24 08:12 SimonMarquis

I'm not sure exactly how for now but I think you can fake the datastore and throw the IOException from the loading method.

Added unit tests by making fake datastore which throws exception while accessing. Then testing data source whether returning the default values or not.

nur-shuvo avatar Dec 10 '24 15:12 nur-shuvo

@dturner Hello. Could you please review this PR at your convenient?

nur-shuvo avatar Dec 17 '24 06:12 nur-shuvo

If datastore isn't available, returning a default UserPreferences isn't the answer. The whole point of the app is that users can save the topics they're interested in and only see news relating to those topics (plus bookmarks etc). Without working data storage, none of that is possible. It's preferable to have the app crash on startup than have users be fooled into thinking it's working when it's not.

What am I missing?

dturner avatar Dec 19 '24 14:12 dturner

If datastore isn't available, returning a default UserPreferences isn't the answer. The whole point of the app is that users can save the topics they're interested in and only see news relating to those topics (plus bookmarks etc). Without working data storage, none of that is possible. It's preferable to have the app crash on startup than have users be fooled into thinking it's working when it's not.

What am I missing?

I think application will continuously crash on startUp if this exception unhandled. User may need to clear data forcefully, or uninstalled to recover from this unexpected situation. That's why I put the default UserPreferences and restored to something where user can fresh start using the app.

nur-shuvo avatar Dec 19 '24 15:12 nur-shuvo

@dturner it is not necessarily that "datastore" is not available, but rather that the datastore files are corrupted, and not recoverable. You can take a look at this report https://issuetracker.google.com/issues/346197747 that hit many of us, for unknown reasons. The only resort for users is to clear app data, or reinstall app, which would be equivalent to restoring an empty datastore file. We will lose the user data anyways.

SimonMarquis avatar Dec 19 '24 15:12 SimonMarquis

@dturner I would be happy to know your opinions regarding.

nur-shuvo avatar Dec 24 '24 09:12 nur-shuvo

Thanks for the additional info, that makes sense. I'll need some time to consider the suggested approach before providing a review. Unfortunately I'm on vacation for a couple of weeks now so it'll likely be mid Jan 2025 now.

dturner avatar Dec 24 '24 16:12 dturner

Hi @dturner This is a polite reminder about the review.

nur-shuvo avatar Feb 16 '25 05:02 nur-shuvo