From b74da07f7ed17226c30bf12adf53cb8a37e98ccd Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Fri, 22 Sep 2017 15:15:04 -0400 Subject: [PATCH] Respond to CR. // FREEBIE --- .../Devices/OWSReadReceiptsForSenderMessage.m | 4 +- .../src/Devices/OWSRecordTranscriptJob.m | 7 +- .../src/Messages/Interactions/TSInteraction.h | 2 - .../src/Messages/Interactions/TSInteraction.m | 30 +---- .../Messages/Interactions/TSOutgoingMessage.h | 2 +- .../Messages/Interactions/TSOutgoingMessage.m | 2 +- .../src/Messages/OWSMessageManager.m | 22 +++- .../src/Messages/OWSReadReceiptManager.h | 4 +- .../src/Messages/OWSReadReceiptManager.m | 121 ++++++++++++++---- 9 files changed, 121 insertions(+), 73 deletions(-) diff --git a/SignalServiceKit/src/Devices/OWSReadReceiptsForSenderMessage.m b/SignalServiceKit/src/Devices/OWSReadReceiptsForSenderMessage.m index 85b1cc24b..6eab6e485 100644 --- a/SignalServiceKit/src/Devices/OWSReadReceiptsForSenderMessage.m +++ b/SignalServiceKit/src/Devices/OWSReadReceiptsForSenderMessage.m @@ -3,7 +3,7 @@ // #import "OWSReadReceiptsForSenderMessage.h" -#import "NSDate+millisecondTimeStamp.h" +#import "NSDate+OWS.h" #import "OWSReadReceipt.h" #import "OWSSignalServiceProtos.pb.h" #import "SignalRecipient.h" @@ -56,8 +56,6 @@ NS_ASSUME_NONNULL_BEGIN [builder addTimestamp:[messageTimestamp unsignedLongLongValue]]; } - // TODO: addLocalProfileKeyIfNecessary. - return [builder build]; } diff --git a/SignalServiceKit/src/Devices/OWSRecordTranscriptJob.m b/SignalServiceKit/src/Devices/OWSRecordTranscriptJob.m index bdd5b0981..cd754520b 100644 --- a/SignalServiceKit/src/Devices/OWSRecordTranscriptJob.m +++ b/SignalServiceKit/src/Devices/OWSRecordTranscriptJob.m @@ -104,17 +104,16 @@ NS_ASSUME_NONNULL_BEGIN } if (outgoingMessage.body.length < 1 && outgoingMessage.attachmentIds.count < 1) { - // TODO: Is this safe? - DDLogInfo(@"Ignoring message transcript for empty message."); + OWSFail(@"Ignoring message transcript for empty message."); return; } - // TODO: Refactor this logic. + // TODO: Refactor this logic. Most of it doesn't belong in `OWSMessageSender`. [self.messageSender handleMessageSentRemotely:outgoingMessage sentAt:transcript.expirationStartedAt transaction:transaction]; - [self.readReceiptManager outgoingMessageFromLinkedDevice:outgoingMessage transaction:transaction]; + [self.readReceiptManager updateOutgoingMessageFromLinkedDevice:outgoingMessage transaction:transaction]; [attachmentsProcessor fetchAttachmentsForMessage:outgoingMessage diff --git a/SignalServiceKit/src/Messages/Interactions/TSInteraction.h b/SignalServiceKit/src/Messages/Interactions/TSInteraction.h index bcc4e67d7..c9bb9ebe7 100644 --- a/SignalServiceKit/src/Messages/Interactions/TSInteraction.h +++ b/SignalServiceKit/src/Messages/Interactions/TSInteraction.h @@ -26,8 +26,6 @@ NS_ASSUME_NONNULL_BEGIN #pragma mark Utility Method -+ (instancetype)interactionForTimestamp:(uint64_t)timestamp - withTransaction:(YapDatabaseReadWriteTransaction *)transaction; + (NSArray *)interactionsWithTimestamp:(uint64_t)timestamp ofClass:(Class)clazz withTransaction:(YapDatabaseReadWriteTransaction *)transaction; diff --git a/SignalServiceKit/src/Messages/Interactions/TSInteraction.m b/SignalServiceKit/src/Messages/Interactions/TSInteraction.m index 30ebfb849..07d7a10d3 100644 --- a/SignalServiceKit/src/Messages/Interactions/TSInteraction.m +++ b/SignalServiceKit/src/Messages/Interactions/TSInteraction.m @@ -12,28 +12,6 @@ NS_ASSUME_NONNULL_BEGIN @implementation TSInteraction -+ (instancetype)interactionForTimestamp:(uint64_t)timestamp - withTransaction:(YapDatabaseReadWriteTransaction *)transaction { - OWSAssert(timestamp > 0); - - // Accept any interaction. - NSArray *interactions = [self interactionsWithTimestamp:timestamp - withFilter:^(TSInteraction *interaction) { - return YES; - } - withTransaction:transaction]; - - if (interactions.count < 1) { - // TODO: OWSFail()? - return nil; - } - if (interactions.count > 1) { - DDLogWarn(@"The database contains %zd colliding timestamps at: %lld.", interactions.count, timestamp); - } - TSInteraction *lastInteraction = interactions.lastObject; - return lastInteraction; -} - + (NSArray *)interactionsWithTimestamp:(uint64_t)timestamp ofClass:(Class)clazz withTransaction:(YapDatabaseReadWriteTransaction *)transaction @@ -42,14 +20,14 @@ NS_ASSUME_NONNULL_BEGIN // Accept any interaction. return [self interactionsWithTimestamp:timestamp - withFilter:^(TSInteraction *interaction) { - return [interaction isKindOfClass:clazz]; - } + filter:^(TSInteraction *interaction) { + return [interaction isKindOfClass:clazz]; + } withTransaction:transaction]; } + (NSArray *)interactionsWithTimestamp:(uint64_t)timestamp - withFilter:(BOOL (^_Nonnull)(TSInteraction *))filter + filter:(BOOL (^_Nonnull)(TSInteraction *))filter withTransaction:(YapDatabaseReadWriteTransaction *)transaction { OWSAssert(timestamp > 0); diff --git a/SignalServiceKit/src/Messages/Interactions/TSOutgoingMessage.h b/SignalServiceKit/src/Messages/Interactions/TSOutgoingMessage.h index be3a48997..3c62ee422 100644 --- a/SignalServiceKit/src/Messages/Interactions/TSOutgoingMessage.h +++ b/SignalServiceKit/src/Messages/Interactions/TSOutgoingMessage.h @@ -176,7 +176,7 @@ typedef NS_ENUM(NSInteger, TSGroupMetaMessage) { - (void)updateWithWasSentFromLinkedDeviceWithTransaction:(YapDatabaseReadWriteTransaction *)transaction; - (void)updateWithSingleGroupRecipient:(NSString *)singleGroupRecipient transaction:(YapDatabaseReadWriteTransaction *)transaction; -- (void)updateWithReadRecipient:(NSString *)recipientId transaction:(YapDatabaseReadWriteTransaction *)transaction; +- (void)updateWithReadRecipientId:(NSString *)recipientId transaction:(YapDatabaseReadWriteTransaction *)transaction; #pragma mark - Sent Recipients diff --git a/SignalServiceKit/src/Messages/Interactions/TSOutgoingMessage.m b/SignalServiceKit/src/Messages/Interactions/TSOutgoingMessage.m index 3af2f649f..b9a97b858 100644 --- a/SignalServiceKit/src/Messages/Interactions/TSOutgoingMessage.m +++ b/SignalServiceKit/src/Messages/Interactions/TSOutgoingMessage.m @@ -411,7 +411,7 @@ NSString *const kTSOutgoingMessageSentRecipientAll = @"kTSOutgoingMessageSentRec }]; } -- (void)updateWithReadRecipient:(NSString *)recipientId transaction:(YapDatabaseReadWriteTransaction *)transaction +- (void)updateWithReadRecipientId:(NSString *)recipientId transaction:(YapDatabaseReadWriteTransaction *)transaction { OWSAssert(recipientId.length > 0); OWSAssert(transaction); diff --git a/SignalServiceKit/src/Messages/OWSMessageManager.m b/SignalServiceKit/src/Messages/OWSMessageManager.m index fc7738489..ac372c17e 100644 --- a/SignalServiceKit/src/Messages/OWSMessageManager.m +++ b/SignalServiceKit/src/Messages/OWSMessageManager.m @@ -187,14 +187,24 @@ NS_ASSUME_NONNULL_BEGIN OWSAssert(envelope); OWSAssert(transaction); - TSInteraction *interaction = [TSInteraction interactionForTimestamp:envelope.timestamp withTransaction:transaction]; - if ([interaction isKindOfClass:[TSOutgoingMessage class]]) { - TSOutgoingMessage *outgoingMessage = (TSOutgoingMessage *)interaction; - [outgoingMessage updateWithWasDeliveredWithTransaction:transaction]; - } else { + NSArray *messages + = (NSArray *)[TSInteraction interactionsWithTimestamp:envelope.timestamp + ofClass:[TSOutgoingMessage class] + withTransaction:transaction]; + if (messages.count < 1) { // Desktop currently sends delivery receipts for "unpersisted" messages // like group updates, so these errors are expected to a certain extent. - DDLogInfo(@"%@ Unexpected message with timestamp: %llu", self.tag, envelope.timestamp); + DDLogInfo(@"%@ Missing message for delivery receipt: %llu", self.tag, envelope.timestamp); + } else { + if (messages.count > 1) { + DDLogInfo(@"%@ More than one message (%zd) for delivery receipt: %llu", + self.tag, + messages.count, + envelope.timestamp); + } + for (TSOutgoingMessage *outgoingMessage in messages) { + [outgoingMessage updateWithWasDeliveredWithTransaction:transaction]; + } } } diff --git a/SignalServiceKit/src/Messages/OWSReadReceiptManager.h b/SignalServiceKit/src/Messages/OWSReadReceiptManager.h index 14fe04909..60dda0bf9 100644 --- a/SignalServiceKit/src/Messages/OWSReadReceiptManager.h +++ b/SignalServiceKit/src/Messages/OWSReadReceiptManager.h @@ -39,8 +39,8 @@ NS_ASSUME_NONNULL_BEGIN - (void)processReadReceiptsFromRecipient:(OWSSignalServiceProtosReceiptMessage *)receiptMessage envelope:(OWSSignalServiceProtosEnvelope *)envelope; -- (void)outgoingMessageFromLinkedDevice:(TSOutgoingMessage *)message - transaction:(YapDatabaseReadWriteTransaction *)transaction; +- (void)updateOutgoingMessageFromLinkedDevice:(TSOutgoingMessage *)message + transaction:(YapDatabaseReadWriteTransaction *)transaction; // This method cues this manager: // diff --git a/SignalServiceKit/src/Messages/OWSReadReceiptManager.m b/SignalServiceKit/src/Messages/OWSReadReceiptManager.m index 2a1b0a3ef..6bb828899 100644 --- a/SignalServiceKit/src/Messages/OWSReadReceiptManager.m +++ b/SignalServiceKit/src/Messages/OWSReadReceiptManager.m @@ -17,9 +17,81 @@ NS_ASSUME_NONNULL_BEGIN +@interface TSRecipientReadReceipt : TSYapDatabaseObject + +@property (nonatomic, readonly) uint64_t timestamp; +@property (nonatomic, readonly) NSSet *recipientIds; + +@end + +#pragma mark - + +@implementation TSRecipientReadReceipt + +- (instancetype)initWithTimestamp:(uint64_t)timestamp +{ + OWSAssert(timestamp > 0); + + self = [super initWithUniqueId:[TSRecipientReadReceipt uniqueIdForTimestamp:timestamp]]; + + if (self) { + _timestamp = timestamp; + _recipientIds = [NSSet set]; + } + + return self; +} + ++ (NSString *)uniqueIdForTimestamp:(uint64_t)timestamp +{ + return [NSString stringWithFormat:@"%llu", timestamp]; +} + +- (void)addRecipientId:(NSString *)recipientId +{ + NSMutableSet *recipientIdsCopy = [self.recipientIds mutableCopy]; + [recipientIdsCopy addObject:recipientId]; + _recipientIds = [recipientIdsCopy copy]; +} + ++ (void)addRecipientId:(NSString *)recipientId + timestamp:(uint64_t)timestamp + transaction:(YapDatabaseReadWriteTransaction *)transaction +{ + OWSAssert(transaction); + + TSRecipientReadReceipt *_Nullable recipientReadReceipt = + [transaction objectForKey:[self uniqueIdForTimestamp:timestamp] inCollection:[self collection]]; + if (!recipientReadReceipt) { + recipientReadReceipt = [[TSRecipientReadReceipt alloc] initWithTimestamp:timestamp]; + } + [recipientReadReceipt addRecipientId:recipientId]; + [recipientReadReceipt saveWithTransaction:transaction]; +} + ++ (nullable NSSet *)recipientIdsForTimestamp:(uint64_t)timestamp + transaction:(YapDatabaseReadWriteTransaction *)transaction +{ + OWSAssert(transaction); + + TSRecipientReadReceipt *_Nullable recipientReadReceipt = + [transaction objectForKey:[self uniqueIdForTimestamp:timestamp] inCollection:[self collection]]; + return recipientReadReceipt.recipientIds; +} + ++ (void)removeRecipientIdsForTimestamp:(uint64_t)timestamp transaction:(YapDatabaseReadWriteTransaction *)transaction +{ + OWSAssert(transaction); + + [transaction removeObjectForKey:[self uniqueIdForTimestamp:timestamp] inCollection:[self collection]]; +} + +@end + +#pragma mark - + NSString *const OWSReadReceiptManagerCollection = @"OWSReadReceiptManagerCollection"; NSString *const OWSReadReceiptManagerAreReadReceiptsEnabled = @"areReadReceiptsEnabled"; -NSString *const OWSRecipientReadReceiptCollection = @"OWSRecipientReadReceiptCollection"; @interface OWSReadReceiptManager () @@ -37,7 +109,7 @@ NSString *const OWSRecipientReadReceiptCollection = @"OWSRecipientReadReceiptCol // we will send to senders. // // Should only be accessed while synchronized on the OWSReadReceiptManager. -@property (nonatomic, readonly) NSMutableDictionary *> *toSenderReadReceiptMap; +@property (nonatomic, readonly) NSMutableDictionary *> *toSenderReadReceiptMap; // Should only be accessed while synchronized on the OWSReadReceiptManager. @property (nonatomic) BOOL isProcessing; @@ -168,16 +240,17 @@ NSString *const OWSRecipientReadReceiptCollection = @"OWSRecipientReadReceiptCol }); } - NSArray *readReceiptsToSend = [[self.toLinkedDevicesReadReceiptMap allValues] copy]; + NSArray *readReceiptsToSend = [self.toLinkedDevicesReadReceiptMap allValues]; [self.toLinkedDevicesReadReceiptMap removeAllObjects]; if (self.toSenderReadReceiptMap.count > 0) { for (NSString *recipientId in self.toSenderReadReceiptMap) { - NSArray *timestamps = self.toSenderReadReceiptMap[recipientId]; + NSSet *timestamps = self.toSenderReadReceiptMap[recipientId]; OWSAssert(timestamps.count > 0); TSThread *thread = [TSContactThread getOrCreateThreadWithContactId:recipientId]; OWSReadReceiptsForSenderMessage *message = - [[OWSReadReceiptsForSenderMessage alloc] initWithThread:thread messageTimestamps:timestamps]; + [[OWSReadReceiptsForSenderMessage alloc] initWithThread:thread + messageTimestamps:timestamps.allObjects]; dispatch_async(dispatch_get_main_queue(), ^{ [self.messageSender sendMessage:message @@ -274,9 +347,9 @@ NSString *const OWSRecipientReadReceiptCollection = @"OWSRecipientReadReceiptCol if ([self areReadReceiptsEnabled]) { DDLogVerbose(@"%@ Enqueuing read receipt for sender.", self.tag); - NSMutableArray *_Nullable timestamps = self.toSenderReadReceiptMap[messageAuthorId]; + NSMutableSet *_Nullable timestamps = self.toSenderReadReceiptMap[messageAuthorId]; if (!timestamps) { - timestamps = [NSMutableArray new]; + timestamps = [NSMutableSet new]; self.toSenderReadReceiptMap[messageAuthorId] = timestamps; } [timestamps addObject:@(message.timestamp)]; @@ -320,42 +393,34 @@ NSString *const OWSRecipientReadReceiptCollection = @"OWSRecipientReadReceiptCol // TODO: We might also need to "mark as read by recipient" any older messages // from us in that thread. Or maybe this state should hang on the thread? for (TSOutgoingMessage *message in messages) { - [message updateWithReadRecipient:recipientId transaction:transaction]; + [message updateWithReadRecipientId:recipientId transaction:transaction]; } } else { // Persist the read receipts so that we can apply them to outgoing messages // that we learn about later through sync messages. - NSString *storageKey = [NSString stringWithFormat:@"%llu", timestamp]; - NSSet *recipientIds = - [transaction objectForKey:storageKey inCollection:OWSRecipientReadReceiptCollection]; - NSMutableSet *recipientIdsCopy - = (recipientIds ? [recipientIds mutableCopy] : [NSMutableSet new]); - [recipientIdsCopy addObject:recipientId]; - [transaction setObject:recipientIdsCopy - forKey:storageKey - inCollection:OWSRecipientReadReceiptCollection]; + [TSRecipientReadReceipt addRecipientId:recipientId timestamp:timestamp transaction:transaction]; } } }]; }); } -- (void)outgoingMessageFromLinkedDevice:(TSOutgoingMessage *)message - transaction:(YapDatabaseReadWriteTransaction *)transaction +- (void)updateOutgoingMessageFromLinkedDevice:(TSOutgoingMessage *)message + transaction:(YapDatabaseReadWriteTransaction *)transaction { OWSAssert(message); OWSAssert(transaction); - NSString *storageKey = [NSString stringWithFormat:@"%llu", message.timestamp]; - NSSet *recipientIds = - [transaction objectForKey:storageKey inCollection:OWSRecipientReadReceiptCollection]; - if (recipientIds) { - OWSAssert(recipientIds.count > 0); - for (NSString *recipientId in recipientIds) { - [message updateWithReadRecipient:recipientId transaction:transaction]; - } + NSSet *_Nullable recipientIds = + [TSRecipientReadReceipt recipientIdsForTimestamp:message.timestamp transaction:transaction]; + if (!recipientIds) { + return; + } + OWSAssert(recipientIds.count > 0); + for (NSString *recipientId in recipientIds) { + [message updateWithReadRecipientId:recipientId transaction:transaction]; } - [transaction removeObjectForKey:storageKey inCollection:OWSRecipientReadReceiptCollection]; + [TSRecipientReadReceipt removeRecipientIdsForTimestamp:message.timestamp transaction:transaction]; } #pragma mark - Settings