fix(settings): enhance import data version compatibility with semantic versioning#226
Conversation
…c versioning Implements proper semantic version comparison for import data validation, allowing imports from older versions while blocking newer versions. Adds comprehensive test coverage for version compatibility scenarios and extends migration service with 0.16.0 to 0.20.0 schema updates including habit status, task recurrence, tag type, and tag order fields.
Summary of ChangesHello @ahmet-cetinkaya, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the robustness of the data import feature by introducing proper semantic version comparison. This change resolves an issue where minor version differences would incorrectly block data imports. Additionally, it includes a crucial data migration to handle schema changes between versions 0.16.0 and 0.20.0, ensuring that user data is correctly transformed and maintained during updates. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Reviewer's GuideAdds semantic-version-aware import compatibility checks and a new 0.16.0→0.20.0 data migration, plus tests that cover version comparison behavior and migration correctness. Sequence diagram for semantic-version-aware import validationsequenceDiagram
actor User
participant UI
participant ImportDataCommandHandler
participant SemanticVersion
participant ImportDataMigrationService
participant Database
User->>UI: Select import file
UI->>ImportDataCommandHandler: execute(importDataCommand)
ImportDataCommandHandler->>ImportDataCommandHandler: read importedVersion
ImportDataCommandHandler->>ImportDataMigrationService: needsMigration(importedVersion, AppInfo_version)
ImportDataMigrationService-->>ImportDataCommandHandler: migrationNeeded / none
alt migration needed
ImportDataCommandHandler->>ImportDataMigrationService: runMigrations(data, importedVersion, AppInfo_version)
ImportDataMigrationService->>Database: persist migrated data
Database-->>ImportDataMigrationService: ok
ImportDataMigrationService-->>ImportDataCommandHandler: migrated data
else no migration and versions differ
ImportDataCommandHandler->>SemanticVersion: parse(importedVersion)
SemanticVersion-->>ImportDataCommandHandler: importedSemVer
ImportDataCommandHandler->>SemanticVersion: parse(AppInfo_version)
SemanticVersion-->>ImportDataCommandHandler: currentSemVer
alt importedSemVer > currentSemVer
ImportDataCommandHandler-->>UI: throw BusinessException versionMismatchError
else importedSemVer <= currentSemVer
ImportDataCommandHandler->>Database: persist imported data
Database-->>ImportDataCommandHandler: ok
ImportDataCommandHandler-->>UI: ImportDataResult success
end
end
ER diagram for updated importable entities and relationserDiagram
HabitRecord {
int id
int status
}
Task {
int id
string recurrenceConfiguration
int plannedDateReminderCustomOffset
int deadlineDateReminderCustomOffset
}
Tag {
int id
int type
}
TaskTag {
int id
int tagOrder
}
NoteTag {
int id
int tagOrder
}
HabitTag {
int id
int tagOrder
}
AppUsageTag {
int id
int tagOrder
}
TaskTimeRecord {
int id
}
Task ||--o{ TaskTag : has
Tag ||--o{ TaskTag : classifies
Note ||--o{ NoteTag : has
Tag ||--o{ NoteTag : classifies
Habit ||--o{ HabitTag : has
Tag ||--o{ HabitTag : classifies
AppUsage ||--o{ AppUsageTag : has
Tag ||--o{ AppUsageTag : classifies
Task ||--o{ TaskTimeRecord : trackedBy
Class diagram for import migration service and semantic version import commandclassDiagram
class IImportDataMigrationService {
<<interface>>
+Future~Map~ migrate(Map data, String fromVersion, String toVersion)
}
class ImportDataMigrationService {
-MigrationRegistry _migrationRegistry
+ImportDataMigrationService()
+Future~Map~ migrate(Map data, String fromVersion, String toVersion)
-Future~Map~ _migrate0_15_0to0_16_0(Map data)
-Future~Map~ _migrate0_16_0to0_20_0(Map data)
}
class MigrationRegistry {
+void registerMigration(String fromVersion, String toVersion, String description, Future~Map~ migrationFunction(Map data))
+Future~Map~ runMigrations(Map data, String fromVersion, String toVersion)
+bool hasMigration(String fromVersion, String toVersion)
}
class ImportDataCommandHandler {
+Future~ImportDataResult~ handle(ImportDataCommand command)
-void _validateVersion(String importedVersion, String currentVersion)
}
class SemanticVersion {
+int major
+int minor
+int patch
+SemanticVersion(int major, int minor, int patch)
+static SemanticVersion parse(String value)
+int compareTo(SemanticVersion other)
+bool operator_greaterThan(SemanticVersion other)
+bool operator_lessThan(SemanticVersion other)
+bool operator_equals(SemanticVersion other)
}
class BusinessException {
+String message
+String translationKey
+Map args
+BusinessException(String message, String translationKey, Map args)
}
class AppInfo {
+static String version
}
IImportDataMigrationService <|.. ImportDataMigrationService
ImportDataMigrationService o-- MigrationRegistry
ImportDataCommandHandler --> IImportDataMigrationService : uses
ImportDataCommandHandler --> SemanticVersion : parses and compares
ImportDataCommandHandler --> BusinessException : throws
ImportDataCommandHandler --> AppInfo : reads version
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
ImportDataCommandHandler,SemanticVersion.parseis called on the imported version without guarding against invalid or non-semver strings; consider handling parse errors explicitly so a malformed backup file produces a clear, controlled error instead of an unhandled exception. - In
_migrate0_16_0to0_20_0,relationTablesincludestaskTimeRecordsbut then explicitly excludes it fromtagOrderhandling; either remove it from the list or document why it's treated differently to avoid confusion for future changes. - The migration verification tests hard-code specific version values (e.g.,
0.20.3) and expectations about whether migration is needed; consider tying these expectations more directly to the configured migration registry orAppInfo.versionsemantics to reduce brittleness when versions change.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `ImportDataCommandHandler`, `SemanticVersion.parse` is called on the imported version without guarding against invalid or non-semver strings; consider handling parse errors explicitly so a malformed backup file produces a clear, controlled error instead of an unhandled exception.
- In `_migrate0_16_0to0_20_0`, `relationTables` includes `taskTimeRecords` but then explicitly excludes it from `tagOrder` handling; either remove it from the list or document why it's treated differently to avoid confusion for future changes.
- The migration verification tests hard-code specific version values (e.g., `0.20.3`) and expectations about whether migration is needed; consider tying these expectations more directly to the configured migration registry or `AppInfo.version` semantics to reduce brittleness when versions change.
## Individual Comments
### Comment 1
<location> `src/lib/core/application/features/settings/services/import_data_migration_service.dart:319-322` </location>
<code_context>
+ if (!task.containsKey('recurrenceConfiguration')) {
+ task['recurrenceConfiguration'] = null;
+ }
+ if (!task.containsKey('plannedDateReminderCustomOffset')) {
+ task['plannedDateReminderCustomOffset'] = null;
+ }
+ if (!task.containsKey('deadlineDateReminderCustomOffset')) {
+ task['deadlineDateReminderCustomOffset'] = null;
+ }
</code_context>
<issue_to_address>
**nitpick:** Keep the migration description in sync with the fields actually handled here.
The doc comment for `_migrate0_16_0to0_20_0` only mentions `recurrenceConfiguration`, but this step also adds `plannedDateReminderCustomOffset` and `deadlineDateReminderCustomOffset`. Please update the description (or briefly justify why these fields are part of this migration) so future maintainers understand why they are introduced here.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if (!task.containsKey('plannedDateReminderCustomOffset')) { | ||
| task['plannedDateReminderCustomOffset'] = null; | ||
| } | ||
| if (!task.containsKey('deadlineDateReminderCustomOffset')) { |
There was a problem hiding this comment.
nitpick: Keep the migration description in sync with the fields actually handled here.
The doc comment for _migrate0_16_0to0_20_0 only mentions recurrenceConfiguration, but this step also adds plannedDateReminderCustomOffset and deadlineDateReminderCustomOffset. Please update the description (or briefly justify why these fields are part of this migration) so future maintainers understand why they are introduced here.
There was a problem hiding this comment.
Code Review
This pull request significantly improves the data import functionality by introducing semantic versioning for compatibility checks, which is a great enhancement. It correctly allows importing data from older compatible versions while preventing downgrades from newer versions. The addition of the 0.16.0 to 0.20.0 migration path is well-implemented and is accompanied by thorough and well-written tests that cover various scenarios, including version compatibility and data transformation verification. Overall, this is a solid contribution. I have a few minor suggestions to enhance error handling and code clarity.
| final importedSemVer = SemanticVersion.parse(importedVersion); | ||
| final currentSemVer = SemanticVersion.parse(AppInfo.version); |
There was a problem hiding this comment.
The SemanticVersion.parse() calls could throw a FormatException if the version string from the imported data is not a valid semantic version. This would be caught by the generic try-catch block in the call method, leading to a non-specific error message for the user. It would be better to handle this specific exception and throw a BusinessException with a more informative error, similar to the pattern used in ImportDataMigrationService.
| final importedSemVer = SemanticVersion.parse(importedVersion); | |
| final currentSemVer = SemanticVersion.parse(AppInfo.version); | |
| final SemanticVersion importedSemVer; | |
| final SemanticVersion currentSemVer; | |
| try { | |
| importedSemVer = SemanticVersion.parse(importedVersion); | |
| currentSemVer = SemanticVersion.parse(AppInfo.version); | |
| } catch (e) { | |
| throw BusinessException( | |
| 'Invalid version format in imported data', | |
| SettingsTranslationKeys.unsupportedVersionError, | |
| args: { | |
| 'version': importedVersion, | |
| }, | |
| ); | |
| } |
| for (var record in habitRecords) { | ||
| if (record is Map<String, dynamic>) { | ||
| if (!record.containsKey('status')) { | ||
| record['status'] = 0; // Default status: Pending/Skipped (0) |
There was a problem hiding this comment.
The comment // Default status: Pending/Skipped (0) is inconsistent with the HabitRecordStatus enum, where index 0 corresponds to complete. This could be confusing for future maintenance. The logic of defaulting to complete is correct, as the existence of a HabitRecord previously implied completion. The comment should be updated to reflect this.
| record['status'] = 0; // Default status: Pending/Skipped (0) | |
| record['status'] = 0; // Default status: complete (0) |
| } | ||
|
|
||
| // 4. Migrate Relations: Add tagOrder | ||
| final relationTables = ['taskTags', 'noteTags', 'habitTags', 'appUsageTags', 'taskTimeRecords']; |
There was a problem hiding this comment.
The relationTables list includes 'taskTimeRecords', but this table is intentionally skipped within the loop. To improve code clarity and avoid an unnecessary iteration, it's better to remove 'taskTimeRecords' from the list.
| final relationTables = ['taskTags', 'noteTags', 'habitTags', 'appUsageTags', 'taskTimeRecords']; | |
| final relationTables = ['taskTags', 'noteTags', 'habitTags', 'appUsageTags']; |
| Uint8List _createMockBackup({required String version}) { | ||
| final jsonData = jsonEncode({ | ||
| 'appInfo': {'version': version}, | ||
| 'tasks': [], | ||
| }); | ||
| return Uint8List.fromList(utf8.encode(jsonData)); | ||
| } |
There was a problem hiding this comment.
The helper function _createMockBackup is defined but is not used in any of the tests. This appears to be dead code and should be removed to keep the test file clean.
ahmet-cetinkaya
left a comment
There was a problem hiding this comment.
PR Review Summary
Overall Assessment
This PR implements semantic versioning for backup/restore compatibility, which addresses issue #219. The core logic is well-designed and follows Clean Architecture principles. However, there are critical error handling issues that must be addressed before merging, particularly around silent failures that could lead to data loss.
Recommendation: Request Changes - Critical issues must be fixed
Critical Issues (Must Fix)
1. Silent Failure in Pre-Import Backup Creation
File: src/lib/core/application/features/settings/commands/import_data_command.dart:204-218
The _createPreImportBackup() method catches all exceptions and returns null, allowing import to proceed without a backup. If the import then fails, users have no recovery path.
Current code:
try {
final backupFile = await dbInstance.createDatabaseBackup();
if (kDebugMode) {
print('Pre-import backup created: ${backupFile?.path}');
}
return backupFile;
} catch (e) {
if (kDebugMode) {
print('Failed to create pre-import backup: $e');
}
return null; // Silent failure!
}Suggested fix:
final backupFile = await dbInstance.createDatabaseBackup();
if (kDebugMode) {
print('Pre-import backup created: ${backupFile?.path}');
}
return backupFile ?? throw BusinessException(
'Failed to create pre-import backup. Cannot proceed with import.',
SettingsTranslationKeys.backupCreateError,
);2. Silent Data Loss from Empty IDs
File: src/lib/core/application/features/settings/commands/import_data_command.dart:512-525
Items with missing/empty IDs are silently skipped during import. Users get a "successful" import but are missing data.
Suggested fix:
int skippedCount = 0;
for (var item in items) {
if (item['id'] == null || (item['id'] is String && (item['id'] as String).isEmpty)) {
skippedCount++;
Logger.warning('Skipping item with missing ID in ${config.name}');
continue;
}
// ... rest of import logic
}
// Throw if too many items skipped (indicates corrupted backup)
if (skippedCount > items.length * 0.1) {
throw BusinessException(
'Too many items ($skippedCount/${items.length}) with missing IDs in ${config.name}',
SettingsTranslationKeys.importDataIntegrityError,
);
}3. Invalid Version Returns False Instead of Throwing
File: src/lib/core/application/features/settings/services/import_data_migration_service.dart:140-143
When isMigrationNeeded() encounters an invalid version format, it returns false. The import proceeds without migration, causing potential data corruption.
Suggested fix:
} catch (e, stackTrace) {
Logger.error(
'Invalid version format: $sourceVersion',
error: e,
stackTrace: stackTrace,
);
throw BusinessException(
'Invalid version format in backup data: $sourceVersion',
SettingsTranslationKeys.backupInvalidFormatError,
args: {'version': sourceVersion},
);
}4. Overly Broad Catch Block
File: src/lib/core/application/features/settings/commands/import_data_command.dart:370-378
Catches all exceptions but throws a generic "Invalid version format", hiding the root cause (whether it's FormatException, NullReferenceError, etc.).
Suggested fix:
try {
importedSemVer = SemanticVersion.parse(importedVersion);
currentSemVer = SemanticVersion.parse(AppInfo.version);
} on FormatException catch (e) {
throw BusinessException(
'Invalid version format in imported data',
SettingsTranslationKeys.backupInvalidFormatError,
args: {'version': importedVersion},
);
} catch (e, stackTrace) {
Logger.error('Unexpected error parsing versions', error: e, stackTrace: stackTrace);
throw BusinessException(
'Failed to parse version information',
SettingsTranslationKeys.versionParseError,
);
}5. Missing Production Error Logging
File: src/lib/core/application/features/settings/commands/import_data_command.dart:434-445
Top-level catch only logs in debug mode. Production import failures have no traceability.
Suggested fix:
} catch (e, stackTrace) {
Logger.error(
'Import failed with strategy: ${request.strategy}',
error: e,
stackTrace: stackTrace,
);
if (kDebugMode) {
print('CRITICAL: Import failed: $e');
}
rethrow;
}Test Coverage Gaps (Should Add)
Invalid Version Format Not Tested
The critical error path for malformed version strings is not tested. This is a high-risk scenario.
Test needed:
test('should throw BusinessException when version format is invalid', () async {
when(mockCompressionService.extractFromWhphFile(any)).thenAnswer((_) async {
return jsonEncode({'appInfo': {'version': 'invalid.version.format'}, 'tasks': []});
});
expect(
() => handler.call(ImportDataCommand(backupData, ImportStrategy.replace)),
throwsA(predicate((e) =>
e is BusinessException &&
e.errorCode == SettingsTranslationKeys.backupInvalidFormatError))
);
});Also missing tests for:
- Migration error logging verification
- Null/missing collections in backup data
- All relation tables (noteTags, habitTags, appUsageTags) - only taskTags is tested
Positive Observations
- Semantic version comparison logic is well-implemented
- Clean Architecture and CQRS patterns followed correctly
- New 0.16.0 → 0.20.0 migration properly handles schema changes
- Tests follow good naming conventions
- Idempotent migration logic (checks
containsKey()before adding fields)
Recommended Next Steps
- Fix the 5 critical issues above
- Add tests for invalid version format error path
- Re-run review after fixes are complete
Improve error handling in import data command with proper logging and business exception throwing. Add comprehensive validation for version parsing, backup creation, and data integrity checks. Update translation keys and locale files to support new error messages. Enhance test coverage for error scenarios including invalid version formats and corrupted backups.
🚀 Motivation and Context
Fixes #219 - Implements proper semantic version comparison for import data validation. Previously, importing data from any older version (even patch versions) would fail with a version mismatch error if the version strings didn't match exactly.
⚙️ Implementation Details
import_data_command.dart): AddedSemanticVersioncomparison logic to distinguish between compatible older versions (allow import) and newer versions (block import to prevent downgrade)import_data_migration_service.dart): Extended with 0.16.0 → 0.20.0 migration handling schema changes:statusfield (default: 0)recurrenceConfigurationand reminder offset fields (default: null)typefield (default: 0 - Label)tagOrderfield (default: 0)📋 Checklist for Reviewer
🔗 Related
Summary by Sourcery
Improve backup import compatibility by adding a new data migration path and using semantic versioning to validate imported backup versions.
Bug Fixes:
Enhancements:
Tests: