Ensure that ClearItems() enforces business logic

JeremyCaney · JeremyCaney · commit abfe213f26ee · 2021-04-05T14:37:42.000-07:00
Previously, when deleting items directly via `Clear()`, the `ClearItems()` wouldn't enforce business logic. This is problematic because sometimes properties either a) disallow nulls, or b) must update their internal states when values are set to null. This would be handled when calling `SetValue()` or going directly through the property, but bypassed when calling `Clear()`. Unfortunately, for `Clear()`, this means checking the business logic for each individual record. As such, instead of just bulk deleting these, we're instead calling `Remove()` for each individual item. This is less performant, but ensures that the business logic is always accounted for. Fortunately, calling `Clear()` on this collection is uncommon; it's far more common to work with individual items via `SetValue()` or, less commonly, `Remove()`. This update patches that hole, and thus resolves the `Clear()` portion of #79.
diff --git a/OnTopic/Collections/Specialized/TrackedRecordCollection{TItem,TValue,TAttribute}.cs b/OnTopic/Collections/Specialized/TrackedRecordCollection{TItem,TValue,TAttribute}.cs
@@ -625,12 +625,15 @@ protected override void RemoveItem(int index) {
     ///   it is appropriately marked as <see cref="IsDirty()"/>.
     /// </summary>
     /// <remarks>
-    ///   When an <see cref="TrackedRecord{T}"/> is removed, <see cref="IsDirty()"/> will return true�even if no remaining <see
-    ///   cref="TrackedRecord{T}"/>s are marked as <see cref="TrackedRecord{T}.IsDirty"/>.
+    ///   In order to ensure any business logic is enforced, <see cref="ClearItems()"/> loops through every <see cref="
+    ///   TrackedRecord{T}"/> in the <see cref="TrackedRecordCollection{TItem, TValue, TAttribute}"/> and explicitly calls <see
+    ///   cref="KeyedCollection{TKey, TItem}.Remove(TKey)"/>. This is slower, but ensures that any state tracking and null
+    ///   validation that occurs in the properties is maintained. Fortunately, this is a rare use case; we typically expect
+    ///   attributes to be handled individually.
     /// </remarks>
     protected override void ClearItems() {
-      if (!AssociatedTopic.IsNew) {
-        DeletedItems.AddRange(Items.Select(a => a.Key));
+      foreach (var item in Items) {
+        Remove(item);
       }
       base.ClearItems();
     }
