Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wrap some osmapi exceptions in Kotlin exceptions #2821

Merged
merged 3 commits into from
May 2, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 9 additions & 17 deletions app/src/main/java/de/westnordost/streetcomplete/MainActivity.kt
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,6 @@ import androidx.core.content.getSystemService
import androidx.lifecycle.lifecycleScope
import androidx.localbroadcastmanager.content.LocalBroadcastManager
import androidx.preference.PreferenceManager
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.streetcomplete.Injector.applicationComponent
import de.westnordost.streetcomplete.controls.NotificationButtonFragment
import de.westnordost.streetcomplete.data.download.DownloadController
Expand All @@ -33,10 +29,12 @@ import de.westnordost.streetcomplete.data.notifications.Notification
import de.westnordost.streetcomplete.data.quest.Quest
import de.westnordost.streetcomplete.data.quest.QuestAutoSyncer
import de.westnordost.streetcomplete.data.UnsyncedChangesCountSource
import de.westnordost.streetcomplete.data.download.ConnectionException
import de.westnordost.streetcomplete.data.osm.mapdata.LatLon
import de.westnordost.streetcomplete.data.upload.UploadController
import de.westnordost.streetcomplete.data.upload.UploadProgressListener
import de.westnordost.streetcomplete.data.upload.VersionBannedException
import de.westnordost.streetcomplete.data.user.AuthorizationException
import de.westnordost.streetcomplete.data.user.UserController
import de.westnordost.streetcomplete.ktx.toast
import de.westnordost.streetcomplete.location.LocationRequestFragment
Expand Down Expand Up @@ -233,11 +231,11 @@ class MainActivity : AppCompatActivity(),
messageView.movementMethod = LinkMovementMethod.getInstance()
Linkify.addLinks(messageView, Linkify.WEB_URLS)
}
} else if (e is OsmConnectionException) {
// a 5xx error is not the fault of this app. Nothing we can do about it, so
// just notify the user
} else if (e is ConnectionException) {
// A network connection 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.
toast(R.string.upload_server_error, Toast.LENGTH_LONG)
} else if (e is OsmAuthorizationException) {
} else if (e is AuthorizationException) {
// delete secret in case it failed while already having a token -> token is invalid
userController.logOut()
RequestLoginDialog(this@MainActivity).show()
Expand All @@ -253,15 +251,9 @@ class MainActivity : AppCompatActivity(),
private val downloadProgressListener: DownloadProgressListener = object : DownloadProgressListener {
@AnyThread override fun onError(e: Exception) {
runOnUiThread {
// a 5xx 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. Further, we treat
// the following errors the same as a (temporary) connection error:
// - an invalid response (OsmApiReadResponseException)
// - request timeout (OsmApiException with error code 408)
val isEnvironmentError = e is OsmConnectionException ||
e is OsmApiReadResponseException ||
e is OsmApiException && e.errorCode == 408
if (isEnvironmentError) {
// A network connection 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.
if (e is ConnectionException) {
toast(R.string.download_server_error, Toast.LENGTH_LONG)
} else {
crashReportExceptionHandler.askUserToSendErrorReport(this@MainActivity, R.string.download_error, e)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
package de.westnordost.streetcomplete.data.download

class ConnectionException @JvmOverloads constructor(
message: String? = null, cause: Throwable? = null) : RuntimeException(message, cause)
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
package de.westnordost.streetcomplete.data.download

class QueryTooBigException @JvmOverloads constructor(
message: String? = null, cause: Throwable? = null) : RuntimeException(message, cause)
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
package de.westnordost.streetcomplete.data.osm.edits.upload

import de.westnordost.osmapi.common.errors.OsmApiException
import de.westnordost.osmapi.common.errors.OsmConflictException
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.OpenQuestChangesetsManager
Expand All @@ -25,21 +23,10 @@ class ElementEditUploader @Inject constructor(

return try {
val changesetId = changesetManager.getOrCreateChangeset(edit.questType, edit.source)
uploadInChangeset(changesetId, mapDataChanges)
mapDataApi.uploadChanges(changesetId, mapDataChanges)
} catch (e: ConflictException) {
val changesetId = changesetManager.createChangeset(edit.questType, edit.source)
uploadInChangeset(changesetId, mapDataChanges)
}
}

/** Upload the changes for a single change. Returns the updated element(s). */
private fun uploadInChangeset(changesetId: Long, mapDataChanges: MapDataChanges): MapDataUpdates {
try {
return mapDataApi.uploadChanges(changesetId, mapDataChanges)
} catch (e: OsmConflictException) {
throw ConflictException(e.message, e)
} catch (e: OsmApiException) {
throw ConflictException(e.message, e)
mapDataApi.uploadChanges(changesetId, mapDataChanges)
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,9 @@ import javax.inject.Inject

import androidx.work.Worker
import androidx.work.WorkerParameters
import de.westnordost.osmapi.common.errors.OsmAuthorizationException
import de.westnordost.osmapi.common.errors.OsmConnectionException
import de.westnordost.streetcomplete.Injector
import de.westnordost.streetcomplete.data.download.ConnectionException
import de.westnordost.streetcomplete.data.user.AuthorizationException

class ChangesetAutoCloserWorker(context: Context, workerParams: WorkerParameters) :
Worker(context, workerParams) {
Expand All @@ -22,11 +22,11 @@ class ChangesetAutoCloserWorker(context: Context, workerParams: WorkerParameters
override fun doWork(): Result {
try {
openQuestChangesetsManager.closeOldChangesets()
} catch (e: OsmConnectionException) {
} catch (e: ConnectionException) {
// wasn't able to connect to the server (i.e. connection timeout). Oh well, then,
// never mind. Could also retry later with Result.retry() but the OSM API closes open
// changesets after 1 hour anyway.
} catch (e: OsmAuthorizationException) {
} catch (e: AuthorizationException) {
// the user may not be authorized yet (or not be authorized anymore) #283
// nothing we can do about here. He will have to reauthenticate when he next opens the app
return Result.failure()
Expand Down
Original file line number Diff line number Diff line change
@@ -1,14 +1,13 @@
package de.westnordost.streetcomplete.data.osm.edits.upload.changesets

import android.util.Log
import de.westnordost.streetcomplete.data.osm.mapdata.MapDataApi

import de.westnordost.osmapi.common.errors.OsmConflictException
import de.westnordost.streetcomplete.ApplicationConstants.QUESTTYPE_TAG_KEY
import de.westnordost.streetcomplete.ApplicationConstants.USER_AGENT
import de.westnordost.streetcomplete.data.osm.edits.upload.LastEditTimeStore
import de.westnordost.streetcomplete.data.quest.QuestType
import de.westnordost.streetcomplete.data.osm.mapdata.MapDataApi
import de.westnordost.streetcomplete.data.osm.osmquests.OsmElementQuestType
import de.westnordost.streetcomplete.data.quest.QuestType
import de.westnordost.streetcomplete.data.upload.ConflictException
import de.westnordost.streetcomplete.ktx.toBcp47LanguageTag
import java.util.Locale
import javax.inject.Inject
Expand Down Expand Up @@ -45,7 +44,7 @@ class OpenQuestChangesetsManager @Inject constructor(
try {
mapDataApi.closeChangeset(info.changesetId)
Log.i(TAG, "Closed changeset #${info.changesetId}")
} catch (e: OsmConflictException) {
} catch (e: ConflictException) {
Log.w(TAG, "Couldn't close changeset #${info.changesetId} because it has already been closed")
} finally {
openChangesetsDB.delete(info.questType, info.source)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
package de.westnordost.streetcomplete.data.osm.mapdata

import de.westnordost.osmapi.common.errors.*
import de.westnordost.streetcomplete.data.download.ConnectionException
import de.westnordost.streetcomplete.data.download.QueryTooBigException
import de.westnordost.streetcomplete.data.upload.ConflictException
import de.westnordost.streetcomplete.data.user.AuthorizationException

// TODO create own exception classes
/** Get and upload changes to map data */
interface MapDataApi : MapDataRepository {

Expand All @@ -12,15 +14,12 @@ interface MapDataApi : MapDataRepository {
* @param changesetId id of the changeset to upload changes into
* @param changes changes to upload.
*
* @throws OsmNotFoundException if the changeset does not exist (yet) or an element in the
* does not exist
* @throws OsmConflictException 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 OsmAuthorizationException if the application does not have permission to edit the
* map (Permission.MODIFY_MAP)
* @throws OsmPreconditionFailedException if the deletion of an element was uploaded but that
* element is still referred to by another element
* @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
*/
Expand All @@ -31,8 +30,10 @@ interface MapDataApi : MapDataRepository {
*
* @param tags tags of this changeset. Usually it is comment and source.
*
* @throws OsmAuthorizationException if the application does not have permission to edit the
* map (Permission.MODIFY_MAP)
* @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<String, String?>): Long
Expand All @@ -42,10 +43,10 @@ interface MapDataApi : MapDataRepository {
*
* @param changesetId id of the changeset to close
*
* @throws OsmConflictException if the changeset has already been closed
* @throws OsmNotFoundException if the changeset does not exist (yet)
* @throws OsmAuthorizationException if the application does not have permission to edit the
* map (Permission.MODIFY_MAP)
* @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)

Expand All @@ -58,8 +59,9 @@ interface MapDataApi : MapDataRepository {
* @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 OsmQueryTooBigException if the bounds are is too large
* @throws QueryTooBigException if the bounds area is too large
* @throws IllegalArgumentException if the bounds cross the 180th meridian.
FloEdelmann marked this conversation as resolved.
Show resolved Hide resolved
* @throws ConnectionException if a temporary network connection problem occurs
*
* @return the map data
*/
Expand All @@ -71,7 +73,7 @@ interface MapDataApi : MapDataRepository {
*
* @param id the way's id
*
* @throws OsmNotFoundException if the way with the given id does not exist
* @throws ConnectionException if a temporary network connection problem occurs
*
* @return the map data
*/
Expand All @@ -84,7 +86,7 @@ interface MapDataApi : MapDataRepository {
*
* @param id the relation's id
*
* @throws OsmNotFoundException if the relation with the given id does not exist
* @throws ConnectionException if a temporary network connection problem occurs
*
* @return the map data
*/
Expand All @@ -95,6 +97,8 @@ interface MapDataApi : MapDataRepository {
*
* @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?
Expand All @@ -104,6 +108,8 @@ interface MapDataApi : MapDataRepository {
*
* @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?
Expand All @@ -113,6 +119,8 @@ interface MapDataApi : MapDataRepository {
*
* @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?
Expand All @@ -122,6 +130,8 @@ interface MapDataApi : MapDataRepository {
*
* @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<Way>
Expand All @@ -131,6 +141,8 @@ interface MapDataApi : MapDataRepository {
*
* @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<Relation>
Expand All @@ -140,6 +152,8 @@ interface MapDataApi : MapDataRepository {
*
* @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<Relation>
Expand All @@ -149,6 +163,8 @@ interface MapDataApi : MapDataRepository {
*
* @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<Relation>
Expand Down
Loading