Fix data store crash

[kmadsen]

Description

This crash was introduced in v2.9.0-alpha.3 by #6336

Steps to reproduce

1. Open qa-test-app
2. Launch Default NavigationView
3. go back
4. Launch Default NavigationVIew
5. 💥

Essentially, the owner of NavigationDataStoreOwner has always been a singleton so we never saw this issue. To ensure NavigationDataStoreOwner can be used by non-singleton entities. It makes sense for the DataStore lifecycle to be owned by NavigationDataStoreOwner, and it fixes the crash.

It is spelled out pretty clearly by the preferencesDataStore that this needs to have a lifecycle.

Creates a property delegate for a single process DataStore. This should only be called once in a file (at the top level), and all usages of the DataStore should use a reference the same Instance. The receiver type for the property delegate must be an instance of Context.

Here's the stack trace in case anyone comes looking for the issue

java.lang.IllegalStateException: There are multiple DataStores active for the same file: /data/user/0/com.mapbox.navigation.qa_test_app/files/datastore/mapbox_navigation_preferences.preferences_pb. You should either maintain your DataStore as a singleton or confirm that there is no two DataStore's active on the same file (by confirming that the scope is cancelled).
    at androidx.datastore.core.SingleProcessDataStore$file$2.invoke(SingleProcessDataStore.kt:168)
    at androidx.datastore.core.SingleProcessDataStore$file$2.invoke(SingleProcessDataStore.kt:163)
    at kotlin.SynchronizedLazyImpl.getValue(LazyJVM.kt:74)
    at androidx.datastore.core.SingleProcessDataStore.getFile(SingleProcessDataStore.kt:163)
    at androidx.datastore.core.SingleProcessDataStore.readData(SingleProcessDataStore.kt:380)
    at androidx.datastore.core.SingleProcessDataStore.readDataOrHandleCorruption(SingleProcessDataStore.kt:359)
    at androidx.datastore.core.SingleProcessDataStore.readAndInit(SingleProcessDataStore.kt:322)
    at androidx.datastore.core.SingleProcessDataStore.readAndInitOrPropagateFailure(SingleProcessDataStore.kt:311)
    at androidx.datastore.core.SingleProcessDataStore.handleRead(SingleProcessDataStore.kt:261)
    at androidx.datastore.core.SingleProcessDataStore.access$handleRead(SingleProcessDataStore.kt:76)
    at androidx.datastore.core.SingleProcessDataStore$actor$3.invokeSuspend(SingleProcessDataStore.kt:239)
    at androidx.datastore.core.SingleProcessDataStore$actor$3.invoke(Unknown Source:8)
    at androidx.datastore.core.SingleProcessDataStore$actor$3.invoke(Unknown Source:4)
    at androidx.datastore.core.SimpleActor$offer$2.invokeSuspend(SimpleActor.kt:122)
    at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
    at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:106)
    at kotlinx.coroutines.scheduling.CoroutineScheduler.runSafely(CoroutineScheduler.kt:571)
    at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.executeTask(CoroutineScheduler.kt:750)
    at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.runWorker(CoroutineScheduler.kt:678)
    at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.run(CoroutineScheduler.kt:665)

[dzinad]

Don't you want to test that it's the same for different instances of NavigationDataStoreOwner?

[kmadsen]

Don't you want to test that it's the same for different instances of NavigationDataStoreOwner?

Yeah I was looking into writing NavigationDataStoreOwnerTest but it's a little bit of a pain.

It's using the android data store delegate, so we would need a wrapper in order to test it. NavigationDataStoreOwner is that small wrapper. I don't think we would want to mock out the internals of PreferenceDataStoreSingletonDelegate because that would break whenever we update the dependency.

public fun preferencesDataStore(
    name: String,
    corruptionHandler: ReplaceFileCorruptionHandler<Preferences>? = null,
    produceMigrations: (Context) -> List<DataMigration<Preferences>> = { listOf() },
    scope: CoroutineScope = CoroutineScope(Dispatchers.IO + SupervisorJob())
): ReadOnlyProperty<Context, DataStore<Preferences>> {
    return PreferenceDataStoreSingletonDelegate(name, corruptionHandler, produceMigrations, scope)
}

This NavigationDataStoreOwner was initially designed to be a small class that wraps the delegate in order to "test the usage" rather than "test the integration" with the jetpack library https://github.com/mapbox/1tap-android/pull/1235/files#diff-e966f44af0c6177798d05d9005d006461a0c8738031204f3b6fad0a00d7015b7R10-R30

I think a separate integration test with jetpack would be good though..

[kmadsen]

@dzinad i added a "known issue" and it broke the changelog validator

[codecov]

Codecov Report

Merging #6392 (26e34fd) into main (420d326) will not change coverage.
The diff coverage is 20.00%.

@@            Coverage Diff            @@
##               main    #6392   +/-   ##
=========================================
  Coverage     68.95%   68.95%           
  Complexity     4579     4579           
=========================================
  Files           691      691           
  Lines         27384    27384           
  Branches       3191     3191           
=========================================
  Hits          18883    18883           
  Misses         7262     7262           
  Partials       1239     1239           

Impacted Files	Coverage Δ
...ils/internal/datastore/NavigationDataStoreOwner.kt	0.00% <0.00%> (ø)
...voice/internal/impl/MapboxAudioGuidanceServices.kt	12.50% <0.00%> (ø)
...box/navigation/ui/voice/api/MapboxAudioGuidance.kt	79.68% <100.00%> (ø)

[dzinad]

@dzinad i added a "known issue" and it broke the changelog validator

It's because you added a new section. It does not happen very often so I'm not sure if we should do something about it. It's more like a guide, this check can't check all possible cases. It doesn't work for updating the CHANGELOG for releases, for example. It's like a sanity check for regular PRs. What do you think? If you feel differently, I can add new sections cases to the checks.

[kmadsen]

It's because you added a new section. It does not happen very often so I'm not sure if we should do something about it

Agree it doesn't happen often so it's not that big of a deal. Leaving it up since you have the best idea for it