Merge branch 'bugfix/extended-attributes-mismatch' into develop

Currently, if an `AttributeDescriptor`'s `IsExtendedAttribute` value changes, the configuration value changes, but the actual storage location of _existing_ attributes isn't updated. That's because the `AttributeDescriptor` is primarily used by the `ITopicRepository.Save()` method as well as the **OnTopic Editor**, but doesn't have the ability to _trigger_ broad-sweeping `ITopicRepository.Save()` operations.

This means that the storage location of an attribute may not be persisted until the next time that `AttributeValue` is saved. This can cause a problem, however, as previously, only attributes marked as `IsDirty` would be persisted as indexed attributes. As such, if `IsExtendedAttribute` was changed to _`true`_, then the value would be correctly removed from the indexed attributes storage and added to the extended attributes storage. But if the opposite were the case, it would be successfully removed from extended storage—but not added indexed storage. Ooops!

In practice, this is probably a rare scenario. We default to storing attributes as indexed. And we only store them as `IsExtendedAttribute` if their value exceeds the maximum character limit for an indexed attribute (currently 255 characters). However, in the rare case that this happens, the consequences would be pretty significant. And because this data loss wouldn't occur until any one topic was saved, it may be difficult to track down and account for the cause.

Unfortunately, despite being an edge case, the solution to this is fairly complicated. This bugfix includes the addition of an `IsExtendedAttribute` property to `AttributeValue`, a new `isExtendedAttribute` parameter on `AttributeValueCollection.SetValue()`, a new `IsExtendedAttributeMismatch()` helper in `TopicRepositoryBase`, and a new `excludeLastModified` parameter to the `TopicRepositoryBase.GetAttributes()` method in order to allow returning `IsExtendedAttributeMismatch()`ed when processing indexed attributes without interfering with the logic for byline (`LastModifiedBy`) and dateline (`LastModified`) processing. Whew!

That said, despite the additional _complexity_, this introduces minimal _overhead_ in terms of loading or even saving data. And it prevents a potentially confusing bug that would be difficult and frustrating to recover from if it were tripped. (And, in fact, it has already been tripped a few times—though only with configuration metadata that wasn't critical to the operation of the site.)

Author: JeremyCaney

OnTopic.Data.Sql/SqlDataReaderExtensions.cs

@@ -203,7 +203,7 @@ private static void SetIndexedAttributes(this SqlDataReader reader, Dictionary<i
       /*------------------------------------------------------------------------------------------------------------------------
       | Set attribute value
       \-----------------------------------------------------------------------------------------------------------------------*/
-      current.Attributes.SetValue(attributeKey, attributeValue, false, version);
+      current.Attributes.SetValue(attributeKey, attributeValue, false, version, false);
 
     }
 
@@ -274,7 +274,7 @@ private static void SetExtendedAttributes(this SqlDataReader reader, Dictionary<
         | Set attribute value
         \---------------------------------------------------------------------------------------------------------------------*/
         if (String.IsNullOrEmpty(attributeValue)) continue;
-        current.Attributes.SetValue(attributeKey, attributeValue, false, version);
+        current.Attributes.SetValue(attributeKey, attributeValue, false, version, true);
 
       } while (xmlReader.Name == "attribute");
 

OnTopic.Data.Sql/SqlTopicRepository.cs

@@ -289,22 +289,26 @@ public override int Save([NotNull]Topic topic, bool isRecursive = false, bool is
       /*------------------------------------------------------------------------------------------------------------------------
       | Add indexed attributes that are dirty
       \-----------------------------------------------------------------------------------------------------------------------*/
-      //Exclude indexed attributes if the only dirty attributes are `LastModified` and/or `LastModifiedBy`, as these are
-      //automatically generated and don't represent genuine changes to the topic.
-      if (topic.Attributes.IsDirty(true)) {
 
-        var indexedAttributes   = GetAttributes(topic, false, true);
+      //Only include `LastModified` and/or `LastModifiedBy` if other attributes in the collection are `IsDirty`, as these are
+      //automatically generated and don't represent genuine changes to the topic.
+      var excludeLastModified   = !topic.Attributes.IsDirty(excludeLastModified: true);
 
-        foreach (var attributeValue in indexedAttributes) {
+      var indexedAttributes     = GetAttributes(
+        topic                   : topic,
+        isExtendedAttribute     : false,
+        isDirty                 : true,
+        excludeLastModified     : excludeLastModified
+      );
 
-          var record              = attributes.NewRow();
-          record["AttributeKey"]  = attributeValue.Key;
-          record["AttributeValue"]= attributeValue.Value;
-          attributeValue.IsDirty  = false;
+      foreach (var attributeValue in indexedAttributes) {
 
-          attributes.Rows.Add(record);
+        var record              = attributes.NewRow();
+        record["AttributeKey"]  = attributeValue.Key;
+        record["AttributeValue"]= attributeValue.Value;
+        attributeValue.IsDirty  = false;
 
-        }
+        attributes.Rows.Add(record);
 
       }
 
@@ -313,7 +317,7 @@ public override int Save([NotNull]Topic topic, bool isRecursive = false, bool is
       \-----------------------------------------------------------------------------------------------------------------------*/
       extendedAttributes.Append("<attributes>");
 
-      foreach (var attributeValue in GetAttributes(topic, true, null)) {
+      foreach (var attributeValue in GetAttributes(topic, isExtendedAttribute:true)) {
 
         extendedAttributes.Append(
           "<attribute key=\"" + attributeValue.Key + "\"><![CDATA[" + attributeValue.Value + "]]></attribute>"

OnTopic.TestDoubles/StubTopicRepository.cs

@@ -168,8 +168,12 @@ public override void Move(Topic topic, Topic target, Topic? sibling) {
     | METHOD: GET ATTRIBUTES (PROXY)
     \-------------------------------------------------------------------------------------------------------------------------*/
     /// <inheritdoc cref="TopicRepositoryBase.GetAttributes(Topic, Boolean?, Boolean?)" />
-    public IEnumerable<AttributeValue> GetAttributesProxy(Topic topic, bool? isExtendedAttribute, bool? isDirty = null) =>
-      base.GetAttributes(topic, isExtendedAttribute, isDirty);
+    public IEnumerable<AttributeValue> GetAttributesProxy(
+      Topic topic,
+      bool? isExtendedAttribute,
+      bool? isDirty = null,
+      bool excludeLastModified = false
+    ) => base.GetAttributes(topic, isExtendedAttribute, isDirty, excludeLastModified);
 
     /*==========================================================================================================================
     | METHOD: GET UNMATCHED ATTRIBUTES (PROXY)

OnTopic.Tests/TopicRepositoryBaseTest.cs

@@ -129,13 +129,80 @@ public void GetAttributes_ExtendedAttributes_ReturnsExtendedAttributes() {
 
     }
 
+    /*==========================================================================================================================
+    | TEST: GET ATTRIBUTES: EXTENDED ATTRIBUTE MISMATCH: RETURNS EXTENDED ATTRIBUTES
+    \-------------------------------------------------------------------------------------------------------------------------*/
+    /// <summary>
+    ///   Retrieves a list of attributes from a topic, filtering by <see cref="AttributeValue.IsDirty"/>. Expects an <see
+    ///   cref="AttributeValue"/> to be returned even if it's <i>not</i> <see cref="AttributeValue.IsDirty"/> <i>but</i> its
+    ///   <see cref="AttributeValue.IsExtendedAttribute"/> disagrees with <see cref="AttributeDescriptor.IsExtendedAttribute"/>.
+    /// </summary>
+    [TestMethod]
+    public void GetAttributes_ExtendedAttributeMismatch_ReturnsExtendedAttributes() {
+
+      var topic                 = TopicFactory.Create("Test", "ContentTypes");
+
+      topic.Attributes.SetValue("Title", "Title", isDirty:false, isExtendedAttribute:false);
+
+      var attributes            = _topicRepository.GetAttributesProxy(topic, true, true);
+
+      //Expect Title, even though it isn't IsDirty
+      Assert.AreEqual<int>(1, attributes.Count());
+
+    }
+
+    /*==========================================================================================================================
+    | TEST: GET ATTRIBUTES: EXTENDED ATTRIBUTE MISMATCH: RETURNS NOTHING
+    \-------------------------------------------------------------------------------------------------------------------------*/
+    /// <summary>
+    ///   Retrieves a list of attributes from a topic, filtering by <see cref="AttributeValue.IsDirty"/>. Expects the <see
+    ///   cref="AttributeValue"/> to <i>not</i> be returned even though its <see cref="AttributeValue.IsExtendedAttribute"/>
+    ///   disagrees with <see cref="AttributeDescriptor.IsExtendedAttribute"/>, since it won't match the <see
+    ///   cref="TopicRepositoryBase.GetAttributes(Topic, Boolean?, Boolean?, Boolean)"/>'s <c>isExtendedAttribute</c> call.
+    /// </summary>
+    [TestMethod]
+    public void GetAttributes_ExtendedAttributeMismatch_ReturnsNothing() {
+
+      var topic                 = TopicFactory.Create("Test", "ContentTypes");
+
+      topic.Attributes.SetValue("Title", "Title", isDirty: false, isExtendedAttribute: false);
+
+      var attributes            = _topicRepository.GetAttributesProxy(topic, false, true);
+
+      //Expect Key and ContentType, but not Title
+      Assert.AreEqual<int>(2, attributes.Count());
+
+    }
+
+    /*==========================================================================================================================
+    | TEST: GET ATTRIBUTES: EXCLUDE LAST MODIFIED: RETURNS OTHER ATTRIBUTES
+    \-------------------------------------------------------------------------------------------------------------------------*/
+    /// <summary>
+    ///   Retrieves a list of attributes from a topic, filtering by <c>excludeLastModified</c>. Confirms that <see
+    ///   cref="AttributeValue"/>s are not returned which start with <c>LastModified</c>.
+    /// </summary>
+    [TestMethod]
+    public void GetAttributes_ExcludeLastModified_ReturnsOtherAttributes() {
+
+      var topic                 = TopicFactory.Create("Test", "ContentTypes");
+
+      topic.Attributes.SetDateTime("LastModified", DateTime.Now);
+      topic.Attributes.SetValue("LastModifiedBy", "Unit Tests");
+
+      var attributes            = _topicRepository.GetAttributesProxy(topic, null, excludeLastModified: true);
+
+      //Expected to return Key and ContentType, butnot LastModified or LastModifiedBy
+      Assert.AreEqual<int>(2, attributes.Count());
+
+    }
+
     /*==========================================================================================================================
     | 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?)"/>.
+    ///   cref="TopicRepositoryBase.GetAttributes(Topic, Boolean?, Boolean?, Boolean)"/>.
     /// </summary>
     [TestMethod]
     public void GetAttributes_ArbitraryAttributeWithShortValue_ReturnsAsIndexedAttributes() {
@@ -156,7 +223,7 @@ public void GetAttributes_ArbitraryAttributeWithShortValue_ReturnsAsIndexedAttri
     /// <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?)"/>.
+    ///   cref="TopicRepositoryBase.GetAttributes(Topic, Boolean?, Boolean?, Boolean)"/>.
     /// </summary>
     [TestMethod]
     public void GetAttributes_ArbitraryAttributeWithLongValue_ReturnsAsExtendedAttributes() {
@@ -297,7 +364,7 @@ public void GetContentTypeDescriptor_GetInvalidContentType_ReturnsNull() {
     | TEST: SAVE: CONTENT TYPE DESCRIPTOR: UPDATES CONTENT TYPE CACHE
     \-------------------------------------------------------------------------------------------------------------------------*/
     /// <summary>
-    ///   Loads the <see cref="TopicRepositoryBase.GetAttributes(Topic, Boolean?, Boolean?)"/>, then saves a new <see
+    ///   Loads the <see cref="TopicRepositoryBase.GetAttributes(Topic, Boolean?, Boolean?, Boolean)"/>, then saves a new <see
     ///   cref="ContentTypeDescriptor"/> via <see cref="TopicRepositoryBase.Save(Topic, Boolean, Boolean)"/>, and ensures that
     ///   it is immediately reflected in the <see cref="TopicRepositoryBase"/> cache of <see cref="ContentTypeDescriptor"/>s.
     /// </summary>
@@ -317,9 +384,9 @@ public void Save_ContentTypeDescriptor_UpdatesContentTypeCache() {
     | TEST: DELETE: CONTENT TYPE DESCRIPTOR: UPDATES CONTENT TYPE CACHE
     \-------------------------------------------------------------------------------------------------------------------------*/
     /// <summary>
-    ///   Loads the <see cref="TopicRepositoryBase.GetAttributes(Topic, Boolean?, Boolean?)"/>, then deletes one of the <see
-    ///   cref="ContentTypeDescriptor"/>s via <see cref="TopicRepositoryBase.Delete(Topic, Boolean)"/>, and ensures that it is
-    ///   immediately reflected in the <see cref="TopicRepositoryBase"/> cache of <see cref="ContentTypeDescriptor"/>s.
+    ///   Loads the <see cref="TopicRepositoryBase.GetAttributes(Topic, Boolean?, Boolean?, Boolean)"/>, then deletes one of the
+    ///   <see cref="ContentTypeDescriptor"/>s via <see cref="TopicRepositoryBase.Delete(Topic, Boolean)"/>, and ensures that it
+    ///   is immediately reflected in the <see cref="TopicRepositoryBase"/> cache of <see cref="ContentTypeDescriptor"/>s.
     /// </summary>
     [TestMethod]
     public void Delete_ContentTypeDescriptor_UpdatesContentTypeCache() {
@@ -337,8 +404,8 @@ public void Delete_ContentTypeDescriptor_UpdatesContentTypeCache() {
     | TEST: MOVE: CONTENT TYPE DESCRIPTOR: UPDATES CONTENT TYPE CACHE
     \-------------------------------------------------------------------------------------------------------------------------*/
     /// <summary>
-    ///   Loads the <see cref="TopicRepositoryBase.GetAttributes(Topic, Boolean?, Boolean?)"/>, then moves one of the <see
-    ///   cref="ContentTypeDescriptor"/>s via <see cref="TopicRepositoryBase.Move(Topic, Topic)"/>, and ensures that it is
+    ///   Loads the <see cref="TopicRepositoryBase.GetAttributes(Topic, Boolean?, Boolean?, Boolean)"/>, then moves one of the
+    ///   <see cref="ContentTypeDescriptor"/>s via <see cref="TopicRepositoryBase.Move(Topic, Topic)"/>, and ensures that it is
     ///   immediately reflected in the <see cref="TopicRepositoryBase"/> cache of <see cref="ContentTypeDescriptor"/>s.
     /// </summary>
     [TestMethod]

OnTopic/Attributes/AttributeValue.cs

@@ -33,7 +33,8 @@ namespace OnTopic.Attributes {
   ///   <para>
   ///     This class is immutable: once it is constructed, the values cannot be changed. To change a value, callers must either
   ///     create a new instance of the <see cref="AttributeValue"/> class or, preferably, call the
-  ///     <see cref="Topic.Attributes"/>'s <see cref="AttributeValueCollection.SetValue(String, String, Boolean?, DateTime?)"/>
+  ///     <see cref="Topic.Attributes"/>'s <see cref="AttributeValueCollection.SetValue(String, String, Boolean?, DateTime?,
+  ///     Boolean?)"/>
   ///     method.
   ///   </para>
   /// </remarks>
@@ -98,6 +99,7 @@ public AttributeValue(string key, string? value, bool isDirty = true) {
     ///   populating the topic graph from a persistent data store as a means of indicating the current version for each
     ///   attribute. This is used when e.g. importing values to determine if the existing value is newer than the source value.
     /// </param>
+    /// <param name="isExtendedAttribute">Determines if the attribute originated from an extended attributes data store.</param>
     /// <requires
     ///   description="The key must be specified for the key/value pair." exception="T:System.ArgumentNullException">
     ///   !String.IsNullOrWhiteSpace(key)
@@ -107,14 +109,16 @@ internal AttributeValue(
       string? value,
       bool isDirty,
       bool enforceBusinessLogic,
-      DateTime? lastModified = null
+      DateTime? lastModified = null,
+      bool? isExtendedAttribute = null
     ): this(
       key,
       value,
       isDirty
     ) {
       EnforceBusinessLogic      = enforceBusinessLogic;
       LastModified              = lastModified?? DateTime.Now;
+      IsExtendedAttribute       = isExtendedAttribute;
     }
 
     /*==========================================================================================================================
@@ -165,13 +169,14 @@ internal AttributeValue(
     /// </summary>
     /// <remarks>
     ///   By default, when a user attempts to update an attribute's value by calling <see
-    ///   cref="AttributeValueCollection.SetValue(String, String, Boolean?, DateTime?)"/>, or when an <see cref="AttributeValue"
-    ///   /> is added to the <see cref="AttributeValueCollection"/>, the <see cref="AttributeValueCollection"/> will
-    ///   automatically attempt to call any corresponding setters on <see cref="Topic"/> (or a derived instance) to ensure that
-    ///   the business logic is enforced. To avoid an infinite loop, however, this is disabled when properties on <see
-    ///   cref="Topic"/> call <see cref="Topic.SetAttributeValue(String, String, Boolean?)"/>. When that happens, the
-    ///   <see cref="EnforceBusinessLogic"/> value is set to false to communicate to the <see cref="AttributeValueCollection"/>
-    ///   that it should not call the local property. This value is only intended for internal use.
+    ///   cref="AttributeValueCollection.SetValue(String, String, Boolean?, DateTime?, Boolean?)"/>, or when an <see
+    ///   cref="AttributeValue"/> is added to the <see cref="AttributeValueCollection"/>, the <see
+    ///   cref="AttributeValueCollection"/> will automatically attempt to call any corresponding setters on <see cref="Topic"/>
+    ///   (or a derived instance) to ensure that the business logic is enforced. To avoid an infinite loop, however, this is
+    ///   disabled when properties on <see cref="Topic"/> call <see cref="Topic.SetAttributeValue(String, String, Boolean?)"/>.
+    ///   When that happens, the <see cref="EnforceBusinessLogic"/> value is set to false to communicate to the <see
+    ///   cref="AttributeValueCollection"/> that it should not call the local property. This value is only intended for internal
+    ///   use.
     /// </remarks>
     /// <requires description="The value from the getter must be specified." exception="T:System.ArgumentNullException">
     ///   !String.IsNullOrWhiteSpace(value)
@@ -191,5 +196,36 @@ internal AttributeValue(
     /// </summary>
     public DateTime LastModified { get; internal set; } = DateTime.Now;
 
+    /*==========================================================================================================================
+    | PROPERTY: IS EXTENDED ATTRIBUTE
+    \-------------------------------------------------------------------------------------------------------------------------*/
+    /// <summary>
+    ///   Determines if this attribute originated from a data store as an extended attribute.
+    /// </summary>
+    /// <remarks>
+    ///   <para>
+    ///     How an attribute is stored in the underlying repository doesn't impact how the attribute is treated as part of the
+    ///     object model. By tracking this, however, OnTopic is able to evaluate configuration mismatches during <see
+    ///     cref="ITopicRepository.Save"/>. This allows the <see cref="ITopicRepository"/> to effective handle scenarios where
+    ///     the configuration for an <see cref="AttributeDescriptor"/> has changed prior to the last time a <see cref="Topic"/>
+    ///     was saved, and thus change the location where it is stored.
+    ///   </para>
+    ///   <para>
+    ///     This is important because, otherwise, <see cref="ITopicRepository"/> implementations rely primarily on <see
+    ///     cref="IsDirty"/> to determine if a value should be saved. If an attribute's value hasn't changed, but the location
+    ///     it should be stored has, that could potentially result in the attribute being deleted, as the attribute won't show
+    ///     up for when <see cref="TopicRepositoryBase.GetAttributes"/> is called with <c>isDirty</c> set to <c>true</c> and
+    ///     <c>isExtendedAttribute</c> is set to either <c>true</c> or <c>false</c>. By introducing <see
+    ///     cref="IsExtendedAttribute"/>, the <see cref="TopicRepositoryBase"/> is able to detect conflicts between the
+    ///     configuration and the underlying data store, and ensure data is stored appropriately.
+    ///   </para>
+    ///   <para>
+    ///     The <see cref="IsExtendedAttribute"/> property maps to the <see cref="AttributeDescriptor.IsExtendedAttribute"/>
+    ///     property. The former describes where the data was <i>actually</i> stored, whereas the latter describes where the
+    ///     data <i>should</i> be stored.
+    ///   </para>
+    /// </remarks>
+    public bool? IsExtendedAttribute { get; internal set; }
+
   } //Class
 } //Namespace