Repository: firebase/firebase-admin-java (java)

Issue:
BatchResponse implementation makes testing difficult.
  * Operating System version: N/A
  * Firebase SDK version: 2
  * Library version: 6.12.2
  * Firebase Product: Messaging

#### Description:

BatchResponse is implemented as a final class with a package-private constructor. As such, this means that as a consumer of the library I have to either wrap the response object or use reflection to make it testable.

#### Steps to reproduce:

```java
// This will not compile
new BatchResponse(responses);
```

#### Suggestions

I would be happy to submit a PR for this but I would like guidance on implementation preference.

a) Remove the final keyword from the BatchResponse class so tools like Mockito can mock the class.
b) Make the constructor public
c) Add a builder or some other method of instantiating the class. This doesn't seem like a useful change.


Pull request:
Testable BatchResponse
Resolves #388 
- Converts batch response to an interface
- Moves batch response implementation to BatchResponseImpl
- Update FirebaseMessagingClientImpl to use BatchResponseImpl


Patch:
diff --git a/src/main/java/com/google/firebase/messaging/BatchResponse.java b/src/main/java/com/google/firebase/messaging/BatchResponse.java
index bd5069f..164403b 100644
--- a/src/main/java/com/google/firebase/messaging/BatchResponse.java
+++ b/src/main/java/com/google/firebase/messaging/BatchResponse.java
@@ -16,7 +16,6 @@
 
 package com.google.firebase.messaging;
 
-import com.google.common.collect.ImmutableList;
 import com.google.firebase.internal.NonNull;
 import java.util.List;
 
@@ -25,32 +24,12 @@ import java.util.List;
  * See {@link FirebaseMessaging#sendAll(List)} and {@link
  * FirebaseMessaging#sendMulticast(MulticastMessage)}.
  */
-public final class BatchResponse {
-
-  private final List<SendResponse> responses;
-  private final int successCount;
-
-  BatchResponse(List<SendResponse> responses) {
-    this.responses = ImmutableList.copyOf(responses);
-    int successCount = 0;
-    for (SendResponse response : this.responses) {
-      if (response.isSuccessful()) {
-        successCount++;
-      }
-    }
-    this.successCount = successCount;
-  }
+public interface BatchResponse {
 
   @NonNull
-  public List<SendResponse> getResponses() {
-    return responses;
-  }
+  List<SendResponse> getResponses();
 
-  public int getSuccessCount() {
-    return successCount;
-  }
+  int getSuccessCount();
 
-  public int getFailureCount() {
-    return responses.size() - successCount;
-  }
+  int getFailureCount();
 }
diff --git a/src/main/java/com/google/firebase/messaging/BatchResponseImpl.java b/src/main/java/com/google/firebase/messaging/BatchResponseImpl.java
new file mode 100644
index 0000000..99cf63d
--- /dev/null
+++ b/src/main/java/com/google/firebase/messaging/BatchResponseImpl.java
@@ -0,0 +1,58 @@
+/*
+ * Copyright  2020 Google Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ *  you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.google.firebase.messaging;
+
+import com.google.common.collect.ImmutableList;
+import com.google.firebase.internal.NonNull;
+
+import java.util.List;
+
+/**
+ * Response from an operation that sends FCM messages to multiple recipients.
+ * See {@link FirebaseMessaging#sendAll(List)} and {@link
+ * FirebaseMessaging#sendMulticast(MulticastMessage)}.
+ */
+class BatchResponseImpl implements BatchResponse {
+
+  private final List<SendResponse> responses;
+  private final int successCount;
+
+  BatchResponseImpl(List<SendResponse> responses) {
+    this.responses = ImmutableList.copyOf(responses);
+    int successCount = 0;
+    for (SendResponse response : this.responses) {
+      if (response.isSuccessful()) {
+        successCount++;
+      }
+    }
+    this.successCount = successCount;
+  }
+
+  @NonNull
+  public List<SendResponse> getResponses() {
+    return responses;
+  }
+
+  public int getSuccessCount() {
+    return successCount;
+  }
+
+  public int getFailureCount() {
+    return responses.size() - successCount;
+  }
+
+}
diff --git a/src/main/java/com/google/firebase/messaging/FirebaseMessagingClientImpl.java b/src/main/java/com/google/firebase/messaging/FirebaseMessagingClientImpl.java
index c2659a2..53d5ab0 100644
--- a/src/main/java/com/google/firebase/messaging/FirebaseMessagingClientImpl.java
+++ b/src/main/java/com/google/firebase/messaging/FirebaseMessagingClientImpl.java
@@ -167,7 +167,7 @@ final class FirebaseMessagingClientImpl implements FirebaseMessagingClient {
     MessagingBatchCallback callback = new MessagingBatchCallback();
     BatchRequest batch = newBatchRequest(messages, dryRun, callback);
     batch.execute();
-    return new BatchResponse(callback.getResponses());
+    return new BatchResponseImpl(callback.getResponses());
   }
 
   private BatchRequest newBatchRequest(


Test patch:
diff --git a/src/test/java/com/google/firebase/messaging/BatchResponseTest.java b/src/test/java/com/google/firebase/messaging/BatchResponseTest.java
index cea0010..9c174f5 100644
--- a/src/test/java/com/google/firebase/messaging/BatchResponseTest.java
+++ b/src/test/java/com/google/firebase/messaging/BatchResponseTest.java
@@ -31,7 +31,7 @@ public class BatchResponseTest {
   public void testEmptyResponses() {
     List<SendResponse> responses = new ArrayList<>();
 
-    BatchResponse batchResponse = new BatchResponse(responses);
+    BatchResponse batchResponse = new BatchResponseImpl(responses);
 
     assertEquals(0, batchResponse.getSuccessCount());
     assertEquals(0, batchResponse.getFailureCount());
@@ -47,7 +47,7 @@ public class BatchResponseTest {
             "error-message", null))
     );
 
-    BatchResponse batchResponse = new BatchResponse(responses);
+    BatchResponse batchResponse = new BatchResponseImpl(responses);
 
     assertEquals(2, batchResponse.getSuccessCount());
     assertEquals(1, batchResponse.getFailureCount());
@@ -61,7 +61,7 @@ public class BatchResponseTest {
   public void testResponsesImmutable() {
     List<SendResponse> responses = new ArrayList<>();
     responses.add(SendResponse.fromMessageId("message1"));
-    BatchResponse batchResponse = new BatchResponse(responses);
+    BatchResponse batchResponse = new BatchResponseImpl(responses);
     SendResponse sendResponse = SendResponse.fromMessageId("message2");
 
     try {
@@ -74,6 +74,6 @@ public class BatchResponseTest {
 
   @Test(expected = NullPointerException.class)
   public void testResponsesCannotBeNull() {
-    new BatchResponse(null);
+    new BatchResponseImpl(null);
   }
 }
diff --git a/src/test/java/com/google/firebase/messaging/FirebaseMessagingTest.java b/src/test/java/com/google/firebase/messaging/FirebaseMessagingTest.java
index bebba08..4968231 100644
--- a/src/test/java/com/google/firebase/messaging/FirebaseMessagingTest.java
+++ b/src/test/java/com/google/firebase/messaging/FirebaseMessagingTest.java
@@ -668,7 +668,7 @@ public class FirebaseMessagingTest {
     for (String messageId : messageIds) {
       listBuilder.add(SendResponse.fromMessageId(messageId));
     }
-    return new BatchResponse(listBuilder.build());
+    return new BatchResponseImpl(listBuilder.build());
   }
 
   private static class MockFirebaseMessagingClient implements FirebaseMessagingClient {