Merge branch 'feature/Arbitrary-Attribute-Support' into develop

Introduces Programmatic Support for Arbitrary Attributes (#19)

Arbitrary attributes are those added by developers, but which aren't part of the `ContentTypeDescriptor` schema. They may be useful for adding one-off metadata that is useful to add some some topics, but doesn't align well with any individual content type. While rarely used, they're a useful tool for developers—and especially when developing proof-of-concepts (e.g., where the attribute may be formalized later).

Previously, arbitrary attributes could be added to the database. But they couldn't be added, modified, or deleted programmatically using the OnTopic library. In fact, this could be a source of confusion as no error was produced in this scenario; the arbitrary attribute was just ignored entirely. As such, a programmatic modification might appear to work, only to revert when the application restarts.

To mitigate this, while supporting these ad hoc scenarios, we've formalized support for working with arbitrary attributes programmatically, thus providing assurances that they will be properly persisted to the underlying data store. This includes:

- Support for saving arbitrary attributes (c3c01ef)
- Support for deleting arbitrary attributes (2151a08)
- Deleting orphaned indexed attributes (7a61005)
- Excluding empty values from `GetAttributes()` (c035ca0)
- Ignoring phantom attributes on `SetValue()` (a0b659d)

Author: JeremyCaney

OnTopic.Data.Sql/SqlTopicRepository.cs

@@ -306,21 +306,35 @@ public override int Save([NotNull]Topic topic, bool isRecursive = false, bool is
       extendedAttributes.Append("<attributes>");
 
       foreach (var attributeValue in GetAttributes(topic, true, null)) {
+
         extendedAttributes.Append(
           "<attribute key=\"" + attributeValue.Key + "\"><![CDATA[" + attributeValue.Value + "]]></attribute>"
         );
         attributeValue.IsDirty  = false;
+
+        //###NOTE JJC20200502: By treating extended attributes as unmatched, we ensure that any indexed attributes with the same
+        //value are overwritten with an empty attribute. This is useful for cases where an indexed attribute is moved to an
+        //extended attribute, as it persists that version history, while removing ambiguity over which record is authoritative.
+        //This is also useful for supporting arbitrary attribute values, since they may be moved from indexed to extended
+        //attributes if their length exceeds 255.
+        addUnmatchedAttribute(attributeValue.Key);
+
       }
 
       extendedAttributes.Append("</attributes>");
 
       /*------------------------------------------------------------------------------------------------------------------------
-      | Add unmatch attributes
+      | Add unmatched attributes
       \-----------------------------------------------------------------------------------------------------------------------*/
+
       //Loop through the content type's supported attributes and add attribute to null attributes if topic does not contain it
       foreach (var attribute in GetUnmatchedAttributes(topic)) {
+        addUnmatchedAttribute(attribute.Key);
+      }
+
+      void addUnmatchedAttribute(string key) {
         var record              = attributes.NewRow();
-        record["AttributeKey"]  = attribute.Key;
+        record["AttributeKey"]  = key;
         record["AttributeValue"]= null;
         attributes.Rows.Add(record);
       }

OnTopic.Tests/AttributeValueCollectionTest.cs

@@ -303,6 +303,45 @@ public void Add_InvalidAttributeValue_ThrowsException() {
       topic.Attributes.Add(new AttributeValue("Key", "# ?"));
     }
 
+    /*==========================================================================================================================
+    | TEST: SET VALUE: EMPTY ATTRIBUTE VALUE: SKIPS
+    \-------------------------------------------------------------------------------------------------------------------------*/
+    /// <summary>
+    ///   Adds a new attribute with an empty value, and confirms that it is <i>not</i> added as a new <see
+    ///   cref="AttributeValue"/>. Empty values are treated as the same as non-existent attributes. They are stored for the sake
+    ///   of tracking <i>deleted</i> attributes, but should not be stored for <i>new</i> attributes.
+    /// </summary>
+    [TestMethod]
+    public void SetValue_EmptyAttributeValue_Skips() {
+
+      var topic = TopicFactory.Create("Test", "Container");
+
+      topic.Attributes.SetValue("Attribute", "");
+
+      Assert.IsFalse(topic.Attributes.Contains("Attribute"));
+
+    }
+
+    /*==========================================================================================================================
+    | TEST: SET VALUE: UPDATE EMPTY ATTRIBUTE VALUE: REPLACES
+    \-------------------------------------------------------------------------------------------------------------------------*/
+    /// <summary>
+    ///   Adds a new attribute with an empty value, and confirms that it is <i>is</i> added as a new <see
+    ///   cref="AttributeValue"/> assuming the value previously existed. Empty values are treated as the same as non-existent
+    ///   attributes, but they should be stored for the sake of tracking <i>deleted</i> attributes.
+    /// </summary>
+    [TestMethod]
+    public void SetValue_EmptyAttributeValue_Replaces() {
+
+      var topic = TopicFactory.Create("Test", "Container");
+
+      topic.Attributes.SetValue("Attribute", "New Value");
+      topic.Attributes.SetValue("Attribute", "");
+
+      Assert.IsTrue(topic.Attributes.Contains("Attribute"));
+
+    }
+
     /*==========================================================================================================================
     | TEST: GET VALUE: INHERIT FROM PARENT: RETURNS PARENT VALUE
     \-------------------------------------------------------------------------------------------------------------------------*/

OnTopic.Tests/TopicRepositoryBaseTest.cs

@@ -68,6 +68,29 @@ public void GetAttributes_AnyAttributes_ReturnsAllAttributes() {
 
     }
 
+    /*==========================================================================================================================
+    | TEST: GET ATTRIBUTES: EMPTY ATTRIBUTES: SKIPS
+    \-------------------------------------------------------------------------------------------------------------------------*/
+    /// <summary>
+    ///   Retrieves a list of attributes from a topic, without any filtering by whether or not the attribute is an <see
+    ///   cref="AttributeDescriptor.IsExtendedAttribute"/>. Any <see cref="AttributeValue"/>s with a null or empty value should
+    ///   be skipped.
+    /// </summary>
+    [TestMethod]
+    public void GetAttributes_EmptyAttributes_Skips() {
+
+      var topic                 = TopicFactory.Create("Test", "ContentTypes");
+
+      topic.Attributes.SetValue("EmptyAttribute", "");
+      topic.Attributes.SetValue("NullAttribute", null);
+
+      var attributes            = _topicRepository.GetAttributesProxy(topic, null);
+
+      Assert.IsFalse(attributes.Any(a => a.Key == "EmptyAttribute"));
+      Assert.IsFalse(attributes.Any(a => a.Key == "NullAttribute"));
+
+    }
+
     /*==========================================================================================================================
     | TEST: GET ATTRIBUTES: INDEXED ATTRIBUTES: RETURNS INDEXED ATTRIBUTES
     \-------------------------------------------------------------------------------------------------------------------------*/
@@ -106,6 +129,48 @@ public void GetAttributes_ExtendedAttributes_ReturnsExtendedAttributes() {
 
     }
 
+    /*==========================================================================================================================
+    | TEST: GET ATTRIBUTES: ARBITRARY ATTRIBUTE WITH SHORT VALUE: RETURNS AS INDEXED ATTRIBUTE
+    \-------------------------------------------------------------------------------------------------------------------------*/
+    /// <summary>
+    ///   Sets an arbitrary (unmatched) attribute on a <see cref="Topic"/> with a value shorter than 255 characters, then
+    ///   ensures that it is returned as an an <i>indexed</i> <see cref="AttributeValue"/> when calling <see
+    ///   cref="TopicRepositoryBase.GetAttributes(Topic, Boolean?, Boolean?)"/>.
+    /// </summary>
+    [TestMethod]
+    public void GetAttributes_ArbitraryAttributeWithShortValue_ReturnsAsIndexedAttributes() {
+
+      var topic                 = TopicFactory.Create("Test", "ContentTypes");
+
+      topic.Attributes.SetValue("ArbitraryAttribute", "Value");
+
+      var attributes            = _topicRepository.GetAttributesProxy(topic, false);
+
+      Assert.IsTrue(attributes.Any(a => a.Key.Equals("ArbitraryAttribute", StringComparison.InvariantCultureIgnoreCase)));
+
+    }
+
+    /*==========================================================================================================================
+    | TEST: GET ATTRIBUTES: ARBITRARY ATTRIBUTE WITH LONG VALUE: RETURNS AS EXTENDED ATTRIBUTE
+    \-------------------------------------------------------------------------------------------------------------------------*/
+    /// <summary>
+    ///   Sets an arbitrary (unmatched) attribute on a <see cref="Topic"/> with a value longer than 255 characters, then
+    ///   ensures that it is returned as an an <see cref="AttributeDescriptor.IsExtendedAttribute"/> when calling <see
+    ///   cref="TopicRepositoryBase.GetAttributes(Topic, Boolean?, Boolean?)"/>.
+    /// </summary>
+    [TestMethod]
+    public void GetAttributes_ArbitraryAttributeWithLongValue_ReturnsAsExtendedAttributes() {
+
+      var topic                 = TopicFactory.Create("Test", "ContentTypes");
+
+      topic.Attributes.SetValue("ArbitraryAttribute", new string('x', 256));
+
+      var attributes            = _topicRepository.GetAttributesProxy(topic, true);
+
+      Assert.IsTrue(attributes.Any(a => a.Key.Equals("ArbitraryAttribute", StringComparison.InvariantCultureIgnoreCase)));
+
+    }
+
     /*==========================================================================================================================
     | TEST: GET UNMATCHED ATTRIBUTES: RETURNS ATTRIBUTES
     \-------------------------------------------------------------------------------------------------------------------------*/
@@ -127,6 +192,34 @@ public void GetUnmatchedAttributes_ReturnsAttributes() {
 
     }
 
+    /*==========================================================================================================================
+    | TEST: GET UNMATCHED ATTRIBUTES: EMPTY ARBITRARY ATTRIBUTES: RETURNS ATTRIBUTES
+    \-------------------------------------------------------------------------------------------------------------------------*/
+    /// <summary>
+    ///   Using <see cref="TopicRepositoryBase.GetUnmatchedAttributes(Topic)"/>, ensures that any attributes that exist on the
+    ///   <see cref="Topic"/> but not the <see cref="ContentTypeDescriptor"/> <i>and</i> are either <c>null</c> or empty are
+    ///   returned. This ensures that arbitrary attributes can be deleted programmatically, instead of lingering as orphans in
+    ///   the database.
+    /// </summary>
+    [TestMethod]
+    public void GetUnmatchedAttributes_EmptyArbitraryAttributes_ReturnsAttributes() {
+
+      var topic                 = TopicFactory.Create("Test", "ContentTypeDescriptor", 1);
+
+      topic.Attributes.SetValue("ArbitraryAttribute", "Value");
+      topic.Attributes.SetValue("ArbitraryAttribute", "");
+      topic.Attributes.SetValue("AnotherArbitraryAttribute", "Value");
+      topic.Attributes.SetValue("YetAnotherArbitraryAttribute", "Value");
+      topic.Attributes.SetValue("YetAnotherArbitraryAttribute", null);
+
+      var attributes            = _topicRepository.GetUnmatchedAttributesProxy(topic);
+
+      Assert.IsTrue(attributes.Any(a => a.Key.Equals("ArbitraryAttribute", StringComparison.InvariantCulture)));
+      Assert.IsTrue(attributes.Any(a => a.Key.Equals("YetAnotherArbitraryAttribute", StringComparison.InvariantCulture)));
+      Assert.IsFalse(attributes.Any(a => a.Key.Equals("AnotherArbitraryAttribute", StringComparison.InvariantCulture)));
+
+    }
+
     /*==========================================================================================================================
     | TEST: GET CONTENT TYPE DESCRIPTORS: RETURNS CONTENT TYPES
     \-------------------------------------------------------------------------------------------------------------------------*/

OnTopic/Collections/AttributeValueCollection.cs

@@ -307,6 +307,16 @@ internal void SetValue(string key, string? value, bool? isDirty, bool enforceBus
         this[IndexOf(originalAttribute)] = newAttribute;
       }
 
+      /*------------------------------------------------------------------------------------------------------------------------
+      | 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.
+      \-----------------------------------------------------------------------------------------------------------------------*/
+      else if (String.IsNullOrEmpty(value)) {
+      }
+
       /*------------------------------------------------------------------------------------------------------------------------
       | Create new attribute value
       \-----------------------------------------------------------------------------------------------------------------------*/

OnTopic/Repositories/TopicRepositoryBase.cs

@@ -502,7 +502,16 @@ protected IEnumerable<AttributeValue> GetAttributes(Topic topic, bool? isExtende
           attribute             = contentType.AttributeDescriptors[key];
         }
 
-        if (attribute != null && (isExtendedAttribute == null || attribute.IsExtendedAttribute == isExtendedAttribute)) {
+        //Skip if the value is null or empty; these values are not persisted to storage and should be treated as equivalent to
+        //non-existent values.
+        if (String.IsNullOrEmpty(attributeValue.Value)) {
+          continue;
+        }
+
+        //Add the attribute based on the isExtendedAttribute paramter. Add all parameters if isExtendedAttribute is null. Assume
+        //an attribute is extended if the corresponding attribute descriptor cannot be located and the value is over 255
+        //characters.
+        if (isExtendedAttribute?.Equals(attribute?.IsExtendedAttribute?? attributeValue.Value?.Length > 255)?? true) {
           attributes.Add(attributeValue);
         }
 
@@ -538,9 +547,9 @@ protected IEnumerable<AttributeDescriptor> GetUnmatchedAttributes(Topic topic) {
       );
 
       /*------------------------------------------------------------------------------------------------------------------------
-      | Get unmatched attributes
+      | Get unmatched attribute descriptors
       \-----------------------------------------------------------------------------------------------------------------------*/
-      var attributes            = new List<AttributeDescriptor>();
+      var attributes            = new TopicCollection<AttributeDescriptor>();
 
       foreach (var attribute in contentType.AttributeDescriptors) {
 
@@ -571,6 +580,21 @@ protected IEnumerable<AttributeDescriptor> GetUnmatchedAttributes(Topic topic) {
 
       }
 
+      /*------------------------------------------------------------------------------------------------------------------------
+      | Get arbitrary attributes
+      >-------------------------------------------------------------------------------------------------------------------------
+      | ###HACK JJC20200502: Arbitrary attributes are those that don't map back to the scheme. These aren't picked up by the
+      | AttributeDescriptors check above. This means there's no way to programmatically delete arbitrary (or orphaned)
+      | attributes. To mitigate this, any null or empty attribute values should be included. By definition, though, arbitrary
+      | attributes don't have corresponding AttributeDescriptors. To mitigate this, an ad hoc AttributeDescriptor object will be
+      | created for each empty AttributeDescriptor.
+      \-----------------------------------------------------------------------------------------------------------------------*/
+      foreach (var attribute in topic.Attributes.Where(a => String.IsNullOrEmpty(a.Value))) {
+        if (!attributes.Contains(attribute.Key)) {
+          attributes.Add((TextAttribute)TopicFactory.Create(attribute.Key, "TextAttribute"));
+        }
+      }
+
       /*------------------------------------------------------------------------------------------------------------------------
       | Return values
       \-----------------------------------------------------------------------------------------------------------------------*/