fix: preserve model JSON in backups#1224
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughBackupV2 in Possibly related PRs
Suggested labels: bug, test Suggested reviewers: lollipopkit 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/backup_v2_test.dart (1)
95-112: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTest doesn't actually verify internal-key skipping.
_validateRestorableStoreskips an entry if_isInternalStoreKey(entry.key)orentry.value is Map. Here'__lkpt_lastUpdateTs'is given aMapvalue ({'server': 1}), so the entry would pass validation regardless of whether it's recognized as an internal key — the test doesn't exercise the internal-key branch at all. To actually prove internal keys are skipped, use a non-Mapvalue (which would otherwise trigger theFormatException) for the internal key.✅ Proposed fix to actually exercise the internal-key skip branch
test('allows internal metadata in typed stores', () { final raw = json.encode({ 'version': BackupV2.formatVer, 'date': 1, 'spis': { - '__lkpt_lastUpdateTs': {'server': 1}, + '__lkpt_lastUpdateTs': 1234567890, }, 'snippets': {}, 'keys': {}, 'container': {}, 'history': {}, 'settings': {}, }); final backup = BackupV2.fromJsonString(raw); - expect(backup.spis['__lkpt_lastUpdateTs'], {'server': 1}); + expect(backup.spis['__lkpt_lastUpdateTs'], 1234567890); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/backup_v2_test.dart` around lines 95 - 112, The test in BackupV2.fromJsonString is not actually exercising the internal-key skip branch because the value for '__lkpt_lastUpdateTs' is a Map, which already bypasses validation. Update the test data so the internal store key uses a non-Map value that would normally fail _validateRestorableStore, then assert BackupV2 still parses it successfully; this should specifically verify the _isInternalStoreKey path rather than the generic Map exception.lib/data/model/app/bak/backup2.dart (1)
157-167: 🩺 Stability & Availability | 🔵 Trivial | ⚖️ Poor tradeoffBroad
NoSuchMethodErrorcatch can mask unrelated bugs insidetoJson().
(value as dynamic).toJson()catches anyNoSuchMethodError, not just the case wheretoJson()itself is missing. If a type does implementtoJson()but that method internally triggers aNoSuchMethodError(e.g. calling a dynamic method on a nested field, or a null dynamic receiver), this handler will misreport it as "missing toJson()" and swallow the real stack trace/cause, making the actual bug harder to diagnose.Consider checking for a shared
toJsoncapability more precisely (e.g., a marker interfaceJsonable { Map<String, dynamic> toJson(); }on the model types actually passed here, or checkingvalue is Map/value is Functiongates before falling back to dynamic dispatch) rather than relying on catchingNoSuchMethodErroras a proxy for "no method".♻️ Possible improvement using an interface instead of dynamic dispatch
-Object? _toEncodable(Object? value) { - if (value is Enum) return value.name; - - try { - return (value as dynamic).toJson(); - } on NoSuchMethodError { - throw UnsupportedError( - 'Cannot JSON-encode ${value.runtimeType}: missing toJson()', - ); - } -} +Object? _toEncodable(Object? value) { + if (value is Enum) return value.name; + if (value is JsonEncodable) return value.toJson(); + + throw UnsupportedError( + 'Cannot JSON-encode ${value.runtimeType}: missing toJson()', + ); +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/data/model/app/bak/backup2.dart` around lines 157 - 167, The _toEncodable helper is catching any NoSuchMethodError from dynamic toJson() calls, which can hide real bugs inside valid toJson implementations. Update _toEncodable in backup2.dart to avoid using NoSuchMethodError as a proxy for “missing toJson()”; instead, use a more explicit capability check such as a shared Jsonable interface for model types, or otherwise narrow the dynamic fallback so only unsupported values reach the UnsupportedError path. Keep the Enum.name handling intact and preserve the original stack trace for genuine toJson() failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@lib/data/model/app/bak/backup2.dart`:
- Around line 157-167: The _toEncodable helper is catching any NoSuchMethodError
from dynamic toJson() calls, which can hide real bugs inside valid toJson
implementations. Update _toEncodable in backup2.dart to avoid using
NoSuchMethodError as a proxy for “missing toJson()”; instead, use a more
explicit capability check such as a shared Jsonable interface for model types,
or otherwise narrow the dynamic fallback so only unsupported values reach the
UnsupportedError path. Keep the Enum.name handling intact and preserve the
original stack trace for genuine toJson() failures.
In `@test/backup_v2_test.dart`:
- Around line 95-112: The test in BackupV2.fromJsonString is not actually
exercising the internal-key skip branch because the value for
'__lkpt_lastUpdateTs' is a Map, which already bypasses validation. Update the
test data so the internal store key uses a non-Map value that would normally
fail _validateRestorableStore, then assert BackupV2 still parses it
successfully; this should specifically verify the _isInternalStoreKey path
rather than the generic Map exception.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c01d2ba3-453d-47b6-8833-435dbdafbaef
📒 Files selected for processing (2)
lib/data/model/app/bak/backup2.darttest/backup_v2_test.dart
1 participant
#1223
Summary by CodeRabbit
UnsupportedErrorinstead of being silently stringified.FormatException, while preserving required internal metadata fields.