[mage-1437] (#84)

1) Refactor observation and attachment sync worker classes to make the sync logic clearer
2) Remove bitwise operators for the retry determination logic

Author: nruss11-om

mage/src/main/java/mil/nga/giat/mage/observation/sync/AttachmentSyncWorker.kt

@@ -13,7 +13,6 @@ import mil.nga.giat.mage.MageApplication
 import mil.nga.giat.mage.R
 import mil.nga.giat.mage.data.repository.observation.AttachmentRepository
 import mil.nga.giat.mage.data.datasource.observation.AttachmentLocalDataSource
-import java.net.HttpURLConnection
 import java.util.concurrent.TimeUnit
 
 @HiltWorker
@@ -27,15 +26,34 @@ class AttachmentSyncWorker @AssistedInject constructor(
    override suspend fun doWork(): Result {
       // Lock to ensure previous running work will complete when cancelled before new work is started.
       return mutex.withLock {
-         val result = try {
-            syncAttachments()
+         try {
+            //submit observations to the server that are marked as "dirty" and track the overall response flag of all transactions
+            val overallResponseFlag = syncAttachments()
+
+            //if any of the transactions returned RetryFlag, then Result.retry() will be returned to retry the work request per back off policy
+            //this should not result in the successful transactions being resubmitted, as they will no longer be marked as "dirty"
+            when (overallResponseFlag) {
+               ResponseFlag.SuccessFlag -> Result.success()
+               ResponseFlag.FailureFlag -> Result.failure()
+               ResponseFlag.RetryFlag -> Result.retry()
+            }
          } catch (e: Exception) {
             Log.e(LOG_NAME, "Failed to sync attachments", e)
-            RESULT_RETRY_FLAG
+            //any unhandled exception should result in a retry
+            Result.retry()
          }
+      }
+   }
 
-         if (result.containsFlag(RESULT_RETRY_FLAG)) Result.retry() else Result.success()
+   private suspend fun syncAttachments(): ResponseFlag {
+      var overallResult: ResponseFlag = ResponseFlag.SuccessFlag
+
+      for (attachment in attachmentLocalDataSource.dirtyAttachments.filter { !it.observation.remoteId.isNullOrEmpty() && it.url.isNullOrEmpty() }) {
+         val response = ResponseFlag.processResponse(attachmentRepository.syncAttachment(attachment))
+         overallResult = ResponseFlag.combineResponseFlags(overallResult, response)
       }
+
+      return overallResult
    }
 
    override suspend fun getForegroundInfo(): ForegroundInfo {
@@ -50,44 +68,8 @@ class AttachmentSyncWorker @AssistedInject constructor(
       return ForegroundInfo(ATTACHMENT_SYNC_NOTIFICATION_ID, notification)
    }
 
-   private suspend fun syncAttachments(): Int {
-      var result = RESULT_SUCCESS_FLAG
-
-      for (attachment in attachmentLocalDataSource.dirtyAttachments.filter { !it.observation.remoteId.isNullOrEmpty() && it.url.isNullOrEmpty() }) {
-         val response = attachmentRepository.syncAttachment(attachment)
-         result = if (response.isSuccessful) {
-            Result.success()
-         } else {
-            if (response.code() == HttpURLConnection.HTTP_UNAUTHORIZED) {
-               Result.failure()
-            } else {
-               Result.retry()
-            }
-         }.withFlag(result)
-      }
-
-      return result
-   }
-
-   private fun Result.withFlag(flag: Int): Int {
-      return when (this) {
-         is Result.Success -> RESULT_FAILURE_FLAG or flag
-         is Result.Retry -> RESULT_RETRY_FLAG or flag
-         else -> RESULT_SUCCESS_FLAG or flag
-      }
-   }
-
-   private fun Int.containsFlag(flag: Int): Boolean {
-      return (this or flag) == this
-   }
-
    companion object {
       private val LOG_NAME = AttachmentSyncWorker::class.java.simpleName
-
-      private const val RESULT_SUCCESS_FLAG = 0
-      private const val RESULT_FAILURE_FLAG = 1
-      private const val RESULT_RETRY_FLAG = 2
-
       private const val ATTACHMENT_SYNC_WORK = "mil.nga.mage.ATTACHMENT_SYNC_WORK"
       private const val ATTACHMENT_SYNC_NOTIFICATION_ID = 200
 

mage/src/main/java/mil/nga/giat/mage/observation/sync/ObservationSyncWorker.kt

@@ -16,7 +16,6 @@ import mil.nga.giat.mage.database.model.observation.Observation
 import mil.nga.giat.mage.database.model.observation.ObservationFavorite
 import mil.nga.giat.mage.data.datasource.observation.ObservationLocalDataSource
 import mil.nga.giat.mage.database.model.observation.State
-import java.net.HttpURLConnection
 import java.util.concurrent.TimeUnit
 
 @HiltWorker
@@ -28,144 +27,117 @@ class ObservationSyncWorker @AssistedInject constructor(
 ) : CoroutineWorker(context, params) {
 
     override suspend fun doWork(): Result {
-        // Lock to ensure previous running work will complete when cancelled before new work is started.
+        //Lock to ensure previous running work will complete when cancelled before new work is started.
         return mutex.withLock {
-            var result = RESULT_SUCCESS_FLAG
-
             try {
-                result = syncObservations().withFlag(result)
-                result = syncObservationImportant().withFlag(result)
-                result = syncObservationFavorites().withFlag(result)
+                //submit observations to the server that are marked as "dirty" and track the overall response flag of all transactions
+                val overallResponseFlag = ResponseFlag.combineResponseFlags(
+                    syncObservations(),
+                    syncObservationImportant(),
+                    syncObservationFavorites()
+                )
+
+                //if any of the transactions returned RetryFlag, then Result.retry() will be returned to retry the work request per back off policy
+                //this should not result in the successful transactions being resubmitted, as they will no longer be marked as "dirty"
+                when (overallResponseFlag) {
+                    ResponseFlag.SuccessFlag -> Result.success()
+                    ResponseFlag.FailureFlag -> Result.failure()
+                    ResponseFlag.RetryFlag -> Result.retry()
+                }
             } catch (e: Exception) {
-                result = RESULT_RETRY_FLAG
                 Log.e(LOG_NAME, "Error trying to sync observations with server", e)
+                //any unhandled exception should result in a retry
+                Result.retry()
             }
-
-            if (result.containsFlag(RESULT_RETRY_FLAG)) Result.retry() else Result.success()
         }
     }
 
-    override suspend fun getForegroundInfo(): ForegroundInfo {
-        val notification = NotificationCompat.Builder(applicationContext, MageApplication.MAGE_NOTIFICATION_CHANNEL_ID)
-            .setSmallIcon(R.drawable.ic_sync_preference_24dp)
-            .setContentTitle("Sync Observations")
-            .setContentText("Pushing observation updates to MAGE.")
-            .setPriority(NotificationCompat.PRIORITY_MAX)
-            .setAutoCancel(true)
-            .build()
-
-        return ForegroundInfo(OBSERVATION_SYNC_NOTIFICATION_ID, notification)
-    }
-
-    private suspend fun syncObservations(): Int {
-        var result = RESULT_SUCCESS_FLAG
+    private suspend fun syncObservations(): ResponseFlag {
+        var overallResult: ResponseFlag = ResponseFlag.SuccessFlag
 
         for (observation in observationLocalDataSource.dirty) {
-            result = syncObservation(observation).withFlag(result)
+            val syncResult = syncObservation(observation)
+            overallResult = ResponseFlag.combineResponseFlags(overallResult, syncResult)
         }
 
-        return result
+        return overallResult
     }
 
-    private suspend fun syncObservation(observation: Observation): Int {
-        val result = RESULT_SUCCESS_FLAG
-
+    private suspend fun syncObservation(observation: Observation): ResponseFlag {
         return if (observation.state == State.ARCHIVE) {
-            archive(observation).withFlag(result)
+            archive(observation)
         } else {
-            save(observation).withFlag(result)
+            save(observation)
         }
     }
 
-    private suspend fun syncObservationImportant(): Int {
-        var result = RESULT_SUCCESS_FLAG
+    private suspend fun syncObservationImportant(): ResponseFlag {
+        var overallResult: ResponseFlag = ResponseFlag.SuccessFlag
 
         for (observation in observationLocalDataSource.dirtyImportant) {
-            result = updateImportant(observation).withFlag(result)
+            val updateResult = updateImportant(observation)
+            overallResult = ResponseFlag.combineResponseFlags(overallResult, updateResult)
         }
 
-        return result
+        return overallResult
     }
 
-    private suspend fun syncObservationFavorites(): Int {
-        var result = RESULT_SUCCESS_FLAG
+    private suspend fun syncObservationFavorites(): ResponseFlag {
+        var overallResult: ResponseFlag = ResponseFlag.SuccessFlag
 
         for (favorite in observationLocalDataSource.dirtyFavorites) {
-            result = updateFavorite(favorite).withFlag(result)
+            val updateResult = updateFavorite(favorite)
+            overallResult = ResponseFlag.combineResponseFlags(overallResult, updateResult)
         }
 
-        return result
+        return overallResult
     }
 
-    private suspend fun save(observation: Observation): Result {
+    private suspend fun save(observation: Observation): ResponseFlag {
         return if (observation.remoteId.isNullOrEmpty()) {
+            //observation doesn't exist on the server, so create it
             create(observation)
         } else {
+            //observation should exist on the server, so update it
             update(observation)
         }
     }
 
-    private suspend fun create(observation: Observation): Result {
+    private suspend fun create(observation: Observation): ResponseFlag {
         val response = observationRepository.create(observation)
-        return if (response.isSuccessful) {
-            Result.success()
-        } else {
-            if (response.code() == HttpURLConnection.HTTP_UNAUTHORIZED) Result.failure() else Result.retry()
-        }
+        return ResponseFlag.processResponse(response)
     }
 
-    private suspend fun update(observation: Observation): Result {
+    private suspend fun update(observation: Observation): ResponseFlag {
         val response = observationRepository.update(observation)
-        return if (response.isSuccessful) {
-            Result.success()
-        } else {
-            if (response.code() == HttpURLConnection.HTTP_UNAUTHORIZED) Result.failure() else Result.retry()
-        }
+        return ResponseFlag.processResponse(response)
     }
 
-    private suspend fun archive(observation: Observation): Result {
-        val response = observationRepository.archive(observation)
-        return when {
-            response.isSuccessful -> Result.success()
-            response.code() == HttpURLConnection.HTTP_NOT_FOUND -> Result.success()
-            response.code() == HttpURLConnection.HTTP_UNAUTHORIZED -> Result.failure()
-            else -> Result.retry()
-        }
-    }
-
-    private suspend fun updateImportant(observation: Observation): Result {
+    private suspend fun updateImportant(observation: Observation): ResponseFlag {
         val response = observationRepository.updateImportant(observation)
-
-        return if (response.isSuccessful) {
-            Result.success()
-        } else {
-            if (response.code() == HttpURLConnection.HTTP_UNAUTHORIZED) Result.failure() else Result.retry()
-        }
+        return ResponseFlag.processResponse(response)
     }
 
-    private suspend fun updateFavorite(favorite: ObservationFavorite): Result {
+    private suspend fun updateFavorite(favorite: ObservationFavorite): ResponseFlag {
         val response = observationRepository.updateFavorite(favorite)
-        return if (response.isSuccessful) {
-            Result.success()
-        } else {
-            if (response.code() == HttpURLConnection.HTTP_UNAUTHORIZED) Result.failure() else Result.retry()
-        }
+        return ResponseFlag.processResponse(response)
     }
 
-    private fun Result.withFlag(flag: Int): Int {
-        return when(this) {
-            is Result.Failure -> RESULT_FAILURE_FLAG or flag
-            is Result.Retry -> RESULT_RETRY_FLAG or flag
-            else -> RESULT_SUCCESS_FLAG or flag
-        }
+    private suspend fun archive(observation: Observation): ResponseFlag {
+        val response = observationRepository.archive(observation)
+        return ResponseFlag.processArchiveResponse(response)
     }
 
-    private fun Int.containsFlag(flag: Int): Boolean {
-        return (this or flag) == this
-    }
+    override suspend fun getForegroundInfo(): ForegroundInfo {
+        val notification = NotificationCompat.Builder(applicationContext, MageApplication.MAGE_NOTIFICATION_CHANNEL_ID)
+            .setSmallIcon(R.drawable.ic_sync_preference_24dp)
+            .setContentTitle("Sync Observations")
+            .setContentText("Pushing observation updates to MAGE.")
+            .setPriority(NotificationCompat.PRIORITY_MAX)
+            .setAutoCancel(true)
+            .build()
 
-    private fun Int.withFlag(flag: Int): Int {
-        return this or flag
+        return ForegroundInfo(OBSERVATION_SYNC_NOTIFICATION_ID, notification)
     }
 
     companion object {
@@ -174,10 +146,6 @@ class ObservationSyncWorker @AssistedInject constructor(
         private const val OBSERVATION_SYNC_WORK = "mil.nga.mage.OBSERVATION_SYNC_WORK"
         private const val OBSERVATION_SYNC_NOTIFICATION_ID = 100
 
-        private const val RESULT_SUCCESS_FLAG = 0
-        private const val RESULT_FAILURE_FLAG = 1
-        private const val RESULT_RETRY_FLAG = 2
-
         private val mutex = Mutex()
 
         fun scheduleWork(context: Context) {

mage/src/main/java/mil/nga/giat/mage/observation/sync/ResponseFlag.kt

@@ -0,0 +1,41 @@
+package mil.nga.giat.mage.observation.sync
+
+import retrofit2.Response
+import java.net.HttpURLConnection
+
+sealed class ResponseFlag {
+    data object SuccessFlag : ResponseFlag()
+    data object FailureFlag : ResponseFlag()
+    data object RetryFlag : ResponseFlag()
+
+    companion object {
+        //Combine multiple ResponseFlags into a single result. If any flag is marked as retry, the result will be retry
+        //If any flag is marked as failure and no flags are marked as retry, the result will be failure
+        fun combineResponseFlags(vararg flags: ResponseFlag): ResponseFlag {
+            return when {
+                flags.any { it is RetryFlag } -> RetryFlag
+                flags.any { it is FailureFlag } -> FailureFlag
+                else -> SuccessFlag
+            }
+        }
+
+        //map retrofit API response to ResultFlag
+        fun <T> processResponse(response: Response<T>): ResponseFlag {
+            return when {
+                response.isSuccessful -> SuccessFlag
+                response.code() == HttpURLConnection.HTTP_UNAUTHORIZED -> FailureFlag
+                else -> RetryFlag
+            }
+        }
+
+        //map retrofit API response to ResultFlag for archive API
+        fun <T> processArchiveResponse(response: Response<T>): ResponseFlag {
+            return when {
+                response.isSuccessful -> SuccessFlag
+                response.code() == HttpURLConnection.HTTP_NOT_FOUND -> SuccessFlag //observation no longer exists on the server
+                response.code() == HttpURLConnection.HTTP_UNAUTHORIZED -> FailureFlag
+                else -> RetryFlag
+            }
+        }
+    }
+}