Merge pull request #6825 from streetcomplete/new-update-on-conflict-logic

fixes #6779, fixes #6419, fixes #6771, supercedes #5073

Author: westnordost

app/src/androidUnitTest/kotlin/de/westnordost/streetcomplete/data/osm/mapdata/MapDataControllerTest.kt

@@ -127,33 +127,17 @@ class MapDataControllerTest {
     }
 
     @Test fun deleteOlderThan() {
-        val nodeKeys = listOf(
+        val elementKeys = listOf(
             ElementKey(NODE, 1L),
             ElementKey(NODE, 2L),
-            ElementKey(NODE, 3L),
-        )
-        val filteredNodeKeys = listOf(
-            ElementKey(NODE, 1L),
-            ElementKey(NODE, 3L),
-        )
-        val wayKeys = listOf(
-            ElementKey(ElementType.WAY, 1L),
-        )
-        val relationKeys = listOf(
-            ElementKey(ElementType.RELATION, 1L),
         )
-        val elementKeys = relationKeys + wayKeys + filteredNodeKeys
-        on(nodeDB.getIdsOlderThan(123L)).thenReturn(nodeKeys.map { it.id })
-        on(wayDB.getIdsOlderThan(123L)).thenReturn(wayKeys.map { it.id })
-        on(relationDB.getIdsOlderThan(123L)).thenReturn(relationKeys.map { it.id })
-        on(wayDB.filterNodeIdsWithoutWays(nodeKeys.map { it.id })).thenReturn(filteredNodeKeys.map { it.id })
+        on(elementDB.getIdsOlderThan(123L)).thenReturn(elementKeys)
         val listener = mock<MapDataController.Listener>()
 
         controller.addListener(listener)
         controller.deleteOlderThan(123L)
 
-        verify(elementDB).deleteAll(wayKeys + relationKeys)
-        verify(elementDB).deleteAll(filteredNodeKeys)
+        verify(elementDB).deleteAll(elementKeys)
         verify(geometryDB).deleteAll(elementKeys)
         verify(createdElementsController).deleteAll(elementKeys)
 

app/src/commonMain/kotlin/de/westnordost/streetcomplete/data/Cleaner.kt

@@ -34,8 +34,14 @@ class Cleaner(
         val time = nowAsEpochMilliseconds()
 
         val oldDataTimestamp = nowAsEpochMilliseconds() - ApplicationConstants.DELETE_OLD_DATA_AFTER
-        noteController.deleteOlderThan(oldDataTimestamp, MAX_DELETE_ELEMENTS)
-        mapDataController.deleteOlderThan(oldDataTimestamp, MAX_DELETE_ELEMENTS)
+        while (true) {
+            val deleted = noteController.deleteOlderThan(oldDataTimestamp, MAX_DELETE_ELEMENTS)
+            if (deleted < MAX_DELETE_ELEMENTS) break
+        }
+        while (true) {
+            val deleted = mapDataController.deleteOlderThan(oldDataTimestamp, MAX_DELETE_ELEMENTS)
+            if (deleted < MAX_DELETE_ELEMENTS) break
+        }
         downloadedTilesController.deleteOlderThan(oldDataTimestamp)
         // do this after cleaning map data and notes, because some metadata rely on map data
         questTypeRegistry.forEach { it.deleteMetadataOlderThan(oldDataTimestamp) }

app/src/commonMain/kotlin/de/westnordost/streetcomplete/data/osm/edits/upload/ElementEditUploader.kt

@@ -11,7 +11,9 @@ import de.westnordost.streetcomplete.data.osm.mapdata.MapDataApiClient
 import de.westnordost.streetcomplete.data.osm.mapdata.MapDataChanges
 import de.westnordost.streetcomplete.data.osm.mapdata.MapDataController
 import de.westnordost.streetcomplete.data.osm.mapdata.MapDataUpdates
+import de.westnordost.streetcomplete.data.osm.mapdata.Node
 import de.westnordost.streetcomplete.data.osm.mapdata.RemoteMapDataRepository
+import de.westnordost.streetcomplete.data.osm.mapdata.Way
 
 class ElementEditUploader(
     private val changesetManager: OpenChangesetsManager,
@@ -64,7 +66,7 @@ class ElementEditUploader(
         // remote in an incompatible way. So, we don't catch the exception but exit
         val remoteChanges = edit.action.createUpdates(RemoteMapDataRepository(mapDataApi), getIdProvider())
 
-        return try {
+        val updates = try {
             uploadChanges(edit, remoteChanges, false)
         }
         // probably changeset was closed -> try again once with new changeset
@@ -75,6 +77,14 @@ class ElementEditUploader(
         catch (e: ChangesetTooLargeException) {
             uploadChanges(edit, remoteChanges, true)
         }
+
+        // We need to backfill any updates of ways which include nodes that we don't have in the
+        // local database already with these nodes from remote. Otherwise, when new nodes have been
+        // added to the updated way, we'd end up with incomplete ways.
+        // We only need to do that in this method because when just uploading local changes without
+        // conflict, there can't be any new elements referenced by the updated way except those we
+        // added ourselves and thus already know.
+        return backfillMapDataUpdates(updates)
     }
 
     private suspend fun uploadChanges(
@@ -89,4 +99,31 @@ class ElementEditUploader(
         }
         return mapDataApi.uploadChanges(changesetId, changes, ApplicationConstants::ignoreRelation)
     }
+
+    /** Ensures that all nodes of all updated ways in [updates] are either already present in the
+     *  local database or otherwise then also included in the result. */
+    private suspend fun backfillMapDataUpdates(updates: MapDataUpdates): MapDataUpdates {
+        val nodeIdsOfUpdatedWays = updates.updated
+            .filterIsInstance<Way>()
+            .flatMapTo(HashSet()) { it.nodeIds }
+
+        val idsOfUpdatedNodes =
+            updates.updated.filterIsInstance<Node>().mapTo(HashSet()) { it.id } +
+            updates.idUpdates.map { it.newElementId }
+
+        val nodeIdsThatMustBePresentInLocalData = nodeIdsOfUpdatedWays - idsOfUpdatedNodes
+
+        val presentNodeIds = mapDataController
+            .getNodes(nodeIdsThatMustBePresentInLocalData)
+            .mapTo(HashSet()) { it.id }
+
+        val nodesThatMustBeFetchedFromRemote = nodeIdsThatMustBePresentInLocalData - presentNodeIds
+
+        return if (nodesThatMustBeFetchedFromRemote.isNotEmpty()) {
+            val nodes = nodesThatMustBeFetchedFromRemote.mapNotNull { mapDataApi.getNode(it) }
+            updates.copy(updated = updates.updated + nodes)
+        } else {
+            updates
+        }
+    }
 }

app/src/commonMain/kotlin/de/westnordost/streetcomplete/data/osm/edits/upload/ElementEditsUploader.kt

@@ -21,6 +21,7 @@ import kotlinx.coroutines.CoroutineName
 import kotlinx.coroutines.CoroutineScope
 import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.IO
+import kotlinx.coroutines.NonCancellable
 import kotlinx.coroutines.SupervisorJob
 import kotlinx.coroutines.sync.Mutex
 import kotlinx.coroutines.sync.withLock
@@ -46,7 +47,7 @@ class ElementEditsUploader(
             /* the sync of local change -> API and its response should not be cancellable because
              * otherwise an inconsistency in the data would occur. E.g. no "star" for an uploaded
              * change, a change could be uploaded twice etc */
-            withContext(scope.coroutineContext) { uploadEdit(edit, getIdProvider) }
+            withContext(NonCancellable) { uploadEdit(edit, getIdProvider) }
         }
     } }
 

app/src/commonMain/kotlin/de/westnordost/streetcomplete/data/osm/mapdata/ElementDao.kt

@@ -85,6 +85,15 @@ class ElementDao(
         wayDao.clear()
         nodeDao.clear()
     }
+
+    fun getIdsOlderThan(timestamp: Long, limit: Int? = null): List<ElementKey> {
+        val result = mutableListOf<ElementKey>()
+        // get relations first, then ways, then nodes because relations depend on ways depend on nodes.
+        result.addAll(relationDao.getIdsOlderThan(timestamp, limit?.minus(result.size)).map { ElementKey(RELATION, it) })
+        result.addAll(wayDao.getIdsOlderThan(timestamp, limit?.minus(result.size)).map { ElementKey(WAY, it) })
+        result.addAll(nodeDao.getIdsOlderThan(timestamp, limit?.minus(result.size)).map { ElementKey(NODE, it) })
+        return result
+    }
 }
 
 private fun Iterable<ElementKey>.filterByType(type: ElementType) =

app/src/commonMain/kotlin/de/westnordost/streetcomplete/data/osm/mapdata/MapDataApiClient.kt

@@ -33,7 +33,6 @@ class MapDataApiClient(
      *
      * @param changesetId id of the changeset to upload changes into
      * @param changes changes to upload.
-```suggestion
      * @param ignoreRelation omit any relations for which the given function returns true.
      *                       Such relations can still be referred to as relation members,
      *                       though, the relations themselves are just not included

app/src/commonMain/kotlin/de/westnordost/streetcomplete/data/osm/mapdata/MapDataController.kt

@@ -241,21 +241,11 @@ class MapDataController internal constructor(
         var elementCount = 0
         var geometryCount = 0
         lock.withLock {
-            val relations = relationDB.getIdsOlderThan(timestamp, limit).map { ElementKey(ElementType.RELATION, it) }
-            val ways = wayDB.getIdsOlderThan(timestamp, limit?.minus(relations.size)).map { ElementKey(ElementType.WAY, it) }
-
-            // delete now, so filterNodeIdsWithoutWays works as intended
-            cache.update(deletedKeys = ways + relations)
-            val wayAndRelationCount = elementDB.deleteAll(ways + relations)
-            val nodes = nodeDB.getIdsOlderThan(timestamp, limit?.minus(relations.size + ways.size))
-            // filter nodes to only delete nodes that are not part of a ways in the database
-            val filteredNodes = wayDB.filterNodeIdsWithoutWays(nodes).map { ElementKey(ElementType.NODE, it) }
-
-            elements = relations + ways + filteredNodes
+            elements = elementDB.getIdsOlderThan(timestamp, limit)
             if (elements.isEmpty()) return 0
 
-            cache.update(deletedKeys = filteredNodes)
-            elementCount = wayAndRelationCount + elementDB.deleteAll(filteredNodes)
+            cache.update(deletedKeys = elements)
+            elementCount = elementDB.deleteAll(elements)
             geometryCount = geometryDB.deleteAll(elements)
             createdElementsController.deleteAll(elements)
         }

app/src/commonMain/kotlin/de/westnordost/streetcomplete/data/osm/mapdata/WayDao.kt

@@ -124,10 +124,4 @@ class WayDao(private val db: Database) {
             limit = limit
         ) { it.getLong(ID) }
     }
-
-    fun filterNodeIdsWithoutWays(nodeIds: Collection<Long>): Collection<Long> {
-        val idsString = nodeIds.joinToString(",")
-        val nodeIdsWithWays = db.query(NAME_NODES, where = "$NODE_ID IN ($idsString)", columns = arrayOf(NODE_ID)) { c -> c.getLong(NODE_ID) }
-        return nodeIds - nodeIdsWithWays.toHashSet()
-    }
 }