From 94e4ae8d5bbc94a756e787a89c9a1906f9aca32b Mon Sep 17 00:00:00 2001 From: andreia Date: Tue, 4 Aug 2026 18:24:40 +0200 Subject: [PATCH 1/5] read LOI geometry straight from the firestore document instead of reflection-based protobuf mapping --- .../android/common/Constants.kt | 2 +- .../protobuf/FirestoreToProtobufExt.kt | 17 +- .../remote/firebase/schema/LoiConverter.kt | 8 +- .../firebase/schema/LoiGeometryConverter.kt | 89 ++++++++++ .../schema/LoiGeometryConverterTest.kt | 157 ++++++++++++++++++ 5 files changed, 266 insertions(+), 7 deletions(-) create mode 100644 app/src/main/java/org/groundplatform/android/data/remote/firebase/schema/LoiGeometryConverter.kt create mode 100644 app/src/test/java/org/groundplatform/android/data/remote/firebase/schema/LoiGeometryConverterTest.kt diff --git a/app/src/main/java/org/groundplatform/android/common/Constants.kt b/app/src/main/java/org/groundplatform/android/common/Constants.kt index d1e940e8eb..ccea7b0fd9 100644 --- a/app/src/main/java/org/groundplatform/android/common/Constants.kt +++ b/app/src/main/java/org/groundplatform/android/common/Constants.kt @@ -30,7 +30,7 @@ object Constants { const val DB_NAME = "ground.db" // Firebase Cloud Firestore settings. - const val FIRESTORE_LOGGING_ENABLED = true + val FIRESTORE_LOGGING_ENABLED = !isReleaseBuild() // Photos const val PHOTO_EXT = ".jpg" diff --git a/app/src/main/java/org/groundplatform/android/data/remote/firebase/protobuf/FirestoreToProtobufExt.kt b/app/src/main/java/org/groundplatform/android/data/remote/firebase/protobuf/FirestoreToProtobufExt.kt index 6ccfd2a310..1dd1e9dfd7 100644 --- a/app/src/main/java/org/groundplatform/android/data/remote/firebase/protobuf/FirestoreToProtobufExt.kt +++ b/app/src/main/java/org/groundplatform/android/data/remote/firebase/protobuf/FirestoreToProtobufExt.kt @@ -37,16 +37,27 @@ typealias MessageBuilder = GeneratedMessageLite.Builder<*, *> * This implementation is tightly bound to the implementation of code generated by * protobuf-kotlin-lite. Future versions of the library may require changes to this util. */ -@Suppress("UNCHECKED_CAST") fun KClass.parseFrom( documentSnapshot: DocumentSnapshot, idFieldNumber: MessageFieldNumber? = null, +): T = parseFrom(documentSnapshot.id, documentSnapshot.data, idFieldNumber) + +/** + * Returns a new instance of the specified [Message] populated with [documentId] and [data]. + * + * Allows callers to skip fields that require custom processing before mapping. + */ +@Suppress("UNCHECKED_CAST") +fun KClass.parseFrom( + documentId: String, + data: Map?, + idFieldNumber: MessageFieldNumber? = null, ): T { val builder = newBuilderForType() if (idFieldNumber != null) { - builder.setOrLog(getFieldName(idFieldNumber), documentSnapshot.id) + builder.setOrLog(getFieldName(idFieldNumber), documentId) } - documentSnapshot.data.copyInto(builder) + data.copyInto(builder) return builder.build() as T } diff --git a/app/src/main/java/org/groundplatform/android/data/remote/firebase/schema/LoiConverter.kt b/app/src/main/java/org/groundplatform/android/data/remote/firebase/schema/LoiConverter.kt index a742efa14c..49b4cbbc61 100644 --- a/app/src/main/java/org/groundplatform/android/data/remote/firebase/schema/LoiConverter.kt +++ b/app/src/main/java/org/groundplatform/android/data/remote/firebase/schema/LoiConverter.kt @@ -18,7 +18,6 @@ package org.groundplatform.android.data.remote.firebase.schema import com.google.firebase.firestore.DocumentSnapshot import org.groundplatform.android.data.remote.DataStoreException import org.groundplatform.android.data.remote.firebase.protobuf.parseFrom -import org.groundplatform.android.data.remote.firebase.schema.GeometryConverter.toGeometry import org.groundplatform.android.proto.LocationOfInterest as LocationOfInterestProto import org.groundplatform.android.proto.LocationOfInterest.Source import org.groundplatform.domain.model.Survey @@ -31,6 +30,8 @@ object LoiConverter { const val GEOMETRY_TYPE = "type" const val POLYGON_TYPE = "Polygon" + private val GEOMETRY_FIELD = LocationOfInterestProto.GEOMETRY_FIELD_NUMBER.toString() + fun toLoi(survey: Survey, doc: DocumentSnapshot): Result = runCatching { toLoiUnchecked(survey, doc) } @@ -39,8 +40,9 @@ object LoiConverter { private fun toLoiUnchecked(survey: Survey, doc: DocumentSnapshot): LocationOfInterest { if (!doc.exists()) throw DataStoreException("LOI missing") val loiId = doc.id - val loiProto = LocationOfInterestProto::class.parseFrom(doc, 1) - val geometry = loiProto.geometry.toGeometry() + val data = doc.data.orEmpty() + val geometry = LoiGeometryConverter.toGeometry(data[GEOMETRY_FIELD]) + val loiProto = LocationOfInterestProto::class.parseFrom(loiId, data - GEOMETRY_FIELD, 1) val jobId = loiProto.jobId val job = DataStoreException.checkNotNull(survey.getJob(jobId), "job $jobId") // Degrade gracefully when audit info missing in remote db. diff --git a/app/src/main/java/org/groundplatform/android/data/remote/firebase/schema/LoiGeometryConverter.kt b/app/src/main/java/org/groundplatform/android/data/remote/firebase/schema/LoiGeometryConverter.kt new file mode 100644 index 0000000000..16269651be --- /dev/null +++ b/app/src/main/java/org/groundplatform/android/data/remote/firebase/schema/LoiGeometryConverter.kt @@ -0,0 +1,89 @@ +/* + * Copyright 2026 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 + * + * https://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 org.groundplatform.android.data.remote.firebase.schema + +import org.groundplatform.android.data.remote.DataStoreException +import org.groundplatform.android.proto.Coordinates as CoordinatesProto +import org.groundplatform.android.proto.Geometry as GeometryProto +import org.groundplatform.android.proto.LinearRing as LinearRingProto +import org.groundplatform.android.proto.MultiPolygon as MultiPolygonProto +import org.groundplatform.android.proto.Point as PointProto +import org.groundplatform.android.proto.Polygon as PolygonProto +import org.groundplatform.domain.model.geometry.Coordinates +import org.groundplatform.domain.model.geometry.Geometry +import org.groundplatform.domain.model.geometry.LinearRing +import org.groundplatform.domain.model.geometry.MultiPolygon +import org.groundplatform.domain.model.geometry.Point +import org.groundplatform.domain.model.geometry.Polygon + +// Keys are proto field numbers, as stored in Firestore. Derived from the generated constants so +// they stay correct if the schema is renumbered. +private val POINT = GeometryProto.POINT_FIELD_NUMBER.toString() +private val POLYGON = GeometryProto.POLYGON_FIELD_NUMBER.toString() +private val MULTI_POLYGON = GeometryProto.MULTI_POLYGON_FIELD_NUMBER.toString() +private val LATITUDE = CoordinatesProto.LATITUDE_FIELD_NUMBER.toString() +private val LONGITUDE = CoordinatesProto.LONGITUDE_FIELD_NUMBER.toString() +private val POINT_COORDINATES = PointProto.COORDINATES_FIELD_NUMBER.toString() +private val RING_COORDINATES = LinearRingProto.COORDINATES_FIELD_NUMBER.toString() +private val SHELL = PolygonProto.SHELL_FIELD_NUMBER.toString() +private val HOLES = PolygonProto.HOLES_FIELD_NUMBER.toString() +private val POLYGONS = MultiPolygonProto.POLYGONS_FIELD_NUMBER.toString() + +/** + * Builds [Geometry] straight from the nested maps of a Firestore document. Direct Firestore + * geometry parsing avoids expensive reflection overhead. + */ +internal object LoiGeometryConverter { + + /** Converts the value of an LOI's geometry field. Throws [DataStoreException] if malformed. */ + fun toGeometry(value: Any?): Geometry { + val geometry = value.orThrow>() + val point = geometry[POINT] + val polygon = geometry[POLYGON] + val multiPolygon = geometry[MULTI_POLYGON] + return when { + point != null -> Point(point.orThrow>()[POINT_COORDINATES].toCoordinates()) + polygon != null -> polygon.toPolygon() + multiPolygon != null -> + MultiPolygon( + multiPolygon.orThrow>()[POLYGONS].orThrow>().map { it.toPolygon() } + ) + else -> throw DataStoreException("Unrecognized geometry type: ${geometry.keys}") + } + } + + private fun Any?.toPolygon(): Polygon { + val polygon = orThrow>() + return Polygon( + polygon[SHELL].toLinearRing(), + polygon[HOLES]?.orThrow>()?.map { it.toLinearRing() } ?: listOf(), + ) + } + + private fun Any?.toLinearRing() = + LinearRing(orThrow>()[RING_COORDINATES].orThrow>().map { it.toCoordinates() }) + + private fun Any?.toCoordinates(): Coordinates { + val coordinates = orThrow>() + return Coordinates( + coordinates[LATITUDE].orThrow().toDouble(), + coordinates[LONGITUDE].orThrow().toDouble(), + ) + } + + private inline fun Any?.orThrow(): T = + this as? T ?: throw DataStoreException("Expected ${T::class.simpleName} but got $this") +} diff --git a/app/src/test/java/org/groundplatform/android/data/remote/firebase/schema/LoiGeometryConverterTest.kt b/app/src/test/java/org/groundplatform/android/data/remote/firebase/schema/LoiGeometryConverterTest.kt new file mode 100644 index 0000000000..6f3a435b15 --- /dev/null +++ b/app/src/test/java/org/groundplatform/android/data/remote/firebase/schema/LoiGeometryConverterTest.kt @@ -0,0 +1,157 @@ +/* + * Copyright 2026 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 + * + * https://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 org.groundplatform.android.data.remote.firebase.schema + +import com.google.common.truth.Truth.assertThat +import kotlin.test.assertFailsWith +import org.groundplatform.android.data.remote.DataStoreException +import org.groundplatform.android.data.remote.firebase.schema.GeometryConverter.toGeometry +import org.groundplatform.android.proto.Coordinates as CoordinatesProto +import org.groundplatform.android.proto.Geometry as GeometryProto +import org.groundplatform.android.proto.LinearRing as LinearRingProto +import org.groundplatform.android.proto.MultiPolygon as MultiPolygonProto +import org.groundplatform.android.proto.Point as PointProto +import org.groundplatform.android.proto.Polygon as PolygonProto +import org.groundplatform.domain.model.geometry.Coordinates +import org.groundplatform.domain.model.geometry.LinearRing +import org.groundplatform.domain.model.geometry.MultiPolygon +import org.groundplatform.domain.model.geometry.Point +import org.groundplatform.domain.model.geometry.Polygon +import org.junit.Test + +class LoiGeometryConverterTest { + + @Test + fun `reads a point`() { + val geometry = LoiGeometryConverter.toGeometry(pointMap(1.0, 2.0)) + + assertThat(geometry).isEqualTo(Point(Coordinates(1.0, 2.0))) + } + + @Test + fun `reads a polygon`() { + val map = polygonGeometryMap(SHELL) + + val geometry = LoiGeometryConverter.toGeometry(map) + + assertThat(geometry).isEqualTo(Polygon(LinearRing(SHELL.map { Coordinates(it[0], it[1]) }))) + } + + @Test + fun `reads a polygon with holes`() { + val map = polygonGeometryMap(SHELL, HOLE) + + val geometry = LoiGeometryConverter.toGeometry(map) as Polygon + + assertThat(geometry.holes).hasSize(1) + assertThat(geometry.holes.first().coordinates).isEqualTo(HOLE.map { Coordinates(it[0], it[1]) }) + } + + @Test + fun `reads a multi polygon`() { + val map = + mapOf( + GeometryProto.MULTI_POLYGON_FIELD_NUMBER.toString() to + mapOf( + MultiPolygonProto.POLYGONS_FIELD_NUMBER.toString() to + listOf(polygonMap(SHELL), polygonMap(HOLE)) + ) + ) + + val geometry = LoiGeometryConverter.toGeometry(map) as MultiPolygon + + assertThat(geometry.polygons).hasSize(2) + } + + @Test + fun `accepts whole-number coordinates, which Firestore returns as Long`() { + val map = + mapOf( + GeometryProto.POINT_FIELD_NUMBER.toString() to + mapOf( + PointProto.COORDINATES_FIELD_NUMBER.toString() to + mapOf( + CoordinatesProto.LATITUDE_FIELD_NUMBER.toString() to 1L, + CoordinatesProto.LONGITUDE_FIELD_NUMBER.toString() to 2L, + ) + ) + ) + + assertThat(LoiGeometryConverter.toGeometry(map)).isEqualTo(Point(Coordinates(1.0, 2.0))) + } + + @Test + fun `fails on an unrecognized geometry type`() { + assertFailsWith { LoiGeometryConverter.toGeometry(mapOf("99" to 1)) } + } + + @Test + fun `fails when the geometry field is missing`() { + assertFailsWith { LoiGeometryConverter.toGeometry(null) } + } + + @Test + fun `fails when a coordinate is not a number`() { + val map = + mapOf( + GeometryProto.POINT_FIELD_NUMBER.toString() to + mapOf( + PointProto.COORDINATES_FIELD_NUMBER.toString() to + mapOf( + CoordinatesProto.LATITUDE_FIELD_NUMBER.toString() to "not a number", + CoordinatesProto.LONGITUDE_FIELD_NUMBER.toString() to 2.0, + ) + ) + ) + + assertFailsWith { LoiGeometryConverter.toGeometry(map) } + } + + private fun pointMap(lat: Double, lng: Double) = + mapOf( + GeometryProto.POINT_FIELD_NUMBER.toString() to + mapOf(PointProto.COORDINATES_FIELD_NUMBER.toString() to coordinatesMap(lat, lng)) + ) + + private fun coordinatesMap(lat: Double, lng: Double) = + mapOf( + CoordinatesProto.LATITUDE_FIELD_NUMBER.toString() to lat, + CoordinatesProto.LONGITUDE_FIELD_NUMBER.toString() to lng, + ) + + private fun ringMap(coordinates: List>) = + mapOf( + LinearRingProto.COORDINATES_FIELD_NUMBER.toString() to + coordinates.map { coordinatesMap(it[0], it[1]) } + ) + + private fun polygonMap(shell: List>, vararg holes: List>) = buildMap { + put(PolygonProto.SHELL_FIELD_NUMBER.toString(), ringMap(shell)) + if (holes.isNotEmpty()) { + put(PolygonProto.HOLES_FIELD_NUMBER.toString(), holes.map { ringMap(it) }) + } + } + + private fun polygonGeometryMap(shell: List>, vararg holes: List>) = + mapOf(GeometryProto.POLYGON_FIELD_NUMBER.toString() to polygonMap(shell, *holes)) + + companion object { + private val SHELL = + listOf(listOf(0.0, 0.0), listOf(0.0, 1.0), listOf(1.0, 1.0), listOf(0.0, 0.0)) + private val HOLE = + listOf(listOf(0.2, 0.2), listOf(0.2, 0.4), listOf(0.4, 0.4), listOf(0.2, 0.2)) + } +} From 9285964cf02d3614add2d551f15efa11bbba0496 Mon Sep 17 00:00:00 2001 From: andreia Date: Tue, 4 Aug 2026 18:38:23 +0200 Subject: [PATCH 2/5] keep only the LOI properties the app reads --- .../remote/firebase/schema/LoiConverter.kt | 53 +++++++++++++++---- .../firebase/schema/LoiConverterTest.kt | 4 +- 2 files changed, 45 insertions(+), 12 deletions(-) diff --git a/app/src/main/java/org/groundplatform/android/data/remote/firebase/schema/LoiConverter.kt b/app/src/main/java/org/groundplatform/android/data/remote/firebase/schema/LoiConverter.kt index 49b4cbbc61..26f53ec976 100644 --- a/app/src/main/java/org/groundplatform/android/data/remote/firebase/schema/LoiConverter.kt +++ b/app/src/main/java/org/groundplatform/android/data/remote/firebase/schema/LoiConverter.kt @@ -21,6 +21,8 @@ import org.groundplatform.android.data.remote.firebase.protobuf.parseFrom import org.groundplatform.android.proto.LocationOfInterest as LocationOfInterestProto import org.groundplatform.android.proto.LocationOfInterest.Source import org.groundplatform.domain.model.Survey +import org.groundplatform.domain.model.locationofinterest.LOI_ID_PROPERTY +import org.groundplatform.domain.model.locationofinterest.LOI_NAME_PROPERTY import org.groundplatform.domain.model.locationofinterest.LocationOfInterest /** Converts between Firestore documents and [LocationOfInterest] instances. */ @@ -31,6 +33,43 @@ object LoiConverter { const val POLYGON_TYPE = "Polygon" private val GEOMETRY_FIELD = LocationOfInterestProto.GEOMETRY_FIELD_NUMBER.toString() + private val PROPERTIES_FIELD = LocationOfInterestProto.PROPERTIES_FIELD_NUMBER.toString() + + /** Keys within a `Property`, which holds one of a string or a numeric value. */ + private val PROPERTY_STRING_VALUE = + LocationOfInterestProto.Property.STRING_VALUE_FIELD_NUMBER.toString() + private val PROPERTY_NUMERIC_VALUE = + LocationOfInterestProto.Property.NUMERIC_VALUE_FIELD_NUMBER.toString() + + /** The only LOI properties the app reads; see [readRetainedProperties]. */ + private val RETAINED_PROPERTIES = listOf(LOI_NAME_PROPERTY, LOI_ID_PROPERTY) + + /** + * Returns the LOI properties the app actually reads, taken straight from the document. + * + * Imported LOIs carry every attribute of the feature they came from — hundreds of key-value pairs + * in some surveys — of which only the name and ID are ever displayed. Keeping the rest costs + * reflective parsing per pair, heap for the lifetime of the survey, and space in the local db, so + * they are dropped on the way in. + * + * Dropping them does not put the remote copy at risk. [LocationOfInterest.toMutation] does carry + * properties into mutations, but deletes remove the document outright and updates are written + * with `SetOptions.merge()`, which merges nested maps key by key rather than replacing them. + * + * What it does mean is that any feature needing an attribute other than these will silently find + * it absent rather than fail; add the key here and re-sync to get it back. + */ + private fun readRetainedProperties(value: Any?): Map { + val properties = value as? Map<*, *> ?: return mapOf() + return RETAINED_PROPERTIES.mapNotNull { key -> + (properties[key] as? Map<*, *>)?.let { property -> + val numeric = property[PROPERTY_NUMERIC_VALUE] as? Number + val text = property[PROPERTY_STRING_VALUE] as? String + (numeric ?: text)?.let { key to it } + } + } + .toMap() + } fun toLoi(survey: Survey, doc: DocumentSnapshot): Result = runCatching { toLoiUnchecked(survey, doc) @@ -42,7 +81,9 @@ object LoiConverter { val loiId = doc.id val data = doc.data.orEmpty() val geometry = LoiGeometryConverter.toGeometry(data[GEOMETRY_FIELD]) - val loiProto = LocationOfInterestProto::class.parseFrom(loiId, data - GEOMETRY_FIELD, 1) + val properties = readRetainedProperties(data[PROPERTIES_FIELD]) + val loiProto = + LocationOfInterestProto::class.parseFrom(loiId, data - GEOMETRY_FIELD - PROPERTIES_FIELD, 1) val jobId = loiProto.jobId val job = DataStoreException.checkNotNull(survey.getJob(jobId), "job $jobId") // Degrade gracefully when audit info missing in remote db. @@ -55,16 +96,6 @@ object LoiConverter { } val submissionCount = loiProto.submissionCount - val properties = - loiProto.propertiesMap.entries.associate { - val propertyValue = - if (it.value.hasNumericValue()) { - it.value.numericValue - } else { - it.value.stringValue - } - it.key to propertyValue - } val isPredefined = loiProto.source == Source.IMPORTED return LocationOfInterest( id = loiId, diff --git a/app/src/test/java/org/groundplatform/android/data/remote/firebase/schema/LoiConverterTest.kt b/app/src/test/java/org/groundplatform/android/data/remote/firebase/schema/LoiConverterTest.kt index 73668e6ebe..05505a7226 100644 --- a/app/src/test/java/org/groundplatform/android/data/remote/firebase/schema/LoiConverterTest.kt +++ b/app/src/test/java/org/groundplatform/android/data/remote/firebase/schema/LoiConverterTest.kt @@ -87,6 +87,8 @@ class LoiConverterTest { source = Source.IMPORTED properties.put("property1", property { stringValue = "value1" }) properties.put("property2", property { numericValue = 123.0 }) + properties.put("name", property { stringValue = "a plot" }) + properties.put("id", property { stringValue = "plot-7" }) } @Test @@ -113,7 +115,7 @@ class LoiConverterTest { lastModified = AuditInfo(user = USER, 987654321L * 1000, 9876543210L * 1000), geometry = Point(coordinates = Coordinates(1.0, 2.0)), submissionCount = 1, - properties = mapOf("property1" to "value1", "property2" to 123.0), + properties = mapOf("name" to "a plot", "id" to "plot-7"), isPredefined = true, ), toLocationOfInterest(), From 7e7783039fd44f417313ed23a57f8248f3c853f1 Mon Sep 17 00:00:00 2001 From: andreia Date: Tue, 4 Aug 2026 18:44:34 +0200 Subject: [PATCH 3/5] cache resolved proto reflection metadata --- .../protobuf/MessageLiteReflectionExt.kt | 76 ++++++++++++++----- 1 file changed, 58 insertions(+), 18 deletions(-) diff --git a/app/src/main/java/org/groundplatform/android/data/remote/firebase/protobuf/MessageLiteReflectionExt.kt b/app/src/main/java/org/groundplatform/android/data/remote/firebase/protobuf/MessageLiteReflectionExt.kt index cc75700d42..043cd1e895 100644 --- a/app/src/main/java/org/groundplatform/android/data/remote/firebase/protobuf/MessageLiteReflectionExt.kt +++ b/app/src/main/java/org/groundplatform/android/data/remote/firebase/protobuf/MessageLiteReflectionExt.kt @@ -18,7 +18,9 @@ package org.groundplatform.android.data.remote.firebase.protobuf import com.google.protobuf.GeneratedMessageLite import com.google.protobuf.Internal.EnumLite +import java.lang.reflect.Method import java.lang.reflect.Modifier +import java.util.concurrent.ConcurrentHashMap import kotlin.reflect.KClass import kotlin.reflect.KFunction import kotlin.reflect.KProperty @@ -27,6 +29,7 @@ import kotlin.reflect.full.declaredFunctions import kotlin.reflect.full.declaredMemberProperties import kotlin.reflect.full.isSubclassOf import kotlin.reflect.jvm.isAccessible +import kotlin.reflect.jvm.javaMethod import timber.log.Timber /** A key used in a document or a nested object in Firestore. */ @@ -86,26 +89,63 @@ fun KClass.getListElementFieldTypeByName(fieldName: Stri java.getDeclaredMethod("get${fieldName.toUpperCamelCase()}", Int::class.java).returnType?.kotlin ?: throw UnsupportedOperationException("Getter not found for field $fieldName") -private fun MessageBuilder.getSetterByFieldName(fieldName: String): KFunction<*> = - // Message fields generated two setters; ignore the Builder's setter in favor of the - // message setter. - this::class.declaredFunctions.find { - it.name == "set${fieldName.toUpperCamelCase()}" && !it.parameters[1].type.isBuilder() - } ?: throw UnsupportedOperationException("Setter not found for field $fieldName") +/** + * Resolved builder methods, keyed by the builder class and method name. + * + * Finding one goes through `declaredFunctions`, which rebuilds the whole class's Kotlin reflection + * metadata on every call and caches nothing — by far the most expensive step of mapping a document + * onto a proto, and repeated for every field of every document. The result depends only on the key, + * so it is resolved once and reused. The underlying [Method] is what gets stored, since invoking it + * directly is much cheaper than going through [KFunction.call]. + */ +private val methodCache = ConcurrentHashMap() + +/** Identifies a single method of a class, for use as a cache key. */ +private data class MemberKey(val declaringClass: Class<*>, val methodName: String) + +/** + * Returns the cached method for [name] on this builder, resolving it with [resolve] on first use. + * + * A plain get-then-put rather than `computeIfAbsent`: the latter allocates a capturing lambda on + * every hit and may lock the bin, and this is read concurrently while a survey's LOI sources are + * fetched in parallel. Resolution is idempotent, so a duplicate compute under a race is harmless. + */ +private fun MessageBuilder.cachedMethod(name: String, resolve: () -> KFunction<*>): Method { + val key = MemberKey(javaClass, name) + return methodCache[key] + ?: resolve().javaMethod!!.apply { isAccessible = true }.also { methodCache[key] = it } +} -private fun MessageBuilder.getAddAllByFieldName(fieldName: String): KFunction<*> = - // Message fields generated two setters; ignore the Builder's setter in favor of the - // message setter. - this::class.declaredFunctions.find { - it.name == "addAll${fieldName.toUpperCamelCase()}" && !it.parameters[1].type.isBuilder() - } ?: throw UnsupportedOperationException("addAll not found for field $fieldName") +private fun MessageBuilder.getSetterByFieldName(fieldName: String): Method { + val name = "set${fieldName.toUpperCamelCase()}" + return cachedMethod(name) { + // Message fields generated two setters; ignore the Builder's setter in favor of the + // message setter. + this::class.declaredFunctions.find { it.name == name && !it.parameters[1].type.isBuilder() } + ?: throw UnsupportedOperationException("Setter not found for field $fieldName") + } +} + +private fun MessageBuilder.getAddAllByFieldName(fieldName: String): Method { + val name = "addAll${fieldName.toUpperCamelCase()}" + return cachedMethod(name) { + // Message fields generated two setters; ignore the Builder's setter in favor of the + // message setter. + this::class.declaredFunctions.find { it.name == name && !it.parameters[1].type.isBuilder() } + ?: throw UnsupportedOperationException("addAll not found for field $fieldName") + } +} private fun KType.isBuilder() = (classifier as KClass<*>).isSubclassOf(GeneratedMessageLite.Builder::class) -private fun MessageBuilder.getPutAllByFieldName(fieldName: String): KFunction<*> = - this::class.declaredFunctions.find { it.name == "putAll${fieldName.toUpperCamelCase()}" } - ?: throw UnsupportedOperationException("Putter not found for field $fieldName") +private fun MessageBuilder.getPutAllByFieldName(fieldName: String): Method { + val name = "putAll${fieldName.toUpperCamelCase()}" + return cachedMethod(name) { + this::class.declaredFunctions.find { it.name == name } + ?: throw UnsupportedOperationException("Putter not found for field $fieldName") + } +} fun KClass.newBuilderForType() = java.getDeclaredMethod("newBuilder").invoke(null) as MessageBuilder @@ -180,15 +220,15 @@ private fun String.toUpperCamelCase(): String = toCamelCase().replaceFirstChar { it.uppercaseChar() } private fun MessageBuilder.set(fieldName: MessageFieldName, value: MessageValue) { - getSetterByFieldName(fieldName).call(this, value) + getSetterByFieldName(fieldName).invoke(this, value) } private fun MessageBuilder.addAll(fieldName: MessageFieldName, value: MessageValue) { - getAddAllByFieldName(fieldName).call(this, value) + getAddAllByFieldName(fieldName).invoke(this, value) } private fun MessageBuilder.putAll(fieldName: MessageFieldName, value: MessageMap) { - getPutAllByFieldName(fieldName).call(this, value) + getPutAllByFieldName(fieldName).invoke(this, value) } fun KClass.getFieldProperties(): List> = From 5ec2e0a6b039e76cf7a237f4c313cd7202a31d9a Mon Sep 17 00:00:00 2001 From: andreia Date: Tue, 4 Aug 2026 19:31:17 +0200 Subject: [PATCH 4/5] simplify code --- .../protobuf/FirestoreToProtobufExt.kt | 4 ++- .../protobuf/MessageLiteReflectionExt.kt | 17 +++-------- .../remote/firebase/schema/LoiConverter.kt | 20 ++----------- .../protobuf/FirestoreToProtobufExtTest.kt | 30 +++++++++++++++++++ .../schema/LoiGeometryConverterTest.kt | 1 - 5 files changed, 39 insertions(+), 33 deletions(-) diff --git a/app/src/main/java/org/groundplatform/android/data/remote/firebase/protobuf/FirestoreToProtobufExt.kt b/app/src/main/java/org/groundplatform/android/data/remote/firebase/protobuf/FirestoreToProtobufExt.kt index 1dd1e9dfd7..52fed401bc 100644 --- a/app/src/main/java/org/groundplatform/android/data/remote/firebase/protobuf/FirestoreToProtobufExt.kt +++ b/app/src/main/java/org/groundplatform/android/data/remote/firebase/protobuf/FirestoreToProtobufExt.kt @@ -111,8 +111,10 @@ private fun FirestoreValue.toMessageValue( (this as FirestoreMap).toMessageMap(builderType.getMapValueType(fieldName)) } else if (fieldType.isSubclassOf(List::class)) { val elementType = builderType.getListElementFieldTypeByName(fieldName) + // Resolved once rather than per element, since repeated fields can be long. + val isMessageElement = elementType.isSubclassOf(GeneratedMessageLite::class) (this as List).map { - if (elementType.isSubclassOf(GeneratedMessageLite::class)) { + if (isMessageElement) { (elementType as KClass).parseFrom(it as FirestoreMap) } else { it.toMessageValue(elementType) diff --git a/app/src/main/java/org/groundplatform/android/data/remote/firebase/protobuf/MessageLiteReflectionExt.kt b/app/src/main/java/org/groundplatform/android/data/remote/firebase/protobuf/MessageLiteReflectionExt.kt index 043cd1e895..5ee03ea05d 100644 --- a/app/src/main/java/org/groundplatform/android/data/remote/firebase/protobuf/MessageLiteReflectionExt.kt +++ b/app/src/main/java/org/groundplatform/android/data/remote/firebase/protobuf/MessageLiteReflectionExt.kt @@ -90,26 +90,17 @@ fun KClass.getListElementFieldTypeByName(fieldName: Stri ?: throw UnsupportedOperationException("Getter not found for field $fieldName") /** - * Resolved builder methods, keyed by the builder class and method name. + * Builder methods already looked up, keyed by builder class and method name. * - * Finding one goes through `declaredFunctions`, which rebuilds the whole class's Kotlin reflection - * metadata on every call and caches nothing — by far the most expensive step of mapping a document - * onto a proto, and repeated for every field of every document. The result depends only on the key, - * so it is resolved once and reused. The underlying [Method] is what gets stored, since invoking it - * directly is much cheaper than going through [KFunction.call]. + * Finding one uses `declaredFunctions`, which is slow and remembers nothing, so it runs once per + * method rather than once per field of every document. */ private val methodCache = ConcurrentHashMap() /** Identifies a single method of a class, for use as a cache key. */ private data class MemberKey(val declaringClass: Class<*>, val methodName: String) -/** - * Returns the cached method for [name] on this builder, resolving it with [resolve] on first use. - * - * A plain get-then-put rather than `computeIfAbsent`: the latter allocates a capturing lambda on - * every hit and may lock the bin, and this is read concurrently while a survey's LOI sources are - * fetched in parallel. Resolution is idempotent, so a duplicate compute under a race is harmless. - */ +/** Returns the method named [name], calling [resolve] to find it the first time only. */ private fun MessageBuilder.cachedMethod(name: String, resolve: () -> KFunction<*>): Method { val key = MemberKey(javaClass, name) return methodCache[key] diff --git a/app/src/main/java/org/groundplatform/android/data/remote/firebase/schema/LoiConverter.kt b/app/src/main/java/org/groundplatform/android/data/remote/firebase/schema/LoiConverter.kt index 26f53ec976..015745cfb5 100644 --- a/app/src/main/java/org/groundplatform/android/data/remote/firebase/schema/LoiConverter.kt +++ b/app/src/main/java/org/groundplatform/android/data/remote/firebase/schema/LoiConverter.kt @@ -41,25 +41,9 @@ object LoiConverter { private val PROPERTY_NUMERIC_VALUE = LocationOfInterestProto.Property.NUMERIC_VALUE_FIELD_NUMBER.toString() - /** The only LOI properties the app reads; see [readRetainedProperties]. */ private val RETAINED_PROPERTIES = listOf(LOI_NAME_PROPERTY, LOI_ID_PROPERTY) - /** - * Returns the LOI properties the app actually reads, taken straight from the document. - * - * Imported LOIs carry every attribute of the feature they came from — hundreds of key-value pairs - * in some surveys — of which only the name and ID are ever displayed. Keeping the rest costs - * reflective parsing per pair, heap for the lifetime of the survey, and space in the local db, so - * they are dropped on the way in. - * - * Dropping them does not put the remote copy at risk. [LocationOfInterest.toMutation] does carry - * properties into mutations, but deletes remove the document outright and updates are written - * with `SetOptions.merge()`, which merges nested maps key by key rather than replacing them. - * - * What it does mean is that any feature needing an attribute other than these will silently find - * it absent rather than fail; add the key here and re-sync to get it back. - */ - private fun readRetainedProperties(value: Any?): Map { + private fun pruneUnusedProperties(value: Any?): Map { val properties = value as? Map<*, *> ?: return mapOf() return RETAINED_PROPERTIES.mapNotNull { key -> (properties[key] as? Map<*, *>)?.let { property -> @@ -81,7 +65,7 @@ object LoiConverter { val loiId = doc.id val data = doc.data.orEmpty() val geometry = LoiGeometryConverter.toGeometry(data[GEOMETRY_FIELD]) - val properties = readRetainedProperties(data[PROPERTIES_FIELD]) + val properties = pruneUnusedProperties(data[PROPERTIES_FIELD]) val loiProto = LocationOfInterestProto::class.parseFrom(loiId, data - GEOMETRY_FIELD - PROPERTIES_FIELD, 1) val jobId = loiProto.jobId diff --git a/app/src/test/java/org/groundplatform/android/data/remote/firebase/protobuf/FirestoreToProtobufExtTest.kt b/app/src/test/java/org/groundplatform/android/data/remote/firebase/protobuf/FirestoreToProtobufExtTest.kt index ac1326e4fd..0a22790b13 100644 --- a/app/src/test/java/org/groundplatform/android/data/remote/firebase/protobuf/FirestoreToProtobufExtTest.kt +++ b/app/src/test/java/org/groundplatform/android/data/remote/firebase/protobuf/FirestoreToProtobufExtTest.kt @@ -26,6 +26,10 @@ import org.groundplatform.android.proto.Task.DateTimeQuestion.Type.BOTH_DATE_AND import org.groundplatform.android.proto.Task.MultipleChoiceQuestion.Type.SELECT_MULTIPLE import org.groundplatform.android.proto.TaskKt.dateTimeQuestion import org.groundplatform.android.proto.TaskKt.multipleChoiceQuestion +import org.groundplatform.android.proto.coordinates +import org.groundplatform.android.proto.geometry +import org.groundplatform.android.proto.linearRing +import org.groundplatform.android.proto.polygon import org.groundplatform.android.proto.survey import org.groundplatform.android.proto.task import org.groundplatform.android.test.deeplyNestedTestObject @@ -126,6 +130,12 @@ class FirestoreToProtobufExtTest( ), testCase(desc = "skips enum value 0", input = mapOf("3" to 0), expected = task {}), testCase(desc = "skips an unspecified enum value", input = mapOf(), expected = task {}), + // Exercises the repeated-message branch, whose per-element type resolution is cached. + testCase( + desc = "converts repeated messages", + input = REPEATED_MESSAGE_PROTO.toFirestoreMap(), + expected = REPEATED_MESSAGE_PROTO, + ), testCase( desc = "converts oneof messages", input = mapOf("10" to mapOf("1" to 2)), @@ -134,6 +144,26 @@ class FirestoreToProtobufExtTest( ), ) + /** A message carrying a repeated nested message: a ring of coordinates. */ + private val REPEATED_MESSAGE_PROTO = geometry { + polygon = polygon { + shell = linearRing { + coordinates.add( + coordinates { + latitude = 1.0 + longitude = 2.0 + } + ) + coordinates.add( + coordinates { + latitude = 3.0 + longitude = 4.0 + } + ) + } + } + } + /** Help to improve readability by provided named args for positional test constructor args. */ private fun testCase( desc: String, diff --git a/app/src/test/java/org/groundplatform/android/data/remote/firebase/schema/LoiGeometryConverterTest.kt b/app/src/test/java/org/groundplatform/android/data/remote/firebase/schema/LoiGeometryConverterTest.kt index 6f3a435b15..e13e271fc8 100644 --- a/app/src/test/java/org/groundplatform/android/data/remote/firebase/schema/LoiGeometryConverterTest.kt +++ b/app/src/test/java/org/groundplatform/android/data/remote/firebase/schema/LoiGeometryConverterTest.kt @@ -18,7 +18,6 @@ package org.groundplatform.android.data.remote.firebase.schema import com.google.common.truth.Truth.assertThat import kotlin.test.assertFailsWith import org.groundplatform.android.data.remote.DataStoreException -import org.groundplatform.android.data.remote.firebase.schema.GeometryConverter.toGeometry import org.groundplatform.android.proto.Coordinates as CoordinatesProto import org.groundplatform.android.proto.Geometry as GeometryProto import org.groundplatform.android.proto.LinearRing as LinearRingProto From 5db38be04e41bf7e512244294927a38d1d9fe8b0 Mon Sep 17 00:00:00 2001 From: andreia Date: Tue, 4 Aug 2026 23:20:24 +0200 Subject: [PATCH 5/5] fix code quality error in test variable --- .../protobuf/FirestoreToProtobufExtTest.kt | 39 +++++++++---------- 1 file changed, 19 insertions(+), 20 deletions(-) diff --git a/app/src/test/java/org/groundplatform/android/data/remote/firebase/protobuf/FirestoreToProtobufExtTest.kt b/app/src/test/java/org/groundplatform/android/data/remote/firebase/protobuf/FirestoreToProtobufExtTest.kt index 0a22790b13..01bce05f47 100644 --- a/app/src/test/java/org/groundplatform/android/data/remote/firebase/protobuf/FirestoreToProtobufExtTest.kt +++ b/app/src/test/java/org/groundplatform/android/data/remote/firebase/protobuf/FirestoreToProtobufExtTest.kt @@ -55,6 +55,25 @@ class FirestoreToProtobufExtTest( companion object { @get:ClassRule @JvmStatic var timberRule = TimberTestRule() + /** A message carrying a repeated nested message: a ring of coordinates. */ + private val REPEATED_MESSAGE_PROTO = geometry { + polygon = polygon { + shell = linearRing { + coordinates.add( + coordinates { + latitude = 1.0 + longitude = 2.0 + } + ) + coordinates.add( + coordinates { + latitude = 3.0 + longitude = 4.0 + } + ) + } + } + } @JvmStatic @Parameterized.Parameters(name = "{0}") @@ -144,26 +163,6 @@ class FirestoreToProtobufExtTest( ), ) - /** A message carrying a repeated nested message: a ring of coordinates. */ - private val REPEATED_MESSAGE_PROTO = geometry { - polygon = polygon { - shell = linearRing { - coordinates.add( - coordinates { - latitude = 1.0 - longitude = 2.0 - } - ) - coordinates.add( - coordinates { - latitude = 3.0 - longitude = 4.0 - } - ) - } - } - } - /** Help to improve readability by provided named args for positional test constructor args. */ private fun testCase( desc: String,