For #12711 - Use a separate reader for autocomplete suggestions

Decouple this functionality from search suggestions allowing for individual
control over the readers used.

(cherry picked from commit 2026625)

# Conflicts resolved:
#	docs/changelog.md

components/browser/storage-sync/src/main/java/mozilla/components/browser/storage/sync/Connection.kt

@@ -29,7 +29,26 @@ const val DB_NAME = "places.sqlite"
 internal interface Connection : Closeable {
     fun registerWithSyncManager()
 
+    /**
+     * Allows to read history, bookmarks, and other data from persistent storage.
+     * All calls on the same reader are queued. Multiple readers are allowed.
+     */
     fun reader(): PlacesReaderConnection
+
+    /**
+     * Create a new reader for history, bookmarks and other data from persistent storage.
+     * Allows for disbanded calls from the default reader from [reader] and so being able to
+     * easily start and cancel any data requests without impacting others using another reader.
+     *
+     * All [PlacesApi] requests are synchronized at lower levels so even with using multiple readers
+     * all requests are ordered with concurrent reads not possible.
+     */
+    fun newReader(): PlacesReaderConnection
+
+    /**
+     * Allowed to add history, bookmarks and other data to persistent storage.
+     * All calls are queued and synchronized at lower levels. Only one writer is recommended.
+     */
     fun writer(): PlacesWriterConnection
 
     // Until we get a real SyncManager in application-services libraries, we'll have to live with this
@@ -87,6 +106,12 @@ internal object RustPlacesConnection : Connection {
         return cachedReader!!
     }
 
+    override fun newReader(): PlacesReaderConnection = synchronized(this) {
+        val api = safeGetApi()
+        check(api != null) { "must call init first" }
+        return api.openReader()
+    }
+
     override fun writer(): PlacesWriterConnection {
         val api = safeGetApi()
         check(api != null) { "must call init first" }

components/browser/storage-sync/src/main/java/mozilla/components/browser/storage/sync/PlacesHistoryStorage.kt

@@ -6,8 +6,10 @@ package mozilla.components.browser.storage.sync
 
 import android.content.Context
 import android.net.Uri
+import androidx.annotation.VisibleForTesting
 import kotlinx.coroutines.withContext
 import mozilla.appservices.places.PlacesApi
+import mozilla.appservices.places.PlacesReaderConnection
 import mozilla.appservices.places.uniffi.PlacesException
 import mozilla.appservices.places.uniffi.VisitObservation
 import mozilla.components.concept.base.crash.CrashReporting
@@ -44,6 +46,12 @@ open class PlacesHistoryStorage(
     context: Context,
     crashReporter: CrashReporting? = null
 ) : PlacesStorage(context, crashReporter), HistoryStorage, HistoryMetadataStorage, SyncableStore {
+    /**
+     * Separate reader used only for autocomplete suggestions allowing to decouple this functionality
+     * from the history suggestions feature and independent reader management.
+     */
+    @VisibleForTesting
+    internal val autocompleteReader: PlacesReaderConnection by lazy { places.newReader() }
 
     override val logger = Logger("PlacesHistoryStorage")
 
@@ -151,7 +159,8 @@ open class PlacesHistoryStorage(
 
     override fun getAutocompleteSuggestion(query: String): HistoryAutocompleteResult? {
         val url = handlePlacesExceptions("getAutoCompleteSuggestions", default = null) {
-            places.reader().matchUrl(query)
+            autocompleteReader.interrupt()
+            autocompleteReader.matchUrl(query)
         } ?: return null
 
         val resultText = segmentAwareDomainMatch(query, arrayListOf(url))

components/browser/storage-sync/src/test/java/mozilla/components/browser/storage/sync/PlacesHistoryStorageTest.kt

@@ -31,6 +31,7 @@ import org.json.JSONObject
 import org.junit.After
 import org.junit.Assert.assertEquals
 import org.junit.Assert.assertFalse
+import org.junit.Assert.assertNotEquals
 import org.junit.Assert.assertNotNull
 import org.junit.Assert.assertNull
 import org.junit.Assert.assertTrue
@@ -39,6 +40,7 @@ import org.junit.Before
 import org.junit.Rule
 import org.junit.Test
 import org.junit.runner.RunWith
+import org.mockito.Mockito.doReturn
 import org.mockito.Mockito.never
 import org.mockito.Mockito.verify
 import java.io.File
@@ -456,6 +458,20 @@ class PlacesHistoryStorageTest {
         assertNull(history.getAutocompleteSuggestion("hello"))
     }
 
+    @Test
+    fun `store uses a different reader for autocomplete suggestions`() {
+        val connection: RustPlacesConnection = mock()
+        doReturn(mock<PlacesReaderConnection>()).`when`(connection).reader()
+        doReturn(mock<PlacesReaderConnection>()).`when`(connection).newReader()
+        val storage = MockingPlacesHistoryStorage(connection)
+
+        storage.getAutocompleteSuggestion("Test")
+
+        assertNotEquals(storage.reader, storage.autocompleteReader)
+        verify(storage.autocompleteReader).interrupt()
+        verify(storage.autocompleteReader).matchUrl("Test")
+    }
+
     @Test
     fun `store ignores url parse exceptions during record operations`() = runTestOnMain {
         // These aren't valid URIs, and if we're not explicitly ignoring exceptions from the underlying
@@ -602,6 +618,11 @@ class PlacesHistoryStorageTest {
                 return mock()
             }
 
+            override fun newReader(): PlacesReaderConnection {
+                fail()
+                return mock()
+            }
+
             override fun writer(): PlacesWriterConnection {
                 fail()
                 return mock()
@@ -658,6 +679,11 @@ class PlacesHistoryStorageTest {
                 return mock()
             }
 
+            override fun newReader(): PlacesReaderConnection {
+                fail()
+                return mock()
+            }
+
             override fun writer(): PlacesWriterConnection {
                 fail()
                 return mock()
@@ -706,6 +732,11 @@ class PlacesHistoryStorageTest {
                 return mock()
             }
 
+            override fun newReader(): PlacesReaderConnection {
+                fail()
+                return mock()
+            }
+
             override fun writer(): PlacesWriterConnection {
                 fail()
                 return mock()
@@ -761,6 +792,11 @@ class PlacesHistoryStorageTest {
                 return mock()
             }
 
+            override fun newReader(): PlacesReaderConnection {
+                fail()
+                return mock()
+            }
+
             override fun writer(): PlacesWriterConnection {
                 fail()
                 return mock()

docs/changelog.md

@@ -11,6 +11,9 @@ permalink: /changelog/
 * [Gecko](https://github.com/mozilla-mobile/android-components/blob/main/buildSrc/src/main/java/Gecko.kt)
 * [Configuration](https://github.com/mozilla-mobile/android-components/blob/main/.config.yml)
 
+* **browser-storage-sync**:
+  * 🚒 Bug fixed [issue #12689](https://github.com/mozilla-mobile/android-components/issues/12689) Decouple autocomplete suggestions from history search suggestions by using a separate reader which allows for separate management.
+
 * **browser-storage-sync**:
   * Stop reporting to the crash servers the expected `OperationInterrupted` exceptions for when interrupting in progress reads/writes from Application-Services. [#12557](https://github.com/mozilla-mobile/android-components/issues/12557), [#12569](https://github.com/mozilla-mobile/android-components/issues/12569).