Ensure that null values are removed entirely

JeremyCaney · JeremyCaney · commit 3d1ad6bc8f41 · 2021-04-05T14:33:03.000-07:00
Previously, when a null value was submitted to `SetValue()`, it would simply overwrite the `TrackedRecord<T>.Value` with `null`, but keep the record in place. That is confusing when e.g. calling `Remove()`, and can lead to bugs when relying on e.g. `Contains()`, as records will exist for values that have effectively been deleted. Since we already track these deleted records via `DeletedItems`, it makes more sense to actually remove them. To facilitate this, the null check now explicitly calls `Remove()` instead of falling back to persisting the `TrackedRecord<T>` with the null `Value`. As part of that, I refactored how null values are handled when an existing record doesn't exist; in that case, the method returns quickly, thus preventing the need to check for null `updatedItem` values, or worrying about the `enforceBusinessLogic` being inappropriately disabled. This satisfies the first portion of #79 by ensuring that `SetValue()` actually removes the record. In subsequent commits, I will ensure that `Remove()` and `Clear()` properly enforce business logic.
diff --git a/OnTopic/Collections/Specialized/TrackedRecordCollection{TItem,TValue,TAttribute}.cs b/OnTopic/Collections/Specialized/TrackedRecordCollection{TItem,TValue,TAttribute}.cs
@@ -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;
