Repository: firebase/firebase-admin-java
Pull request #508 (2021-01-07 23:19:02)

fix(fcm): Gracefully handling non-json error responses
FCM service seems to occasionally respond with non-json (HTML) error responses, and if these responses contain certain special characters (e.g. `[` or `]`), the SDK throws a cryptic JSON parse exception (see #506). 

The `MessagingErrorHandler` is supposed to catch all parse errors, but today it only handles IOExceptions thrown by the parser. But in the above case parser throws an IllegalArgumentException. This PR addresses this issue by updating the `MessagingErrorHandler` to catch *all* exceptions thrown by the parser. This makes sure the developer gets a `FirebaseMessagingException` with the original error response attached to it, when the above error condition occurs.

RELEASE NOTE: Improved support for handling certain non-JSON error responses sent by the FCM backend service.


Patch:
diff --git a/src/main/java/com/google/firebase/messaging/FirebaseMessagingClientImpl.java b/src/main/java/com/google/firebase/messaging/FirebaseMessagingClientImpl.java
index 43b9b34..1271bf9 100644
--- a/src/main/java/com/google/firebase/messaging/FirebaseMessagingClientImpl.java
+++ b/src/main/java/com/google/firebase/messaging/FirebaseMessagingClientImpl.java
@@ -307,7 +307,7 @@ final class FirebaseMessagingClientImpl implements FirebaseMessagingClient {
         try {
           return jsonFactory.createJsonParser(response)
               .parseAndClose(MessagingServiceErrorResponse.class);
-        } catch (IOException ignore) {
+        } catch (Exception ignore) {
           // Ignore any error that may occur while parsing the error response. The server
           // may have responded with a non-json payload.
         }


Test patch:
diff --git a/src/test/java/com/google/firebase/messaging/FirebaseMessagingClientImplTest.java b/src/test/java/com/google/firebase/messaging/FirebaseMessagingClientImplTest.java
index 9f8780e..a64c68d 100644
--- a/src/test/java/com/google/firebase/messaging/FirebaseMessagingClientImplTest.java
+++ b/src/test/java/com/google/firebase/messaging/FirebaseMessagingClientImplTest.java
@@ -82,7 +82,7 @@ public class FirebaseMessagingClientImplTest {
       .setTopic("test-topic")
       .build();
   private static final List<Message> MESSAGE_LIST = ImmutableList.of(EMPTY_MESSAGE, EMPTY_MESSAGE);
-  
+
   private static final boolean DRY_RUN_ENABLED = true;
   private static final boolean DRY_RUN_DISABLED = false;
 
@@ -100,7 +100,7 @@ public class FirebaseMessagingClientImplTest {
   @Test
   public void testSend() throws Exception {
     Map<Message, Map<String, Object>> testMessages = buildTestMessages();
-    
+
     for (Map.Entry<Message, Map<String, Object>> entry : testMessages.entrySet()) {
       response.setContent(MOCK_RESPONSE);
       String resp = client.send(entry.getKey(), DRY_RUN_DISABLED);
@@ -200,14 +200,14 @@ public class FirebaseMessagingClientImplTest {
   @Test
   public void testSendErrorWithMalformedResponse() {
     for (int code : HTTP_ERRORS) {
-      response.setStatusCode(code).setContent("not json");
+      response.setStatusCode(code).setContent("[not json]");
 
       try {
         client.send(EMPTY_MESSAGE, DRY_RUN_DISABLED);
         fail("No error thrown for HTTP error");
       } catch (FirebaseMessagingException error) {
         checkExceptionFromHttpResponse(error, HTTP_2_ERROR.get(code), null,
-            "Unexpected HTTP response with status: " + code + "\nnot json");
+            "Unexpected HTTP response with status: " + code + "\n[not json]");
       }
       checkRequestHeader(interceptor.getLastRequest());
     }
diff --git a/src/test/java/com/google/firebase/remoteconfig/FirebaseRemoteConfigIT.java b/src/test/java/com/google/firebase/remoteconfig/FirebaseRemoteConfigIT.java
index 3a13d8d..bb90c6c 100644
--- a/src/test/java/com/google/firebase/remoteconfig/FirebaseRemoteConfigIT.java
+++ b/src/test/java/com/google/firebase/remoteconfig/FirebaseRemoteConfigIT.java
@@ -42,89 +42,131 @@ public class FirebaseRemoteConfigIT {
 
   private static FirebaseRemoteConfig remoteConfig;
 
-  private static final long timestamp = System.currentTimeMillis();
-
-  private static final Map<String, Parameter> PARAMETERS = ImmutableMap.of(
-          "welcome_message_text", new Parameter()
-                  .setDefaultValue(ParameterValue
-                          .of(String.format("welcome to app %s", timestamp)))
-                  .setConditionalValues(ImmutableMap.<String, ParameterValue>of(
-                          "ios_en",
-                          ParameterValue.of(String.format("welcome to app en %s", timestamp))
-                  ))
-                  .setDescription("text for welcome message!"),
-          "header_text", new Parameter()
-                  .setDefaultValue(ParameterValue.inAppDefault()));
-
-  private static final Map<String, ParameterGroup> PARAMETER_GROUPS = ImmutableMap.of(
-          "new menu", new ParameterGroup()
-                  .setDescription(String.format("New Menu %s", timestamp))
-                  .setParameters(ImmutableMap.of(
-                          "pumpkin_spice_season", new Parameter()
-                                  .setDefaultValue(ParameterValue.of("true"))
-                                  .setDescription("Whether it's currently pumpkin spice season."))
-                  ));
-
   private static final List<Condition> CONDITIONS = ImmutableList.of(
           new Condition("ios_en", "device.os == 'ios' && device.country in ['us', 'uk']")
                   .setTagColor(TagColor.INDIGO),
           new Condition("android_en",
                   "device.os == 'android' && device.country in ['us', 'uk']"));
 
-  private static final Version VERSION = Version
-          .withDescription(String.format("promo config %s", timestamp));
-
   @BeforeClass
   public static void setUpClass() {
     remoteConfig = FirebaseRemoteConfig.getInstance(IntegrationTestUtils.ensureDefaultApp());
   }
 
   @Test
-  public void testTemplateOperations() throws FirebaseRemoteConfigException {
-    // get template to fetch the active template with correct etag
-    final Template oldTemplate = remoteConfig.getTemplate();
-    final Template inputTemplate = Template.fromJSON(oldTemplate.toJSON());
-    final String versionNumber = oldTemplate.getVersion().getVersionNumber();
-
-    // modify template
-    inputTemplate.setParameters(PARAMETERS)
-            .setParameterGroups(PARAMETER_GROUPS)
+  public void testValidateTemplate() throws FirebaseRemoteConfigException {
+    final Template inputTemplate = remoteConfig.getTemplate();
+    final Map<String, Parameter> expectedParameters = getParameters();
+    final Map<String, ParameterGroup> expectedParameterGroups = getParameterGroups();
+    final Version expectedVersion = getVersion();
+    inputTemplate.setParameters(expectedParameters)
+            .setParameterGroups(expectedParameterGroups)
             .setConditions(CONDITIONS)
-            .setVersion(VERSION);
+            .setVersion(expectedVersion);
 
-    // validate template
     Template validatedTemplate = remoteConfig.validateTemplate(inputTemplate);
-    assertEquals(inputTemplate.getETag(), validatedTemplate.getETag());
-    assertEquals(PARAMETERS, validatedTemplate.getParameters());
-    assertEquals(PARAMETER_GROUPS, validatedTemplate.getParameterGroups());
-    assertEquals(CONDITIONS, validatedTemplate.getConditions());
-    assertEquals(VERSION, validatedTemplate.getVersion());
+
+    assertEquals(inputTemplate, validatedTemplate);
+  }
+
+  @Test
+  public void testPublishTemplate() throws FirebaseRemoteConfigException {
+    // get template to fetch the active template with correct etag
+    final Template inputTemplate = remoteConfig.getTemplate();
+    final Map<String, Parameter> parameters = getParameters();
+    final Map<String, ParameterGroup> parameterGroups = getParameterGroups();
+    final Version version = getVersion();
+    // modify template
+    inputTemplate.setParameters(parameters)
+            .setParameterGroups(parameterGroups)
+            .setConditions(CONDITIONS)
+            .setVersion(version);
 
     // publish template
     Template publishedTemplate = remoteConfig.publishTemplate(inputTemplate);
+
     assertNotEquals(inputTemplate.getETag(), publishedTemplate.getETag());
-    assertEquals(PARAMETERS, publishedTemplate.getParameters());
-    assertEquals(PARAMETER_GROUPS, publishedTemplate.getParameterGroups());
+    assertEquals(parameters, publishedTemplate.getParameters());
+    assertEquals(parameterGroups, publishedTemplate.getParameterGroups());
     assertEquals(CONDITIONS, publishedTemplate.getConditions());
-    assertNotEquals(VERSION, publishedTemplate.getVersion());
+    assertEquals(version.getDescription(), publishedTemplate.getVersion().getDescription());
+    assertNotEquals(version, publishedTemplate.getVersion());
+  }
+
+  @Test
+  public void testGetTemplate() throws FirebaseRemoteConfigException {
+    // get template to fetch the active template with correct etag
+    // modify and publish a known template to test get template operation.
+    final Template inputTemplate = remoteConfig.getTemplate();
+    final Map<String, Parameter> parameters = getParameters();
+    final Map<String, ParameterGroup> parameterGroups = getParameterGroups();
+    final Version version = getVersion();
+    inputTemplate.setParameters(parameters)
+            .setParameterGroups(parameterGroups)
+            .setConditions(CONDITIONS)
+            .setVersion(version);
+    // publish a known template
+    Template publishedTemplate = remoteConfig.publishTemplate(inputTemplate);
 
     // get template
     Template currentTemplate = remoteConfig.getTemplate();
+
     assertEquals(publishedTemplate, currentTemplate);
+  }
+
+  @Test
+  public void testGetTemplateAtVersion() throws FirebaseRemoteConfigException {
+    // get template to fetch the active template with correct etag
+    // store the template version number
+    // publish a new template to test get template at version operation.
+    final Template previousTemplate = remoteConfig.getTemplate();
+    final String versionNumber = previousTemplate.getVersion().getVersionNumber();
+    final Template inputTemplate = new Template(previousTemplate.getETag());
+    final Map<String, Parameter> parameters = getParameters();
+    final Map<String, ParameterGroup> parameterGroups = getParameterGroups();
+    final Version version = getVersion();
+    inputTemplate.setParameters(parameters)
+            .setParameterGroups(parameterGroups)
+            .setConditions(CONDITIONS)
+            .setVersion(version);
+    // publish a new template
+    Template publishedTemplate = remoteConfig.publishTemplate(inputTemplate);
 
     // get template at version
     Template atVersionTemplate = remoteConfig.getTemplateAtVersion(versionNumber);
-    assertEquals(oldTemplate, atVersionTemplate);
+
+    assertEquals(previousTemplate, atVersionTemplate);
     assertEquals(versionNumber, atVersionTemplate.getVersion().getVersionNumber());
+    assertNotEquals(publishedTemplate, atVersionTemplate);
+  }
+
+  @Test
+  public void testRollbackTemplate() throws FirebaseRemoteConfigException {
+    // get template to fetch the active template with correct etag.
+    // store the template version number to rollback.
+    final Template inputTemplate = remoteConfig.getTemplate();
+    final String versionNumber = inputTemplate.getVersion().getVersionNumber();
+    final Map<String, Parameter> parameters = getParameters();
+    final Map<String, ParameterGroup> parameterGroups = getParameterGroups();
+    final Version version = getVersion();
+    inputTemplate.setParameters(parameters)
+            .setParameterGroups(parameterGroups)
+            .setConditions(CONDITIONS)
+            .setVersion(version);
+    // publish a new template before rolling back to versionNumber
+    Template publishedTemplate = remoteConfig.publishTemplate(inputTemplate);
 
     // rollback template
     Template rolledBackTemplate = remoteConfig.rollback(versionNumber);
+
     assertEquals(String.format("Rollback to version %s", versionNumber),
             rolledBackTemplate.getVersion().getDescription());
 
     // get template to verify rollback
     Template activeTemplate = remoteConfig.getTemplate();
+
     assertEquals(rolledBackTemplate, activeTemplate);
+    assertNotEquals(publishedTemplate, activeTemplate);
   }
 
   @Test
@@ -196,4 +238,36 @@ public class FirebaseRemoteConfigIT {
     assertEquals(versions.size(), collected.get());
     assertNull(error.get());
   }
+
+  private Map<String, Parameter> getParameters() {
+    final long timestamp = System.currentTimeMillis();
+    return ImmutableMap.of(
+            "welcome_message_text", new Parameter()
+                    .setDefaultValue(ParameterValue
+                            .of(String.format("welcome to app %s", timestamp)))
+                    .setConditionalValues(ImmutableMap.<String, ParameterValue>of(
+                            "ios_en",
+                            ParameterValue.of(String.format("welcome to app en %s", timestamp))
+                    ))
+                    .setDescription("text for welcome message!"),
+            "header_text", new Parameter()
+                    .setDefaultValue(ParameterValue.inAppDefault()));
+  }
+
+  private Map<String, ParameterGroup> getParameterGroups() {
+    final long timestamp = System.currentTimeMillis();
+    return ImmutableMap.of(
+            "new menu", new ParameterGroup()
+                    .setDescription(String.format("New Menu %s", timestamp))
+                    .setParameters(ImmutableMap.of(
+                            "pumpkin_spice_season", new Parameter()
+                                    .setDefaultValue(ParameterValue.of("true"))
+                                    .setDescription("Whether it's currently pumpkin spice season."))
+                    ));
+  }
+
+  private Version getVersion() {
+    final long timestamp = System.currentTimeMillis();
+    return Version.withDescription(String.format("promo config %s", timestamp));
+  }
 }