fix(android, utils): fix rare crash getting documents directory

[LittoCats]

NullPointerException Firebase Crashlytics

Description

The exported constant value for key KEY_DOCUMENT_DIRECTORY will be missed in ReactNativeFirebaseUtilsModule when Build.VERSION.SDK_INT >= Build.VERSION_CODES.KITKAT and externalDirectory == null

Related issues

Fixes #4706

Release Summary

Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • Yes
• My change supports the following platforms;
  • Android
  • iOS
• My change includes tests;
  • e2e tests added or updated in packages/\*\*/e2e
  • jest tests added or updated in packages/\*\*/__tests__
• I have updated TypeScript types that are affected by my change.
• This is a breaking change;
  • Yes
  • No

Test Plan

---

Think react-native-firebase is great? Please consider supporting the project with any of the below:

• 👉 Star this repo on GitHub ⭐️
• 👉 Follow React Native Firebase and Invertase on Twitter

[vercel]

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/invertase/react-native-firebase/GuHm9FZ3bJhW76Q9L8k7QqNzb9pb
✅ Preview: https://react-native-firebase-git-fork-littocats-crash-fix-invertase.vercel.app

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ mikehardy
❌ LittoCats
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[codecov]

Codecov Report

Merging #5118 (ea8e36f) into master (d2838ff) will not change coverage.
The diff coverage is n/a.

❗ Current head ea8e36f differs from pull request most recent head 1146880. Consider uploading reports for the commit 1146880 to get more accurate results

@@           Coverage Diff           @@
##           master    #5118   +/-   ##
=======================================
  Coverage   88.99%   88.99%           
=======================================
  Files         109      109           
  Lines        3739     3739           
  Branches      358      358           
=======================================
  Hits         3327     3327           
  Misses        367      367           
  Partials       45       45           

[mikehardy]

Hi @LittoCats ! I think this was already addressed in #4990 - when I look at the original diagnosis you did it seems correct but it's based on an old version of the code. In v11.1.0+ It already has a null check and so will not crash here

[LittoCats]

Yes, I notice that last night, but I found, the constant value exported to js will be not exists for KEY_DOCUMENT_DIRECTORY when Build.VERSION.SDK_INT >= Build.VERSION_CODES.KITKAT and externalDirectory == null

    File externalDirectory = context.getExternalFilesDir(null);
    if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.KITKAT) {
      if ( externalDirectory != null ) {
        constants.put(KEY_DOCUMENT_DIRECTORY, externalDirectory.getAbsolutePath());
      }
    } else {
      constants.put(KEY_DOCUMENT_DIRECTORY, context.getFilesDir().getAbsolutePath());
    }

[mikehardy]

That's true and I thought of that as I compared the two types of fix for the issue. Your fix will cause the directory to change between runs though, so the value will exist in your version but it won't be stable as external media is changed.

I think that may just be an unsolvable problem though - maybe something just to put in the docs here vs attempt to solve in code

The doc is here

react-native-firebase/packages/app/lib/index.d.ts

Line 285 in c94546d

* Use this directory to place documents that have been created by the user.

and if there was just a note that there is a small chance if a user uses removable external storage on Android and that storage configuration changes or is not available, the path returned could change, then this seems fine.

[LittoCats]

Thanks for your explaination.

[mikehardy]

Wait! @LittoCats I actually like this PR, I think the code itself is both correct and necessary to correct the null pointer exception - I am sorry I wasn't clear, what I think it needs is just an addition to the documentation as well as the code fix, to warn developers that this is an external directory which could change (making it appear as if documents appear or disappear) if the users external storage environment changes

Hopefully that's more clear :-). I actually really want to get this fix in, you have solved a real crash!

[mikehardy]

@LittoCats I had time today to look at this - I pulled your branch, fixed the compile error and then altered the documentation to explain the conditions when documents directory could be internal vs external.

I think this should work and fix the crash - can you take a look? I can merge it and release it if it looks good