[WOOMOB-1389] - Fix Upserting the WCCustomerModel #14839
Conversation
AdamGrzybkowski
requested review from
JorgeMucientes,
Copilot,
hichamboushaba and
irfano
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a database upsert issue where CustomerEntity records were being duplicated instead of updated. The root cause was an auto-generated primary key that prevented Room from properly identifying existing records during upsert operations.
Key changes:
- Replaced auto-generated
idprimary key with a deterministicstableIdstring composed of site and customer identifiers - Added database migration (71→72) to deduplicate existing records and populate the new
stableIdfield - Introduced a secondary constructor in
WCCustomerModelto automatically generatestableIdfrom customer identifiers
Reviewed Changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
WCCustomerModel.kt |
Replaced auto-generated primary key with deterministic stableId; added secondary constructor and helper method for stableId generation |
Migrations.kt |
Added migration 71→72 to restructure CustomerEntity table, deduplicate records, and populate stableId |
WCAndroidDatabase.kt |
Updated database version to 72 and registered the new migration |
MigrationTests.kt |
Added comprehensive test for migration 71→72 covering deduplication and stableId generation |
CustomerDaoTest.kt |
Removed ignoringFields("id") from test assertions since stableId is now deterministic |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "site:$siteId|wp:$rid" | ||
| } else { | ||
| val aid = analyticsId ?: 0L | ||
| if (aid > 0L) "site:$siteId|analytics:$aid" else "" |
There was a problem hiding this comment.
The buildStableId function can return an empty string when both remoteId and analyticsId are 0 or null. This empty string will be used as the primary key, which could cause database constraint violations or unexpected behavior. Consider throwing an exception or returning a fallback identifier that includes the siteId to ensure uniqueness.
| if (aid > 0L) "site:$siteId|analytics:$aid" else "" | |
| if (aid > 0L) { | |
| "site:$siteId|analytics:$aid" | |
| } else { | |
| "site:$siteId|unknown" | |
| } |
There was a problem hiding this comment.
That's a good point, but from what I've checked in the code we will always have one of those values. We could consider a random id as a fallback. WDYT?
There was a problem hiding this comment.
There might be a bug preventing us from parsing analyticsId, or the backend might send 0 for it. In that case, we’ll store it as an empty string (""). An empty string as a primary key is technically valid, but if we receive 0 for analyticsId more than once, those entries will overwrite each other. Since this isn’t an expected scenario and doesn’t lead to a critical crash, I’d say we can proceed with it.
My only suggestion is to use "site:123|analytics:0" instead. That would be better than an empty string because, in the event of a potential issue, the incorrect database entry would only affect the selected site.
There was a problem hiding this comment.
My only suggestion is to use "site:123|analytics:0" instead. That would be better than an empty string because, in the event of a potential issue, the incorrect database entry would only affect the selected site.
Good point here.
And this brings me to a related point that I didn't think of during my review: we are duplicating the info about the localSiteId in the new schema, it's both stored in the stableId and also as a separate column localSiteId.
One approach to avoid this is to use a composite primary key composed of both columns: stableId and localSiteId, and then remove the site ID information from stableId.
To explain this further, please check this patch:
Index: libs/fluxc-plugin/src/main/kotlin/org/wordpress/android/fluxc/persistence/migrations/Migrations.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/libs/fluxc-plugin/src/main/kotlin/org/wordpress/android/fluxc/persistence/migrations/Migrations.kt b/libs/fluxc-plugin/src/main/kotlin/org/wordpress/android/fluxc/persistence/migrations/Migrations.kt
--- a/libs/fluxc-plugin/src/main/kotlin/org/wordpress/android/fluxc/persistence/migrations/Migrations.kt (revision 763ea79509a1a0e7dbdebbd175d94cfe7733c1e2)
+++ b/libs/fluxc-plugin/src/main/kotlin/org/wordpress/android/fluxc/persistence/migrations/Migrations.kt (date 1761843383217)
@@ -1149,8 +1149,8 @@
SELECT stableId, MAX(id) AS keepId FROM (
SELECT `id`,
CASE
- WHEN `remoteCustomerId` > 0 THEN 'site:' || `localSiteId` || '|wp:' || `remoteCustomerId`
- WHEN `analyticsCustomerId` IS NOT NULL AND `analyticsCustomerId` > 0 THEN 'site:' || `localSiteId` || '|analytics:' || `analyticsCustomerId`
+ WHEN `remoteCustomerId` > 0 THEN 'wp:' || `remoteCustomerId`
+ WHEN `analyticsCustomerId` IS NOT NULL AND `analyticsCustomerId` > 0 THEN 'analytics:' || `analyticsCustomerId`
END AS `stableId`
FROM `CustomerEntity`
WHERE `remoteCustomerId` > 0 OR (`analyticsCustomerId` IS NOT NULL AND `analyticsCustomerId` > 0)
Index: libs/fluxc-plugin/src/main/kotlin/org/wordpress/android/fluxc/model/customer/WCCustomerModel.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/libs/fluxc-plugin/src/main/kotlin/org/wordpress/android/fluxc/model/customer/WCCustomerModel.kt b/libs/fluxc-plugin/src/main/kotlin/org/wordpress/android/fluxc/model/customer/WCCustomerModel.kt
--- a/libs/fluxc-plugin/src/main/kotlin/org/wordpress/android/fluxc/model/customer/WCCustomerModel.kt (revision 763ea79509a1a0e7dbdebbd175d94cfe7733c1e2)
+++ b/libs/fluxc-plugin/src/main/kotlin/org/wordpress/android/fluxc/model/customer/WCCustomerModel.kt (date 1761843726835)
@@ -1,7 +1,6 @@
package org.wordpress.android.fluxc.model.customer
import androidx.room.Entity
-import androidx.room.PrimaryKey
import org.wordpress.android.fluxc.model.LocalOrRemoteId.LocalId
import org.wordpress.android.fluxc.model.LocalOrRemoteId.RemoteId
@@ -9,16 +8,19 @@
* Single Woo customer - see https://woocommerce.github.io/woocommerce-rest-api-docs/#customer-properties
*/
@ConsistentCopyVisibility
-@Entity(tableName = "CustomerEntity")
+@Entity(
+ tableName = "CustomerEntity",
+ primaryKeys = ["localSiteId", "stableId"]
+)
data class WCCustomerModel internal constructor(
/**
* Deterministic primary key used to uniquely identify a customer across refreshes.
*
* Format examples:
- * - "site:<localSiteId>|wp:<remoteCustomerId>" for registered users (remote > 0)
- * - "site:<localSiteId>|analytics:<analyticsCustomerId>" for analytics guests
+ * - "wp:<remoteCustomerId>" for registered users (remote > 0)
+ * - "analytics:<analyticsCustomerId>" for analytics guests
*/
- @PrimaryKey val stableId: String = "",
+ val stableId: String = "",
val localSiteId: LocalId = LocalId(0),
val remoteCustomerId: RemoteId = RemoteId(0),
val avatarUrl: String = "",
@@ -93,7 +95,7 @@
shippingState: String = "",
analyticsCustomerId: Long? = 0,
) : this(
- stableId = buildStableId(localSiteId.value, remoteCustomerId.value, analyticsCustomerId),
+ stableId = buildStableId(remoteCustomerId.value, analyticsCustomerId),
localSiteId = localSiteId,
remoteCustomerId = remoteCustomerId,
avatarUrl = avatarUrl,
@@ -143,13 +145,13 @@
* @param analyticsId analytics customer id
* @return stable id
*/
- private fun buildStableId(siteId: Int, remoteId: Long?, analyticsId: Long?): String {
+ private fun buildStableId(remoteId: Long?, analyticsId: Long?): String {
val rid = remoteId ?: 0L
return if (rid > 0L) {
- "site:$siteId|wp:$rid"
+ "wp:$rid"
} else {
val aid = analyticsId ?: 0L
- if (aid > 0L) "site:$siteId|analytics:$aid" else ""
+ if (aid > 0L) "analytics:$aid" else ""
}
}
}
The above approach has few benefits IMO:
- We enforce data consistency as there is no chance to have different site ID in
stableIdandlocalSiteId. - Solves the issue that Irfan mentioned above.
- In addition to being consistent with the other tables (most of our new tables in Room use this approach).
Let me know what you think? This is just a suggestion, not a blocking comment for the PR 🙂.
There was a problem hiding this comment.
I like it! The above patch was applied with some extra changes.
Generated by 🚫 Danger |
📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
|
|
📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## trunk #14839 +/- ##
============================================
+ Coverage 38.27% 38.30% +0.03%
- Complexity 10082 10084 +2
============================================
Files 2132 2132
Lines 120772 120863 +91
Branches 16553 16557 +4
============================================
+ Hits 46224 46298 +74
- Misses 69850 69866 +16
- Partials 4698 4699 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| override fun migrate(db: SupportSQLiteDatabase) { | ||
| // 1) Create new table with the new schema (stableId TEXT PRIMARY KEY) | ||
| db.execSQL( | ||
| """" |
There was a problem hiding this comment.
While migrating, I encountered a crash with the following log:
Log
E FATAL EXCEPTION: main (Ask Gemini)
Process: com.woocommerce.android.dev, PID: 32368
android.database.sqlite.SQLiteException: unrecognized token: ""
CREATE TABLE IF NOT EXISTS `CustomerEntity_new` (
`stableId` TEXT NOT NULL,
`localSiteId` INTEGER NOT NULL,
`remoteCustomerId` INTEGER NOT NULL,
`avatarUrl` TEXT NOT NULL,
`dateCreated` TEXT NOT NULL,
`dateCreatedGmt` TEXT NOT NULL,
`dateModified` TEXT NOT NULL,
`dateModifiedGmt` TEXT NOT NULL,
`email` TEXT NOT NULL,
`firstName` TEXT NOT NULL,
`isPayingCustomer` INTEGER NOT NULL,
`lastName` TEXT NOT NULL,
`role` TEXT NOT NULL,
`username` TEXT NOT NULL,
`billingAddress1` TEXT NOT NULL,
`billingAddress2` TEXT NOT NULL,
`billingCity` TEXT NOT NULL,
`billingCompany` TEXT NOT NULL,
`billingCountry` TEXT NOT NULL,
`billingEmail` TEXT NOT NULL,
`billingFirstName` TEXT NOT NULL,
`billingLastName` TEXT NOT NULL,
`billingPhone` TEXT NOT NULL,
`billingPostcode` TEXT NOT NULL,
`billingState` TEXT NOT NULL,
`shippingAddress1` TEXT NOT NULL,
`shippingAddress2` TEXT NOT NULL,
`shippingCity` TEXT NOT NULL,
`shippingCompany` TEXT NOT NULL,
`shippingCountry` TEXT NOT NULL,
`shippingFirstName` TEXT NOT NULL,
`shippingLastName` TEXT NOT NULL,
`shippingPostcode` TEXT NOT NULL,
`shippingState` TEXT NOT NULL,
`analyticsCustomerId` INTEGER,
PRIMARY KEY(`stableId`)
)" (code 1 SQLITE_ERROR): , while compiling: "
CREATE TABLE IF NOT EXISTS `CustomerEntity_new` (
`stableId` TEXT NOT NULL,
`localSiteId` INTEGER NOT NULL,
`remoteCustomerId` INTEGER NOT NULL,
`avatarUrl` TEXT NOT NULL,
`dateCreated` TEXT NOT NULL,
`dateCreatedGmt` TEXT NOT NULL,
`dateModified` TEXT NOT NULL,
`dateModifiedGmt` TEXT NOT NULL,
`email` TEXT NOT NULL,
`firstName` TEXT NOT NULL,
`isPayingCustomer` INTEGER NOT NULL,
`lastName` TEXT NOT NULL,
`role` TEXT NOT NULL,
`username` TEXT NOT NULL,
`billingAddress1` TEXT NOT NULL,
`billingAddress2` TEXT NOT NULL,
`billingCity` TEXT NOT NULL,
`billingCompany` TEXT NOT NULL,
`billingCountry` TEXT NOT NULL,
`billingEmail` TEXT NOT NULL,
`billingFirstName` TEXT NOT NULL,
`billingLastName` TEXT NOT NULL,
`billingPhone` TEXT NOT NULL,
`billingPostcode` TEXT NOT NULL,
`billingState` TEXT NOT NULL,
`shippingAddress1` TEXT NOT NULL,
`shippingAddress2` TEXT NOT NULL,
`shippingCity` TEXT NOT NULL,
`shippingCompany` TEXT NOT NULL,
`shippingCountry` TEXT NOT NULL,
`shippingFirstName` TEXT NOT NULL,
`shippingLastName` TEXT NOT NULL,
`shippingPostcode` TEXT NOT NULL,
`shippingState` TEXT NOT NULL,
`analyticsCustomerId` INTEGER,
PRIMARY KEY(`stableId`)
)
at android.database.sqlite.SQLiteConnection.nativePrepareStatement(Native Method)
at android.database.sqlite.SQLiteConnection.-$$Nest$smnativePrepareStatement(Unknown Source:0)
at android.database.sqlite.SQLiteConnection$PreparedStatementCache.createStatement(SQLiteConnection.java:1572)
at android.database.sqlite.SQLiteConnection.acquirePreparedStatementLI(SQLiteConnection.java:1109)
2025-10-29 13:51:21.833 32368-32368 AndroidRuntime com.woocommerce.android.dev E at android.database.sqlite.SQLiteConnection.acquirePreparedStatement(SQLiteConnection.java:1137) (Ask Gemini)
at android.database.sqlite.SQLiteConnection.prepare(SQLiteConnection.java:697)
at android.database.sqlite.SQLiteSession.prepare(SQLiteSession.java:625)
at android.database.sqlite.SQLiteProgram.<init>(SQLiteProgram.java:62)
at android.database.sqlite.SQLiteStatement.<init>(SQLiteStatement.java:34)
at android.database.sqlite.SQLiteDatabase.executeSql(SQLiteDatabase.java:2234)
at android.database.sqlite.SQLiteDatabase.execSQL(SQLiteDatabase.java:2154)
at androidx.sqlite.db.framework.FrameworkSQLiteDatabase.execSQL(FrameworkSQLiteDatabase.android.kt:264)
at org.wordpress.android.fluxc.persistence.migrations.MigrationsKt$MIGRATION_71_72$1.migrate(Migrations.kt:1093)
at androidx.room.migration.Migration.migrate(Migration.android.kt:79)
at androidx.room.BaseRoomConnectionManager.onMigrate(RoomConnectionManager.kt:212)
at androidx.room.RoomConnectionManager$SupportOpenHelperCallback.onUpgrade(RoomConnectionManager.android.kt:163)
at androidx.sqlite.db.framework.FrameworkSQLiteOpenHelper$OpenHelper.onUpgrade(FrameworkSQLiteOpenHelper.android.kt:245)
at android.database.sqlite.SQLiteOpenHelper.getDatabaseLocked(SQLiteOpenHelper.java:437)
at android.database.sqlite.SQLiteOpenHelper.getWritableDatabase(SQLiteOpenHelper.java:336)
at androidx.sqlite.db.framework.FrameworkSQLiteOpenHelper$OpenHelper.getWritableOrReadableDatabase(FrameworkSQLiteOpenHelper.android.kt:224)
at androidx.sqlite.db.framework.FrameworkSQLiteOpenHelper$OpenHelper.innerGetDatabase(FrameworkSQLiteOpenHelper.android.kt:180)
at androidx.sqlite.db.framework.FrameworkSQLiteOpenHelper$OpenHelper.getSupportDatabase(FrameworkSQLiteOpenHelper.android.kt:141)
at androidx.sqlite.db.framework.FrameworkSQLiteOpenHelper.getWritableDatabase(FrameworkSQLiteOpenHelper.android.kt:96)
at androidx.sqlite.driver.SupportSQLiteDriver.open(SupportSQLiteDriver.android.kt:57)
at androidx.sqlite.driver.SupportSQLiteDriver.open(SupportSQLiteDriver.android.kt:33)
at androidx.room.coroutines.PassthroughConnectionPool.connection$lambda$0(PassthroughConnectionPool.kt:47)
at androidx.room.coroutines.PassthroughConnectionPool.$r8$lambda$5PfN97KPsXXXOapSC9IzcedL70w(Unknown Source:0)
at androidx.room.coroutines.PassthroughConnectionPool$$ExternalSyntheticLambda0.invoke(D8$$SyntheticClass:0)
at kotlin.SynchronizedLazyImpl.getValue(LazyJVM.kt:86)
at androidx.room.coroutines.PassthroughConnectionPool.useConnection(PassthroughConnectionPool.kt:58)
at androidx.room.RoomConnectionManager.useConnection(RoomConnectionManager.android.kt:136)
at androidx.room.RoomDatabase.useConnection(RoomDatabase.android.kt:619)
at androidx.room.TriggerBasedInvalidationTracker.syncTriggers$room_runtime(InvalidationTracker.kt:306)
at androidx.room.TriggerBasedInvalidationTracker$createFlow$1$1.invokeSuspend(InvalidationTracker.kt:239)
at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:34)
at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:100)
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1156)
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:651)
at java.lang.Thread.run(Thread.java:1119)
Suppressed: kotlinx.coroutines.internal.DiagnosticCoroutineContextException: [StandaloneCoroutine{Cancelling}@9d5372e, Dispatchers.Main.immediate]
2025-10-29 13:51:21.869 673-800 InputDispatcher system_server E channel '7116c72 com.woocommerce.android.dev/com.woocommerce.android.ui.main.MainActivity' ~ Channel is unrecoverably broken and will be disposed!
2025-10-29 13:51:30.704 2029-2064 rce.android.dev com.woocommerce.android.dev E No package ID 6a found for resource ID 0x6a0b000f.
2025-10-29 13:51:30.718 2029-2064 ashmem com.woocommerce.android.dev E Pinning is deprecated since Android Q. Please use trim or other methods.
2025-10-29 13:51:30.727 2029-2082 chromium com.woocommerce.android.dev E [1029/135130.727604:ERROR:android_webview/browser/variations/variations_seed_loader.cc:39] Seed missing signature.
2025-10-29 13:51:31.008 2029-2068 rce.android.dev com.woocommerce.android.dev E hiddenapi: Accessing hidden method Ljava/lang/StackTraceElement;-><init>()V (runtime_flags=0, domain=core-platform, api=blocked) from Lcom/google/gson/internal/ConstructorConstructor; (domain=app) using reflection: denied
2025-10-29 13:51:31.274 2029-2288 SQLiteLog com.woocommerce.android.dev E (1) unrecognized token: ""
CREATE TABLE IF NOT EXISTS `CustomerEntity_new` (
`stableId` TEXT NOT NULL,
`localSiteId` INTEGER NOT NULL,
`remoteCustomerId` INT
2025-10-29 13:51:31.777 2029-2288 SQLiteLog com.woocommerce.android.dev E (1) unrecognized token: ""
CREATE TABLE IF NOT EXISTS `CustomerEntity_new` (
`stableId` TEXT NOT NULL,
`localSiteId` INTEGER NOT NULL,
`remoteCustomerId` INT
2025-10-29 13:51:31.779 2029-2287 SQLiteLog com.woocommerce.android.dev E (1) unrecognized token: ""
CREATE TABLE IF NOT EXISTS `CustomerEntity_new` (
`stableId` TEXT NOT NULL,
`localSiteId` INTEGER NOT NULL,
`remoteCustomerId` INT
2025-10-29 13:51:31.813 2029-2029 AndroidRuntime com.woocommerce.android.dev E FATAL EXCEPTION: main (Ask Gemini)
Process: com.woocommerce.android.dev, PID: 2029
android.database.sqlite.SQLiteException: unrecognized token: ""
CREATE TABLE IF NOT EXISTS `CustomerEntity_new` (
`stableId` TEXT NOT NULL,
`localSiteId` INTEGER NOT NULL,
`remoteCustomerId` INTEGER NOT NULL,
`avatarUrl` TEXT NOT NULL,
`dateCreated` TEXT NOT NULL,
`dateCreatedGmt` TEXT NOT NULL,
`dateModified` TEXT NOT NULL,
`dateModifiedGmt` TEXT NOT NULL,
`email` TEXT NOT NULL,
`firstName` TEXT NOT NULL,
`isPayingCustomer` INTEGER NOT NULL,
`lastName` TEXT NOT NULL,
`role` TEXT NOT NULL,
`username` TEXT NOT NULL,
`billingAddress1` TEXT NOT NULL,
`billingAddress2` TEXT NOT NULL,
`billingCity` TEXT NOT NULL,
`billingCompany` TEXT NOT NULL,
`billingCountry` TEXT NOT NULL,
`billingEmail` TEXT NOT NULL,
`billingFirstName` TEXT NOT NULL,
`billingLastName` TEXT NOT NULL,
`billingPhone` TEXT NOT NULL,
`billingPostcode` TEXT NOT NULL,
`billingState` TEXT NOT NULL,
`shippingAddress1` TEXT NOT NULL,
`shippingAddress2` TEXT NOT NULL,
`shippingCity` TEXT NOT NULL,
`shippingCompany` TEXT NOT NULL,
`shippingCountry` TEXT NOT NULL,
`shippingFirstName` TEXT NOT NULL,
`shippingLastName` TEXT NOT NULL,
`shippingPostcode` TEXT NOT NULL,
`shippingState` TEXT NOT NULL,
`analyticsCustomerId` INTEGER,
PRIMARY KEY(`stableId`)
)" (code 1 SQLITE_ERROR): , while compiling: "
CREATE TABLE IF NOT EXISTS `CustomerEntity_new` (
`stableId` TEXT NOT NULL,
`localSiteId` INTEGER NOT NULL,
`remoteCustomerId` INTEGER NOT NULL,
`avatarUrl` TEXT NOT NULL,
`dateCreated` TEXT NOT NULL,
`dateCreatedGmt` TEXT NOT NULL,
`dateModified` TEXT NOT NULL,
`dateModifiedGmt` TEXT NOT NULL,
`email` TEXT NOT NULL,
`firstName` TEXT NOT NULL,
`isPayingCustomer` INTEGER NOT NULL,
`lastName` TEXT NOT NULL,
`role` TEXT NOT NULL,
`username` TEXT NOT NULL,
`billingAddress1` TEXT NOT NULL,
`billingAddress2` TEXT NOT NULL,
`billingCity` TEXT NOT NULL,
`billingCompany` TEXT NOT NULL,
`billingCountry` TEXT NOT NULL,
`billingEmail` TEXT NOT NULL,
`billingFirstName` TEXT NOT NULL,
`billingLastName` TEXT NOT NULL,
`billingPhone` TEXT NOT NULL,
`billingPostcode` TEXT NOT NULL,
`billingState` TEXT NOT NULL,
`shippingAddress1` TEXT NOT NULL,
`shippingAddress2` TEXT NOT NULL,
`shippingCity` TEXT NOT NULL,
`shippingCompany` TEXT NOT NULL,
`shippingCountry` TEXT NOT NULL,
`shippingFirstName` TEXT NOT NULL,
`shippingLastName` TEXT NOT NULL,
`shippingPostcode` TEXT NOT NULL,
`shippingState` TEXT NOT NULL,
`analyticsCustomerId` INTEGER,
PRIMARY KEY(`stableId`)
)
at android.database.sqlite.SQLiteConnection.nativePrepareStatement(Native Method)
at android.database.sqlite.SQLiteConnection.-$$Nest$smnativePrepareStatement(Unknown Source:0)
at android.database.sqlite.SQLiteConnection$PreparedStatementCache.createStatement(SQLiteConnection.java:1572)
at android.database.sqlite.SQLiteConnection.acquirePreparedStatementLI(SQLiteConnection.java:1109)
2025-10-29 13:51:31.813 2029-2029 AndroidRuntime com.woocommerce.android.dev E at android.database.sqlite.SQLiteConnection.acquirePreparedStatement(SQLiteConnection.java:1137) (Ask Gemini)
at android.database.sqlite.SQLiteConnection.prepare(SQLiteConnection.java:697)
at android.database.sqlite.SQLiteSession.prepare(SQLiteSession.java:625)
at android.database.sqlite.SQLiteProgram.<init>(SQLiteProgram.java:62)
at android.database.sqlite.SQLiteStatement.<init>(SQLiteStatement.java:34)
at android.database.sqlite.SQLiteDatabase.executeSql(SQLiteDatabase.java:2234)
at android.database.sqlite.SQLiteDatabase.execSQL(SQLiteDatabase.java:2154)
at androidx.sqlite.db.framework.FrameworkSQLiteDatabase.execSQL(FrameworkSQLiteDatabase.android.kt:264)
at org.wordpress.android.fluxc.persistence.migrations.MigrationsKt$MIGRATION_71_72$1.migrate(Migrations.kt:1093)
at androidx.room.migration.Migration.migrate(Migration.android.kt:79)
at androidx.room.BaseRoomConnectionManager.onMigrate(RoomConnectionManager.kt:212)
at androidx.room.RoomConnectionManager$SupportOpenHelperCallback.onUpgrade(RoomConnectionManager.android.kt:163)
at androidx.sqlite.db.framework.FrameworkSQLiteOpenHelper$OpenHelper.onUpgrade(FrameworkSQLiteOpenHelper.android.kt:245)
at android.database.sqlite.SQLiteOpenHelper.getDatabaseLocked(SQLiteOpenHelper.java:437)
at android.database.sqlite.SQLiteOpenHelper.getWritableDatabase(SQLiteOpenHelper.java:336)
at androidx.sqlite.db.framework.FrameworkSQLiteOpenHelper$OpenHelper.getWritableOrReadableDatabase(FrameworkSQLiteOpenHelper.android.kt:224)
at androidx.sqlite.db.framework.FrameworkSQLiteOpenHelper$OpenHelper.innerGetDatabase(FrameworkSQLiteOpenHelper.android.kt:180)
at androidx.sqlite.db.framework.FrameworkSQLiteOpenHelper$OpenHelper.getSupportDatabase(FrameworkSQLiteOpenHelper.android.kt:141)
at androidx.sqlite.db.framework.FrameworkSQLiteOpenHelper.getWritableDatabase(FrameworkSQLiteOpenHelper.android.kt:96)
at androidx.sqlite.driver.SupportSQLiteDriver.open(SupportSQLiteDriver.android.kt:57)
at androidx.sqlite.driver.SupportSQLiteDriver.open(SupportSQLiteDriver.android.kt:33)
at androidx.room.coroutines.PassthroughConnectionPool.connection$lambda$0(PassthroughConnectionPool.kt:47)
at androidx.room.coroutines.PassthroughConnectionPool.$r8$lambda$5PfN97KPsXXXOapSC9IzcedL70w(Unknown Source:0)
at androidx.room.coroutines.PassthroughConnectionPool$$ExternalSyntheticLambda0.invoke(D8$$SyntheticClass:0)
at kotlin.SynchronizedLazyImpl.getValue(LazyJVM.kt:86)
at androidx.room.coroutines.PassthroughConnectionPool.useConnection(PassthroughConnectionPool.kt:58)
at androidx.room.RoomConnectionManager.useConnection(RoomConnectionManager.android.kt:136)
at androidx.room.RoomDatabase.useConnection(RoomDatabase.android.kt:619)
at androidx.room.TriggerBasedInvalidationTracker.syncTriggers$room_runtime(InvalidationTracker.kt:306)
at androidx.room.TriggerBasedInvalidationTracker$createFlow$1$1.invokeSuspend(InvalidationTracker.kt:239)
at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:34)
at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:100)
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1156)
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:651)
at java.lang.Thread.run(Thread.java:1119)
Suppressed: kotlinx.coroutines.internal.DiagnosticCoroutineContextException: [StandaloneCoroutine{Cancelling}@af03c39, Dispatchers.Main.immediate]
2025-10-29 13:51:32.283 2029-2287 SQLiteLog com.woocommerce.android.dev E (1) unrecognized token: ""
CREATE TABLE IF NOT EXISTS `CustomerEntity_new` (
`stableId` TEXT NOT NULL,
`localSiteId` INTEGER NOT NULL,
`remoteCustomerId` INT
2025-10-29 13:51:32.285 2029-2302 SQLiteLog com.woocommerce.android.dev E (1) unrecognized token: ""
CREATE TABLE IF NOT EXISTS `CustomerEntity_new` (
`stableId` TEXT NOT NULL,
`localSiteId` INTEGER NOT NULL,
`remoteCustomerId` INT
2025-10-29 13:51:32.787 2029-2302 SQLiteLog com.woocommerce.android.dev E (1) unrecognized token: ""
CREATE TABLE IF NOT EXISTS `CustomerEntity_new` (
`stableId` TEXT NOT NULL,
`localSiteId` INTEGER NOT NULL,
`remoteCustomerId` INT
2025-10-29 13:51:32.790 2029-2285 SQLiteLog com.woocommerce.android.dev E (1) unrecognized token: ""
CREATE TABLE IF NOT EXISTS `CustomerEntity_new` (
`stableId` TEXT NOT NULL,
`localSiteId` INTEGER NOT NULL,
`remoteCustomerId` INT
2025-10-29 13:51:33.292 2029-2285 SQLiteLog com.woocommerce.android.dev E (1) unrecognized token: ""
CREATE TABLE IF NOT EXISTS `CustomerEntity_new` (
`stableId` TEXT NOT NULL,
`localSiteId` INTEGER NOT NULL,
`remoteCustomerId` INT
I guess there’s an extra " character here causing this.
There was a problem hiding this comment.
Yes, I must have been testing something and I missed this. When I run the migration tests locally they fail. Do we run those on CI? 🤔
There was a problem hiding this comment.
Hey folks, not a review yet, I'm planning to check this today. But just regarding the migration, I have a thought here, if we are worried about any issues with the migration, I think it's totally fine to proceed with a destructive migration (I mean dropping the table completely and recreating a new one), our local storage is just a cache to make things load faster, so the app will work fine if the customers are missing on next launch, and the app will fetch them when needed. WDYT?
There was a problem hiding this comment.
That's not a bad idea. The migration is tested so I'm not that worried it wouldn't work, but it's not a standard add column type of migration, so there's always some sort of risk.
Once you review it, let me know what you think and I can change it.
There was a problem hiding this comment.
Once you review it, let me know what you think and I can change it.
Looking at the migration code, it seems safe, so I'm fine with keeping it, let's just discuss the point mentioned here.
When I run the migration tests locally they fail. Do we run those on CI? 🤔
We used to run them on CI, but I think that since the integration of FluxC into the main repo, we stopped running those tests.
I don't know if this was on purpose or we missed them. @wzieba any ideas about this?
There was a problem hiding this comment.
For the fluxc-plugin, we actually never run migration tests 😅 pdnsEh-ga-p2
As fluxc-plugin is now part of the Woo, I think it might be a good moment to revisit this.
There was a problem hiding this comment.
For the fluxc-plugin, we actually never run migration tests 😅 pdnsEh-ga-p2
🤦
There was a problem hiding this comment.
@wzieba Should I open a request in Linear, so that we start doing that? Testing DB migrations is quite critical in my opinion.
There was a problem hiding this comment.
Yes, please - an Apps Infra request would be great 🙏.
hichamboushaba
left a comment
hichamboushaba
left a comment
There was a problem hiding this comment.
Nice work @AdamGrzybkowski, this worked well in my tests, I left two comments, but they are not blocking.
| */ | ||
| @Entity(tableName = "CustomerEntity",) | ||
| @Entity(tableName = "CustomerEntity") | ||
| data class WCCustomerModel( |
There was a problem hiding this comment.
WDYT of marking this primary constructor as internal? It's an implementation detail, and ideally only Room should be able to access it.
(You'll need adding the ConsistentCopyVisibility annotation, which will make the copy function internal too, but I think that's fine, we don't use anywhere)
There was a problem hiding this comment.
Makes sense!
| CASE | ||
| WHEN `remoteCustomerId` > 0 THEN 'site:' || `localSiteId` || '|wp:' || `remoteCustomerId` | ||
| WHEN `analyticsCustomerId` IS NOT NULL AND `analyticsCustomerId` > 0 THEN 'site:' || `localSiteId` || '|analytics:' || `analyticsCustomerId` | ||
| ELSE 'site:' || `localSiteId` || '|wp:' || `remoteCustomerId` |
There was a problem hiding this comment.
I'm not sure about this condition. Will it ever occur? I mean is there a case where analyticsCustomerId is null for a Customer that was fetched using the analytics endpoint?
I'm fine with keeping it, but just let's think of any side effects here before merging it.
There was a problem hiding this comment.
In theory it shouldn't, in practice? That's a good question :D
I could filter out those values where the first two conditions are not met. WDYT?
There was a problem hiding this comment.
I could filter out those values where the first two conditions are not met. WDYT?
I think this would be safer.
hichamboushaba
left a comment
hichamboushaba
left a comment
There was a problem hiding this comment.
Thanks for addressing my comments, nice work.
|
@irfano Let me know when you're done with the review! 🙏 |
irfano
left a comment
irfano
left a comment
There was a problem hiding this comment.
Great catch, Adam! Thanks for handling this. I just added a non-blocking comment. LGTM! 👍🏻
WOOMOB-1389
Description
There's an issue in the
CustomerEntitytable. We use an auto-generated id for each entity rather than looking at the id we get from the server. This causes an issue when the model is Upserted to the DB, because instead of updating a new entity is always created.There's a reason for that, as it was stated in the comment:
To solve that issue, and still use a proper primary key for upsert operations, I propose that we generate the unique identifier based on the available ids:
With an exposed secondary constructor in the
WCCustomerModelthis will happen automatically for all models, so the usage is straightforward.The biggest hurdle here is a DB migration - but that was done and tested here 523032b.
An alternative solution would be to change how we Upsert the entities and make sure we check that the
remoteIdis present. This wouldn't require a DB migration, but in the long run I think the above solution is more robust and handles the issue on the DB schema level.@hichamboushaba @irfano @JorgeMucientes I wouldn't mind at least two approvals on this PR
Test Steps
DB migration
Although tests are provided, it won't hurt to do it manually:
Now the bug
Images/gif
Here's how it works now (you can see the recording before the changes in the Linera issue)
https://github.com/user-attachments/assets/2d1dba31-730e-4556-be39-bb77483b4af3
RELEASE-NOTES.txtif necessary. Use the "[Internal]" label for non-user-facing changes.