From a69c247b21469d4f0f6b6cc18e055ab8054ec2ba Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Mon, 22 Jul 2019 17:22:06 -0300 Subject: [PATCH 1/3] Ensure thread finder only deals with visible threads. --- SignalServiceKit/src/Contacts/TSThread.m | 9 +++++++++ SignalServiceKit/src/SignalServiceKit.h | 1 + .../src/Storage/Database/Records/ThreadFinder.swift | 4 ++-- 3 files changed, 12 insertions(+), 2 deletions(-) diff --git a/SignalServiceKit/src/Contacts/TSThread.m b/SignalServiceKit/src/Contacts/TSThread.m index 1e8f869caf..fe657b1475 100644 --- a/SignalServiceKit/src/Contacts/TSThread.m +++ b/SignalServiceKit/src/Contacts/TSThread.m @@ -391,7 +391,16 @@ isArchivedByLegacyTimestampForSorting:(BOOL)isArchivedByLegacyTimestampForSortin { OWSAssertDebug(interaction); + if (!interaction.shouldBeSaved) { + OWSFailDebug(@"Unexpected interaction type: %@", interaction.class); + return NO; + } if (interaction.isDynamicInteraction) { + OWSFailDebug(@"Unexpected interaction type: %@", interaction.class); + return NO; + } + if ([interaction isKindOfClass:[OWSOutgoingSyncMessage class]]) { + OWSFailDebug(@"Unexpected interaction type: %@", interaction.class); return NO; } diff --git a/SignalServiceKit/src/SignalServiceKit.h b/SignalServiceKit/src/SignalServiceKit.h index 1abfb09b56..87d1b7a9f1 100644 --- a/SignalServiceKit/src/SignalServiceKit.h +++ b/SignalServiceKit/src/SignalServiceKit.h @@ -21,6 +21,7 @@ #import #import #import +#import #import #import #import diff --git a/SignalServiceKit/src/Storage/Database/Records/ThreadFinder.swift b/SignalServiceKit/src/Storage/Database/Records/ThreadFinder.swift index 4d2a635428..92b64a4b3e 100644 --- a/SignalServiceKit/src/Storage/Database/Records/ThreadFinder.swift +++ b/SignalServiceKit/src/Storage/Database/Records/ThreadFinder.swift @@ -92,7 +92,7 @@ struct GRDBThreadFinder: ThreadFinder { ELSE 0 END isArchived FROM \(ThreadRecord.databaseTableName) - LEFT JOIN ( + INNER JOIN ( SELECT MAX(\(interactionColumn: .id)) as maxInteractionId, \(interactionColumn: .threadUniqueId) @@ -122,7 +122,7 @@ struct GRDBThreadFinder: ThreadFinder { ELSE 0 END isArchived FROM \(ThreadRecord.databaseTableName) - LEFT JOIN ( + INNER JOIN ( SELECT MAX(\(interactionColumn: .id)) as maxInteractionId, \(interactionColumn: .threadUniqueId) From 8df19c2ed613ebecd20c0d1cda7b929562ecfb36 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Mon, 22 Jul 2019 17:35:27 -0300 Subject: [PATCH 2/3] Ensure thread finder only deals with visible threads. --- .../src/Storage/Database/Records/ThreadFinder.swift | 7 +++++-- SignalServiceKit/src/Storage/TSDatabaseView.m | 2 ++ 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/SignalServiceKit/src/Storage/Database/Records/ThreadFinder.swift b/SignalServiceKit/src/Storage/Database/Records/ThreadFinder.swift index 92b64a4b3e..039232ba89 100644 --- a/SignalServiceKit/src/Storage/Database/Records/ThreadFinder.swift +++ b/SignalServiceKit/src/Storage/Database/Records/ThreadFinder.swift @@ -87,12 +87,13 @@ struct GRDBThreadFinder: ThreadFinder { SELECT COUNT(*) FROM ( SELECT + \( threadColumn: .shouldThreadBeVisible), CASE maxInteractionId <= \(threadColumn: .archivedAsOfMessageSortId) WHEN 1 THEN 1 ELSE 0 END isArchived FROM \(ThreadRecord.databaseTableName) - INNER JOIN ( + LEFT JOIN ( SELECT MAX(\(interactionColumn: .id)) as maxInteractionId, \(interactionColumn: .threadUniqueId) @@ -102,6 +103,7 @@ struct GRDBThreadFinder: ThreadFinder { ON latestInteractions.\(interactionColumn: .threadUniqueId) = \(threadColumn: .uniqueId) ) WHERE isArchived = ? + AND \(threadColumn: .shouldThreadBeVisible) = 1 """, arguments: [isArchived]) else { owsFailDebug("count was unexpectedly nil") @@ -122,7 +124,7 @@ struct GRDBThreadFinder: ThreadFinder { ELSE 0 END isArchived FROM \(ThreadRecord.databaseTableName) - INNER JOIN ( + LEFT JOIN ( SELECT MAX(\(interactionColumn: .id)) as maxInteractionId, \(interactionColumn: .threadUniqueId) @@ -133,6 +135,7 @@ struct GRDBThreadFinder: ThreadFinder { ORDER BY maxInteractionId DESC ) WHERE isArchived = ? + AND \( threadColumn: .shouldThreadBeVisible) = 1 """, arguments: [isArchived]).forEach { threadRecord in block(try TSThread.fromRecord(threadRecord)) diff --git a/SignalServiceKit/src/Storage/TSDatabaseView.m b/SignalServiceKit/src/Storage/TSDatabaseView.m index c914c21211..c07f6459c9 100644 --- a/SignalServiceKit/src/Storage/TSDatabaseView.m +++ b/SignalServiceKit/src/Storage/TSDatabaseView.m @@ -319,10 +319,12 @@ NSString *const TSLazyRestoreAttachmentsGroup = @"TSLazyRestoreAttachmentsGroup" OWSAssertDebug(viewTransaction); NSUInteger threadMessageCount = [viewTransaction numberOfItemsInGroup:thread.uniqueId]; if (threadMessageCount < 1) { + OWSAssertDebug(!thread.shouldThreadBeVisible); return nil; } } + OWSAssertDebug(thread.shouldThreadBeVisible); return [thread isArchivedWithTransaction:transaction.asAnyRead] ? TSArchiveGroup : TSInboxGroup; }]; From a8776bc35f511b61f7eb26cdde7f96b08d56a3c9 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Wed, 24 Jul 2019 12:38:46 -0300 Subject: [PATCH 3/3] Respond to CR. --- Signal/src/Models/ThreadMapping.swift | 6 +-- .../Database/Records/ThreadFinder.swift | 38 +++++++++---------- 2 files changed, 22 insertions(+), 22 deletions(-) diff --git a/Signal/src/Models/ThreadMapping.swift b/Signal/src/Models/ThreadMapping.swift index ff0bae1371..524d3b5d9b 100644 --- a/Signal/src/Models/ThreadMapping.swift +++ b/Signal/src/Models/ThreadMapping.swift @@ -136,10 +136,10 @@ class ThreadMapping: NSObject { } func update(isViewingArchive: Bool, transaction: SDSAnyReadTransaction) throws { - archiveCount = try threadFinder.threadCount(isArchived: true, transaction: transaction) - inboxCount = try threadFinder.threadCount(isArchived: false, transaction: transaction) + archiveCount = try threadFinder.visibleThreadCount(isArchived: true, transaction: transaction) + inboxCount = try threadFinder.visibleThreadCount(isArchived: false, transaction: transaction) var newThreads: [TSThread] = [] - try threadFinder.enumerateThreads(isArchived: isViewingArchive, transaction: transaction) { thread in + try threadFinder.enumerateVisibleThreads(isArchived: isViewingArchive, transaction: transaction) { thread in newThreads.append(thread) } threads = newThreads diff --git a/SignalServiceKit/src/Storage/Database/Records/ThreadFinder.swift b/SignalServiceKit/src/Storage/Database/Records/ThreadFinder.swift index 039232ba89..ba137d4766 100644 --- a/SignalServiceKit/src/Storage/Database/Records/ThreadFinder.swift +++ b/SignalServiceKit/src/Storage/Database/Records/ThreadFinder.swift @@ -8,8 +8,8 @@ import GRDBCipher public protocol ThreadFinder { associatedtype ReadTransaction - func threadCount(isArchived: Bool, transaction: ReadTransaction) throws -> UInt - func enumerateThreads(isArchived: Bool, transaction: ReadTransaction, block: @escaping (TSThread) -> Void) throws + func visibleThreadCount(isArchived: Bool, transaction: ReadTransaction) throws -> UInt + func enumerateVisibleThreads(isArchived: Bool, transaction: ReadTransaction, block: @escaping (TSThread) -> Void) throws } public class AnyThreadFinder: ThreadFinder { @@ -20,21 +20,21 @@ public class AnyThreadFinder: ThreadFinder { public init() { } - public func threadCount(isArchived: Bool, transaction: SDSAnyReadTransaction) throws -> UInt { + public func visibleThreadCount(isArchived: Bool, transaction: SDSAnyReadTransaction) throws -> UInt { switch transaction.readTransaction { case .grdbRead(let grdb): - return try grdbAdapter.threadCount(isArchived: isArchived, transaction: grdb.database) + return try grdbAdapter.visibleThreadCount(isArchived: isArchived, transaction: grdb.database) case .yapRead(let yap): - return yapAdapter.threadCount(isArchived: isArchived, transaction: yap) + return yapAdapter.visibleThreadCount(isArchived: isArchived, transaction: yap) } } - public func enumerateThreads(isArchived: Bool, transaction: SDSAnyReadTransaction, block: @escaping (TSThread) -> Void) throws { + public func enumerateVisibleThreads(isArchived: Bool, transaction: SDSAnyReadTransaction, block: @escaping (TSThread) -> Void) throws { switch transaction.readTransaction { case .grdbRead(let grdb): - try grdbAdapter.enumerateThreads(isArchived: isArchived, transaction: grdb.database, block: block) + try grdbAdapter.enumerateVisibleThreads(isArchived: isArchived, transaction: grdb.database, block: block) case .yapRead(let yap): - yapAdapter.enumerateThreads(isArchived: isArchived, transaction: yap, block: block) + yapAdapter.enumerateVisibleThreads(isArchived: isArchived, transaction: yap, block: block) } } } @@ -42,14 +42,14 @@ public class AnyThreadFinder: ThreadFinder { struct YAPDBThreadFinder: ThreadFinder { typealias ReadTransaction = YapDatabaseReadTransaction - func threadCount(isArchived: Bool, transaction: YapDatabaseReadTransaction) -> UInt { + func visibleThreadCount(isArchived: Bool, transaction: YapDatabaseReadTransaction) -> UInt { guard let view = ext(transaction) else { return 0 } return view.numberOfItems(inGroup: group(isArchived: isArchived)) } - func enumerateThreads(isArchived: Bool, transaction: YapDatabaseReadTransaction, block: @escaping (TSThread) -> Void) { + func enumerateVisibleThreads(isArchived: Bool, transaction: YapDatabaseReadTransaction, block: @escaping (TSThread) -> Void) { guard let view = ext(transaction) else { return } @@ -82,7 +82,7 @@ struct GRDBThreadFinder: ThreadFinder { static let cn = ThreadRecord.columnName - func threadCount(isArchived: Bool, transaction: Database) throws -> UInt { + func visibleThreadCount(isArchived: Bool, transaction: Database) throws -> UInt { guard let count = try UInt.fetchOne(transaction, sql: """ SELECT COUNT(*) FROM ( @@ -104,16 +104,16 @@ struct GRDBThreadFinder: ThreadFinder { ) WHERE isArchived = ? AND \(threadColumn: .shouldThreadBeVisible) = 1 - """, - arguments: [isArchived]) else { - owsFailDebug("count was unexpectedly nil") - return 0 + """, + arguments: [isArchived]) else { + owsFailDebug("count was unexpectedly nil") + return 0 } return count } - func enumerateThreads(isArchived: Bool, transaction: Database, block: @escaping (TSThread) -> Void) throws { + func enumerateVisibleThreads(isArchived: Bool, transaction: Database, block: @escaping (TSThread) -> Void) throws { try ThreadRecord.fetchCursor(transaction, sql: """ SELECT * FROM ( @@ -136,9 +136,9 @@ struct GRDBThreadFinder: ThreadFinder { ) WHERE isArchived = ? AND \( threadColumn: .shouldThreadBeVisible) = 1 - """, - arguments: [isArchived]).forEach { threadRecord in - block(try TSThread.fromRecord(threadRecord)) + """, + arguments: [isArchived]).forEach { threadRecord in + block(try TSThread.fromRecord(threadRecord)) } } }