Page MenuHomePhabricator
Differential D5207

[services] Tunnelbroker - Update sending of the `newNotifyTokenRequired` in `Get` stream request
ClosedPublic
Actions

Authored by • max on Sep 21 2022, 4:06 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Apr 9, 1:20 AM
Unknown Object (File)
Wed, Apr 9, 1:20 AM
Unknown Object (File)
Wed, Apr 9, 1:20 AM
Unknown Object (File)
Wed, Apr 9, 1:20 AM
Unknown Object (File)
Wed, Apr 9, 1:20 AM
Unknown Object (File)
Wed, Apr 9, 1:20 AM
Unknown Object (File)
Mar 19 2025, 7:33 PM
Unknown Object (File)
Mar 19 2025, 7:33 PM
View All Files
Subscribers
• abosh
ashoat
atul
tomek

Details

Reviewers
• jon
tomek
Commits
rCOMM3e557c0336aa: [services] Tunnelbroker - Update sending of the `newNotifyTokenRequired` in…
Summary

This diff introduces sending of the newNotifyTokenRequired to the client in Get stream request.
When the device has an invalid or expired notification token it's empty in the database and we are requesting a new token from the client using the newNotifyTokenRequired response.
The client gets back with the new token in Get stream request when starts listening for new messages (getting messages) from the Tunnelbroker (ref to D5206).

This is a part of the stack.
This approach was used keeping in mind the transition to the bidirectional stream soon.

Related Linear task: ENG-1782 and ENG-1816

Test Plan

Successfully built the service and passed unit and integration tests.

Diff Detail

Repository
rCOMM Comm
Branch
handler-update-device-token-request
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

• max created this revision.Sep 21 2022, 4:06 PM
• max held this revision as a draft.
Herald added a reviewer: • jon. · View Herald TranscriptSep 21 2022, 4:06 PM
Herald added subscribers: • abosh, atul, tomek, ashoat. · View Herald Transcript
• max mentioned this in D5206: [services] Tunnelbroker - Update handling of the `newNotifyToken` in `Get` stream request.Sep 21 2022, 4:08 PM
• max published this revision for review.Sep 21 2022, 4:11 PM
• max edited the summary of this revision. (Show Details)
• max edited the test plan for this revision. (Show Details)
• max added a reviewer: tomek.
• max added a parent revision: D5206: [services] Tunnelbroker - Update handling of the `newNotifyToken` in `Get` stream request.
Harbormaster completed remote builds in B12360: Diff 16969.Sep 21 2022, 4:16 PM
• max mentioned this in D5210: [services] Tunnelbroker - Updating `Send` handler with using of the Rust notification library.Sep 22 2022, 5:12 AM
• jon accepted this revision.Sep 22 2022, 4:56 PM
tomek accepted this revision.Sep 23 2022, 5:11 AM
tomek added inline comments.
services/tunnelbroker/src/Service/TunnelbrokerServiceImpl.cpp
199–201

Is it really failed? I guess it depends, because sending a response where we inform that a new token is required sounds like a success. But making it a failure also makes some sense. So up to you.

This revision is now accepted and ready to land.Sep 23 2022, 5:11 AM
ashoat added inline comments.Sep 23 2022, 5:20 AM
services/tunnelbroker/src/Service/TunnelbrokerServiceImpl.cpp
199–201

We should not fail. Some devices will not have a notifyToken

tomek requested changes to this revision.Sep 23 2022, 5:21 AM
This revision now requires changes to proceed.Sep 23 2022, 5:21 AM
• max planned changes to this revision.Sep 23 2022, 5:53 AM
• max updated this revision to Diff 17029.Sep 23 2022, 7:37 AM

Removing return of error if the device token is empty.

• max marked 2 inline comments as done.Sep 23 2022, 7:38 AM
• max added inline comments.
services/tunnelbroker/src/Service/TunnelbrokerServiceImpl.cpp
199–201

Is it really failed? I guess it depends, because sending a response where we inform that a new token is required sounds like a success. But making it a failure also makes some sense. So up to you.

We should not fail. Some devices will not have a notifyToken

Makes sense to remove returning of the error here.
I've removed the return of error here.

Harbormaster failed remote builds in B12410: Diff 17029!Sep 23 2022, 7:44 AM
tomek accepted this revision.Sep 23 2022, 7:52 AM
tomek added inline comments.
services/tunnelbroker/src/Service/TunnelbrokerServiceImpl.cpp
188–189 ↗(On Diff #17029)

Shouldn't we fetch the session item again after this update?

194–198 ↗(On Diff #17029)

Shouldn't we clear the response after it is sent?

This revision is now accepted and ready to land.Sep 23 2022, 7:52 AM
Harbormaster completed remote builds in B12410: Diff 17029.Sep 23 2022, 11:14 AM
• max mentioned this in rCOMMa206a33acdc2: [services] Tunnelbroker - Updating `Send` handler with using of the Rust….Sep 23 2022, 12:46 PM
• max mentioned this in rCOMM8f4cc18c8948: [services] Tunnelbroker - Update handling of the `newNotifyToken` in `Get`….Sep 23 2022, 12:49 PM
• max updated this revision to Diff 17063.Sep 23 2022, 1:04 PM
• max marked 2 inline comments as done.

Adding of request cleaning.
Adding of session re-fetching after token update.
Resolving merge conflict.

• max marked an inline comment as done.Sep 23 2022, 1:05 PM
• max added inline comments.
services/tunnelbroker/src/Service/TunnelbrokerServiceImpl.cpp
188–189 ↗(On Diff #17029)

Shouldn't we fetch the session item again after this update?

Nice catch! I've added refetching of the session after the token update.

194–198 ↗(On Diff #17029)

Shouldn't we clear the response after it is sent?

Yes, that makes sense! I've added the .Clean() here.

This revision was landed with ongoing or failed builds.Sep 23 2022, 1:13 PM
Closed by commit rCOMM3e557c0336aa: [services] Tunnelbroker - Update sending of the `newNotifyTokenRequired` in… (authored by • max). · Explain Why
This revision was automatically updated to reflect the committed changes.
• max marked an inline comment as done.
• max added a commit: rCOMM3e557c0336aa: [services] Tunnelbroker - Update sending of the `newNotifyTokenRequired` in….
Harbormaster completed remote builds in B12427: Diff 17063.Sep 23 2022, 1:14 PM
tomek added inline comments.Sep 26 2022, 1:33 AM
services/tunnelbroker/src/Service/TunnelbrokerServiceImpl.cpp
188–189 ↗(On Diff #17029)

The fact that this was caught during the review suggests that this functionality wasn't tested thoroughly. Could you test if setting new token works properly? Have you checked it before this change?

• max marked an inline comment as done.Sep 26 2022, 1:37 AM
• max added inline comments.
services/tunnelbroker/src/Service/TunnelbrokerServiceImpl.cpp
188–189 ↗(On Diff #17029)

The fact that this was caught during the review suggests that this functionality wasn't tested thoroughly. Could you test if setting new token works properly? Have you checked it before this change?

Sure, I'll re-test all the API flow and will re-check it now (I'm testing now using httpyac script ) The fact that this condition was lost in this certain diff was the result of the code merging from my side.

Thanks, @tomek !

• max marked an inline comment as done.Sep 26 2022, 1:39 AM
• max added inline comments.
services/tunnelbroker/src/Service/TunnelbrokerServiceImpl.cpp
188–189 ↗(On Diff #17029)

The fact that this was caught during the review suggests that this functionality wasn't tested thoroughly. Could you test if setting new token works properly? Have you checked it before this change?

Sure, I'll re-test all the API flow and will re-check it now (I'm testing now using httpyac script ) The fact that this condition was lost in this certain diff was the result of the code merging from my side.

Thanks, @tomek !

By the way, upcoming integration tests in CI will catch such errors.

Revision Contents
Changeset List

PathSize
services/
tunnelbroker/
src/
Service/
12 lines

Diff 16969

View Options

services/tunnelbroker/src/Service/TunnelbrokerServiceImpl.cpp

// @flow // @flow
import invariant from 'invariant'; import invariant from 'invariant';
import * as React from 'react'; import * as React from 'react';
import { View } from 'react-native'; import { View } from 'react-native';
import { ScrollView } from 'react-native-gesture-handler'; import { ScrollView } from 'react-native-gesture-handler';
import { fetchPinnedMessages } from 'lib/actions/message-actions.js'; import { fetchPinnedMessages } from 'lib/actions/message-actions.js';
import { messageListData } from 'lib/selectors/chat-selectors.js'; import { messageListData } from 'lib/selectors/chat-selectors.js';
import { createMessageInfo } from 'lib/shared/message-utils.js'; import { createMessageInfo } from 'lib/shared/message-utils.js';
import { isComposableMessageType } from 'lib/types/message-types.js';
import type { ThreadInfo } from 'lib/types/thread-types.js'; import type { ThreadInfo } from 'lib/types/thread-types.js';
import { useServerCall } from 'lib/utils/action-utils.js'; import { useServerCall } from 'lib/utils/action-utils.js';
import { useHeightMeasurer } from './chat-context.js'; import { useHeightMeasurer } from './chat-context.js';
import type { ChatNavigationProp } from './chat.react'; import type { ChatNavigationProp } from './chat.react';
import MessageResult from './message-result.react.js'; import MessageResult from './message-result.react.js';
import type { NavigationRoute } from '../navigation/route-names'; import type { NavigationRoute } from '../navigation/route-names';
import { useSelector } from '../redux/redux-utils.js'; import { useSelector } from '../redux/redux-utils.js';
▲ Show 20 Lines • Show All 46 Lines • ▼ Show 20 Lines const chatMessageInfos = useSelector(
messageListData(threadInfo.id, translatedMessageResults), messageListData(threadInfo.id, translatedMessageResults),
); );
const sortedUniqueChatMessageInfoItems = React.useMemo(() => { const sortedUniqueChatMessageInfoItems = React.useMemo(() => {
if (!chatMessageInfos) { if (!chatMessageInfos) {
return []; return [];
} }
const pinnedMessageIDs = new Set(); const pinnedMessageIDs = new Set();
translatedMessageResults.forEach(item => pinnedMessageIDs.add(item.id)); translatedMessageResults.forEach(item => pinnedMessageIDs.add(item.id));
inkaUnsubmitted
Not Done
Inline Actions

We could probably go back to using isPinned property here. The thing we undid in D8097. This would make the code more readable, but it's up to you

inka: We could probably go back to using `isPinned` property here. The thing we undid in D8097. This…
const chatMessageInfoItems = chatMessageInfos.filter( const chatMessageInfoItems = chatMessageInfos.filter(
item => item =>
item.itemType === 'message' && item.itemType === 'message' &&
pinnedMessageIDs.has(item.messageInfo.id), pinnedMessageIDs.has(item.messageInfo.id) &&
isComposableMessageType(item.messageInfo.type),
rohanAuthorUnsubmitted
Done
Inline Actions

As mentioned in the description, this is because toggle_pin messages are visible robotext, so if we don't filter those out then they will become visible in the message results modal. This is done here, after the ChatMessageInfos have been created, so we can accurately use the updated fetched related messages to determine the isPinned flag. If we filtered this earlier, then we'd be in the same situation as before

This also is fine because we only allow the user to pin composable message types so we won't exclude anything from the view that we need to show.

rohan: As mentioned in the description, this is because toggle_pin messages are visible robotext, so…
); );
// By the nature of using messageListData and passing in // By the nature of using messageListData and passing in
// the desired translatedMessageResults as additional // the desired translatedMessageResults as additional
// messages, we will have duplicate ChatMessageInfoItems. // messages, we will have duplicate ChatMessageInfoItems.
const uniqueChatMessageInfoItemsMap = new Map(); const uniqueChatMessageInfoItemsMap = new Map();
chatMessageInfoItems.forEach( chatMessageInfoItems.forEach(
item => item =>
▲ Show 20 Lines • Show All 93 Lines • Show Last 20 Lines