firebase / firebase-android-sdk

Firebase Android SDK
https://firebase.google.com
Apache License 2.0
2.28k stars 580 forks source link

FR: Support Custom Class Mapper annotations on Kotlin properties #252

Closed lexicalninja closed 4 years ago

lexicalninja commented 5 years ago

I have a problem with the custom class mapper. I have a Profile class that contains an array of TrainingPlanProgress objects.

When I run the set function on the document with the current profile object, the custom object mapper runs and changes the UUID field on all the array items to lowercase uuid.

Profile class:

const val CLASS_NAME = "ProfileDataClass"

data class Profile(override var UUID: String = "",
                   var userUuid: String = "",
                   var powerZonesCache: MutableList<Int> = mutableListOf(),
                   var heartZonesCache: MutableList<Int> = mutableListOf(),
                   var pzpCache: MutableList<Double> = mutableListOf(),
                   val favoriteWorkouts: MutableList<String> = mutableListOf(),
                   var name: String = "Kinetic User",
                   var username: String = "",
                   var email: String = "",
                   var roles: MutableList<String>? = null,
                   var birthdate: Date = DateTime(1980, 1, 1, 0, 0).toDate(),
                   var weightKG: Double = 80.0,
                   var heightCM: Double = 175.0,
                   var powerFTP: Int = 150,
                   var heartMax: Int = 200,
                   var heartResting: Int = 60,
                   var totalDistanceKM: Double = 0.0,
                   var totalKilojoules: Double = 0.0,
                   var totalTime: Double = 0.0,
                   var customHuds: String? = null,
                   var devices: MutableList<Device>? = null,
                   var planProgressArray: MutableList<TrainingPlanProgress> = mutableListOf(),
                   @get:Exclude val PowerZoneDefaultCeilings: List<Double> = listOf(0.0, 0.55, 0.75, 0.90, 1.05, 1.20, 1.50),
                   @get:Exclude val HeartZoneDefaultCeilings: List<Double> = listOf(0.0, 0.60, 0.70, 0.77, 0.88))
    : Model(UUID) {

Model class:

abstract class Model(open var UUID: String) : Serializable {
    inline fun <reified T : Model> withId(id: String): T {
        this.UUID = id
        return this as T
    }
}

TrainingPlanProgress class:

data class TrainingPlanProgress(override var UUID: String = "",
                           var trainingPlan: String = "",
                           var startDate: Date = Date(),
                           var finishDate: Date? = null
) : Model(UUID) {
    fun toMap(): Map<String, Any?> {
        val map = mutableMapOf<String, Any?>()
        map["UUID"] = this.UUID
        map["trainingPlan"] = this.trainingPlan
        map["startDate"] = this.startDate
        map["finishDate"] = this.finishDate
        return map
    }
}

I have traced this all the way through the sdk to the CustomClassMapper class to this function

At the point where the array of TrainingPlanProgress is returning results from serialization this is what the debugger shows: screen shot 2019-02-22 at 2 26 12 pm

you can see o and list still have UUID in the members, but the result being returned has changed it all to lowercase.

When this gets to firestore it changes the objects then the next sync of the profile loses sight of the TrainingPlanProgress items because it can't map by key to the UUID key because it is no longer all upper case.

Here is what the class decompiles to in Android Studio.

private static <T> Object serialize(T o, CustomClassMapper.ErrorPath path) {
        if (path.getLength() > 500) {
            throw serializeError(path, "Exceeded maximum depth of 500, which likely indicates there's an object cycle");
        } else if (o == null) {
            return null;
        } else if (o instanceof Number) {
            if (o instanceof Float) {
                return ((Float)o).doubleValue();
            } else if (o instanceof Short) {
                throw serializeError(path, "Shorts are not supported, please use int or long");
            } else if (o instanceof Byte) {
                throw serializeError(path, "Bytes are not supported, please use int or long");
            } else {
                return o;
            }
        } else if (o instanceof String) {
            return o;
        } else if (o instanceof Boolean) {
            return o;
        } else if (o instanceof Character) {
            throw serializeError(path, "Characters are not supported, please use Strings.");
        } else if (o instanceof Map) {
            Map<String, Object> result = new HashMap();
            Iterator var10 = ((Map)o).entrySet().iterator();

            while(var10.hasNext()) {
                Entry<Object, Object> entry = (Entry)var10.next();
                Object key = entry.getKey();
                if (!(key instanceof String)) {
                    throw serializeError(path, "Maps with non-string keys are not supported");
                }

                String keyString = (String)key;
                result.put(keyString, serialize(entry.getValue(), path.child(keyString)));
            }

            return result;
        } else if (!(o instanceof Collection)) {
            if (o.getClass().isArray()) {
                throw serializeError(path, "Serializing Arrays is not supported, please use Lists instead");
            } else if (o instanceof Enum) {
                return ((Enum)o).name();
            } else if (!(o instanceof Date) && !(o instanceof Timestamp) && !(o instanceof GeoPoint) && !(o instanceof Blob) && !(o instanceof DocumentReference) && !(o instanceof FieldValue)) {
                Class<T> clazz = o.getClass();
                CustomClassMapper.BeanMapper<T> mapper = loadOrCreateBeanMapperForClass(clazz);
                return mapper.serialize(o, path);
            } else {
                return o;
            }
        } else if (!(o instanceof List)) {
            throw serializeError(path, "Serializing Collections is not supported, please use Lists instead");
        } else {
            List<Object> list = (List)o;
            List<Object> result = new ArrayList(list.size());

            for(int i = 0; i < list.size(); ++i) {
                result.add(serialize(list.get(i), path.child("[" + i + "]")));
            }

            return result;
        }
    }

my break point was on the last case return result.

Is there a way around this that doesn't require changing all the UUID to uuid in the database?

Please let me know if you need any more information.

wilhuff commented 5 years ago

Use the @PropertyName annotation to force the name to be whatever you like if our default naming rules don't work for you.

Firestore maps names as follows:

The underlying issue here is that Kotlin properties end up generating both backing fields, getters, and setters (for var) so the getter rule is being applied.

Something like this would do what you're looking for:

@get:PropertyName("UUID") override var UUID: String = ""
lexicalninja commented 5 years ago

I added it as you suggested

data class TrainingPlanProgress(@get:PropertyName("UUID") override var UUID: String = "",
                                var trainingPlan: String = "",
                                var startDate: Date = Date(),
                                var finishDate: Date? = null
) : Model(UUID) {
    fun toMap(): Map<String, Any?> {
        val map = mutableMapOf<String, Any?>()
        map["UUID"] = this.UUID
        map["trainingPlan"] = this.trainingPlan
        map["startDate"] = this.startDate
        map["finishDate"] = this.finishDate
        return map
    }
}

I receive the following exception:

E/AndroidRuntime: FATAL EXCEPTION: main
    Process: com.kinetic.fit, PID: 16866
    java.lang.RuntimeException: Found setter on com.kinetic.fit.data.firestore.Model with invalid case-sensitive name: setUUID
        at com.google.firebase.firestore.util.CustomClassMapper$BeanMapper.<init>(com.google.firebase:firebase-firestore@@18.0.1:607)
        at com.google.firebase.firestore.util.CustomClassMapper.loadOrCreateBeanMapperForClass(com.google.firebase:firebase-firestore@@18.0.1:348)
        at com.google.firebase.firestore.util.CustomClassMapper.convertBean(com.google.firebase:firebase-firestore@@18.0.1:502)
        at com.google.firebase.firestore.util.CustomClassMapper.deserializeToClass(com.google.firebase:firebase-firestore@@18.0.1:243)
        at com.google.firebase.firestore.util.CustomClassMapper.deserializeToType(com.google.firebase:firebase-firestore@@18.0.1:181)
        at com.google.firebase.firestore.util.CustomClassMapper.deserializeToParameterizedType(com.google.firebase:firebase-firestore@@18.0.1:258)
        at com.google.firebase.firestore.util.CustomClassMapper.deserializeToType(com.google.firebase:firebase-firestore@@18.0.1:179)
        at com.google.firebase.firestore.util.CustomClassMapper.access$200(com.google.firebase:firebase-firestore@@18.0.1:53)
        at com.google.firebase.firestore.util.CustomClassMapper$BeanMapper.deserialize(com.google.firebase:firebase-firestore@@18.0.1:701)
        at com.google.firebase.firestore.util.CustomClassMapper$BeanMapper.deserialize(com.google.firebase:firebase-firestore@@18.0.1:675)
        at com.google.firebase.firestore.util.CustomClassMapper.convertBean(com.google.firebase:firebase-firestore@@18.0.1:504)
        at com.google.firebase.firestore.util.CustomClassMapper.deserializeToClass(com.google.firebase:firebase-firestore@@18.0.1:243)
        at com.google.firebase.firestore.util.CustomClassMapper.convertToCustomClass(com.google.firebase:firebase-firestore@@18.0.1:97)
        at com.google.firebase.firestore.DocumentSnapshot.toObject(com.google.firebase:firebase-firestore@@18.0.1:203)
        at com.google.firebase.firestore.DocumentSnapshot.toObject(com.google.firebase:firebase-firestore@@18.0.1:183)
        at com.kinetic.fit.data.firestore.KineticAccount$syncProfile$1.onComplete(KineticAccount.kt:144)
        at com.google.android.gms.tasks.zzj.run(Unknown Source:4)
        at android.os.Handler.handleCallback(Handler.java:873)
        at android.os.Handler.dispatchMessage(Handler.java:99)
        at android.os.Looper.loop(Looper.java:193)
        at android.app.ActivityThread.main(ActivityThread.java:6718)
        at java.lang.reflect.Method.invoke(Native Method)
        at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:493)
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:858)

Please let me know if I misunderstood the solution you proposed.

Thanks in advance for your time.

P

wilhuff commented 5 years ago

You didn't misunderstand, this just isn't working as expected. You might try applying the property name to the setter as well or other variations including just the blanket @PropertyName("UUID").

Another option is to rename the property in code, now that the external form is explicit:

@get:PropertyName("UUID") override var Uuid: String = ""

I'll have a look at why that failure is happening though.

wilhuff commented 5 years ago

I've confirmed that annotating the setter with @PropertyName("UUID") has the intended effect.

wilhuff commented 5 years ago

255 adds tests to verify this behavior. Omitting the @PropertyName("UUID") on the setter results in the error you saw, which essentially just says that there was a mismatch between the value as seen in Firestore vs the expected case of the setter name.

(To be clear: this is working as expected, it's just not obvious that annotating both is required.)

lexicalninja commented 5 years ago

I tried with all the combinations of @PropertyName("UUID") on getters, setters and blanket for both Model and TrainingPlanProgress classes.

It either works, but changes it to uuid or fails on the setter like above.

The test is checking against a plain static class and isn't testing against extended abstract class that Model is or whether it works when there is an array of the AllCapsClass on another class. Could these be part of the problem?

Any ideas or any clue whether this is from the Kotlin data class and the way it generates all of the getters and setters on the classes causing some conflict that overrides the @PropertyName decorator?

Thanks in advance.

wilhuff commented 5 years ago

Sorry for the delay in response here, but a bunch of internal investigation has led us to this conclusion: there is currently no way to create a Java annotation that will end up on Kotlin property getters/setters so the only available option is to modify our POJO mapper to take private field annotations into account. Unfortunately we haven't come up with any way to do that isn't a breaking change for existing users.

I'll recharacterize this as a feature request for supporting @PropertyName on Kotlin properties.

lexicalninja commented 5 years ago

Thanks, @wilhuff.

Anyone who reads this, the easiest way around at this time is to change the name to something that is not all caps. Hopefully this isn't too crazy of a refactor for you.