Page MenuHomePhabricator

[services] Tunnelbroker - Update FCM library wrapper with return of token-related errors
ClosedPublic

Authored by max on Sep 15 2022, 6:40 AM.
Tags
None
Referenced Files
F3378562: D5145.id16693.diff
Wed, Nov 27, 11:33 AM
F3378535: D5145.id16988.diff
Wed, Nov 27, 11:23 AM
Unknown Object (File)
Sat, Nov 23, 9:42 PM
Unknown Object (File)
Sat, Nov 23, 6:48 PM
Unknown Object (File)
Sat, Nov 23, 6:03 PM
Unknown Object (File)
Sat, Nov 23, 5:45 PM
Unknown Object (File)
Sat, Nov 23, 3:32 PM
Unknown Object (File)
Thu, Nov 14, 2:52 PM

Details

Summary

This diff introduces changes to the FCM library wrapper and fcm_status shared enum.

To distinguish common errors and wrong/expired device token errors the function should return different results.
Following a current keyserver's approach we should check for the following errors:

To pass the different errors to the C++ level the best and most elegant way is to return enum as Ok Rust result which formats like:

enum fcm_status {
        Ok,
        InvalidRegistration,
        NotRegistered,
}

and use CXX shared types.

Because when can't distinguish error types from Rust on the C++ side, they are always rust::error type and have only what() method.
Also, we will catch the common errors which are not needed to be distinguished using the catch approach.

Related Linear task: ENG-1764

Test Plan

App building:

  1. Rust:

Run cargo build from services/tunnelbroker/rust-notifications directory.
Rust library will be successfully built.

  1. C++ (Docker):

Running run run-tunnelbroker-service successfully built the Rust library and link it.

  1. C++ (Nix):

Call cd services/tunnelbroker.
Running rm -dfr build && cmake -B build . && make -C build -j4 successfully built the Rust library and link it.

Notifications testings:
Library notifications push method can be called from the C++ as:

#include "rust_notifications/src/lib.rs.h"
#include "rust/cxx.h"
...

const fcmReturnStatus result = sendNotifToFCM = sendNotifToFCM(
    char const *fcmAPIKey,
    char const *deviceRegistrationID,
    char const *messageTitle,
    char const *messageBody
);

Behavior testings:

  • Test where errors are not related to the tokens appears and must be thrown and caught by the C++ catch:
    • Wrong server key throws a Rust exception error.
  • Test token-related errors:
    • Wrong token format returns an fcmReturnStatus enum with the value of InvalidRegistration.
    • The client requested a new token and we are sending notifs to the old one: return a fcmReturnStatus enum with the value of NotRegistered.
  • Test with good device token:
    • Returns an fcmReturnStatus enum with the value of Ok.

Diff Detail

Repository
rCOMM Comm
Branch
add-token-errs-fcm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

max held this revision as a draft.

Removing unnecessary semicolons.

max published this revision for review.Sep 15 2022, 7:07 AM
max edited the summary of this revision. (Show Details)
max edited the test plan for this revision. (Show Details)
max added reviewers: varun, tomek.
jon requested changes to this revision.Sep 16 2022, 1:18 PM
jon added inline comments.
services/tunnelbroker/rust-notifications/src/fcm.rs
31–34 ↗(On Diff #16694)

This can exit early, do we want to disregard potential errors which follow.

If it's expected that an error would be last, we should probably add a comment stating that this assumption is being made.

33 ↗(On Diff #16694)

Similar to https://phab.comm.dev/D5149, I'm not sure if this should be returning Ok with a failed result

This revision now requires changes to proceed.Sep 16 2022, 1:18 PM

Adding a comment to explain why we are returning Ok with error types.

max added inline comments.
services/tunnelbroker/rust-notifications/src/fcm.rs
31–34 ↗(On Diff #16694)

This can exit early, do we want to disregard potential errors which follow.

If it's expected that an error would be last, we should probably add a comment stating that this assumption is being made.

Yes this is expected behavior. There is only one error returned from client.send(message_builder.finalize()).await?; and we are distinguishing them here. That's why this is expected behavior to return here.

33 ↗(On Diff #16694)

Similar to https://phab.comm.dev/D5149, I'm not sure if this should be returning Ok with a failed result

Copying my comment in D5149 here about the purpose to return an error type in Ok, because there are same:

The reason here to use an ENUM is to distinguish different error types on C++ side. We can't send a different type of error to the C++ side it's always rust::error type. The only information we can pass is the error text message in get it by calling .what(). From my perspective, this is a more elegant way to distinguish. You can check how it looks on the C++ side in D5210.The reason here to use an ENUM is to distinguish different error types on C++ side. We can't send a different type of error to the C++ side it's always rust::error type. The only information we can pass is the error text message in get it by calling .what(). From my perspective, this is a more elegant way to distinguish. You can check how it looks on the C++ side in D5210.

@max, can you rebase on master to see if the CI still fails?

tomek requested changes to this revision.Sep 22 2022, 6:57 AM
tomek added inline comments.
services/tunnelbroker/rust-notifications/src/fcm.rs
38–41 ↗(On Diff #16988)

By returning here we ignore all the possible error reasons for other elements in results.

This revision now requires changes to proceed.Sep 22 2022, 6:57 AM
services/tunnelbroker/rust-notifications/src/fcm.rs
38–41 ↗(On Diff #16988)

I've missed an answer to this

Yes this is expected behavior. There is only one error returned from client.send(message_builder.finalize()).await?; and we are distinguishing them here. That's why this is expected behavior to return here.

I'm not really sure. Can we at least check if there is indeed just one error?

max marked 2 inline comments as done.

Rebasing on a latest master.

@max, can you rebase on master to see if the CI still fails?

I've rebased - successfully built!
Sorry, I've missed rebasing it on the latest master today to fix it.

max marked 2 inline comments as done.
max added inline comments.
services/tunnelbroker/rust-notifications/src/fcm.rs
38–41 ↗(On Diff #16988)

I'm not really sure. Can we at least check if there is indeed just one error?

By returning here we ignore all the possible error reasons for other elements in results.

We are sending only one message here, but the FCM (and the Rust library) available to send a few messages in a bunch. That's why we possibly can have more than one result here but not in our scenario. As we expected to have only a single result here it's ok to return. To make this code more self-describing I can just inspect the first result instead of iterating using for.

looks like the awkwardness is for Rust + C++ compatibility, LGTM

tomek added inline comments.
services/tunnelbroker/rust-notifications/src/fcm.rs
38–41 ↗(On Diff #16988)

Ok, that makes sense. I think that inspecting just the first item would be less confusing.

This revision is now accepted and ready to land.Sep 23 2022, 3:57 AM

Merging on recent changes. Changes to inspect first error item instead of iterating.

max added inline comments.
services/tunnelbroker/rust-notifications/src/fcm.rs
38–41 ↗(On Diff #16988)

Ok, that makes sense. I think that inspecting just the first item would be less confusing.

I've changed to inspect the first error item instead of iteration.