fix: users logged out after SDK upgrade due to different cache path

[rommansabbir]

ParsePlugins ::

• getCacheDir(), getFilesDir() APIs now has support for backward compatibility before migration to v3.0.0

[parse-github-assistant[bot]]

Thanks for opening this pull request!

• ❌ Please edit your post and use the provided template when creating a new pull request. This helps everyone to understand your post better and asks for essential information to quicker review the pull request.

[codecov[bot]]

Codecov Report

Merging #1168 (4e2c22c) into master (4842329) will increase coverage by 0.50%. The diff coverage is 90.00%.

@@             Coverage Diff              @@
##             master    #1168      +/-   ##
============================================
+ Coverage     66.81%   67.31%   +0.50%     
- Complexity     2249     2285      +36     
============================================
  Files           121      122       +1     
  Lines          9892     9962      +70     
  Branches       1332     1343      +11     
============================================
+ Hits           6609     6706      +97     
+ Misses         2771     2735      -36     
- Partials        512      521       +9     

Impacted Files	Coverage Δ
parse/src/main/java/com/parse/ParseFileUtils.java	45.78% <75.00%> (+9.07%)	:arrow_up:
...in/java/com/parse/ParseCacheDirMigrationUtils.java	91.80% <91.80%> (ø)
parse/src/main/java/com/parse/Parse.java	65.58% <100.00%> (+3.89%)	:arrow_up:
...rc/main/java/com/parse/ParseRESTObjectCommand.java	73.07% <0.00%> (-11.54%)	:arrow_down:
parse/src/main/java/com/parse/ParseRequest.java	82.00% <0.00%> (+2.00%)	:arrow_up:
parse/src/main/java/com/parse/ParsePlugins.java	50.00% <0.00%> (+18.75%)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 4842329...4e2c22c. Read the comment docs.

[azlekov]

Here you can see an migration logic about the push notifications, I have expected to see something similar but much more simple: https://github.com/parse-community/Parse-SDK-Android/commit/7d0faa38bf3ccc85b715dc61965d5b82103c19a4#diff-6cb14853da236c30db3f14e4005ea40c5c45cb8fcb9fc49541693186620de1dcR60

[mman]

@rommansabbir Thanks for the PR, but looking at the proposed fix I think it is not gonna work at all.

There are potentially multiple cached files stored in both, the cache dir and the files dir. Your code will probably allow to read them from the original location, and then you delete them when the app exists (assuming the delete on exit works, as it might not as @L3K0V explains).

The actually files we care about are never moved anywhere, and even if they will have been be re-saved by the app at some point, they would end up in the old location and then then get deleted.

Needs more work IMHO...

[mtrezza]

Hi everyone, just to emphasize it, this is a particularly important PR, because any release without this migration logic is for most people likely a useless release. So this PR currently blocks the release pipeline. The sooner we get this merged the better, considering we have another issue with Facebook Login that is waiting to get merged. Any helping hand here is greatly appreciated and we have a bounty on this issue as well.

[rommansabbir]

@mtrezza @mman @L3K0V First of all, sorry for messed up with the PR. This is my first contribution to a such big community of Parse.

About my logic for cache path migrations. Based on @mman migration thoughts (https://github.com/parse-community/Parse-SDK-Android/issues/1158#issuecomment-1110097992)

I came up with this ::

• First, looking for the all files (including sub directory) in the old "app_Parse" directory
• If files exists, moving the all files to the directory "com.parse" by following the old sub directory name. After that delete the old file (already copied) silently.
• For each file, migration will be failed if there is already a file exist in the new directory with the same file name or other known cases.

If the solution seems suitable for migration process, I will create a new PR (which will include changes in ParsePlugins.java and ParseFileUtils.java).

Let me know, Thanks.

[mman]

@rommansabbir Yes, this is the approach I would take, but please note that since 3.0.0 is already in the wild, and since it forced people to re-login, there will be cases where there will be fresh files in new location com.parse as well as files in old location, the app_Parse directory. In that case, the new files should be used, and old just deleted...

[rommansabbir]

One things more, in the old version, I have seen that ParsePlugins.getParseDir() API return file for app_Parse and it's deprecated. But in new APIs ParsePlugins.getCacheDir() and ParsePlugins.getFilesDir() points to (eg.) */cache/com.parse and */files/com.parse, two different path.

So, when running migration, for a specific file, in which location I should point to move the spcific file, */cache/com.parse or */files/com.parse?.

[mman]

@rommansabbir That is exactly what I have not yet had a time to investigate properly and in detail (read: This PR is actually trickier then it may appear on a first look).

[rommansabbir]

@mman I think we can find out from where the old API is accessed and which FILE_NAME (File name) was used, eg. File(ParsePlugins.get().getParseDir(), FILENAME_CURRENT_CONFIG). We can look for others Constants like FILENAME_CURRENT_CONFIG. So, when migration is ongoing, we can check for item name matches with our Constants list or not, if match - then move item to */files/com.parse/ else */cache/com.parse/

[mtrezza]

So, when running migration, for a specific file, in which location I should point to move the spcific file, /cache/com.parse or /files/com.parse?.

The two storages have a different purpose and are managed differently. That determines which file should go where.

• The cache dir is authoritatively managed by the OS and only co-managed by the app. It must not be considered persistent, in other words, the data the app stores there can be deleted by the OS at any time. That also means that if the app doesn't properly delete files the OS will make sure the cache doesn't swell up too much.
• The files dir is exclusively managed by the app. It is persistent and must be managed by the app. If the app doesn't delete outdated files there the files will stay and the app's footprint will swell up until the OS will report a storage issue to the user.

For example, the user session token would go into persistent storage.

I would just make a list of all the SDK's internal files from looking at the pre-3.x logic, post the list here and then we can determine whether it should be persistent or not. That wrapped into the logic I proposed earlier should do the trick. I don't think there is a lot more to it.

[rommansabbir]

ParsePlugins::installationId(){File(getParseDir(), INSTALLATION_ID_LOCATION)}
ParseCorePlugins::getCurrentUserController() {File(Parse.getParseDir(), FILENAME_CURRENT_USER)}
ParseCorePlugins::getConfigController() {File(ParsePlugins.get().getParseDir(), FILENAME_CURRENT_CONFIG)}
ParseCorePlugins::getCurrentInstallationController() {File(ParsePlugins.get().getParseDir(), FILENAME_CURRENT_INSTALLATION)}
ParseCommandCache::getCacheDir(){File(Parse.getParseDir(), "CommandCache")}
LocalIdManager(Parse.getParseDir()){File(root, "LocalId")}
PushRouter::getInstance(){File(ParsePlugins.get().getParseDir(), LEGACY_STATE_LOCATION)}

from where ParsePlugins.get().getParseDir() API has been accessed. All of those files managed by SDK itself, so when migration is ongoing, we can look for those listed files and move files to */files/com.parse/ directory, then delete the old files from app_Parse by covering known cases.

After that, all other files that left in app_Parse directory should move to */cache/com.parse/ directory by covering known cases.

• The files dir is exclusively managed by the app. It is persistent and must be managed by the app. If the app doesn't delete outdated files there the files will stay and the app's footprint will swell up until the OS will report a storage issue to the user.

Version: 2.0.6

ParseCorePlugins::getFileController(){
             ParseFileController(
                    ParsePlugins.get().restClient(), 
                    Parse.getParseCacheDir("files")
            ))
}

calling Parse.getParseCacheDir("files") API which point to */cache/com.parse/. In this case, all files (eg. when storing an ParseObject to LocalDataStore that contain an File itself call something.mp3) that are stored in the cache directory should point to */files/com.parse/ as something.mp3 is managed by SDK itself.

[mtrezza]

All of those files managed by SDK itself, so when migration is ongoing, we can look for those listed files and move files to */files/com.parse/ directory, then delete the old files from app_Parse by covering known cases.

Sounds good; how about the other modules like facebook, fcm, it is possible that they store anything in there as well?

After that, all other files that left in app_Parse directory should move to */cache/com.parse/ directory by covering known cases.

What are these other files?

Reopening this PR, as the issue conversation is happening here instead of in the issue. I guess this PR can just be extended with the modifications discussed above, instead of opening a new one.

[rommansabbir]

@mtrezza I found those usages in the mentioned list. If any other module deal with File, at the end they all point to the same API ParsePlugins.get().getParseDir() to retreive File object.

What are these other files?

I'm not sure about how many files (File) is accessed (CRUD) to old dir (app_Parse) by the SDK itself. So,

• Files that matches with our lists, will be moved to */files/com.parse/ but what about others files in the old dir? (If exists)
• We can assume those files should be moved to */cache/com.parse/ dir.

[mtrezza]

In general I agree with your strategy, but I wonder what these other files could be?

I assume it's only the Parse SDK itself that writes to that dir, so any write logic, even for temporary files should be identifiable in code. Have you seen that there are also other files, or is that just an assumption? Or is it that the OS uses the cache dir of a module automatically to store cache files that are opaque to us?

[rommansabbir]

According to my exploration of the code, many controller and classes using new locations ( */files/com.parse/,*/cache/com.parse/), but some classes still accessing the old location app_Parse.

Regarding the migrations: I have checked the migration with mentioned files and some random data.

I'm not sure about how many files (File) is accessed (CRUD) to old dir (app_Parse) by the SDK itself. So,

* Files that matches with our lists, will be moved to `*/files/com.parse/` but what about others files in the old dir? (If exists)

* We can assume those files should be moved to `*/cache/com.parse/` dir.

[mtrezza]

OK, so I guess let's go with your suggested migration concept and then just test it out.

but some classes still accessing the old location app_Parse.

You mean pre 3.x or even with the latest version of the SDK?

[rommansabbir]

You mean pre 3.x or even with the latest version of the SDK?

pre 3.x since, from 3.x API removed from codebase and using */files/com.parse/ and */cache/com.parse/.

Also, Can I write the test in Kotlin?

[mtrezza]

Got it. Not sure about Kotlin, we probably want to keep all tests in a consistent language, what do you think @L3K0V?

[rommansabbir]

Test Cases has been written in Java. I tried to push code to the remote branch patch-1 but return fatal: unable to access : 403. Please, help me with this.

[azlekov]

Tests should be in Java for now.

[mtrezza]

@rommansabbir you get a 403 error pushing to the branch in your own fork? If so, you need to authenticate yourself for GitHub to accept a push.

[rommansabbir]

you get a 403 error pushing to the branch in your own fork? If so, you need to authenticate yourself for GitHub to accept a push.

I think, Git was trying push to the remote branch as well as my forked branch. But yeah, code is psuhed to my fork's branch.

I have added test for the migration process. Still, one more thing left, calling new overloaded APIs ParsePlugins.get().getCacheDir(true) and ParsePlugins.get().getFilesDir(true) should be used instead of old APIs in the respective classes.

[mtrezza]

Amazing!

calling new overloaded APIs

Is this an obvious change or do you need any guidance?

[rommansabbir]

Thanks, I will definitely ask you for further guidance. I think, yes. Changes is obvious for silent migration (Although it will be called everytimes, when cache or files API is called). Or how do we do it?. Can review the changes? So that, we can close this ASAP.

[mtrezza]

it will be called everytimes, when cache or files API is called

I would call the migration routine only once on SDK initialization. I don't think we need to set a flag that the migration has happened - checking whether the old path exists should be enough. Maybe a flag is not even a safe way, because in a scenario where the app is restored from a backup that logic may get messed up. So I would always run the migration on SDK init.

I think we can keep the migration routine for ~1 or 2 years, then remove it as a breaking change and note in the changelog that an app may first have to migrate using a previous version where the routine is present.

Can review the changes?

I just ran the CI to see whether the tests pass. When you say it's ready maybe @L3K0V could do a review.

So that, we can close this ASAP.

That would be wonderful!

[mtrezza]

@PLR2388 Since you originally reported the issue, do you think you could test out this fix to see whether the migration happens successfully?

Would be nice to have a pre-release branch to test things out easier, to consider for the future.

[rommansabbir]

@mman @mtrezza Any update on this PR?

[mtrezza]

Given the severity of the issue, I would like to have someone test this out with an actual app before we merge it and recommend to developers that the release is safe to use. That means:

1. use the 2.x SDK
2. log in a user
3. update the SDK using this PR, relaunch the app, confirm at least whether:

• user is still logged in (verify on server side by making a request to the server)
• installation is set correctly (verify on server side by making a request to the server)
• data is transferred, old dir deleted, non-essential files in cache dir, etc (verify via device file explorer)

If @PLR2388, @qmetzler-luna or @mman could try that out, that would be fantastic.

[rommansabbir]

If @PLR2388, @qmetzler-luna or @mman could try that out, that would be fantastic.

@mtrezza @mman I have tested the "patch-1" version to check the migrations status & users logged out issue. Migrations was successful & users stayed logged in after SDK version updates from 2.x to patch-1.

Step 1:

• Install Parse SDK v2.0.5
• Logged In as a user (Fresh Login)
• Check app data folder on Device Explorer

Step 2:

• Install Parse SDK from branch patch-1
• Check logged in user status
• Fetch current user again from server
• Save an ParseObject to the server
• Check app data folder on Device Explorer

The result is as we expected 😃

[mtrezza]

@rommansabbir Thanks for documenting it in such detail. I will try this out as well just to confirm before we merge.

Side note, since you already did a lot of investigation, do you know why the applicationId file is stored in cache and what the file is used for? The ID is set during SDK initialization and probably stored in memory as it's always required for SDK requests. So why is it written to a cache file that may be deleted by the OS anytime anyway.

[rommansabbir]

@mtrezza I will review the codes and will update you.

[mman]

@rommansabbir I will try to test until the end of this week...

[rommansabbir]

@mtrezza According to the code comments:

• Make sure the data on disk for Parse is for the current application on Parse SDK Init.
• Verifies that the data stored on disk for Parse was generated using the same application that is running now
• Make sure the current version of the cache is for this application id else the applicationId file was malformed or something
• The application id has changed, so everything on disk is invalid.

[mtrezza]

Thanks for investigating, so the applicationId there is essentially to validate that the cache belongs to the app. So if an app would allow to switch between different application IDs, the cache would get invalidated with every switch. That makes sense.

[mtrezza]

I can confirm that the migration works fine.

It moves from com.example.app/app_Parse/ to com.example.app/files/com.parse/:

• currentConfig
• currentInstallation
• currentUser
• installationId

It moves from com.example.app/app_Parse/ to com.example.app/cache/com.parse/:

• CommandCache/

The following file locations are unchanged:

• com.example.app/cache/com.parse/applicationId
• com.example.app/files/com.parse/push

Everything as expected, installation update and user requests server side also work fine.

Great job, @rommansabbir! Would you be interested in joining the Parse Android SDK team? No commitment in terms of contribution, but it would be great if you could at times help out with things like PR reviews for example.

[mtrezza]

Changelog entry:

Fix

• users logged out after SDK upgrade due to different cache path; this fixes the bug that was introduced with release 3.0.0 which ignores SDK-internal data that is stored locally on the client side (#1168)

[mtrezza]

@mman If you want to test that would be great, then we'll wait until the end of the week with the release.

[rommansabbir]

Great job, @rommansabbir! Would you be interested in joining the Parse Android SDK team? No commitment in terms of contribution, but it would be great if you could at times help out with things like PR reviews for example.

Yes, I would love to 😃

[mman]

@rommansabbir @mtrezza I'd love to double check, the code looks good. What's the easiest way to build my app against this branch. I have tried naive approach to depend directly on git branch, but that apparently does not work. What steps are needed? Sorry for my lame question.

[rommansabbir]

What's the easiest way to build my app against this branch. I have tried naive approach to depend directly on git branch, but that apparently does not work. What steps are needed?

@mman

Add this branch dependency:

implementation 'com.github.rommansabbir:Parse-SDK-Android:patch-1-SNAPSHOT'

I was able to install by following this approach. Now, you can install the latest SDK from this branch.

[mtrezza]

You can also clone the repo, checkout the PR branch, build to local maven and reference the builds in your example app. I think that's a question many may have; maybe we should add a step-by-step instruction to the contribution guide.

[mman]

Thanks @rommansabbir, I was missing the -SNAPSHOT in the github reference.

implementation 'com.github.rommansabbir:Parse-SDK-Android:patch-1-SNAPSHOT'

I have now tried our apps and looks like this PR does correctly move all files around.

Thanks, LGTM 👍

[mtrezza]

Thanks confirming @mman. We now have 3 confirmations that this PR works, so I will go ahead and merge.

Great teamwork everyone and thanks especially to @rommansabbir for kicking it off with this PR!

[parseplatformorg]

🎉 This change has been released in version 3.0.1