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

[CIS-692] Implement Channel Truncating #864

Merged
merged 16 commits into from
Mar 5, 2021
Merged

Conversation

VojtaStavik
Copy link
Contributor

When the channel is truncated, it means all messages sent before the truncation disappear. When you query the channel, it has no messages and even the last_message_at value is reset.

Possible ways to implement this locally

a) Delete all messages from the DB -> hard deleting entities from CoreData is not trivial because of our complex data model scheme. This would be the most complicated way to implement this feature. 🔴

b) Soft-delete messages by message.type = .deleted-> this is less complex but not ideal. Deleted messages have different semantics from the truncated messages, especially when it's the messages sent by the current user. Almost there... 🔴

c) Do it the same way the backed does it -> I added truncatedAt field to ChannelDTO and adjusted the message predicates to ignore the message created before this date. This solution is the most simple one and works nicely ✅

Issues

  • We will have to introduce some kind of DB cleanup worker that will take care of "physically" removing the truncated messages from the DB. This worker can run only when there are no observers attached to the DB in order to prevent invalid updates. However, the clean-up worker is needed anyway otherwise the DB size would just grow and grow with more incoming messages and new channels.

  • Our current custom layout crashes when all messages are removed. I found the issue but the fix was not obvious. I didn't want to spend more time on it since @olejnjak is working on fixing the layout anyway.

@VojtaStavik VojtaStavik added the 🌐 SDK: StreamChat (LLC) Tasks related to the StreamChat LLC SDK label Mar 4, 2021
@VojtaStavik VojtaStavik requested a review from b-onc March 4, 2021 17:05
Copy link
Contributor

@b-onc b-onc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM overall 🎉

Comment on lines 507 to 509
controller.messages.forEach {
print($0.createdAt)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forgot from debugging? :D

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes :) 🤦‍♂️

Comment on lines +45 to +59
override func willSave() {
super.willSave()

// Change to the `trunctedAt` value have effect on messages, we need to mark them dirty manually
// to triggers related FRC updates
if changedValues().keys.contains("truncatedAt") {
messages
.filter { !$0.hasChanges }
.forEach {
// Simulate an update
$0.willChangeValue(for: \.id)
$0.didChangeValue(for: \.id)
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't understand what's going on here... What are the updates we need to trigger?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created CIS-695 which improves this. It's the same trick we use for attachments and messages. Fetched results controller doesn't observe changes in related objects, even when you specify them in the predicate. So for example:

predicate = ALL messages WHERE message.createdAt > channel.truncatedAt

will get evaluated properly when you fetch the first time. However, when you change the channel.truncatedAt value, the FRC ignores it because it observers only changes to MessageDTO entity. This way we force the FRC to re-evaluate the predicate and update the results.

@@ -260,3 +260,16 @@ private extension Logger {
}
}
}

public extension Data {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why public?

@@ -36,7 +36,9 @@ enum EventType: String, Codable {
case channelDeleted = "channel.deleted"
/// When a channel was hidden.
case channelHidden = "channel.hidden"

/// When a channel was trucnated.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// When a channel was trucnated.
/// When a channel was truncated.

// Simulate `deleteChannel(cid:, completion:)` call
// Simulate `truncateChannel(cid:, completion:)` call
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change?

@VojtaStavik VojtaStavik force-pushed the CIS-692-channel-truncating branch from 2f7eb4c to 0eae013 Compare March 4, 2021 17:41
}
}
}

Copy link
Collaborator

@Stream-SDK-Bot Stream-SDK-Bot Mar 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Files should have a single trailing newline.
trailing_newline Logger.swift:276

@Stream-SDK-Bot
Copy link
Collaborator

1 Warning
⚠️ Big PR

Generated by 🚫 Danger

@codecov
Copy link

codecov bot commented Mar 4, 2021

Codecov Report

Merging #864 (87b6284) into main (54846bb) will decrease coverage by 0.54%.
The diff coverage is 89.23%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #864      +/-   ##
==========================================
- Coverage   87.35%   86.80%   -0.55%     
==========================================
  Files         229      230       +1     
  Lines        8680     8744      +64     
==========================================
+ Hits         7582     7590       +8     
- Misses       1098     1154      +56     
Impacted Files Coverage Δ
Sources/StreamChat/ChatClient.swift 95.69% <ø> (ø)
Sources/StreamChat/Utils/Logger/Logger.swift 79.74% <0.00%> (-7.76%) ⬇️
...eamChat/APIClient/Endpoints/ChannelEndpoints.swift 100.00% <100.00%> (ø)
...trollers/ChannelController/ChannelController.swift 91.38% <100.00%> (+0.18%) ⬆️
Sources/StreamChat/Database/DTOs/ChannelDTO.swift 100.00% <100.00%> (ø)
Sources/StreamChat/Database/DTOs/MessageDTO.swift 98.63% <100.00%> (+0.02%) ⬆️
...tMiddlewares/ChannelTruncatedEventMiddleware.swift 100.00% <100.00%> (ø)
...eamChat/WebSocketClient/Events/ChannelEvents.swift 93.33% <100.00%> (+1.02%) ⬆️
.../StreamChat/WebSocketClient/Events/EventType.swift 100.00% <100.00%> (ø)
...s/StreamChat/WebSocketClient/WebSocketClient.swift 86.47% <100.00%> (+0.08%) ⬆️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 54846bb...87b6284. Read the comment docs.

}
} completion: { error in
if let error = error {
log.error("Failed to update message reaction in the database, error: \(error)")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(While copying your code for my Watcher Middleware) I've realized you forgot to change the error message here :trollface:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice 😅

@VojtaStavik VojtaStavik force-pushed the CIS-692-channel-truncating branch from 1d4d142 to 87b6284 Compare March 5, 2021 10:25
@VojtaStavik VojtaStavik merged commit 92f3b90 into main Mar 5, 2021
@VojtaStavik VojtaStavik deleted the CIS-692-channel-truncating branch March 5, 2021 11:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌐 SDK: StreamChat (LLC) Tasks related to the StreamChat LLC SDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants