fix creating of history entries. added test for correct dirtyObject events.

CHANGELOG.md

@@ -1,6 +1,7 @@
 ## unreleased 
 
-- fixed bug saving files with history entries which contain attachments.
+- fix bug saving files with history entries which contain attachments.
+- fix bug which would create wrong history entries.
 
 ## 0.4.0+1
 

lib/src/kdbx_custom_data.dart

@@ -25,8 +25,7 @@ class KdbxCustomData extends KdbxNode {
 
   String operator [](String key) => _data[key];
   void operator []=(String key, String value) {
-    _data[key] = value;
-    isDirty = true;
+    modify(() => _data[key] = value);
   }
 
   @override

lib/src/kdbx_entry.dart

@@ -74,11 +74,14 @@ class KdbxEntry extends KdbxObject {
 
       return MapEntry(key, KdbxBinary.readBinaryXml(valueNode, isInline: true));
     }));
-    history.addAll(_historyElement
-        .findElements('Entry')
-        .map(
-            (entry) => KdbxEntry.read(ctx, parent, entry, isHistoryEntry: true))
-        .toList());
+    history.addAll(node
+            .findElements(KdbxXml.NODE_HISTORY)
+            .singleOrNull
+            ?.findElements('Entry')
+            ?.map((entry) =>
+                KdbxEntry.read(ctx, parent, entry, isHistoryEntry: true))
+            ?.toList() ??
+        []);
   }
 
   final bool isHistoryEntry;
@@ -95,21 +98,10 @@ class KdbxEntry extends KdbxObject {
     }
   }
 
-  XmlElement get _historyElement => node
-          .findElements(KdbxXml.NODE_HISTORY)
-          .singleWhere((_) => true, orElse: () {
-        final el = XmlElement(XmlName(KdbxXml.NODE_HISTORY));
-        node.children.add(el);
-        return el;
-      });
-
   @override
-  set isDirty(bool newDirty) {
-    if (!isDirty && newDirty) {
-      final history = _historyElement;
-      history.children.add(toXml());
-    }
-    super.isDirty = newDirty;
+  void onBeforeModify() {
+    super.onBeforeModify();
+    history.add(KdbxEntry.read(ctx, parent, toXml())..file = file);
   }
 
   @override
@@ -180,12 +172,13 @@ class KdbxEntry extends KdbxObject {
       _logger.finest('Value did not change for $key');
       return;
     }
-    isDirty = true;
-    if (value == null) {
-      _strings.remove(key);
-    } else {
-      _strings[key] = value;
-    }
+    modify(() {
+      if (value == null) {
+        _strings.remove(key);
+      } else {
+        _strings[key] = value;
+      }
+    });
   }
 
   void renameKey(KdbxKey oldKey, KdbxKey newKey) {
@@ -224,22 +217,23 @@ class KdbxEntry extends KdbxObject {
       isProtected: isProtected,
       value: bytes,
     );
-    file.ctx.addBinary(binary);
-    _binaries[key] = binary;
-    isDirty = true;
+    modify(() {
+      file.ctx.addBinary(binary);
+      _binaries[key] = binary;
+    });
     return binary;
   }
 
   void removeBinary(KdbxKey binaryKey) {
-    final binary = _binaries.remove(binaryKey);
-    if (binary == null) {
-      throw StateError(
-          'Trying to remove binary key $binaryKey does not exist.');
-    }
-    if (!binary.isInline) {
-      file.ctx.removeBinary(binary);
-    }
-    isDirty = true;
+    modify(() {
+      final binary = _binaries.remove(binaryKey);
+      if (binary == null) {
+        throw StateError(
+            'Trying to remove binary key $binaryKey does not exist.');
+      }
+      // binary will not be removed (yet) from file, because it will
+      // be referenced in history.
+    });
   }
 
   KdbxKey _uniqueBinaryName(String fileName) {
@@ -261,3 +255,7 @@ class KdbxEntry extends KdbxObject {
     return 'KdbxGroup{uuid=$uuid,name=$label}';
   }
 }
+
+extension<T> on Iterable<T> {
+  T get singleOrNull => singleWhere((element) => true, orElse: () => null);
+}

lib/src/kdbx_file.dart

@@ -1,6 +1,7 @@
 import 'dart:async';
 import 'dart:typed_data';
 
+import 'package:collection/collection.dart';
 import 'package:kdbx/src/crypto/protected_value.dart';
 import 'package:kdbx/src/kdbx_consts.dart';
 import 'package:kdbx/src/kdbx_dao.dart';
@@ -51,7 +52,7 @@ class KdbxFile {
   /// Marks all dirty objects as clean. Called by [KdbxFormat.save].
   void onSaved() {
     dirtyObjects.clear();
-    _dirtyObjectsChanged.add(dirtyObjects);
+    _dirtyObjectsChanged.add(const {});
   }
 
   Iterable<KdbxObject> get _allObjects => body.rootGroup
@@ -61,7 +62,7 @@ class KdbxFile {
 
   void dirtyObject(KdbxObject kdbxObject) {
     dirtyObjects.add(kdbxObject);
-    _dirtyObjectsChanged.add(dirtyObjects);
+    _dirtyObjectsChanged.add(UnmodifiableSetView(Set.of(dirtyObjects)));
   }
 
   void dispose() {

lib/src/kdbx_group.dart

@@ -65,8 +65,7 @@ class KdbxGroup extends KdbxObject {
       throw StateError(
           'Invalid operation. Trying to add entry which is already in another group.');
     }
-    _entries.add(entry);
-    isDirty = true;
+    modify(() => _entries.add(entry));
   }
 
   void addGroup(KdbxGroup group) {
@@ -74,22 +73,23 @@ class KdbxGroup extends KdbxObject {
       throw StateError(
           'Invalid operation. Trying to add group which is already in another group.');
     }
-    _groups.add(group);
-    isDirty = true;
+    modify(() => _groups.add(group));
   }
 
   void internalRemoveGroup(KdbxGroup group) {
-    if (!_groups.remove(group)) {
-      throw StateError('Unable to remove $group from $this (Not found)');
-    }
-    isDirty = true;
+    modify(() {
+      if (!_groups.remove(group)) {
+        throw StateError('Unable to remove $group from $this (Not found)');
+      }
+    });
   }
 
   void internalRemoveEntry(KdbxEntry entry) {
-    if (!_entries.remove(entry)) {
-      throw StateError('Unable to remove $entry from $this (Not found)');
-    }
-    isDirty = true;
+    modify(() {
+      if (!_entries.remove(entry)) {
+        throw StateError('Unable to remove $entry from $this (Not found)');
+      }
+    });
   }
 
   /// returns all parents recursively including this group.

lib/src/kdbx_object.dart

@@ -35,14 +35,38 @@ mixin Changeable<T> {
 
   bool _isDirty = false;
 
-  set isDirty(bool dirty) {
-//    _logger.finest('changing dirty (old:$_isDirty) $dirty');
-    if (!_isDirty && !dirty) {
-      // no need for change events when already not-dirty.
+  /// Called before the *first* modification (ie. before `isDirty` changes
+  /// from false to true)
+  @protected
+  @mustCallSuper
+  void onBeforeModify() {}
+
+  /// Called after the *first* modification (ie. after `isDirty` changed
+  /// from false to true)
+  @protected
+  @mustCallSuper
+  void onAfterModify() {}
+
+  RET modify<RET>(RET Function() modify) {
+    if (_isDirty) {
+      return modify();
+    }
+    onBeforeModify();
+    try {
+      return modify();
+    } finally {
+      _isDirty = true;
+      onAfterModify();
+      _controller.add(ChangeEvent(object: this as T, isDirty: _isDirty));
+    }
+  }
+
+  void clean() {
+    if (!_isDirty) {
       return;
     }
-    _isDirty = dirty;
-    _controller.add(ChangeEvent(object: this as T, isDirty: dirty));
+    _isDirty = false;
+    _controller.add(ChangeEvent(object: this as T, isDirty: _isDirty));
   }
 
   bool get isDirty => _isDirty;
@@ -59,6 +83,9 @@ abstract class KdbxNode with Changeable<KdbxNode> {
 
   KdbxNode.read(this.node);
 
+  /// XML Node used while reading this KdbxNode.
+  /// Must NOT be modified. Only copies which are obtained through [toXml].
+  /// this node should always represent the original loaded state.
   final XmlElement node;
 
 //  @protected
@@ -68,7 +95,7 @@ abstract class KdbxNode with Changeable<KdbxNode> {
   /// will mark this object as not dirty.
   @mustCallSuper
   XmlElement toXml() {
-    isDirty = false;
+    clean();
     final el = node.copy() as XmlElement;
     return el;
   }
@@ -108,15 +135,11 @@ abstract class KdbxObject extends KdbxNode {
   KdbxGroup _parent;
 
   @override
-  set isDirty(bool dirty) {
-    if (dirty) {
-      times.modifiedNow();
-      if (/*!isDirty && */ dirty) {
-        // during initial `create` the file will be null.
-        file?.dirtyObject(this);
-      }
-    }
-    super.isDirty = dirty;
+  void onAfterModify() {
+    super.onAfterModify();
+    times.modifiedNow();
+    // during initial `create` the file will be null.
+    file?.dirtyObject(this);
   }
 
   @override
@@ -128,8 +151,7 @@ abstract class KdbxObject extends KdbxNode {
   }
 
   void internalChangeParent(KdbxGroup parent) {
-    _parent = parent;
-    isDirty = true;
+    modify(() => _parent = parent);
   }
 }
 

lib/src/kdbx_xml.dart

@@ -75,22 +75,23 @@ abstract class KdbxSubTextNode<T> extends KdbxSubNode<T> {
     if (get() == value) {
       return;
     }
-    node.isDirty = true;
-    final el =
-        node.node.findElements(name).singleWhere((x) => true, orElse: () {
-      final el = XmlElement(XmlName(name));
-      node.node.children.add(el);
-      return el;
+    node.modify(() {
+      final el =
+          node.node.findElements(name).singleWhere((x) => true, orElse: () {
+        final el = XmlElement(XmlName(name));
+        node.node.children.add(el);
+        return el;
+      });
+      el.children.clear();
+      if (value == null) {
+        return;
+      }
+      final stringValue = encode(value);
+      if (stringValue == null) {
+        return;
+      }
+      el.children.add(XmlText(stringValue));
     });
-    el.children.clear();
-    if (value == null) {
-      return;
-    }
-    final stringValue = encode(value);
-    if (stringValue == null) {
-      return;
-    }
-    el.children.add(XmlText(stringValue));
   }
 
   @override

pubspec.yaml

@@ -35,3 +35,4 @@ dependencies:
 dev_dependencies:
   pedantic: '>=1.7.0 <2.0.0'
   test: '>=1.6.0 <2.0.0'
+  fake_async: ^1.1.0

test/internal/test_utils.dart

@@ -12,6 +12,8 @@ import 'package:logging/logging.dart';
 final _logger = Logger('test_utils');
 
 class TestUtil {
+  static final keyTitle = KdbxKey('Title');
+
   static KdbxFormat kdbxFormat() {
     Argon2.resolveLibraryForceDynamic = true;
     return KdbxFormat(Argon2FfiFlutter(resolveLibrary: (path) {
@@ -35,10 +37,15 @@ class TestUtil {
   }
 
   static Future<KdbxFile> readKdbxFileBytes(Uint8List data,
-      {String password = 'asdf'}) async {
+      {String password = 'asdf', Credentials credentials}) async {
     final kdbxFormat = TestUtil.kdbxFormat();
     final file = await kdbxFormat.read(
-        data, Credentials(ProtectedValue.fromString(password)));
+        data, credentials ?? Credentials(ProtectedValue.fromString(password)));
     return file;
   }
+
+  static Future<KdbxFile> saveAndRead(KdbxFile file) async {
+    return await readKdbxFileBytes(await file.save(),
+        credentials: file.credentials);
+  }
 }

test/kdbx_binaries_test.dart

@@ -85,6 +85,18 @@ void main() {
       final entry = file.body.rootGroup.entries.first;
       expectKeepass2binariesContents(entry);
     });
+    test('modify file with binary in history', () async {
+      final fileRead =
+          await TestUtil.readKdbxFile('test/keepass2binaries.kdbx');
+      final updateEntry = (KdbxFile file) {
+        final entry = fileRead.body.rootGroup.entries.first;
+        entry.setString(KdbxKey('title'), PlainValue('example'));
+      };
+      updateEntry(fileRead);
+      final saved = await fileRead.save();
+      final file = await TestUtil.readKdbxFileBytes(saved);
+      await file.save();
+    });
     test('Add new attachment', () async {
       await _testAddNewAttachment('test/keepass2binaries.kdbx');
     });
@@ -95,7 +107,7 @@ void main() {
         expectKeepass2binariesContents(entry);
         expect(file.ctx.binariesIterable, hasLength(3));
         entry.removeBinary(KdbxKey('example1.txt'));
-        expect(file.ctx.binariesIterable, hasLength(2));
+        expect(file.ctx.binariesIterable, hasLength(3));
         return await file.save();
       })();
       final file = await TestUtil.readKdbxFileBytes(saved);
@@ -103,7 +115,12 @@ void main() {
       expect(entry.binaryEntries, hasLength(2));
       expect(entry.binaryEntries.map((e) => (e.key.key)),
           ['example2.txt', 'keepasslogo.jpeg']);
-      expect(file.ctx.binariesIterable, hasLength(2));
+      // the file itself will contain 3 items, because it is still
+      // available in history.
+      expect(file.ctx.binariesIterable, hasLength(3));
+      expect(entry.history.last.binaryEntries, hasLength(3));
+      // make sure the file can still be saved.
+      await file.save();
     });
   });
   group('kdbx4 attachment', () {
@@ -139,7 +156,8 @@ void main() {
             hasLength(7092));
         expect(file.ctx.binariesIterable, hasLength(2));
         entry.removeBinary(KdbxKey('example2.txt'));
-        expect(file.ctx.binariesIterable, hasLength(1));
+        // the binary remains in the file, since it is referenced in the history
+        expect(file.ctx.binariesIterable, hasLength(2));
         expect(file.dirtyObjects, [entry]);
         return await file.save();
       })();
@@ -148,7 +166,7 @@ void main() {
       expect(entry.binaryEntries, hasLength(0));
       expectBinary(file.body.rootGroup.entries.last, 'keepasslogo.jpeg',
           hasLength(7092));
-      expect(file.ctx.binariesIterable, hasLength(1));
+      expect(file.ctx.binariesIterable, hasLength(2));
     });
     test('Add new attachment kdbx4', () async {
       await _testAddNewAttachment('test/keepass2kdbx4binaries.kdbx');

test/kdbx_history_test.dart

@@ -0,0 +1,102 @@
+import 'dart:async';
+
+import 'package:kdbx/kdbx.dart';
+import 'package:logging_appenders/logging_appenders.dart';
+import 'package:quiver/core.dart';
+import 'package:test/test.dart';
+
+import 'internal/test_utils.dart';
+
+class StreamExpect<T> {
+  StreamExpect(this.stream) {
+    stream.listen((event) {
+      if (_expectNext == null) {
+        fail('Got event, but none was expected. $event');
+      }
+      expect(event, _expectNext.orNull);
+      _expectNext = null;
+    }, onDone: () {
+      expect(_expectNext, isNull);
+      isDone = true;
+    }, onError: (dynamic error) {
+      expect(_expectNext, isNull);
+      this.error = error;
+    });
+  }
+
+  Future<RET> expectNext<RET>(T value, FutureOr<RET> Function() cb) async {
+    if (_expectNext != null) {
+      fail('The last event was never received. last: $_expectNext');
+    }
+    _expectNext = Optional.fromNullable(value);
+    try {
+      return await cb();
+    } finally {
+      await pumpEventQueue();
+    }
+  }
+
+  void expectFinished() {
+    expect(isDone, true);
+  }
+
+  final Stream<T> stream;
+  bool isDone = false;
+  dynamic error;
+  Optional<T> _expectNext;
+}
+
+void main() {
+  PrintAppender.setupLogging();
+  group('test history for values', () {
+    test('check history creation', () async {
+      final file = await TestUtil.readKdbxFile('test/keepass2test.kdbx');
+      const valueOrig = 'Sample Entry';
+      const value1 = 'new';
+      const value2 = 'new2';
+      final dirtyExpect = StreamExpect(file.dirtyObjectsChanged);
+      {
+        final first = file.body.rootGroup.entries.first;
+        expect(file.header.versionMajor, 3);
+        expect(first.getString(TestUtil.keyTitle).getText(), valueOrig);
+        await dirtyExpect.expectNext({first}, () {
+          first.setString(TestUtil.keyTitle, PlainValue(value1));
+        });
+      }
+      expect(file.dirtyObjects, hasLength(1));
+      final f2 =
+          await dirtyExpect.expectNext({}, () => TestUtil.saveAndRead(file));
+      expect(file.dirtyObjects, isEmpty);
+      {
+        final first = f2.body.rootGroup.entries.first;
+        expect(first.getString(TestUtil.keyTitle).getText(), value1);
+        expect(first.history.last.getString(TestUtil.keyTitle).getText(),
+            valueOrig);
+        await dirtyExpect.expectNext({}, () => file.save());
+      }
+
+      // edit the original file again, and there should be a second history
+      {
+        final first = file.body.rootGroup.entries.first;
+        await dirtyExpect.expectNext({first},
+            () => first.setString(TestUtil.keyTitle, PlainValue(value2)));
+      }
+      final f3 =
+          await dirtyExpect.expectNext({}, () => TestUtil.saveAndRead(file));
+      expect(file.dirtyObjects, isEmpty);
+      {
+        final first = f3.body.rootGroup.entries.first;
+        expect(first.getString(TestUtil.keyTitle).getText(), value2);
+        expect(first.history, hasLength(2));
+        expect(
+            first.history.last.getString(TestUtil.keyTitle).getText(), value1);
+        expect(first.history.first.getString(TestUtil.keyTitle).getText(),
+            valueOrig);
+        await dirtyExpect.expectNext({}, () => file.save());
+      }
+      file.dispose();
+      await pumpEventQueue();
+      dirtyExpect.expectFinished();
+    });
+  });
+}