Merge branch 'improvement/TrackedRecordCollection.Remove-handling' into develop

Previously, calling `Remove()` and `Clear()` on `TrackedRecordCollection<>` and its derivatives `AttributeCollection` and `TopicReferenceCollection` would bypass any business logic on associated properties. It was also inconsistent with `SetValue()` which would accept null values, and correctly enforce business logic, but would keep the `TrackedRecord<>` in place, thus causing potential confusion and bugs (since e.g. `Contains()` would report that the `Key` existed, even though it had a null `Value`).

To mitigate this, the `SetValue()` logic was updated to call `Remove()` if the `Value` was null (3d1ad6b), `Remove()` was updated to enforce business logic (0bccf1f), and `Clear()` was updated to call `Remove()` (abfe213)—thus enforcing business logic on each individual item. In addition to updating existing tests which assumed that `SetValue()` wouldn't delete a record (7aae9bc), I also introduced a _new_ unit test to validate that `Clear()` (and, thus, `Remove()`) will, in fact, enforce business logic (fa08bfb).

This satisfies the requirements for #79.

Author: JeremyCaney

OnTopic.Tests/AttributeCollectionTest.cs

@@ -439,6 +439,26 @@ public void SetValue_ValueChanged_IsDirty() {
 
     }
 
+    /*==========================================================================================================================
+    | TEST: CLEAR: NON-NULLABLE VALUE WITH BUSINESS LOGIC: THROWS EXCEPTION
+    \-------------------------------------------------------------------------------------------------------------------------*/
+    /// <summary>
+    ///   Sets an attribute on a topic instance with an valid value, and then attempts to clear all attributes. Ensures that the
+    ///   business logic is enforced by throwing an exception on the non-nullable property.
+    /// </summary>
+    [Fact]
+    public void Clear_NonNullableValueWithBusinessLogic_ThrowsException() {
+
+      var topic = new CustomTopic("Test", "Page");
+
+      topic.NonNullableAttribute = "Test";
+
+      Assert.Throws<ArgumentNullException>(() =>
+        topic.Attributes.Clear()
+      );
+
+    }
+
     /*==========================================================================================================================
     | TEST: CLEAR: EXISTING VALUES: IS DIRTY?
     \-------------------------------------------------------------------------------------------------------------------------*/

OnTopic.Tests/Entities/CustomTopic.cs

@@ -83,6 +83,22 @@ public int NumericAttribute {
       }
     }
 
+    /*==========================================================================================================================
+    | NON-NULLABLE ATTRIBUTE
+    \-------------------------------------------------------------------------------------------------------------------------*/
+    /// <summary>
+    ///   Provides a property which does not permit null values as a means of ensuring the business logic is enforced when
+    ///   removing values.
+    /// </summary>
+    [AttributeSetter]
+    public string NonNullableAttribute {
+      get => Attributes.GetValue("NonNullableAttribute")?? "";
+      set {
+        Contract.Requires(value, nameof(value));
+        SetAttributeValue("NonNullableAttribute", value.ToString(CultureInfo.InvariantCulture));
+      }
+    }
+
     /*==========================================================================================================================
     | DATE/TIME ATTRIBUTE
     \-------------------------------------------------------------------------------------------------------------------------*/

OnTopic.Tests/TopicReferenceCollectionTest.cs

@@ -220,7 +220,7 @@ public void SetValue_ExistingReference_TopicUpdated() {
     ///   TrackedRecordCollection{TItem, TValue, TAttribute}.SetValue(String, TValue, Boolean?, DateTime?)"/>, updates the
     ///   reference using <see cref="TrackedRecordCollection{TItem, TValue, TAttribute}.SetValue(String, TValue, Boolean?,
     ///   DateTime?)"/> with a <c>null</c> value, and confirms that the <see cref="Topic"/> reference and <see cref="Topic.
-    ///   IncomingRelationships"/> are correctly updated.
+    ///   IncomingRelationships"/> are correctly removed.
     /// </summary>
     /// <remarks>
     ///   This calls <see cref="TrackedRecordCollection{TItem, TValue, TAttribute}.SetValue(String, TValue, Boolean?, DateTime?)
@@ -238,7 +238,7 @@ public void SetValue_ExistingReference_IncomingRelationshipsUpdates() {
       topic.References.SetValue("Reference", null);
       topic.References.SetValue("Reference", null);
 
-      Assert.True(topic.References.Contains("Reference"));
+      Assert.False(topic.References.Contains("Reference"));
       Assert.Null(topic.References.GetValue("Reference"));
       Assert.Equal<int?>(0, reference.IncomingRelationships.GetValues("Reference")?.Count);
 
@@ -262,7 +262,7 @@ public void SetValue_NullReference_TopicRemoved() {
       topic.References.SetValue("Reference", reference);
       topic.References.SetValue("Reference", null);
 
-      Assert.Single(topic.References);
+      Assert.Empty(topic.References);
       Assert.Null(topic.References.GetValue("Reference"));
 
     }

OnTopic/Associations/TopicReferenceCollection.cs

@@ -135,25 +135,5 @@ protected override sealed void RemoveItem(int index) {
 
     }
 
-    /*==========================================================================================================================
-    | OVERRIDE: CLEAR ITEMS
-    \-------------------------------------------------------------------------------------------------------------------------*/
-    /// <inheritdoc/>
-    protected override sealed void ClearItems() {
-
-      /*------------------------------------------------------------------------------------------------------------------------
-      | Handle recipricol references
-      \-----------------------------------------------------------------------------------------------------------------------*/
-      foreach (var item in Items) {
-        item.Value?.IncomingRelationships.Remove(item.Key, AssociatedTopic, true);
-      }
-
-      /*------------------------------------------------------------------------------------------------------------------------
-      | Provide base logic
-      \-----------------------------------------------------------------------------------------------------------------------*/
-      base.ClearItems();
-
-    }
-
   } //Class
 } //Namespace

OnTopic/Collections/Specialized/TrackedRecordCollection{TItem,TValue,TAttribute}.cs

@@ -433,7 +433,7 @@ internal void SetValue(
 
       /*------------------------------------------------------------------------------------------------------------------------
       | Update from business logic
-      >-----------------------------------------------------------------------------------------------------------------------—
+      >-------------------------------------------------------------------------------------------------------------------------
       | If the original values have already been applied, and SetValue() is being triggered a second time after enforcing
       | business logic, then use the original values, while applying any change in the value triggered by the business logic.
       \-----------------------------------------------------------------------------------------------------------------------*/
@@ -447,7 +447,7 @@ internal void SetValue(
 
       /*------------------------------------------------------------------------------------------------------------------------
       | Update existing item
-      >-----------------------------------------------------------------------------------------------------------------------—
+      >-------------------------------------------------------------------------------------------------------------------------
       | Because TrackedRecord<T> is immutable, a new instance must be constructed to replace the previous version.
       \-----------------------------------------------------------------------------------------------------------------------*/
       else if (originalItem is not null) {
@@ -472,10 +472,11 @@ internal void SetValue(
       | Ignore if null
       >-------------------------------------------------------------------------------------------------------------------------
       | ###NOTE JJC20200501: Null or empty values are treated as deletions, and are not persisted to the data store. With
-      | existing values, these are written to ensure that the collection is marked as IsDirty, thus allowing previous values to
-      | be overwritten. Non-existent values, however, should simply be ignored.
+      | existing values, these are written to DeletedItems to ensure the collection is marked as IsDirty, thus allowing previous
+      | values to be overwritten. Non-existent values should simply be ignored, however; we shouldn't delete what doesn't exist.
       \-----------------------------------------------------------------------------------------------------------------------*/
       else if (value is null || String.IsNullOrEmpty(value.ToString())) {
+        return;
       }
 
       /*------------------------------------------------------------------------------------------------------------------------
@@ -507,8 +508,8 @@ internal void SetValue(
       /*------------------------------------------------------------------------------------------------------------------------
       | Persist item to collection
       \-----------------------------------------------------------------------------------------------------------------------*/
-      if (updatedItem is null) {
-        return;
+      if (updatedItem.Value is null) {
+        Remove(key);
       }
       else if (originalItem is not null) {
         this[IndexOf(originalItem)] = updatedItem;
@@ -605,11 +606,15 @@ protected override void SetItem(int index, TItem item) {
     ///   cref="TrackedRecord{T}"/>s are marked as <see cref="TrackedRecord{T}.IsDirty"/>.
     /// </remarks>
     protected override void RemoveItem(int index) {
-      var trackedRecord = this[index];
-      if (!AssociatedTopic.IsNew) {
-        DeletedItems.Add(trackedRecord.Key);
+      var trackedRecord = this[index] with {
+        Value = null
+      };
+      if (_topicPropertyDispatcher.Enforce(trackedRecord.Key, trackedRecord)) {
+        if (!AssociatedTopic.IsNew) {
+          DeletedItems.Add(trackedRecord.Key);
+        }
+        base.RemoveItem(index);
       }
-      base.RemoveItem(index);
     }
 
     /*==========================================================================================================================
@@ -620,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.ToList()) {
+        Remove(item);
       }
       base.ClearItems();
     }