Handling Side Navigation Clicks #3294
Conversation
…ide drawer menu. Only showing side drawer in Login when PersonalID is configured. Always showing side drawer in Setup. Never showing side drawer in App Home.
📝 WalkthroughWalkthroughThe PR migrates the app’s database layer to a new IDatabase abstraction, replacing direct SQLCipher SQLiteDatabase usage across app, user, global, and connect databases. It upgrades SQLCipher dependency to net.zetetic:sqlcipher-android:4.9.0, updates ProGuard rules, adds EncryptedDatabaseAdapter/UnencryptedDatabaseAdapter, and introduces schema managers for app/global/user DBs with corresponding upgraders. CommCareApplication and multiple services, models, storage, and parsers are refactored to use IDatabase. DB name resolution moves to schema managers. Navigation drawer logic changes: removal of Personal ID sign-in in setup, drawer visibility gating tweaks, and OPPORTUNITIES now navigates to Connect via unlock flow. Unit tests updated accordingly. Sequence Diagram(s)sequenceDiagram
participant UI as UI/Feature
participant App as CommCareApplication
participant Helper as *DatabaseOpenHelper
participant Adapt as EncryptedDatabaseAdapter
participant DB as SQLCipher DB
UI->>App: getUserDbOpenHelper(userId, key)
App->>Helper: construct/open with app context
Helper->>DB: getWritableDatabase()
Helper-->>Adapt: wrap(SQLiteDatabase)
Adapt-->>App: IDatabase
App-->>UI: IDatabase handle
UI->>Adapt: beginTransaction/queries/commit via IDatabase
sequenceDiagram
actor User
participant BDA as BaseDrawerActivity
participant PIM as PersonalIdManager
participant Nav as ConnectNavHelper
User->>BDA: Select OPPORTUNITIES
BDA->>PIM: init + unlockConnect(callback)
alt unlock success
BDA->>Nav: goToConnectJobsList(context)
BDA-->>User: setResult(OK) + finish()
else unlock failure
BDA-->>User: remain on screen
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🔭 Outside diff range comments (10)
app/src/org/commcare/models/database/user/UserDatabaseUpgrader.java (1)
66-238: Upgrade flow ignores target newVersion; consider honoring it to prevent overshootingupgrade(IDatabase db, int oldVersion, int newVersion) never uses newVersion, so the chain runs through all steps up to the latest known version regardless of the requested target. This is risky if the helper calls with an intermediate target.
Two options:
- Minimal: Gate each step with newVersion (e.g., only run step N→N+1 when newVersion >= N+1).
- Preferred: Replace the linear if-chain with a while/switch that iterates until oldVersion < newVersion.
Example sketch:
- public void upgrade(IDatabase db, int oldVersion, int newVersion) { + public void upgrade(IDatabase db, int oldVersion, int newVersion) { CommCareApplication.instance().setCustomServiceBindTimeout(5 * 60 * 1000); - int startVersion = oldVersion; + int startVersion = oldVersion; + while (oldVersion < newVersion) { + switch (oldVersion) { + case 1: if (upgradeOneTwo(db)) { oldVersion = 2; } break; + case 2: if (upgradeTwoThree(db)) { oldVersion = 3; } break; + // ...continue for each step... + case 28: if (updateTwentyEightTwentyNine(db)) { oldVersion = 29; } break; + default: + // Unknown path; bail to avoid looping + return; + } + } // existing startVersion > 22 special-case for 24->25 would be handled inside its case }app/src/org/commcare/activities/CommCareSetupActivity.java (1)
655-661: Don’t navigate to Connect if unlock failsThe unlockConnect callback ignores the success flag and navigates unconditionally. This can route users to Connect screens without a valid session.
- installFragment.updateConnectButton(!fromManager && !fromExternal && PersonalIdManager.getInstance().isloggedIn(), v -> { - PersonalIdManager.getInstance().unlockConnect(this, success -> { - ConnectNavHelper.INSTANCE.goToConnectJobsList(this); - }); - }); + installFragment.updateConnectButton( + !fromManager && !fromExternal && PersonalIdManager.getInstance().isloggedIn(), + v -> PersonalIdManager.getInstance().unlockConnect(this, success -> { + if (success) { + ConnectNavHelper.INSTANCE.goToConnectJobsList(this); + } else { + Toast.makeText(this, Localization.get("connect.unlock.failed"), Toast.LENGTH_SHORT).show(); + } + }) + );app/unit-tests/src/org/commcare/android/util/TestUtils.java (1)
161-167: Close InputStreams on all paths (use try-with-resources)InputStreams are only closed on the success path. If parsing throws, the streams leak. Wrap in try-with-resources in both methods.
Apply these diffs:
--- a/app/unit-tests/src/org/commcare/android/util/TestUtils.java +++ b/app/unit-tests/src/org/commcare/android/util/TestUtils.java @@ -159,12 +159,11 @@ - DataModelPullParser parser; - - try { - InputStream is = TestUtils.class.getResourceAsStream(resourcePath); - - parser = new DataModelPullParser(is, getFactory(db, bulkProcessingEnabled), true, true); - parser.parse(); - is.close(); + try (InputStream is = TestUtils.class.getResourceAsStream(resourcePath)) { + DataModelPullParser parser = + new DataModelPullParser(is, getFactory(db, bulkProcessingEnabled), + true, true); + parser.parse(); } catch (IOException ioe) { throw wrapError(ioe, "IO Error parsing transactions"); } catch (InvalidStructureException e) { @@ -191,12 +190,11 @@ - DataModelPullParser parser; - try { - InputStream is = TestUtils.class.getResourceAsStream(resourcePath); - - parser = new DataModelPullParser(is, androidTransactionFactory, true, true); - parser.parse(); - is.close(); + try (InputStream is = TestUtils.class.getResourceAsStream(resourcePath)) { + DataModelPullParser parser = + new DataModelPullParser(is, androidTransactionFactory, + true, true); + parser.parse(); + } } catch (IOException ioe) { throw wrapError(ioe, "IO Error parsing transactions"); } catch (InvalidStructureException e) {Also applies to: 191-197
app/src/org/commcare/adapters/EntityStringFilterer.java (1)
75-99: Avoid holding a DB transaction while doing in-memory sortingbuildMatchList() opens a transaction on the user DB but does no DB work inside the lambda; this unnecessarily blocks other DB users and can hurt concurrency. If the only purpose is to periodically yield, use CPU yielding instead and drop the transaction.
Apply this refactor:
@@ - private void buildMatchList() { + private void buildMatchList() { Locale currentLocale = Locale.getDefault(); - //It's a bit sketchy here, because this DB lock will prevent - //anything else from processing - IDatabase db; - try { - db = CommCareApplication.instance().getUserDbHandle(); - } catch (SessionUnavailableException e) { - this.cancelSearch(); - return; - } - db.beginTransaction(); - try { - EntitySortUtil.sortEntities(fullEntityList, - searchTerms, - currentLocale, - isFuzzySearchEnabled, - matchScores, - matchList, - index -> getEntityAtIndex(db, index)); - db.setTransactionSuccessful(); - } finally { - db.endTransaction(); - } + EntitySortUtil.sortEntities( + fullEntityList, + searchTerms, + currentLocale, + isFuzzySearchEnabled, + matchScores, + matchList, + this::getEntityAtIndex + ); }And adjust the helper to remove DB coupling and use CPU yield:
- private Entity<TreeReference> getEntityAtIndex(IDatabase db, int index) { - if (index % 500 == 0) { - db.yieldIfContendedSafely(); - } + private Entity<TreeReference> getEntityAtIndex(int index) { + if (index % 500 == 0) { + Thread.yield(); + } Entity<TreeReference> e = fullEntityList.get(index); if (isCancelled()) { return null; } return e; }app/src/org/commcare/xml/AndroidBulkCaseXmlParser.java (1)
72-95: Refactor AndroidCaseIndexTable.indexCase to avoid nested transactionsOur investigation shows:
- SqlStorage.write(c) calls helper.getHandle() to grab the shared IDatabase connection and does not wrap its inserts in its own transaction—so it participates in the outer transaction as intended.
- However, AndroidCaseIndexTable.indexCase(Case) (app/src/org/commcare/models/database/user/models/AndroidCaseIndexTable.java:76–92) begins and ends its own transaction on the same connection. When invoked inside the bulk‐write transaction, those inner commits will persist index rows even if the outer transaction later aborts, breaking atomicity.
Please update as follows:
- In AndroidCaseIndexTable.indexCase, remove the calls to
• db.beginTransaction();
• db.setTransactionSuccessful();
• db.endTransaction();
so that indexCase simply performs the inserts under the caller’s transaction.- Audit clearCaseIndices(...) to confirm it likewise does not open its own transaction.
app/src/org/commcare/models/database/user/models/AndroidCaseIndexTablePreV21.java (1)
29-44: Eliminate nested transactions during re-indexing.Avoid starting a transaction in indexCase when reIndexAllCases already has one open. Extract a no-TX helper and reuse it in both paths.
Apply this diff:
@@ - public void indexCase(Case c) { - db.beginTransaction(); - try { - for (CaseIndex ci : c.getIndices()) { - ContentValues cv = new ContentValues(); - cv.put(COL_CASE_RECORD_ID, c.getID()); - cv.put(COL_INDEX_NAME, ci.getName()); - cv.put(COL_INDEX_TYPE, ci.getTargetType()); - cv.put(COL_INDEX_TARGET, ci.getTarget()); - db.insert(TABLE_NAME, null, cv); - } - db.setTransactionSuccessful(); - } finally { - db.endTransaction(); - } - } + public void indexCase(Case c) { + db.beginTransaction(); + try { + indexCaseNoTx(c); + db.setTransactionSuccessful(); + } finally { + db.endTransaction(); + } + } + + // Inserts indices without starting/ending a transaction. + private void indexCaseNoTx(Case c) { + for (CaseIndex ci : c.getIndices()) { + ContentValues cv = new ContentValues(); + cv.put(COL_CASE_RECORD_ID, c.getID()); + cv.put(COL_INDEX_NAME, ci.getName()); + cv.put(COL_INDEX_TYPE, ci.getTargetType()); + cv.put(COL_INDEX_TARGET, ci.getTarget()); + db.insert(TABLE_NAME, null, cv); + } + } @@ - public void reIndexAllCases(SqlStorage<ACase> caseStorage) { + public void reIndexAllCases(SqlStorage<ACase> caseStorage) { db.beginTransaction(); try { wipeTable(); for (ACase c : caseStorage) { - indexCase(c); + indexCaseNoTx(c); } db.setTransactionSuccessful(); } finally { db.endTransaction(); } }Also applies to: 56-67
app/src/org/commcare/models/database/HybridFileBackedSqlHelpers.java (1)
96-102: Fix typo in log messageThere's a typo in the log message on line 100.
- "Unable to unset orphaned file: " + deleteCount + " entries effected.b"); + "Unable to unset orphaned file: " + deleteCount + " entries affected");app/src/org/commcare/tasks/DataPullTask.java (1)
584-589: Wrap wipeStorageForFourTwelveSync in the transaction try blockCurrently, in app/src/org/commcare/tasks/DataPullTask.java (around lines 563–578), you call
wipeStorageForFourTwelveSync(userDb)immediately afterbeginTransaction()but before entering thetry { … } finally { endTransaction(); }block. If any of the delete calls insidewipeStorageForFourTwelveSyncthrow, execution will never reach thefinallyand the transaction remains open.• Move the wipe call inside the
tryso thatendTransaction()always runs:--- a/app/src/org/commcare/tasks/DataPullTask.java +++ b/app/src/org/commcare/tasks/DataPullTask.java @@ -563,7 +563,8 @@ IDatabase userDb = CommCareApplication.instance().getUserDbHandle(); - userDb.beginTransaction(); - wipeStorageForFourTwelveSync(userDb); - - try { + userDb.beginTransaction(); + try { + wipeStorageForFourTwelveSync(userDb); String syncToken = readInputWithoutCommit(cache.retrieveCache(), factory); updateUserSyncToken(syncToken); Logger.log(LogTypes.TYPE_USER, "Sync Recovery Successful"); @@ -570,7 +571,6 @@ userDb.setTransactionSuccessful(); return new Pair<>(PROGRESS_DONE, ""); } catch (InvalidStructureException | XmlPullParserException - // … ) { Logger.exception("Sync recovery failed|" + e.getLocalizedMessage(), e); return new Pair<>(PROGRESS_RECOVERY_FAIL_BAD, e.getLocalizedMessage());This guarantees that, even if a wipe operation fails,
endTransaction()(andcache.release()) runs and the database isn’t left in an open transaction.app/src/org/commcare/models/database/app/AppDatabaseUpgrader.java (2)
141-152: Verify table name constant usage.The
upgradeOneTwomethod uses the hardcoded string"RECOVERY_RESOURCE_TABLE"instead of the constantRECOVERY_RESOURCE_TABLE_NAME. This inconsistency could lead to maintenance issues.- TableBuilder builder = new TableBuilder("RECOVERY_RESOURCE_TABLE"); + TableBuilder builder = new TableBuilder(RECOVERY_RESOURCE_TABLE_NAME);
154-165: Duplicate RECOVERY_RESOURCE_TABLE creation in upgradeTwoThree
TheupgradeTwoThreemethod in AppDatabaseUpgrader.java (lines 154–165) is identical toupgradeOneTwoand re-createsRECOVERY_RESOURCE_TABLE, even though that table was already created during the v1→v2 migration. This will either fail at runtime or silently skip the new descriptor field added in v3. You should instead alter the existing table to add the optional descriptor column (or guard against duplicate creation with IF NOT EXISTS) rather than re-running a full CREATE.• File: app/src/org/commcare/models/database/app/AppDatabaseUpgrader.java
• Method: upgradeTwoThree (around lines 154–165)
🧹 Nitpick comments (30)
app/unit-tests/src/org/commcare/android/shadows/SQLiteCursorNative.java (1)
13-19: Update class documentation to reflect interface change.The comment mentions "SQL-Cipher compatible cursor" but the class now implements the standard Android Cursor interface. Consider updating the documentation to reflect this change.
/** - * Wrapper which translates a plain android SQLite cursor - * into a SQL-Cipher compatible cursor + * Wrapper which adapts a plain android SQLite cursor + * to the standard Android Cursor interface for testing * * @author ctsims * */app/unit-tests/src/org/commcare/android/shadows/SQLiteDatabaseNative.java (2)
205-206: Typo: “factor” → “factory” in exception messagesMinor copy edit in error strings.
- throw new UnsupportedOperationException("Can't perform queries with a factor in the mock db"); + throw new UnsupportedOperationException("Can't perform queries with a factory in the mock db");- throw new UnsupportedOperationException("Can't perform queries with a factor in the mock db"); + throw new UnsupportedOperationException("Can't perform queries with a factory in the mock db");Also applies to: 228-229
31-33: Document intentional no-ops for password/hook in the shadow constructorsClarify that password and hook are intentionally ignored under Robolectric to prevent confusion during future maintenance.
public void __constructor__(String path, char[] password, CursorFactory factory, int flags, SQLiteDatabaseHook hook) { - db = android.database.sqlite.SQLiteDatabase.openDatabase(path, null, flags); + // Note: password and hook are intentionally ignored in Robolectric shadow. + db = android.database.sqlite.SQLiteDatabase.openDatabase(path, null, flags); }app/src/org/commcare/models/database/user/UserDatabaseUpgrader.java (3)
333-349: Misleading log message contentThe log says "Case model update complete..." in upgradeSevenEight, but this step migrates AUser to User. Consider updating the message for clarity.
- Log.d(TAG, "Case model update complete in " + (System.currentTimeMillis() - start) + "ms"); + Log.d(TAG, "AUser -> User migration complete in " + (System.currentTimeMillis() - start) + "ms");
596-614: Possible NoSuchElementException when re-indexing fixtures with empty tablesexampleChildElement = storageForThisFixture.iterate().nextRecord() will throw if a fixture table exists but has no rows. Guard against empty iterators before building indices.
- StorageIndexedTreeElementModel exampleChildElement = - storageForThisFixture.iterate().nextRecord(); - IndexedFixturePathUtils.buildFixtureIndices(db, tableName, exampleChildElement.getIndexColumnNames()); + SqlStorageIterator<StorageIndexedTreeElementModel> it = storageForThisFixture.iterate(); + if (it.hasMore()) { + StorageIndexedTreeElementModel exampleChildElement = it.nextRecord(); + IndexedFixturePathUtils.buildFixtureIndices(db, tableName, exampleChildElement.getIndexColumnNames()); + }
753-766: Method name inconsistent with others (update vs upgrade)For consistency with the rest of the upgrader, consider renaming updateTwentySixTwentySeven to upgradeTwentySixTwentySeven.
app/src/org/commcare/activities/CommCareSetupActivity.java (1)
510-514: Nit: method naming style for PersonalIdManager.isloggedIn()If feasible, prefer isLoggedIn() to match Java bean naming conventions. If the API is fixed elsewhere, ignore.
app/src/org/commcare/models/database/app/AppDatabaseSchemaManager.java (1)
86-89: Sanitize appId used in DB filenameIf appId can contain characters not ideal for filenames or SQLite DB names, consider sanitizing (e.g., alphanumerics, underscore, dash) to avoid edge cases.
- public static String getDbName(String appId) { - return DB_LOCATOR_PREF_APP + appId; - } + public static String getDbName(String appId) { + String sanitized = appId == null ? "" : appId.replaceAll("[^A-Za-z0-9._-]", "_"); + return DB_LOCATOR_PREF_APP + sanitized; + }app/src/org/commcare/models/database/IDatabase.java (2)
31-34: Add execSQL overload with bind argsMany call sites (and SQLiteDatabase API) support execSQL with bind arguments. Consider exposing it on the interface to avoid raw string concatenation.
void execSQL(String sql); + void execSQL(String sql, Object[] bindArgs);
6-9: Tiny Javadoc nitGrammar and extra space fix for clarity.
- * Interface to abstracts database operations to allow for different and heterogeneous implementations, such as - * SQLCipher and SQLite databases. + * Interface that abstracts database operations to allow different and heterogeneous implementations, + * such as SQLCipher and SQLite databases.app/src/org/commcare/adapters/EntityStringFilterer.java (1)
64-73: Decouple helper from DB to simplify testing and reduce couplinggetEntityAtIndex now only uses the DB to yield. With the above change, it no longer needs a DB parameter, making it easier to test and reason about.
app/src/org/commcare/models/database/AndroidSandbox.java (1)
59-61: Reuse a single DB handle for drop/build/index to avoid repeated lookups.
Minor cleanup; also ensures all steps use the same handle instance.- UserDatabaseSchemaManager.dropTable(app.getUserDbHandle(), tableName); - UserDatabaseSchemaManager.buildTable(app.getUserDbHandle(), tableName, exampleEntry); - IndexedFixturePathUtils.buildFixtureIndices(app.getUserDbHandle(), tableName, indices); + IDatabase db = app.getUserDbHandle(); + UserDatabaseSchemaManager.dropTable(db, tableName); + UserDatabaseSchemaManager.buildTable(db, tableName, exampleEntry); + IndexedFixturePathUtils.buildFixtureIndices(db, tableName, indices);app/src/org/commcare/xml/AndroidCaseXmlParser.java (1)
87-89: Add @OverRide to ensure compile-time contract with parent.
If the parent now exposes the same signature, adding @OverRide prevents accidental mismatches.- protected IDatabase getDbHandle() { + @Override + protected IDatabase getDbHandle() { return CommCareApplication.instance().getUserDbHandle(); }app/src/org/commcare/sync/FormSubmissionHelper.java (1)
541-542: Fix typos in comment (“progress”, “appropriate”).Minor, but visible in code search and IDE tooltips.
Apply this diff:
- // parses prpgress and calls the approproate methods for form submission listeners + // Parses progress and calls the appropriate methods for form submission listenersapp/src/org/commcare/services/CommCareSessionService.java (2)
97-97: Document thread-safety expectations for userDatabase.userDatabase is guarded by the service’s lock for reads/writes, but the handle will be used outside of that lock by callers. Add a brief Javadoc noting:
- The handle is valid only while the session is active.
- Callers must not cache it across session boundaries.
- It may be closed by logout/expiration paths.
This will reduce misuse after the IDatabase migration.
221-225: Consider annotating getUserDbHandle() with @nullable (or throw on inactive session).This can return null before storage is prepared or after logout. Annotating reduces downstream NPE risk and improves static analysis. Alternatively, throw SessionUnavailableException to make the contract explicit, but that would be a stricter, potentially breaking API change.
Apply this diff:
- public IDatabase getUserDbHandle() { + @Nullable + public IDatabase getUserDbHandle() { synchronized (lock) { return userDatabase; } }app/src/org/commcare/navdrawer/BaseDrawerActivity.kt (1)
40-41: Document intentional no-op for COMMCARE_APPSThe comment indicates this is intentional behavior (expands/collapses menu without navigation). Consider making this more explicit.
- NavItemType.COMMCARE_APPS -> { /* No nav, expands/collapses menu */} + NavItemType.COMMCARE_APPS -> { + // Intentional no-op: menu item only expands/collapses submenu + }app/unit-tests/src/org/commcare/models/database/user/DatabaseUserOpenHelperMock.java (1)
14-16: Nit: Use the 4-arg SQLiteOpenHelper constructor for consistencyOther test helpers (e.g., DatabaseAppOpenHelperMock) use the 4-arg constructor. Unless you rely on a custom DatabaseErrorHandler, simplify for consistency.
- super(context, getDbName(userKeyRecordId), null, USER_DB_VERSION, null); + super(context, getDbName(userKeyRecordId), null, USER_DB_VERSION);app/src/org/commcare/activities/LoginActivity.java (1)
1023-1034: Guard selectedAppIndex assignment to avoid -1 when recordId is not in the listIf recordId isn’t present, indexOf returns -1 and we overwrite any previous selection with an invalid index. Guarding this keeps selectedAppIndex stable.
- if (recordId != null) { - if (!appIdDropdownList.isEmpty()) { - selectedAppIndex = appIdDropdownList.indexOf(recordId); - } - seatAppIfNeeded(recordId); - } + if (recordId != null) { + if (!appIdDropdownList.isEmpty()) { + final int idx = appIdDropdownList.indexOf(recordId); + if (idx >= 0) { + selectedAppIndex = idx; + } + } + seatAppIfNeeded(recordId); + }app/unit-tests/src/org/commcare/CommCareTestApplication.java (1)
338-356: Test-time IDatabase providers look good; clarify ignored parametersThese overrides correctly return unencrypted adapters for tests. Minor clarity nit: keys/passwords are intentionally ignored in test implementations—consider a short comment to avoid confusion for readers.
@Override public IDatabase getGlobalDbOpenHelper() { - return new UnencryptedDatabaseAdapter(new DatabaseGlobalOpenHelperMock(this)); + // Tests use unencrypted DBs. No password/key is required. + return new UnencryptedDatabaseAdapter(new DatabaseGlobalOpenHelperMock(this)); } @Override public IDatabase getUserDbOpenHelper(String userKeyRecordId, String key) { - return new UnencryptedDatabaseAdapter(new DatabaseUserOpenHelperMock(this, userKeyRecordId)); + // 'key' is intentionally ignored in tests + return new UnencryptedDatabaseAdapter(new DatabaseUserOpenHelperMock(this, userKeyRecordId)); } @Override public IDatabase getUserDbOpenHelperFromFile(String path, String password) { - return new UnencryptedDatabaseAdapter(path); + // 'password' is intentionally ignored in tests + return new UnencryptedDatabaseAdapter(path); } @Override public IDatabase getAppDbOpenHelper(String appId) { - return new UnencryptedDatabaseAdapter(new DatabaseAppOpenHelperMock(this, appId)); + // Tests use unencrypted DBs. No password/key is required. + return new UnencryptedDatabaseAdapter(new DatabaseAppOpenHelperMock(this, appId)); }app/src/org/commcare/models/database/global/DatabaseGlobalOpenHelper.java (1)
17-21: Docstring is outdated (says “unencrypted” while using SQLCipher adapter)The class Javadoc still says “global (unencrypted) db space” but we now use SQLCipher APIs and EncryptedDatabaseAdapter. Update to avoid confusion.
- * The helper for opening/updating the global (unencrypted) db space for CommCare. + * Helper for opening/updating the global database for CommCare (SQLCipher-backed).app/src/org/commcare/models/database/app/DatabaseAppOpenHelper.java (1)
28-28: Hardcoded key value may cause confusion.The field
keyis hardcoded to"null"string, which is then passed toDbUtil.trySqlCipherDbUpdate. This appears to be a placeholder for unencrypted app databases. Consider using a more descriptive constant name or adding a comment to clarify the intent.- private final String key = "null"; + // App databases are not encrypted, using "null" as a placeholder key + private static final String UNENCRYPTED_KEY = "null";Then update line 47:
- DbUtil.trySqlCipherDbUpdate(key, context, getDbName(mAppId)); + DbUtil.trySqlCipherDbUpdate(UNENCRYPTED_KEY, context, getDbName(mAppId));app/src/org/commcare/models/database/connect/DatabaseConnectOpenHelper.java (1)
154-167: Consider more specific exception handling.The catch block at line 161-165 catches and rethrows the exception after logging. Since we're already in a catch block from a retry attempt, consider adding more context to help diagnose migration failures.
} catch (SQLiteException e) { // Handle the exception, log the error, or inform the user - CrashUtil.log(e.getMessage()); + CrashUtil.log("Failed to open Connect database after migration attempt: " + e.getMessage()); throw e; }app/src/org/commcare/connect/database/ConnectDatabaseHelper.java (3)
28-28: Remove unnecessary importThe Base64 import is added but Base64 is already available through android.util.Base64 (line 28). This appears to be a duplicate import.
-import android.util.Base64;
31-31: Remove empty lineUnnecessary blank line added.
-
79-91: Consider simplifying the legacy path detectionThe current logic uses the remote/local passphrase equality check to determine the path, but the legacy comment indicates this is for backward compatibility. Consider adding more context about when this legacy path would be triggered and if there's a migration plan to deprecate it.
Can you provide more context about when the legacy path would be triggered? Is there a plan to migrate all users off this path?
app/src/org/commcare/models/database/migration/FixtureSerializationMigration.java (1)
82-92: Verify error handling behaviorThe code sets transaction as successful even on exception to prevent app crashes. While this allows users to sync and recover, it could mask data corruption issues. Consider logging metrics or creating a recovery mechanism.
Consider implementing a recovery mechanism or telemetry to track when fixture migration fails, so you can monitor the impact and potentially provide targeted fixes for affected users.
app/src/org/commcare/models/database/user/UserDatabaseSchemaManager.java (1)
82-83: Fix inconsistent indentation.The indentation is inconsistent - Line 82 uses 5 spaces while surrounding lines use 4 spaces.
- builder = new TableBuilder(DeviceReportRecord.class); - database.execSQL(builder.getTableCreateString()); + builder = new TableBuilder(DeviceReportRecord.class); + database.execSQL(builder.getTableCreateString());app/src/org/commcare/CommCareApplication.java (2)
185-185: Consider documenting the IDatabase abstraction layerThe introduction of
IDatabaseas an abstraction over SQLiteDatabase is a significant architectural change. Consider adding documentation to explain why this abstraction was introduced and its benefits.
1264-1278: Consider centralizing database open helper creationThe new database open helper methods provide a good abstraction. However, consider adding error handling or validation for the parameters, especially for the password in
getUserDbOpenHelperFromFile.Consider adding parameter validation:
public IDatabase getUserDbOpenHelperFromFile(String path, String password) { + if (path == null || path.isEmpty()) { + throw new IllegalArgumentException("Database path cannot be null or empty"); + } + if (password == null || password.isEmpty()) { + throw new IllegalArgumentException("Database password cannot be null or empty"); + } return new EncryptedDatabaseAdapter(path, password); }
| for (String id : dbIdsToRemove) { | ||
| CommCareApplication.instance().getDatabasePath(DatabaseUserOpenHelper.getDbName(id)).delete(); | ||
| CommCareApplication.instance().getDatabasePath(UserDatabaseSchemaManager.getDbName(id)).delete(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use Context.deleteDatabase to remove WAL/SHM sidecar files and add failure logging.
Deleting via File.delete() can leave -wal/-shm files behind and ignores failures. Prefer deleteDatabase(name), and fall back to deleting the path and sidecars if needed.
Apply this diff:
- for (String id : dbIdsToRemove) {
- CommCareApplication.instance().getDatabasePath(UserDatabaseSchemaManager.getDbName(id)).delete();
- }
+ for (String id : dbIdsToRemove) {
+ String dbName = UserDatabaseSchemaManager.getDbName(id);
+ boolean deleted = CommCareApplication.instance().deleteDatabase(dbName);
+ if (!deleted) {
+ // Fallback: try direct file deletion and sidecars
+ File dbFile = CommCareApplication.instance().getDatabasePath(dbName);
+ if (dbFile.exists() && !FileUtil.deleteFileOrDir(dbFile)) {
+ Logger.log(LogTypes.TYPE_ERROR_STORAGE, "Failed to delete user DB file for id: " + id);
+ }
+ File wal = new File(dbFile.getPath() + "-wal");
+ File shm = new File(dbFile.getPath() + "-shm");
+ if (wal.exists() && !wal.delete()) {
+ Logger.log(LogTypes.TYPE_ERROR_STORAGE, "Failed to delete user DB WAL for id: " + id);
+ }
+ if (shm.exists() && !shm.delete()) {
+ Logger.log(LogTypes.TYPE_ERROR_STORAGE, "Failed to delete user DB SHM for id: " + id);
+ }
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for (String id : dbIdsToRemove) { | |
| CommCareApplication.instance().getDatabasePath(DatabaseUserOpenHelper.getDbName(id)).delete(); | |
| CommCareApplication.instance().getDatabasePath(UserDatabaseSchemaManager.getDbName(id)).delete(); | |
| } | |
| for (String id : dbIdsToRemove) { | |
| String dbName = UserDatabaseSchemaManager.getDbName(id); | |
| boolean deleted = CommCareApplication.instance().deleteDatabase(dbName); | |
| if (!deleted) { | |
| // Fallback: try direct file deletion and sidecars | |
| File dbFile = CommCareApplication.instance().getDatabasePath(dbName); | |
| if (dbFile.exists() && !FileUtil.deleteFileOrDir(dbFile)) { | |
| Logger.log(LogTypes.TYPE_ERROR_STORAGE, "Failed to delete user DB file for id: " + id); | |
| } | |
| File wal = new File(dbFile.getPath() + "-wal"); | |
| File shm = new File(dbFile.getPath() + "-shm"); | |
| if (wal.exists() && !wal.delete()) { | |
| Logger.log(LogTypes.TYPE_ERROR_STORAGE, "Failed to delete user DB WAL for id: " + id); | |
| } | |
| if (shm.exists() && !shm.delete()) { | |
| Logger.log(LogTypes.TYPE_ERROR_STORAGE, "Failed to delete user DB SHM for id: " + id); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In app/src/org/commcare/AppUtils.java around lines 135 to 137, the code
currently deletes databases by calling File.delete() on the DB path which can
leave -wal/-shm sidecar files and ignores failures; replace that with
CommCareApplication.instance().deleteDatabase(UserDatabaseSchemaManager.getDbName(id))
to properly remove DB and sidecars, and if deleteDatabase returns false fall
back to attempting to delete the DB file and its -wal and -shm sidecars
explicitly while logging any failures via the app logger (include DB name and
which file failed) so unsuccessful deletions are recorded.
| private boolean upgradeThreeFour(IDatabase db) { | ||
| db.beginTransaction(); | ||
| try { | ||
| db.execSQL(DatabaseAppOpenHelper.indexOnTableWithPGUIDCommand("global_index_id", "GLOBAL_RESOURCE_TABLE")); | ||
| db.execSQL(DatabaseAppOpenHelper.indexOnTableWithPGUIDCommand("upgrade_index_id", "UPGRADE_RESOURCE_TABLE")); | ||
| db.execSQL(DatabaseAppOpenHelper.indexOnTableWithPGUIDCommand("recovery_index_id", "RECOVERY_RESOURCE_TABLE")); | ||
| db.execSQL(indexOnTableWithPGUIDCommand("global_index_id", "GLOBAL_RESOURCE_TABLE")); | ||
| db.execSQL(indexOnTableWithPGUIDCommand("upgrade_index_id", "UPGRADE_RESOURCE_TABLE")); | ||
| db.execSQL(indexOnTableWithPGUIDCommand("recovery_index_id", "RECOVERY_RESOURCE_TABLE")); | ||
| db.setTransactionSuccessful(); | ||
| return true; | ||
| } finally { | ||
| db.endTransaction(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Verify hardcoded table names in index creation.
The upgradeThreeFour method uses hardcoded table name strings instead of constants. This is inconsistent with the rest of the codebase and could cause issues if table names change.
- db.execSQL(indexOnTableWithPGUIDCommand("global_index_id", "GLOBAL_RESOURCE_TABLE"));
- db.execSQL(indexOnTableWithPGUIDCommand("upgrade_index_id", "UPGRADE_RESOURCE_TABLE"));
- db.execSQL(indexOnTableWithPGUIDCommand("recovery_index_id", "RECOVERY_RESOURCE_TABLE"));
+ db.execSQL(indexOnTableWithPGUIDCommand("global_index_id", GLOBAL_RESOURCE_TABLE_NAME));
+ db.execSQL(indexOnTableWithPGUIDCommand("upgrade_index_id", UPGRADE_RESOURCE_TABLE_NAME));
+ db.execSQL(indexOnTableWithPGUIDCommand("recovery_index_id", RECOVERY_RESOURCE_TABLE_NAME));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private boolean upgradeThreeFour(IDatabase db) { | |
| db.beginTransaction(); | |
| try { | |
| db.execSQL(DatabaseAppOpenHelper.indexOnTableWithPGUIDCommand("global_index_id", "GLOBAL_RESOURCE_TABLE")); | |
| db.execSQL(DatabaseAppOpenHelper.indexOnTableWithPGUIDCommand("upgrade_index_id", "UPGRADE_RESOURCE_TABLE")); | |
| db.execSQL(DatabaseAppOpenHelper.indexOnTableWithPGUIDCommand("recovery_index_id", "RECOVERY_RESOURCE_TABLE")); | |
| db.execSQL(indexOnTableWithPGUIDCommand("global_index_id", "GLOBAL_RESOURCE_TABLE")); | |
| db.execSQL(indexOnTableWithPGUIDCommand("upgrade_index_id", "UPGRADE_RESOURCE_TABLE")); | |
| db.execSQL(indexOnTableWithPGUIDCommand("recovery_index_id", "RECOVERY_RESOURCE_TABLE")); | |
| db.setTransactionSuccessful(); | |
| return true; | |
| } finally { | |
| db.endTransaction(); | |
| } | |
| } | |
| private boolean upgradeThreeFour(IDatabase db) { | |
| db.beginTransaction(); | |
| try { | |
| db.execSQL(indexOnTableWithPGUIDCommand("global_index_id", GLOBAL_RESOURCE_TABLE_NAME)); | |
| db.execSQL(indexOnTableWithPGUIDCommand("upgrade_index_id", UPGRADE_RESOURCE_TABLE_NAME)); | |
| db.execSQL(indexOnTableWithPGUIDCommand("recovery_index_id", RECOVERY_RESOURCE_TABLE_NAME)); | |
| db.setTransactionSuccessful(); | |
| return true; | |
| } finally { | |
| db.endTransaction(); | |
| } | |
| } |
🤖 Prompt for AI Agents
In app/src/org/commcare/models/database/app/AppDatabaseUpgrader.java around
lines 167 to 178, the index creation calls use hardcoded table name strings;
replace those string literals with the appropriate table name constants used
elsewhere in the codebase (e.g., GLOBAL_RESOURCE_TABLE, UPGRADE_RESOURCE_TABLE,
RECOVERY_RESOURCE_TABLE) so the calls become:
indexOnTableWithPGUIDCommand(<CONSTANT>, ...). Ensure you import or reference
the same constants as other classes (or the DB constants holder) and run the
build to confirm the constant names are correct.
| public EncryptedDatabaseAdapter(String path, String password) { | ||
| this.db = SQLiteDatabase.openDatabase(path, password, null, SQLiteDatabase.OPEN_READWRITE, null, null); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling for database opening
The constructor that opens a database from a path should handle potential exceptions when the database cannot be opened or the password is incorrect.
Consider wrapping the database opening in error handling:
public EncryptedDatabaseAdapter(String path, String password) {
- this.db = SQLiteDatabase.openDatabase(path, password, null, SQLiteDatabase.OPEN_READWRITE, null, null);
+ try {
+ this.db = SQLiteDatabase.openDatabase(path, password, null, SQLiteDatabase.OPEN_READWRITE, null, null);
+ } catch (net.zetetic.database.sqlcipher.SQLiteException e) {
+ throw new RuntimeException("Failed to open encrypted database at path: " + path, e);
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public EncryptedDatabaseAdapter(String path, String password) { | |
| this.db = SQLiteDatabase.openDatabase(path, password, null, SQLiteDatabase.OPEN_READWRITE, null, null); | |
| } | |
| public EncryptedDatabaseAdapter(String path, String password) { | |
| try { | |
| this.db = SQLiteDatabase.openDatabase(path, password, null, SQLiteDatabase.OPEN_READWRITE, null, null); | |
| } catch (net.zetetic.database.sqlcipher.SQLiteException e) { | |
| throw new RuntimeException("Failed to open encrypted database at path: " + path, e); | |
| } | |
| } |
🤖 Prompt for AI Agents
In app/src/org/commcare/models/database/EncryptedDatabaseAdapter.java around
lines 22-24, the constructor directly calls SQLiteDatabase.openDatabase which
can throw when the file is missing or the password is wrong; wrap the
openDatabase call in a try-catch that catches SQLiteException (or Exception),
log the error, clean up any partially opened resources, and throw a clear
checked or runtime exception (or rethrow with context) so callers can handle
failed opens; ensure the db field remains null on failure and include a helpful
message (e.g., path and reason) in the thrown/logged error.
| public long runCompileStatement(String sql) { | ||
| SQLiteStatement stmt = null; | ||
| try { | ||
| stmt = db.compileStatement(sql); | ||
| return stmt.simpleQueryForLong(); | ||
| } finally{ | ||
| stmt.close(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix potential NPE in runCompileStatement
The finally block attempts to close stmt which could be null if compileStatement throws an exception.
Apply this fix to prevent NPE:
@Override
public long runCompileStatement(String sql) {
SQLiteStatement stmt = null;
try {
stmt = db.compileStatement(sql);
return stmt.simpleQueryForLong();
- } finally{
- stmt.close();
+ } finally {
+ if (stmt != null) {
+ stmt.close();
+ }
}
}🤖 Prompt for AI Agents
In app/src/org/commcare/models/database/EncryptedDatabaseAdapter.java around
lines 83 to 91, the finally block calls stmt.close() which can throw a
NullPointerException if db.compileStatement(sql) fails and stmt remains null;
update the finally block to null-check stmt before calling close (or convert to
try-with-resources if Java version permits) so close() is only invoked when stmt
!= null.
| import android.database.sqlite.SQLiteException; | ||
|
|
||
| import net.sqlcipher.database.SQLiteDatabase; | ||
| import net.sqlcipher.database.SQLiteException; | ||
| import net.sqlcipher.database.SQLiteOpenHelper; | ||
| import net.zetetic.database.sqlcipher.SQLiteDatabase; | ||
| import net.zetetic.database.sqlcipher.SQLiteOpenHelper; | ||
|
|
There was a problem hiding this comment.
Catching the wrong SQLiteException class breaks cipher migration fallback
With net.zetetic classes, getWritableDatabase() throws net.zetetic.database.sqlcipher.SQLiteException, not android.database.sqlite.SQLiteException. As written, the catch won’t fire on SQLCipher errors, and DbUtil.trySqlCipherDbUpdate(...) won’t be invoked, risking startup crashes on cipher version mismatches.
Apply this diff to import and catch the correct exception:
-import android.database.sqlite.SQLiteException;
+import net.zetetic.database.sqlcipher.SQLiteException;No further code changes needed since the short name remains SQLiteException in the catch block.
Also applies to: 38-45
🤖 Prompt for AI Agents
In app/src/org/commcare/models/database/global/DatabaseGlobalOpenHelper.java
around lines 4-8 and 38-45, the code imports
android.database.sqlite.SQLiteException but SQLCipher throws
net.zetetic.database.sqlcipher.SQLiteException; replace the android import with
import net.zetetic.database.sqlcipher.SQLiteException (remove the
android.database.sqlite import) so the existing catch(SQLiteException e) blocks
will catch the correct exception and allow DbUtil.trySqlCipherDbUpdate(...) to
run.
| SqlStorage<UserKeyRecord> userDatabase = app.getStorage(UserKeyRecord.class); | ||
| for (UserKeyRecord user : userDatabase) { | ||
| File f = ccInstance.getDatabasePath(DatabaseUserOpenHelper.getDbName(user.getUuid())); | ||
| File f = ccInstance.getDatabasePath(UserDatabaseSchemaManager.getDbName(user.getUuid())); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prefer deleteDatabase over deleting the file path to remove sidecars reliably.
Using Context.deleteDatabase(name) removes the main file and -wal/-shm/-journal files; file deletion may leave sidecars behind and fail silently.
- File f = ccInstance.getDatabasePath(UserDatabaseSchemaManager.getDbName(user.getUuid()));
- if (!FileUtil.deleteFileOrDir(f)) {
+ String dbName = UserDatabaseSchemaManager.getDbName(user.getUuid());
+ if (!ccInstance.deleteDatabase(dbName)) {
Logger.log(LogTypes.TYPE_RESOURCES, "A user database was unable to be " +
"deleted during app uninstall. Aborting uninstall process for now.");
// If we failed to delete a file, it is likely because there is an open pointer
// to that db still in use, so stop the uninstall for now, and rely on it to
// complete the next time the app starts up
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| File f = ccInstance.getDatabasePath(UserDatabaseSchemaManager.getDbName(user.getUuid())); | |
| String dbName = UserDatabaseSchemaManager.getDbName(user.getUuid()); | |
| if (!ccInstance.deleteDatabase(dbName)) { | |
| Logger.log(LogTypes.TYPE_RESOURCES, "A user database was unable to be " + | |
| "deleted during app uninstall. Aborting uninstall process for now."); | |
| // If we failed to delete a file, it is likely because there is an open pointer | |
| // to that db still in use, so stop the uninstall for now, and rely on it to | |
| // complete the next time the app starts up | |
| return; | |
| } |
🤖 Prompt for AI Agents
In app/src/org/commcare/utils/AppLifecycleUtils.java around line 56, the code
deletes the DB by removing the File path (File f =
ccInstance.getDatabasePath(...)), which can leave -wal/-shm/-journal sidecar
files behind; replace that approach by calling Context.deleteDatabase(dbName)
using the same UserDatabaseSchemaManager.getDbName(user.getUuid()) value so
Android removes the main DB and any sidecars reliably, and remove any direct
File.delete() usage related to the database path.
|
|
||
| // 7) Delete the app database | ||
| File f = ccInstance.getDatabasePath(DatabaseAppOpenHelper.getDbName(app.getAppRecord().getApplicationId())); | ||
| File f = ccInstance.getDatabasePath(AppDatabaseSchemaManager.getDbName(app.getAppRecord().getApplicationId())); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Apply the same deleteDatabase approach for the app DB.
Consistent with user DB cleanup and ensures WAL/SHM removal.
- File f = ccInstance.getDatabasePath(AppDatabaseSchemaManager.getDbName(app.getAppRecord().getApplicationId()));
- if (!FileUtil.deleteFileOrDir(f)) {
+ String appDbName = AppDatabaseSchemaManager.getDbName(app.getAppRecord().getApplicationId());
+ if (!ccInstance.deleteDatabase(appDbName)) {
Logger.log(LogTypes.TYPE_RESOURCES, "The app database was unable to be deleted" +
"during app uninstall. Aborting uninstall process for now.");
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| File f = ccInstance.getDatabasePath(AppDatabaseSchemaManager.getDbName(app.getAppRecord().getApplicationId())); | |
| String appDbName = AppDatabaseSchemaManager.getDbName(app.getAppRecord().getApplicationId()); | |
| if (!ccInstance.deleteDatabase(appDbName)) { | |
| Logger.log(LogTypes.TYPE_RESOURCES, "The app database was unable to be deleted" + | |
| "during app uninstall. Aborting uninstall process for now."); | |
| return; | |
| } |
| import net.zetetic.database.DatabaseErrorHandler; | ||
| import net.zetetic.database.sqlcipher.SQLiteDatabase; | ||
| import net.zetetic.database.sqlcipher.SQLiteDatabase.CursorFactory; | ||
| import net.zetetic.database.sqlcipher.SQLiteDatabaseHook; | ||
| import net.zetetic.database.sqlcipher.SQLiteStatement; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix DatabaseErrorHandler package and wire it through to openDatabase(...)
- The import for DatabaseErrorHandler should come from android.database, not net.zetetic.database (which likely doesn’t exist and will cause linkage issues).
- The constructor that accepts a DatabaseErrorHandler currently ignores it; pass it through to the underlying openDatabase call to preserve error handling behavior.
Apply this diff:
- import net.zetetic.database.DatabaseErrorHandler;
+ import android.database.DatabaseErrorHandler;
@@
- public void __constructor__(String path, CursorFactory factory, int flags, DatabaseErrorHandler errorHandler) {
- db = android.database.sqlite.SQLiteDatabase.openDatabase(path, null, flags);
+ public void __constructor__(String path, CursorFactory factory, int flags, DatabaseErrorHandler errorHandler) {
+ db = android.database.sqlite.SQLiteDatabase.openDatabase(path, null, flags, errorHandler);
}Also applies to: 35-37
🤖 Prompt for AI Agents
In app/unit-tests/src/org/commcare/android/shadows/SQLiteDatabaseNative.java
around lines 8-12 and also affecting lines 35-37, replace the incorrect import
net.zetetic.database.DatabaseErrorHandler with
android.database.DatabaseErrorHandler, and update the constructor(s) that accept
a DatabaseErrorHandler so they no longer ignore that parameter: pass the
provided DatabaseErrorHandler into the underlying openDatabase(...) invocation
(or the equivalent delegate call) so the handler is preserved and used by the
opened database.
| * @author ctsims | ||
| */ | ||
| @Implements(net.sqlcipher.database.SQLiteDatabase.class) | ||
| @Implements(SQLiteDatabase.class) |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Make shadow method matching resilient to shaded types with looseSignatures
Given the target is a non-Android SDK class (net.zetetic...) and some method signatures/return types differ from android.* (e.g., Cursor, exceptions), enabling looseSignatures reduces brittleness across library versions and shaded types.
Apply this diff:
-@Implements(SQLiteDatabase.class)
+@Implements(value = SQLiteDatabase.class, looseSignatures = true)#!/bin/bash
# Display the top of the shadow file including imports and annotation
sed -n '1,50p' app/unit-tests/src/org/commcare/android/shadows/SQLiteDatabaseNative.java
# Show explicit import of SQLiteDatabase to confirm its package
grep -R "import .*SQLiteDatabase" -n app/unit-tests/src/org/commcare/android/shadows/SQLiteDatabaseNative.java
# Locate the @Implements line with context
grep -R "@Implements" -n app/unit-tests/src/org/commcare/android/shadows/SQLiteDatabaseNative.java -A2 -B2#!/bin/bash
# Search for any existing use of looseSignatures in the repo
grep -R "looseSignatures" -n .
# List all @Implements annotations to check for other patterns or usage
grep -R "@Implements" -n .Enable looseSignatures on Shadow SQLiteDatabase for better resilience
To ensure your shadow methods continue matching even when using shaded or variant SQLiteDatabase signatures, update the @Implements annotation:
• File: app/unit-tests/src/org/commcare/android/shadows/SQLiteDatabaseNative.java (around line 23)
- @Implements(SQLiteDatabase.class)
+ @Implements(value = SQLiteDatabase.class, looseSignatures = true)This makes the shadowing less brittle across library versions and shaded types.
🤖 Prompt for AI Agents
In app/unit-tests/src/org/commcare/android/shadows/SQLiteDatabaseNative.java
around line 23, the @Implements annotation should enable looseSignatures to make
the shadow resilient to shaded/variant SQLiteDatabase signatures; update the
annotation on that line to include the looseSignatures attribute (set it to
true) so the shadow methods match across library versions.
| @Override | ||
| public long runCompileStatement(String sql) { | ||
| SQLiteStatement stmt = null; | ||
| try { | ||
| stmt = db.compileStatement(sql); | ||
| return stmt.simpleQueryForLong(); | ||
| } finally{ | ||
| stmt.close(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix potential NullPointerException in runCompileStatement.
The finally block unconditionally calls stmt.close() which will throw NPE if compileStatement fails before assigning to stmt.
@Override
public long runCompileStatement(String sql) {
SQLiteStatement stmt = null;
try {
stmt = db.compileStatement(sql);
return stmt.simpleQueryForLong();
- } finally{
- stmt.close();
+ } finally {
+ if (stmt != null) {
+ stmt.close();
+ }
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Override | |
| public long runCompileStatement(String sql) { | |
| SQLiteStatement stmt = null; | |
| try { | |
| stmt = db.compileStatement(sql); | |
| return stmt.simpleQueryForLong(); | |
| } finally{ | |
| stmt.close(); | |
| } | |
| } | |
| @Override | |
| public long runCompileStatement(String sql) { | |
| SQLiteStatement stmt = null; | |
| try { | |
| stmt = db.compileStatement(sql); | |
| return stmt.simpleQueryForLong(); | |
| } finally { | |
| if (stmt != null) { | |
| stmt.close(); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In
app/unit-tests/src/org/commcare/models/database/UnencryptedDatabaseAdapter.java
around lines 81 to 90, the finally block always calls stmt.close() which can
throw a NullPointerException if db.compileStatement(sql) fails and stmt is still
null; change the finally block to only call stmt.close() when stmt is non-null
(or use try-with-resources if available) so the statement is closed safely
without risking NPE.
Showing Messaging menu option when user has at least one channel.
| @Override | ||
| protected boolean shouldShowDrawer() { | ||
| return true; | ||
| return false; |
There was a problem hiding this comment.
we need to show drawer on App Home
| private fun navigateToConnectMenu() { | ||
| val personalIdManager: PersonalIdManager = PersonalIdManager.getInstance() | ||
| personalIdManager.init(this) | ||
| personalIdManager.unlockConnect( | ||
| this | ||
| ) { success: Boolean -> | ||
| if (success) { | ||
| goToConnectJobsList(this) | ||
| setResult(RESULT_OK) | ||
| finish() | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
can we put all of this into ConnectNavHelper ?
There was a problem hiding this comment.
I created a couple helper methods in ConnectNavHelper to add the unlock code, now calling the helper methods wherever possible. ad5fbbc
| ) { success: Boolean -> | ||
| if (success) { | ||
| goToMessaging(this) | ||
| drawerController?.closeDrawer() |
There was a problem hiding this comment.
think the drawer logic should live out of this method so that we can move this method to ConnectNavHelper as well
…to jobs list or messaging. Using the new helper methods anywhere applicable.
Re-enabled side drawer in app home
…droid into dv/side_drawer_nav
https://dimagi.atlassian.net/browse/CCCT-1597
Product Description
Navigates to Connect opportunity list when user selects Opportunities from the side drawer menu.
Also addresses visibility of menu in different scenarios:
Technical Summary
Nothing special above what's described in the Product Description
Feature Flag
Side Drawer Navigation Menu
Safety Assurance
Safety story
Dev tested with:
-No apps installed vs. apps installed
Automated test coverage
None
QA Plan
Corner-cases: