Skip to content

Commit a0b659d

Browse files
committed
Don't save phantom attributes
In the previous commit, we patched an issue where empty attributes were being committed to the _extended attributes_ storage (c035ca0). But, in many cases, those attributes shouldn't exist in the first place. There's no such thing as an empty attribtute in OnTopic; they should be treated as deletions. For this reason, the main reason we store `AttributeValue`s with a `null` or empty value is to signal to `ITopicRepository` implementations that there is a deletion that needs to be accounted for. But if the attribute never previously existed, then there's no attribute to delete, and the attribute should be ignored entirely. This update prevents `null` or empty `AttributeValue`s from being added to the `AttributeValueCollection` via the `SetValue()` entry point. This doesn't solve any bugs, as these cases are handled downstream by the `ITopicRepository` and, potentially, their backend data stores. But it does help prevent unnecessary and extraneous objects from being added to the object graph, and thus might have present minor gains in terms of memory usage and performance. It's worth noting that developers can still inect `null` or empty `AttributeValue`s directly by using e.g. the `Add()` entry point. We usually expect developers to access attributes via `SetValue()`, though, as it's eaiser than constructing a new `AttributeValue` and comparing it to existing values. If a developer is accessing the collection directly, we're assuming s/he knows what they're doing and will either prevent these values on their end, or has a good reason for adding them. Regardless, it's not the priority or concern of this patch. Includes unit tests for validating functionality.
1 parent c035ca0 commit a0b659d

2 files changed

Lines changed: 49 additions & 0 deletions

File tree

OnTopic.Tests/AttributeValueCollectionTest.cs

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -303,6 +303,45 @@ public void Add_InvalidAttributeValue_ThrowsException() {
303303
topic.Attributes.Add(new AttributeValue("Key", "# ?"));
304304
}
305305

306+
/*==========================================================================================================================
307+
| TEST: SET VALUE: EMPTY ATTRIBUTE VALUE: SKIPS
308+
\-------------------------------------------------------------------------------------------------------------------------*/
309+
/// <summary>
310+
/// Adds a new attribute with an empty value, and confirms that it is <i>not</i> added as a new <see
311+
/// cref="AttributeValue"/>. Empty values are treated as the same as non-existent attributes. They are stored for the sake
312+
/// of tracking <i>deleted</i> attributes, but should not be stored for <i>new</i> attributes.
313+
/// </summary>
314+
[TestMethod]
315+
public void SetValue_EmptyAttributeValue_Skips() {
316+
317+
var topic = TopicFactory.Create("Test", "Container");
318+
319+
topic.Attributes.SetValue("Attribute", "");
320+
321+
Assert.IsFalse(topic.Attributes.Contains("Attribute"));
322+
323+
}
324+
325+
/*==========================================================================================================================
326+
| TEST: SET VALUE: UPDATE EMPTY ATTRIBUTE VALUE: REPLACES
327+
\-------------------------------------------------------------------------------------------------------------------------*/
328+
/// <summary>
329+
/// Adds a new attribute with an empty value, and confirms that it is <i>is</i> added as a new <see
330+
/// cref="AttributeValue"/> assuming the value previously existed. Empty values are treated as the same as non-existent
331+
/// attributes, but they should be stored for the sake of tracking <i>deleted</i> attributes.
332+
/// </summary>
333+
[TestMethod]
334+
public void SetValue_EmptyAttributeValue_Replaces() {
335+
336+
var topic = TopicFactory.Create("Test", "Container");
337+
338+
topic.Attributes.SetValue("Attribute", "New Value");
339+
topic.Attributes.SetValue("Attribute", "");
340+
341+
Assert.IsTrue(topic.Attributes.Contains("Attribute"));
342+
343+
}
344+
306345
/*==========================================================================================================================
307346
| TEST: GET VALUE: INHERIT FROM PARENT: RETURNS PARENT VALUE
308347
\-------------------------------------------------------------------------------------------------------------------------*/

OnTopic/Collections/AttributeValueCollection.cs

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -307,6 +307,16 @@ internal void SetValue(string key, string? value, bool? isDirty, bool enforceBus
307307
this[IndexOf(originalAttribute)] = newAttribute;
308308
}
309309

310+
/*------------------------------------------------------------------------------------------------------------------------
311+
| Ignore if null
312+
>-------------------------------------------------------------------------------------------------------------------------
313+
| ###NOTE JJC20200501: Null or empty values are treated as deletions, and are not persisted to the data store. With
314+
| existing values, these are written to ensure that the collection is marked as IsDirty, thus allowing previous values to
315+
| be overwritten. Non-existent values, however, should simply be ignored.
316+
\-----------------------------------------------------------------------------------------------------------------------*/
317+
else if (String.IsNullOrEmpty(value)) {
318+
}
319+
310320
/*------------------------------------------------------------------------------------------------------------------------
311321
| Create new attribute value
312322
\-----------------------------------------------------------------------------------------------------------------------*/

0 commit comments

Comments
 (0)