13730 inconsistent behaviour of sormas mobile application#13749
Conversation
WalkthroughThis PR extends the Symptoms data model by introducing database schema version 360 with 16 new symptom-related columns and corresponding Java entity properties. Changes include database migration, entity getter/setters, and DTO helper mappings to synchronize new fields between database and application objects. Additionally, contact DAO jurisdiction filter logic is refactored for defensive null handling. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sormas-app/app/src/main/java/de/symeda/sormas/app/backend/symptoms/Symptoms.java (1)
652-654: Fix incorrect method signature - getter named but acts as setter.This method is named as a getter but takes a parameter and assigns it to the field, which is setter behavior. Since a proper getter exists at line 656 and a proper setter exists at line 2196, this method appears to be a duplicate setter with an incorrect name.
Apply this diff to remove the incorrectly named method:
- public void getNocturnalCough(SymptomState nocturnalCough) { - this.nocturnalCough = nocturnalCough; - } -
🧹 Nitpick comments (1)
sormas-app/app/src/main/java/de/symeda/sormas/app/backend/symptoms/SymptomsDtoHelper.java (1)
462-465: Consider returning a realistic estimate from getApproximateJsonSizeInBytesOverriding
getApproximateJsonSizeInBytesto always return0effectively disables size estimation forSymptomspayloads. If the sync layer relies on this for batching or progress reporting, you might get sub‑optimal behaviour.If other DTO helpers provide a non‑zero estimate, consider aligning with them (even a rough constant based on typical serialized size per record).
Please double‑check how
getApproximateJsonSizeInBytesis used in the sync layer and whether0has any side effects beyond minor performance/UX impacts.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
sormas-app/app/src/main/java/de/symeda/sormas/app/backend/common/DatabaseHelper.java(3 hunks)sormas-app/app/src/main/java/de/symeda/sormas/app/backend/contact/ContactDao.java(3 hunks)sormas-app/app/src/main/java/de/symeda/sormas/app/backend/symptoms/Symptoms.java(2 hunks)sormas-app/app/src/main/java/de/symeda/sormas/app/backend/symptoms/SymptomsDtoHelper.java(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: android app test (28)
- GitHub Check: android app test (27)
- GitHub Check: android app test (26)
- GitHub Check: SORMAS CI
🔇 Additional comments (4)
sormas-app/app/src/main/java/de/symeda/sormas/app/backend/common/DatabaseHelper.java (1)
193-193: Schema upgrade to 360 matches new Symptoms fields; check server/mobile consistencyThe bump to
DATABASE_VERSION = 360plus thecase 360block cleanly adds all newSymptomscolumns:
- acuteEncephalitis, eggyBurps, weightLoss, weightLossAmount
- reoccurrence, symptomCurrentStatus, durationOfSymptoms
- overnightStayRequired, bloating, difficultyBreathingDuringMeals
- paradoxicalBreathing, respiratoryFatigue
- parentTimeOffWork, timeOffWorkDays
- unknownSymptom, skinRashOnsetDate
Types (
varchar(255),float8,int,timestamp) look consistent with how similar fields are defined elsewhere in this file, and thebreakimmediately aftercase 360is correct as the last version.Please verify:
- The
Symptomsentity and API DTO define matching fields for all these columns.- Backend/server DB migrations use the same column names and types so that mobile and server schemas stay in sync.
A quick grep over the repo for each new column name inSymptoms.javaand the server migration scripts should be sufficient.Also applies to: 3183-3225
sormas-app/app/src/main/java/de/symeda/sormas/app/backend/contact/ContactDao.java (1)
221-234: No external callers found—original concern unfounded; code change is correctVerification confirms that
createJurisdictionFilterForCaseandcreateJurisdictionFilterare only called internally within ContactDao (lines 223–224) and nowhere else in the codebase. The methods are explicitly designed to returnnullwhen no jurisdiction conditions apply (lines 254, 308, 301, 336). The new null-handling logic at lines 221–234 correctly implements safe OR composition and properly falls back to individual clause additions. The original review's concern about external callers adapting to the null-returning change does not apply.Likely an incorrect or invalid review comment.
sormas-app/app/src/main/java/de/symeda/sormas/app/backend/symptoms/Symptoms.java (2)
476-509: LGTM! Field declarations are properly annotated.The new symptom-related fields are correctly declared with appropriate ORMLite annotations and data types.
2083-2214: LGTM! New getter/setter methods are correctly implemented.All new symptom-related fields have properly implemented accessor methods following JavaBean conventions with appropriate return types and parameter types.
| int currentVersion = oldVersion; | ||
| try { | ||
| switch (oldVersion) { | ||
| switch (newVersion) { |
There was a problem hiding this comment.
🧩 Analysis chain
Using switch (newVersion) in onUpgrade is likely incorrect and breaks multi‑step upgrades
onUpgrade is called once with (oldVersion, newVersion) by SQLiteOpenHelper. With the current code:
int currentVersion = oldVersion;
switch (newVersion) {
case 91:
currentVersion = 91;
// ...
// ...
case 360:
currentVersion = 360;
// ...
break;
}When upgrading from, say, oldVersion = 200 to newVersion = 360, this switch enters at case 360 and skips all upgrade steps 201–359, which still need to be applied. The previous pattern with switch (oldVersion) (or switch (currentVersion)) and fall‑through is the standard way to ensure all intermediate migrations run.
Unless you have custom code that calls onUpgrade repeatedly for each intermediate version (which would be very unusual and is not visible here), this change will result in inconsistent schemas for users upgrading from older app versions.
I strongly recommend reverting this to switch on the starting version (or currentVersion) so that all cases from oldVersion+1 up to newVersion execute via fall‑through.
To be safe, please:
- Re‑check the previous implementation of
onUpgradein this class. - Confirm via Android docs or a small test app that
SQLiteOpenHelper.onUpgradeis only called once per upgrade with the finalnewVersion, and not iteratively.
🌐 Web query:
How does Android's SQLiteOpenHelper.onUpgrade get invoked with oldVersion and newVersion, and what is the recommended pattern for running incremental schema migrations?
💡 Result:
Short answer:
-
When you call getWritableDatabase() or getReadableDatabase() and the on-disk DB version (PRAGMA user_version) is less than the version you passed into your SQLiteOpenHelper constructor, Android opens the DB and invokes onUpgrade(db, oldVersion, newVersion) inside a transaction to move the schema from oldVersion to newVersion. onCreate is used only when the DB file does not yet exist. [1]
-
Recommended pattern: perform incremental, idempotent migration steps from the current version up to the target version—apply each intermediate migration in sequence (e.g. for (v = oldVersion+1; v <= newVersion; v++) { switch(v) { case 2: /* migrate 1→2 /; break; case 3: / migrate 2→3 */; break; ... } }). This ensures upgrades work for any skip-upgrade (e.g. 1→4) and keeps each step small and testable. Do not alter a released migration step for a given from→to pair; if it was incorrect, add a new migration to fix it. [1][2][3]
References
- Android SQLiteOpenHelper docs (onCreate/onUpgrade behavior, transaction guarantee, and migration guidance). [1]
- Common community patterns / examples showing loop or switch/fall‑through incremental upgrades (StackOverflow / tutorials). [2][3]
References list
[1] Android Developers — SQLiteOpenHelper (onUpgrade/onCreate/onConfigure).
[2] Stack Overflow examples: looping/switch pattern for incremental onUpgrade.
[3] Tutorials documenting switch/for-loop migration pattern for SQLiteOpenHelper.
I need to inspect the actual code to verify the severity and exact issue. Let me examine the DatabaseHelper.java file around the reported lines.
---
🏁 Script executed:
```shell
#!/bin/bash
# Examine the onUpgrade method in DatabaseHelper.java around line 401
find . -name "DatabaseHelper.java" -type f | head -5
Length of output: 86
🏁 Script executed:
#!/bin/bash
# Once we find the file, examine the onUpgrade method structure
find . -name "DatabaseHelper.java" -path "*/backend/common/*" -type f -exec head -450 {} \; | tail -100Length of output: 5221
🏁 Script executed:
#!/bin/bash
# Search for the onUpgrade method definition and structure
rg -n "onUpgrade" --type=java -A 100 | head -200Length of output: 29032
🏁 Script executed:
#!/bin/bash
# Get specific context around line 401
fd "DatabaseHelper.java" | xargs -I {} sh -c 'sed -n "350,450p" "{}"'Length of output: 5287
Fix critical database migration bug: switch must target oldVersion, not newVersion
When you call getWritableDatabase() or getReadableDatabase() and the on-disk DB version is less than the version you passed into your SQLiteOpenHelper constructor, Android invokes onUpgrade(db, oldVersion, newVersion) inside a transaction to move the schema from oldVersion to newVersion. The current code uses switch (newVersion) at line 401, which causes intermediate migrations to be skipped.
Example: upgrading from version 200 to 360 will jump directly to case 360: and skip all migrations from 201–359, leaving the schema in an inconsistent state.
The recommended pattern is to perform incremental, idempotent migration steps from the current version up to the target version—apply each intermediate migration in sequence (e.g. for (v = oldVersion+1; v <= newVersion; v++) { switch(v) { case 2: /* migrate 1→2 /; break; case 3: / migrate 2→3 */; break; ... } }).
Change the switch to iterate from oldVersion + 1 to newVersion, or switch on oldVersion to ensure fall-through executes all required steps.
🤖 Prompt for AI Agents
In
sormas-app/app/src/main/java/de/symeda/sormas/app/backend/common/DatabaseHelper.java
around line 401, the code currently switches on newVersion which skips
intermediate migration steps; change the logic to perform incremental migrations
from oldVersion+1 through newVersion (for example: loop v from oldVersion+1 to
newVersion and switch on v to apply each migration case), ensuring each
migration step is idempotent and applied in sequence so no intermediate versions
are skipped.
| target.setAcuteEncephalitis(source.getAcuteEncephalitis()); | ||
| target.setEggyBurps(source.getEggyBurps()); | ||
| target.setWeightLoss(source.getWeightLoss()); | ||
| target.setWeightLossAmount(source.getWeightLossAmount()); | ||
| target.setReoccurrence(source.getReoccurrence()); | ||
| target.setSymptomCurrentStatus(source.getSymptomCurrentStatus()); | ||
| target.setDurationOfSymptoms(source.getDurationOfSymptoms()); | ||
| target.setOvernightStayRequired(source.getOvernightStayRequired()); | ||
| target.setBloating(source.getBloating()); | ||
| target.setDifficultyBreathing(source.getDifficultyBreathing()); | ||
| target.setParadoxicalBreathing(source.getParadoxicalBreathing()); | ||
| target.setRespiratoryFatigue(source.getRespiratoryFatigue()); | ||
| target.setParentTimeOffWork(source.getParentTimeOffWork()); | ||
| target.setTimeOffWorkDays(source.getTimeOffWorkDays()); | ||
| target.setUnknownSymptom(source.getUnknownSymptom()); | ||
| target.setSkinRashOnsetDate(source.getSkinRashOnsetDate()); | ||
| target.setAsymptomatic(source.getAsymptomatic()); |
There was a problem hiding this comment.
New difficultyBreathingDuringMeals field is not mapped; duplicate difficultyBreathing call is likely a bug
The new DB column and model field are difficultyBreathingDuringMeals, but both DTO⇄entity mappings added here call setDifficultyBreathing a second time. This leaves difficultyBreathingDuringMeals unmapped and can cause lost or never‑synced data for that field.
Suggest correcting both directions:
@@ fillInnerFromDto
- target.setBloating(source.getBloating());
- target.setDifficultyBreathing(source.getDifficultyBreathing());
+ target.setBloating(source.getBloating());
+ target.setDifficultyBreathingDuringMeals(source.getDifficultyBreathingDuringMeals());
@@ fillInnerFromAdo
- target.setBloating(source.getBloating());
- target.setDifficultyBreathing(source.getDifficultyBreathing());
+ target.setBloating(source.getBloating());
+ target.setDifficultyBreathingDuringMeals(source.getDifficultyBreathingDuringMeals());All other newly added fields (acuteEncephalitis, eggyBurps, weightLoss, weightLossAmount, reoccurrence, symptomCurrentStatus, durationOfSymptoms, overnightStayRequired, bloating, paradoxicalBreathing, respiratoryFatigue, parentTimeOffWork, timeOffWorkDays, unknownSymptom, skinRashOnsetDate, asymptomatic) are mapped symmetrically, which is good.
Also applies to: 441-457
🤖 Prompt for AI Agents
In
sormas-app/app/src/main/java/de/symeda/sormas/app/backend/symptoms/SymptomsDtoHelper.java
around lines 238-254 (and the symmetric mapping at 441-457), the mapping
mistakenly calls setDifficultyBreathing twice so difficultyBreathingDuringMeals
is never mapped; change the duplicate setDifficultyBreathing call to use the
difficultyBreathingDuringMeals accessor/mutator (use
source.getDifficultyBreathingDuringMeals when copying into target and
target.setDifficultyBreathingDuringMeals when copying back) so the new field is
mapped symmetrically like the other fields.
Fixes #
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.