Firebase Cloud Messaging API #127

hiranya911 · 2018-01-17T19:14:20Z

Implements the messaging related types and the following entry points:

FirebaseMessaging.getInstance()
FirebaseMessaging.getInstance(FirebaseApp app)
FirebaseMessaging.sendAsync(Message msg)
FirebaseMessaging.sendAsync(Message msg, boolean dryRun)
FirebaseMessaging.subscribeToTopicAsync(List<String> tokens, String topic)
FirebaseMessaging.unsubscribeFromTopicAsync(List<String> tokens, String topic)

go/firebase-admin-fcm-api

jcvillalta03 · 2018-01-18T17:10:25Z

@hiranya911 I am wondering if there is a ticket some where of features desired in this PR. I have been working on an implementation myself in a personal project and would love to contribute if there is an actual list of requirements for the API

hiranya911 · 2018-01-18T18:20:37Z

@jcvillalta03 The initial offering is going to include the methods described above. If you have ideas for more features or further expansions, please feel free to create a new issue in this repo.

jcvillalta03 · 2018-01-18T18:25:14Z

Thanks @hiranya911. So at this point, this is initial offering is considered “code complete”?

hiranya911 · 2018-01-18T18:31:28Z

@jcvillalta03 for the moment yes. Equivalent set of operations are being implemented in other Admin SDK variants (Node, Python and Go) as well. But you should feel encouraged to propose changes/additions on top of what I have done here. If a certain use case cannot be handled properly with the initial set of APIs, we'd like to know, and see if we can do something about it.

jcvillalta03 · 2018-01-18T18:48:14Z

Ok, thanks for the explanation. Sorry to keeping conversation going in comments, I just wanted to make sure that I understand the Contribution guidelines.
#101 (which I believe this implements) was a high-level request, but now I see that the requirements are to match functionality of other Admin SDKs. 👍 on using HTTP v1.

I look forward to this release!

… into hkj-fcm-api

schmidt-sebastian

Overall, this is very good. I left some nits from an initial review that apply to a few places in this PR. It might make sense for you to do a first pass on these before I look at this in more depth.

schmidt-sebastian · 2018-01-30T01:38:52Z

src/main/java/com/google/firebase/messaging/AndroidConfig.java

@@ -0,0 +1,201 @@
+/*
+ * Copyright 2017 Google Inc.


Done. Also added the header to a set of files that were missing it.

schmidt-sebastian · 2018-01-30T01:39:46Z

src/main/java/com/google/firebase/messaging/AndroidConfig.java

+
+/**
+ * Represents the Android-specific options that can be included in a {@link Message}.
+ * Instances of this class are thread-safe and immutable.


How does this show up in the JavaDoc output? I wonder if we should add a newline (or <p>).

They show up as a single paragraph. We already have a quite a bit of similarly documented classes (See UserRecord class from auth for example)

schmidt-sebastian · 2018-01-30T01:43:30Z

src/main/java/com/google/firebase/messaging/AndroidConfig.java

+        this.ttl = String.format("%ds", seconds);
+      }
+    } else {
+      this.ttl = null;


Nit: This is not needed since it is the default (here and above)

It is needed since it's final

schmidt-sebastian · 2018-01-30T01:46:07Z

src/main/java/com/google/firebase/messaging/AndroidConfig.java

+     * @return This builder.
+     */
+    public Builder putData(@NonNull String key, @NonNull String value) {
+      this.data.put(key, value);


This doesn't override the existing value. Do you instead mean that the final AndroidConfig's data map will be overridden?

It overrides the data set using the Message.Builder class. It's an FCM mechanic executed at the backend. In other words AndroidConfig.data map gets priority over Message.data map when FCM sends notifications. I've slightly modified the text to make this clear.

schmidt-sebastian · 2018-01-30T01:48:03Z

src/main/java/com/google/firebase/messaging/AndroidNotification.java

+    this.body = builder.body;
+    this.icon = builder.icon;
+    if (builder.color != null) {
+      checkArgument(builder.color.matches("^#[0-9a-fA-F]{6}$"),


Suggestion (here and below):

this.color = checkArgument(...)

Actually checkArgument() does not return anything. Only checkNotNull() returns the input argument.

schmidt-sebastian · 2018-01-30T01:57:54Z

src/main/java/com/google/firebase/messaging/Aps.java

+  private final String threadId;
+
+  private Aps(Builder builder) {
+    int alerts = Booleans.countTrue(


How about:

checkArgument(Strings.isNullOrEmpty(builder.alertString) || builder.alert == null, ...)

schmidt-sebastian · 2018-01-30T02:00:04Z

src/main/java/com/google/firebase/messaging/Aps.java

+
+    /**
+     * Sets the badge to be displayed with the message. Set to 0 to remove the badge. Do not
+     * specify any value to keep the badge unchanged.


This reads a little weird since it is not possible to not specify this value using this API. Can you make it more clear that you mean "do not call this method"?

schmidt-sebastian · 2018-01-30T02:04:33Z

src/main/java/com/google/firebase/messaging/FirebaseMessaging.java

+  }
+
+  /**
+   * Sends the given {@link Message} via Firebase Cloud Messaging. If the {@code dryRun} option


Insert new paragraph after the first sentence.

schmidt-sebastian · 2018-01-30T02:37:47Z

src/main/java/com/google/firebase/messaging/TopicManagementResponse.java

+
+  // Server error codes as defined in https://developers.google.com/instance-id/reference/server
+  // TODO: Should we handle other error codes here (e.g. PERMISSION_DENIED)?
+  private static final Map<String, String> ERROR_CODES = ImmutableMap.<String, String>builder()


Can we move these to a single place so the Admin SDK can share these errors?

The backend error codes (keys of the map) are specific to the services we invoke at various places. I'm not sure there's a lot to be gained by sharing them across the package/SDK. What sort of sharing were you thinking of? Put them all in a shared FirebaseMessagingConstants class?

Yeah, that's what I was leaning towards. It looks like https://github.com/firebase/firebase-admin-java/blob/master/src/main/java/com/google/firebase/iid/FirebaseInstanceId.java uses HTTP Response Codes though and not these same errors.

schmidt-sebastian · 2018-01-30T02:38:09Z

src/main/java/com/google/firebase/messaging/TopicManagementResponse.java

+      }
+    }
+    this.successCount = successCount;
+    this.failureCount = failureCount;


This is just errors.size().

hiranya911 · 2018-01-30T20:11:43Z

Thanks @schmidt-sebastian and @sampson-chen for the initial round of feedback. I've addressed almost all the comments. Over to you for another look.

avishalom

LGTM(nit and question)

avishalom · 2018-01-31T22:34:30Z

src/test/java/com/google/firebase/messaging/FirebaseMessagingIT.java

+    Message message = Message.builder()
+        .setNotification(new Notification("Title", "Body"))
+        .setAndroidConfig(AndroidConfig.builder()
+            .setRestrictedPackageName("com.demoapps.hkj")


avishalom · 2018-01-31T22:47:14Z

src/main/java/com/google/firebase/messaging/FirebaseMessaging.java

+
+  private String makeSendRequest(Message message,
+      boolean dryRun) throws FirebaseMessagingException {
+    ImmutableMap.Builder<String, Object> payload = ImmutableMap.<String, Object>builder()


consider ImmutableMap.of(...)

Have to use Builder here since we are conditionally putting some values.

avishalom · 2018-01-31T23:11:08Z

src/test/java/com/google/firebase/messaging/FirebaseMessagingIT.java

+    FirebaseMessaging messaging = FirebaseMessaging.getInstance();
+    TopicManagementResponse results = messaging.subscribeToTopicAsync(
+        ImmutableList.of(TEST_REGISTRATION_TOKEN), "mock-topic").get();
+    assertEquals(1, results.getSuccessCount() + results.getFailureCount());


Is there no point in testing success and failure separately like in the next FirebaseMessagingTest.java?

There's no meaningful way to test success case since the IID tokens keep changing.

sampson-chen · 2018-02-01T00:13:05Z

src/main/java/com/google/firebase/messaging/Message.java

 * information as well as the recipient information. In particular, the message must contain
 * exactly one of token, topic or condition parameters. Instances of this class are thread-safe
- * and immutable. Use {@link Message.Builder} co crete new instances.
+ * and immutable. Use {@link Message.Builder} to crete new instances.


s/crete/create/

schmidt-sebastian · 2018-02-06T01:31:40Z

src/main/java/com/google/firebase/messaging/AndroidConfig.java

+    if (builder.ttl != null) {
+      checkArgument(builder.ttl >= 0, "ttl must not be negative");
+      long seconds = TimeUnit.MILLISECONDS.toSeconds(builder.ttl);
+      long subsecondNanos = TimeUnit.MILLISECONDS.toNanos(builder.ttl - seconds * 1000L);


I think this should be 1000 * 1000.

builder.ttl is in millis. So I believe this is correct. We have a test case for this where 10ms is correctly transformed to 0.010000000s (where the decimal part is subsecond nanos).

Sorry, I think I had nanoseconds in my head.

schmidt-sebastian · 2018-02-06T01:47:13Z

src/main/java/com/google/firebase/messaging/FirebaseMessaging.java

+
+    @Override
+    public void destroy() {
+      // NOTE: We don't explicitly tear down anything here, but public methods of StorageClient


StorageClient -> FirebaseMessaging

schmidt-sebastian · 2018-02-06T01:48:05Z

src/main/java/com/google/firebase/messaging/Message.java

+/**
+ * Represents a message that can be sent via Firebase Cloud Messaging (FCM). Contains payload
+ * information as well as the recipient information. In particular, the message must contain
+ * exactly one of token, topic or condition parameters. Instances of this class are thread-safe


s/one of token, topic or condition parameters/one token, topic or condition parameter/

schmidt-sebastian · 2018-02-06T01:50:51Z

src/main/java/com/google/firebase/messaging/TopicManagementResponse.java

+
+  // Server error codes as defined in https://developers.google.com/instance-id/reference/server
+  // TODO: Should we handle other error codes here (e.g. PERMISSION_DENIED)?
+  private static final Map<String, String> ERROR_CODES = ImmutableMap.<String, String>builder()


Yeah, that's what I was leaning towards. It looks like https://github.com/firebase/firebase-admin-java/blob/master/src/main/java/com/google/firebase/iid/FirebaseInstanceId.java uses HTTP Response Codes though and not these same errors.

schmidt-sebastian · 2018-02-06T01:56:52Z

src/test/java/com/google/firebase/messaging/FirebaseMessagingTest.java

+    Map<Message, Map<String, Object>> testMessages = buildTestMessages();
+
+    for (Map.Entry<Message, Map<String, Object>> entry : testMessages.entrySet()) {
+      response.setContent("{\"name\": \"mock-name\"}");


Can you remove the code duplication in this test its counterpart?

schmidt-sebastian · 2018-02-06T01:58:42Z

src/test/java/com/google/firebase/messaging/FirebaseMessagingTest.java

+    MockLowLevelHttpResponse response = new MockLowLevelHttpResponse();
+    FirebaseMessaging messaging = initMessaging(response);
+    for (int code : HTTP_ERRORS) {
+      response.setStatusCode(code).setContent(


Same for these tests. The code here could use some de-duping.

schmidt-sebastian · 2018-02-06T02:01:04Z

src/test/java/com/google/firebase/messaging/FirebaseMessagingTest.java

+
+    ByteArrayOutputStream out = new ByteArrayOutputStream();
+    request.getContent().writeTo(out);
+    assertEquals("{\"to\":\"/topics/test-topic\",\"registration_tokens\":[\"id1\",\"id2\"]}",


This seems overly harsh as it relies on an unspecified order during the serialization.

Fixed by parsing into a map, and then comparing entries.

schmidt-sebastian · 2018-02-06T02:02:32Z

src/test/java/com/google/firebase/messaging/FirebaseMessagingTest.java

+    assertEquals(TEST_IID_UNSUBSCRIBE_URL, request.getUrl().toString());
+    assertEquals("Bearer test-token", request.getHeaders().getAuthorization());
+    assertEquals("true", request.getHeaders().get("access_token_auth"));
+


The lines from 343-356 also seem duplicated to me.

schmidt-sebastian · 2018-02-06T02:03:59Z

src/test/java/com/google/firebase/messaging/FirebaseMessagingTest.java

+        .build();
+    FirebaseApp.initializeApp(options);
+    FirebaseMessaging messaging = FirebaseMessaging.getInstance();
+    TestResponseInterceptor interceptor = new TestResponseInterceptor();


Can 314-319 be done in the @before method?

Moved to a helper method.

hiranya911 · 2018-02-06T23:53:04Z

Made the suggested changes.

* Accept prefixed topic names * Parsing topic name in a helper method * Updated javadoc for setTopic()

hiranya911 added 17 commits December 25, 2017 15:20

Adding experimental FCM API

7249ab7

Added Webpush notification support

12f7da8

Added APNS support and tests

f095734

Added subscribe method

c037733

Added TopicManagementResponse type

da0f5d1

Added unsub method

565fd01

Improved error handling and json parsing

174b302

More tests, validation and dry run mode

97a15a0

Improved error handling and tests

4798b43

Added license header; Implemented topic name normalization

4aecab9

Improved error handling

5a22fcb

Improved error handling

b856625

Added javadocs

84f9908

Merge branch 'master' into hkj-fcm-api

e845957

Fixing auth (ADC) test failure

13ae556

Updated documentation

c2d8462

Updated documentation

a4661a0

hiranya911 mentioned this pull request Jan 18, 2018

Adding Firebase Cloud Messaging firebase/firebase-admin-go#62

Merged

Updated javadocs

084e8e9

Updated test

f898ad1

hiranya911 added 5 commits January 18, 2018 14:49

Renamed Priority enum values (using uppercase)

9e5e7e7

Adding APNS types

1b598a4

Merge branch 'hkj-fcm-api' of github.com:firebase/firebase-admin-java…

c2b3d55

… into hkj-fcm-api

Updated APNS documentation

f971206

Updated integration test

7a2b311

schmidt-sebastian suggested changes Jan 30, 2018

View reviewed changes

Addressing a number of readability nits, and other code review comments

c6043be

avishalom approved these changes Jan 31, 2018

View reviewed changes

avishalom assigned hiranya911 Jan 31, 2018

hiranya911 unassigned avishalom and hiranya911 Feb 1, 2018

sampson-chen approved these changes Feb 1, 2018

View reviewed changes

hiranya911 unassigned sampson-chen Feb 1, 2018

Fixing some nits

011e6e8

avishalom approved these changes Feb 2, 2018 •

edited

Loading

View reviewed changes

schmidt-sebastian suggested changes Feb 6, 2018

View reviewed changes

hiranya911 assigned hiranya911 and unassigned schmidt-sebastian Feb 6, 2018

Removing some duplicate code in messaging tests

668b29f

hiranya911 assigned schmidt-sebastian and unassigned hiranya911 Feb 6, 2018

Further deduping code in test

854c6df

schmidt-sebastian approved these changes Feb 7, 2018

View reviewed changes

hiranya911 added 6 commits February 6, 2018 18:05

Accept prefixed topic names (#136)

b96b220

* Accept prefixed topic names * Parsing topic name in a helper method * Updated javadoc for setTopic()

Updated FCM error codes (#138)

5531a08

Merge branch 'master' into hkj-fcm-api

0b4bf41

Merge branch 'master' into hkj-fcm-api

a2a1447

Merged with dev; updated changelog

7fc9fe0

Updated changelog

374a643

hiranya911 assigned hiranya911 and unassigned schmidt-sebastian Feb 13, 2018

hiranya911 merged commit f83cdec into master Feb 13, 2018

hiranya911 deleted the hkj-fcm-api branch February 13, 2018 02:32

Firebase Cloud Messaging API #127

Firebase Cloud Messaging API #127

Uh oh!

Conversation

hiranya911 commented Jan 17, 2018

Uh oh!

jcvillalta03 commented Jan 18, 2018

Uh oh!

hiranya911 commented Jan 18, 2018

Uh oh!

jcvillalta03 commented Jan 18, 2018

Uh oh!

hiranya911 commented Jan 18, 2018

Uh oh!

jcvillalta03 commented Jan 18, 2018

Uh oh!

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schmidt-sebastian Jan 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hiranya911 Jan 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schmidt-sebastian Jan 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hiranya911 Jan 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hiranya911 Jan 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hiranya911 commented Jan 30, 2018

Uh oh!

avishalom left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schmidt-sebastian Jan 30, 2018 •

edited

Loading

hiranya911 Jan 30, 2018 •

edited

Loading

schmidt-sebastian Jan 30, 2018 •

edited

Loading

hiranya911 Jan 30, 2018 •

edited

Loading

hiranya911 Jan 30, 2018 •

edited

Loading