From 6090c26a58e9a994dff530cd5db5b1a2346b59b9 Mon Sep 17 00:00:00 2001 From: Tobias Zwick Date: Thu, 15 Aug 2024 23:31:31 +0200 Subject: [PATCH] Multiplatform OSM API client (#5686) --- app/build.gradle.kts | 20 +- .../data/ApiClientExceptions.kt | 69 +++++ .../data/CommunicationException.kt | 29 -- .../streetcomplete/data/OsmApiModule.kt | 43 +-- .../data/download/QueryTooBigException.kt | 6 - .../osm/edits/upload/ElementEditUploader.kt | 28 +- .../osm/edits/upload/ElementEditsUploader.kt | 14 +- .../upload/changesets/ChangesetApiClient.kt | 68 +++++ .../changesets/ChangesetApiSerializer.kt | 29 ++ .../changesets/OpenChangesetsManager.kt | 34 +-- .../data/osm/mapdata/BoundingBox.kt | 9 + .../data/osm/mapdata/Element.kt | 13 +- .../data/osm/mapdata/MapDataApi.kt | 192 ------------- .../data/osm/mapdata/MapDataApiClient.kt | 209 ++++++++++++++ .../data/osm/mapdata/MapDataApiImpl.kt | 257 ------------------ .../data/osm/mapdata/MapDataApiParser.kt | 96 +++++++ .../data/osm/mapdata/MapDataApiSerializer.kt | 74 +++++ .../data/osm/mapdata/MapDataDownloader.kt | 19 +- .../data/osm/mapdata/MapDataUpdates.kt | 94 +++++++ .../data/osm/mapdata/MutableMapData.kt | 12 +- .../osm/mapdata/RemoteMapDataRepository.kt | 21 ++ .../osm/mapdata/UpdatedElementsHandler.kt | 79 ------ .../data/osmnotes/AvatarsDownloader.kt | 6 +- .../streetcomplete/data/osmnotes/NotesApi.kt | 68 ----- .../data/osmnotes/NotesApiClient.kt | 142 ++++++++++ .../data/osmnotes/NotesApiImpl.kt | 109 -------- .../data/osmnotes/NotesApiParser.kt | 93 +++++++ .../data/osmnotes/NotesDownloader.kt | 10 +- .../data/osmnotes/NotesModule.kt | 2 +- .../data/osmnotes/PhotoServiceApiClient.kt | 83 ++++++ .../osmnotes/StreetCompleteImageUploader.kt | 115 -------- .../data/osmnotes/edits/NoteEditsUploader.kt | 17 +- .../data/osmtracks/TracksApi.kt | 26 -- .../data/osmtracks/TracksApiClient.kt | 63 +++++ .../data/osmtracks/TracksApiImpl.kt | 67 ----- .../data/osmtracks/TracksSerializer.kt | 45 +++ .../streetcomplete/data/upload/Uploader.kt | 2 +- .../streetcomplete/data/user/UserApiClient.kt | 56 ++++ .../streetcomplete/data/user/UserApiParser.kt | 40 +++ .../data/user/UserDataController.kt | 5 +- .../streetcomplete/data/user/UserInfo.kt | 8 + .../data/user/UserLoginController.kt | 4 - .../streetcomplete/data/user/UserModule.kt | 6 +- .../streetcomplete/data/user/UserUpdater.kt | 9 +- .../{OAuthService.kt => OAuthApiClient.kt} | 61 +++-- .../user/oauth/OAuthConnectionException.kt | 7 - ...csDownloader.kt => StatisticsApiClient.kt} | 12 +- .../data/user/statistics/StatisticsModule.kt | 2 +- .../data/user/statistics/StatisticsParser.kt | 80 +++--- .../streetcomplete/screens/MainActivity.kt | 3 +- .../screens/user/login/LoginViewModel.kt | 8 +- .../streetcomplete/util/ktx/XmlReader.kt | 7 + .../streetcomplete/util/ktx/XmlWriter.kt | 9 + .../edits/upload/ElementEditUploaderTest.kt | 27 +- .../edits/upload/ElementEditsUploaderTest.kt | 4 +- .../osm/edits/upload/MapDataUpdatesTest.kt | 207 ++++++++++++++ .../upload/UpdatedElementsHandlerTest.kt | 201 -------------- .../changesets/ChangesetApiClientTest.kt | 46 ++++ .../changesets/ChangesetApiSerializerTest.kt | 28 ++ .../changesets/OpenChangesetsManagerTest.kt | 36 +-- .../data/osm/mapdata/MapDataApiClientTest.kt | 217 +++++++++++++++ .../data/osm/mapdata/MapDataApiParserTest.kt | 78 ++++++ .../osm/mapdata/MapDataApiSerializerTest.kt | 38 +++ .../data/osm/mapdata/MapDataApiTestUtils.kt | 88 ++++++ .../data/osmnotes/AvatarsDownloaderTest.kt | 58 ++-- .../data/osmnotes/NotesApiClientTest.kt | 140 ++++++++++ .../data/osmnotes/NotesApiParserTest.kt | 126 +++++++++ .../data/osmnotes/NotesDownloaderTest.kt | 4 +- .../osmnotes/PhotoServiceApiClientTest.kt | 95 +++++++ .../StreetCompleteImageUploaderTest.kt | 121 --------- .../osmnotes/edits/NoteEditsUploaderTest.kt | 24 +- .../data/osmtracks/TracksApiClientTest.kt | 44 +++ .../data/osmtracks/TracksSerializerTest.kt | 52 ++++ .../data/user/UserApiClientTest.kt | 55 ++++ .../data/user/UserApiParserTest.kt | 61 +++++ .../data/user/oauth/OAuthAuthorizationTest.kt | 33 +-- .../statistics/StatisticsApiClientTest.kt | 57 ++++ .../statistics/StatisticsDownloaderTest.kt | 44 --- .../streetcomplete/testutils/OsmDevApi.kt | 9 + .../test/resources/image_uploader/empty.jpg | 0 .../test/resources/image_uploader/invalid.jpg | 1 - .../resources/image_uploader/ioexception.jpg | 1 - .../test/resources/image_uploader/valid.jpg | 1 - 83 files changed, 2849 insertions(+), 1627 deletions(-) create mode 100644 app/src/main/java/de/westnordost/streetcomplete/data/ApiClientExceptions.kt delete mode 100644 app/src/main/java/de/westnordost/streetcomplete/data/CommunicationException.kt delete mode 100644 app/src/main/java/de/westnordost/streetcomplete/data/download/QueryTooBigException.kt create mode 100644 app/src/main/java/de/westnordost/streetcomplete/data/osm/edits/upload/changesets/ChangesetApiClient.kt create mode 100644 app/src/main/java/de/westnordost/streetcomplete/data/osm/edits/upload/changesets/ChangesetApiSerializer.kt delete mode 100644 app/src/main/java/de/westnordost/streetcomplete/data/osm/mapdata/MapDataApi.kt create mode 100644 app/src/main/java/de/westnordost/streetcomplete/data/osm/mapdata/MapDataApiClient.kt delete mode 100644 app/src/main/java/de/westnordost/streetcomplete/data/osm/mapdata/MapDataApiImpl.kt create mode 100644 app/src/main/java/de/westnordost/streetcomplete/data/osm/mapdata/MapDataApiParser.kt create mode 100644 app/src/main/java/de/westnordost/streetcomplete/data/osm/mapdata/MapDataApiSerializer.kt create mode 100644 app/src/main/java/de/westnordost/streetcomplete/data/osm/mapdata/MapDataUpdates.kt create mode 100644 app/src/main/java/de/westnordost/streetcomplete/data/osm/mapdata/RemoteMapDataRepository.kt delete mode 100644 app/src/main/java/de/westnordost/streetcomplete/data/osm/mapdata/UpdatedElementsHandler.kt delete mode 100644 app/src/main/java/de/westnordost/streetcomplete/data/osmnotes/NotesApi.kt create mode 100644 app/src/main/java/de/westnordost/streetcomplete/data/osmnotes/NotesApiClient.kt delete mode 100644 app/src/main/java/de/westnordost/streetcomplete/data/osmnotes/NotesApiImpl.kt create mode 100644 app/src/main/java/de/westnordost/streetcomplete/data/osmnotes/NotesApiParser.kt create mode 100644 app/src/main/java/de/westnordost/streetcomplete/data/osmnotes/PhotoServiceApiClient.kt delete mode 100644 app/src/main/java/de/westnordost/streetcomplete/data/osmnotes/StreetCompleteImageUploader.kt delete mode 100644 app/src/main/java/de/westnordost/streetcomplete/data/osmtracks/TracksApi.kt create mode 100644 app/src/main/java/de/westnordost/streetcomplete/data/osmtracks/TracksApiClient.kt delete mode 100644 app/src/main/java/de/westnordost/streetcomplete/data/osmtracks/TracksApiImpl.kt create mode 100644 app/src/main/java/de/westnordost/streetcomplete/data/osmtracks/TracksSerializer.kt create mode 100644 app/src/main/java/de/westnordost/streetcomplete/data/user/UserApiClient.kt create mode 100644 app/src/main/java/de/westnordost/streetcomplete/data/user/UserApiParser.kt create mode 100644 app/src/main/java/de/westnordost/streetcomplete/data/user/UserInfo.kt rename app/src/main/java/de/westnordost/streetcomplete/data/user/oauth/{OAuthService.kt => OAuthApiClient.kt} (76%) delete mode 100644 app/src/main/java/de/westnordost/streetcomplete/data/user/oauth/OAuthConnectionException.kt rename app/src/main/java/de/westnordost/streetcomplete/data/user/statistics/{StatisticsDownloader.kt => StatisticsApiClient.kt} (51%) create mode 100644 app/src/main/java/de/westnordost/streetcomplete/util/ktx/XmlReader.kt create mode 100644 app/src/main/java/de/westnordost/streetcomplete/util/ktx/XmlWriter.kt create mode 100644 app/src/test/java/de/westnordost/streetcomplete/data/osm/edits/upload/MapDataUpdatesTest.kt delete mode 100644 app/src/test/java/de/westnordost/streetcomplete/data/osm/edits/upload/UpdatedElementsHandlerTest.kt create mode 100644 app/src/test/java/de/westnordost/streetcomplete/data/osm/edits/upload/changesets/ChangesetApiClientTest.kt create mode 100644 app/src/test/java/de/westnordost/streetcomplete/data/osm/edits/upload/changesets/ChangesetApiSerializerTest.kt create mode 100644 app/src/test/java/de/westnordost/streetcomplete/data/osm/mapdata/MapDataApiClientTest.kt create mode 100644 app/src/test/java/de/westnordost/streetcomplete/data/osm/mapdata/MapDataApiParserTest.kt create mode 100644 app/src/test/java/de/westnordost/streetcomplete/data/osm/mapdata/MapDataApiSerializerTest.kt create mode 100644 app/src/test/java/de/westnordost/streetcomplete/data/osm/mapdata/MapDataApiTestUtils.kt create mode 100644 app/src/test/java/de/westnordost/streetcomplete/data/osmnotes/NotesApiClientTest.kt create mode 100644 app/src/test/java/de/westnordost/streetcomplete/data/osmnotes/NotesApiParserTest.kt create mode 100644 app/src/test/java/de/westnordost/streetcomplete/data/osmnotes/PhotoServiceApiClientTest.kt delete mode 100644 app/src/test/java/de/westnordost/streetcomplete/data/osmnotes/StreetCompleteImageUploaderTest.kt create mode 100644 app/src/test/java/de/westnordost/streetcomplete/data/osmtracks/TracksApiClientTest.kt create mode 100644 app/src/test/java/de/westnordost/streetcomplete/data/osmtracks/TracksSerializerTest.kt create mode 100644 app/src/test/java/de/westnordost/streetcomplete/data/user/UserApiClientTest.kt create mode 100644 app/src/test/java/de/westnordost/streetcomplete/data/user/UserApiParserTest.kt create mode 100644 app/src/test/java/de/westnordost/streetcomplete/data/user/statistics/StatisticsApiClientTest.kt delete mode 100644 app/src/test/java/de/westnordost/streetcomplete/data/user/statistics/StatisticsDownloaderTest.kt create mode 100644 app/src/test/java/de/westnordost/streetcomplete/testutils/OsmDevApi.kt delete mode 100644 app/src/test/resources/image_uploader/empty.jpg delete mode 100644 app/src/test/resources/image_uploader/invalid.jpg delete mode 100644 app/src/test/resources/image_uploader/ioexception.jpg delete mode 100644 app/src/test/resources/image_uploader/valid.jpg diff --git a/app/build.gradle.kts b/app/build.gradle.kts index ec8ffcc7a8..d4e0f21ea6 100644 --- a/app/build.gradle.kts +++ b/app/build.gradle.kts @@ -102,14 +102,6 @@ repositories { mavenCentral() } -configurations { - all { - // it's already included in Android - exclude(group = "net.sf.kxml", module = "kxml2") - exclude(group = "xmlpull", module = "xmlpull") - } -} - dependencies { val mockitoVersion = "3.12.4" @@ -180,19 +172,16 @@ dependencies { // HTTP Client implementation("io.ktor:ktor-client-core:2.3.12") - implementation("io.ktor:ktor-client-cio:2.3.12") + implementation("io.ktor:ktor-client-android:2.3.12") testImplementation("io.ktor:ktor-client-mock:2.3.12") + // TODO: as soon as both ktor-client and kotlinx-serialization have been refactored to be based + // on kotlinx-io, revisit sending and receiving xml/json payloads via APIs, currently it + // is all String-based, i.e. no KMP equivalent of InputStream/OutputStream involved // finding in which country we are for country-specific logic implementation("de.westnordost:countryboundaries:2.1") // finding a name for a feature without a name tag implementation("de.westnordost:osmfeatures:6.1") - // talking with the OSM API - implementation("de.westnordost:osmapi-map:3.0") - implementation("de.westnordost:osmapi-changesets:3.0") - implementation("de.westnordost:osmapi-notes:3.0") - implementation("de.westnordost:osmapi-traces:3.1") - implementation("de.westnordost:osmapi-user:3.0") // widgets implementation("androidx.viewpager2:viewpager2:1.1.0") @@ -210,6 +199,7 @@ dependencies { // serialization implementation("org.jetbrains.kotlinx:kotlinx-serialization-json:1.7.1") implementation("com.charleskorn.kaml:kaml:0.61.0") + implementation("io.github.pdvrieze.xmlutil:core:0.90.1") // map and location implementation("org.maplibre.gl:android-sdk:11.1.0") diff --git a/app/src/main/java/de/westnordost/streetcomplete/data/ApiClientExceptions.kt b/app/src/main/java/de/westnordost/streetcomplete/data/ApiClientExceptions.kt new file mode 100644 index 0000000000..c23559dd19 --- /dev/null +++ b/app/src/main/java/de/westnordost/streetcomplete/data/ApiClientExceptions.kt @@ -0,0 +1,69 @@ +package de.westnordost.streetcomplete.data + +import io.ktor.client.plugins.ClientRequestException +import io.ktor.client.plugins.ServerResponseException +import io.ktor.http.HttpStatusCode +import kotlinx.io.IOException +import kotlinx.serialization.SerializationException + +inline fun wrapApiClientExceptions(block: () -> T): T = + try { + block() + } + // server replied with (server) error 5xx + catch (e: ServerResponseException) { + throw ConnectionException(e.message, e) + } + // unexpected answer by server -> server issue + catch (e: SerializationException) { + throw ConnectionException(e.message, e) + } + // issue with establishing a connection -> nothing we can do about + catch (e: IOException) { + throw ConnectionException(e.message, e) + } + // server replied with (client) error 4xx + catch (e: ClientRequestException) { + when (e.response.status) { + // request timeout is rather a temporary connection error + HttpStatusCode.RequestTimeout -> { + throw ConnectionException(e.message, e) + } + // rate limiting is treated like a temporary connection error, i.e. try again later + HttpStatusCode.TooManyRequests -> { + throw ConnectionException(e.message, e) + } + // authorization is something we can handle (by requiring (re-)login of the user) + HttpStatusCode.Forbidden, HttpStatusCode.Unauthorized -> { + throw AuthorizationException(e.message, e) + } + else -> { + throw ApiClientException(e.message, e) + } + } + } + +/** The server responded with an unhandled error code */ +class ApiClientException(message: String? = null, cause: Throwable? = null) + : RuntimeException(message, cause) + +/** An error occurred while trying to communicate with an API over the internet. Either the + * connection with the API cannot be established, the server replies with a server error (5xx), + * request timeout (408) or it responds with an unexpected response, i.e. an error occurs while + * parsing the response. */ +class ConnectionException(message: String? = null, cause: Throwable? = null) + : RuntimeException(message, cause) + +/** While posting an update to an API over the internet, the API reports that our data is based on + * outdated data */ +class ConflictException(message: String? = null, cause: Throwable? = null) + : RuntimeException(message, cause) + +/** When a query made on an API over an internet would (probably) return a too large result */ +class QueryTooBigException (message: String? = null, cause: Throwable? = null) + : RuntimeException(message, cause) + +/** An error that indicates that the user either does not have the necessary authorization or + * authentication to execute an action through an API over the internet. */ +class AuthorizationException(message: String? = null, cause: Throwable? = null) + : RuntimeException(message, cause) diff --git a/app/src/main/java/de/westnordost/streetcomplete/data/CommunicationException.kt b/app/src/main/java/de/westnordost/streetcomplete/data/CommunicationException.kt deleted file mode 100644 index 65ca35eb69..0000000000 --- a/app/src/main/java/de/westnordost/streetcomplete/data/CommunicationException.kt +++ /dev/null @@ -1,29 +0,0 @@ -package de.westnordost.streetcomplete.data - -/** An error while communicating with an API over the internet occured. */ -open class CommunicationException @JvmOverloads constructor( - message: String? = null, - cause: Throwable? = null -) : RuntimeException(message, cause) - -/** An error that indicates that the user either does not have the necessary authorization or - * authentication to executes an action through an API over the internet. */ -class AuthorizationException @JvmOverloads constructor( - message: String? = null, - cause: Throwable? = null -) : CommunicationException(message, cause) - -/** An error occurred while trying to communicate with an API over the internet. Either the API is - * not reachable or replies with a server error. - * Like with an IO error, there is nothing we can do about that other than trying again later. */ -class ConnectionException @JvmOverloads constructor( - message: String? = null, - cause: Throwable? = null -) : CommunicationException(message, cause) - -/** While posting an update to an API over the internet, the API reports that our data is based on - * outdated data */ -class ConflictException @JvmOverloads constructor( - message: String? = null, - cause: Throwable? = null -) : CommunicationException(message, cause) diff --git a/app/src/main/java/de/westnordost/streetcomplete/data/OsmApiModule.kt b/app/src/main/java/de/westnordost/streetcomplete/data/OsmApiModule.kt index c15f8ec557..8600656368 100644 --- a/app/src/main/java/de/westnordost/streetcomplete/data/OsmApiModule.kt +++ b/app/src/main/java/de/westnordost/streetcomplete/data/OsmApiModule.kt @@ -1,15 +1,16 @@ package de.westnordost.streetcomplete.data -import de.westnordost.osmapi.OsmConnection -import de.westnordost.osmapi.user.UserApi -import de.westnordost.streetcomplete.ApplicationConstants -import de.westnordost.streetcomplete.data.osm.mapdata.MapDataApi -import de.westnordost.streetcomplete.data.osm.mapdata.MapDataApiImpl -import de.westnordost.streetcomplete.data.osmnotes.NotesApi -import de.westnordost.streetcomplete.data.osmnotes.NotesApiImpl -import de.westnordost.streetcomplete.data.osmtracks.TracksApi -import de.westnordost.streetcomplete.data.osmtracks.TracksApiImpl -import de.westnordost.streetcomplete.data.preferences.Preferences +import de.westnordost.streetcomplete.data.osm.edits.upload.changesets.ChangesetApiClient +import de.westnordost.streetcomplete.data.osm.edits.upload.changesets.ChangesetApiSerializer +import de.westnordost.streetcomplete.data.user.UserApiClient +import de.westnordost.streetcomplete.data.osm.mapdata.MapDataApiClient +import de.westnordost.streetcomplete.data.osm.mapdata.MapDataApiParser +import de.westnordost.streetcomplete.data.osm.mapdata.MapDataApiSerializer +import de.westnordost.streetcomplete.data.osmnotes.NotesApiClient +import de.westnordost.streetcomplete.data.osmnotes.NotesApiParser +import de.westnordost.streetcomplete.data.osmtracks.TracksApiClient +import de.westnordost.streetcomplete.data.osmtracks.TracksSerializer +import de.westnordost.streetcomplete.data.user.UserApiParser import org.koin.androidx.workmanager.dsl.worker import org.koin.core.qualifier.named import org.koin.dsl.module @@ -19,17 +20,21 @@ private const val OSM_API_URL = "https://api.openstreetmap.org/api/0.6/" val osmApiModule = module { factory { Cleaner(get(), get(), get(), get(), get(), get()) } factory { CacheTrimmer(get(), get()) } - factory { MapDataApiImpl(get()) } - factory { NotesApiImpl(get()) } - factory { TracksApiImpl(get()) } + factory { MapDataApiClient(get(), OSM_API_URL, get(), get(), get()) } + factory { NotesApiClient(get(), OSM_API_URL, get(), get()) } + factory { TracksApiClient(get(), OSM_API_URL, get(), get()) } + factory { UserApiClient(get(), OSM_API_URL, get(), get()) } + factory { ChangesetApiClient(get(), OSM_API_URL, get(), get()) } + factory { Preloader(get(named("CountryBoundariesLazy")), get(named("FeatureDictionaryLazy"))) } - factory { UserApi(get()) } - single { OsmConnection( - OSM_API_URL, - ApplicationConstants.USER_AGENT, - get().oAuth2AccessToken - ) } + factory { UserApiParser() } + factory { NotesApiParser() } + factory { TracksSerializer() } + factory { MapDataApiParser() } + factory { MapDataApiSerializer() } + factory { ChangesetApiSerializer() } + single { UnsyncedChangesCountSource(get(), get()) } worker { CleanerWorker(get(), get(), get()) } diff --git a/app/src/main/java/de/westnordost/streetcomplete/data/download/QueryTooBigException.kt b/app/src/main/java/de/westnordost/streetcomplete/data/download/QueryTooBigException.kt deleted file mode 100644 index ff441f7187..0000000000 --- a/app/src/main/java/de/westnordost/streetcomplete/data/download/QueryTooBigException.kt +++ /dev/null @@ -1,6 +0,0 @@ -package de.westnordost.streetcomplete.data.download - -class QueryTooBigException @JvmOverloads constructor( - message: String? = null, - cause: Throwable? = null -) : RuntimeException(message, cause) diff --git a/app/src/main/java/de/westnordost/streetcomplete/data/osm/edits/upload/ElementEditUploader.kt b/app/src/main/java/de/westnordost/streetcomplete/data/osm/edits/upload/ElementEditUploader.kt index 60f22c7b61..184e6118c1 100644 --- a/app/src/main/java/de/westnordost/streetcomplete/data/osm/edits/upload/ElementEditUploader.kt +++ b/app/src/main/java/de/westnordost/streetcomplete/data/osm/edits/upload/ElementEditUploader.kt @@ -5,14 +5,15 @@ import de.westnordost.streetcomplete.data.ConflictException import de.westnordost.streetcomplete.data.osm.edits.ElementEdit import de.westnordost.streetcomplete.data.osm.edits.ElementIdProvider import de.westnordost.streetcomplete.data.osm.edits.upload.changesets.OpenChangesetsManager -import de.westnordost.streetcomplete.data.osm.mapdata.MapDataApi +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.RemoteMapDataRepository class ElementEditUploader( private val changesetManager: OpenChangesetsManager, - private val mapDataApi: MapDataApi, + private val mapDataApi: MapDataApiClient, private val mapDataController: MapDataController ) { @@ -20,8 +21,8 @@ class ElementEditUploader( * * @throws ConflictException if element has been changed server-side in an incompatible way */ - fun upload(edit: ElementEdit, getIdProvider: () -> ElementIdProvider): MapDataUpdates { - val remoteChanges by lazy { edit.action.createUpdates(mapDataApi, getIdProvider()) } + suspend fun upload(edit: ElementEdit, getIdProvider: () -> ElementIdProvider): MapDataUpdates { + val remoteChanges by lazy { edit.action.createUpdates(RemoteMapDataRepository(mapDataApi), getIdProvider()) } val localChanges by lazy { edit.action.createUpdates(mapDataController, getIdProvider()) } val mustUseRemoteData = edit.action::class in ApplicationConstants.EDIT_ACTIONS_NOT_ALLOWED_TO_USE_LOCAL_CHANGES @@ -48,13 +49,16 @@ class ElementEditUploader( } } - private fun uploadChanges(edit: ElementEdit, mapDataChanges: MapDataChanges, newChangeset: Boolean): MapDataUpdates { - val changesetId = - if (newChangeset) { - changesetManager.createChangeset(edit.type, edit.source, edit.position) - } else { - changesetManager.getOrCreateChangeset(edit.type, edit.source, edit.position, edit.isNearUserLocation) - } - return mapDataApi.uploadChanges(changesetId, mapDataChanges, ApplicationConstants.IGNORED_RELATION_TYPES) + private suspend fun uploadChanges( + edit: ElementEdit, + changes: MapDataChanges, + newChangeset: Boolean + ): MapDataUpdates { + val changesetId = if (newChangeset) { + changesetManager.createChangeset(edit.type, edit.source, edit.position) + } else { + changesetManager.getOrCreateChangeset(edit.type, edit.source, edit.position, edit.isNearUserLocation) + } + return mapDataApi.uploadChanges(changesetId, changes, ApplicationConstants.IGNORED_RELATION_TYPES) } } diff --git a/app/src/main/java/de/westnordost/streetcomplete/data/osm/edits/upload/ElementEditsUploader.kt b/app/src/main/java/de/westnordost/streetcomplete/data/osm/edits/upload/ElementEditsUploader.kt index c4e2b3d488..98335d76ee 100644 --- a/app/src/main/java/de/westnordost/streetcomplete/data/osm/edits/upload/ElementEditsUploader.kt +++ b/app/src/main/java/de/westnordost/streetcomplete/data/osm/edits/upload/ElementEditsUploader.kt @@ -9,7 +9,7 @@ import de.westnordost.streetcomplete.data.osm.mapdata.Element import de.westnordost.streetcomplete.data.osm.mapdata.ElementKey import de.westnordost.streetcomplete.data.osm.mapdata.ElementType import de.westnordost.streetcomplete.data.osm.mapdata.MapData -import de.westnordost.streetcomplete.data.osm.mapdata.MapDataApi +import de.westnordost.streetcomplete.data.osm.mapdata.MapDataApiClient import de.westnordost.streetcomplete.data.osm.mapdata.MapDataController import de.westnordost.streetcomplete.data.osm.mapdata.MapDataUpdates import de.westnordost.streetcomplete.data.osm.mapdata.MutableMapData @@ -30,7 +30,7 @@ class ElementEditsUploader( private val noteEditsController: NoteEditsController, private val mapDataController: MapDataController, private val singleUploader: ElementEditUploader, - private val mapDataApi: MapDataApi, + private val mapDataApi: MapDataApiClient, private val statisticsController: StatisticsController ) { var uploadedChangeListener: OnUploadedChangeListener? = null @@ -95,12 +95,10 @@ class ElementEditsUploader( } private suspend fun fetchElementComplete(elementType: ElementType, elementId: Long): MapData? = - withContext(Dispatchers.IO) { - when (elementType) { - ElementType.NODE -> mapDataApi.getNode(elementId)?.let { MutableMapData(listOf(it)) } - ElementType.WAY -> mapDataApi.getWayComplete(elementId) - ElementType.RELATION -> mapDataApi.getRelationComplete(elementId) - } + when (elementType) { + ElementType.NODE -> mapDataApi.getNode(elementId)?.let { MutableMapData(listOf(it)) } + ElementType.WAY -> mapDataApi.getWayComplete(elementId) + ElementType.RELATION -> mapDataApi.getRelationComplete(elementId) } companion object { diff --git a/app/src/main/java/de/westnordost/streetcomplete/data/osm/edits/upload/changesets/ChangesetApiClient.kt b/app/src/main/java/de/westnordost/streetcomplete/data/osm/edits/upload/changesets/ChangesetApiClient.kt new file mode 100644 index 0000000000..a4e4c26bb8 --- /dev/null +++ b/app/src/main/java/de/westnordost/streetcomplete/data/osm/edits/upload/changesets/ChangesetApiClient.kt @@ -0,0 +1,68 @@ +package de.westnordost.streetcomplete.data.osm.edits.upload.changesets + +import de.westnordost.streetcomplete.data.AuthorizationException +import de.westnordost.streetcomplete.data.ConflictException +import de.westnordost.streetcomplete.data.ConnectionException +import de.westnordost.streetcomplete.data.user.UserLoginSource +import de.westnordost.streetcomplete.data.wrapApiClientExceptions +import io.ktor.client.HttpClient +import io.ktor.client.call.body +import io.ktor.client.plugins.ClientRequestException +import io.ktor.client.plugins.expectSuccess +import io.ktor.client.request.bearerAuth +import io.ktor.client.request.put +import io.ktor.client.request.setBody +import io.ktor.http.HttpStatusCode + +class ChangesetApiClient( + private val httpClient: HttpClient, + private val baseUrl: String, + private val userLoginSource: UserLoginSource, + private val serializer: ChangesetApiSerializer, +) { + /** + * Open a new changeset with the given tags + * + * @param tags tags of this changeset. Usually it is comment and source. + * + * @throws AuthorizationException if the application does not have permission to edit the map + * (OAuth scope "write_api") + * @throws ConnectionException if a temporary network connection problem occurs + * + * @return the id of the changeset + */ + suspend fun open(tags: Map): Long = wrapApiClientExceptions { + val response = httpClient.put(baseUrl + "changeset/create") { + userLoginSource.accessToken?.let { bearerAuth(it) } + setBody(serializer.serialize(tags)) + expectSuccess = true + } + return response.body().toLong() + } + + /** + * Closes the given changeset. + * + * @param id id of the changeset to close + * + * @throws ConflictException if the changeset has already been closed or does not exist + * @throws AuthorizationException if the application does not have permission to edit the map + * (OAuth scope "write_api") + * @throws ConnectionException if a temporary network connection problem occurs + */ + suspend fun close(id: Long): Unit = wrapApiClientExceptions { + try { + httpClient.put(baseUrl + "changeset/$id/close") { + userLoginSource.accessToken?.let { bearerAuth(it) } + expectSuccess = true + } + } catch (e: ClientRequestException) { + when (e.response.status) { + HttpStatusCode.Conflict, HttpStatusCode.NotFound -> { + throw ConflictException(e.message, e) + } + else -> throw e + } + } + } +} diff --git a/app/src/main/java/de/westnordost/streetcomplete/data/osm/edits/upload/changesets/ChangesetApiSerializer.kt b/app/src/main/java/de/westnordost/streetcomplete/data/osm/edits/upload/changesets/ChangesetApiSerializer.kt new file mode 100644 index 0000000000..0906973dee --- /dev/null +++ b/app/src/main/java/de/westnordost/streetcomplete/data/osm/edits/upload/changesets/ChangesetApiSerializer.kt @@ -0,0 +1,29 @@ +package de.westnordost.streetcomplete.data.osm.edits.upload.changesets + +import de.westnordost.streetcomplete.util.ktx.attribute +import de.westnordost.streetcomplete.util.ktx.endTag +import de.westnordost.streetcomplete.util.ktx.startTag +import nl.adaptivity.xmlutil.XmlWriter +import nl.adaptivity.xmlutil.newWriter +import nl.adaptivity.xmlutil.xmlStreaming + +class ChangesetApiSerializer { + fun serialize(changesetTags: Map): String { + val buffer = StringBuilder() + xmlStreaming.newWriter(buffer).serializeChangeset(changesetTags) + return buffer.toString() + } +} + +private fun XmlWriter.serializeChangeset(changesetTags: Map) { + startTag("osm") + startTag("changeset") + for ((k, v) in changesetTags) { + startTag("tag") + attribute("k", k) + attribute("v", v) + endTag("tag") + } + endTag("changeset") + endTag("osm") +} diff --git a/app/src/main/java/de/westnordost/streetcomplete/data/osm/edits/upload/changesets/OpenChangesetsManager.kt b/app/src/main/java/de/westnordost/streetcomplete/data/osm/edits/upload/changesets/OpenChangesetsManager.kt index 82aad39e8c..1fdd85df85 100644 --- a/app/src/main/java/de/westnordost/streetcomplete/data/osm/edits/upload/changesets/OpenChangesetsManager.kt +++ b/app/src/main/java/de/westnordost/streetcomplete/data/osm/edits/upload/changesets/OpenChangesetsManager.kt @@ -5,27 +5,28 @@ import de.westnordost.streetcomplete.ApplicationConstants.USER_AGENT import de.westnordost.streetcomplete.data.ConflictException import de.westnordost.streetcomplete.data.osm.edits.ElementEditType import de.westnordost.streetcomplete.data.osm.mapdata.LatLon -import de.westnordost.streetcomplete.data.osm.mapdata.MapDataApi import de.westnordost.streetcomplete.data.preferences.Preferences import de.westnordost.streetcomplete.util.ktx.nowAsEpochMilliseconds import de.westnordost.streetcomplete.util.logs.Log import de.westnordost.streetcomplete.util.math.distanceTo +import kotlinx.coroutines.Dispatchers.IO +import kotlinx.coroutines.withContext import java.util.Locale /** Manages the creation and reusage of changesets */ class OpenChangesetsManager( - private val mapDataApi: MapDataApi, + private val changesetApiClient: ChangesetApiClient, private val openChangesetsDB: OpenChangesetsDao, private val changesetAutoCloser: ChangesetAutoCloser, private val prefs: Preferences ) { - fun getOrCreateChangeset( + suspend fun getOrCreateChangeset( type: ElementEditType, source: String, position: LatLon, createNewIfTooFarAway: Boolean - ): Long = synchronized(this) { - val openChangeset = openChangesetsDB.get(type.name, source) + ): Long { + val openChangeset = withContext(IO) { openChangesetsDB.get(type.name, source) } ?: return createChangeset(type, source, position) if (createNewIfTooFarAway && position.distanceTo(openChangeset.lastPosition) > MAX_LAST_EDIT_DISTANCE) { @@ -36,35 +37,30 @@ class OpenChangesetsManager( } } - fun createChangeset( - type: ElementEditType, - source: String, - position: LatLon - ): Long = synchronized(this) { - val changesetId = mapDataApi.openChangeset(createChangesetTags(type, source)) - openChangesetsDB.put(OpenChangeset(type.name, source, changesetId, position)) + suspend fun createChangeset(type: ElementEditType, source: String, position: LatLon): Long { + val changesetId = changesetApiClient.open(createChangesetTags(type, source)) + withContext(IO) { openChangesetsDB.put(OpenChangeset(type.name, source, changesetId, position)) } changesetAutoCloser.enqueue(CLOSE_CHANGESETS_AFTER_INACTIVITY_OF) Log.i(TAG, "Created changeset #$changesetId") return changesetId } - fun closeOldChangesets() = synchronized(this) { + suspend fun closeOldChangesets() { val timePassed = nowAsEpochMilliseconds() - prefs.lastEditTime if (timePassed < CLOSE_CHANGESETS_AFTER_INACTIVITY_OF) return - for (info in openChangesetsDB.getAll()) { - closeChangeset(info) - } + val openChangesets = withContext(IO) { openChangesetsDB.getAll() } + openChangesets.forEach { closeChangeset(it) } } - private fun closeChangeset(openChangeset: OpenChangeset) { + private suspend fun closeChangeset(openChangeset: OpenChangeset) { try { - mapDataApi.closeChangeset(openChangeset.changesetId) + changesetApiClient.close(openChangeset.changesetId) Log.i(TAG, "Closed changeset #${openChangeset.changesetId}") } catch (e: ConflictException) { Log.w(TAG, "Couldn't close changeset #${openChangeset.changesetId} because it has already been closed") } finally { - openChangesetsDB.delete(openChangeset.questType, openChangeset.source) + withContext(IO) { openChangesetsDB.delete(openChangeset.questType, openChangeset.source) } } } diff --git a/app/src/main/java/de/westnordost/streetcomplete/data/osm/mapdata/BoundingBox.kt b/app/src/main/java/de/westnordost/streetcomplete/data/osm/mapdata/BoundingBox.kt index 866eff5a5e..65d69257de 100644 --- a/app/src/main/java/de/westnordost/streetcomplete/data/osm/mapdata/BoundingBox.kt +++ b/app/src/main/java/de/westnordost/streetcomplete/data/osm/mapdata/BoundingBox.kt @@ -1,5 +1,6 @@ package de.westnordost.streetcomplete.data.osm.mapdata +import de.westnordost.streetcomplete.util.ktx.format import de.westnordost.streetcomplete.util.math.normalizeLongitude import kotlinx.serialization.Serializable @@ -47,3 +48,11 @@ fun BoundingBox.toPolygon() = listOf( LatLon(max.latitude, min.longitude), min, ) + +/** bounding box bounds in counter-clockwise direction, starting with min longitude */ +fun BoundingBox.toOsmApiString(): String = listOf( + min.longitude, + min.latitude, + max.longitude, + max.latitude +).joinToString(",") { it.format(7) } diff --git a/app/src/main/java/de/westnordost/streetcomplete/data/osm/mapdata/Element.kt b/app/src/main/java/de/westnordost/streetcomplete/data/osm/mapdata/Element.kt index 98466d233d..a6baeb95d5 100644 --- a/app/src/main/java/de/westnordost/streetcomplete/data/osm/mapdata/Element.kt +++ b/app/src/main/java/de/westnordost/streetcomplete/data/osm/mapdata/Element.kt @@ -17,7 +17,7 @@ sealed class Element { data class Node( override val id: Long, val position: LatLon, - override val tags: Map = HashMap(0), + override val tags: Map = emptyMap(), override val version: Int = 1, override val timestampEdited: Long = 0 ) : Element() { @@ -30,7 +30,7 @@ data class Node( data class Way( override val id: Long, val nodeIds: List, - override val tags: Map = HashMap(0), + override val tags: Map = emptyMap(), override val version: Int = 1, override val timestampEdited: Long = 0 ) : Element() { @@ -45,7 +45,7 @@ data class Way( data class Relation( override val id: Long, val members: List, - override val tags: Map = HashMap(0), + override val tags: Map = emptyMap(), override val version: Int = 1, override val timestampEdited: Long = 0 ) : Element() { @@ -63,13 +63,6 @@ data class RelationMember( enum class ElementType { NODE, WAY, RELATION } -data class DiffElement( - val type: ElementType, - val clientId: Long, - val serverId: Long? = null, - val serverVersion: Int? = null -) - @Serializable data class LatLon( @SerialName("lat") diff --git a/app/src/main/java/de/westnordost/streetcomplete/data/osm/mapdata/MapDataApi.kt b/app/src/main/java/de/westnordost/streetcomplete/data/osm/mapdata/MapDataApi.kt deleted file mode 100644 index b76dc360d0..0000000000 --- a/app/src/main/java/de/westnordost/streetcomplete/data/osm/mapdata/MapDataApi.kt +++ /dev/null @@ -1,192 +0,0 @@ -package de.westnordost.streetcomplete.data.osm.mapdata - -import de.westnordost.streetcomplete.data.AuthorizationException -import de.westnordost.streetcomplete.data.ConflictException -import de.westnordost.streetcomplete.data.ConnectionException -import de.westnordost.streetcomplete.data.download.QueryTooBigException - -/** Get and upload changes to map data */ -interface MapDataApi : MapDataRepository { - - /** - * Upload changes into an opened changeset. - * - * @param changesetId id of the changeset to upload changes into - * @param changes changes to upload. - * - * @throws ConflictException if the changeset has already been closed, there is a conflict for - * the elements being uploaded or the user who created the changeset - * is not the same as the one uploading the change - * @throws AuthorizationException if the application does not have permission to edit the map - * (Permission.MODIFY_MAP) - * @throws ConnectionException if a temporary network connection problem occurs - * - * @return the updated elements - */ - fun uploadChanges(changesetId: Long, changes: MapDataChanges, ignoreRelationTypes: Set = emptySet()): MapDataUpdates - - /** - * Open a new changeset with the given tags - * - * @param tags tags of this changeset. Usually it is comment and source. - * - * @throws AuthorizationException if the application does not have permission to edit the map - * (Permission.MODIFY_MAP) - * @throws ConnectionException if a temporary network connection problem occurs - * - * @return the id of the changeset - */ - fun openChangeset(tags: Map): Long - - /** - * Closes the given changeset. - * - * @param changesetId id of the changeset to close - * - * @throws ConflictException if the changeset has already been closed - * @throws AuthorizationException if the application does not have permission to edit the map - * (Permission.MODIFY_MAP) - * @throws ConnectionException if a temporary network connection problem occurs - */ - fun closeChangeset(changesetId: Long) - - /** - * Feeds map data to the given MapDataHandler. - * If not logged in, the Changeset for each returned element will be null - * - * @param bounds rectangle in which to query map data. May not cross the 180th meridian. This is - * usually limited at 0.25 square degrees. Check the server capabilities. - * @param mutableMapData mutable map data to add the add the data to - * @param ignoreRelationTypes don't put any relations of the given types in the given mutableMapData - * - * @throws QueryTooBigException if the bounds area is too large - * @throws IllegalArgumentException if the bounds cross the 180th meridian. - * @throws ConnectionException if a temporary network connection problem occurs - * - * @return the map data - */ - fun getMap(bounds: BoundingBox, mutableMapData: MutableMapData, ignoreRelationTypes: Set = emptySet()) - - /** - * Queries the way with the given id plus all nodes that are in referenced by it. - * If not logged in, the Changeset for each returned element will be null - * - * @param id the way's id - * - * @throws ConnectionException if a temporary network connection problem occurs - * - * @return the map data - */ - override fun getWayComplete(id: Long): MapData? - - /** - * Queries the relation with the given id plus all it's members and all nodes of ways that are - * members of the relation. - * If not logged in, the Changeset for each returned element will be null - * - * @param id the relation's id - * - * @throws ConnectionException if a temporary network connection problem occurs - * - * @return the map data - */ - override fun getRelationComplete(id: Long): MapData? - - /** - * Note that if not logged in, the Changeset for each returned element will be null - * - * @param id the node's id - * - * @throws ConnectionException if a temporary network connection problem occurs - * - * @return the node with the given id or null if it does not exist - */ - override fun getNode(id: Long): Node? - - /** - * Note that if not logged in, the Changeset for each returned element will be null - * - * @param id the way's id - * - * @throws ConnectionException if a temporary network connection problem occurs - * - * @return the way with the given id or null if it does not exist - */ - override fun getWay(id: Long): Way? - - /** - * Note that if not logged in, the Changeset for each returned element will be null - * - * @param id the relation's id - * - * @throws ConnectionException if a temporary network connection problem occurs - * - * @return the relation with the given id or null if it does not exist - */ - override fun getRelation(id: Long): Relation? - - /** - * Note that if not logged in, the Changeset for each returned element will be null - * - * @param id the node's id - * - * @throws ConnectionException if a temporary network connection problem occurs - * - * @return all ways that reference the node with the given id. Empty if none. - */ - override fun getWaysForNode(id: Long): List - - /** - * Note that if not logged in, the Changeset for each returned element will be null - * - * @param id the node's id - * - * @throws ConnectionException if a temporary network connection problem occurs - * - * @return all relations that reference the node with the given id. Empty if none. - */ - override fun getRelationsForNode(id: Long): List - - /** - * Note that if not logged in, the Changeset for each returned element will be null - * - * @param id the way's id - * - * @throws ConnectionException if a temporary network connection problem occurs - * - * @return all relations that reference the way with the given id. Empty if none. - */ - override fun getRelationsForWay(id: Long): List - - /** - * Note that if not logged in, the Changeset for each returned element will be null - * - * @param id the relation's id - * - * @throws ConnectionException if a temporary network connection problem occurs - * - * @return all relations that reference the relation with the given id. Empty if none. - */ - override fun getRelationsForRelation(id: Long): List -} - -/** Data class that contains the map data updates (updated elements, deleted elements, elements - * whose id have been updated) after the modifications have been uploaded */ -data class MapDataUpdates( - val updated: Collection = emptyList(), - val deleted: Collection = emptyList(), - val idUpdates: Collection = emptyList() -) - -data class ElementIdUpdate( - val elementType: ElementType, - val oldElementId: Long, - val newElementId: Long -) - -/** Data class that contains a the request to create, modify elements and delete the given elements */ -data class MapDataChanges( - val creations: Collection = emptyList(), - val modifications: Collection = emptyList(), - val deletions: Collection = emptyList() -) diff --git a/app/src/main/java/de/westnordost/streetcomplete/data/osm/mapdata/MapDataApiClient.kt b/app/src/main/java/de/westnordost/streetcomplete/data/osm/mapdata/MapDataApiClient.kt new file mode 100644 index 0000000000..677a6e9304 --- /dev/null +++ b/app/src/main/java/de/westnordost/streetcomplete/data/osm/mapdata/MapDataApiClient.kt @@ -0,0 +1,209 @@ +package de.westnordost.streetcomplete.data.osm.mapdata + +import de.westnordost.streetcomplete.data.AuthorizationException +import de.westnordost.streetcomplete.data.ConflictException +import de.westnordost.streetcomplete.data.ConnectionException +import de.westnordost.streetcomplete.data.QueryTooBigException +import de.westnordost.streetcomplete.data.user.UserLoginSource +import de.westnordost.streetcomplete.data.wrapApiClientExceptions +import io.ktor.client.HttpClient +import io.ktor.client.call.body +import io.ktor.client.plugins.ClientRequestException +import io.ktor.client.plugins.expectSuccess +import io.ktor.client.request.bearerAuth +import io.ktor.client.request.get +import io.ktor.client.request.parameter +import io.ktor.client.request.post +import io.ktor.client.request.setBody +import io.ktor.http.HttpStatusCode + +/** Get and upload changes to map data */ +class MapDataApiClient( + private val httpClient: HttpClient, + private val baseUrl: String, + private val userLoginSource: UserLoginSource, + private val parser: MapDataApiParser, + private val serializer: MapDataApiSerializer, +) { + + /** + * Upload changes into an opened changeset. + * + * @param changesetId id of the changeset to upload changes into + * @param changes changes to upload. + * @param ignoreRelationTypes omit any updates to relations of the given types from the result. + * Such relations can still be referred to as relation members, + * though, the relations themselves are just not included + * + * @throws ConflictException if the changeset has already been closed, there is a conflict for + * the elements being uploaded or the user who created the changeset + * is not the same as the one uploading the change + * @throws AuthorizationException if the application does not have permission to edit the map + * (OAuth scope "write_api") + * @throws ConnectionException if a temporary network connection problem occurs + * + * @return the updated elements + */ + suspend fun uploadChanges( + changesetId: Long, + changes: MapDataChanges, + ignoreRelationTypes: Set = emptySet() + ): MapDataUpdates = wrapApiClientExceptions { + try { + val response = httpClient.post(baseUrl + "changeset/$changesetId/upload") { + userLoginSource.accessToken?.let { bearerAuth(it) } + setBody(serializer.serializeMapDataChanges(changes, changesetId)) + expectSuccess = true + } + val updates = parser.parseElementUpdates(response.body()) + val changedElements = changes.creations + changes.modifications + changes.deletions + return createMapDataUpdates(changedElements, updates, ignoreRelationTypes) + } catch (e: ClientRequestException) { + when (e.response.status) { + // current element version is outdated, current changeset has been closed already + HttpStatusCode.Conflict, + // an element referred to by another element does not exist (anymore) or was redacted + HttpStatusCode.PreconditionFailed, + // some elements do not exist (anymore) + HttpStatusCode.NotFound -> { + throw ConflictException(e.message, e) + } + else -> throw e + } + } + } + + /** + * Returns the map data in the given bounding box. + * + * @param bounds rectangle in which to query map data. May not cross the 180th meridian. This is + * usually limited at 0.25 square degrees. Check the server capabilities. + * @param ignoreRelationTypes omit any relations of the given types from the result. + * Such relations can still be referred to as relation members, + * though, the relations themselves are just not included + * + * @throws QueryTooBigException if the bounds area is too large or too many elements would be returned + * @throws IllegalArgumentException if the bounds cross the 180th meridian. + * @throws ConnectionException if a temporary network connection problem occurs + * + * @return the map data + */ + suspend fun getMap( + bounds: BoundingBox, + ignoreRelationTypes: Set = emptySet() + ): MutableMapData = wrapApiClientExceptions { + if (bounds.crosses180thMeridian) { + throw IllegalArgumentException("Bounding box crosses 180th meridian") + } + + try { + val response = httpClient.get(baseUrl + "map") { + parameter("bbox", bounds.toOsmApiString()) + expectSuccess = true + } + return parser.parseMapData(response.body(), ignoreRelationTypes) + } catch (e: ClientRequestException) { + if (e.response.status == HttpStatusCode.BadRequest) { + throw QueryTooBigException(e.message, e) + } else { + throw e + } + } + } + + /** + * Returns the given way by id plus all its nodes or null if the way does not exist. + * + * @throws ConnectionException if a temporary network connection problem occurs + */ + suspend fun getWayComplete(id: Long): MapData? = + getMapDataOrNull("way/$id/full") + + /** + * Returns the given relation by id plus all its members and all nodes of ways that are members + * of the relation. Or null if the relation does not exist. + * + * @throws ConnectionException if a temporary network connection problem occurs + */ + suspend fun getRelationComplete(id: Long): MapData? = + getMapDataOrNull("relation/$id/full") + + /** + * Return the given node by id or null if it doesn't exist + * + * @throws ConnectionException if a temporary network connection problem occurs + */ + suspend fun getNode(id: Long): Node? = + getMapDataOrNull("node/$id")?.nodes?.single() + + /** + * Return the given way by id or null if it doesn't exist + * + * @throws ConnectionException if a temporary network connection problem occurs + */ + suspend fun getWay(id: Long): Way? = + getMapDataOrNull("way/$id")?.ways?.single() + + /** + * Return the given relation by id or null if it doesn't exist + * + * @throws ConnectionException if a temporary network connection problem occurs + */ + suspend fun getRelation(id: Long): Relation? = + getMapDataOrNull("relation/$id")?.relations?.single() + + /** + * Return all ways in which the given node is used. + * + * @throws ConnectionException if a temporary network connection problem occurs + */ + suspend fun getWaysForNode(id: Long): Collection = + getMapDataOrNull("node/$id/ways")?.ways.orEmpty() + + /** + * Return all relations in which the given node is used. + * + * @throws ConnectionException if a temporary network connection problem occurs + */ + suspend fun getRelationsForNode(id: Long): Collection = + getMapDataOrNull("node/$id/relations")?.relations.orEmpty() + + /** + * Return all relations in which the given way is used. + * + * @throws ConnectionException if a temporary network connection problem occurs + */ + suspend fun getRelationsForWay(id: Long): Collection = + getMapDataOrNull("way/$id/relations")?.relations.orEmpty() + + /** + * Return all relations in which the given relation is used. + * + * @throws ConnectionException if a temporary network connection problem occurs + */ + suspend fun getRelationsForRelation(id: Long): Collection = + getMapDataOrNull("relation/$id/relations")?.relations.orEmpty() + + private suspend fun getMapDataOrNull(query: String): MapData? = wrapApiClientExceptions { + try { + val response = httpClient.get(baseUrl + query) { expectSuccess = true } + return parser.parseMapData(response.body(), emptySet()) + } catch (e: ClientRequestException) { + when (e.response.status) { + HttpStatusCode.Gone, HttpStatusCode.NotFound -> return null + else -> throw e + } + } + } +} + +/** Data class that contains the request to create, modify elements and delete the given elements */ +data class MapDataChanges( + val creations: Collection = emptyList(), + val modifications: Collection = emptyList(), + val deletions: Collection = emptyList() +) + +sealed interface ElementUpdateAction +data class UpdateElement(val newId: Long, val newVersion: Int) : ElementUpdateAction +data object DeleteElement : ElementUpdateAction diff --git a/app/src/main/java/de/westnordost/streetcomplete/data/osm/mapdata/MapDataApiImpl.kt b/app/src/main/java/de/westnordost/streetcomplete/data/osm/mapdata/MapDataApiImpl.kt deleted file mode 100644 index fc16614240..0000000000 --- a/app/src/main/java/de/westnordost/streetcomplete/data/osm/mapdata/MapDataApiImpl.kt +++ /dev/null @@ -1,257 +0,0 @@ -package de.westnordost.streetcomplete.data.osm.mapdata - -import de.westnordost.osmapi.OsmConnection -import de.westnordost.osmapi.common.errors.OsmApiException -import de.westnordost.osmapi.common.errors.OsmApiReadResponseException -import de.westnordost.osmapi.common.errors.OsmAuthorizationException -import de.westnordost.osmapi.common.errors.OsmConflictException -import de.westnordost.osmapi.common.errors.OsmConnectionException -import de.westnordost.osmapi.common.errors.OsmNotFoundException -import de.westnordost.osmapi.common.errors.OsmQueryTooBigException -import de.westnordost.osmapi.map.data.OsmElement -import de.westnordost.osmapi.map.data.OsmLatLon -import de.westnordost.osmapi.map.data.OsmNode -import de.westnordost.osmapi.map.data.OsmRelation -import de.westnordost.osmapi.map.data.OsmRelationMember -import de.westnordost.osmapi.map.data.OsmWay -import de.westnordost.osmapi.map.handler.MapDataHandler -import de.westnordost.streetcomplete.data.AuthorizationException -import de.westnordost.streetcomplete.data.ConflictException -import de.westnordost.streetcomplete.data.ConnectionException -import de.westnordost.streetcomplete.data.download.QueryTooBigException -import kotlinx.datetime.Instant -import kotlinx.datetime.toJavaInstant -import de.westnordost.osmapi.map.MapDataApi as OsmApiMapDataApi -import de.westnordost.osmapi.map.changes.DiffElement as OsmApiDiffElement -import de.westnordost.osmapi.map.data.BoundingBox as OsmApiBoundingBox -import de.westnordost.osmapi.map.data.Element as OsmApiElement -import de.westnordost.osmapi.map.data.Node as OsmApiNode -import de.westnordost.osmapi.map.data.Relation as OsmApiRelation -import de.westnordost.osmapi.map.data.RelationMember as OsmApiRelationMember -import de.westnordost.osmapi.map.data.Way as OsmApiWay - -class MapDataApiImpl(osm: OsmConnection) : MapDataApi { - - private val api: OsmApiMapDataApi = OsmApiMapDataApi(osm) - - override fun uploadChanges( - changesetId: Long, - changes: MapDataChanges, - ignoreRelationTypes: Set - ) = wrapExceptions { - try { - val handler = UpdatedElementsHandler(ignoreRelationTypes) - api.uploadChanges(changesetId, changes.toOsmApiElements()) { - handler.handle(it.toDiffElement()) - } - val allChangedElements = changes.creations + changes.modifications + changes.deletions - handler.getElementUpdates(allChangedElements) - } catch (e: OsmApiException) { - throw ConflictException(e.message, e) - } - } - - override fun openChangeset(tags: Map): Long = wrapExceptions { - api.openChangeset(tags) - } - - override fun closeChangeset(changesetId: Long) = - try { - wrapExceptions { api.closeChangeset(changesetId) } - } catch (e: OsmNotFoundException) { - throw ConflictException(e.message, e) - } - - override fun getMap( - bounds: BoundingBox, - mutableMapData: MutableMapData, - ignoreRelationTypes: Set - ) = wrapExceptions { - api.getMap( - bounds.toOsmApiBoundingBox(), - MapDataApiHandler(mutableMapData, ignoreRelationTypes) - ) - } - - override fun getWayComplete(id: Long): MapData? = - try { - val result = MutableMapData() - wrapExceptions { api.getWayComplete(id, MapDataApiHandler(result)) } - result - } catch (e: OsmNotFoundException) { - null - } - - override fun getRelationComplete(id: Long): MapData? = - try { - val result = MutableMapData() - wrapExceptions { api.getRelationComplete(id, MapDataApiHandler(result)) } - result - } catch (e: OsmNotFoundException) { - null - } - - override fun getNode(id: Long): Node? = wrapExceptions { - api.getNode(id)?.toNode() - } - - override fun getWay(id: Long): Way? = wrapExceptions { - api.getWay(id)?.toWay() - } - - override fun getRelation(id: Long): Relation? = wrapExceptions { - api.getRelation(id)?.toRelation() - } - - override fun getWaysForNode(id: Long): List = wrapExceptions { - api.getWaysForNode(id).map { it.toWay() } - } - - override fun getRelationsForNode(id: Long): List = wrapExceptions { - api.getRelationsForNode(id).map { it.toRelation() } - } - - override fun getRelationsForWay(id: Long): List = wrapExceptions { - api.getRelationsForWay(id).map { it.toRelation() } - } - - override fun getRelationsForRelation(id: Long): List = wrapExceptions { - api.getRelationsForRelation(id).map { it.toRelation() } - } -} - -private inline fun wrapExceptions(block: () -> T): T = - try { - block() - } catch (e: OsmAuthorizationException) { - throw AuthorizationException(e.message, e) - } catch (e: OsmConflictException) { - throw ConflictException(e.message, e) - } catch (e: OsmQueryTooBigException) { - throw QueryTooBigException(e.message, e) - } catch (e: OsmConnectionException) { - throw ConnectionException(e.message, e) - } catch (e: OsmApiReadResponseException) { - // probably a temporary connection error - throw ConnectionException(e.message, e) - } catch (e: OsmApiException) { - // request timeout is a temporary connection error - throw if (e.errorCode == 408) ConnectionException(e.message, e) else e - } - -/* --------------------------------- Element -> OsmApiElement ----------------------------------- */ - -private fun MapDataChanges.toOsmApiElements(): List = - creations.map { it.toOsmApiElement().apply { isNew = true } } + - modifications.map { it.toOsmApiElement().apply { isModified = true } } + - deletions.map { it.toOsmApiElement().apply { isDeleted = true } } - -private fun Element.toOsmApiElement(): OsmElement = when (this) { - is Node -> toOsmApiNode() - is Way -> toOsmApiWay() - is Relation -> toOsmApiRelation() -} - -private fun Node.toOsmApiNode() = OsmNode( - id, - version, - OsmLatLon(position.latitude, position.longitude), - tags, - null, - Instant.fromEpochMilliseconds(timestampEdited).toJavaInstant() -) - -private fun Way.toOsmApiWay() = OsmWay( - id, - version, - nodeIds, - tags, - null, - Instant.fromEpochMilliseconds(timestampEdited).toJavaInstant() -) - -private fun Relation.toOsmApiRelation() = OsmRelation( - id, - version, - members.map { it.toOsmRelationMember() }, - tags, - null, - Instant.fromEpochMilliseconds(timestampEdited).toJavaInstant() -) - -private fun RelationMember.toOsmRelationMember() = OsmRelationMember( - ref, - role, - type.toOsmElementType() -) - -private fun ElementType.toOsmElementType(): OsmApiElement.Type = when (this) { - ElementType.NODE -> OsmApiElement.Type.NODE - ElementType.WAY -> OsmApiElement.Type.WAY - ElementType.RELATION -> OsmApiElement.Type.RELATION -} - -private fun BoundingBox.toOsmApiBoundingBox() = - OsmApiBoundingBox(min.latitude, min.longitude, max.latitude, max.longitude) - -/* --------------------------------- OsmApiElement -> Element ----------------------------------- */ - -private fun OsmApiNode.toNode() = - Node(id, LatLon(position.latitude, position.longitude), HashMap(tags), version, editedAt.toEpochMilli()) - -private fun OsmApiWay.toWay() = - Way(id, ArrayList(nodeIds), HashMap(tags), version, editedAt.toEpochMilli()) - -private fun OsmApiRelation.toRelation() = Relation( - id, - members.map { it.toRelationMember() }.toMutableList(), - HashMap(tags), - version, - editedAt.toEpochMilli() -) - -private fun OsmApiRelationMember.toRelationMember() = - RelationMember(type.toElementType(), ref, role) - -private fun OsmApiElement.Type.toElementType(): ElementType = when (this) { - OsmApiElement.Type.NODE -> ElementType.NODE - OsmApiElement.Type.WAY -> ElementType.WAY - OsmApiElement.Type.RELATION -> ElementType.RELATION -} - -private fun OsmApiDiffElement.toDiffElement() = DiffElement( - type.toElementType(), - clientId, - serverId, - serverVersion -) - -private fun OsmApiBoundingBox.toBoundingBox() = - BoundingBox(minLatitude, minLongitude, maxLatitude, maxLongitude) - -/* ---------------------------------------------------------------------------------------------- */ - -private class MapDataApiHandler( - val data: MutableMapData, - val ignoreRelationTypes: Set = emptySet() -) : MapDataHandler { - - override fun handle(bounds: OsmApiBoundingBox) { - data.boundingBox = bounds.toBoundingBox() - } - - override fun handle(node: OsmApiNode) { - data.add(node.toNode()) - } - - override fun handle(way: OsmApiWay) { - data.add(way.toWay()) - } - - override fun handle(relation: OsmApiRelation) { - val relationType = relation.tags?.get("type") - if (relationType !in ignoreRelationTypes) { - data.add(relation.toRelation()) - } - } -} diff --git a/app/src/main/java/de/westnordost/streetcomplete/data/osm/mapdata/MapDataApiParser.kt b/app/src/main/java/de/westnordost/streetcomplete/data/osm/mapdata/MapDataApiParser.kt new file mode 100644 index 0000000000..f1a43c384d --- /dev/null +++ b/app/src/main/java/de/westnordost/streetcomplete/data/osm/mapdata/MapDataApiParser.kt @@ -0,0 +1,96 @@ +package de.westnordost.streetcomplete.data.osm.mapdata + +import de.westnordost.streetcomplete.util.ktx.attribute +import de.westnordost.streetcomplete.util.ktx.attributeOrNull +import kotlinx.datetime.Instant +import kotlinx.serialization.SerializationException +import nl.adaptivity.xmlutil.EventType.END_ELEMENT +import nl.adaptivity.xmlutil.EventType.START_ELEMENT +import nl.adaptivity.xmlutil.XmlReader +import nl.adaptivity.xmlutil.xmlStreaming + +class MapDataApiParser { + fun parseMapData(osmXml: String, ignoreRelationTypes: Set): MutableMapData = + xmlStreaming.newReader(osmXml).parseMapData(ignoreRelationTypes) + + fun parseElementUpdates(diffResultXml: String): Map = + xmlStreaming.newReader(diffResultXml).parseElementUpdates() +} + +private fun XmlReader.parseMapData(ignoreRelationTypes: Set): MutableMapData = try { + val result = MutableMapData() + var tags: MutableMap? = null + var nodes: MutableList = ArrayList() + var members: MutableList = ArrayList() + var id: Long? = null + var position: LatLon? = null + var version: Int? = null + var timestamp: Long? = null + + forEach { when (it) { + START_ELEMENT -> when (localName) { + "tag" -> { + if (tags == null) tags = HashMap() + tags!![attribute("k")] = attribute("v") + } + "nd" -> nodes.add(attribute("ref").toLong()) + "member" -> members.add( + RelationMember( + type = ElementType.valueOf(attribute("type").uppercase()), + ref = attribute("ref").toLong(), + role = attribute("role") + ) + ) + "bounds" -> result.boundingBox = BoundingBox( + minLatitude = attribute("minlat").toDouble(), + minLongitude = attribute("minlon").toDouble(), + maxLatitude = attribute("maxlat").toDouble(), + maxLongitude = attribute("maxlon").toDouble() + ) + "node", "way", "relation" -> { + tags = null + id = attribute("id").toLong() + version = attribute("version").toInt() + timestamp = Instant.parse(attribute("timestamp")).toEpochMilliseconds() + when (localName) { + "node" -> position = LatLon(attribute("lat").toDouble(), attribute("lon").toDouble()) + "way" -> nodes = ArrayList() + "relation" -> members = ArrayList() + } + } + } + END_ELEMENT -> when (localName) { + "node" -> result.add(Node(id!!, position!!, tags.orEmpty(), version!!, timestamp!!)) + "way" -> result.add(Way(id!!, nodes, tags.orEmpty(), version!!, timestamp!!)) + "relation" -> if (tags.orEmpty()["type"] !in ignoreRelationTypes) { + result.add(Relation(id!!, members, tags.orEmpty(), version!!, timestamp!!)) + } + } + else -> {} + } } + result +} catch (e: Exception) { throw SerializationException(e) } + +private fun XmlReader.parseElementUpdates(): Map = try { + val result = HashMap() + forEach { + if (it == START_ELEMENT) { + when (localName) { + "node", "way", "relation" -> { + val key = ElementKey( + ElementType.valueOf(localName.uppercase()), + attribute("old_id").toLong() + ) + val newId = attributeOrNull("new_id")?.toLong() + val newVersion = attributeOrNull("new_version")?.toInt() + val action = + if (newId != null && newVersion != null) UpdateElement(newId, newVersion) + else DeleteElement + + result[key] = action + } + } + } + } + result +} catch (e: Exception) { throw SerializationException(e) } diff --git a/app/src/main/java/de/westnordost/streetcomplete/data/osm/mapdata/MapDataApiSerializer.kt b/app/src/main/java/de/westnordost/streetcomplete/data/osm/mapdata/MapDataApiSerializer.kt new file mode 100644 index 0000000000..f0ade4a63f --- /dev/null +++ b/app/src/main/java/de/westnordost/streetcomplete/data/osm/mapdata/MapDataApiSerializer.kt @@ -0,0 +1,74 @@ +package de.westnordost.streetcomplete.data.osm.mapdata + +import de.westnordost.streetcomplete.util.ktx.attribute +import de.westnordost.streetcomplete.util.ktx.endTag +import de.westnordost.streetcomplete.util.ktx.startTag +import kotlinx.datetime.Instant +import nl.adaptivity.xmlutil.XmlWriter +import nl.adaptivity.xmlutil.newWriter +import nl.adaptivity.xmlutil.xmlStreaming + +class MapDataApiSerializer { + fun serializeMapDataChanges(changes: MapDataChanges, changesetId: Long): String { + val buffer = StringBuilder() + xmlStreaming.newWriter(buffer).serializeMapDataChanges(changes, changesetId) + return buffer.toString() + } +} + +private fun XmlWriter.serializeMapDataChanges(changes: MapDataChanges, changesetId: Long) { + startTag("osmChange") + if (changes.creations.isNotEmpty()) { + startTag("create") + changes.creations.forEach { serializeElement(it, changesetId) } + endTag("create") + } + if (changes.modifications.isNotEmpty()) { + startTag("modify") + changes.modifications.forEach { serializeElement(it, changesetId) } + endTag("modify") + } + if (changes.deletions.isNotEmpty()) { + startTag("delete") + changes.deletions.forEach { serializeElement(it, changesetId) } + endTag("delete") + } + endTag("osmChange") +} + +private fun XmlWriter.serializeElement(element: Element, changesetId: Long) { + startTag(element.type.name.lowercase()) + attribute("id", element.id.toString()) + attribute("version", element.version.toString()) + attribute("changeset", changesetId.toString()) + attribute("timestamp", Instant.fromEpochMilliseconds(element.timestampEdited).toString()) + when (element) { + is Node -> { + attribute("lat", element.position.latitude.toString()) + attribute("lon", element.position.longitude.toString()) + } + is Way -> { + for (node in element.nodeIds) { + startTag("nd") + attribute("ref", node.toString()) + endTag("nd") + } + } + is Relation -> { + for (member in element.members) { + startTag("member") + attribute("type", member.type.name.lowercase()) + attribute("ref", member.ref.toString()) + attribute("role", member.role) + endTag("member") + } + } + } + for ((k, v) in element.tags) { + startTag("tag") + attribute("k", k) + attribute("v", v) + endTag("tag") + } + endTag(element.type.name.lowercase()) +} diff --git a/app/src/main/java/de/westnordost/streetcomplete/data/osm/mapdata/MapDataDownloader.kt b/app/src/main/java/de/westnordost/streetcomplete/data/osm/mapdata/MapDataDownloader.kt index 533f0d659e..61b70187ba 100644 --- a/app/src/main/java/de/westnordost/streetcomplete/data/osm/mapdata/MapDataDownloader.kt +++ b/app/src/main/java/de/westnordost/streetcomplete/data/osm/mapdata/MapDataDownloader.kt @@ -1,7 +1,7 @@ package de.westnordost.streetcomplete.data.osm.mapdata import de.westnordost.streetcomplete.ApplicationConstants -import de.westnordost.streetcomplete.data.download.QueryTooBigException +import de.westnordost.streetcomplete.data.QueryTooBigException import de.westnordost.streetcomplete.util.ktx.format import de.westnordost.streetcomplete.util.ktx.nowAsEpochMilliseconds import de.westnordost.streetcomplete.util.logs.Log @@ -12,18 +12,14 @@ import kotlinx.coroutines.yield /** Takes care of downloading all note and osm quests */ class MapDataDownloader( - private val mapDataApi: MapDataApi, + private val mapDataApi: MapDataApiClient, private val mapDataController: MapDataController ) { suspend fun download(bbox: BoundingBox) = withContext(Dispatchers.IO) { val time = nowAsEpochMilliseconds() - val mapData = MutableMapData() val expandedBBox = bbox.enlargedBy(ApplicationConstants.QUEST_FILTER_PADDING) - getMapAndHandleTooBigQuery(expandedBBox, mapData) - /* The map data might be filled with several bboxes one after another if the download is - split up in several, so lets set the bbox back to the bbox of the complete download */ - mapData.boundingBox = expandedBBox + val mapData = getMapAndHandleTooBigQuery(expandedBBox) val seconds = (nowAsEpochMilliseconds() - time) / 1000.0 Log.i(TAG, "Downloaded ${mapData.nodes.size} nodes, ${mapData.ways.size} ways and ${mapData.relations.size} relations in ${seconds.format(1)}s") @@ -33,13 +29,16 @@ class MapDataDownloader( mapDataController.putAllForBBox(bbox, mapData) } - private fun getMapAndHandleTooBigQuery(bounds: BoundingBox, mutableMapData: MutableMapData) { + private suspend fun getMapAndHandleTooBigQuery(bounds: BoundingBox): MutableMapData { try { - mapDataApi.getMap(bounds, mutableMapData, ApplicationConstants.IGNORED_RELATION_TYPES) + return mapDataApi.getMap(bounds, ApplicationConstants.IGNORED_RELATION_TYPES) } catch (e: QueryTooBigException) { + val mapData = MutableMapData() for (subBounds in bounds.splitIntoFour()) { - getMapAndHandleTooBigQuery(subBounds, mutableMapData) + mapData.addAll(getMapAndHandleTooBigQuery(subBounds)) } + mapData.boundingBox = bounds + return mapData } } diff --git a/app/src/main/java/de/westnordost/streetcomplete/data/osm/mapdata/MapDataUpdates.kt b/app/src/main/java/de/westnordost/streetcomplete/data/osm/mapdata/MapDataUpdates.kt new file mode 100644 index 0000000000..5130ed1a2b --- /dev/null +++ b/app/src/main/java/de/westnordost/streetcomplete/data/osm/mapdata/MapDataUpdates.kt @@ -0,0 +1,94 @@ +package de.westnordost.streetcomplete.data.osm.mapdata + +/** Data class that contains the map data updates (updated elements, deleted elements, elements + * whose id have been updated) after the modifications have been uploaded */ +data class MapDataUpdates( + val updated: Collection = emptyList(), + val deleted: Collection = emptyList(), + val idUpdates: Collection = emptyList() +) + +data class ElementIdUpdate( + val elementType: ElementType, + val oldElementId: Long, + val newElementId: Long +) + +fun createMapDataUpdates( + elements: Collection, + updates: Map, + ignoreRelationTypes: Set = emptySet() +): MapDataUpdates { + val updatedElements = mutableListOf() + val deletedElementKeys = mutableListOf() + val idUpdates = mutableListOf() + + for (element in elements) { + if (element is Relation && element.tags["type"] in ignoreRelationTypes) continue + + val newElement = element.update(updates) + if (newElement == null) { + deletedElementKeys.add(element.key) + } else if (newElement !== element) { + updatedElements.add(newElement) + if (element.id != newElement.id) { + idUpdates.add(ElementIdUpdate(element.type, element.id, newElement.id)) + } + } + } + + return MapDataUpdates(updatedElements, deletedElementKeys, idUpdates) +} + +/** + * Apply the given updates to this element. + * + * @return null if the element was deleted, this if nothing was changed or an updated element + * if anything way changed, e.g. the element's version or id, but also if any way node or + * relation member('s id) was changed */ +private fun Element.update(updates: Map): Element? { + val update = updates[key] + if (update is DeleteElement) return null + val u = update as UpdateElement? // kotlin doesn't infer this + return when (this) { + is Node -> update(u) + is Relation -> update(u, updates) + is Way -> update(u, updates) + } +} + +private fun Node.update(update: UpdateElement?): Node { + return if (update != null) copy(id = update.newId, version = update.newVersion) else this +} + +private fun Way.update(update: UpdateElement?, updates: Map): Way { + val newNodeIds = nodeIds.mapNotNull { nodeId -> + when (val nodeUpdate = updates[ElementKey(ElementType.NODE, nodeId)]) { + DeleteElement -> null + is UpdateElement -> nodeUpdate.newId + null -> nodeId + } + } + if (newNodeIds == nodeIds && update == null) return this + return copy( + id = update?.newId ?: id, + version = update?.newVersion ?: version, + nodeIds = newNodeIds + ) +} + +private fun Relation.update(update: UpdateElement?, updates: Map): Relation { + val newMembers = members.mapNotNull { member -> + when (val memberUpdate = updates[ElementKey(member.type, member.ref)]) { + DeleteElement -> null + is UpdateElement -> member.copy(ref = memberUpdate.newId) + null -> member + } + } + if (newMembers == members && update == null) return this + return copy( + id = update?.newId ?: id, + version = update?.newVersion ?: version, + members = newMembers + ) +} diff --git a/app/src/main/java/de/westnordost/streetcomplete/data/osm/mapdata/MutableMapData.kt b/app/src/main/java/de/westnordost/streetcomplete/data/osm/mapdata/MutableMapData.kt index 9d1ab0b9d6..90c632c304 100644 --- a/app/src/main/java/de/westnordost/streetcomplete/data/osm/mapdata/MutableMapData.kt +++ b/app/src/main/java/de/westnordost/streetcomplete/data/osm/mapdata/MutableMapData.kt @@ -1,14 +1,18 @@ package de.westnordost.streetcomplete.data.osm.mapdata -open class MutableMapData() : MapData { +open class MutableMapData( + nodes: Collection = emptyList(), + ways: Collection = emptyList(), + relations: Collection = emptyList() +) : MapData { constructor(other: Iterable) : this() { addAll(other) } - protected val nodesById: MutableMap = mutableMapOf() - protected val waysById: MutableMap = mutableMapOf() - protected val relationsById: MutableMap = mutableMapOf() + private val nodesById: MutableMap = nodes.associateByTo(HashMap()) { it.id } + private val waysById: MutableMap = ways.associateByTo(HashMap()) { it.id } + private val relationsById: MutableMap = relations.associateByTo(HashMap()) { it.id } override var boundingBox: BoundingBox? = null override val nodes get() = nodesById.values diff --git a/app/src/main/java/de/westnordost/streetcomplete/data/osm/mapdata/RemoteMapDataRepository.kt b/app/src/main/java/de/westnordost/streetcomplete/data/osm/mapdata/RemoteMapDataRepository.kt new file mode 100644 index 0000000000..680908e3b8 --- /dev/null +++ b/app/src/main/java/de/westnordost/streetcomplete/data/osm/mapdata/RemoteMapDataRepository.kt @@ -0,0 +1,21 @@ +package de.westnordost.streetcomplete.data.osm.mapdata + +import kotlinx.coroutines.runBlocking + +/** Blocking wrapper around MapDataApiClient + * + * TODO: In the long term, MapDataRepository itself should have a suspending interface, however + * this is a quite far-reaching refactor, as MapDataRepository is used by quite some + * blocking code. E.g. MapDataController and MapDataWithEditsSource should then also + * suspend. */ +class RemoteMapDataRepository(private val mapDataApiClient: MapDataApiClient) : MapDataRepository { + override fun getNode(id: Long) = runBlocking { mapDataApiClient.getNode(id) } + override fun getWay(id: Long) = runBlocking { mapDataApiClient.getWay(id) } + override fun getRelation(id: Long) = runBlocking { mapDataApiClient.getRelation(id) } + override fun getWayComplete(id: Long) = runBlocking { mapDataApiClient.getWayComplete(id) } + override fun getRelationComplete(id: Long) = runBlocking { mapDataApiClient.getRelationComplete(id) } + override fun getWaysForNode(id: Long) = runBlocking { mapDataApiClient.getWaysForNode(id) } + override fun getRelationsForNode(id: Long) = runBlocking { mapDataApiClient.getRelationsForNode(id) } + override fun getRelationsForWay(id: Long) = runBlocking { mapDataApiClient.getRelationsForWay(id) } + override fun getRelationsForRelation(id: Long) = runBlocking { mapDataApiClient.getRelationsForRelation(id) } +} diff --git a/app/src/main/java/de/westnordost/streetcomplete/data/osm/mapdata/UpdatedElementsHandler.kt b/app/src/main/java/de/westnordost/streetcomplete/data/osm/mapdata/UpdatedElementsHandler.kt deleted file mode 100644 index aad92ff8c6..0000000000 --- a/app/src/main/java/de/westnordost/streetcomplete/data/osm/mapdata/UpdatedElementsHandler.kt +++ /dev/null @@ -1,79 +0,0 @@ -package de.westnordost.streetcomplete.data.osm.mapdata - -/** Reads the answer of an update map call on the OSM API. */ -class UpdatedElementsHandler(val ignoreRelationTypes: Set = emptySet()) { - private val nodeDiffs: MutableMap = mutableMapOf() - private val wayDiffs: MutableMap = mutableMapOf() - private val relationDiffs: MutableMap = mutableMapOf() - - fun handle(d: DiffElement) { - when (d.type) { - ElementType.NODE -> nodeDiffs[d.clientId] = d - ElementType.WAY -> wayDiffs[d.clientId] = d - ElementType.RELATION -> relationDiffs[d.clientId] = d - } - } - - fun getElementUpdates(elements: Collection): MapDataUpdates { - val updatedElements = mutableListOf() - val deletedElementKeys = mutableListOf() - val idUpdates = mutableListOf() - for (element in elements) { - if (element is Relation && element.tags["type"] in ignoreRelationTypes) { - continue - } - val diff = getDiff(element.type, element.id) ?: continue - if (diff.serverId != null && diff.serverVersion != null) { - updatedElements.add(createUpdatedElement(element, diff.serverId, diff.serverVersion)) - } else { - deletedElementKeys.add(ElementKey(diff.type, diff.clientId)) - } - if (diff.clientId != diff.serverId && diff.serverId != null) { - idUpdates.add(ElementIdUpdate(diff.type, diff.clientId, diff.serverId)) - } - } - return MapDataUpdates(updatedElements, deletedElementKeys, idUpdates) - } - - private fun getDiff(type: ElementType, id: Long): DiffElement? = when (type) { - ElementType.NODE -> nodeDiffs[id] - ElementType.WAY -> wayDiffs[id] - ElementType.RELATION -> relationDiffs[id] - } - - private fun createUpdatedElement(element: Element, newId: Long, newVersion: Int): Element = - when (element) { - is Node -> createUpdatedNode(element, newId, newVersion) - is Way -> createUpdatedWay(element, newId, newVersion) - is Relation -> createUpdatedRelation(element, newId, newVersion) - } - - private fun createUpdatedNode(node: Node, newId: Long, newVersion: Int): Node = - Node(newId, node.position, HashMap(node.tags), newVersion, node.timestampEdited) - - private fun createUpdatedWay(way: Way, newId: Long, newVersion: Int): Way { - val newNodeIds = ArrayList(way.nodeIds.size) - for (nodeId in way.nodeIds) { - val diff = nodeDiffs[nodeId] - if (diff == null) { - newNodeIds.add(nodeId) - } else if (diff.serverId != null) { - newNodeIds.add(diff.serverId) - } - } - return Way(newId, newNodeIds, HashMap(way.tags), newVersion, way.timestampEdited) - } - - private fun createUpdatedRelation(relation: Relation, newId: Long, newVersion: Int): Relation { - val newRelationMembers = ArrayList(relation.members.size) - for (member in relation.members) { - val diff = getDiff(member.type, member.ref) - if (diff == null) { - newRelationMembers.add(RelationMember(member.type, member.ref, member.role)) - } else if (diff.serverId != null) { - newRelationMembers.add(RelationMember(member.type, diff.serverId, member.role)) - } - } - return Relation(newId, newRelationMembers, HashMap(relation.tags), newVersion, relation.timestampEdited) - } -} diff --git a/app/src/main/java/de/westnordost/streetcomplete/data/osmnotes/AvatarsDownloader.kt b/app/src/main/java/de/westnordost/streetcomplete/data/osmnotes/AvatarsDownloader.kt index 939871ab5e..0aceaf8c81 100644 --- a/app/src/main/java/de/westnordost/streetcomplete/data/osmnotes/AvatarsDownloader.kt +++ b/app/src/main/java/de/westnordost/streetcomplete/data/osmnotes/AvatarsDownloader.kt @@ -1,6 +1,6 @@ package de.westnordost.streetcomplete.data.osmnotes -import de.westnordost.osmapi.user.UserApi +import de.westnordost.streetcomplete.data.user.UserApiClient import de.westnordost.streetcomplete.util.ktx.format import de.westnordost.streetcomplete.util.ktx.nowAsEpochMilliseconds import de.westnordost.streetcomplete.util.logs.Log @@ -15,7 +15,7 @@ import java.io.File /** Downloads and stores the OSM avatars of users */ class AvatarsDownloader( private val httpClient: HttpClient, - private val userApi: UserApi, + private val userApi: UserApiClient, private val cacheDir: File ) { @@ -36,7 +36,7 @@ class AvatarsDownloader( Log.i(TAG, "Downloaded ${userIds.size} avatar images in ${seconds.format(1)}s") } - private fun getProfileImageUrl(userId: Long): String? = + private suspend fun getProfileImageUrl(userId: Long): String? = try { userApi.get(userId)?.profileImageUrl } catch (e: Exception) { diff --git a/app/src/main/java/de/westnordost/streetcomplete/data/osmnotes/NotesApi.kt b/app/src/main/java/de/westnordost/streetcomplete/data/osmnotes/NotesApi.kt deleted file mode 100644 index 39272e1aa0..0000000000 --- a/app/src/main/java/de/westnordost/streetcomplete/data/osmnotes/NotesApi.kt +++ /dev/null @@ -1,68 +0,0 @@ -package de.westnordost.streetcomplete.data.osmnotes - -import de.westnordost.streetcomplete.data.AuthorizationException -import de.westnordost.streetcomplete.data.ConflictException -import de.westnordost.streetcomplete.data.ConnectionException -import de.westnordost.streetcomplete.data.download.QueryTooBigException -import de.westnordost.streetcomplete.data.osm.mapdata.BoundingBox -import de.westnordost.streetcomplete.data.osm.mapdata.LatLon - -/** - * Creates, comments, closes, reopens and search for notes. - * All interactions with this class require an OsmConnection with a logged in user. - */ -interface NotesApi { - /** - * Create a new note at the given location - * - * @param pos position of the note. - * @param text text for the new note. Must not be empty. - * - * @throws AuthorizationException if this application is not authorized to write notes - * (Permission.WRITE_NOTES) - * @throws ConnectionException if a temporary network connection problem occurs - * - * @return the new note - */ - fun create(pos: LatLon, text: String): Note - - /** - * @param id id of the note - * @param text comment to be added to the note. Must not be empty - * - * @throws ConflictException if the note has already been closed or doesn't exist (anymore). - * @throws AuthorizationException if this application is not authorized to write notes - * (Permission.WRITE_NOTES) - * @throws ConnectionException if a temporary network connection problem occurs - * - * @return the updated commented note - */ - fun comment(id: Long, text: String): Note - - /** - * @param id id of the note - * - * @throws ConnectionException if a temporary network connection problem occurs - * - * @return the note with the given id. null if the note with that id does not exist (anymore). - */ - fun get(id: Long): Note? - - /** - * Retrieve those notes in the given area that match the given search string - * - * @param bounds the area within the notes should be queried. This is usually limited at 25 - * square degrees. Check the server capabilities. - * @param limit number of entries returned at maximum. Any value between 1 and 10000 - * @param hideClosedNoteAfter number of days until a closed note should not be shown anymore. - * -1 means that all notes should be returned, 0 that only open notes - * are returned. - * - * @throws QueryTooBigException if the bounds area is too large - * @throws IllegalArgumentException if the bounds cross the 180th meridian - * @throws ConnectionException if a temporary network connection problem occurs - * - * @return the incoming notes - */ - fun getAll(bounds: BoundingBox, limit: Int, hideClosedNoteAfter: Int): List -} diff --git a/app/src/main/java/de/westnordost/streetcomplete/data/osmnotes/NotesApiClient.kt b/app/src/main/java/de/westnordost/streetcomplete/data/osmnotes/NotesApiClient.kt new file mode 100644 index 0000000000..186463e8ec --- /dev/null +++ b/app/src/main/java/de/westnordost/streetcomplete/data/osmnotes/NotesApiClient.kt @@ -0,0 +1,142 @@ +package de.westnordost.streetcomplete.data.osmnotes + +import de.westnordost.streetcomplete.data.ConnectionException +import de.westnordost.streetcomplete.data.QueryTooBigException +import de.westnordost.streetcomplete.data.osm.mapdata.BoundingBox +import de.westnordost.streetcomplete.data.osm.mapdata.LatLon +import de.westnordost.streetcomplete.data.ConflictException +import de.westnordost.streetcomplete.data.AuthorizationException +import de.westnordost.streetcomplete.data.osm.mapdata.toOsmApiString +import de.westnordost.streetcomplete.data.user.UserLoginSource +import de.westnordost.streetcomplete.data.wrapApiClientExceptions +import de.westnordost.streetcomplete.util.ktx.format +import io.ktor.client.HttpClient +import io.ktor.client.call.body +import io.ktor.client.plugins.ClientRequestException +import io.ktor.client.plugins.expectSuccess +import io.ktor.client.request.bearerAuth +import io.ktor.client.request.get +import io.ktor.client.request.parameter +import io.ktor.client.request.post +import io.ktor.http.HttpStatusCode + +/** + * Creates, comments, closes, reopens and search for notes. + */ +class NotesApiClient( + private val httpClient: HttpClient, + private val baseUrl: String, + private val userLoginSource: UserLoginSource, + private val notesApiParser: NotesApiParser +) { + /** + * Create a new note at the given location + * + * @param pos position of the note. + * @param text text for the new note. Must not be empty. + * + * @throws AuthorizationException if this application is not authorized to write notes + * (scope "write_notes") + * @throws ConnectionException if a temporary network connection problem occurs + * + * @return the new note + */ + suspend fun create(pos: LatLon, text: String): Note = wrapApiClientExceptions { + val response = httpClient.post(baseUrl + "notes") { + userLoginSource.accessToken?.let { bearerAuth(it) } + parameter("lat", pos.latitude.format(7)) + parameter("lon", pos.longitude.format(7)) + parameter("text", text) + expectSuccess = true + } + return notesApiParser.parseNotes(response.body()).single() + } + + /** + * @param id id of the note + * @param text comment to be added to the note. Must not be empty + * + * @throws ConflictException if the note has already been closed or doesn't exist (anymore). + * @throws AuthorizationException if this application is not authorized to write notes + * (scope "write_notes") + * @throws ConnectionException if a temporary network connection problem occurs + * + * @return the updated commented note + */ + suspend fun comment(id: Long, text: String): Note = wrapApiClientExceptions { + try { + val response = httpClient.post(baseUrl + "notes/$id/comment") { + userLoginSource.accessToken?.let { bearerAuth(it) } + parameter("text", text) + expectSuccess = true + } + return notesApiParser.parseNotes(response.body()).single() + } catch (e: ClientRequestException) { + when (e.response.status) { + // hidden by moderator, does not exist (yet), has already been closed + HttpStatusCode.Gone, HttpStatusCode.NotFound, HttpStatusCode.Conflict -> { + throw ConflictException(e.message, e) + } + else -> throw e + } + } + } + + /** + * @param id id of the note + * + * @throws ConnectionException if a temporary network connection problem occurs + * + * @return the note with the given id. null if the note with that id does not exist (anymore). + */ + suspend fun get(id: Long): Note? = wrapApiClientExceptions { + try { + val response = httpClient.get(baseUrl + "notes/$id") { expectSuccess = true } + val body = response.body() + return notesApiParser.parseNotes(body).singleOrNull() + } catch (e: ClientRequestException) { + when (e.response.status) { + // hidden by moderator, does not exist (yet) + HttpStatusCode.Gone, HttpStatusCode.NotFound -> return null + else -> throw e + } + } + } + + /** + * Retrieve all open notes in the given area + * + * @param bounds the area within the notes should be queried. This is usually limited at 25 + * square degrees. Check the server capabilities. + * @param limit number of entries returned at maximum. Any value between 1 and 10000 + * + * @throws QueryTooBigException if the bounds area or the limit is too large + * @throws IllegalArgumentException if the bounds cross the 180th meridian + * @throws ConnectionException if a temporary network connection problem occurs + * + * @return the incoming notes + */ + suspend fun getAllOpen(bounds: BoundingBox, limit: Int? = null): List = wrapApiClientExceptions { + if (bounds.crosses180thMeridian) { + throw IllegalArgumentException("Bounding box crosses 180th meridian") + } + + try { + val response = httpClient.get(baseUrl + "notes") { + userLoginSource.accessToken?.let { bearerAuth(it) } + parameter("bbox", bounds.toOsmApiString()) + parameter("limit", limit) + parameter("closed", 0) + expectSuccess = true + } + val body = response.body() + return notesApiParser.parseNotes(body) + } catch (e: ClientRequestException) { + if (e.response.status == HttpStatusCode.BadRequest) { + throw QueryTooBigException(e.message, e) + } else { + throw e + } + } + } +} diff --git a/app/src/main/java/de/westnordost/streetcomplete/data/osmnotes/NotesApiImpl.kt b/app/src/main/java/de/westnordost/streetcomplete/data/osmnotes/NotesApiImpl.kt deleted file mode 100644 index a10fbe7e9c..0000000000 --- a/app/src/main/java/de/westnordost/streetcomplete/data/osmnotes/NotesApiImpl.kt +++ /dev/null @@ -1,109 +0,0 @@ -package de.westnordost.streetcomplete.data.osmnotes - -import de.westnordost.osmapi.OsmConnection -import de.westnordost.osmapi.common.errors.OsmApiException -import de.westnordost.osmapi.common.errors.OsmApiReadResponseException -import de.westnordost.osmapi.common.errors.OsmAuthorizationException -import de.westnordost.osmapi.common.errors.OsmConflictException -import de.westnordost.osmapi.common.errors.OsmConnectionException -import de.westnordost.osmapi.common.errors.OsmNotFoundException -import de.westnordost.osmapi.common.errors.OsmQueryTooBigException -import de.westnordost.osmapi.map.data.OsmLatLon -import de.westnordost.streetcomplete.data.AuthorizationException -import de.westnordost.streetcomplete.data.ConflictException -import de.westnordost.streetcomplete.data.ConnectionException -import de.westnordost.streetcomplete.data.download.QueryTooBigException -import de.westnordost.streetcomplete.data.osm.mapdata.BoundingBox -import de.westnordost.streetcomplete.data.osm.mapdata.LatLon -import de.westnordost.streetcomplete.data.user.User -import de.westnordost.osmapi.map.data.BoundingBox as OsmApiBoundingBox -import de.westnordost.osmapi.notes.Note as OsmApiNote -import de.westnordost.osmapi.notes.NoteComment as OsmApiNoteComment -import de.westnordost.osmapi.notes.NotesApi as OsmApiNotesApi -import de.westnordost.osmapi.user.User as OsmApiUser - -class NotesApiImpl(osm: OsmConnection) : NotesApi { - private val api: OsmApiNotesApi = OsmApiNotesApi(osm) - - override fun create(pos: LatLon, text: String): Note = wrapExceptions { - api.create(OsmLatLon(pos.latitude, pos.longitude), text).toNote() - } - - override fun comment(id: Long, text: String): Note = - try { - wrapExceptions { api.comment(id, text).toNote() } - } catch (e: OsmNotFoundException) { - // someone else already closed the note -> our contribution is probably worthless - throw ConflictException(e.message, e) - } - - override fun get(id: Long): Note? = wrapExceptions { api.get(id)?.toNote() } - - override fun getAll(bounds: BoundingBox, limit: Int, hideClosedNoteAfter: Int) = wrapExceptions { - val notes = ArrayList() - api.getAll( - OsmApiBoundingBox( - bounds.min.latitude, bounds.min.longitude, - bounds.max.latitude, bounds.max.longitude - ), - null, - { notes.add(it.toNote()) }, - limit, - hideClosedNoteAfter - ) - notes - } -} - -private inline fun wrapExceptions(block: () -> T): T = - try { - block() - } catch (e: OsmAuthorizationException) { - throw AuthorizationException(e.message, e) - } catch (e: OsmConflictException) { - throw ConflictException(e.message, e) - } catch (e: OsmQueryTooBigException) { - throw QueryTooBigException(e.message, e) - } catch (e: OsmConnectionException) { - throw ConnectionException(e.message, e) - } catch (e: OsmApiReadResponseException) { - // probably a temporary connection error - throw ConnectionException(e.message, e) - } catch (e: OsmApiException) { - // request timeout is a temporary connection error - throw if (e.errorCode == 408) ConnectionException(e.message, e) else e - } - -private fun OsmApiNote.toNote() = Note( - LatLon(position.latitude, position.longitude), - id, - createdAt.toEpochMilli(), - closedAt?.toEpochMilli(), - status.toNoteStatus(), - comments.map { it.toNoteComment() } -) - -private fun OsmApiNote.Status.toNoteStatus() = when (this) { - OsmApiNote.Status.OPEN -> Note.Status.OPEN - OsmApiNote.Status.CLOSED -> Note.Status.CLOSED - OsmApiNote.Status.HIDDEN -> Note.Status.HIDDEN - else -> throw NoSuchFieldError() -} - -private fun OsmApiNoteComment.toNoteComment() = NoteComment( - date.toEpochMilli(), - action.toNoteCommentAction(), - text, - user?.toUser() -) - -private fun OsmApiNoteComment.Action.toNoteCommentAction() = when (this) { - OsmApiNoteComment.Action.OPENED -> NoteComment.Action.OPENED - OsmApiNoteComment.Action.COMMENTED -> NoteComment.Action.COMMENTED - OsmApiNoteComment.Action.CLOSED -> NoteComment.Action.CLOSED - OsmApiNoteComment.Action.REOPENED -> NoteComment.Action.REOPENED - OsmApiNoteComment.Action.HIDDEN -> NoteComment.Action.HIDDEN - else -> throw NoSuchFieldError() -} - -private fun OsmApiUser.toUser() = User(id, displayName) diff --git a/app/src/main/java/de/westnordost/streetcomplete/data/osmnotes/NotesApiParser.kt b/app/src/main/java/de/westnordost/streetcomplete/data/osmnotes/NotesApiParser.kt new file mode 100644 index 0000000000..3d9eee3f71 --- /dev/null +++ b/app/src/main/java/de/westnordost/streetcomplete/data/osmnotes/NotesApiParser.kt @@ -0,0 +1,93 @@ +package de.westnordost.streetcomplete.data.osmnotes + +import de.westnordost.streetcomplete.data.osm.mapdata.LatLon +import de.westnordost.streetcomplete.data.user.User +import de.westnordost.streetcomplete.util.ktx.attribute +import kotlinx.datetime.LocalDate +import kotlinx.datetime.LocalTime +import kotlinx.datetime.format.DateTimeComponents +import kotlinx.datetime.format.char +import kotlinx.serialization.SerializationException +import nl.adaptivity.xmlutil.EventType.* +import nl.adaptivity.xmlutil.XmlReader +import nl.adaptivity.xmlutil.xmlStreaming + +class NotesApiParser { + fun parseNotes(osmXml: String): List = + xmlStreaming.newReader(osmXml).parseNotes() +} + +private fun XmlReader.parseNotes(): List = try { + val result = ArrayList() + + var note: ApiNote? = null + var comment: ApiNoteComment? = null + val names = ArrayList() + + forEach { when (it) { + START_ELEMENT -> { + when (localName) { + "note" -> note = ApiNote(LatLon(attribute("lat").toDouble(), attribute("lon").toDouble())) + "comment" -> comment = ApiNoteComment() + } + names.add(localName) + } + TEXT -> when (names.lastOrNull()) { + // note + "id" -> note?.id = text.toLong() + "date_created" -> note?.timestampCreated = parseTimestamp(text) + "date_closed" -> note?.timestampClosed = parseTimestamp(text) + "status" -> note?.status = Note.Status.valueOf(text.uppercase()) + // comment + "date" -> comment?.date = parseTimestamp(text) + "action" -> comment?.action = NoteComment.Action.valueOf(text.uppercase()) + "text" -> comment?.text = text + "uid" -> comment?.uid = text.toLong() + "user" -> comment?.user = text + } + END_ELEMENT -> { + when (localName) { + "note" -> { + val n = note!! + result.add(Note(n.position, n.id!!, n.timestampCreated!!, n.timestampClosed, n.status!!, n.comments)) + } + "comment" -> { + val c = comment!! + val cUser = if (c.user != null && c.uid != null) User(c.uid!!, c.user!!) else null + note?.comments?.add(NoteComment(c.date!!, c.action!!, c.text, cUser)) + } + } + names.removeLastOrNull() + } + else -> {} + } } + result +} catch (e: Exception) { throw SerializationException(e) } + +private data class ApiNote( + val position: LatLon, + var id: Long? = null, + var timestampCreated: Long? = null, + var timestampClosed: Long? = null, + var status: Note.Status? = null, + val comments: MutableList = ArrayList(), +) + +private data class ApiNoteComment( + var date: Long? = null, + var action: NoteComment.Action? = null, + var text: String? = null, + var uid: Long? = null, + var user: String? = null, +) + +private val dateFormat = DateTimeComponents.Format { + date(LocalDate.Formats.ISO) + char(' ') + time(LocalTime.Formats.ISO) + char(' ') + timeZoneId() +} + +private fun parseTimestamp(date: String): Long = + dateFormat.parse(date).toInstantUsingOffset().toEpochMilliseconds() diff --git a/app/src/main/java/de/westnordost/streetcomplete/data/osmnotes/NotesDownloader.kt b/app/src/main/java/de/westnordost/streetcomplete/data/osmnotes/NotesDownloader.kt index 2ef54e6962..1202a076a1 100644 --- a/app/src/main/java/de/westnordost/streetcomplete/data/osmnotes/NotesDownloader.kt +++ b/app/src/main/java/de/westnordost/streetcomplete/data/osmnotes/NotesDownloader.kt @@ -8,16 +8,16 @@ import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.withContext import kotlinx.coroutines.yield -/** Takes care of downloading notes and referenced avatar pictures into persistent storage */ +/** Takes care of downloading notes into persistent storage */ class NotesDownloader( - private val notesApi: NotesApi, + private val notesApi: NotesApiClient, private val noteController: NoteController ) { - suspend fun download(bbox: BoundingBox) = withContext(Dispatchers.IO) { + suspend fun download(bbox: BoundingBox) { val time = nowAsEpochMilliseconds() val notes = notesApi - .getAll(bbox, 10000, 0) + .getAllOpen(bbox, 10000) // exclude invalid notes (#1338) .filter { it.comments.isNotEmpty() } @@ -26,7 +26,7 @@ class NotesDownloader( yield() - noteController.putAllForBBox(bbox, notes) + withContext(Dispatchers.IO) { noteController.putAllForBBox(bbox, notes) } } companion object { diff --git a/app/src/main/java/de/westnordost/streetcomplete/data/osmnotes/NotesModule.kt b/app/src/main/java/de/westnordost/streetcomplete/data/osmnotes/NotesModule.kt index 183a808292..c04c018645 100644 --- a/app/src/main/java/de/westnordost/streetcomplete/data/osmnotes/NotesModule.kt +++ b/app/src/main/java/de/westnordost/streetcomplete/data/osmnotes/NotesModule.kt @@ -12,7 +12,7 @@ val notesModule = module { factory { AvatarsInNotesUpdater(get()) } factory { NoteDao(get()) } factory { NotesDownloader(get(), get()) } - factory { StreetCompleteImageUploader(get(), ApplicationConstants.SC_PHOTO_SERVICE_URL) } + factory { PhotoServiceApiClient(get(), ApplicationConstants.SC_PHOTO_SERVICE_URL) } single { NoteController(get()).apply { diff --git a/app/src/main/java/de/westnordost/streetcomplete/data/osmnotes/PhotoServiceApiClient.kt b/app/src/main/java/de/westnordost/streetcomplete/data/osmnotes/PhotoServiceApiClient.kt new file mode 100644 index 0000000000..f3c863aae3 --- /dev/null +++ b/app/src/main/java/de/westnordost/streetcomplete/data/osmnotes/PhotoServiceApiClient.kt @@ -0,0 +1,83 @@ +package de.westnordost.streetcomplete.data.osmnotes + +import de.westnordost.streetcomplete.data.ConnectionException +import de.westnordost.streetcomplete.data.wrapApiClientExceptions +import io.ktor.client.HttpClient +import io.ktor.client.call.body +import io.ktor.client.plugins.ClientRequestException +import io.ktor.client.plugins.expectSuccess +import io.ktor.client.request.header +import io.ktor.client.request.post +import io.ktor.client.request.setBody +import io.ktor.http.ContentType +import io.ktor.http.HttpStatusCode +import io.ktor.http.contentType +import io.ktor.http.defaultForFile +import io.ktor.http.isSuccess +import io.ktor.util.cio.readChannel +import io.ktor.utils.io.errors.IOException +import kotlinx.serialization.SerialName +import kotlinx.serialization.Serializable +import kotlinx.serialization.json.Json +import java.io.File + +/** Upload and activate a list of image paths to an instance of the + * https://github.com/streetcomplete/sc-photo-service + */ +class PhotoServiceApiClient( + private val httpClient: HttpClient, + private val baseUrl: String +) { + private val json = Json { ignoreUnknownKeys = true } + + /** Upload list of images. + * + * @throws ConnectionException on connection or server error */ + suspend fun upload(imagePaths: List): List = wrapApiClientExceptions { + val imageLinks = ArrayList() + + for (path in imagePaths) { + val file = File(path) + if (!file.exists()) continue + + val response = httpClient.post(baseUrl + "upload.php") { + contentType(ContentType.defaultForFile(file)) + header("Content-Transfer-Encoding", "binary") + setBody(file.readChannel()) + expectSuccess = true + } + + val body = response.body() + val parsedResponse = json.decodeFromString(body) + imageLinks.add(parsedResponse.futureUrl) + } + + return imageLinks + } + + /** Activate the images in the given note. + * + * @throws ConnectionException on connection or server error */ + suspend fun activate(noteId: Long): Unit = wrapApiClientExceptions { + try { + httpClient.post(baseUrl + "activate.php") { + contentType(ContentType.Application.Json) + setBody("{\"osm_note_id\": $noteId}") + expectSuccess = true + } + } catch (e: ClientRequestException) { + if (e.response.status == HttpStatusCode.Gone) { + // it's gone if the note does not exist anymore. That's okay, it should only fail + // if we might want to try again later. + } else { + throw e + } + } + } +} + +@Serializable +private data class PhotoUploadResponse( + @SerialName("future_url") + val futureUrl: String +) diff --git a/app/src/main/java/de/westnordost/streetcomplete/data/osmnotes/StreetCompleteImageUploader.kt b/app/src/main/java/de/westnordost/streetcomplete/data/osmnotes/StreetCompleteImageUploader.kt deleted file mode 100644 index 11640615c2..0000000000 --- a/app/src/main/java/de/westnordost/streetcomplete/data/osmnotes/StreetCompleteImageUploader.kt +++ /dev/null @@ -1,115 +0,0 @@ -package de.westnordost.streetcomplete.data.osmnotes - -import de.westnordost.streetcomplete.data.ConnectionException -import io.ktor.client.HttpClient -import io.ktor.client.call.body -import io.ktor.client.request.header -import io.ktor.client.request.post -import io.ktor.client.request.setBody -import io.ktor.http.ContentType -import io.ktor.http.HttpStatusCode -import io.ktor.http.contentType -import io.ktor.http.defaultForFile -import io.ktor.http.isSuccess -import io.ktor.util.cio.readChannel -import io.ktor.utils.io.errors.IOException -import kotlinx.serialization.SerialName -import kotlinx.serialization.Serializable -import kotlinx.serialization.SerializationException -import kotlinx.serialization.json.Json -import java.io.File - -@Serializable -private data class PhotoUploadResponse( - @SerialName("future_url") - val futureUrl: String -) - -/** Upload and activate a list of image paths to an instance of the - * StreetComplete image hosting service - */ -class StreetCompleteImageUploader( - private val httpClient: HttpClient, - private val baseUrl: String -) { - private val json = Json { - ignoreUnknownKeys = true - } - - /** Upload list of images. - * - * @throws ImageUploadServerException when there was a server error on upload (server error) - * @throws ImageUploadClientException when the server rejected the upload request (client error) - * @throws ConnectionException if it is currently not reachable (no internet etc) */ - suspend fun upload(imagePaths: List): List { - val imageLinks = ArrayList() - - for (path in imagePaths) { - val file = File(path) - if (!file.exists()) continue - - try { - val response = httpClient.post(baseUrl + "upload.php") { - contentType(ContentType.defaultForFile(file)) - header("Content-Transfer-Encoding", "binary") - setBody(file.readChannel()) - } - - val status = response.status - val body = response.body() - if (status.isSuccess()) { - try { - val parsedResponse = json.decodeFromString(body) - imageLinks.add(parsedResponse.futureUrl) - } catch (e: SerializationException) { - throw ImageUploadServerException("Upload Failed: Unexpected response \"$body\"") - } - } else { - if (status.value in 500..599) { - throw ImageUploadServerException("Upload failed: Error code $status, Message: \"$body\"") - } else { - throw ImageUploadClientException("Upload failed: Error code $status, Message: \"$body\"") - } - } - } catch (e: IOException) { - throw ConnectionException("Upload failed", e) - } - } - - return imageLinks - } - - /** Activate the images in the given note. - * @throws ImageUploadServerException when there was a server error on upload (server error) - * @throws ImageUploadClientException when the server rejected the upload request (client error) - * @throws ConnectionException if it is currently not reachable (no internet etc) */ - suspend fun activate(noteId: Long) { - try { - val response = httpClient.post(baseUrl + "activate.php") { - contentType(ContentType.Application.Json) - setBody("{\"osm_note_id\": $noteId}") - } - - val status = response.status - if (status == HttpStatusCode.Gone) { - // it's gone if the note does not exist anymore. That's okay, it should only fail - // if we might want to try again later. - } else if (!status.isSuccess()) { - val error = response.body() - if (status.value in 500..599) { - throw ImageUploadServerException("Error code $status, Message: \"$error\"") - } else { - throw ImageUploadClientException("Error code $status, Message: \"$error\"") - } - } - } catch (e: IOException) { - throw ConnectionException("", e) - } - } -} - -class ImageUploadServerException(message: String? = null, cause: Throwable? = null) : - RuntimeException(message, cause) - -class ImageUploadClientException(message: String? = null, cause: Throwable? = null) : - RuntimeException(message, cause) diff --git a/app/src/main/java/de/westnordost/streetcomplete/data/osmnotes/edits/NoteEditsUploader.kt b/app/src/main/java/de/westnordost/streetcomplete/data/osmnotes/edits/NoteEditsUploader.kt index efe57f5148..b3d89415ee 100644 --- a/app/src/main/java/de/westnordost/streetcomplete/data/osmnotes/edits/NoteEditsUploader.kt +++ b/app/src/main/java/de/westnordost/streetcomplete/data/osmnotes/edits/NoteEditsUploader.kt @@ -2,16 +2,15 @@ package de.westnordost.streetcomplete.data.osmnotes.edits import de.westnordost.streetcomplete.data.ConflictException import de.westnordost.streetcomplete.data.osmnotes.NoteController -import de.westnordost.streetcomplete.data.osmnotes.NotesApi -import de.westnordost.streetcomplete.data.osmnotes.StreetCompleteImageUploader +import de.westnordost.streetcomplete.data.osmnotes.NotesApiClient +import de.westnordost.streetcomplete.data.osmnotes.PhotoServiceApiClient import de.westnordost.streetcomplete.data.osmnotes.deleteImages import de.westnordost.streetcomplete.data.osmnotes.edits.NoteEditAction.COMMENT import de.westnordost.streetcomplete.data.osmnotes.edits.NoteEditAction.CREATE import de.westnordost.streetcomplete.data.osmtracks.Trackpoint -import de.westnordost.streetcomplete.data.osmtracks.TracksApi +import de.westnordost.streetcomplete.data.osmtracks.TracksApiClient import de.westnordost.streetcomplete.data.upload.OnUploadedChangeListener import de.westnordost.streetcomplete.data.user.UserDataSource -import de.westnordost.streetcomplete.util.ktx.truncate import de.westnordost.streetcomplete.util.logs.Log import io.ktor.http.encodeURLPathPart import kotlinx.coroutines.CoroutineName @@ -26,9 +25,9 @@ class NoteEditsUploader( private val noteEditsController: NoteEditsController, private val noteController: NoteController, private val userDataSource: UserDataSource, - private val notesApi: NotesApi, - private val tracksApi: TracksApi, - private val imageUploader: StreetCompleteImageUploader + private val notesApi: NotesApiClient, + private val tracksApi: TracksApiClient, + private val imageUploader: PhotoServiceApiClient ) { var uploadedChangeListener: OnUploadedChangeListener? = null @@ -126,12 +125,12 @@ class NoteEditsUploader( return "" } - private fun uploadAndGetAttachedTrackText( + private suspend fun uploadAndGetAttachedTrackText( trackpoints: List, noteText: String? ): String { if (trackpoints.isEmpty()) return "" - val trackId = tracksApi.create(trackpoints, noteText?.truncate(255)) + val trackId = tracksApi.create(trackpoints, noteText) val encodedUsername = userDataSource.userName!!.encodeURLPathPart() return "\n\nGPS Trace: https://www.openstreetmap.org/user/$encodedUsername/traces/$trackId\n" } diff --git a/app/src/main/java/de/westnordost/streetcomplete/data/osmtracks/TracksApi.kt b/app/src/main/java/de/westnordost/streetcomplete/data/osmtracks/TracksApi.kt deleted file mode 100644 index 3f2c33026e..0000000000 --- a/app/src/main/java/de/westnordost/streetcomplete/data/osmtracks/TracksApi.kt +++ /dev/null @@ -1,26 +0,0 @@ -package de.westnordost.streetcomplete.data.osmtracks - -import de.westnordost.streetcomplete.data.AuthorizationException -import de.westnordost.streetcomplete.data.ConnectionException - -/** - * Creates GPS / GPX trackpoint histories - */ -interface TracksApi { - - /** - * Create a new GPX track history - * - * @param trackpoints history of recorded trackpoints - * @param noteText optional text appended to the track - * - * @throws AuthorizationException if this application is not authorized to write traces - * (Permission.WRITE_GPS_TRACES) - * @throws ConnectionException if a temporary network connection problem occurs - * - * @throws IllegalArgumentException if noteText is longer than 255 characters - * - * @return id of the new track - */ - fun create(trackpoints: List, noteText: String?): Long -} diff --git a/app/src/main/java/de/westnordost/streetcomplete/data/osmtracks/TracksApiClient.kt b/app/src/main/java/de/westnordost/streetcomplete/data/osmtracks/TracksApiClient.kt new file mode 100644 index 0000000000..35bcc56b06 --- /dev/null +++ b/app/src/main/java/de/westnordost/streetcomplete/data/osmtracks/TracksApiClient.kt @@ -0,0 +1,63 @@ +package de.westnordost.streetcomplete.data.osmtracks + +import de.westnordost.streetcomplete.ApplicationConstants +import de.westnordost.streetcomplete.data.ConnectionException +import de.westnordost.streetcomplete.data.AuthorizationException +import de.westnordost.streetcomplete.data.user.UserLoginSource +import de.westnordost.streetcomplete.data.wrapApiClientExceptions +import de.westnordost.streetcomplete.util.ktx.truncate +import io.ktor.client.HttpClient +import io.ktor.client.call.body +import io.ktor.client.plugins.expectSuccess +import io.ktor.client.request.bearerAuth +import io.ktor.client.request.forms.MultiPartFormDataContent +import io.ktor.client.request.forms.formData +import io.ktor.client.request.post +import io.ktor.client.request.setBody +import io.ktor.http.Headers +import io.ktor.http.HttpHeaders +import kotlinx.datetime.Instant + +/** + * Talks with OSM traces API to uploads GPS trackpoints + */ +class TracksApiClient( + private val httpClient: HttpClient, + private val baseUrl: String, + private val userLoginSource: UserLoginSource, + private val tracksSerializer: TracksSerializer +) { + /** + * Upload a list of trackpoints as a GPX + * + * @param trackpoints recorded trackpoints + * @param noteText optional description text + * + * @throws AuthorizationException if not logged in or not not authorized to upload traces + * (scope "write_gpx") + * @throws ConnectionException if a temporary network connection problem occurs + * + * @return id of the uploaded track + */ + suspend fun create(trackpoints: List, noteText: String? = null): Long = wrapApiClientExceptions { + val name = Instant.fromEpochMilliseconds(trackpoints.first().time).toString() + ".gpx" + val description = noteText ?: "Uploaded via ${ApplicationConstants.USER_AGENT}" + val tags = listOf(ApplicationConstants.NAME.lowercase()).joinToString() + val xml = tracksSerializer.serialize(trackpoints) + + val response = httpClient.post(baseUrl + "gpx/create") { + userLoginSource.accessToken?.let { bearerAuth(it) } + setBody(MultiPartFormDataContent(formData { + append("file", xml, Headers.build { + append(HttpHeaders.ContentType, "application/gpx+xml") + append(HttpHeaders.ContentDisposition, "filename=\"$name\"") + }) + append("description", description.truncate(255)) + append("tags", tags) + append("visibility", "identifiable") + })) + expectSuccess = true + } + return response.body().toLong() + } +} diff --git a/app/src/main/java/de/westnordost/streetcomplete/data/osmtracks/TracksApiImpl.kt b/app/src/main/java/de/westnordost/streetcomplete/data/osmtracks/TracksApiImpl.kt deleted file mode 100644 index 3dfdde4ab4..0000000000 --- a/app/src/main/java/de/westnordost/streetcomplete/data/osmtracks/TracksApiImpl.kt +++ /dev/null @@ -1,67 +0,0 @@ -package de.westnordost.streetcomplete.data.osmtracks - -import de.westnordost.osmapi.OsmConnection -import de.westnordost.osmapi.common.errors.OsmApiException -import de.westnordost.osmapi.common.errors.OsmApiReadResponseException -import de.westnordost.osmapi.common.errors.OsmAuthorizationException -import de.westnordost.osmapi.common.errors.OsmConnectionException -import de.westnordost.osmapi.map.data.OsmLatLon -import de.westnordost.osmapi.traces.GpsTraceDetails -import de.westnordost.osmapi.traces.GpsTracesApi -import de.westnordost.osmapi.traces.GpsTrackpoint -import de.westnordost.streetcomplete.ApplicationConstants -import de.westnordost.streetcomplete.data.AuthorizationException -import de.westnordost.streetcomplete.data.ConnectionException -import kotlinx.datetime.Instant -import kotlinx.datetime.LocalDateTime -import kotlinx.datetime.TimeZone -import kotlinx.datetime.toJavaInstant -import kotlinx.datetime.toLocalDateTime - -class TracksApiImpl(osm: OsmConnection) : TracksApi { - private val api: GpsTracesApi = GpsTracesApi(osm) - - override fun create(trackpoints: List, noteText: String?): Long = wrapExceptions { - // Filename is just the start of the track - // https://stackoverflow.com/a/49862573/7718197 - val name = Instant.fromEpochMilliseconds(trackpoints[0].time).toLocalDateTime(TimeZone.UTC).toTrackFilename() - val visibility = GpsTraceDetails.Visibility.IDENTIFIABLE - val description = noteText ?: "Uploaded via ${ApplicationConstants.USER_AGENT}" - val tags = listOf(ApplicationConstants.NAME.lowercase()) - - // Generate history of trackpoints - val history = trackpoints.mapIndexed { idx, it -> - GpsTrackpoint( - OsmLatLon(it.position.latitude, it.position.longitude), - Instant.fromEpochMilliseconds(it.time).toJavaInstant(), - idx == 0, - it.accuracy, - it.elevation - ) - } - - // Finally query the API and return! - api.create(name, visibility, description, tags, history) - } -} - -private inline fun wrapExceptions(block: () -> T): T = - try { - block() - } catch (e: OsmAuthorizationException) { - throw AuthorizationException(e.message, e) - } catch (e: OsmConnectionException) { - throw ConnectionException(e.message, e) - } catch (e: OsmApiReadResponseException) { - // probably a temporary connection error - throw ConnectionException(e.message, e) - } catch (e: OsmApiException) { - // request timeout is a temporary connection error - throw if (e.errorCode == 408) ConnectionException(e.message, e) else e - } - -private fun LocalDateTime.toTrackFilename(): String { - fun Int.f(len: Int): String = toString().padStart(len, '0') - return ("${year.f(4)}_${monthNumber.f(2)}_${dayOfMonth.f(2)}" - + "T${hour.f(2)}_${minute.f(2)}_${second.f(2)}.${nanosecond.f(6)}Z.gpx") -} diff --git a/app/src/main/java/de/westnordost/streetcomplete/data/osmtracks/TracksSerializer.kt b/app/src/main/java/de/westnordost/streetcomplete/data/osmtracks/TracksSerializer.kt new file mode 100644 index 0000000000..46725e62e1 --- /dev/null +++ b/app/src/main/java/de/westnordost/streetcomplete/data/osmtracks/TracksSerializer.kt @@ -0,0 +1,45 @@ +package de.westnordost.streetcomplete.data.osmtracks + +import de.westnordost.streetcomplete.ApplicationConstants +import de.westnordost.streetcomplete.util.ktx.attribute +import de.westnordost.streetcomplete.util.ktx.endTag +import de.westnordost.streetcomplete.util.ktx.startTag +import kotlinx.datetime.Instant +import nl.adaptivity.xmlutil.XmlWriter +import nl.adaptivity.xmlutil.newWriter +import nl.adaptivity.xmlutil.xmlStreaming + +class TracksSerializer { + fun serialize(trackpoints: List): String { + val buffer = StringBuilder() + xmlStreaming.newWriter(buffer).serializeToGpx(trackpoints) + return buffer.toString() + } +} + +private fun XmlWriter.serializeToGpx(trackpoints: List) { + startTag("gpx") + attribute("xmlns", "http://www.topografix.com/GPX/1/0") + attribute("version", "1.0") + attribute("creator", ApplicationConstants.USER_AGENT) + startTag("trk") + startTag("trkseg") + for (trackpoint in trackpoints) { + startTag("trkpt") + attribute("lat", trackpoint.position.latitude.toString()) + attribute("lon", trackpoint.position.longitude.toString()) + startTag("time") + text(Instant.fromEpochMilliseconds(trackpoint.time).toString()) + endTag("time") + startTag("ele") + text(trackpoint.elevation.toString()) + endTag("ele") + startTag("hdop") + text(trackpoint.accuracy.toString()) + endTag("hdop") + endTag("trkpt") + } + endTag("trkseg") + endTag("trk") + endTag("gpx") +} diff --git a/app/src/main/java/de/westnordost/streetcomplete/data/upload/Uploader.kt b/app/src/main/java/de/westnordost/streetcomplete/data/upload/Uploader.kt index 8135a7cb0b..707bdcfbcb 100644 --- a/app/src/main/java/de/westnordost/streetcomplete/data/upload/Uploader.kt +++ b/app/src/main/java/de/westnordost/streetcomplete/data/upload/Uploader.kt @@ -54,7 +54,7 @@ class Uploader( listeners.forEach { it.onStarted() } if (!::bannedInfo.isInitialized) { - bannedInfo = withContext(Dispatchers.IO) { versionIsBannedChecker.get() } + bannedInfo = versionIsBannedChecker.get() } val banned = bannedInfo if (banned is IsBanned) { diff --git a/app/src/main/java/de/westnordost/streetcomplete/data/user/UserApiClient.kt b/app/src/main/java/de/westnordost/streetcomplete/data/user/UserApiClient.kt new file mode 100644 index 0000000000..10de4c73ac --- /dev/null +++ b/app/src/main/java/de/westnordost/streetcomplete/data/user/UserApiClient.kt @@ -0,0 +1,56 @@ +package de.westnordost.streetcomplete.data.user + +import de.westnordost.streetcomplete.data.AuthorizationException +import de.westnordost.streetcomplete.data.ConnectionException +import de.westnordost.streetcomplete.data.wrapApiClientExceptions +import io.ktor.client.HttpClient +import io.ktor.client.call.body +import io.ktor.client.plugins.ClientRequestException +import io.ktor.client.plugins.expectSuccess +import io.ktor.client.request.bearerAuth +import io.ktor.client.request.get +import io.ktor.http.HttpStatusCode + +/** + * Talks with OSM user API + */ +class UserApiClient( + private val httpClient: HttpClient, + private val baseUrl: String, + private val userLoginSource: UserLoginSource, + private val userApiParser: UserApiParser, +) { + /** + * @return the user info of the current user + * + * @throws AuthorizationException if we are not authorized to read user details (scope "read_prefs") + * @throws ConnectionException on connection or server error + */ + suspend fun getMine(): UserInfo = wrapApiClientExceptions { + val response = httpClient.get(baseUrl + "user/details") { + userLoginSource.accessToken?.let { bearerAuth(it) } + expectSuccess = true + } + val body = response.body() + return userApiParser.parseUsers(body).first() + } + + /** + * @param userId id of the user to get the user info for + * @return the user info of the given user. Null if the user does not exist. + * + * @throws ConnectionException on connection or server error + */ + suspend fun get(userId: Long): UserInfo? = wrapApiClientExceptions { + try { + val response = httpClient.get(baseUrl + "user/$userId") { expectSuccess = true } + val body = response.body() + return userApiParser.parseUsers(body).first() + } catch (e: ClientRequestException) { + when (e.response.status) { + HttpStatusCode.Gone, HttpStatusCode.NotFound -> return null + else -> throw e + } + } + } +} diff --git a/app/src/main/java/de/westnordost/streetcomplete/data/user/UserApiParser.kt b/app/src/main/java/de/westnordost/streetcomplete/data/user/UserApiParser.kt new file mode 100644 index 0000000000..d1e4171f09 --- /dev/null +++ b/app/src/main/java/de/westnordost/streetcomplete/data/user/UserApiParser.kt @@ -0,0 +1,40 @@ +package de.westnordost.streetcomplete.data.user + +import de.westnordost.streetcomplete.util.ktx.attribute +import kotlinx.serialization.SerializationException +import nl.adaptivity.xmlutil.EventType.* +import nl.adaptivity.xmlutil.XmlReader +import nl.adaptivity.xmlutil.xmlStreaming + +class UserApiParser { + fun parseUsers(osmXml: String): List = + xmlStreaming.newReader(osmXml).parseUsers() +} + +private fun XmlReader.parseUsers(): List = try { + val result = ArrayList(1) + var id: Long? = null + var displayName: String? = null + var img: String? = null + var unread: Int? = null + var isMessages = false + forEach { when (it) { + START_ELEMENT -> when (localName) { + "user" -> { + id = attribute("id").toLong() + displayName = attribute("display_name") + img = null + unread = null + } + "img" -> img = attribute("href") + "messages" -> isMessages = true + "received" -> if (isMessages) unread = attribute("unread").toInt() + } + END_ELEMENT -> when (localName) { + "user" -> result.add(UserInfo(id!!, displayName!!, img, unread)) + "messages" -> isMessages = false + } + else -> {} + } } + result +} catch (e: Exception) { throw SerializationException(e) } diff --git a/app/src/main/java/de/westnordost/streetcomplete/data/user/UserDataController.kt b/app/src/main/java/de/westnordost/streetcomplete/data/user/UserDataController.kt index dc32c54aba..e1feb20a54 100644 --- a/app/src/main/java/de/westnordost/streetcomplete/data/user/UserDataController.kt +++ b/app/src/main/java/de/westnordost/streetcomplete/data/user/UserDataController.kt @@ -1,6 +1,5 @@ package de.westnordost.streetcomplete.data.user -import de.westnordost.osmapi.user.UserDetails import de.westnordost.streetcomplete.data.preferences.Preferences import de.westnordost.streetcomplete.util.Listeners @@ -19,10 +18,10 @@ class UserDataController(private val prefs: Preferences) : UserDataSource { listeners.forEach { it.onUpdated() } } - fun setDetails(userDetails: UserDetails) { + fun setDetails(userDetails: UserInfo) { prefs.userId = userDetails.id prefs.userName = userDetails.displayName - prefs.userUnreadMessages = userDetails.unreadMessagesCount + userDetails.unreadMessagesCount?.let { prefs.userUnreadMessages } listeners.forEach { it.onUpdated() } } diff --git a/app/src/main/java/de/westnordost/streetcomplete/data/user/UserInfo.kt b/app/src/main/java/de/westnordost/streetcomplete/data/user/UserInfo.kt new file mode 100644 index 0000000000..bff4800266 --- /dev/null +++ b/app/src/main/java/de/westnordost/streetcomplete/data/user/UserInfo.kt @@ -0,0 +1,8 @@ +package de.westnordost.streetcomplete.data.user + +data class UserInfo( + val id: Long, + val displayName: String, + val profileImageUrl: String?, + val unreadMessagesCount: Int? = null, +) diff --git a/app/src/main/java/de/westnordost/streetcomplete/data/user/UserLoginController.kt b/app/src/main/java/de/westnordost/streetcomplete/data/user/UserLoginController.kt index fe0f8b7e97..20ec2c62e8 100644 --- a/app/src/main/java/de/westnordost/streetcomplete/data/user/UserLoginController.kt +++ b/app/src/main/java/de/westnordost/streetcomplete/data/user/UserLoginController.kt @@ -1,11 +1,9 @@ package de.westnordost.streetcomplete.data.user -import de.westnordost.osmapi.OsmConnection import de.westnordost.streetcomplete.data.preferences.Preferences import de.westnordost.streetcomplete.util.Listeners class UserLoginController( - private val osmConnection: OsmConnection, private val prefs: Preferences, ) : UserLoginSource { @@ -18,14 +16,12 @@ class UserLoginController( fun logIn(accessToken: String) { prefs.oAuth2AccessToken = accessToken - osmConnection.oAuthAccessToken = accessToken listeners.forEach { it.onLoggedIn() } } fun logOut() { prefs.oAuth2AccessToken = null prefs.removeOAuth1Data() - osmConnection.oAuthAccessToken = null listeners.forEach { it.onLoggedOut() } } diff --git a/app/src/main/java/de/westnordost/streetcomplete/data/user/UserModule.kt b/app/src/main/java/de/westnordost/streetcomplete/data/user/UserModule.kt index 29ee845fc7..018904676a 100644 --- a/app/src/main/java/de/westnordost/streetcomplete/data/user/UserModule.kt +++ b/app/src/main/java/de/westnordost/streetcomplete/data/user/UserModule.kt @@ -1,6 +1,6 @@ package de.westnordost.streetcomplete.data.user -import de.westnordost.streetcomplete.data.user.oauth.OAuthService +import de.westnordost.streetcomplete.data.user.oauth.OAuthApiClient import org.koin.dsl.module const val OAUTH2_TOKEN_URL = "https://www.openstreetmap.org/oauth2/token" @@ -40,9 +40,9 @@ val userModule = module { single { UserDataController(get()) } single { get() } - single { UserLoginController(get(), get()) } + single { UserLoginController(get()) } single { UserUpdater(get(), get(), get(), get(), get(), get()) } - single { OAuthService(get()) } + single { OAuthApiClient(get()) } } diff --git a/app/src/main/java/de/westnordost/streetcomplete/data/user/UserUpdater.kt b/app/src/main/java/de/westnordost/streetcomplete/data/user/UserUpdater.kt index 4162bc62f2..b721bad333 100644 --- a/app/src/main/java/de/westnordost/streetcomplete/data/user/UserUpdater.kt +++ b/app/src/main/java/de/westnordost/streetcomplete/data/user/UserUpdater.kt @@ -1,9 +1,8 @@ package de.westnordost.streetcomplete.data.user -import de.westnordost.osmapi.user.UserApi import de.westnordost.streetcomplete.data.osmnotes.AvatarsDownloader import de.westnordost.streetcomplete.data.user.statistics.StatisticsController -import de.westnordost.streetcomplete.data.user.statistics.StatisticsDownloader +import de.westnordost.streetcomplete.data.user.statistics.StatisticsApiClient import de.westnordost.streetcomplete.util.Listeners import de.westnordost.streetcomplete.util.logs.Log import kotlinx.coroutines.CoroutineScope @@ -12,9 +11,9 @@ import kotlinx.coroutines.SupervisorJob import kotlinx.coroutines.launch class UserUpdater( - private val userApi: UserApi, + private val userApi: UserApiClient, private val avatarsDownloader: AvatarsDownloader, - private val statisticsDownloader: StatisticsDownloader, + private val statisticsApiClient: StatisticsApiClient, private val userDataController: UserDataController, private val statisticsController: StatisticsController, private val userLoginSource: UserLoginSource @@ -67,7 +66,7 @@ class UserUpdater( private fun updateStatistics(userId: Long) = coroutineScope.launch(Dispatchers.IO) { try { - val statistics = statisticsDownloader.download(userId) + val statistics = statisticsApiClient.get(userId) statisticsController.updateAll(statistics) } catch (e: Exception) { Log.w(TAG, "Unable to download statistics", e) diff --git a/app/src/main/java/de/westnordost/streetcomplete/data/user/oauth/OAuthService.kt b/app/src/main/java/de/westnordost/streetcomplete/data/user/oauth/OAuthApiClient.kt similarity index 76% rename from app/src/main/java/de/westnordost/streetcomplete/data/user/oauth/OAuthService.kt rename to app/src/main/java/de/westnordost/streetcomplete/data/user/oauth/OAuthApiClient.kt index 91535d9228..76945762a9 100644 --- a/app/src/main/java/de/westnordost/streetcomplete/data/user/oauth/OAuthService.kt +++ b/app/src/main/java/de/westnordost/streetcomplete/data/user/oauth/OAuthApiClient.kt @@ -1,7 +1,12 @@ package de.westnordost.streetcomplete.data.user.oauth +import de.westnordost.streetcomplete.data.ConnectionException +import de.westnordost.streetcomplete.data.wrapApiClientExceptions import io.ktor.client.HttpClient import io.ktor.client.call.body +import io.ktor.client.plugins.ClientRequestException +import io.ktor.client.plugins.expectSuccess +import io.ktor.client.request.parameter import io.ktor.client.request.post import io.ktor.http.ContentType import io.ktor.http.HttpStatusCode @@ -10,6 +15,8 @@ import io.ktor.http.URLParserException import io.ktor.http.Url import io.ktor.http.contentType import io.ktor.http.decodeURLQueryComponent +import io.ktor.http.isSuccess +import io.ktor.http.parameters import io.ktor.http.takeFrom import io.ktor.utils.io.errors.IOException import kotlinx.serialization.Serializable @@ -21,6 +28,8 @@ import kotlin.io.encoding.Base64 import kotlin.io.encoding.ExperimentalEncodingApi /** + * Client to get the access token as the third and final step of the OAuth authorization flow. + * * Authorization flow: * * 1. Generate and store a OAuthAuthorizationParams instance and open the authorizationRequestUrl @@ -30,50 +39,42 @@ import kotlin.io.encoding.ExperimentalEncodingApi * 3. Check if the URI received is matches the OAuthAuthorizationParams instance with itsForMe(uri) * and feed the received uri to OAuthService.retrieveAccessToken(uri) */ -class OAuthService(private val httpClient: HttpClient) { +class OAuthApiClient(private val httpClient: HttpClient) { private val json = Json { ignoreUnknownKeys = true } /** * Retrieves the access token, given the [authorizationResponseUrl] * * @throws OAuthException if there has been an OAuth authorization error - * @throws OAuthConnectionException if the server reply is malformed or there is an issue with - * the connection + * @throws ConnectionException if the server reply is malformed or there is an issue with + * the connection */ - suspend fun retrieveAccessToken( + suspend fun getAccessToken( request: OAuthAuthorizationParams, authorizationResponseUrl: String - ): AccessTokenResponse { - val authorizationCode = extractAuthorizationCode(authorizationResponseUrl) + ): AccessTokenResponse = wrapApiClientExceptions { try { val response = httpClient.post(request.accessTokenUrl) { - url { - parameters.append("grant_type", "authorization_code") - parameters.append("client_id", request.clientId) - parameters.append("code", authorizationCode) - parameters.append("redirect_uri", request.redirectUri) - parameters.append("code_verifier", request.codeVerifier) - } contentType(ContentType.Application.FormUrlEncoded) + parameter("grant_type", "authorization_code") + parameter("client_id", request.clientId) + parameter("code", extractAuthorizationCode(authorizationResponseUrl)) + parameter("redirect_uri", request.redirectUri) + parameter("code_verifier", request.codeVerifier) + expectSuccess = true } - - if (response.status != HttpStatusCode.OK) { - val errorResponse = json.decodeFromString(response.body()) + val accessTokenResponse = json.decodeFromString(response.body()) + if (accessTokenResponse.token_type.lowercase() != "bearer") { + throw ConnectionException("OAuth 2 token endpoint returned an unknown token type (${accessTokenResponse.token_type})") + } + return AccessTokenResponse(accessTokenResponse.access_token, accessTokenResponse.scope?.split(" ")) + } catch (e: ClientRequestException) { + if (e.response.status == HttpStatusCode.BadRequest) { + val errorResponse = json.decodeFromString(e.response.body()) throw OAuthException(errorResponse.error, errorResponse.error_description, errorResponse.error_uri) } else { - val accessTokenResponse = json.decodeFromString(response.body()) - if (accessTokenResponse.token_type.lowercase() != "bearer") { - throw OAuthConnectionException("OAuth 2 token endpoint returned an unknown token type (${accessTokenResponse.token_type})") - } - return AccessTokenResponse(accessTokenResponse.access_token, accessTokenResponse.scope?.split(" ")) + throw e } - // if OSM server does not return valid JSON, it is the server's fault, hence - } catch (e: SerializationException) { - throw OAuthConnectionException("OAuth 2 token endpoint did not return a valid response", e) - } catch (e: IllegalArgumentException) { - throw OAuthConnectionException("OAuth 2 token endpoint did not return a valid response", e) - } catch (e: IOException) { - throw OAuthConnectionException(cause = e) } } } @@ -145,7 +146,7 @@ class OAuthService(private val httpClient: HttpClient) { * * @throws OAuthException if the URI does not contain the authorization code, e.g. * the user did not accept the requested permissions - * @throws OAuthConnectionException if the server reply is malformed + * @throws ConnectionException if the server reply is malformed */ private fun extractAuthorizationCode(uri: String): String { val parameters = Url(uri).parameters @@ -153,7 +154,7 @@ private fun extractAuthorizationCode(uri: String): String { if (authorizationCode != null) return authorizationCode val error = parameters["error"] - ?: throw OAuthConnectionException("OAuth 2 authorization endpoint did not return a valid error response: $uri") + ?: throw ConnectionException("OAuth 2 authorization endpoint did not return a valid error response: $uri") throw OAuthException( error.decodeURLQueryComponent(plusIsSpace = true), diff --git a/app/src/main/java/de/westnordost/streetcomplete/data/user/oauth/OAuthConnectionException.kt b/app/src/main/java/de/westnordost/streetcomplete/data/user/oauth/OAuthConnectionException.kt deleted file mode 100644 index 4bfebe7c34..0000000000 --- a/app/src/main/java/de/westnordost/streetcomplete/data/user/oauth/OAuthConnectionException.kt +++ /dev/null @@ -1,7 +0,0 @@ -package de.westnordost.streetcomplete.data.user.oauth - -/** OAuth failed due to an issue with the connection or a malformed server response */ -class OAuthConnectionException @JvmOverloads constructor( - message: String? = null, - cause: Throwable? = null -) : RuntimeException(message, cause) diff --git a/app/src/main/java/de/westnordost/streetcomplete/data/user/statistics/StatisticsDownloader.kt b/app/src/main/java/de/westnordost/streetcomplete/data/user/statistics/StatisticsApiClient.kt similarity index 51% rename from app/src/main/java/de/westnordost/streetcomplete/data/user/statistics/StatisticsDownloader.kt rename to app/src/main/java/de/westnordost/streetcomplete/data/user/statistics/StatisticsApiClient.kt index d4ff206b19..6ae2657fd9 100644 --- a/app/src/main/java/de/westnordost/streetcomplete/data/user/statistics/StatisticsDownloader.kt +++ b/app/src/main/java/de/westnordost/streetcomplete/data/user/statistics/StatisticsApiClient.kt @@ -1,17 +1,23 @@ package de.westnordost.streetcomplete.data.user.statistics +import de.westnordost.streetcomplete.data.ConnectionException +import de.westnordost.streetcomplete.data.wrapApiClientExceptions import io.ktor.client.HttpClient import io.ktor.client.call.body import io.ktor.client.plugins.expectSuccess import io.ktor.client.request.get -/** Downloads statistics from the backend */ -class StatisticsDownloader( +/** Client for the statistics service + * https://github.com/streetcomplete/sc-statistics-service/ */ +class StatisticsApiClient( private val httpClient: HttpClient, private val baseUrl: String, private val statisticsParser: StatisticsParser ) { - suspend fun download(osmUserId: Long): Statistics { + /** Get the statistics for the given user id + * + * @throws ConnectionException on connection or server error */ + suspend fun get(osmUserId: Long): Statistics = wrapApiClientExceptions { val response = httpClient.get("$baseUrl?user_id=$osmUserId") { expectSuccess = true } return statisticsParser.parse(response.body()) } diff --git a/app/src/main/java/de/westnordost/streetcomplete/data/user/statistics/StatisticsModule.kt b/app/src/main/java/de/westnordost/streetcomplete/data/user/statistics/StatisticsModule.kt index feb6b3880e..c715dfb14b 100644 --- a/app/src/main/java/de/westnordost/streetcomplete/data/user/statistics/StatisticsModule.kt +++ b/app/src/main/java/de/westnordost/streetcomplete/data/user/statistics/StatisticsModule.kt @@ -14,7 +14,7 @@ val statisticsModule = module { factory { ActiveDatesDao(get()) } - factory { StatisticsDownloader(get(), STATISTICS_BACKEND_URL, get()) } + factory { StatisticsApiClient(get(), STATISTICS_BACKEND_URL, get()) } factory { StatisticsParser(get(named("TypeAliases"))) } single { get() } diff --git a/app/src/main/java/de/westnordost/streetcomplete/data/user/statistics/StatisticsParser.kt b/app/src/main/java/de/westnordost/streetcomplete/data/user/statistics/StatisticsParser.kt index 1bf5f68758..3046640741 100644 --- a/app/src/main/java/de/westnordost/streetcomplete/data/user/statistics/StatisticsParser.kt +++ b/app/src/main/java/de/westnordost/streetcomplete/data/user/statistics/StatisticsParser.kt @@ -3,46 +3,33 @@ package de.westnordost.streetcomplete.data.user.statistics import kotlinx.datetime.Instant import kotlinx.datetime.LocalDate import kotlinx.serialization.Serializable -import kotlinx.serialization.json.Json.Default.decodeFromString - -@Serializable -private data class StatisticsDTO( - val questTypes: Map, - val countries: Map, - val countryRanks: Map, - val rank: Int, - val currentWeekRank: Int, - val currentWeekQuestTypes: Map, - val currentWeekCountries: Map, - val currentWeekCountryRanks: Map, - val daysActive: Int, - val activeDatesRange: Int, - val activeDates: List, - val lastUpdate: Instant, - val isAnalyzing: Boolean, -) +import kotlinx.serialization.json.Json class StatisticsParser(private val typeAliases: List>) { - fun parse(json: String): Statistics = - with(decodeFromString(json)) { - Statistics( - types = parseEditTypeStatistics(questTypes), - countries = countries.map { (key, value) -> - CountryStatistics(countryCode = key, count = value, rank = countryRanks[key]) - }.sortedBy(CountryStatistics::countryCode), - rank = rank, - daysActive = daysActive, - currentWeekRank = currentWeekRank, - currentWeekTypes = parseEditTypeStatistics(currentWeekQuestTypes), - currentWeekCountries = currentWeekCountries.map { (key, value) -> - CountryStatistics(countryCode = key, count = value, rank = currentWeekCountryRanks[key]) - }.sortedBy(CountryStatistics::countryCode), - activeDatesRange = activeDatesRange, - activeDates = activeDates, - lastUpdate = lastUpdate.toEpochMilliseconds(), - isAnalyzing = isAnalyzing, - ) - } + private val jsonParser = Json { ignoreUnknownKeys = true } + + fun parse(json: String): Statistics { + val apiStatistics = jsonParser.decodeFromString(json) + return apiStatistics.toStatistics() + } + + private fun ApiStatistics.toStatistics() = Statistics( + types = parseEditTypeStatistics(questTypes), + countries = countries.map { (key, value) -> + CountryStatistics(countryCode = key, count = value, rank = countryRanks[key]) + }.sortedBy(CountryStatistics::countryCode), + rank = rank, + daysActive = daysActive, + currentWeekRank = currentWeekRank, + currentWeekTypes = parseEditTypeStatistics(currentWeekQuestTypes), + currentWeekCountries = currentWeekCountries.map { (key, value) -> + CountryStatistics(countryCode = key, count = value, rank = currentWeekCountryRanks[key]) + }.sortedBy(CountryStatistics::countryCode), + activeDatesRange = activeDatesRange, + activeDates = activeDates, + lastUpdate = lastUpdate.toEpochMilliseconds(), + isAnalyzing = isAnalyzing, + ) private fun parseEditTypeStatistics(input: Map): List { val result = input.toMutableMap() @@ -60,3 +47,20 @@ class StatisticsParser(private val typeAliases: List>) { } } } + +@Serializable +private data class ApiStatistics( + val questTypes: Map, + val countries: Map, + val countryRanks: Map, + val rank: Int, + val currentWeekRank: Int, + val currentWeekQuestTypes: Map, + val currentWeekCountries: Map, + val currentWeekCountryRanks: Map, + val daysActive: Int, + val activeDatesRange: Int, + val activeDates: List, + val lastUpdate: Instant, + val isAnalyzing: Boolean, +) diff --git a/app/src/main/java/de/westnordost/streetcomplete/screens/MainActivity.kt b/app/src/main/java/de/westnordost/streetcomplete/screens/MainActivity.kt index 4b84974e3f..a92f1c373d 100644 --- a/app/src/main/java/de/westnordost/streetcomplete/screens/MainActivity.kt +++ b/app/src/main/java/de/westnordost/streetcomplete/screens/MainActivity.kt @@ -28,7 +28,6 @@ import de.westnordost.streetcomplete.data.messages.Message import de.westnordost.streetcomplete.data.osm.edits.ElementEdit import de.westnordost.streetcomplete.data.osm.edits.ElementEditsSource import de.westnordost.streetcomplete.data.osm.mapdata.LatLon -import de.westnordost.streetcomplete.data.osmnotes.ImageUploadServerException import de.westnordost.streetcomplete.data.osmnotes.edits.NoteEdit import de.westnordost.streetcomplete.data.osmnotes.edits.NoteEditsSource import de.westnordost.streetcomplete.data.preferences.Preferences @@ -240,7 +239,7 @@ class MainActivity : messageView.movementMethod = LinkMovementMethod.getInstance() Linkify.addLinks(messageView, Linkify.WEB_URLS) } - } else if (e is ConnectionException || e is ImageUploadServerException) { + } else if (e is ConnectionException) { /* A network connection error or server error is not the fault of this app. Nothing we can do about it, so it does not make sense to send an error report. Just notify the user. */ diff --git a/app/src/main/java/de/westnordost/streetcomplete/screens/user/login/LoginViewModel.kt b/app/src/main/java/de/westnordost/streetcomplete/screens/user/login/LoginViewModel.kt index 7f60458777..84d9157428 100644 --- a/app/src/main/java/de/westnordost/streetcomplete/screens/user/login/LoginViewModel.kt +++ b/app/src/main/java/de/westnordost/streetcomplete/screens/user/login/LoginViewModel.kt @@ -11,7 +11,7 @@ import de.westnordost.streetcomplete.data.user.OAUTH2_TOKEN_URL import de.westnordost.streetcomplete.data.user.UserLoginController import de.westnordost.streetcomplete.data.user.oauth.OAuthAuthorizationParams import de.westnordost.streetcomplete.data.user.oauth.OAuthException -import de.westnordost.streetcomplete.data.user.oauth.OAuthService +import de.westnordost.streetcomplete.data.user.oauth.OAuthApiClient import de.westnordost.streetcomplete.util.ktx.launch import de.westnordost.streetcomplete.util.logs.Log import kotlinx.coroutines.Dispatchers @@ -56,7 +56,7 @@ data object LoggedIn : LoginState class LoginViewModelImpl( private val unsyncedChangesCountSource: UnsyncedChangesCountSource, private val userLoginController: UserLoginController, - private val oAuthService: OAuthService + private val oAuthApiClient: OAuthApiClient ) : LoginViewModel() { override val loginState = MutableStateFlow(LoggedOut) override val unsyncedChangesCount = MutableStateFlow(0) @@ -101,9 +101,7 @@ class LoginViewModelImpl( private suspend fun retrieveAccessToken(authorizationResponseUrl: String): String? { try { loginState.value = RetrievingAccessToken - val accessTokenResponse = withContext(Dispatchers.IO) { - oAuthService.retrieveAccessToken(oAuth, authorizationResponseUrl) - } + val accessTokenResponse = oAuthApiClient.getAccessToken(oAuth, authorizationResponseUrl) if (accessTokenResponse.grantedScopes?.containsAll(OAUTH2_REQUIRED_SCOPES) == false) { loginState.value = LoginError.RequiredPermissionsNotGranted return null diff --git a/app/src/main/java/de/westnordost/streetcomplete/util/ktx/XmlReader.kt b/app/src/main/java/de/westnordost/streetcomplete/util/ktx/XmlReader.kt new file mode 100644 index 0000000000..cf85d795de --- /dev/null +++ b/app/src/main/java/de/westnordost/streetcomplete/util/ktx/XmlReader.kt @@ -0,0 +1,7 @@ +package de.westnordost.streetcomplete.util.ktx + +import nl.adaptivity.xmlutil.XmlReader + +fun XmlReader.attribute(name: String): String = getAttributeValue(null, name)!! + +fun XmlReader.attributeOrNull(name: String): String? = getAttributeValue(null, name) diff --git a/app/src/main/java/de/westnordost/streetcomplete/util/ktx/XmlWriter.kt b/app/src/main/java/de/westnordost/streetcomplete/util/ktx/XmlWriter.kt new file mode 100644 index 0000000000..6254364a5c --- /dev/null +++ b/app/src/main/java/de/westnordost/streetcomplete/util/ktx/XmlWriter.kt @@ -0,0 +1,9 @@ +package de.westnordost.streetcomplete.util.ktx + +import nl.adaptivity.xmlutil.XmlWriter + +fun XmlWriter.startTag(name: String) = startTag("", name, null) + +fun XmlWriter.endTag(name: String) = endTag("", name, null) + +fun XmlWriter.attribute(name: String, value: String) = attribute(null, name, null, value) diff --git a/app/src/test/java/de/westnordost/streetcomplete/data/osm/edits/upload/ElementEditUploaderTest.kt b/app/src/test/java/de/westnordost/streetcomplete/data/osm/edits/upload/ElementEditUploaderTest.kt index 1afbc4b55c..c557f37733 100644 --- a/app/src/test/java/de/westnordost/streetcomplete/data/osm/edits/upload/ElementEditUploaderTest.kt +++ b/app/src/test/java/de/westnordost/streetcomplete/data/osm/edits/upload/ElementEditUploaderTest.kt @@ -4,13 +4,14 @@ import de.westnordost.streetcomplete.data.ConflictException import de.westnordost.streetcomplete.data.osm.edits.ElementEdit import de.westnordost.streetcomplete.data.osm.edits.ElementEditAction import de.westnordost.streetcomplete.data.osm.edits.upload.changesets.OpenChangesetsManager -import de.westnordost.streetcomplete.data.osm.mapdata.MapDataApi +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.testutils.any import de.westnordost.streetcomplete.testutils.mock import de.westnordost.streetcomplete.testutils.on +import kotlinx.coroutines.runBlocking import org.mockito.ArgumentMatchers.anyBoolean import org.mockito.ArgumentMatchers.anyLong import org.mockito.Mockito.doThrow @@ -21,7 +22,7 @@ import kotlin.test.assertFailsWith class ElementEditUploaderTest { private lateinit var changesetManager: OpenChangesetsManager - private lateinit var mapDataApi: MapDataApi + private lateinit var mapDataApi: MapDataApiClient private lateinit var mapDataController: MapDataController private lateinit var uploader: ElementEditUploader @@ -33,21 +34,7 @@ class ElementEditUploaderTest { uploader = ElementEditUploader(changesetManager, mapDataApi, mapDataController) } - @Test - fun `passes on conflict exception`() { - val edit: ElementEdit = mock() - val action: ElementEditAction = mock() - on(edit.action).thenReturn(action) - on(action.createUpdates(any(), any())).thenReturn(MapDataChanges()) - on(mapDataApi.uploadChanges(anyLong(), any(), any())).thenThrow(ConflictException()) - - assertFailsWith { - uploader.upload(edit, { mock() }) - } - } - - @Test - fun `passes on element conflict exception`() { + @Test fun `passes on conflict exception`(): Unit = runBlocking { val edit: ElementEdit = mock() val action: ElementEditAction = mock() on(edit.action).thenReturn(action) @@ -55,15 +42,15 @@ class ElementEditUploaderTest { on(changesetManager.getOrCreateChangeset(any(), any(), any(), anyBoolean())).thenReturn(1) on(changesetManager.createChangeset(any(), any(), any())).thenReturn(1) - on(mapDataApi.uploadChanges(anyLong(), any(), any())) - .thenThrow(ConflictException()) + on(mapDataApi.uploadChanges(anyLong(), any(), any())).thenThrow(ConflictException()) assertFailsWith { uploader.upload(edit, { mock() }) } } - @Test fun `handles changeset conflict exception`() { + + @Test fun `handles changeset conflict exception`(): Unit = runBlocking { val edit: ElementEdit = mock() val action: ElementEditAction = mock() on(edit.action).thenReturn(action) diff --git a/app/src/test/java/de/westnordost/streetcomplete/data/osm/edits/upload/ElementEditsUploaderTest.kt b/app/src/test/java/de/westnordost/streetcomplete/data/osm/edits/upload/ElementEditsUploaderTest.kt index 9650f75a2f..38a45ea06e 100644 --- a/app/src/test/java/de/westnordost/streetcomplete/data/osm/edits/upload/ElementEditsUploaderTest.kt +++ b/app/src/test/java/de/westnordost/streetcomplete/data/osm/edits/upload/ElementEditsUploaderTest.kt @@ -5,7 +5,7 @@ import de.westnordost.streetcomplete.data.osm.edits.ElementEditAction import de.westnordost.streetcomplete.data.osm.edits.ElementEditsController import de.westnordost.streetcomplete.data.osm.mapdata.ElementKey import de.westnordost.streetcomplete.data.osm.mapdata.ElementType -import de.westnordost.streetcomplete.data.osm.mapdata.MapDataApi +import de.westnordost.streetcomplete.data.osm.mapdata.MapDataApiClient import de.westnordost.streetcomplete.data.osm.mapdata.MapDataController import de.westnordost.streetcomplete.data.osm.mapdata.MapDataUpdates import de.westnordost.streetcomplete.data.osmnotes.edits.NoteEditsController @@ -31,7 +31,7 @@ class ElementEditsUploaderTest { private lateinit var mapDataController: MapDataController private lateinit var noteEditsController: NoteEditsController private lateinit var singleUploader: ElementEditUploader - private lateinit var mapDataApi: MapDataApi + private lateinit var mapDataApi: MapDataApiClient private lateinit var statisticsController: StatisticsController private lateinit var uploader: ElementEditsUploader diff --git a/app/src/test/java/de/westnordost/streetcomplete/data/osm/edits/upload/MapDataUpdatesTest.kt b/app/src/test/java/de/westnordost/streetcomplete/data/osm/edits/upload/MapDataUpdatesTest.kt new file mode 100644 index 0000000000..8f52ab22bc --- /dev/null +++ b/app/src/test/java/de/westnordost/streetcomplete/data/osm/edits/upload/MapDataUpdatesTest.kt @@ -0,0 +1,207 @@ +package de.westnordost.streetcomplete.data.osm.edits.upload + +import de.westnordost.streetcomplete.data.osm.mapdata.DeleteElement +import de.westnordost.streetcomplete.data.osm.mapdata.ElementIdUpdate +import de.westnordost.streetcomplete.data.osm.mapdata.ElementKey +import de.westnordost.streetcomplete.data.osm.mapdata.ElementType.NODE +import de.westnordost.streetcomplete.data.osm.mapdata.ElementType.RELATION +import de.westnordost.streetcomplete.data.osm.mapdata.ElementType.WAY +import de.westnordost.streetcomplete.data.osm.mapdata.Relation +import de.westnordost.streetcomplete.data.osm.mapdata.UpdateElement +import de.westnordost.streetcomplete.data.osm.mapdata.Way +import de.westnordost.streetcomplete.data.osm.mapdata.createMapDataUpdates +import de.westnordost.streetcomplete.data.osm.mapdata.key +import de.westnordost.streetcomplete.testutils.member +import de.westnordost.streetcomplete.testutils.node +import de.westnordost.streetcomplete.testutils.rel +import de.westnordost.streetcomplete.testutils.way +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertTrue + +class MapDataUpdatesTest { + @Test fun `updates element version`() { + val updates = createMapDataUpdates( + elements = listOf(node(1), way(2), rel(3)), + updates = mapOf( + ElementKey(NODE, 1) to UpdateElement(1L, 123), + ElementKey(WAY, 2) to UpdateElement(2L, 124), + ElementKey(RELATION, 3) to UpdateElement(3L, 125), + ) + ) + + val elements = updates.updated.associateBy { it.key } + assertEquals(123, elements[ElementKey(NODE, 1)]?.version) + assertEquals(124, elements[ElementKey(WAY, 2)]?.version) + assertEquals(125, elements[ElementKey(RELATION, 3)]?.version) + assertTrue(updates.deleted.isEmpty()) + assertTrue(updates.idUpdates.isEmpty()) + } + + @Test fun `deletes element`() { + val updates = createMapDataUpdates( + elements = listOf(node(1), way(2), rel(3)), + updates = mapOf( + ElementKey(NODE, 1) to DeleteElement, + ElementKey(WAY, 2) to DeleteElement, + ElementKey(RELATION, 3) to DeleteElement, + ) + ) + + assertTrue(updates.updated.isEmpty()) + assertTrue(updates.idUpdates.isEmpty()) + assertEquals( + setOf( + ElementKey(NODE, 1), + ElementKey(WAY, 2), + ElementKey(RELATION, 3) + ), + updates.deleted.toSet() + ) + } + + @Test fun `updates element id`() { + val updates = createMapDataUpdates( + elements = listOf(node(1), way(2), rel(3)), + updates = mapOf( + ElementKey(NODE, 1) to UpdateElement(12L, 1), + ElementKey(WAY, 2) to UpdateElement(22L, 1), + ElementKey(RELATION, 3) to UpdateElement(32L, 1), + ) + ) + + assertEquals( + setOf( + ElementKey(NODE, 12), + ElementKey(WAY, 22), + ElementKey(RELATION, 32) + ), + updates.updated.mapTo(HashSet()) { it.key } + ) + + assertTrue(updates.deleted.isEmpty()) + assertEquals( + setOf( + ElementIdUpdate(NODE, 1, 12), + ElementIdUpdate(WAY, 2, 22), + ElementIdUpdate(RELATION, 3, 32) + ), + updates.idUpdates.toSet() + ) + } + + @Test fun `updates node id and all ways containing this id`() { + val updates = createMapDataUpdates( + elements = listOf( + node(-1), + way(1, listOf(3, 2, -1)), // contains it once + way(2, listOf(-1, 2, -1, -1)), // contains it multiple times + way(3, listOf(3, 4)) // contains it not + ), + updates = mapOf(ElementKey(NODE, -1) to UpdateElement(1L, 1),) + ) + + val ways = updates.updated.filterIsInstance().associateBy { it.id } + assertEquals(2, ways.size) + assertEquals(listOf(3, 2, 1), ways[1]?.nodeIds) + assertEquals(listOf(1, 2, 1, 1), ways[2]?.nodeIds) + + assertTrue(updates.deleted.isEmpty()) + assertEquals(listOf(ElementIdUpdate(NODE, -1, 1)), updates.idUpdates) + } + + @Test fun `updates node id and all relations containing this id`() { + val updates = createMapDataUpdates( + elements = listOf( + node(-1), + rel(1, listOf(member(NODE, 3), member(NODE, -1))), // contains it once + rel(2, listOf(member(NODE, -1), member(NODE, 2), member(NODE, -1))), // contains it multiple times + rel(3, listOf(member(WAY, -1), member(RELATION, -1), member(NODE, 1))) // contains it not + ), + updates = mapOf(ElementKey(NODE, -1) to UpdateElement(1L, 1),) + ) + + val relations = updates.updated.filterIsInstance().associateBy { it.id } + assertEquals(2, relations.size) + assertEquals( + listOf(member(NODE, 3), member(NODE, 1)), + relations[1]?.members + ) + assertEquals( + listOf(member(NODE, 1), member(NODE, 2), member(NODE, 1)), + relations[2]?.members + ) + + assertTrue(updates.deleted.isEmpty()) + assertEquals(listOf(ElementIdUpdate(NODE, -1, 1)), updates.idUpdates) + } + + @Test fun `deletes node id and updates all ways containing this id`() { + val updates = createMapDataUpdates( + elements = listOf( + node(1), + way(1, listOf(3, 1)), // contains it once + way(2, listOf(1, 2, 1)), // contains it multiple times + way(3, listOf(3, 4)) // contains it not + ), + updates = mapOf(ElementKey(NODE, 1) to DeleteElement) + ) + + assertTrue(updates.idUpdates.isEmpty()) + assertEquals(listOf(ElementKey(NODE, 1)), updates.deleted) + + val ways = updates.updated.filterIsInstance().associateBy { it.id } + assertEquals(2, ways.size) + assertEquals(listOf(3), ways[1]?.nodeIds) + assertEquals(listOf(2), ways[2]?.nodeIds) + } + + @Test fun `deletes node id and updates all relations containing this id`() { + val updates = createMapDataUpdates( + elements = listOf( + node(1), + rel(1, listOf(member(NODE, 3), member(NODE, 1))), // contains it once + rel(2, listOf(member(NODE, 1), member(NODE, 2), member(NODE, 1))), // contains it multiple times + rel(3, listOf(member(WAY, 1), member(RELATION, 1), member(NODE, 2))) // contains it not + ), + updates = mapOf(ElementKey(NODE, 1) to DeleteElement) + ) + assertTrue(updates.idUpdates.isEmpty()) + assertEquals(listOf(ElementKey(NODE, 1)), updates.deleted) + + val relations = updates.updated.filterIsInstance().associateBy { it.id } + assertEquals(2, relations.size) + assertEquals(listOf(member(NODE, 3)), relations[1]?.members) + assertEquals(listOf(member(NODE, 2)), relations[2]?.members) + } + + @Test fun `does nothing with ignored relation types`() { + val updates = createMapDataUpdates( + elements = listOf( + rel(-4, tags = mapOf("type" to "route")) + ), + updates = mapOf(ElementKey(RELATION, -4) to UpdateElement(4, 1)), + ignoreRelationTypes = setOf("route") + ) + assertTrue(updates.idUpdates.isEmpty()) + assertTrue(updates.updated.isEmpty()) + assertTrue(updates.deleted.isEmpty()) + } + + @Test fun `references to ignored relation types are updated`() { + val updates = createMapDataUpdates( + elements = listOf( + rel(1, members = listOf(member(RELATION, -4))), + rel(-4, tags = mapOf("type" to "route")) + ), + updates = mapOf(ElementKey(RELATION, -4) to UpdateElement(4, 1)), + ignoreRelationTypes = setOf("route") + ) + assertTrue(updates.idUpdates.isEmpty()) + assertEquals( + listOf(member(RELATION, 4)), + (updates.updated.single() as Relation).members + ) + assertTrue(updates.deleted.isEmpty()) + } +} diff --git a/app/src/test/java/de/westnordost/streetcomplete/data/osm/edits/upload/UpdatedElementsHandlerTest.kt b/app/src/test/java/de/westnordost/streetcomplete/data/osm/edits/upload/UpdatedElementsHandlerTest.kt deleted file mode 100644 index 272bbf871d..0000000000 --- a/app/src/test/java/de/westnordost/streetcomplete/data/osm/edits/upload/UpdatedElementsHandlerTest.kt +++ /dev/null @@ -1,201 +0,0 @@ -package de.westnordost.streetcomplete.data.osm.edits.upload - -import de.westnordost.streetcomplete.data.osm.mapdata.DiffElement -import de.westnordost.streetcomplete.data.osm.mapdata.ElementIdUpdate -import de.westnordost.streetcomplete.data.osm.mapdata.ElementKey -import de.westnordost.streetcomplete.data.osm.mapdata.ElementType.NODE -import de.westnordost.streetcomplete.data.osm.mapdata.ElementType.RELATION -import de.westnordost.streetcomplete.data.osm.mapdata.ElementType.WAY -import de.westnordost.streetcomplete.data.osm.mapdata.Relation -import de.westnordost.streetcomplete.data.osm.mapdata.UpdatedElementsHandler -import de.westnordost.streetcomplete.data.osm.mapdata.Way -import de.westnordost.streetcomplete.testutils.member -import de.westnordost.streetcomplete.testutils.node -import de.westnordost.streetcomplete.testutils.rel -import de.westnordost.streetcomplete.testutils.way -import de.westnordost.streetcomplete.util.ktx.containsExactlyInAnyOrder -import kotlin.test.Test -import kotlin.test.assertEquals -import kotlin.test.assertTrue - -class UpdatedElementsHandlerTest { - @Test fun `updates element version`() { - val handler = UpdatedElementsHandler() - handler.handle(DiffElement(NODE, 1, 1, 123)) - - val element = handler.getElementUpdates(listOf(node(1))).updated.single() - assertEquals(123, element.version) - } - - @Test fun `deletes element`() { - val handler = UpdatedElementsHandler() - handler.handle(DiffElement(NODE, 1)) - - val deletedElementKey = handler.getElementUpdates(listOf(node(1))).deleted.single() - assertEquals(1, deletedElementKey.id) - assertEquals(NODE, deletedElementKey.type) - } - - @Test fun `updates element id`() { - val handler = UpdatedElementsHandler() - handler.handle(DiffElement(NODE, -1, 123456, 1)) - - val element = handler.getElementUpdates(listOf(node(-1))).updated.single() - assertEquals(123456, element.id) - } - - @Test fun `updates node id and all ways containing this id`() { - val elements = listOf( - node(-1), - way(1, listOf(3, 2, -1)), // contains it once - way(2, listOf(-1, 2, -1, -1)), // contains it multiple times - way(3, listOf(3, 4)) // contains it not - ) - val handler = UpdatedElementsHandler() - handler.handleAll( - DiffElement(NODE, -1, 1, 1), - DiffElement(WAY, 1, 1, 2), - DiffElement(WAY, 2, 2, 2), - DiffElement(WAY, 3, 3, 2) - ) - - val updatedElements = handler.getElementUpdates(elements).updated - assertEquals(4, updatedElements.size) - val updatedWays = updatedElements.filterIsInstance() - assertEquals(3, updatedWays.size) - assertEquals(listOf(3L, 2L, 1L), updatedWays.find { it.id == 1L }!!.nodeIds) - assertEquals(listOf(1L, 2L, 1L, 1L), updatedWays.find { it.id == 2L }!!.nodeIds) - assertEquals(listOf(3L, 4L), updatedWays.find { it.id == 3L }!!.nodeIds) - } - - @Test fun `updates node id and all relations containing this id`() { - val elements = listOf( - node(-1), - rel(1, listOf(member(NODE, 3), member(NODE, -1))), // contains it once - rel(2, listOf(member(NODE, -1), member(NODE, 2), member(NODE, -1))), // contains it multiple times - rel(3, listOf(member(WAY, -1), member(RELATION, -1), member(NODE, 1))) // contains it not - ) - val handler = UpdatedElementsHandler() - handler.handle(DiffElement(NODE, -1, 1, 1)) - handler.handleAll( - DiffElement(NODE, -1, 1, 1), - DiffElement(RELATION, 1, 1, 2), - DiffElement(RELATION, 2, 2, 2), - DiffElement(RELATION, 3, 3, 2) - ) - - val updatedElements = handler.getElementUpdates(elements).updated - assertEquals(4, updatedElements.size) - val updatedRelations = updatedElements.filterIsInstance() - assertEquals(3, updatedRelations.size) - assertEquals( - listOf(member(NODE, 3), member(NODE, 1)), - updatedRelations.find { it.id == 1L }!!.members - ) - assertEquals( - listOf(member(NODE, 1), member(NODE, 2), member(NODE, 1)), - updatedRelations.find { it.id == 2L }!!.members - ) - assertEquals( - listOf(member(WAY, -1), member(RELATION, -1), member(NODE, 1)), - updatedRelations.find { it.id == 3L }!!.members - ) - } - - @Test fun `deletes node id and updates all ways containing this id`() { - val elements = listOf( - node(1), - way(1, listOf(3, 1)), // contains it once - way(2, listOf(1, 2, 1)), // contains it multiple times - way(3, listOf(3, 4)) // contains it not - ) - val handler = UpdatedElementsHandler() - handler.handleAll( - DiffElement(NODE, 1), - DiffElement(WAY, 1, 1, 2), - DiffElement(WAY, 2, 2, 2), - DiffElement(WAY, 3, 3, 2) - ) - - val elementUpdates = handler.getElementUpdates(elements) - assertEquals(1, elementUpdates.deleted.size) - assertEquals(3, elementUpdates.updated.size) - val updatedWays = elementUpdates.updated.filterIsInstance() - assertEquals(3, updatedWays.size) - assertEquals(listOf(3L), updatedWays.find { it.id == 1L }!!.nodeIds) - assertEquals(listOf(2L), updatedWays.find { it.id == 2L }!!.nodeIds) - assertEquals(listOf(3L, 4L), updatedWays.find { it.id == 3L }!!.nodeIds) - } - - @Test fun `deletes node id and updates all relations containing this id`() { - val elements = listOf( - node(1), - rel(1, listOf(member(NODE, 3), member(NODE, 1))), // contains it once - rel(2, listOf(member(NODE, 1), member(NODE, 2), member(NODE, 1))), // contains it multiple times - rel(3, listOf(member(WAY, 1), member(RELATION, 1), member(NODE, 2))) // contains it not - ) - val handler = UpdatedElementsHandler() - handler.handleAll( - DiffElement(NODE, 1), - DiffElement(RELATION, 1, 1, 2), - DiffElement(RELATION, 2, 2, 2), - DiffElement(RELATION, 3, 3, 2) - ) - - val elementUpdates = handler.getElementUpdates(elements) - assertEquals(1, elementUpdates.deleted.size) - assertEquals(3, elementUpdates.updated.size) - val updatedRelations = elementUpdates.updated.filterIsInstance() - assertEquals(3, updatedRelations.size) - assertEquals( - listOf(member(NODE, 3)), - updatedRelations.find { it.id == 1L }!!.members - ) - assertEquals( - listOf(member(NODE, 2)), - updatedRelations.find { it.id == 2L }!!.members - ) - assertEquals( - listOf(member(WAY, 1), member(RELATION, 1), member(NODE, 2)), - updatedRelations.find { it.id == 3L }!!.members - ) - } - - @Test fun `relays element id updates of non-deleted elements`() { - val elements = listOf( - node(-1), - node(-2), - way(-3, listOf()), - rel(-4, listOf()) - ) - val handler = UpdatedElementsHandler() - handler.handleAll( - DiffElement(NODE, -1, 11), - DiffElement(NODE, -2, null), - DiffElement(WAY, -3, 33), - DiffElement(RELATION, -4, 44) - ) - val updates = handler.getElementUpdates(elements) - assertTrue(updates.idUpdates.containsExactlyInAnyOrder(listOf( - ElementIdUpdate(NODE, -1, 11), - ElementIdUpdate(WAY, -3, 33), - ElementIdUpdate(RELATION, -4, 44), - ))) - updates.deleted.containsExactlyInAnyOrder(listOf( - ElementKey(NODE, -2) - )) - } - - @Test fun `does nothing with ignored relation types`() { - val elements = listOf(rel(-4, listOf(), tags = mapOf("type" to "route"))) - val ignoredRelationTypes = setOf("route") - val handler = UpdatedElementsHandler(ignoredRelationTypes) - handler.handle(DiffElement(RELATION, -4, 44)) - val updates = handler.getElementUpdates(elements) - assertTrue(updates.idUpdates.isEmpty()) - } -} - -private fun UpdatedElementsHandler.handleAll(vararg diffs: DiffElement) { - diffs.forEach { handle(it) } -} diff --git a/app/src/test/java/de/westnordost/streetcomplete/data/osm/edits/upload/changesets/ChangesetApiClientTest.kt b/app/src/test/java/de/westnordost/streetcomplete/data/osm/edits/upload/changesets/ChangesetApiClientTest.kt new file mode 100644 index 0000000000..445be03388 --- /dev/null +++ b/app/src/test/java/de/westnordost/streetcomplete/data/osm/edits/upload/changesets/ChangesetApiClientTest.kt @@ -0,0 +1,46 @@ +package de.westnordost.streetcomplete.data.osm.edits.upload.changesets + +import de.westnordost.streetcomplete.data.AuthorizationException +import de.westnordost.streetcomplete.data.ConflictException +import de.westnordost.streetcomplete.data.user.UserLoginSource +import de.westnordost.streetcomplete.testutils.OsmDevApi +import de.westnordost.streetcomplete.testutils.mock +import de.westnordost.streetcomplete.testutils.on +import io.ktor.client.HttpClient +import kotlinx.coroutines.runBlocking +import kotlin.test.Test +import kotlin.test.assertFailsWith + +// other than some other APIs we are speaking to, we do not control the OSM API, so I think it is +// more effective to test with the official test API instead of mocking some imagined server +// response +class ChangesetApiClientTest { + private val allowEverything = mock() + private val allowNothing = mock() + private val anonymous = mock() + + init { + on(allowEverything.accessToken).thenReturn(OsmDevApi.ALLOW_EVERYTHING_TOKEN) + on(allowNothing.accessToken).thenReturn(OsmDevApi.ALLOW_NOTHING_TOKEN) + on(anonymous.accessToken).thenReturn(null) + } + + @Test fun `open throws exception on insufficient privileges`(): Unit = runBlocking { + assertFailsWith { client(anonymous).open(mapOf()) } + assertFailsWith { client(allowNothing).open(mapOf()) } + } + + @Test fun `open and close works without error`(): Unit = runBlocking { + val id = client(allowEverything).open(mapOf("testKey" to "testValue")) + client(allowEverything).close(id) + assertFailsWith { client(allowEverything).close(id) } + } + + @Test fun `close throws exception on insufficient privileges`(): Unit = runBlocking { + assertFailsWith { client(anonymous).close(1) } + assertFailsWith { client(allowNothing).close(1) } + } + + private fun client(userLoginSource: UserLoginSource) = + ChangesetApiClient(HttpClient(), OsmDevApi.URL, userLoginSource, ChangesetApiSerializer()) +} diff --git a/app/src/test/java/de/westnordost/streetcomplete/data/osm/edits/upload/changesets/ChangesetApiSerializerTest.kt b/app/src/test/java/de/westnordost/streetcomplete/data/osm/edits/upload/changesets/ChangesetApiSerializerTest.kt new file mode 100644 index 0000000000..edf43ec7d3 --- /dev/null +++ b/app/src/test/java/de/westnordost/streetcomplete/data/osm/edits/upload/changesets/ChangesetApiSerializerTest.kt @@ -0,0 +1,28 @@ +package de.westnordost.streetcomplete.data.osm.edits.upload.changesets + +import kotlin.test.Test +import kotlin.test.assertEquals + +class ChangesetApiSerializerTest { + + @Test fun `serialize to xml`() { + val osm = """ + + + + + + + """ + + val changesetTags = mapOf( + "one" to "1", + "two" to "2", + ) + + assertEquals( + osm.replace(Regex("[\n\r] *"), ""), + ChangesetApiSerializer().serialize(changesetTags) + ) + } +} diff --git a/app/src/test/java/de/westnordost/streetcomplete/data/osm/edits/upload/changesets/OpenChangesetsManagerTest.kt b/app/src/test/java/de/westnordost/streetcomplete/data/osm/edits/upload/changesets/OpenChangesetsManagerTest.kt index 7a609e2c79..06044559d7 100644 --- a/app/src/test/java/de/westnordost/streetcomplete/data/osm/edits/upload/changesets/OpenChangesetsManagerTest.kt +++ b/app/src/test/java/de/westnordost/streetcomplete/data/osm/edits/upload/changesets/OpenChangesetsManagerTest.kt @@ -2,7 +2,7 @@ package de.westnordost.streetcomplete.data.osm.edits.upload.changesets import de.westnordost.streetcomplete.ApplicationConstants import de.westnordost.streetcomplete.data.osm.mapdata.LatLon -import de.westnordost.streetcomplete.data.osm.mapdata.MapDataApi +import de.westnordost.streetcomplete.data.osm.mapdata.MapDataApiClient import de.westnordost.streetcomplete.data.osm.osmquests.OsmElementQuestType import de.westnordost.streetcomplete.data.preferences.Preferences import de.westnordost.streetcomplete.data.quest.TestQuestTypeA @@ -10,6 +10,7 @@ import de.westnordost.streetcomplete.testutils.any import de.westnordost.streetcomplete.testutils.mock import de.westnordost.streetcomplete.testutils.on import de.westnordost.streetcomplete.util.math.translate +import kotlinx.coroutines.runBlocking import org.mockito.Mockito.never import org.mockito.Mockito.verify import java.util.Locale @@ -20,7 +21,7 @@ import kotlin.test.assertEquals class OpenChangesetsManagerTest { private lateinit var questType: OsmElementQuestType<*> - private lateinit var mapDataApi: MapDataApi + private lateinit var changesetApiClient: ChangesetApiClient private lateinit var openChangesetsDB: OpenChangesetsDao private lateinit var changesetAutoCloser: ChangesetAutoCloser private lateinit var manager: OpenChangesetsManager @@ -28,34 +29,34 @@ class OpenChangesetsManagerTest { @BeforeTest fun setUp() { questType = TestQuestTypeA() - mapDataApi = mock() + changesetApiClient = mock() openChangesetsDB = mock() changesetAutoCloser = mock() prefs = mock() - manager = OpenChangesetsManager(mapDataApi, openChangesetsDB, changesetAutoCloser, prefs) + manager = OpenChangesetsManager(changesetApiClient, openChangesetsDB, changesetAutoCloser, prefs) } - @Test fun `create new changeset if none exists`() { + @Test fun `create new changeset if none exists`(): Unit = runBlocking { on(openChangesetsDB.get(any(), any())).thenReturn(null) - on(mapDataApi.openChangeset(any())).thenReturn(123L) + on(changesetApiClient.open(any())).thenReturn(123L) assertEquals(123L, manager.getOrCreateChangeset(questType, "my source", LatLon(0.0, 0.0), false)) - verify(mapDataApi).openChangeset(any()) + verify(changesetApiClient).open(any()) verify(openChangesetsDB).put(any()) } - @Test fun `reuse changeset if one exists`() { + @Test fun `reuse changeset if one exists`(): Unit = runBlocking { on(openChangesetsDB.get(questType.name, "source")).thenReturn( OpenChangeset(questType.name, "source", 123, LatLon(0.0, 0.0)) ) assertEquals(123L, manager.getOrCreateChangeset(questType, "source", LatLon(0.0, 0.0), false)) - verify(mapDataApi, never()).openChangeset(any()) + verify(changesetApiClient, never()).open(any()) } - @Test fun `reuse changeset if one exists and position is far away but should not create new if too far away`() { + @Test fun `reuse changeset if one exists and position is far away but should not create new if too far away`(): Unit = runBlocking { val p0 = LatLon(0.0, 0.0) val p1 = p0.translate(5001.0, 0.0) @@ -63,35 +64,36 @@ class OpenChangesetsManagerTest { OpenChangeset(questType.name, "source", 123, p0) ) assertEquals(123L, manager.getOrCreateChangeset(questType, "source", p1, false)) - verify(mapDataApi, never()).openChangeset(any()) + verify(changesetApiClient, never()).open(any()) } - @Test fun `close changeset and create new if one exists and position is far away`() { + @Test fun `close changeset and create new if one exists and position is far away`(): Unit = runBlocking { val p0 = LatLon(0.0, 0.0) val p1 = p0.translate(5001.0, 0.0) on(openChangesetsDB.get(questType.name, "source")).thenReturn( OpenChangeset(questType.name, "source", 123, p0) ) - on(mapDataApi.openChangeset(any())).thenReturn(124L) + on(changesetApiClient.open(any())).thenReturn(124L) assertEquals(124L, manager.getOrCreateChangeset(questType, "source", p1, true)) - verify(mapDataApi).closeChangeset(123L) - verify(mapDataApi).openChangeset(any()) + verify(changesetApiClient).close(123L) + verify(changesetApiClient).open(any()) verify(openChangesetsDB).delete(questType.name, "source") verify(openChangesetsDB).put(OpenChangeset(questType.name, "source", 124L, p1)) } - @Test fun `create correct changeset tags`() { + @Test fun `create correct changeset tags`(): Unit = runBlocking { on(openChangesetsDB.get(any(), any())).thenReturn(null) val locale = Locale.getDefault() Locale.setDefault(Locale("es", "AR")) + on(changesetApiClient.open(any())).thenReturn(1) manager.getOrCreateChangeset(questType, "my source", LatLon(0.0, 0.0), false) Locale.setDefault(locale) - verify(mapDataApi).openChangeset(mapOf( + verify(changesetApiClient).open(mapOf( "source" to "my source", "created_by" to ApplicationConstants.USER_AGENT, "comment" to "test me", diff --git a/app/src/test/java/de/westnordost/streetcomplete/data/osm/mapdata/MapDataApiClientTest.kt b/app/src/test/java/de/westnordost/streetcomplete/data/osm/mapdata/MapDataApiClientTest.kt new file mode 100644 index 0000000000..c32523dcf4 --- /dev/null +++ b/app/src/test/java/de/westnordost/streetcomplete/data/osm/mapdata/MapDataApiClientTest.kt @@ -0,0 +1,217 @@ +package de.westnordost.streetcomplete.data.osm.mapdata + +import de.westnordost.streetcomplete.data.AuthorizationException +import de.westnordost.streetcomplete.data.ConflictException +import de.westnordost.streetcomplete.data.QueryTooBigException +import de.westnordost.streetcomplete.data.osm.edits.upload.changesets.ChangesetApiClient +import de.westnordost.streetcomplete.data.osm.edits.upload.changesets.ChangesetApiSerializer +import de.westnordost.streetcomplete.data.user.UserLoginSource +import de.westnordost.streetcomplete.testutils.OsmDevApi +import de.westnordost.streetcomplete.testutils.mock +import de.westnordost.streetcomplete.testutils.node +import de.westnordost.streetcomplete.testutils.on +import de.westnordost.streetcomplete.testutils.p +import de.westnordost.streetcomplete.testutils.way +import io.ktor.client.HttpClient +import kotlinx.coroutines.runBlocking +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertFailsWith +import kotlin.test.assertNotNull +import kotlin.test.assertNull +import kotlin.test.assertTrue + +class MapDataApiClientTest { + + private val allowEverything = mock() + private val allowNothing = mock() + private val anonymous = mock() + + init { + on(allowEverything.accessToken).thenReturn(OsmDevApi.ALLOW_EVERYTHING_TOKEN) + on(allowNothing.accessToken).thenReturn(OsmDevApi.ALLOW_NOTHING_TOKEN) + on(anonymous.accessToken).thenReturn(null) + } + + @Test fun getNode(): Unit = runBlocking { + assertEquals("Yangon", liveClient.getNode(NDDE_YANGON)?.tags?.get("name:en")) + assertNull(liveClient.getNode(0)) + } + + @Test fun getWay(): Unit = runBlocking { + assertEquals("Oderhafen", liveClient.getWay(WAY_ODERHAFEN)?.tags?.get("name")) + assertNull(liveClient.getWay(0)) + } + + @Test fun getRelation(): Unit = runBlocking { + assertEquals("Hamburg", liveClient.getRelation(RELATION_HAMBURG)?.tags?.get("name")) + assertNull(liveClient.getRelation(0)) + } + + @Test fun getWaysForNode(): Unit = runBlocking { + assertTrue(liveClient.getWaysForNode(VERTEX_OF_ELBPHILHARMONIE).isNotEmpty()) + assertTrue(liveClient.getWaysForNode(0).isEmpty()) + } + + @Test fun getRelationsForNode(): Unit = runBlocking { + assertTrue(liveClient.getRelationsForNode(NODE_BUS_STATION).isNotEmpty()) + assertTrue(liveClient.getRelationsForNode(0).isEmpty()) + } + + @Test fun getRelationsForWay(): Unit = runBlocking { + assertTrue(liveClient.getRelationsForWay(WAY_NEAR_BUS_STATION).isNotEmpty()) + assertTrue(liveClient.getRelationsForWay(0).isEmpty()) + } + + @Test fun getRelationsForRelation(): Unit = runBlocking { + assertTrue(liveClient.getRelationsForRelation(RELATION_ONE_WAY_OF_BUS_ROUTE).isNotEmpty()) + assertTrue(liveClient.getRelationsForRelation(0).isEmpty()) + } + + @Test fun getWayComplete(): Unit = runBlocking { + val data = liveClient.getWayComplete(WAY_ODERHAFEN) + assertNotNull(data) + assertTrue(data.nodes.isNotEmpty()) + assertTrue(data.ways.size == 1) + + assertNull(liveClient.getWayComplete(0)) + } + + @Test fun getRelationComplete(): Unit = runBlocking { + val data = liveClient.getRelationComplete(RELATION_HAMBURG) + assertNotNull(data) + assertTrue(data.nodes.isNotEmpty()) + assertTrue(data.ways.isNotEmpty()) + + assertNull(liveClient.getRelationComplete(0)) + } + + @Test fun getMap(): Unit = runBlocking { + val hamburg = liveClient.getMap(HAMBURG_CITY_AREA) + assertTrue(hamburg.nodes.isNotEmpty()) + assertTrue(hamburg.ways.isNotEmpty()) + assertTrue(hamburg.relations.isNotEmpty()) + } + + @Test fun `getMap does not return relations of ignored type`(): Unit = runBlocking { + val hamburg = liveClient.getMap(AREA_NEAR_BUS_STATION, setOf("route")) + assertTrue(hamburg.relations.none { it.tags["type"] == "route" }) + } + + @Test fun `getMap fails when bbox crosses 180th meridian`(): Unit = runBlocking { + assertFailsWith { + liveClient.getMap(BoundingBox(0.0, 179.9999999, 0.0000001, -179.9999999)) + } + } + + @Test fun `getMap fails when bbox is too big`(): Unit = runBlocking { + assertFailsWith { + liveClient.getMap(BoundingBox(-90.0, -180.0, 90.0, 180.0)) + } + } + + @Test fun `getMap returns bounding box that was specified in request`(): Unit = runBlocking { + val hamburg = liveClient.getMap(HAMBURG_CITY_AREA) + assertEquals(HAMBURG_CITY_AREA, hamburg.boundingBox) + } + + @Test fun `uploadChanges as anonymous fails`(): Unit = runBlocking { + assertFailsWith { + client(anonymous).uploadChanges(1L, MapDataChanges()) + } + } + + @Test fun `uploadChanges without authorization fails`(): Unit = runBlocking { + assertFailsWith { + client(allowNothing).uploadChanges(1L, MapDataChanges()) + } + } + + @Test fun `uploadChanges in already closed changeset fails`(): Unit = runBlocking { + val changesetId = changesetClient(allowEverything).open(mapOf()) + changesetClient(allowEverything).close(changesetId) + assertFailsWith { + client(allowEverything).uploadChanges(changesetId, MapDataChanges()) + } + } + + @Test fun `uploadChanges of non-existing element fails`(): Unit = runBlocking { + val changesetId = changesetClient(allowEverything).open(mapOf()) + assertFailsWith { + client(allowEverything).uploadChanges( + changesetId = changesetId, + changes = MapDataChanges(modifications = listOf(node(Long.MAX_VALUE))) + ) + } + changesetClient(allowEverything).close(changesetId) + } + + @Test fun uploadChanges(): Unit = runBlocking { + val changesetId = changesetClient(allowEverything).open(mapOf()) + + val updates1 = client(allowEverything).uploadChanges( + changesetId = changesetId, + changes = MapDataChanges( + creations = listOf( + node(-1, pos = p(15.0, -39.0), tags = mapOf("first" to "1")), + node(-2, pos = p(15.0, -39.1), tags = mapOf("second" to "2")), + node(-3, pos = p(15.0, -39.1), tags = mapOf("third" to "3")), + way(-4, nodes = listOf(-1, -2, -3)), + ) + ) + ) + assertEquals( + setOf( + ElementKey(ElementType.NODE, -1), + ElementKey(ElementType.NODE, -2), + ElementKey(ElementType.NODE, -3), + ElementKey(ElementType.WAY, -4), + ), + updates1.idUpdates.map { ElementKey(it.elementType, it.oldElementId) }.toSet() + ) + assertEquals(4, updates1.updated.size) + + val elements = updates1.updated + val firstNode = elements.find { it.tags["first"] == "1" } as Node + val secondNode = elements.find { it.tags["second"] == "2" } as Node + val thirdNode = elements.find { it.tags["third"] == "3" } as Node + val way = elements.filterIsInstance().single() + + val updates2 = client(allowEverything).uploadChanges( + changesetId = changesetId, + changes = MapDataChanges( + modifications = listOf(way.copy(nodeIds = listOf(secondNode.id, thirdNode.id))), + deletions = listOf(firstNode), + ) + ) + assertTrue(updates2.idUpdates.isEmpty()) + assertEquals(listOf(firstNode.key), updates2.deleted) + assertEquals(listOf(way.key), updates2.updated.map { it.key }) + + changesetClient(allowEverything).close(changesetId) + } + + private fun client(userLoginSource: UserLoginSource) = + MapDataApiClient(HttpClient(), OsmDevApi.URL, userLoginSource, MapDataApiParser(), MapDataApiSerializer()) + + private fun changesetClient(userLoginSource: UserLoginSource) = + ChangesetApiClient(HttpClient(), OsmDevApi.URL, userLoginSource, ChangesetApiSerializer()) + + private val liveClient = + MapDataApiClient(HttpClient(), "https://api.openstreetmap.org/api/0.6/", anonymous, MapDataApiParser(), MapDataApiSerializer()) + + // some elements that should exist on the live API + + private val NDDE_YANGON = 26576175L + private val WAY_ODERHAFEN = 23564402L + private val RELATION_HAMBURG = 451087L + + private val VERTEX_OF_ELBPHILHARMONIE = 271454735L + + private val NODE_BUS_STATION = 483688378L + private val WAY_NEAR_BUS_STATION = 148796410L + private val RELATION_ONE_WAY_OF_BUS_ROUTE = 36912L + + private val AREA_NEAR_BUS_STATION = BoundingBox(53.6068315, 9.9046576, 53.6079471, 9.9062240) + private val HAMBURG_CITY_AREA = BoundingBox(53.579, 9.939, 53.580, 9.940) +} diff --git a/app/src/test/java/de/westnordost/streetcomplete/data/osm/mapdata/MapDataApiParserTest.kt b/app/src/test/java/de/westnordost/streetcomplete/data/osm/mapdata/MapDataApiParserTest.kt new file mode 100644 index 0000000000..a24acee808 --- /dev/null +++ b/app/src/test/java/de/westnordost/streetcomplete/data/osm/mapdata/MapDataApiParserTest.kt @@ -0,0 +1,78 @@ +package de.westnordost.streetcomplete.data.osm.mapdata + +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertNull + +class MapDataApiParserTest { + + @Test fun `parseMapData minimum`() { + val empty = MapDataApiParser().parseMapData("", emptySet()) + assertEquals(0, empty.size) + assertNull(empty.boundingBox) + } + + @Test fun `parseMapData full`() { + val osm = """ + + + ${nodesOsm(123)} + ${waysOsm(345)} + ${relationsOsm(567)} + + """ + + val data = MapDataApiParser().parseMapData(osm, emptySet()) + assertEquals(nodesList.toSet(), data.nodes.toSet()) + assertEquals(waysList.toSet(), data.ways.toSet()) + assertEquals(relationsList.toSet(), data.relations.toSet()) + assertEquals(BoundingBox(53.0, 9.0, 53.01, 9.01), data.boundingBox) + } + + @Test fun `parseMapData with ignored relation types`() { + val osm = """ + + + + + + """ + + val empty = MapDataApiParser().parseMapData(osm, setOf("route")) + assertEquals(0, empty.size) + } + + @Test fun `parseElementUpdates minimum`() { + assertEquals( + mapOf(), + MapDataApiParser().parseElementUpdates("") + ) + } + + @Test fun `parseElementUpdates full`() { + val diffResult = """ + + + + + + + + + """ + + val elementUpdates = mapOf( + ElementKey(ElementType.NODE, 1) to DeleteElement, + ElementKey(ElementType.WAY, 2) to DeleteElement, + ElementKey(ElementType.RELATION, 3) to DeleteElement, + ElementKey(ElementType.NODE, -1) to UpdateElement(9, 99), + ElementKey(ElementType.WAY, -2) to UpdateElement(8, 88), + ElementKey(ElementType.RELATION, -3) to UpdateElement(7, 77), + ) + + assertEquals( + elementUpdates, + MapDataApiParser().parseElementUpdates(diffResult) + ) + } +} diff --git a/app/src/test/java/de/westnordost/streetcomplete/data/osm/mapdata/MapDataApiSerializerTest.kt b/app/src/test/java/de/westnordost/streetcomplete/data/osm/mapdata/MapDataApiSerializerTest.kt new file mode 100644 index 0000000000..57ebb0141d --- /dev/null +++ b/app/src/test/java/de/westnordost/streetcomplete/data/osm/mapdata/MapDataApiSerializerTest.kt @@ -0,0 +1,38 @@ +package de.westnordost.streetcomplete.data.osm.mapdata + +import kotlin.test.Test +import kotlin.test.assertEquals + +class MapDataApiSerializerTest { + + @Test fun `serializeMapDataChanges full`() { + val osmChange = """ + + + ${nodesOsm(1234)} + ${waysOsm(1234)} + + + ${waysOsm(1234)} + ${relationsOsm(1234)} + + + ${nodesOsm(1234)} + ${relationsOsm(1234)} + + + """ + + val mapDataChanges = MapDataChanges( + creations = nodesList + waysList, + modifications = waysList + relationsList, + deletions = nodesList + relationsList, + ) + + assertEquals( + osmChange.replace(Regex("[\n\r] *"), ""), + MapDataApiSerializer().serializeMapDataChanges(mapDataChanges, 1234L) + ) + } + +} diff --git a/app/src/test/java/de/westnordost/streetcomplete/data/osm/mapdata/MapDataApiTestUtils.kt b/app/src/test/java/de/westnordost/streetcomplete/data/osm/mapdata/MapDataApiTestUtils.kt new file mode 100644 index 0000000000..3894ab7986 --- /dev/null +++ b/app/src/test/java/de/westnordost/streetcomplete/data/osm/mapdata/MapDataApiTestUtils.kt @@ -0,0 +1,88 @@ +package de.westnordost.streetcomplete.data.osm.mapdata + +import kotlinx.datetime.Instant + +fun nodesOsm(c: Long): String = """ + + + + + + """ + +fun waysOsm(c: Long): String = """ + + + + + + + + + """ + +fun relationsOsm(c: Long): String = """ + + + + + + + + + """ + +val nodesList = listOf( + Node( + id = 122, + position = LatLon(53.0098761, 9.0065254), + tags = emptyMap(), + version = 2, + timestampEdited = Instant.parse("2019-03-15T01:52:26Z").toEpochMilliseconds() + ), + Node( + id = 123, + position = LatLon(53.0098760, 9.0065253), + tags = mapOf("emergency" to "fire_hydrant", "fire_hydrant:type" to "pillar"), + version = 1, + timestampEdited = Instant.parse("2019-03-15T01:52:25Z").toEpochMilliseconds() + ), +) + +val waysList = listOf( + Way( + id = 336145990, + nodeIds = emptyList(), + tags = emptyMap(), + version = 20, + timestampEdited = Instant.parse("2018-10-17T06:39:01Z").toEpochMilliseconds() + ), + Way( + id = 47076097, + nodeIds = listOf(600397018, 600397019, 600397020), + tags = mapOf("landuse" to "farmland", "name" to "Hippiefarm"), + version = 2, + timestampEdited = Instant.parse("2012-08-12T22:14:39Z").toEpochMilliseconds() + ), +) + +val relationsList = listOf( + Relation( + id = 55555, + members = emptyList(), + tags = emptyMap(), + version = 3, + timestampEdited = Instant.parse("2021-05-08T14:14:51Z").toEpochMilliseconds() + ), + Relation( + id = 8379313, + members = listOf( + RelationMember(ElementType.NODE, 123, "something"), + RelationMember(ElementType.WAY, 234, ""), + RelationMember(ElementType.RELATION, 345, "connection"), + ), + tags = mapOf("network" to "rcn", "route" to "bicycle"), + version = 21, + timestampEdited = Instant.parse("2023-05-08T14:14:51Z").toEpochMilliseconds() + ) +) diff --git a/app/src/test/java/de/westnordost/streetcomplete/data/osmnotes/AvatarsDownloaderTest.kt b/app/src/test/java/de/westnordost/streetcomplete/data/osmnotes/AvatarsDownloaderTest.kt index 6e142befaf..0d5eeb319e 100644 --- a/app/src/test/java/de/westnordost/streetcomplete/data/osmnotes/AvatarsDownloaderTest.kt +++ b/app/src/test/java/de/westnordost/streetcomplete/data/osmnotes/AvatarsDownloaderTest.kt @@ -1,7 +1,7 @@ package de.westnordost.streetcomplete.data.osmnotes -import de.westnordost.osmapi.user.UserApi -import de.westnordost.osmapi.user.UserInfo +import de.westnordost.streetcomplete.data.user.UserApiClient +import de.westnordost.streetcomplete.data.user.UserInfo import de.westnordost.streetcomplete.testutils.mock import de.westnordost.streetcomplete.testutils.on import io.ktor.client.HttpClient @@ -13,66 +13,76 @@ import io.ktor.http.HttpStatusCode import io.ktor.utils.io.errors.IOException import kotlinx.coroutines.runBlocking import java.nio.file.Files -import kotlin.test.BeforeTest import kotlin.test.Test import kotlin.test.assertEquals class AvatarsDownloaderTest { - private val mockEngine = MockEngine { request -> when (request.url.encodedPath) { - "/NotFound" -> respondError(HttpStatusCode.NotFound) - "/ConnectionError" -> throw IOException("Cannot connect") - else -> respondOk("Image Content") - } } + private val mockEngine = MockEngine { request -> + when (request.url.encodedPath) { + "/NotFound" -> respondError(HttpStatusCode.NotFound) + "/ConnectionError" -> throw IOException("Cannot connect") + else -> respondOk("Image Content") + } + } private val tempFolder = Files.createTempDirectory("images").toFile() - private val userApi: UserApi = mock() + private val userApi: UserApiClient = mock() private val downloader = AvatarsDownloader(HttpClient(mockEngine), userApi, tempFolder) - private val userInfo = UserInfo(100, "Map Enthusiast 530") - - @BeforeTest fun setUp() { - userInfo.profileImageUrl = "http://example.com/BigImage.png" - on(userApi.get(userInfo.id)).thenReturn(userInfo) - } @Test fun `download makes GET request to profileImageUrl`() = runBlocking { - downloader.download(listOf(userInfo.id)) + val user = user() + on(userApi.get(user.id)).thenReturn(user) + + downloader.download(listOf(user.id)) assertEquals(1, mockEngine.requestHistory.size) - assertEquals(userInfo.profileImageUrl, mockEngine.requestHistory[0].url.toString()) + assertEquals(user.profileImageUrl, mockEngine.requestHistory[0].url.toString()) assertEquals(HttpMethod.Get, mockEngine.requestHistory[0].method) } @Test fun `download copies HTTP response from profileImageUrl into tempFolder`() = runBlocking { - downloader.download(listOf(userInfo.id)) + val user = user() + on(userApi.get(user.id)).thenReturn(user) + + downloader.download(listOf(user.id)) assertEquals("Image Content", tempFolder.resolve("100").readText()) } @Test fun `download does not throw exception on HTTP NotFound`() = runBlocking { - userInfo.profileImageUrl = "http://example.com/NotFound" + val user = user(profileImageUrl = "http://example.com/NotFound") + on(userApi.get(user.id)).thenReturn(user) - downloader.download(listOf(userInfo.id)) + downloader.download(listOf(user.id)) assertEquals(404, mockEngine.responseHistory[0].statusCode.value) } @Test fun `download does not throw exception on networking error`() = runBlocking { - userInfo.profileImageUrl = "http://example.com/ConnectionError" + val user = user(profileImageUrl = "http://example.com/ConnectionError") + on(userApi.get(user.id)).thenReturn(user) - downloader.download(listOf(userInfo.id)) + downloader.download(listOf(user.id)) assertEquals(0, mockEngine.responseHistory.size) } @Test fun `download does not make HTTP request if profileImageUrl is NULL`() = runBlocking { - userInfo.profileImageUrl = null + val user = user(profileImageUrl = null) + on(userApi.get(user.id)).thenReturn(user) - downloader.download(listOf(userInfo.id)) + downloader.download(listOf(user.id)) assertEquals(0, mockEngine.requestHistory.size) } + + private fun user(profileImageUrl: String? = "http://example.com/BigImage.png") = UserInfo( + id = 100, + displayName = "Map Enthusiast 530", + profileImageUrl = profileImageUrl, + ) } diff --git a/app/src/test/java/de/westnordost/streetcomplete/data/osmnotes/NotesApiClientTest.kt b/app/src/test/java/de/westnordost/streetcomplete/data/osmnotes/NotesApiClientTest.kt new file mode 100644 index 0000000000..2c0ef64c9b --- /dev/null +++ b/app/src/test/java/de/westnordost/streetcomplete/data/osmnotes/NotesApiClientTest.kt @@ -0,0 +1,140 @@ +package de.westnordost.streetcomplete.data.osmnotes + +import de.westnordost.streetcomplete.data.AuthorizationException +import de.westnordost.streetcomplete.data.ConflictException +import de.westnordost.streetcomplete.data.QueryTooBigException +import de.westnordost.streetcomplete.data.osm.mapdata.BoundingBox +import de.westnordost.streetcomplete.data.osm.mapdata.LatLon +import de.westnordost.streetcomplete.data.user.UserLoginSource +import de.westnordost.streetcomplete.testutils.OsmDevApi +import de.westnordost.streetcomplete.testutils.mock +import de.westnordost.streetcomplete.testutils.on +import io.ktor.client.HttpClient +import io.ktor.client.request.bearerAuth +import io.ktor.client.request.parameter +import io.ktor.client.request.post +import kotlinx.coroutines.runBlocking +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertFailsWith +import kotlin.test.assertNull +import kotlin.test.assertTrue + +// other than some other APIs we are speaking to, we do not control the OSM API, so I think it is +// more effective to test with the official test API instead of mocking some imagined server +// response +class NotesApiClientTest { + + private val allowEverything = mock() + private val allowNothing = mock() + private val anonymous = mock() + + init { + on(allowEverything.accessToken).thenReturn(OsmDevApi.ALLOW_EVERYTHING_TOKEN) + on(allowNothing.accessToken).thenReturn(OsmDevApi.ALLOW_NOTHING_TOKEN) + on(anonymous.accessToken).thenReturn(null) + } + + @Test fun `create note`(): Unit = runBlocking { + val note = client(allowEverything).create(LatLon(83.0, 9.0), "Created note!") + closeNote(note.id) + + assertEquals(LatLon(83.0, 9.0), note.position) + assertEquals(Note.Status.OPEN, note.status) + assertEquals(1, note.comments.size) + + val comment = note.comments.first() + assertEquals("Created note!", comment.text) + assertEquals(NoteComment.Action.OPENED, comment.action) + assertEquals("westnordost", comment.user?.displayName) + } + + @Test fun `comment note`(): Unit = runBlocking { + var note = client(allowEverything).create(LatLon(83.0, 9.1), "Created note for comment!") + note = client(allowEverything).comment(note.id, "First comment!") + closeNote(note.id) + + assertEquals(2, note.comments.size) + assertEquals("Created note for comment!", note.comments[0].text) + assertEquals(NoteComment.Action.OPENED, note.comments[0].action) + assertEquals("westnordost", note.comments[0].user?.displayName) + + assertEquals("First comment!", note.comments[1].text) + assertEquals(NoteComment.Action.COMMENTED, note.comments[1].action) + assertEquals("westnordost", note.comments[1].user?.displayName) + } + + @Test fun `comment note fails when not logged in`(): Unit = runBlocking { + val note = client(allowEverything).create(LatLon(83.0, 9.1), "Created note for comment!") + assertFailsWith { + client(anonymous).comment(note.id, "test") + } + closeNote(note.id) + } + + @Test fun `comment note fails when not authorized`(): Unit = runBlocking { + val note = client(allowEverything).create(LatLon(83.0, 9.1), "Created note for comment!") + assertFailsWith { + client(allowNothing).comment(note.id, "test") + } + closeNote(note.id) + } + + @Test fun `comment note fails when already closed`(): Unit = runBlocking { + val note = client(allowEverything).create(LatLon(83.0, 9.1), "Created note for comment!") + closeNote(note.id) + assertFailsWith { + client(allowEverything).comment(note.id, "test") + } + } + + @Test fun `get note`(): Unit = runBlocking { + val note = client(allowEverything).create(LatLon(83.0, 9.2), "Created note to get it!") + val note2 = client(anonymous).get(note.id) + closeNote(note.id) + + assertEquals(note, note2) + } + + @Test fun `get no note`(): Unit = runBlocking { + assertNull(client(anonymous).get(0)) + } + + @Test fun `get notes`(): Unit = runBlocking { + val note1 = client(allowEverything).create(LatLon(83.0, 9.3), "Note a") + val note2 = client(allowEverything).create(LatLon(83.1, 9.4), "Note b") + + val notes = client(anonymous).getAllOpen(BoundingBox(83.0, 9.3, 83.2, 9.5)) + + closeNote(note1.id) + closeNote(note2.id) + + assertTrue(notes.isNotEmpty()) + } + + @Test fun `get notes fails when bbox crosses 180th meridian`(): Unit = runBlocking { + assertFailsWith { + client(anonymous).getAllOpen(BoundingBox(0.0, 179.0, 0.1, -179.0)) + } + } + + @Test fun `get notes fails when limit is too large`(): Unit = runBlocking { + assertFailsWith { + client(anonymous).getAllOpen(BoundingBox(0.0, 0.0, 0.1, 0.1), 100000000) + } + assertFailsWith { + client(anonymous).getAllOpen(BoundingBox(0.0, 0.0, 90.0, 90.0)) + } + } + + private fun client(userLoginSource: UserLoginSource) = + NotesApiClient(HttpClient(), OsmDevApi.URL, userLoginSource, NotesApiParser()) + + // for cleanup + private fun closeNote(id: Long): Unit = runBlocking { + HttpClient().post(OsmDevApi.URL + "notes/$id/close") { + bearerAuth(OsmDevApi.ALLOW_EVERYTHING_TOKEN) + parameter("text", "") + } + } +} diff --git a/app/src/test/java/de/westnordost/streetcomplete/data/osmnotes/NotesApiParserTest.kt b/app/src/test/java/de/westnordost/streetcomplete/data/osmnotes/NotesApiParserTest.kt new file mode 100644 index 0000000000..2fb2141bd5 --- /dev/null +++ b/app/src/test/java/de/westnordost/streetcomplete/data/osmnotes/NotesApiParserTest.kt @@ -0,0 +1,126 @@ +package de.westnordost.streetcomplete.data.osmnotes + +import de.westnordost.streetcomplete.data.osm.mapdata.LatLon +import de.westnordost.streetcomplete.data.user.User +import kotlinx.datetime.Instant +import kotlin.test.Test +import kotlin.test.assertEquals + +class NotesApiParserTest { + + @Test fun `parse one minimum note`() { + val xml = """ + + + 1 + 2024-06-06 12:47:50 UTC + open + + + """.trimIndent() + + val note = Note( + position = LatLon(51.5085707, 0.0689357), + id = 1, + timestampCreated = Instant.parse("2024-06-06T12:47:50Z").toEpochMilliseconds(), + timestampClosed = null, + status = Note.Status.OPEN, + comments = listOf() + ) + + assertEquals(listOf(note), NotesApiParser().parseNotes(xml)) + } + + @Test fun `parse one full note`() { + val xml = """ + + + 1 + https://api.openstreetmap.org/api/0.6/notes/1 + https://api.openstreetmap.org/api/0.6/notes/1/comment + https://api.openstreetmap.org/api/0.6/notes/1/close + 2024-06-06 12:47:50 UTC + closed + 2024-06-06 12:47:51 UTC + + + 2024-06-06 12:47:50 UTC + 1234 + dude + https://api.openstreetmap.org/user/dude + opened + I opened it! +

Some

text

+ + + 2024-06-06 12:47:51 UTC + closed + + + + + """.trimIndent() + + val note = Note( + position = LatLon(51.5085707, 0.0689357), + id = 1, + timestampCreated = Instant.parse("2024-06-06T12:47:50Z").toEpochMilliseconds(), + timestampClosed = Instant.parse("2024-06-06T12:47:51Z").toEpochMilliseconds(), + status = Note.Status.CLOSED, + comments = listOf( + NoteComment( + timestamp = Instant.parse("2024-06-06T12:47:50Z").toEpochMilliseconds(), + action = NoteComment.Action.OPENED, + text = "I opened it!", + user = User(1234, "dude") + ), + NoteComment( + timestamp = Instant.parse("2024-06-06T12:47:51Z").toEpochMilliseconds(), + action = NoteComment.Action.CLOSED, + text = null, + user = null + ), + ) + ) + + assertEquals(listOf(note), NotesApiParser().parseNotes(xml)) + } + + @Test fun `parse several notes`() { + val xml = """ + + + 1 + 2024-06-06 12:47:50 UTC + open + + + 2 + 2024-06-06 12:47:51 UTC + hidden + + + """.trimIndent() + + val notes = listOf( + Note( + position = LatLon(51.5085707, 0.0689357), + id = 1, + timestampCreated = Instant.parse("2024-06-06T12:47:50Z").toEpochMilliseconds(), + timestampClosed = null, + status = Note.Status.OPEN, + comments = listOf() + ), + Note( + position = LatLon(51.5085709, 0.0689359), + id = 2, + timestampCreated = Instant.parse("2024-06-06T12:47:51Z").toEpochMilliseconds(), + timestampClosed = null, + status = Note.Status.HIDDEN, + comments = listOf() + ), + ) + + assertEquals(notes, NotesApiParser().parseNotes(xml)) + } +} diff --git a/app/src/test/java/de/westnordost/streetcomplete/data/osmnotes/NotesDownloaderTest.kt b/app/src/test/java/de/westnordost/streetcomplete/data/osmnotes/NotesDownloaderTest.kt index c7f20ef31d..f4ddddf450 100644 --- a/app/src/test/java/de/westnordost/streetcomplete/data/osmnotes/NotesDownloaderTest.kt +++ b/app/src/test/java/de/westnordost/streetcomplete/data/osmnotes/NotesDownloaderTest.kt @@ -14,7 +14,7 @@ import kotlin.test.Test class NotesDownloaderTest { private lateinit var noteController: NoteController - private lateinit var notesApi: NotesApi + private lateinit var notesApi: NotesApiClient @BeforeTest fun setUp() { noteController = mock() @@ -25,7 +25,7 @@ class NotesDownloaderTest { val note1 = note() val bbox = bbox() - on(notesApi.getAll(any(), anyInt(), anyInt())).thenReturn(listOf(note1)) + on(notesApi.getAllOpen(any(), anyInt())).thenReturn(listOf(note1)) val dl = NotesDownloader(notesApi, noteController) dl.download(bbox) diff --git a/app/src/test/java/de/westnordost/streetcomplete/data/osmnotes/PhotoServiceApiClientTest.kt b/app/src/test/java/de/westnordost/streetcomplete/data/osmnotes/PhotoServiceApiClientTest.kt new file mode 100644 index 0000000000..0c17f27c2d --- /dev/null +++ b/app/src/test/java/de/westnordost/streetcomplete/data/osmnotes/PhotoServiceApiClientTest.kt @@ -0,0 +1,95 @@ +package de.westnordost.streetcomplete.data.osmnotes + +import de.westnordost.streetcomplete.data.ConnectionException +import io.ktor.client.HttpClient +import io.ktor.client.engine.HttpClientEngine +import io.ktor.client.engine.mock.MockEngine +import io.ktor.client.engine.mock.respondError +import io.ktor.client.engine.mock.respondOk +import io.ktor.client.engine.mock.toByteArray +import io.ktor.http.ContentType +import io.ktor.http.HttpMethod +import io.ktor.http.HttpStatusCode +import io.ktor.utils.io.errors.IOException +import kotlinx.coroutines.runBlocking +import kotlinx.serialization.SerializationException +import kotlin.test.Test +import kotlin.test.assertContentEquals +import kotlin.test.assertEquals +import kotlin.test.assertFailsWith + +class PhotoServiceApiClientTest { + + private val picture = "src/test/resources/hai_phong_street.jpg" + + @Test + fun `upload makes POST request with file contents and returns response`() = runBlocking { + val mockEngine = MockEngine { respondOk("{\"future_url\": \"market.jpg\"}") } + val client = PhotoServiceApiClient(HttpClient(mockEngine), "http://example.com/") + + val response = client.upload(listOf(picture)) + + assertEquals(1, mockEngine.requestHistory.size) + assertEquals(HttpMethod.Post, mockEngine.requestHistory[0].method) + assertEquals("http://example.com/upload.php", mockEngine.requestHistory[0].url.toString()) + assertEquals(ContentType.Image.JPEG, mockEngine.requestHistory[0].body.contentType) + assertEquals("binary", mockEngine.requestHistory[0].headers["Content-Transfer-Encoding"]) + + assertContentEquals(listOf("market.jpg"), response) + } + + @Test + fun `upload throws ConnectionException on such errors`(): Unit = runBlocking { + val pics = listOf("src/test/resources/hai_phong_street.jpg") + + assertFailsWith(ConnectionException::class) { + client(MockEngine { throw IOException() }).upload(pics) + } + assertFailsWith(ConnectionException::class) { + client(MockEngine { respondError(HttpStatusCode.InternalServerError) }).upload(pics) + } + assertFailsWith(ConnectionException::class) { + client(MockEngine { throw SerializationException() }).upload(pics) + } + } + + @Test + fun `upload performs no requests with missing file`() = runBlocking { + val mockEngine = MockEngine { respondOk() } + val client = PhotoServiceApiClient(HttpClient(mockEngine), "http://example.com/") + + assertContentEquals(listOf(), client.upload(listOf("no-such-file-at-this-path.jpg"))) + assertEquals(0, mockEngine.requestHistory.size) + } + + @Test + fun `activate makes POST request with note ID`() = runBlocking { + val mockEngine = MockEngine { respondOk() } + val client = PhotoServiceApiClient(HttpClient(mockEngine), "http://example.com/") + + client.activate(123) + + assertEquals(1, mockEngine.requestHistory.size) + assertEquals("http://example.com/activate.php", mockEngine.requestHistory[0].url.toString()) + assertEquals(ContentType.Application.Json, mockEngine.requestHistory[0].body.contentType) + assertEquals("{\"osm_note_id\": 123}", String(mockEngine.requestHistory[0].body.toByteArray())) + } + + @Test + fun `activate throws ConnectionException on such errors`(): Unit = runBlocking { + assertFailsWith(ConnectionException::class) { + client(MockEngine { respondError(HttpStatusCode.InternalServerError) }).activate(1) + } + assertFailsWith(ConnectionException::class) { + client(MockEngine { throw IOException() }).activate(1) + } + } + + @Test + fun `activate ignores error code 410 (gone)`(): Unit = runBlocking { + client(MockEngine { respondError(HttpStatusCode.Gone) }).activate(1) + } + + private fun client(engine: HttpClientEngine) = + PhotoServiceApiClient(HttpClient(engine), "http://example.com/") +} diff --git a/app/src/test/java/de/westnordost/streetcomplete/data/osmnotes/StreetCompleteImageUploaderTest.kt b/app/src/test/java/de/westnordost/streetcomplete/data/osmnotes/StreetCompleteImageUploaderTest.kt deleted file mode 100644 index 83469a345b..0000000000 --- a/app/src/test/java/de/westnordost/streetcomplete/data/osmnotes/StreetCompleteImageUploaderTest.kt +++ /dev/null @@ -1,121 +0,0 @@ -package de.westnordost.streetcomplete.data.osmnotes - -import de.westnordost.streetcomplete.data.ConnectionException -import io.ktor.client.HttpClient -import io.ktor.client.engine.mock.MockEngine -import io.ktor.client.engine.mock.respondBadRequest -import io.ktor.client.engine.mock.respondError -import io.ktor.client.engine.mock.respondOk -import io.ktor.client.engine.mock.toByteArray -import io.ktor.http.ContentType -import io.ktor.http.HttpMethod -import io.ktor.http.HttpStatusCode -import io.ktor.utils.io.errors.IOException -import kotlinx.coroutines.runBlocking -import kotlin.test.Test -import kotlin.test.assertContentEquals -import kotlin.test.assertEquals -import kotlin.test.assertFailsWith - -class StreetCompleteImageUploaderTest { - private val mockEngine = MockEngine { request -> when (String(request.body.toByteArray())) { - // Upload requests - "valid\n" -> respondOk("{\"future_url\": \"market.jpg\"}") - "invalid\n" -> respondError(HttpStatusCode.InternalServerError) - "" -> respondBadRequest() - "ioexception\n" -> throw IOException("Unable to connect") - - // Activate requests - "{\"osm_note_id\": 180}" -> respondOk() - "{\"osm_note_id\": 190}" -> respondError(HttpStatusCode.InternalServerError) - "{\"osm_note_id\": 200}" -> respondBadRequest() - "{\"osm_note_id\": 210}" -> throw IOException("Unable to connect") - - else -> throw Exception("Invalid request body") - } } - private val uploader = StreetCompleteImageUploader(HttpClient(mockEngine), "http://example.com/" ) - - @Test - fun `upload makes POST request with file contents`() = runBlocking { - uploader.upload(listOf("src/test/resources/image_uploader/valid.jpg")) - - assertEquals(1, mockEngine.requestHistory.size) - assertEquals(HttpMethod.Post, mockEngine.requestHistory[0].method) - assertEquals("http://example.com/upload.php", mockEngine.requestHistory[0].url.toString()) - assertEquals(ContentType.Image.JPEG, mockEngine.requestHistory[0].body.contentType) - assertEquals("binary", mockEngine.requestHistory[0].headers["Content-Transfer-Encoding"]) - } - - @Test - fun `upload returns future_url value from response`() = runBlocking { - val uploads = uploader.upload(listOf("src/test/resources/image_uploader/valid.jpg")) - - assertContentEquals(listOf("market.jpg"), uploads) - } - - @Test - fun `upload throws ImageUploadServerException on 500 error`() = runBlocking { - val exception = assertFailsWith(ImageUploadServerException::class) { - uploader.upload(listOf("src/test/resources/image_uploader/invalid.jpg")) - } - - assertEquals("Upload failed: Error code 500 Internal Server Error, Message: \"Internal Server Error\"", exception.message) - } - - @Test - fun `upload throws ImageUploadClientException on 400 error`() = runBlocking { - val exception = assertFailsWith(ImageUploadClientException::class) { - uploader.upload(listOf("src/test/resources/image_uploader/empty.jpg")) - } - - assertEquals("Upload failed: Error code 400 Bad Request, Message: \"Bad Request\"", exception.message) - } - - @Test - fun `upload performs no requests with missing file`() = runBlocking { - assertContentEquals(listOf(), uploader.upload(listOf("no-such-file-at-this-path.jpg"))) - assertEquals(0, mockEngine.requestHistory.size) - } - - @Test - fun `upload throws ConnectionException on IOException`(): Unit = runBlocking { - assertFailsWith(ConnectionException::class) { - uploader.upload(listOf("src/test/resources/image_uploader/ioexception.jpg")) - } - } - - @Test - fun `activate makes POST request with note ID`() = runBlocking { - uploader.activate(180) - - assertEquals(1, mockEngine.requestHistory.size) - assertEquals("http://example.com/activate.php", mockEngine.requestHistory[0].url.toString()) - assertEquals(ContentType.Application.Json, mockEngine.requestHistory[0].body.contentType) - assertEquals("{\"osm_note_id\": 180}", String(mockEngine.requestHistory[0].body.toByteArray())) - } - - @Test - fun `activate throws ImageUploadServerException on 500 error`() = runBlocking { - val exception = assertFailsWith(ImageUploadServerException::class) { - uploader.activate(190) - } - - assertEquals("Error code 500 Internal Server Error, Message: \"Internal Server Error\"", exception.message) - } - - @Test - fun `activate throws ImageUploadClientException on 400 error`() = runBlocking { - val exception = assertFailsWith(ImageUploadClientException::class) { - uploader.activate(200) - } - - assertEquals("Error code 400 Bad Request, Message: \"Bad Request\"", exception.message) - } - - @Test - fun `activate throws ConnectionException on IOException`(): Unit = runBlocking { - assertFailsWith(ConnectionException::class) { - uploader.activate(210) - } - } -} diff --git a/app/src/test/java/de/westnordost/streetcomplete/data/osmnotes/edits/NoteEditsUploaderTest.kt b/app/src/test/java/de/westnordost/streetcomplete/data/osmnotes/edits/NoteEditsUploaderTest.kt index a288820f04..08475189d6 100644 --- a/app/src/test/java/de/westnordost/streetcomplete/data/osmnotes/edits/NoteEditsUploaderTest.kt +++ b/app/src/test/java/de/westnordost/streetcomplete/data/osmnotes/edits/NoteEditsUploaderTest.kt @@ -3,10 +3,10 @@ package de.westnordost.streetcomplete.data.osmnotes.edits import de.westnordost.streetcomplete.data.ConflictException import de.westnordost.streetcomplete.data.osm.mapdata.LatLon import de.westnordost.streetcomplete.data.osmnotes.NoteController -import de.westnordost.streetcomplete.data.osmnotes.NotesApi -import de.westnordost.streetcomplete.data.osmnotes.StreetCompleteImageUploader +import de.westnordost.streetcomplete.data.osmnotes.NotesApiClient +import de.westnordost.streetcomplete.data.osmnotes.PhotoServiceApiClient import de.westnordost.streetcomplete.data.osmtracks.Trackpoint -import de.westnordost.streetcomplete.data.osmtracks.TracksApi +import de.westnordost.streetcomplete.data.osmtracks.TracksApiClient import de.westnordost.streetcomplete.data.upload.OnUploadedChangeListener import de.westnordost.streetcomplete.data.user.UserDataSource import de.westnordost.streetcomplete.testutils.any @@ -29,15 +29,15 @@ class NoteEditsUploaderTest { private lateinit var noteController: NoteController private lateinit var noteEditsController: NoteEditsController - private lateinit var notesApi: NotesApi - private lateinit var tracksApi: TracksApi - private lateinit var imageUploader: StreetCompleteImageUploader + private lateinit var notesApi: NotesApiClient + private lateinit var tracksApi: TracksApiClient + private lateinit var imageUploader: PhotoServiceApiClient private lateinit var userDataSource: UserDataSource private lateinit var uploader: NoteEditsUploader private lateinit var listener: OnUploadedChangeListener - @BeforeTest fun setUp() { + @BeforeTest fun setUp(): Unit = runBlocking { notesApi = mock() noteController = mock() noteEditsController = mock() @@ -63,7 +63,7 @@ class NoteEditsUploaderTest { verifyNoInteractions(noteEditsController, noteController, notesApi, imageUploader) } - @Test fun `upload note comment`() { + @Test fun `upload note comment`(): Unit = runBlocking { val pos = p(1.0, 13.0) val edit = noteEdit(noteId = 1L, action = NoteEditAction.COMMENT, text = "abc", pos = pos) val note = note(id = 1L) @@ -80,7 +80,7 @@ class NoteEditsUploaderTest { verify(listener)!!.onUploaded("NOTE", pos) } - @Test fun `upload create note`() { + @Test fun `upload create note`(): Unit = runBlocking { val pos = p(1.0, 13.0) val edit = noteEdit(noteId = -5L, action = NoteEditAction.CREATE, text = "abc", pos = pos) val note = note(123) @@ -97,7 +97,7 @@ class NoteEditsUploaderTest { verify(listener)!!.onUploaded("NOTE", pos) } - @Test fun `fail uploading note comment because of a conflict`() { + @Test fun `fail uploading note comment because of a conflict`(): Unit = runBlocking { val pos = p(1.0, 13.0) val edit = noteEdit(noteId = 1L, action = NoteEditAction.COMMENT, text = "abc", pos = pos) val note = note(1) @@ -115,7 +115,7 @@ class NoteEditsUploaderTest { verify(listener)!!.onDiscarded("NOTE", pos) } - @Test fun `fail uploading note comment because note was deleted`() { + @Test fun `fail uploading note comment because note was deleted`(): Unit = runBlocking { val pos = p(1.0, 13.0) val edit = noteEdit(noteId = 1L, action = NoteEditAction.COMMENT, text = "abc", pos = pos) val note = note(1) @@ -133,7 +133,7 @@ class NoteEditsUploaderTest { verify(listener)!!.onDiscarded("NOTE", pos) } - @Test fun `upload several note edits`() { + @Test fun `upload several note edits`(): Unit = runBlocking { on(noteEditsController.getOldestUnsynced()).thenReturn(noteEdit()).thenReturn(noteEdit()).thenReturn(null) on(notesApi.comment(anyLong(), any())).thenReturn(note()) diff --git a/app/src/test/java/de/westnordost/streetcomplete/data/osmtracks/TracksApiClientTest.kt b/app/src/test/java/de/westnordost/streetcomplete/data/osmtracks/TracksApiClientTest.kt new file mode 100644 index 0000000000..cb90411768 --- /dev/null +++ b/app/src/test/java/de/westnordost/streetcomplete/data/osmtracks/TracksApiClientTest.kt @@ -0,0 +1,44 @@ +package de.westnordost.streetcomplete.data.osmtracks + +import de.westnordost.streetcomplete.data.AuthorizationException +import de.westnordost.streetcomplete.data.osm.mapdata.LatLon +import de.westnordost.streetcomplete.data.user.UserLoginSource +import de.westnordost.streetcomplete.testutils.OsmDevApi +import de.westnordost.streetcomplete.testutils.mock +import de.westnordost.streetcomplete.testutils.on +import io.ktor.client.HttpClient +import kotlinx.coroutines.runBlocking +import java.time.Instant +import kotlin.test.Test +import kotlin.test.assertFailsWith + +// other than some other APIs we are speaking to, we do not control the OSM API, so I think it is +// more effective to test with the official test API instead of mocking some imagined server +// response +class TracksApiClientTest { + + private val trackpoint = Trackpoint(LatLon(1.23, 3.45), Instant.now().toEpochMilli(), 1f, 1f) + + private val allowEverything = mock() + private val allowNothing = mock() + private val anonymous = mock() + + init { + on(allowEverything.accessToken).thenReturn(OsmDevApi.ALLOW_EVERYTHING_TOKEN) + on(allowNothing.accessToken).thenReturn(OsmDevApi.ALLOW_NOTHING_TOKEN) + on(anonymous.accessToken).thenReturn(null) + } + + @Test fun `throws exception on insufficient privileges`(): Unit = runBlocking { + assertFailsWith { client(anonymous).create(listOf(trackpoint)) } + assertFailsWith { client(allowNothing).create(listOf(trackpoint)) } + } + + @Test fun `create works without error`(): Unit = runBlocking { + client(allowEverything).create(listOf(trackpoint)) + } + + private fun client(userLoginSource: UserLoginSource) = + TracksApiClient(HttpClient(), OsmDevApi.URL, userLoginSource, TracksSerializer()) +} + diff --git a/app/src/test/java/de/westnordost/streetcomplete/data/osmtracks/TracksSerializerTest.kt b/app/src/test/java/de/westnordost/streetcomplete/data/osmtracks/TracksSerializerTest.kt new file mode 100644 index 0000000000..3dd13a33e2 --- /dev/null +++ b/app/src/test/java/de/westnordost/streetcomplete/data/osmtracks/TracksSerializerTest.kt @@ -0,0 +1,52 @@ +package de.westnordost.streetcomplete.data.osmtracks + +import de.westnordost.streetcomplete.ApplicationConstants +import de.westnordost.streetcomplete.data.osm.mapdata.LatLon +import kotlinx.datetime.Instant +import kotlin.test.Test +import kotlin.test.assertEquals + +class TracksSerializerTest { + + @Test + fun `serialize to xml`() { + val gpx = """ + + + + + + 1.23 + 4.56 + + + + 1.24 + 4.57 + + + + + """ + + val track = listOf( + Trackpoint( + position = LatLon(12.34, 56.78), + time = Instant.parse("2024-06-05T09:51:14Z").toEpochMilliseconds(), + accuracy = 4.56f, + elevation = 1.23f + ), + Trackpoint( + position = LatLon(12.3999, 56.7999), + time = Instant.parse("2024-06-05T09:51:15Z").toEpochMilliseconds(), + accuracy = 4.57f, + elevation = 1.24f + ), + ) + + assertEquals( + gpx.replace(Regex("[\n\r] *"), ""), + TracksSerializer().serialize(track) + ) + } +} diff --git a/app/src/test/java/de/westnordost/streetcomplete/data/user/UserApiClientTest.kt b/app/src/test/java/de/westnordost/streetcomplete/data/user/UserApiClientTest.kt new file mode 100644 index 0000000000..74b514d6ae --- /dev/null +++ b/app/src/test/java/de/westnordost/streetcomplete/data/user/UserApiClientTest.kt @@ -0,0 +1,55 @@ +package de.westnordost.streetcomplete.data.user + +import de.westnordost.streetcomplete.data.AuthorizationException +import de.westnordost.streetcomplete.testutils.OsmDevApi +import de.westnordost.streetcomplete.testutils.mock +import de.westnordost.streetcomplete.testutils.on +import io.ktor.client.HttpClient +import kotlinx.coroutines.runBlocking +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertFailsWith +import kotlin.test.assertNotNull + +// other than some other APIs we are speaking to, we do not control the OSM API, so I think it is +// more effective to test with the official test API instead of mocking some imagined server +// response +class UserApiClientTest { + private val allowEverything = mock() + private val allowNothing = mock() + private val anonymous = mock() + + init { + on(allowEverything.accessToken).thenReturn(OsmDevApi.ALLOW_EVERYTHING_TOKEN) + on(allowNothing.accessToken).thenReturn(OsmDevApi.ALLOW_NOTHING_TOKEN) + on(anonymous.accessToken).thenReturn(null) + } + + @Test + fun get(): Unit = runBlocking { + val info = client(anonymous).get(3625) + + assertNotNull(info) + assertEquals(3625, info.id) + assertEquals("westnordost", info.displayName) + assertNotNull(info.profileImageUrl) + } + + @Test + fun getMine(): Unit = runBlocking { + val info = client(allowEverything).getMine() + + assertNotNull(info) + assertEquals(3625, info.id) + assertEquals("westnordost", info.displayName) + assertNotNull(info.profileImageUrl) + } + + @Test + fun `getMine fails when not logged in`(): Unit = runBlocking { + assertFailsWith { client(anonymous).getMine() } + } + + private fun client(userLoginSource: UserLoginSource) = + UserApiClient(HttpClient(), OsmDevApi.URL, userLoginSource, UserApiParser()) +} diff --git a/app/src/test/java/de/westnordost/streetcomplete/data/user/UserApiParserTest.kt b/app/src/test/java/de/westnordost/streetcomplete/data/user/UserApiParserTest.kt new file mode 100644 index 0000000000..f9cdb88a20 --- /dev/null +++ b/app/src/test/java/de/westnordost/streetcomplete/data/user/UserApiParserTest.kt @@ -0,0 +1,61 @@ +package de.westnordost.streetcomplete.data.user + +import kotlin.test.Test +import kotlin.test.assertEquals + +class UserApiParserTest { + + @Test + fun `parse minimum user info`() { + val xml = """""".trimIndent() + + assertEquals( + UserInfo( + id = 1234, + displayName = "Max Muster", + profileImageUrl = null + ), + UserApiParser().parseUsers(xml).single() + ) + } + + @Test + fun `parse full user info`() { + val xml = """ + + + + + + + + + + + + The description of your profile + + de-DE + de + en-US + en + + + + + + + + """.trimIndent() + + assertEquals( + UserInfo( + id = 1234, + displayName = "Max Muster", + profileImageUrl = "https://www.openstreetmap.org/attachments/users/images/000/000/1234/original/someLongURLOrOther.JPG", + unreadMessagesCount = 0, + ), + UserApiParser().parseUsers(xml).single() + ) + } +} diff --git a/app/src/test/java/de/westnordost/streetcomplete/data/user/oauth/OAuthAuthorizationTest.kt b/app/src/test/java/de/westnordost/streetcomplete/data/user/oauth/OAuthAuthorizationTest.kt index c27e5297f0..22600cafa4 100644 --- a/app/src/test/java/de/westnordost/streetcomplete/data/user/oauth/OAuthAuthorizationTest.kt +++ b/app/src/test/java/de/westnordost/streetcomplete/data/user/oauth/OAuthAuthorizationTest.kt @@ -1,5 +1,6 @@ package de.westnordost.streetcomplete.data.user.oauth +import de.westnordost.streetcomplete.data.ConnectionException import io.ktor.client.HttpClient import io.ktor.client.engine.mock.MockEngine import io.ktor.client.engine.mock.respondError @@ -82,40 +83,40 @@ class OAuthAuthorizationTest { @Test fun `extractAuthorizationCode fails with useful error messages`(): Unit = runBlocking { val oauth = createOAuth() - val service = OAuthService(HttpClient(MockEngine { respondOk() })) + val service = OAuthApiClient(HttpClient(MockEngine { respondOk() })) // server did not respond correctly with "error" - assertFailsWith { - service.retrieveAccessToken(oauth, "localhost://oauth?e=something") + assertFailsWith { + service.getAccessToken(oauth, "localhost://oauth?e=something") } try { - service.retrieveAccessToken(oauth, "localhost://oauth?error=hey%2Bwhat%27s%2Bup") + service.getAccessToken(oauth, "localhost://oauth?error=hey%2Bwhat%27s%2Bup") } catch (e: OAuthException) { assertEquals("hey what's up", e.message) } try { - service.retrieveAccessToken(oauth, "localhost://oauth?error=A%21&error_description=B%21") + service.getAccessToken(oauth, "localhost://oauth?error=A%21&error_description=B%21") } catch (e: OAuthException) { assertEquals("A!: B!", e.message) } try { - service.retrieveAccessToken(oauth, "localhost://oauth?error=A%21&error_uri=http%3A%2F%2Fabc.de") + service.getAccessToken(oauth, "localhost://oauth?error=A%21&error_uri=http%3A%2F%2Fabc.de") } catch (e: OAuthException) { assertEquals("A! (see http://abc.de)", e.message) } try { - service.retrieveAccessToken(oauth, "localhost://oauth?error=A%21&error_description=B%21&error_uri=http%3A%2F%2Fabc.de") + service.getAccessToken(oauth, "localhost://oauth?error=A%21&error_description=B%21&error_uri=http%3A%2F%2Fabc.de") } catch (e: OAuthException) { assertEquals("A!: B! (see http://abc.de)", e.message) } } @Test fun extractAuthorizationCode() = runBlocking { - val service = OAuthService(HttpClient(MockEngine { request -> + val service = OAuthApiClient(HttpClient(MockEngine { request -> if (request.url.parameters["code"] == "my code") { respondOk("""{ "access_token": "TOKEN", @@ -130,20 +131,20 @@ class OAuthAuthorizationTest { assertEquals( AccessTokenResponse("TOKEN", listOf("A", "B", "C")), - service.retrieveAccessToken(oauth, "localhost://oauth?code=my%20code") + service.getAccessToken(oauth, "localhost://oauth?code=my%20code") ) } @Test fun `retrieveAccessToken throws OAuthConnectionException with invalid response token_type`(): Unit = runBlocking { - val service = OAuthService(HttpClient(MockEngine { respondOk("""{ + val service = OAuthApiClient(HttpClient(MockEngine { respondOk("""{ "access_token": "TOKEN", "token_type": "an_unusual_token_type", "scope": "A B C" }""") })) - val exception = assertFailsWith { - service.retrieveAccessToken(dummyOAuthAuthorization(), "localhost://oauth?code=code") + val exception = assertFailsWith { + service.getAccessToken(dummyOAuthAuthorization(), "localhost://oauth?code=code") } assertEquals( @@ -153,7 +154,7 @@ class OAuthAuthorizationTest { } @Test fun `retrieveAccessToken throws OAuthException when error response`(): Unit = runBlocking { - val service = OAuthService(HttpClient(MockEngine { respondError( + val service = OAuthApiClient(HttpClient(MockEngine { respondError( HttpStatusCode.BadRequest, """{ "error": "Missing auth code", "error_description": "Please specify a code", @@ -162,7 +163,7 @@ class OAuthAuthorizationTest { ) })) val exception = assertFailsWith { - service.retrieveAccessToken(dummyOAuthAuthorization(), "localhost://oauth?code=code") + service.getAccessToken(dummyOAuthAuthorization(), "localhost://oauth?code=code") } assertEquals("Missing auth code", exception.error) @@ -181,7 +182,7 @@ class OAuthAuthorizationTest { "scheme://there" ) - assertFails { OAuthService(HttpClient(mockEngine)).retrieveAccessToken(auth, "scheme://there?code=C0D3") } + assertFails { OAuthApiClient(HttpClient(mockEngine)).getAccessToken(auth, "scheme://there?code=C0D3") } val expectedParams = ParametersBuilder() expectedParams.append("grant_type", "authorization_code") @@ -199,7 +200,7 @@ class OAuthAuthorizationTest { val mockEngine = MockEngine { respondOk() } assertFails { - OAuthService(HttpClient(mockEngine)).retrieveAccessToken(dummyOAuthAuthorization(), "localhost://oauth?code=code") + OAuthApiClient(HttpClient(mockEngine)).getAccessToken(dummyOAuthAuthorization(), "localhost://oauth?code=code") } val expectedHeaders = HeadersBuilder() diff --git a/app/src/test/java/de/westnordost/streetcomplete/data/user/statistics/StatisticsApiClientTest.kt b/app/src/test/java/de/westnordost/streetcomplete/data/user/statistics/StatisticsApiClientTest.kt new file mode 100644 index 0000000000..4549aa45b0 --- /dev/null +++ b/app/src/test/java/de/westnordost/streetcomplete/data/user/statistics/StatisticsApiClientTest.kt @@ -0,0 +1,57 @@ +package de.westnordost.streetcomplete.data.user.statistics + +import de.westnordost.streetcomplete.data.ApiClientException +import de.westnordost.streetcomplete.testutils.mock +import de.westnordost.streetcomplete.testutils.on +import io.ktor.client.HttpClient +import io.ktor.client.engine.mock.MockEngine +import io.ktor.client.engine.mock.respondBadRequest +import io.ktor.client.engine.mock.respondOk +import kotlinx.coroutines.runBlocking +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertFailsWith + +class StatisticsApiClientTest { + private val statisticsParser: StatisticsParser = mock() + + private val validResponseMockEngine = MockEngine { respondOk("simple response") } + + @Test fun `download parses all statistics`() = runBlocking { + val client = StatisticsApiClient(HttpClient(validResponseMockEngine), "", statisticsParser) + val stats = Statistics( + types = listOf(), + countries = listOf(), + rank = 2, + daysActive = 100, + currentWeekRank = 50, + currentWeekTypes = listOf(), + currentWeekCountries = listOf(), + activeDates = listOf(), + activeDatesRange = 100, + isAnalyzing = false, + lastUpdate = 10 + ) + on(statisticsParser.parse("simple response")).thenReturn(stats) + assertEquals(stats, client.get(100)) + } + + @Test fun `download throws Exception for a 400 response`(): Unit = runBlocking { + val mockEngine = MockEngine { _ -> respondBadRequest() } + val client = StatisticsApiClient(HttpClient(mockEngine), "", statisticsParser) + assertFailsWith { client.get(100) } + } + + @Test fun `download constructs request URL`() = runBlocking { + StatisticsApiClient( + HttpClient(validResponseMockEngine), + "https://example.com/stats/", + statisticsParser + ).get(100) + + assertEquals( + "https://example.com/stats/?user_id=100", + validResponseMockEngine.requestHistory[0].url.toString() + ) + } +} diff --git a/app/src/test/java/de/westnordost/streetcomplete/data/user/statistics/StatisticsDownloaderTest.kt b/app/src/test/java/de/westnordost/streetcomplete/data/user/statistics/StatisticsDownloaderTest.kt deleted file mode 100644 index 03ec69519c..0000000000 --- a/app/src/test/java/de/westnordost/streetcomplete/data/user/statistics/StatisticsDownloaderTest.kt +++ /dev/null @@ -1,44 +0,0 @@ -package de.westnordost.streetcomplete.data.user.statistics - -import de.westnordost.streetcomplete.testutils.mock -import de.westnordost.streetcomplete.testutils.on -import io.ktor.client.HttpClient -import io.ktor.client.engine.mock.MockEngine -import io.ktor.client.engine.mock.respondBadRequest -import io.ktor.client.engine.mock.respondOk -import kotlinx.coroutines.runBlocking -import kotlin.test.Test -import kotlin.test.assertEquals -import kotlin.test.assertFails - -class StatisticsDownloaderTest { - private val statisticsParser: StatisticsParser = mock() - - private val validResponseMockEngine = MockEngine { _ -> respondOk("simple response") } - - @Test fun `download parses all statistics`() = runBlocking { - val stats = Statistics(types = listOf(), countries = listOf(), rank = 2, daysActive = 100, currentWeekRank = 50, currentWeekTypes = listOf(), currentWeekCountries = listOf(), activeDates = listOf(), activeDatesRange = 100, isAnalyzing = false, lastUpdate = 10) - on(statisticsParser.parse("simple response")).thenReturn(stats) - assertEquals(stats, StatisticsDownloader(HttpClient(validResponseMockEngine), "", statisticsParser).download(100)) - } - - @Test fun `download throws Exception for a 400 response`() = runBlocking { - val mockEngine = MockEngine { _ -> respondBadRequest() } - val exception = assertFails { StatisticsDownloader(HttpClient(mockEngine), "", statisticsParser).download(100) } - - assertEquals( - "Client request(GET http://localhost/?user_id=100) invalid: 400 Bad Request. Text: \"Bad Request\"", - exception.message - ) - } - - @Test fun `download constructs request URL`() = runBlocking { - StatisticsDownloader( - HttpClient(validResponseMockEngine), - "https://example.com/stats/", - statisticsParser - ).download(100) - - assertEquals("https://example.com/stats/?user_id=100", validResponseMockEngine.requestHistory[0].url.toString()) - } -} diff --git a/app/src/test/java/de/westnordost/streetcomplete/testutils/OsmDevApi.kt b/app/src/test/java/de/westnordost/streetcomplete/testutils/OsmDevApi.kt new file mode 100644 index 0000000000..8e7f24db9a --- /dev/null +++ b/app/src/test/java/de/westnordost/streetcomplete/testutils/OsmDevApi.kt @@ -0,0 +1,9 @@ +package de.westnordost.streetcomplete.testutils + +object OsmDevApi { + const val URL = "https://master.apis.dev.openstreetmap.org/api/0.6/" + + // copied straight from osmapi (Java), because, why not + const val ALLOW_EVERYTHING_TOKEN = "qzaxWiG2tprF1IfEcwf4-mn7Al4f2lsM3CNrvGEaIL0" + const val ALLOW_NOTHING_TOKEN = "fp2SjHKQ55rSdI2x4FN_s0wNUh67dgNbf9x3WdjCa5Y" +} diff --git a/app/src/test/resources/image_uploader/empty.jpg b/app/src/test/resources/image_uploader/empty.jpg deleted file mode 100644 index e69de29bb2..0000000000 diff --git a/app/src/test/resources/image_uploader/invalid.jpg b/app/src/test/resources/image_uploader/invalid.jpg deleted file mode 100644 index 9977a2836c..0000000000 --- a/app/src/test/resources/image_uploader/invalid.jpg +++ /dev/null @@ -1 +0,0 @@ -invalid diff --git a/app/src/test/resources/image_uploader/ioexception.jpg b/app/src/test/resources/image_uploader/ioexception.jpg deleted file mode 100644 index abfff0458b..0000000000 --- a/app/src/test/resources/image_uploader/ioexception.jpg +++ /dev/null @@ -1 +0,0 @@ -ioexception diff --git a/app/src/test/resources/image_uploader/valid.jpg b/app/src/test/resources/image_uploader/valid.jpg deleted file mode 100644 index 1e2466dfad..0000000000 --- a/app/src/test/resources/image_uploader/valid.jpg +++ /dev/null @@ -1 +0,0 @@ -valid