From 157a85acbc1b1b61addd6a0470cdeaa5a9d39304 Mon Sep 17 00:00:00 2001 From: Herbert Poul Date: Mon, 16 Aug 2021 10:14:29 +0200 Subject: [PATCH] - Mark objects only as clean when saving was successful. - Only mark objects as clean if they have not been modified since we started saving. --- CHANGELOG.md | 5 +++ lib/src/kdbx_entry.dart | 7 ++-- lib/src/kdbx_file.dart | 16 +++++++--- lib/src/kdbx_format.dart | 16 ++++++++-- lib/src/kdbx_group.dart | 2 +- lib/src/kdbx_object.dart | 48 +++++++++++++++++----------- lib/src/kdbx_xml.dart | 1 - lib/src/utils/sequence.dart | 28 ++++++++++++++++ test/kdbx_dirty_save_test.dart | 58 ++++++++++++++++++++++++++++++++++ 9 files changed, 148 insertions(+), 33 deletions(-) create mode 100644 lib/src/utils/sequence.dart create mode 100644 test/kdbx_dirty_save_test.dart diff --git a/CHANGELOG.md b/CHANGELOG.md index 37e182e..ffa2d38 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,8 @@ +## 2.3.0 + +- Mark objects only as clean when saving was successful. +- Only mark objects as clean if they have not been modified since we started saving. + ## 2.2.0 - If argon2 ffi implementation is not available, fallback to pointycastle (dart-only) diff --git a/lib/src/kdbx_entry.dart b/lib/src/kdbx_entry.dart index fcf474b..3b3b168 100644 --- a/lib/src/kdbx_entry.dart +++ b/lib/src/kdbx_entry.dart @@ -9,7 +9,6 @@ import 'package:kdbx/src/kdbx_exceptions.dart'; import 'package:kdbx/src/kdbx_file.dart'; import 'package:kdbx/src/kdbx_format.dart'; import 'package:kdbx/src/kdbx_group.dart'; -import 'package:kdbx/src/kdbx_header.dart'; import 'package:kdbx/src/kdbx_object.dart'; import 'package:kdbx/src/kdbx_xml.dart'; import 'package:logging/logging.dart'; @@ -77,7 +76,7 @@ class KdbxKey { extension KdbxEntryInternal on KdbxEntry { KdbxEntry cloneInto(KdbxGroup otherGroup, {bool toHistoryEntry = false}) => KdbxEntry.create( - otherGroup.file!, + otherGroup.file, otherGroup, isHistoryEntry: toHistoryEntry, ) @@ -212,7 +211,7 @@ class KdbxEntry extends KdbxObject { StringNode get tags => StringNode(this, 'Tags'); @override - set file(KdbxFile? file) { + set file(KdbxFile file) { super.file = file; // TODO this looks like some weird workaround, get rid of the // `file` reference. @@ -346,7 +345,7 @@ class KdbxEntry extends KdbxObject { value: bytes, ); modify(() { - file!.ctx.addBinary(binary); + file.ctx.addBinary(binary); _binaries[key] = binary; }); return binary; diff --git a/lib/src/kdbx_file.dart b/lib/src/kdbx_file.dart index 2a2475e..8c930d1 100644 --- a/lib/src/kdbx_file.dart +++ b/lib/src/kdbx_file.dart @@ -10,6 +10,7 @@ import 'package:kdbx/src/kdbx_format.dart'; import 'package:kdbx/src/kdbx_group.dart'; import 'package:kdbx/src/kdbx_header.dart'; import 'package:kdbx/src/kdbx_object.dart'; +import 'package:kdbx/src/utils/sequence.dart'; import 'package:logging/logging.dart'; import 'package:quiver/check.dart'; import 'package:synchronized/synchronized.dart'; @@ -17,6 +18,8 @@ import 'package:xml/xml.dart' as xml; final _logger = Logger('kdbx_file'); +typedef FileSaveCallback = Future Function(Uint8List bytes); + class KdbxFile { KdbxFile( this.ctx, this.kdbxFormat, this.credentials, this.header, this.body) { @@ -54,14 +57,17 @@ class KdbxFile { Stream> get dirtyObjectsChanged => _dirtyObjectsChanged.stream; - Future save() async { - return kdbxFormat.save(this); + Future save([FileSaveCallback? saveBytes]) async { + return kdbxFormat.save(this, saveBytes); } /// Marks all dirty objects as clean. Called by [KdbxFormat.save]. - void onSaved() { - dirtyObjects.clear(); - _dirtyObjectsChanged.add(const {}); + void onSaved(TimeSequence savedAt) { + final cleanedObjects = dirtyObjects.where((e) => e.clean(savedAt)).toList(); + + dirtyObjects.removeAll(cleanedObjects); + _logger.finer('Saved. Remaining dirty objects: ${dirtyObjects.length}'); + _dirtyObjectsChanged.add(dirtyObjects); } Iterable get _allObjects => body.rootGroup diff --git a/lib/src/kdbx_format.dart b/lib/src/kdbx_format.dart index 881e22b..69ef538 100644 --- a/lib/src/kdbx_format.dart +++ b/lib/src/kdbx_format.dart @@ -21,6 +21,7 @@ import 'package:kdbx/src/kdbx_group.dart'; import 'package:kdbx/src/kdbx_header.dart'; import 'package:kdbx/src/kdbx_xml.dart'; import 'package:kdbx/src/utils/byte_utils.dart'; +import 'package:kdbx/src/utils/sequence.dart'; import 'package:logging/logging.dart'; import 'package:meta/meta.dart'; import 'package:pointycastle/export.dart'; @@ -576,10 +577,20 @@ class KdbxFormat { } /// Saves the given file. - Future save(KdbxFile file) async { + Future save(KdbxFile file, FileSaveCallback? saveBytes) async { _logger.finer('Saving ${file.body.rootGroup.uuid} ' '(locked: ${file.saveLock.locked})'); - return file.saveLock.synchronized(() => _saveSynchronized(file)); + return file.saveLock.synchronized(() async { + final savedAt = TimeSequence.now(); + final bytes = await _saveSynchronized(file); + if (saveBytes != null) { + _logger.fine('Saving bytes.'); + final byteCount = await saveBytes(bytes); + _logger.fine('Saved bytes. $byteCount'); + } + file.onSaved(savedAt); + return bytes; + }); } Future _saveSynchronized(KdbxFile file) async { @@ -613,7 +624,6 @@ class KdbxFormat { } else { throw UnsupportedError('Unsupported version ${header.version}'); } - file.onSaved(); return output.toBytes(); } diff --git a/lib/src/kdbx_group.dart b/lib/src/kdbx_group.dart index 25af7e2..90e67f1 100644 --- a/lib/src/kdbx_group.dart +++ b/lib/src/kdbx_group.dart @@ -159,7 +159,7 @@ class KdbxGroup extends KdbxObject { // item was moved. if (otherObj.wasMovedAfter(movedObj)) { // item was moved in the other file, so we have to move it here. - file!.move(movedObj, this); + file.move(movedObj, this); mergeContext.trackChange(movedObj, debug: 'moved to another group'); } else { // item was moved in this file, so nothing to do. diff --git a/lib/src/kdbx_object.dart b/lib/src/kdbx_object.dart index 60db13d..2e98a8a 100644 --- a/lib/src/kdbx_object.dart +++ b/lib/src/kdbx_object.dart @@ -10,6 +10,7 @@ import 'package:kdbx/src/kdbx_group.dart'; import 'package:kdbx/src/kdbx_meta.dart'; import 'package:kdbx/src/kdbx_times.dart'; import 'package:kdbx/src/kdbx_xml.dart'; +import 'package:kdbx/src/utils/sequence.dart'; import 'package:logging/logging.dart'; import 'package:meta/meta.dart'; import 'package:quiver/iterables.dart'; @@ -37,7 +38,7 @@ mixin Changeable { Stream> get changes => _controller.stream; - bool _isDirty = false; + TimeSequence? _isDirty; /// allow recursive calls to [modify] bool _isInModify = false; @@ -60,10 +61,11 @@ mixin Changeable { void onAfterAnyModify() {} RET modify(RET Function() modify) { - if (_isDirty || _isInModify) { + if (isDirty || _isInModify) { try { return modify(); } finally { + _isDirty = TimeSequence.now(); onAfterAnyModify(); } } @@ -72,23 +74,30 @@ mixin Changeable { try { return modify(); } finally { - _isDirty = true; + _isDirty = TimeSequence.now(); _isInModify = false; onAfterModify(); onAfterAnyModify(); - _controller.add(ChangeEvent(object: this as T, isDirty: _isDirty)); + _controller.add(ChangeEvent(object: this as T, isDirty: isDirty)); } } - void clean() { - if (!_isDirty) { - return; + bool clean(TimeSequence savedAt) { + final dirty = _isDirty; + if (dirty == null) { + _logger.warning('clean() called, even though we are not even dirty.'); + return false; + } + if (savedAt.isBefore(dirty)) { + _logger.fine('We got dirty after save was invoked. so we are not clean.'); + return false; } - _isDirty = false; - _controller.add(ChangeEvent(object: this as T, isDirty: _isDirty)); + _isDirty = null; + _controller.add(ChangeEvent(object: this as T, isDirty: isDirty)); + return true; } - bool get isDirty => _isDirty; + bool get isDirty => _isDirty != null; } abstract class KdbxNodeContext implements KdbxNode { @@ -97,7 +106,7 @@ abstract class KdbxNodeContext implements KdbxNode { abstract class KdbxNode with Changeable { KdbxNode.create(String nodeName) : node = XmlElement(XmlName(nodeName)) { - _isDirty = true; + _isDirty = TimeSequence.now(); } KdbxNode.read(this.node); @@ -111,10 +120,8 @@ abstract class KdbxNode with Changeable { // String text(String nodeName) => _opt(nodeName)?.text; /// must only be called to save this object. - /// will mark this object as not dirty. @mustCallSuper XmlElement toXml() { - clean(); return node.copy(); } } @@ -158,7 +165,7 @@ extension KdbxObjectInternal on KdbxObject { abstract class KdbxObject extends KdbxNode { KdbxObject.create( this.ctx, - this.file, + this._file, String nodeName, KdbxGroup? parent, ) : times = KdbxTimes.create(ctx), @@ -173,8 +180,11 @@ abstract class KdbxObject extends KdbxNode { super.read(node); /// the file this object is part of. will be set AFTER loading, etc. + KdbxFile get file => _file!; + set file(KdbxFile file) => _file = file; + /// TODO: We should probably get rid of this `file` reference. - KdbxFile? file; + KdbxFile? _file; final KdbxReadWriteContext ctx; @@ -196,11 +206,11 @@ abstract class KdbxObject extends KdbxNode { UuidNode(this, 'PreviousParentGroup'); KdbxCustomIcon? get customIcon => - customIconUuid.get()?.let((uuid) => file!.body.meta.customIcons[uuid]); + customIconUuid.get()?.let((uuid) => file.body.meta.customIcons[uuid]); set customIcon(KdbxCustomIcon? icon) { if (icon != null) { - file!.body.meta.addCustomIcon(icon); + file.body.meta.addCustomIcon(icon); customIconUuid.set(icon.uuid); } else { customIconUuid.set(null); @@ -220,7 +230,7 @@ abstract class KdbxObject extends KdbxNode { super.onAfterAnyModify(); times.modifiedNow(); // during initial `create` the file will be null. - file?.dirtyObject(this); + _file?.dirtyObject(this); } bool wasModifiedAfter(KdbxObject other) => times.lastModificationTime @@ -249,7 +259,7 @@ abstract class KdbxObject extends KdbxNode { void merge(MergeContext mergeContext, covariant KdbxObject other); bool isInRecycleBin() { - final targetGroup = file!.recycleBin; + final targetGroup = file.recycleBin; if (targetGroup == null) { return false; } diff --git a/lib/src/kdbx_xml.dart b/lib/src/kdbx_xml.dart index 72ca036..e745f10 100644 --- a/lib/src/kdbx_xml.dart +++ b/lib/src/kdbx_xml.dart @@ -6,7 +6,6 @@ import 'package:collection/collection.dart' show IterableExtension; import 'package:kdbx/src/kdbx_consts.dart'; import 'package:kdbx/src/kdbx_exceptions.dart'; import 'package:kdbx/src/kdbx_format.dart'; -import 'package:kdbx/src/kdbx_header.dart'; import 'package:kdbx/src/kdbx_object.dart'; import 'package:kdbx/src/utils/byte_utils.dart'; import 'package:logging/logging.dart'; diff --git a/lib/src/utils/sequence.dart b/lib/src/utils/sequence.dart new file mode 100644 index 0000000..058997c --- /dev/null +++ b/lib/src/utils/sequence.dart @@ -0,0 +1,28 @@ +import 'package:clock/clock.dart'; + +/// Simple class to assign a unique integer for any point in time. +/// This is basically to ensure that even if two events happen at the +/// same millisecond we know which came first. +/// (realistically this will only make a difference in tests). +class TimeSequence { + TimeSequence._(this._sequenceIndex); + factory TimeSequence.now() => TimeSequence._(_sequenceCounter++); + + static int _sequenceCounter = 0; + + final int _sequenceIndex; + final DateTime _date = clock.now(); + + bool isAfter(TimeSequence other) { + return _sequenceIndex > other._sequenceIndex; + } + + bool isBefore(TimeSequence other) { + return _sequenceIndex < other._sequenceIndex; + } + + @override + String toString() { + return '{Sequence: $_sequenceIndex time: $_date}'; + } +} diff --git a/test/kdbx_dirty_save_test.dart b/test/kdbx_dirty_save_test.dart new file mode 100644 index 0000000..ed665a3 --- /dev/null +++ b/test/kdbx_dirty_save_test.dart @@ -0,0 +1,58 @@ +import 'package:kdbx/kdbx.dart'; +import 'package:test/test.dart'; + +import 'internal/test_utils.dart'; + +void main() { + final testUtil = TestUtil(); + group('test save with dirty objects', () { + test('modify object after save', () async { + final file = testUtil.createEmptyFile(); + final group = file.body.rootGroup; + final entry = testUtil.createEntry(file, group, 'user', 'pass'); + final entry2 = testUtil.createEntry(file, group, 'user', 'pass'); + await file.save(); + + const value1 = 'new'; + const value2 = 'new2'; + entry.setString(TestUtil.keyTitle, PlainValue(value1)); + entry2.setString(TestUtil.keyTitle, PlainValue(value1)); + expect(file.isDirty, isTrue); + + await file.save((bytes) async { + // must still be dirty as long as we are not finished saving. + expect(file.isDirty, isTrue); + expect(entry.isDirty, isTrue); + expect(entry2.isDirty, isTrue); + return 1; + }); + expect(file.isDirty, isFalse); + expect(entry.isDirty, isFalse); + expect(entry2.isDirty, isFalse); + }); + test('parallel modify', () async { + final file = testUtil.createEmptyFile(); + final group = file.body.rootGroup; + final entry = testUtil.createEntry(file, group, 'user', 'pass'); + final entry2 = testUtil.createEntry(file, group, 'user', 'pass'); + await file.save(); + + const value1 = 'new'; + const value2 = 'new2'; + + entry.setString(TestUtil.keyTitle, PlainValue(value2)); + entry2.setString(TestUtil.keyTitle, PlainValue(value2)); + await file.save((bytes) async { + // must still be dirty as long as we are not finished saving. + expect(file.isDirty, isTrue); + expect(entry.isDirty, isTrue); + expect(entry2.isDirty, isTrue); + entry2.setString(TestUtil.keyTitle, PlainValue(value1)); + return 1; + }); + expect(file.isDirty, isTrue); + expect(entry.isDirty, isFalse); + expect(entry2.isDirty, isTrue); + }); + }); +}