[services] Tunnelbroker - Add `updateSessionItemDeviceToken` database method
ClosedPublic
Actions

Authored by • max on Sep 19 2022, 7:04 AM.

Details

Reviewers

• jon
tomek

Commits

rCOMMdf8907048ae7: [services] Tunnelbroker - Add `updateSessionItemDeviceToken` database method

Summary

This diff introduces a database method to update the device notification token field.
The purpose is to update the device notification token when the token is expired or wrong.

Related Linear task: ENG-1782

Test Plan

Successfully built the server.
Run the test in D5184 using yarn run-unit-tests and yarn run-integration-tests.

Diff Detail

Repository

rCOMM Comm

Branch

database-update-device-token

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

• max created this revision.Sep 19 2022, 7:04 AM

• max held this revision as a draft.

Herald added a reviewer: • jon. · View Herald TranscriptSep 19 2022, 7:04 AM

Herald added subscribers: • abosh, • karol, atul and 2 others. · View Herald Transcript

• max published this revision for review.Sep 19 2022, 7:08 AM

• max edited the summary of this revision. (Show Details)

• max edited the test plan for this revision. (Show Details)

• max added a reviewer: tomek.

Herald added a reviewer: • karol. · View Herald TranscriptSep 19 2022, 7:09 AM

Harbormaster completed remote builds in B12275: Diff 16837.Sep 19 2022, 7:14 AM

not familiar with Aws sdk, but the C++ looks about right. I feel like having a test case for this would help a lot of asserting that it has the desired behavior

• max mentioned this in D5184: [services] Tunnelbroker - Add test for `updateSessionItemDeviceToken` database method.Sep 19 2022, 3:56 PM

• max added a child revision: D5184: [services] Tunnelbroker - Add test for `updateSessionItemDeviceToken` database method.

In D5179#151896, @jon wrote:

I feel like having a test case for this would help a lot of asserting that it has the desired behavior

Sounds reasonable! D5184 was created and the test description was updated as well.

tomek requested changes to this revision.Sep 20 2022, 7:09 AM

tomek added inline comments.

services/tunnelbroker/src/Database/DatabaseManager.cpp
141–145 ↗	(On Diff #16837)	Is there a way to inform the caller that this operation failed? Assuming that it always succeeds sounds dangerous.

This revision now requires changes to proceed.Sep 20 2022, 7:09 AM

Changes to return bool result instead of void.

• max marked an inline comment as done.Sep 21 2022, 7:10 AM

• max added inline comments.

services/tunnelbroker/src/Database/DatabaseManager.cpp
141–145 ↗	(On Diff #16837)	Is there a way to inform the caller that this operation failed? Assuming that it always succeeds sounds dangerous. I think it's a good idea to get rid of `void`ing everywhere practice in the internal database functions. I've changed it to the `bool`, and we will return `grpc::Status(grpc::StatusCode::INTERNAL, "Can not update the token in the Database");` to the client in the follow-up diff where it handles.