E57E Revert "Firestore/Database: Handle bridge methods while mapping (#5626)" · firebase/firebase-android-sdk@4b6ab73 · GitHub
[go: up one dir, main page]
Skip to content

Commit 4b6ab73

Browse files
cherylEnkiducherylEnkidu
authored
Revert "Firestore/Database: Handle bridge methods while mapping (#5626)"
This reverts commit 5ef5cdd.
1 parent 5125155 commit 4b6ab73

File tree

8 files changed

+29
-192
lines changed

8 files changed

+29
-192
lines changed

‎firebase-database/CHANGELOG.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
# Unreleased
2-
* [fixed] Fixed the `@Exclude` annotation doesn't been propagated to Kotlin's corresponding bridge methods. [#5626](//github.com/firebase/firebase-android-sdk/pull/5626)
2+
33

44
# 20.3.0
55
* [changed] Added Kotlin extensions (KTX) APIs from `com.google.firebase:firebase-database-ktx`

‎firebase-database/src/main/java/com/google/firebase/database/core/utilities/encoding/CustomClassMapper.java‎

Lines changed: 6 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -38,11 +38,9 @@
3838
import java.util.Collection;
3939
import java.util.Collections;
4040
import java.util.HashMap;
41-
import java.util.HashSet;
4241
import java.util.List;
4342
import java.util.Locale;
4443
import java.util.Map;
45-
import java.util.Set;
4644
import java.util.concurrent.ConcurrentHashMap;
4745
import java.util.concurrent.ConcurrentMap;
4846

@@ -496,8 +494,6 @@ public BeanMapper(Class<T> clazz) {
496494
// getMethods/getFields only returns public methods/fields we need to traverse the
497495
// class hierarchy to find the appropriate setter or field.
498496
Class<? super T> currentClass = clazz;
499-
Map<String, Method> bridgeMethods = new HashMap<>();
500-
Set<String> propertyNamesOfExcludedSetters = new HashSet<>();
501497
do {
502498
// Add any setters
503499
for (Method method : currentClass.getDeclaredMethods()) {
@@ -508,23 +504,12 @@ public BeanMapper(Class<T> clazz) {
508504
if (!existingPropertyName.equals(propertyName)) {
509505
throw new DatabaseException(
510506
"Found setter with invalid " + "case-sensitive name: " + method.getName());
511-
} else if (method.isBridge()) {
512-
// We ignore bridge setters when creating a bean, but include them in the map
513-
// for the purpose of the `isSetterOverride()` check
514-
bridgeMethods.put(propertyName, method);
515507
} else {
516508
Method existingSetter = setters.get(propertyName);
517-
Method correspondingBridgeMethod = bridgeMethods.get(propertyName);
518509
if (existingSetter == null) {
510+
method.setAccessible(true);
519511
setters.put(propertyName, method);
520-
if (!method.isAnnotationPresent(Exclude.class)) {
521-
method.setAccessible(true);
522-
} else {
523-
propertyNamesOfExcludedSetters.add(propertyName);
524-
}
525-
} else if (!isSetterOverride(method, existingSetter)
526-
&& !(correspondingBridgeMethod != null
527-
&& isSetterOverride(method, correspondingBridgeMethod))) {
512+
} else if (!isSetterOverride(method, existingSetter)) {
528513
// We require that setters with conflicting property names are
529514
// overrides from a base class
530515
throw new DatabaseException(
@@ -559,12 +544,6 @@ && isSetterOverride(method, correspondingBridgeMethod))) {
559544
currentClass = currentClass.getSuperclass();
560545
} while (currentClass != null && !currentClass.equals(Object.class));
561546

562-
// When subclass setter is annotated with `@Exclude`, the corresponding superclass setter
563-
// also need to be filtered out.
564-
for (String propertyName : propertyNamesOfExcludedSetters) {
565-
setters.remove(propertyName);
566-
}
567-
568547
if (properties.isEmpty()) {
569548
throw new DatabaseException("No properties to serialize found on class " + clazz.getName());
570549
}
@@ -724,10 +703,6 @@ private static boolean shouldIncludeGetter(Method method) {
724703
if (method.getParameterTypes().length != 0) {
725704
return false;
726705
}
727-
// Bridge methods
728-
if (method.isBridge()) {
729-
return false;
730-
}
731706
// Excluded methods
732707
if (method.isAnnotationPresent(Exclude.class)) {
733708
return false;
@@ -755,7 +730,10 @@ private static boolean shouldIncludeSetter(Method method) {
755730
if (method.getParameterTypes().length != 1) {
756731
return false;
757732
}
758-
733+
// Excluded methods
734+
if (method.isAnnotationPresent(Exclude.class)) {
735+
return false;
736+
}
759737
return true;
760738
}
761739

‎firebase-database/src/test/java/com/google/firebase/database/KotlinEntities.kt‎

Lines changed: 0 additions & 30 deletions
This file was deleted.

‎firebase-database/src/test/java/com/google/firebase/database/MapperTest.java‎

Lines changed: 4 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,6 @@
2020
import static org.junit.Assert.fail;
2121

2222
import androidx.annotation.Keep;
23-
import androidx.annotation.Nullable;
2423
import com.google.common.collect.ImmutableList;
2524
import com.google.common.collect.ImmutableMap;
2625
import com.google.firebase.database.core.utilities.encoding.CustomClassMapper;
@@ -850,22 +849,6 @@ public void setValue(String value) {
850849
}
851850
}
852851

853-
private static class GenericExcludedSetterBean implements GenericInterface<String> {
854-
private String value = null;
855-
856-
@Nullable
857-
@Override
858-
public String getValue() {
859-
return value;
860-
}
861-
862-
@Exclude
863-
@Override
864-
public void setValue(@Nullable String value) {
865-
this.value = "wrong setter";
866-
}
867-
}
868-
869852
private abstract static class GenericTypeIndicatorSubclass<T> extends GenericTypeIndicator<T> {}
870853

871854
private abstract static class NonGenericTypeIndicatorSubclass
@@ -2017,26 +2000,12 @@ public void genericSettersFromSubclassConflictsWithBaseClass() {
20172000
serialize(bean);
20182001
}
20192002

2020-
@Test
2021-
public void settersCanOverrideGenericSettersParsing() {
2003+
// This should work, but generics and subclassing are tricky to get right. For now we will just
2004+
// throw and we can add support for generics & subclassing if it becomes a high demand feature
2005+
@Test(expected = DatabaseException.class)
2006+
public void settersCanOverrideGenericSettersParsingNot() {
20222007
NonConflictingGenericSetterSubBean bean =
20232008
deserialize("{'value': 'value'}", NonConflictingGenericSetterSubBean.class);
20242009
assertEquals("subsetter:value", bean.value);
20252010
}
2026-
2027-
@Test
2028-
public void excludedOverriddenGenericSetterSetsValueNotJava() {
2029-
GenericExcludedSetterBean bean =
2030-
deserialize("{'value': 'foo'}", GenericExcludedSetterBean.class);
2031-
assertEquals("foo", bean.value);
2032-
}
2033-
2034-
// Unlike Java, in Kotlin, annotations do not get propagated to bridge methods.
2035-
// That's why there are 2 separate tests for Java and Kotlin
2036-
@Test
2037-
public void excludedOverriddenGenericSetterSetsValueNotKotlin() {
2038-
GenericExcludedSetterBeanKotlin bean =
2039-
deserialize("{'value': 'foo'}", GenericExcludedSetterBeanKotlin.class);
2040-
assertEquals("foo", bean.getValue());
2041-
}
20422011
}

‎firebase-firestore/CHANGELOG.md‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@
33

44
# 24.10.2
55
* [changed] Internal test improvements.
6-
* [fixed] Fixed the `@Exclude` annotation doesn't been propagated to Kotlin's corresponding bridge methods. [#5626](//github.com/firebase/firebase-android-sdk/pull/5626)
76

87

98
## Kotlin
@@ -890,4 +889,3 @@ updates.
890889
or
891890
[`FieldValue.serverTimestamp()`](/docs/reference/android/com/google/firebase/firestore/FieldValue.html#serverTimestamp())
892891
values.
893-

‎firebase-firestore/src/main/java/com/google/firebase/firestore/util/CustomClassMapper.java‎

Lines changed: 7 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,6 @@
5050
import java.util.List;
5151
import java.util.Locale;
5252
import java.util.Map;
53-
import java.util.Set;
5453
import java.util.concurrent.ConcurrentHashMap;
5554
import java.util.concurrent.ConcurrentMap;
5655

@@ -649,8 +648,6 @@ private static class BeanMapper<T> {
649648
// getMethods/getFields only returns public methods/fields we need to traverse the
650649
// class hierarchy to find the appropriate setter or field.
651650
Class<? super T> currentClass = clazz;
652-
Map<String, Method> bridgeMethods = new HashMap<>();
653-
Set<String> propertyNamesOfExcludedSetters = new HashSet<>();
654651
do {
655652
// Add any setters
656653
for (Method method : currentClass.getDeclaredMethods()) {
@@ -664,23 +661,13 @@ private static class BeanMapper<T> {
664661
+ currentClass.getName()
665662
+ " with invalid case-sensitive name: "
666663
E746 + method.getName());
667-
} else if (method.isBridge()) {
668-
// We ignore bridge setters when creating a bean, but include them in the map
669-
// for the purpose of the `isSetterOverride()` check
670-
bridgeMethods.put(propertyName, method);
671664
} else {
672665
Method existingSetter = setters.get(propertyName);
673-
Method correspondingBridgeMethod = bridgeMethods.get(propertyName);
674666
if (existingSetter == null) {
667+
method.setAccessible(true);
675668
setters.put(propertyName, method);
676-
if (!method.isAnnotationPresent(Exclude.class)) {
677-
method.setAccessible(true);
678-
} else {
679-
propertyNamesOfExcludedSetters.add(propertyName);
680-
}
681-
} else if (!isSetterOverride(method, existingSetter)
682-
&& !(correspondingBridgeMethod != null
683-
&& isSetterOverride(method, correspondingBridgeMethod))) {
669+
applySetterAnnotations(method);
670+
} else if (!isSetterOverride(method, existingSetter)) {
684671
// We require that setters with conflicting property names are
685672
// overrides from a base class
686673
if (currentClass == clazz) {
@@ -725,12 +712,6 @@ && isSetterOverride(method, correspondingBridgeMethod))) {
725712
currentClass = currentClass.getSuperclass();
726713
} while (currentClass != null && !currentClass.equals(Object.class));
727714

728-
// When subclass setter is annotated with `@Exclude`, the corresponding superclass setter
729-
// also need to be filtered out.
730-
for (String propertyName : propertyNamesOfExcludedSetters) {
731-
setters.remove(propertyName);
732-
}
733-
734715
if (properties.isEmpty()) {
735716
throw new RuntimeException("No properties to serialize found on class " + clazz.getName());
736717
}
@@ -1022,10 +1003,6 @@ private static boolean shouldIncludeGetter(Method method) {
10221003
if (method.getParameterTypes().length != 0) {
10231004
return false;
10241005
}
1025-
// Bridge methods
1026-
if (method.isBridge()) {
1027-
return false;
1028-
}
10291006
// Excluded methods
10301007
if (method.isAnnotationPresent(Exclude.class)) {
10311008
return false;
@@ -1053,7 +1030,10 @@ private static boolean shouldIncludeSetter(Method method) {
10531030
if (method.getParameterTypes().length != 1) {
10541031
return false;
10551032
}
1056-
1033+
// Excluded methods
1034+
if (method.isAnnotationPresent(Exclude.class)) {
1035+
return false;
1036+
}
10571037
return true;
10581038
}
10591039

‎firebase-firestore/src/test/java/com/google/firebase/firestore/util/KotlinEntities.kt‎

Lines changed: 0 additions & 32 deletions
This file was deleted.

‎firebase-firestore/src/test/java/com/google/firebase/firestore/util/MapperTest.java‎

Lines changed: 11 additions & 37 deletions
-
@Override
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@
2121
import static org.junit.Assert.assertNull;
2222
import static org.junit.Assert.fail;
2323

24-
import androidx.annotation.Nullable;
2524
import com.google.firebase.firestore.DocumentId;
2625
import com.google.firebase.firestore.DocumentReference;
2726
import com.google.firebase.firestore.Exclude;
@@ -908,22 +907,6 @@ public void setValue(String value) {
908907
}
909908
}
910909

911-
private static class GenericExcludedSetterBean implements GenericInterface<String> {
912-
private String value = null;
913-
914-
@Nullable
915-
@Override
916-
public String getValue() {
917-
return value;
918-
}
919-
920-
@Exclude
921
922-
public void setValue(@Nullable String value) {
923-
this.value = "wrong setter";
924-
}
925-
}
926-
927910
private static <T> T deserialize(String jsonString, Class<T> clazz) {
928911
return deserialize(jsonString, clazz, /*docRef=*/ null);
929912
}
@@ -2235,27 +2218,18 @@ public void genericSettersFromSubclassConflictsWithBaseClass() {
22352218
() -> serialize(bean));
22362219
}
22372220

2221+
// This should work, but generics and subclassing are tricky to get right. For now we will just
2222+
// throw and we can add support for generics & subclassing if it becomes a high demand feature
22382223
@Test
2239-
public void settersCanOverrideGenericSettersParsing() {
2240-
NonConflictingGenericSetterSubBean bean =
2241-
deserialize("{'value': 'value'}", NonConflictingGenericSetterSubBean.class);
2242-
assertEquals("subsetter:value", bean.value);
2243-
}
2244-
2245-
@Test
2246-
public void excludedOverriddenGenericSetterSetsValueNotJava() {
2247-
GenericExcludedSetterBean bean =
2248-
deserialize("{'value': 'foo'}", GenericExcludedSetterBean.class);
2249-
assertEquals("foo", bean.value);
2250-
}
2251-
2252-
// Unlike Java, in Kotlin, annotations do not get propagated to bridge methods.
2253-
// That's why there are 2 separate tests for Java and Kotlin
2254-
@Test
2255-
public void excludedOverriddenGenericSetterSetsValueNotKotlin() {
2256-
GenericExcludedSetterBeanKotlin bean =
2257-
deserialize("{'value': 'foo'}", GenericExcludedSetterBeanKotlin.class);
2258-
assertEquals("foo", bean.getValue());
2224+
public void settersCanOverrideGenericSettersParsingNot() {
2225+
assertExceptionContains(
2226+
"Class com.google.firebase.firestore.util.MapperTest$NonConflictingGenericSetterSubBean "
2227+
+ "has multiple setter overloads",
2228+
() -> {
2229+
NonConflictingGenericSetterSubBean bean =
2230+
deserialize("{'value': 'value'}", NonConflictingGenericSetterSubBean.class);
2231+
assertEquals("subsetter:value", bean.value);
2232+
});
22592233
}
22602234

22612235
@Test

0 commit comments

Comments
 (0)
0