Skip to content

Fixes Connect DB upgrade #3345

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
shubham1g5 merged 1 commit into from
Sep 17, 2025
Merged

Fixes Connect DB upgrade #3345

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/src/org/commcare/models/database/connect/ConnectDatabaseUpgrader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -641,6 +641,7 @@ private void upgradeSeventeenEighteen(IDatabase db) {
// We have not been populating this table yet, so just drop and recreate
SqlStorage.dropTable(db, PersonalIdCredential.STORAGE_KEY);
addTableForNewModel(db, PersonalIdCredential.STORAGE_KEY, new PersonalIdCredential());
db.setTransactionSuccessful();
} finally {
db.endTransaction();
Comment on lines 641 to 646
Copy link

@coderabbitai coderabbitai bot Sep 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

🧩 Analysis chain

Safety/telemetry before destructive drop.

If the “not populated yet” assumption ever breaks in the field, we’ll silently delete data. Log row count before dropping to surface surprises.

     try {
-        // We have not been populating this table yet, so just drop and recreate
+        // We have not been populating this table yet, so just drop and recreate
+        // Telemetry: log if we find unexpected rows before drop
+        SqlStorage<Persistable> picStorage = new SqlStorage<>(
+                PersonalIdCredential.STORAGE_KEY,
+                PersonalIdCredential.class,
+                new ConcreteAndroidDbHelper(c, db));
+        int existing = picStorage.getNumRecords();
+        if (existing > 0) {
+            CrashUtil.log("upgradeSeventeenEighteen: dropping PersonalIdCredential with " + existing + " rows");
+        }
         SqlStorage.dropTable(db, PersonalIdCredential.STORAGE_KEY);
-        addTableForNewModel(db, PersonalIdCredential.STORAGE_KEY, new PersonalIdCredential());
+        TableBuilder builder = new TableBuilder(PersonalIdCredential.STORAGE_KEY);
+        builder.addData(new PersonalIdCredential());
+        db.execSQL(builder.getTableCreateString());
         db.setTransactionSuccessful();

To validate the assumption across the codebase (no writes before v18), run:

🏁 Script executed:

#!/bin/bash
# Find any writes/usages of PersonalIdCredential
rg -nP -C3 --type=java '\bPersonalIdCredential\b'
rg -nP -C2 --type=java 'SqlStorage<\s*PersonalIdCredential\s*>'
rg -nP -C3 --type=java '\.write\s*\(' | rg -nP -C0 'PersonalIdCredential|STORAGE_KEY'

Length of output: 8968

Do not unconditionally drop PersonalIdCredential — log/count and preserve/migrate if populated.

ConnectAppDatabaseUtil.storeCredentialDataInTable writes to this table, so the “not populated yet” assumption can be false. Before calling SqlStorage.dropTable(...), run a COUNT(*) on PersonalIdCredential.STORAGE_KEY and log via CrashUtil; if count > 0, preserve or migrate the data (or fail the upgrade and surface telemetry) instead of silently dropping it.

Relevant locations: app/src/org/commcare/connect/database/ConnectAppDatabaseUtil.java (storeCredentialDataInTable) and app/src/org/commcare/models/database/connect/DatabaseConnectOpenHelper.java (table creation).

}
Expand All @@ -653,7 +654,6 @@ private static void addTableForNewModel(IDatabase db, String storageKey,
TableBuilder builder = new TableBuilder(storageKey);
builder.addData(modelToAdd);
db.execSQL(builder.getTableCreateString());

db.setTransactionSuccessful();
} finally {
db.endTransaction();
Expand Down