Merge pull request #1107 from mpretty-cyro/fix/group-avatar-download-…

…job-duplication Fixed a few issues related to the GroupAvatarDownloadJob
oxen-io · Feb 16, 2023 · 391418a · 391418a
2 parents c8846bc + 9fd68d2
commit 391418a
Show file tree

Hide file tree

Showing 11 changed files with 83 additions and 16 deletions.
diff --git a/app/src/main/java/org/thoughtcrime/securesms/database/GroupDatabase.java b/app/src/main/java/org/thoughtcrime/securesms/database/GroupDatabase.java
@@ -318,6 +318,25 @@ public void updateProfilePicture(String groupID, byte[] newValue) {
     notifyConversationListListeners();
   }
 
+  @Override
+  public void removeProfilePicture(String groupID) {
+    databaseHelper.getWritableDatabase()
+            .execSQL("UPDATE " + TABLE_NAME +
+                    " SET " + AVATAR + " = NULL, " +
+                    AVATAR_ID + " = NULL, " +
+                    AVATAR_KEY + " = NULL, " +
+                    AVATAR_CONTENT_TYPE + " = NULL, " +
+                    AVATAR_RELAY + " = NULL, " +
+                    AVATAR_DIGEST + " = NULL, " +
+                    AVATAR_URL + " = NULL" +
+                    " WHERE " +
+                    GROUP_ID + " = ?",
+            new String[] {groupID});
+
+    Recipient.applyCached(Address.fromSerialized(groupID), recipient -> recipient.setGroupAvatarId(null));
+    notifyConversationListListeners();
+  }
+
   public boolean hasDownloadedProfilePicture(String groupId) {
     try (Cursor cursor = databaseHelper.getReadableDatabase().query(TABLE_NAME, new String[]{AVATAR}, GROUP_ID + " = ?",
             new String[] {groupId},

diff --git a/app/src/main/java/org/thoughtcrime/securesms/database/SessionJobDatabase.kt b/app/src/main/java/org/thoughtcrime/securesms/database/SessionJobDatabase.kt
@@ -83,11 +83,11 @@ class SessionJobDatabase(context: Context, helper: SQLCipherOpenHelper) : Databa
         }
     }
 
-    fun getGroupAvatarDownloadJob(server: String, room: String): GroupAvatarDownloadJob? {
+    fun getGroupAvatarDownloadJob(server: String, room: String, imageId: String?): GroupAvatarDownloadJob? {
         val database = databaseHelper.readableDatabase
         return database.getAll(sessionJobTable, "$jobType = ?", arrayOf(GroupAvatarDownloadJob.KEY)) {
             jobFromCursor(it) as GroupAvatarDownloadJob?
-        }.filterNotNull().find { it.server == server && it.room == room }
+        }.filterNotNull().find { it.server == server && it.room == room && (imageId == null || it.imageId == imageId) }
     }
 
     fun cancelPendingMessageSendJobs(threadID: Long) {

diff --git a/app/src/main/java/org/thoughtcrime/securesms/database/Storage.kt b/app/src/main/java/org/thoughtcrime/securesms/database/Storage.kt
@@ -226,8 +226,8 @@ class Storage(context: Context, helper: SQLCipherOpenHelper) : Database(context,
         return DatabaseComponent.get(context).sessionJobDatabase().getMessageReceiveJob(messageReceiveJobID)
     }
 
-    override fun getGroupAvatarDownloadJob(server: String, room: String): GroupAvatarDownloadJob? {
-        return DatabaseComponent.get(context).sessionJobDatabase().getGroupAvatarDownloadJob(server, room)
+    override fun getGroupAvatarDownloadJob(server: String, room: String, imageId: String?): GroupAvatarDownloadJob? {
+        return DatabaseComponent.get(context).sessionJobDatabase().getGroupAvatarDownloadJob(server, room, imageId)
     }
 
     override fun resumeMessageSendJobIfNeeded(messageSendJobID: String) {
@@ -324,6 +324,10 @@ class Storage(context: Context, helper: SQLCipherOpenHelper) : Database(context,
         DatabaseComponent.get(context).groupDatabase().updateProfilePicture(groupID, newValue)
     }
 
+    override fun removeProfilePicture(groupID: String) {
+        DatabaseComponent.get(context).groupDatabase().removeProfilePicture(groupID)
+    }
+
     override fun hasDownloadedProfilePicture(groupID: String): Boolean {
         return DatabaseComponent.get(context).groupDatabase().hasDownloadedProfilePicture(groupID)
     }

diff --git a/app/src/main/java/org/thoughtcrime/securesms/database/model/ThreadRecord.java b/app/src/main/java/org/thoughtcrime/securesms/database/model/ThreadRecord.java
@@ -51,6 +51,7 @@ public class ThreadRecord extends DisplayRecord {
   private           final long    expiresIn;
   private           final long    lastSeen;
   private           final boolean pinned;
+  private           final int initialRecipientHash;
 
   public ThreadRecord(@NonNull String body, @Nullable Uri snippetUri,
                       @NonNull Recipient recipient, long date, long count, int unreadCount,
@@ -68,6 +69,7 @@ public ThreadRecord(@NonNull String body, @Nullable Uri snippetUri,
     this.expiresIn          = expiresIn;
     this.lastSeen           = lastSeen;
     this.pinned             = pinned;
+    this.initialRecipientHash = recipient.hashCode();
   }
 
   public @Nullable Uri getSnippetUri() {
@@ -176,4 +178,8 @@ public long getLastSeen() {
   public boolean isPinned() {
     return pinned;
   }
+
+  public int getInitialRecipientHash() {
+    return initialRecipientHash;
+  }
 }
diff --git a/app/src/main/java/org/thoughtcrime/securesms/home/HomeDiffUtil.kt b/app/src/main/java/org/thoughtcrime/securesms/home/HomeDiffUtil.kt
@@ -28,8 +28,11 @@ class HomeDiffUtil(
         if (isSameItem) { isSameItem = (oldItem.unreadCount == newItem.unreadCount) }
         if (isSameItem) { isSameItem = (oldItem.isPinned == newItem.isPinned) }
 
-        // Note: For some reason the 'hashCode' value can change after initialisation so we can't cache it
-        if (isSameItem) { isSameItem = (oldItem.recipient.hashCode() == newItem.recipient.hashCode()) }
+        // The recipient is passed as a reference and changes to recipients update the reference so we
+        // need to cache the hashCode for the recipient and use that for diffing - unfortunately
+        // recipient data is also loaded asyncronously which means every thread will refresh at least
+        // once when the initial recipient data is loaded
+        if (isSameItem) { isSameItem = (oldItem.initialRecipientHash == newItem.initialRecipientHash) }
 
         // Note: Two instances of 'SpannableString' may not equate even though their content matches
         if (isSameItem) { isSameItem = (oldItem.getDisplayBody(context).toString() == newItem.getDisplayBody(context).toString()) }

diff --git a/libsession/src/main/java/org/session/libsession/database/StorageProtocol.kt b/libsession/src/main/java/org/session/libsession/database/StorageProtocol.kt
@@ -49,7 +49,7 @@ interface StorageProtocol {
     fun getAttachmentUploadJob(attachmentID: Long): AttachmentUploadJob?
     fun getMessageSendJob(messageSendJobID: String): MessageSendJob?
     fun getMessageReceiveJob(messageReceiveJobID: String): Job?
-    fun getGroupAvatarDownloadJob(server: String, room: String): Job?
+    fun getGroupAvatarDownloadJob(server: String, room: String, imageId: String?): Job?
     fun resumeMessageSendJobIfNeeded(messageSendJobID: String)
     fun isJobCanceled(job: Job): Boolean
 
@@ -80,6 +80,7 @@ interface StorageProtocol {
     // Open Group Metadata
     fun updateTitle(groupID: String, newValue: String)
     fun updateProfilePicture(groupID: String, newValue: ByteArray)
+    fun removeProfilePicture(groupID: String)
     fun hasDownloadedProfilePicture(groupID: String): Boolean
     fun setUserCount(room: String, server: String, newValue: Int)
 

diff --git a/libsession/src/main/java/org/session/libsession/messaging/jobs/BackgroundGroupAddJob.kt b/libsession/src/main/java/org/session/libsession/messaging/jobs/BackgroundGroupAddJob.kt
@@ -43,8 +43,8 @@ class BackgroundGroupAddJob(val joinUrl: String): Job {
             storage.setOpenGroupPublicKey(openGroup.server, openGroup.serverPublicKey)
             val info = storage.addOpenGroup(openGroup.joinUrl())
             val imageId = info?.imageId
-            if (imageId != null) {
-                JobQueue.shared.add(GroupAvatarDownloadJob(openGroup.room, openGroup.server))
+            if (imageId != null && storage.getGroupAvatarDownloadJob(openGroup.server, openGroup.room, imageId) == null) {
+                JobQueue.shared.add(GroupAvatarDownloadJob(openGroup.server, openGroup.room, imageId))
             }
             Log.d(KEY, "onOpenGroupAdded(${openGroup.server})")
             storage.onOpenGroupAdded(openGroup.server)

diff --git a/libsession/src/main/java/org/session/libsession/messaging/jobs/GroupAvatarDownloadJob.kt b/libsession/src/main/java/org/session/libsession/messaging/jobs/GroupAvatarDownloadJob.kt
@@ -5,18 +5,38 @@ import org.session.libsession.messaging.open_groups.OpenGroupApi
 import org.session.libsession.messaging.utilities.Data
 import org.session.libsession.utilities.GroupUtil
 
-class GroupAvatarDownloadJob(val room: String, val server: String) : Job {
+class GroupAvatarDownloadJob(val server: String, val room: String, val imageId: String?) : Job {
 
     override var delegate: JobDelegate? = null
     override var id: String? = null
     override var failureCount: Int = 0
     override val maxFailureCount: Int = 10
 
     override fun execute(dispatcherName: String) {
+        if (imageId == null) {
+            delegate?.handleJobFailedPermanently(this, dispatcherName, Exception("GroupAvatarDownloadJob now requires imageId"))
+            return
+        }
+
         val storage = MessagingModuleConfiguration.shared.storage
-        val imageId = storage.getOpenGroup(room, server)?.imageId ?: return
+        val storedImageId = storage.getOpenGroup(room, server)?.imageId
+
+        if (storedImageId == null || storedImageId != imageId) {
+            delegate?.handleJobFailedPermanently(this, dispatcherName, Exception("GroupAvatarDownloadJob imageId does not match the OpenGroup"))
+            return
+        }
+
         try {
             val bytes = OpenGroupApi.downloadOpenGroupProfilePicture(server, room, imageId).get()
+
+            // Once the download is complete the imageId might no longer match, so we need to fetch it again just in case
+            val postDownloadStoredImageId = storage.getOpenGroup(room, server)?.imageId
+
+            if (postDownloadStoredImageId == null || postDownloadStoredImageId != imageId) {
+                delegate?.handleJobFailedPermanently(this, dispatcherName, Exception("GroupAvatarDownloadJob imageId no longer matches the OpenGroup"))
+                return
+            }
+
             val groupId = GroupUtil.getEncodedOpenGroupID("$server.$room".toByteArray())
             storage.updateProfilePicture(groupId, bytes)
             storage.updateTimestampUpdated(groupId, System.currentTimeMillis())
@@ -30,6 +50,7 @@ class GroupAvatarDownloadJob(val room: String, val server: String) : Job {
         return Data.Builder()
             .putString(ROOM, room)
             .putString(SERVER, server)
+            .putString(IMAGE_ID, imageId)
             .build()
     }
 
@@ -40,14 +61,16 @@ class GroupAvatarDownloadJob(val room: String, val server: String) : Job {
 
         private const val ROOM = "room"
         private const val SERVER = "server"
+        private const val IMAGE_ID = "imageId"
     }
 
     class Factory : Job.Factory<GroupAvatarDownloadJob> {
 
         override fun create(data: Data): GroupAvatarDownloadJob {
             return GroupAvatarDownloadJob(
+                data.getString(SERVER),
                 data.getString(ROOM),
-                data.getString(SERVER)
+                if (data.hasString(IMAGE_ID)) { data.getString(IMAGE_ID) } else { null }
             )
         }
     }

diff --git a/libsession/src/main/java/org/session/libsession/messaging/open_groups/OpenGroup.kt b/libsession/src/main/java/org/session/libsession/messaging/open_groups/OpenGroup.kt
@@ -36,7 +36,7 @@ data class OpenGroup(
                 val server = json.get("server").asText().lowercase(Locale.US)
                 val displayName = json.get("displayName").asText()
                 val publicKey = json.get("publicKey").asText()
-                val imageId = json.get("imageId")?.asText()
+                val imageId = if (json.hasNonNull("imageId")) { json.get("imageId")?.asText() } else { null }
                 val canWrite = json.get("canWrite")?.asText()?.toBoolean() ?: true
                 val infoUpdates = json.get("infoUpdates")?.asText()?.toIntOrNull() ?: 0
                 OpenGroup(server = server, room = room, name = displayName, publicKey = publicKey, imageId = imageId, canWrite = canWrite, infoUpdates = infoUpdates)

diff --git a/...c/main/java/org/session/libsession/messaging/sending_receiving/pollers/OpenGroupPoller.kt b/...c/main/java/org/session/libsession/messaging/sending_receiving/pollers/OpenGroupPoller.kt
@@ -159,20 +159,30 @@ class OpenGroupPoller(private val server: String, private val executorService: S
             })
         }
 
+        // Update the group avatar
         if (
             (
                 pollInfo.details != null &&
                 pollInfo.details.imageId != null && (
                     pollInfo.details.imageId != existingOpenGroup.imageId ||
                     !storage.hasDownloadedProfilePicture(dbGroupId)
-                )
+                ) &&
+                storage.getGroupAvatarDownloadJob(openGroup.server, openGroup.room, pollInfo.details.imageId) == null
             ) || (
                 pollInfo.details == null &&
                 existingOpenGroup.imageId != null &&
-                !storage.hasDownloadedProfilePicture(dbGroupId)
+                !storage.hasDownloadedProfilePicture(dbGroupId) &&
+                storage.getGroupAvatarDownloadJob(openGroup.server, openGroup.room, existingOpenGroup.imageId) == null
             )
         ) {
-            JobQueue.shared.add(GroupAvatarDownloadJob(roomToken, server))
+            JobQueue.shared.add(GroupAvatarDownloadJob(server, roomToken, existingOpenGroup.imageId))
+        }
+        else if (
+            pollInfo.details != null &&
+            pollInfo.details.imageId == null &&
+            existingOpenGroup.imageId != null
+        ) {
+            storage.removeProfilePicture(dbGroupId)
         }
     }
 

diff --git a/libsignal/src/main/java/org/session/libsignal/database/LokiOpenGroupDatabaseProtocol.kt b/libsignal/src/main/java/org/session/libsignal/database/LokiOpenGroupDatabaseProtocol.kt
@@ -4,4 +4,5 @@ interface LokiOpenGroupDatabaseProtocol {
 
     fun updateTitle(groupID: String, newValue: String)
     fun updateProfilePicture(groupID: String, newValue: ByteArray)
+    fun removeProfilePicture(groupID: String)
 }