Fixes Connect DB upgrade #3345
Fixes Connect DB upgrade #3345
Conversation
📝 WalkthroughWalkthroughThe upgrade path from database version 17 to 18 now explicitly calls db.setTransactionSuccessful() after dropping and recreating the PersonalIdCredential table within a transaction, ensuring the transaction commits on success. A minor whitespace change was made in addTableForNewModel with no functional impact. No public APIs were altered. Sequence Diagram(s)sequenceDiagram
autonumber
participant App
participant Upgrader as ConnectDatabaseUpgrader
participant DB as SQLite DB
App->>Upgrader: onUpgrade(17 -> 18)
Upgrader->>DB: beginTransaction()
Upgrader->>DB: DROP TABLE personal_id_credential
Upgrader->>DB: CREATE TABLE personal_id_credential
note over Upgrader,DB: New: mark transaction successful
Upgrader->>DB: setTransactionSuccessful()
Upgrader->>DB: endTransaction()
DB-->>Upgrader: Commit applied
Upgrader-->>App: Upgrade complete
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/src/org/commcare/models/database/connect/ConnectDatabaseUpgrader.java (1)
639-647: Avoid nested transactions (inline table creation inside the same txn).
addTableForNewModelopens its own transaction; calling it within an outer transaction creates nesting. Inline the table creation to keep a single transaction.try { // 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()); + TableBuilder builder = new TableBuilder(PersonalIdCredential.STORAGE_KEY); + builder.addData(new PersonalIdCredential()); + db.execSQL(builder.getTableCreateString()); db.setTransactionSuccessful();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/src/org/commcare/models/database/connect/ConnectDatabaseUpgrader.java(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: shubham1g5
PR: dimagi/commcare-android#0
File: :0-0
Timestamp: 2025-05-08T11:08:18.530Z
Learning: PR #3048 "Phase 4 Connect PR" introduces a substantial feature called "Connect" to the CommCare Android app, which includes messaging, job management, delivery tracking, payment processing, authentication flows, and learning modules. It follows a modern architecture using Navigation Components with three navigation graphs, segregated business logic in Manager classes, and proper database persistence.
🔇 Additional comments (1)
app/src/org/commcare/models/database/connect/ConnectDatabaseUpgrader.java (1)
644-644: Correct fix: transaction now commits for v17→18.Adding
db.setTransactionSuccessful()ensures the upgrade applies. LGTM.
| // 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(); |
There was a problem hiding this comment.
🧩 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).
Product Description
https://dimagi.atlassian.net/browse/CI-287
Technical Summary
We were missing a
setTransactionSuccessfulwhich was causing the upgrade to not be applied.Labels and Review