Skip to content

Commit 8bc413c

Browse files
mahibiAndyScherzinger
authored andcommitted
revert isPlausibleLastReadMessageId check
Checking the real data for a big instance unveiled i was pretty off with my assumption how high the difference between messageId and newestKnownRealMessageId could be. Logging for C.N.C was: "advanceLocalLastReadMessageIfNeeded, messageId (5748751) is implausibly higher than the conversation's newest known message id (5694085). We won't advance." I wont go down that road to increase PLAUSIBLE_MESSAGE_ID_BUFFER every know and then when it turns out it is still too low. The curretn logic might introduce more problems than it would solve. There should be other checks to make sure readmarkers don't make it to the server when they are wrong. Signed-off-by: Marcel Hibbe <dev@mhibbe.de>
1 parent bd7e226 commit 8bc413c

2 files changed

Lines changed: 0 additions & 72 deletions

File tree

app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.kt

Lines changed: 0 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -1845,17 +1845,6 @@ class ChatViewModel @AssistedInject constructor(
18451845
Log.d(TAG, "advanceLocalLastReadMessageIfNeeded, messageId: $messageId")
18461846
Log.d(TAG, "advanceLocalLastReadMessageIfNeeded, localLastReadMessage: $localLastReadMessage")
18471847

1848-
val newestKnownRealMessageId = _uiState.value.conversation?.lastMessage?.id
1849-
1850-
if (!isPlausibleLastReadMessageId(messageId, newestKnownRealMessageId)) {
1851-
logger.w(
1852-
TAG,
1853-
"advanceLocalLastReadMessageIfNeeded, messageId ($messageId) is implausibly higher than the " +
1854-
"conversation's newest known message id ($newestKnownRealMessageId). We won't advance."
1855-
)
1856-
return
1857-
}
1858-
18591848
if (localLastReadMessage < messageId && -1 < messageId) {
18601849
Log.d(TAG, "advanceLocalLastReadMessageIfNeeded, setting localLastReadMessage to $messageId")
18611850
localLastReadMessage = messageId
@@ -2487,18 +2476,6 @@ class ChatViewModel @AssistedInject constructor(
24872476
// cycle (up to 2 minutes later) before its "sent, not yet confirmed" spinner clears.
24882477
private val POST_UPLOAD_FETCH_RETRY_DELAYS_MS = listOf(1_000L, 1_500L, 2_000L, 3_000L, 4_000L, 5_000L, 5_000L)
24892478
private const val LOCAL_PREVIEW_GRACE_PERIOD_MS = 15_000L
2490-
private const val PLAUSIBLE_MESSAGE_ID_BUFFER = 10_000L
2491-
2492-
/**
2493-
* A real server-assigned message id is always positive, so a null, zero or negative
2494-
* [newestKnownRealMessageId] is never a trustworthy ceiling to judge [messageId] against -
2495-
* e.g. a federated conversation's cached lastMessage can carry an id of 0 when it was
2496-
* never populated. Without a trustworthy ceiling, [messageId] is accepted unchecked here.
2497-
*/
2498-
fun isPlausibleLastReadMessageId(messageId: Int, newestKnownRealMessageId: Long?): Boolean {
2499-
val trustworthyCeiling = newestKnownRealMessageId?.takeIf { it > 0 } ?: return true
2500-
return messageId <= trustworthyCeiling + PLAUSIBLE_MESSAGE_ID_BUFFER
2501-
}
25022479
}
25032480

25042481
sealed class OutOfOfficeUIState {

app/src/test/java/com/nextcloud/talk/chat/viewmodels/ChatViewModelTest.kt

Lines changed: 0 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -19,55 +19,6 @@ import java.time.LocalDate
1919

2020
class ChatViewModelTest {
2121

22-
@Test
23-
fun `isPlausibleLastReadMessageId returns true when there is no known real message id yet`() {
24-
assertTrue(ChatViewModel.isPlausibleLastReadMessageId(messageId = 158761, newestKnownRealMessageId = null))
25-
}
26-
27-
@Test
28-
fun `isPlausibleLastReadMessageId returns true for the newest known real message id itself`() {
29-
assertTrue(ChatViewModel.isPlausibleLastReadMessageId(messageId = 158761, newestKnownRealMessageId = 158761L))
30-
}
31-
32-
@Test
33-
fun `isPlausibleLastReadMessageId returns true for an older message id`() {
34-
assertTrue(ChatViewModel.isPlausibleLastReadMessageId(messageId = 100, newestKnownRealMessageId = 158761L))
35-
}
36-
37-
@Test
38-
fun `isPlausibleLastReadMessageId returns true within the buffer above the newest known message id`() {
39-
assertTrue(
40-
ChatViewModel.isPlausibleLastReadMessageId(
41-
messageId = 158761 + 10_000,
42-
newestKnownRealMessageId = 158761L
43-
)
44-
)
45-
}
46-
47-
@Test
48-
fun `isPlausibleLastReadMessageId returns false just beyond the buffer above the newest known message id`() {
49-
assertFalse(
50-
ChatViewModel.isPlausibleLastReadMessageId(
51-
messageId = 158761 + 10_000 + 1,
52-
newestKnownRealMessageId = 158761L
53-
)
54-
)
55-
}
56-
57-
@Test
58-
fun `isPlausibleLastReadMessageId rejects a hash-derived placeholder id far beyond the real message id space`() {
59-
assertFalse(
60-
ChatViewModel.isPlausibleLastReadMessageId(messageId = 1_963_726_147, newestKnownRealMessageId = 158761L)
61-
)
62-
}
63-
64-
@Test
65-
fun `isPlausibleLastReadMessageId treats a newest known id of 0 as unknown`() {
66-
// A federated conversation's cached lastMessage can carry an id of 0 - a real message id
67-
// is never 0, so this must not be treated as a real ceiling near the start of the room.
68-
assertTrue(ChatViewModel.isPlausibleLastReadMessageId(messageId = 136556, newestKnownRealMessageId = 0L))
69-
}
70-
7122
// The unread marker latch: the marker position must only be derived from the visible window
7223
// when the window provably reaches back to the unread boundary — otherwise a window of
7324
// only-unread messages (e.g. after a capped fetch of the newest messages) would place the

0 commit comments

Comments
 (0)