firebase · cherylEnkidu · Jan 31, 2024 · Jan 4, 2024 · Jan 9, 2024 · Jan 6, 2024
diff --git a/firebase-database/CHANGELOG.md b/firebase-database/CHANGELOG.md
@@ -1,5 +1,5 @@
 # Unreleased
-
+* [fixed] Fixed the `@Exclude` annotation doesn't been propagated to Kotlin's corresponding bridge methods. [#5626](//github.com/firebase/firebase-android-sdk/pull/5626)
 
 # 20.3.0
 * [changed] Added Kotlin extensions (KTX) APIs from `com.google.firebase:firebase-database-ktx`

diff --git a/...src/main/java/com/google/firebase/database/core/utilities/encoding/CustomClassMapper.java b/...src/main/java/com/google/firebase/database/core/utilities/encoding/CustomClassMapper.java
@@ -38,9 +38,11 @@
 import java.util.Collection;
 import java.util.Collections;
 import java.util.HashMap;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Locale;
 import java.util.Map;
+import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ConcurrentMap;
 
@@ -494,6 +496,8 @@ public BeanMapper(Class<T> clazz) {
       // getMethods/getFields only returns public methods/fields we need to traverse the
       // class hierarchy to find the appropriate setter or field.
       Class<? super T> currentClass = clazz;
+      Map<String, Method> bridgeMethods = new HashMap<>();
+      Set<String> propertyNamesOfExcludedSetters = new HashSet<>();
       do {
         // Add any setters
         for (Method method : currentClass.getDeclaredMethods()) {
@@ -504,12 +508,23 @@ public BeanMapper(Class<T> clazz) {
               if (!existingPropertyName.equals(propertyName)) {
                 throw new DatabaseException(
                     "Found setter with invalid " + "case-sensitive name: " + method.getName());
+              } else if (method.isBridge()) {
+                // We ignore bridge setters when creating a bean, but include them in the map
+                // for the purpose of the `isSetterOverride()` check
+                bridgeMethods.put(propertyName, method);
               } else {
                 Method existingSetter = setters.get(propertyName);
+                Method correspondingBridgeMethod = bridgeMethods.get(propertyName);
                 if (existingSetter == null) {
-                  method.setAccessible(true);
                   setters.put(propertyName, method);
-                } else if (!isSetterOverride(method, existingSetter)) {
+                  if (!method.isAnnotationPresent(Exclude.class)) {
+                    method.setAccessible(true);
+                  } else {
+                    propertyNamesOfExcludedSetters.add(propertyName);
+                  }
+                } else if (!isSetterOverride(method, existingSetter)
+                    && !(correspondingBridgeMethod != null
+                        && isSetterOverride(method, correspondingBridgeMethod))) {
                   // We require that setters with conflicting property names are
                   // overrides from a base class
                   throw new DatabaseException(
@@ -544,6 +559,12 @@ public BeanMapper(Class<T> clazz) {
         currentClass = currentClass.getSuperclass();
       } while (currentClass != null && !currentClass.equals(Object.class));
 
+      // When subclass setter is annotated with `@Exclude`, the corresponding superclass setter
+      // also need to be filtered out.
+      for (String propertyName : propertyNamesOfExcludedSetters) {
+        setters.remove(propertyName);
+      }
+
       if (properties.isEmpty()) {
         throw new DatabaseException("No properties to serialize found on class " + clazz.getName());
       }
@@ -703,6 +724,10 @@ private static boolean shouldIncludeGetter(Method method) {
       if (method.getParameterTypes().length != 0) {
         return false;
       }
+      // Bridge methods
+      if (method.isBridge()) {
+        return false;
+      }
       // Excluded methods
       if (method.isAnnotationPresent(Exclude.class)) {
         return false;
@@ -730,10 +755,7 @@ private static boolean shouldIncludeSetter(Method method) {
       if (method.getParameterTypes().length != 1) {
         return false;
       }
-      // Excluded methods
-      if (method.isAnnotationPresent(Exclude.class)) {
-        return false;
-      }
+
       return true;
     }
 

diff --git a/firebase-database/src/test/java/com/google/firebase/database/KotlinEntities.kt b/firebase-database/src/test/java/com/google/firebase/database/KotlinEntities.kt
@@ -0,0 +1,30 @@
+/*
+ * Copyright 2024 Google LLC
+ *
+ * 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.database
+
+interface GenericInterface<T> {
+  var value: T?
+}
+
+class GenericExcludedSetterBeanKotlin : GenericInterface<String> {
+
+  @set:Exclude
+  override var value: String? = null
+    set(value) {
+      field = "wrong_setter"
+    }
+}
diff --git a/firebase-database/src/test/java/com/google/firebase/database/MapperTest.java b/firebase-database/src/test/java/com/google/firebase/database/MapperTest.java
@@ -20,6 +20,7 @@
 import static org.junit.Assert.fail;
 
 import androidx.annotation.Keep;
+import androidx.annotation.Nullable;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.firebase.database.core.utilities.encoding.CustomClassMapper;
@@ -849,6 +850,22 @@ public void setValue(String value) {
     }
   }
 
+  private static class GenericExcludedSetterBean implements GenericInterface<String> {
+    private String value = null;
+
+    @Nullable
+    @Override
+    public String getValue() {
+      return value;
+    }
+
+    @Exclude
+    @Override
+    public void setValue(@Nullable String value) {
+      this.value = "wrong setter";
+    }
+  }
+
   private abstract static class GenericTypeIndicatorSubclass<T> extends GenericTypeIndicator<T> {}
 
   private abstract static class NonGenericTypeIndicatorSubclass
@@ -2000,12 +2017,26 @@ public void genericSettersFromSubclassConflictsWithBaseClass() {
     serialize(bean);
   }
 
-  // This should work, but generics and subclassing are tricky to get right. For now we will just
-  // throw and we can add support for generics & subclassing if it becomes a high demand feature
-  @Test(expected = DatabaseException.class)
-  public void settersCanOverrideGenericSettersParsingNot() {
+  @Test
+  public void settersCanOverrideGenericSettersParsing() {
     NonConflictingGenericSetterSubBean bean =
         deserialize("{'value': 'value'}", NonConflictingGenericSetterSubBean.class);
     assertEquals("subsetter:value", bean.value);
   }
+
+  @Test
+  public void excludedOverriddenGenericSetterSetsValueNotJava() {
+    GenericExcludedSetterBean bean =
+        deserialize("{'value': 'foo'}", GenericExcludedSetterBean.class);
+    assertEquals("foo", bean.value);
+  }
+
+  // Unlike Java, in Kotlin, annotations do not get propagated to bridge methods.
+  // That's why there are 2 separate tests for Java and Kotlin
+  @Test
+  public void excludedOverriddenGenericSetterSetsValueNotKotlin() {
+    GenericExcludedSetterBeanKotlin bean =
+        deserialize("{'value': 'foo'}", GenericExcludedSetterBeanKotlin.class);
+    assertEquals("foo", bean.getValue());
+  }
 }
diff --git a/firebase-firestore/CHANGELOG.md b/firebase-firestore/CHANGELOG.md
@@ -1,5 +1,6 @@
 # Unreleased
 * [changed] Internal test improvements.
+* [fixed] Fixed the `@Exclude` annotation doesn't been propagated to Kotlin's corresponding bridge methods. [#5626](//github.com/firebase/firebase-android-sdk/pull/5626)
 
 # 24.10.1
 * [fixed] Fixed an issue caused by calling mutation on immutable map object. [#5573](//github.com/firebase/firebase-android-sdk/pull/5573)
@@ -880,3 +881,4 @@ updates.
   or
   [`FieldValue.serverTimestamp()`](/docs/reference/android/com/google/firebase/firestore/FieldValue.html#serverTimestamp())
   values.
+
diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/util/CustomClassMapper.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/util/CustomClassMapper.java
@@ -50,6 +50,7 @@
 import java.util.List;
 import java.util.Locale;
 import java.util.Map;
+import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ConcurrentMap;
 
@@ -648,6 +649,8 @@ private static class BeanMapper<T> {
       // getMethods/getFields only returns public methods/fields we need to traverse the
       // class hierarchy to find the appropriate setter or field.
       Class<? super T> currentClass = clazz;
+      Map<String, Method> bridgeMethods = new HashMap<>();
+      Set<String> propertyNamesOfExcludedSetters = new HashSet<>();
       do {
         // Add any setters
         for (Method method : currentClass.getDeclaredMethods()) {
@@ -661,13 +664,23 @@ private static class BeanMapper<T> {
                         + currentClass.getName()
                         + " with invalid case-sensitive name: "
                         + method.getName());
+              } else if (method.isBridge()) {
+                // We ignore bridge setters when creating a bean, but include them in the map
+                // for the purpose of the `isSetterOverride()` check
+                bridgeMethods.put(propertyName, method);
               } else {
                 Method existingSetter = setters.get(propertyName);
+                Method correspondingBridgeMethod = bridgeMethods.get(propertyName);
                 if (existingSetter == null) {
-                  method.setAccessible(true);
                   setters.put(propertyName, method);
-                  applySetterAnnotations(method);
-                } else if (!isSetterOverride(method, existingSetter)) {
+                  if (!method.isAnnotationPresent(Exclude.class)) {
+                    method.setAccessible(true);
+                  } else {
+                    propertyNamesOfExcludedSetters.add(propertyName);
+                  }
+                } else if (!isSetterOverride(method, existingSetter)
+                    && !(correspondingBridgeMethod != null
+                        && isSetterOverride(method, correspondingBridgeMethod))) {
                   // We require that setters with conflicting property names are
                   // overrides from a base class
                   if (currentClass == clazz) {
@@ -712,6 +725,12 @@ private static class BeanMapper<T> {
         currentClass = currentClass.getSuperclass();
       } while (currentClass != null && !currentClass.equals(Object.class));
 
+      // When subclass setter is annotated with `@Exclude`, the corresponding superclass setter
+      // also need to be filtered out.
+      for (String propertyName : propertyNamesOfExcludedSetters) {
+        setters.remove(propertyName);
+      }
+
       if (properties.isEmpty()) {
         throw new RuntimeException("No properties to serialize found on class " + clazz.getName());
       }
@@ -1003,6 +1022,10 @@ private static boolean shouldIncludeGetter(Method method) {
       if (method.getParameterTypes().length != 0) {
         return false;
       }
+      // Bridge methods
+      if (method.isBridge()) {
+        return false;
+      }
       // Excluded methods
       if (method.isAnnotationPresent(Exclude.class)) {
         return false;
@@ -1030,10 +1053,7 @@ private static boolean shouldIncludeSetter(Method method) {
       if (method.getParameterTypes().length != 1) {
         return false;
       }
-      // Excluded methods
-      if (method.isAnnotationPresent(Exclude.class)) {
-        return false;
-      }
+
       return true;
     }
 

diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/util/KotlinEntities.kt b/firebase-firestore/src/test/java/com/google/firebase/firestore/util/KotlinEntities.kt
@@ -0,0 +1,32 @@
+/*
+ * Copyright 2024 Google LLC
+ *
+ * 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.firestore.util
+
+import com.google.firebase.firestore.Exclude
+
+interface GenericInterface<T> {
+  var value: T?
+}
+
+class GenericExcludedSetterBeanKotlin : GenericInterface<String> {
+
+  @set:Exclude
+  override var value: String? = null
+    set(value) {
+      field = "wrong_setter"
+    }
+}
diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/util/MapperTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/util/MapperTest.java
@@ -21,6 +21,7 @@
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.fail;
 
+import androidx.annotation.Nullable;
 import com.google.firebase.firestore.DocumentId;
 import com.google.firebase.firestore.DocumentReference;
 import com.google.firebase.firestore.Exclude;
@@ -907,6 +908,22 @@ public void setValue(String value) {
     }
   }
 
+  private static class GenericExcludedSetterBean implements GenericInterface<String> {
+    private String value = null;
+
+    @Nullable
+    @Override
+    public String getValue() {
+      return value;
+    }
+
+    @Exclude
+    @Override
+    public void setValue(@Nullable String value) {
+      this.value = "wrong setter";
+    }
+  }
+
   private static <T> T deserialize(String jsonString, Class<T> clazz) {
     return deserialize(jsonString, clazz, /*docRef=*/ null);
   }
@@ -2218,18 +2235,27 @@ public void genericSettersFromSubclassConflictsWithBaseClass() {
         () -> serialize(bean));
   }
 
-  // This should work, but generics and subclassing are tricky to get right. For now we will just
-  // throw and we can add support for generics & subclassing if it becomes a high demand feature
   @Test
-  public void settersCanOverrideGenericSettersParsingNot() {
-    assertExceptionContains(
-        "Class com.google.firebase.firestore.util.MapperTest$NonConflictingGenericSetterSubBean "
-            + "has multiple setter overloads",
-        () -> {
-          NonConflictingGenericSetterSubBean bean =
-              deserialize("{'value': 'value'}", NonConflictingGenericSetterSubBean.class);
-          assertEquals("subsetter:value", bean.value);
-        });
+  public void settersCanOverrideGenericSettersParsing() {
+    NonConflictingGenericSetterSubBean bean =
+        deserialize("{'value': 'value'}", NonConflictingGenericSetterSubBean.class);
+    assertEquals("subsetter:value", bean.value);
+  }
+
+  @Test
+  public void excludedOverriddenGenericSetterSetsValueNotJava() {
+    GenericExcludedSetterBean bean =
+        deserialize("{'value': 'foo'}", GenericExcludedSetterBean.class);
+    assertEquals("foo", bean.value);
+  }
+
+  // Unlike Java, in Kotlin, annotations do not get propagated to bridge methods.
+  // That's why there are 2 separate tests for Java and Kotlin
+  @Test
+  public void excludedOverriddenGenericSetterSetsValueNotKotlin() {
+    GenericExcludedSetterBeanKotlin bean =
+        deserialize("{'value': 'foo'}", GenericExcludedSetterBeanKotlin.class);
+    assertEquals("foo", bean.getValue());
   }
 
   @Test