Use Room for filelist table reads

[starypatyk]

This is an attempt to improve performance of retrieval of information from the filelist table of the local SQLite database by using Room infrastructure prepared by @AlvaroBrey in #10977.

As the first step I have converted the getFolderContent method only.

Preliminary timings from a low end phone (on a folder with ~330 files) suggest reduction of the execution time of this method from ~400 ms to ~200 ms.

I have tried to follow the pattern proposed by @AlvaroBrey - any comments are welcome.

I think that the PR might be merged once all the methods in FileDataStorageManager that reference the filelist table are converted to Room. Any suggestions otherwise?

EDIT

I have noticed a significant increase of issues reported by SpotBugs. This seems to be caused by the fact that the old code somehow automatically converted NULL values in integer columns to zero. Although I am not sure about this behaviour - see: https://stackoverflow.com/a/43702322

I have implemented wrapper functions nullToZero to mitigate this, but I am not sure if this is a correct approach.

I have also noticed that the file/directory modification dates are incorrect. Seems that the timestamp columns must be declared as Long rather than Int. I have changed the FileEntity class accordingly - for all columns that looked like a timestamp.

EDIT 2

I have also converted the getFileById method.

[codecov]

Codecov Report

Merging #11098 (12b4977) into master (23d0b40) will increase coverage by 27.83%.
The diff coverage is 81.48%.

❗ Current head 12b4977 differs from pull request most recent head 6b5cdb0. Consider uploading reports for the commit 6b5cdb0 to get more accurate results

Additional details and impacted files

@@              Coverage Diff              @@
##             master   #11098       +/-   ##
=============================================
+ Coverage      3.18%   31.02%   +27.83%     
- Complexity      420     3319     +2899     
=============================================
  Files           564      564               
  Lines         41616    41550       -66     
  Branches       5657     5628       -29     
=============================================
+ Hits           1327    12891    +11564     
+ Misses        40203    26739    -13464     
- Partials         86     1920     +1834     

Impacted Files	Coverage Δ
...va/com/nextcloud/client/database/DatabaseModule.kt	80.00% <0.00%> (+80.00%)	⬆️
...com/nextcloud/client/database/NextcloudDatabase.kt	100.00% <ø> (+100.00%)	⬆️
...loud/android/datamodel/FileDataStorageManager.java	61.94% <80.80%> (+61.86%)	⬆️
...com/nextcloud/client/database/entity/FileEntity.kt	96.59% <100.00%> (+96.59%)	⬆️
.../java/com/owncloud/android/datamodel/GalleryRow.kt	50.00% <0.00%> (ø)
...java/com/nextcloud/client/di/ComponentsModule.java	0.00% <0.00%> (ø)
.../nextcloud/client/widget/DashboardWidgetService.kt	0.00% <0.00%> (ø)
...cloud/android/ui/preview/PreviewMediaFragment.java	0.00% <0.00%> (ø)
...wncloud/android/ui/preview/PreviewVideoActivity.kt
...android/ui/preview/PreviewVideoFullscreenDialog.kt	0.00% <0.00%> (ø)
... and 290 more

[github-actions]

blue-Light-IT test failed: https://www.kaminsky.me/nc-dev/android-integrationTests/11098-Screenshot-blue-Light-21-33

[AlvaroBrey]

This is great 💙 ! Thank you very much for getting this started.

The approach generally looks good; the only thing I dislike is the duplication of createFileInstance (and later createContentValues* if you tackle write methods), but it can't be undone until everything is ported over to Room.

I think that the PR might be merged once all the methods in FileDataStorageManager that reference the filelist table are converted to Room. Any suggestions otherwise?

Yes, this makes sense. I wouldn't merge it before because of the aforementioned duplication. There is a lot of outstanding technical debt in FileDataStorageManager (like, why does this class also handle capabilities and shares? and media scans!?), but that is for another moment.

However, migrating all of this may not be so easy. If you need any guidance please reach out in our community chat, and I can contribute to this PR as well when I have the time, if you find that OK.

I have implemented wrapper functions nullToZero to mitigate this, but I am not sure if this is a correct approach.

This looks fine in principle. At a later stage, the fields that are actually booleans may be converted to Boolean in the entity and that should make the code much cleaner too. (this is again, out of scope for this PR)

I have also noticed that the file/directory modification dates are incorrect. Seems that the timestamp columns must be declared as Long rather than Int. I have changed the FileEntity class accordingly - for all columns that looked like a timestamp.

This was an oversight on my part, thanks for noticing. I've looked through the old createFileInstance to see which fields were read as Long before, and found that parent and content_length also need to be changed. Pushed a commit for that.

In general, this is the most critical migration to Room, which is why I didn't start directly with it. Extensive testing is needed to verify everything works as expected with existing databases as well as new ones. Please do your best to test everything you can, but I'll also do a lot of smoketesting before merging, when the PR is ready.

Again, thank you very much for attempting this difficult task, and reach out for guidance if needed!

[starypatyk]

@AlvaroBrey @tobiasKaminsky

When I submitted this PR I thought that it would be good to merge this PR when all database operations related to the filelist table are migrated to Room.

I have underestimated the number of changes required to achieve this. 😞 I would now suggest to merge it in (more or less) current state.

I have managed to migrate most of the query operations, just enough to remove the old createFileInstance(cursor) method. So there is no need to maintain two very similar methods.

I have been able to fix most of the issues reported by various CI jobs - with two exceptions:

• In the CodeQL job I have not been able to find information about the reason of the failure.
• The continuous-integration/drone/pr reports a failure, but when I look at the report everything looks fine. I do not understand what happened. 😮

I have tried to make sure that the new methods defined on the FileDao interface maintain semantics of the old database calls, but would be good, if you could review them carefully.

I have explicitly tested behaviour of the getFolderContent, getGalleryItems and moveLocalFile methods. Regarding other changed methods - I have not been able to find relevant scenarios easily, but I have not noticed any obvious regressions in the app behaviour.

Please suggest next steps.

[tobiasKaminsky]

• In the CodeQL job I have not been able to find information about the reason of the failure.

CodeQL has currently some unknown troubles, also in other PRs.

• The continuous-integration/drone/pr reports a failure, but when I look at the report everything looks fine. I do not understand what happened. open_mouth

That is indeed strange. It seems that all tests went fine, but then it still failed:

> Task :app:connectedGplayDebugAndroidTest
--


> Task :app:connectedGplayDebugAndroidTest FAILED

I will restart it.

[starypatyk]

I will restart it.

Thanks @tobiasKaminsky! I see that the last tests completed OK. 👍

[AlvaroBrey]

I have underestimated the number of changes required to achieve this. disappointed I would now suggest to merge it in (more or less) current state.

I have managed to migrate most of the query operations, just enough to remove the old createFileInstance(cursor) method. So there is no need to maintain two very similar methods.

This is fine, thank you very much for your contribution :) It's great progress already.

I've reviewed the code and have a few minor suggestions I am attaching here. I still have to do some smoketesting before merging

[AlvaroBrey]

Smoketesting was successful! After pending comments are addressed and the branch is rebased, it'll be OK to merge from me. Thanks @starypatyk !

[github-actions]

Codacy

Lint

Type	master	PR
Warnings	82	82
Errors	0	0

SpotBugs

Category	Base	New
Bad practice	27	27
Correctness	45	44
Dodgy code	335	335
Internationalization	9	9
Multithreaded correctness	9	9
Performance	58	58
Security	18	18
Total	501	500

[github-actions]

APK file: https://www.kaminsky.me/nc-dev/android-artifacts/11098.apk



To test this change/fix you can simply download above APK file and install and test it in parallel to your existing Nextcloud app.

[starypatyk]

Smoketesting was successful! After pending comments are addressed and the branch is rebased, it'll be OK to merge from me. Thanks @starypatyk !

Great! 😄

@AlvaroBrey - I have removed TODOs in FileDao (the last unresolved comment) and rebased.

[AlvaroBrey]

Ready to merge from my side! The CI failures are unrelated.

@tobiasKaminsky pinging in case you want a last look before merging

[tobiasKaminsky]

I am fine with merging.
I just restarted Drone.

@starypatyk many thanks for this huge PR!

[nextcloud-android-bot]

master-IT test failed: https://www.kaminsky.me/nc-dev/android-integrationTests/7132-IT-master-09-14

[AlvaroBrey]

Thank you very much @starypatyk !