Revert "Handle bridge methods while mapping" (#5704)

This PR breaks some tests, need to be re-evaluated before merging in.
firebase · Feb 12, 2024 · a4ca0cb · a4ca0cb
1 parent 5125155
commit a4ca0cb
Show file tree

Hide file tree

Showing 8 changed files with 29 additions and 192 deletions.
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,11 +38,9 @@
 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;
 
@@ -496,8 +494,6 @@ 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()) {
@@ -508,23 +504,12 @@ 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);
-                  if (!method.isAnnotationPresent(Exclude.class)) {
-                    method.setAccessible(true);
-                  } else {
-                    propertyNamesOfExcludedSetters.add(propertyName);
-                  }
-                } else if (!isSetterOverride(method, existingSetter)
-                    && !(correspondingBridgeMethod != null
-                        && isSetterOverride(method, correspondingBridgeMethod))) {
+                } else if (!isSetterOverride(method, existingSetter)) {
                   // We require that setters with conflicting property names are
                   // overrides from a base class
                   throw new DatabaseException(
@@ -559,12 +544,6 @@ && isSetterOverride(method, correspondingBridgeMethod))) {
         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());
       }
@@ -724,10 +703,6 @@ 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;
@@ -755,7 +730,10 @@ 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
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,7 +20,6 @@
 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;
@@ -850,22 +849,6 @@ 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
@@ -2017,26 +2000,12 @@ public void genericSettersFromSubclassConflictsWithBaseClass() {
     serialize(bean);
   }
 
-  @Test
-  public void settersCanOverrideGenericSettersParsing() {
+  // 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() {
     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
@@ -3,7 +3,6 @@
 
 # 24.10.2
 * [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)
 
 
 ## Kotlin
@@ -890,4 +889,3 @@ 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,7 +50,6 @@
 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;
 
@@ -649,8 +648,6 @@ 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()) {
@@ -664,23 +661,13 @@ 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);
-                  if (!method.isAnnotationPresent(Exclude.class)) {
-                    method.setAccessible(true);
-                  } else {
-                    propertyNamesOfExcludedSetters.add(propertyName);
-                  }
-                } else if (!isSetterOverride(method, existingSetter)
-                    && !(correspondingBridgeMethod != null
-                        && isSetterOverride(method, correspondingBridgeMethod))) {
+                  applySetterAnnotations(method);
+                } else if (!isSetterOverride(method, existingSetter)) {
                   // We require that setters with conflicting property names are
                   // overrides from a base class
                   if (currentClass == clazz) {
@@ -725,12 +712,6 @@ && isSetterOverride(method, correspondingBridgeMethod))) {
         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());
       }
@@ -1022,10 +1003,6 @@ 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;
@@ -1053,7 +1030,10 @@ 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
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,7 +21,6 @@
 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;
@@ -908,22 +907,6 @@ 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);
   }
@@ -2235,27 +2218,18 @@ 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 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());
+  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);
+        });
   }
 
   @Test