Import and export data#34
Conversation
IamDg
left a comment
There was a problem hiding this comment.
Hi @odweta,
Thank you for taking the time to contribute!
While the raw SQL backup implementation is performant, I believe we should move toward a JSON-based architecture for the following reasons:
- Raw SQL files are tightly coupled to the database schema, so migrations across different app versions become fragile.
- Raw SQL is not human-readable, while JSON is.
- Raw imports require a full file-system swap and an app restart (not ideal for UX). JSON allows granular operations (e.g., single workouts) without affecting the entire database.
- The current approach only supports overwriting, not merging with the existing DB or other operations.
For these reasons, I think a more robust and idiomatic approach is to use JSON for imports/exports and implement a clear merge strategy.
Let me know what you think!
P.S.: Please merge main before proceeding. Thanks!
|
Hi @IamDg, Thank you for your suggestions! I see your point with using a JSON-based architecture for backing up data, as it is more "modular" and allows for more granular control while importing into the database -> fine-grain entity operations instead of overwriting the whole database. I have a question though about importing a backup made from an old migration into a new version of the app with a DB with a newer migration. Would it cause conflicts? If so, what could be a viable strategy to utilize here? Also, I have modified strings.xml directly but only for the "default" English translation while the translation guide states that contributors should not modify them directly. I just want to know whether I took the right approach or not.
Looking forward to your reply! |
I think the best solution is to include the schema version in the JSON so parsing can be based on that (errors can be handled gracefully). Implement migrations for each schema version and chain them together to be compatible with the current installation.
That warning is for translators who might open a PR on GitHub to submit translations in the base file |
|
Hey @IamDg, I have just committed changes that should fulfill your requirements. Exporting now creates a versioned JSON file and importing makes sure the target file is up-to-date on the version, then it upserts the entries into the SQLite DB -- e.g. previously present entries do not get overwritten. I have tested it on a small sample dataset and both functionalities seem to be working just fine. Though, if you want to, I can try to add some test coverage via unit tests on top of that. Let me know what you think! |
IamDg
left a comment
There was a problem hiding this comment.
Thank you for your work on this feature.
I have reviewed the implementation and would like to suggest some architectural refinements to ensure this feature is robust, maintainable, and aligned with the project's reactive architecture.
- Architectural Integrity & Reactive Data Flow
- Issue: The current import process uses application-level restarts.
- Recommendation: Remove the restart logic.
- Reasoning: LibreFit leverages Room and Flow to observe data changes. When the database is updated, the UI layer will automatically reflect those changes. A manual restart provides a poor user experience and is unnecessary. Please rely on standard UI feedback to inform the user that the import is complete.
- Introduce Data Transfer Objects (DTOs)
- The Issue: It appears the Room entities are being serialized directly.
- Recommendation: Introduce explicit Data Transfer Objects (DTOs) for JSON serialization, annotated with @serializable.
- Reasoning: Room entities are tied to our database schema. Decoupling them via DTOs ensures that future database changes do not break our user's ability to restore their old backups. It creates a stable "contract" for the export format.
- Formalize Schema Versioning
- The Issue: The export version is currently hardcoded or managed inconsistently.
- Recommendation: Implement an "envelope" structure for the JSON output that includes a schema_version.
- Reasoning: As the app grows, our database schema will evolve. A versioned envelope allows us to write "upgrader" functions to transform older JSON structures into the current format during import, ensuring backward compatibility and preventing data loss.
- Unified DAO Naming & Responsibilities
- The Issue: There is some inconsistency in naming between MeasurementDao and DatasetDao for Flow (reactive) vs suspend (one-shot) methods.
- Recommendation: Adopt a consistent naming convention:
- Reasoning: This improves code readability and makes the intent of each method immediately clear to other developers.
- Enforce Transactional Integrity
- The Issue: The addition of generic @upsert methods in WorkoutDao.
- Recommendation: I am concerned that these methods bypass our Aggregate Root logic. Please use the existing addWorkoutWithExercisesAndSets transaction method for all Workout-related saves. If the generic methods are not strictly required for the Repository implementation, please remove them to reduce API clutter and prevent accidental data corruption.
- Reasoning: The addWorkoutWithExercisesAndSets method handles orphaned record cleanup and transactional consistency. Bypassing it could lead to an inconsistent database state. All Workout-related writes must pass through this transactional gatekeeper.
- Serialization Refinement
- The Issue: Using @contextual for LocalDateTime.
- Recommendation: Please use @serializable(with = LocalDateTimeSerializer::class) explicitly on the data class field.
- Reasoning: This is a safer, more explicit approach that doesn't rely on global SerializersModule registration. It makes the code self-documenting and resilient against configuration errors.
The feature is well-started, but we need to ensure it is "future-proofed" against schema changes and adheres to the app's reactive nature. Moving toward an envelope-based versioning system and strictly enforcing the repository-level transaction logic will significantly improve the long-term reliability of this feature.
Please let me know if you have any questions regarding these recommendations. I am happy to discuss the implementation details further!
|
|
||
| class ImportExportRepository @Inject constructor( | ||
| private val db: AppDatabase, | ||
| @ApplicationContext private val context: Context |
There was a problem hiding this comment.
| @ApplicationContext private val context: Context | |
| @param:ApplicationContext private val context: Context |
| } | ||
| } | ||
|
|
||
| suspend fun importFrom(uri: Uri) = withContext(Dispatchers.IO) { |
There was a problem hiding this comment.
Remember to inject the dispatcher ioDispatcher in the constructor marked with param:@IoDispatcher (whichis from DispatcherModule.kt ( https://github.com/LibreFitOrg/LibreFit/blob/main/app/src/main/java/org/librefit/di/DispatcherModule.kt#L27 )
| suspend fun importFrom(uri: Uri) = withContext(Dispatchers.IO) { | |
| suspend fun importFrom(uri: Uri) = withContext(ioDispatcher) { |
| material = { group = "com.google.android.material", name = "material", version.ref = "material" } | ||
| coil = { group = "io.coil-kt.coil3", name = "coil-compose", version.ref = "coil" } | ||
| reorderable = { module = "sh.calvin.reorderable:reorderable", version.ref = "reorderable" } | ||
| androidx-compose-foundation-layout = { group = "androidx.compose.foundation", name = "foundation-layout", version.ref = "foundationLayout" } |
There was a problem hiding this comment.
See above
| androidx-compose-foundation-layout = { group = "androidx.compose.foundation", name = "foundation-layout", version.ref = "foundationLayout" } |
| material = "1.14.0-beta01" | ||
| coil = "3.4.0" | ||
| reorderable = "3.1.0" | ||
| foundationLayout = "1.11.0" |
There was a problem hiding this comment.
See above
| foundationLayout = "1.11.0" |
|
|
||
| dependencies { | ||
|
|
||
| implementation(libs.androidx.compose.foundation.layout) |
There was a problem hiding this comment.
Why this is added? It is redundant because it's already included in implementation(platform(libs.androidx.compose.bom))
| implementation(libs.androidx.compose.foundation.layout) |
| fun getAllMeasurements(): Flow<List<Measurement>> | ||
|
|
||
| @Query("SELECT * FROM measurements ORDER BY date DESC") | ||
| suspend fun getAllMeasurementsForBackup(): List<Measurement> |
There was a problem hiding this comment.
Use standard naming conventions
| suspend fun getAllMeasurementsForBackup(): List<Measurement> | |
| suspend fun getAllMeasurementsOnce(): List<Measurement> |
| fun getDataset(): Flow<List<ExerciseDC>> | ||
|
|
||
| @Query("SELECT * FROM dataset ORDER BY name") | ||
| suspend fun getAllExerciseDCs(): List<ExerciseDC> |
There was a problem hiding this comment.
Use standard naming conventions
| suspend fun getAllExerciseDCs(): List<ExerciseDC> | |
| suspend fun getDatasetOnce(): List<ExerciseDC> |
| @get:FloatRange(0.0, 300.0) val bodyWeight: Double = 0.0, | ||
| @get:IntRange(0, 100) val bodyFatPercentage: Int = 0, | ||
| @get:IntRange(0, 100) val muscleMassPercentage: Int = 0, | ||
| @Contextual |
There was a problem hiding this comment.
Use this serializer LocalDateTimeSerializer ( https://github.com/LibreFitOrg/LibreFit/blob/main/app/src/main/java/org/librefit/db/entity/LocalDateTimeSerializer.kt ) instead:
| @Contextual | |
| @Serializable(with = LocalDateTimeSerializer::class) |
|
Sorry, I accidentally deleted the branch and that closed this PR. I reopened it. |
|
Thank you for the patience with me on this one. I believe I have followed through and implemented your suggestions. The export and import processes emit events now and simple snackbars are shown to signal the results. The app now does not restart and utilizes Flows and observing them. Let me know your stance on this! |
IamDg
left a comment
There was a problem hiding this comment.
Hi,
Sorry for late reply!
Firstly, I'd suggest you to merge conflicts with main branch before proceding further in order to avoid work later.
Secondly, the PR itself is much better than previous iteration but it needs major corrections/improvements.
Thank you for taking the time to contribute!
|
Hi, I resolved all of your open conversations, please let me know if the current state of my changes is up to the app's standards now. Thanks, looking forward to your review! |
Solves #3.
Added the ability to import and export the app's data by extracting and importing the SQLite database.
This feature can be used by interacting with two new buttons at SettingsScreen.