Centralized core business logic from SqlTopicRepository.Save()

JeremyCaney · JeremyCaney · commit fdd00258c5db · 2021-01-26T17:48:58.000-08:00
The `SqlTopicRepository.Save()` implementation had a lot of logic that wasn't specific to SQL Server, and would likely need to be repeated by subsequent implementations. To help avoid that, it has been migrated from `SqlTopicRepository.Save()` and into the underlying `TopicRepository.Save()`.

This includes tracking `unresolvedTopics` (i.e., topics whose relationships or references haven't yet been saved) and then resaving them at the end. It also includes the core recursion logic for looping over child topics, if `isRecursive` is set. Finally, it includes establishing a shared `version` to use across all attributes, relationships, and references so all updates that are part of this operation share the same version.

While not yet implemented, this will also allow us to extend the unit testing capabilities to evaluate the above features, as the unit tests don't currently cover `SqlTopicRepository.Save()`, but do cover `TopicRepository.Save()`.

As part of this, the `Save()` method is marked as `sealed` and the core implementation is moved to the `abstract` `Save(topic, version, persistRelationships)` overload. This prevents the need for derived implementations to call `base.Save()`, and likewise allows the `TopicRepository.Save()` implementation more control over the order of operations—e.g., by placing validation logic above the implementation, while putting topic graph operations after it.

This fixes a behavior where an operation would be reflected in the local topic graph even if the database operation failed, which often resulted in confusion since it would appear as though the operation completed based on the cached state of the topic graph. This could also potentially result in bugs in subsequent saves, since those operations would have already been completed.

Finally, this includes updates to the `StubTopicRepository` to `override` the updated `Save()` overload, instead of the `ITopicRepository` version.
diff --git a/OnTopic.Data.Sql/SqlTopicRepository.cs b/OnTopic.Data.Sql/SqlTopicRepository.cs
@@ -12,6 +12,7 @@
 using System.Linq;
 using System.Text;
 using Microsoft.Data.SqlClient;
+using OnTopic.Collections;
 using OnTopic.Data.Sql.Models;
 using OnTopic.Internal.Diagnostics;
 using OnTopic.Querying;
@@ -335,79 +336,16 @@ public override void Refresh(Topic referenceTopic, DateTime since) {
     /*==========================================================================================================================
     | METHOD: SAVE
     \-------------------------------------------------------------------------------------------------------------------------*/
-    /// <inheritdoc />
-    public override void Save([NotNull]Topic topic, bool isRecursive = false) {
-
-      /*------------------------------------------------------------------------------------------------------------------------
-      | Establish dependencies
-      \-----------------------------------------------------------------------------------------------------------------------*/
-      var version               = DateTime.UtcNow;
-      var unresolvedTopics      = new List<Topic>();
-
-      using var connection      = new SqlConnection(_connectionString);
-
-      connection.Open();
-
-      /*------------------------------------------------------------------------------------------------------------------------
-      | Handle first pass
-      \-----------------------------------------------------------------------------------------------------------------------*/
-      Save(topic, isRecursive, connection, unresolvedTopics, version);
-
-      /*------------------------------------------------------------------------------------------------------------------------
-      | Attempt to resolve outstanding relationships
-      \-----------------------------------------------------------------------------------------------------------------------*/
-      foreach (var unresolvedTopic in unresolvedTopics) {
-        Save(unresolvedTopic, false, connection, unresolvedTopics, version);
-      }
-
-      /*------------------------------------------------------------------------------------------------------------------------
-      | Close shared connection
-      \-----------------------------------------------------------------------------------------------------------------------*/
-      connection.Close();
-
-    }
-
-    /// <summary>
-    ///   The private overload of the <see cref="Save"/> method provides support for sharing the <see cref="SqlConnection"/>
-    ///   between multiple requests, and maintaining a list of <paramref name="unresolvedRelationships"/>.
-    /// </summary>
-    /// <remarks>
-    ///   <para>
-    ///     When recursively saving a topic graph, it is conceivable that references to other topics—such as <see
-    ///     cref="Topic.Relationships"/> or <see cref="Topic.DerivedTopic"/>—can't yet be persisted because the target <see
-    ///     cref="Topic"/> hasn't yet been saved, and thus the <see cref="Topic.Id"/> is still set to <c>-1</c>. To mitigate
-    ///     this, the <paramref name="unresolvedRelationships"/> allows this private overload to keep track of unresolved
-    ///     relationships. The public <see cref="Save(Topic, Boolean, Boolean)"/> overload uses this list to resave any topics
-    ///     that include such references. This adds some overhead due to the duplicate <see cref="Save"/>, but helps avoid
-    ///     potential data loss when working with complex topic graphs.
-    ///   </para>
-    ///   <para>
-    ///     The connection sharing probably doesn't provide that much of a gain in that .NET does a good job of connection
-    ///     pooling. Nevertheless, there is some overhead to opening a new connection, so sharing an open connection when we
-    ///     doing a recursive save could potentially provide some performance benefit.
-    ///   </para>
-    /// </remarks>
-    /// <param name="topic">The source <see cref="Topic"/> to save.</param>
-    /// <param name="isRecursive">Determines whether or not to recursively save <see cref="Topic.Children"/>.</param>
-    /// <param name="connection">The open <see cref="SqlConnection"/> to use for executing <see cref="SqlCommand"/>s.</param>
-    /// <param name="unresolvedRelationships">A list of <see cref="Topic"/>s with unresolved topic references.</param>
-    private void Save(
+    /// <inheritdoc/>
+    protected override sealed void Save(
       [NotNull]Topic topic,
-      bool isRecursive,
-      SqlConnection connection,
-      List<Topic> unresolvedRelationships,
-      DateTime version
+      DateTime version,
+      bool persistRelationships
     ) {
 
-      /*------------------------------------------------------------------------------------------------------------------------
-      | Call base method - will trigger any events associated with the save
-      \-----------------------------------------------------------------------------------------------------------------------*/
-      base.Save(topic, isRecursive);
-
       /*------------------------------------------------------------------------------------------------------------------------
       | Define variables
       \-----------------------------------------------------------------------------------------------------------------------*/
-      var areReferencesResolved = true;
       var isTopicDirty          = topic.IsDirty();
       var areRelationshipsDirty = topic.Relationships.IsDirty();
       var areReferencesDirty    = topic.References.IsDirty();
@@ -440,7 +378,6 @@ DateTime version
       | Bypass is not dirty
       \-----------------------------------------------------------------------------------------------------------------------*/
       if (!isDirty) {
-        recurse();
         return;
       }
 
@@ -494,29 +431,15 @@ DateTime version
       /*------------------------------------------------------------------------------------------------------------------------
       | Establish database connection
       \-----------------------------------------------------------------------------------------------------------------------*/
+      using var connection      = new SqlConnection(_connectionString);
       var procedureName         = topic.IsNew? "CreateTopic" : "UpdateTopic";
 
+      connection.Open();
+
       using var command         = new SqlCommand(procedureName, connection) {
         CommandType             = CommandType.StoredProcedure
       };
 
-      /*------------------------------------------------------------------------------------------------------------------------
-      | Handle unresolved references
-      >-------------------------------------------------------------------------------------------------------------------------
-      | If it's a recursive save and there are any unresolved relationships, come back to this after the topic graph has been
-      | saved; that ensures that any relationships within the topic graph have been saved and can be properly persisted. The
-      | same can be done for DerivedTopics references, which are effectively establish a 1:1 relationship.
-      \-----------------------------------------------------------------------------------------------------------------------*/
-      if (
-        isRecursive &&
-        (  topic.Relationships.Any(r => r.Values.Any(t => t.Id < 0)) ||
-           topic.References.Values.Any(t => t.Id < 0)
-        )
-      ) {
-        unresolvedRelationships.Add(topic);
-        areReferencesResolved = false;
-      }
-
       /*------------------------------------------------------------------------------------------------------------------------
       | Establish query parameters
       \-----------------------------------------------------------------------------------------------------------------------*/
@@ -553,18 +476,14 @@ DateTime version
           "The call to the CreateTopic stored procedure did not return the expected 'Id' parameter."
         );
 
-        if (areReferencesResolved && areRelationshipsDirty) {
+        if (persistRelationships && areRelationshipsDirty) {
           PersistRelations(topic, version, connection);
         }
 
-        if (areReferencesResolved && areReferencesDirty) {
+        if (persistRelationships && areReferencesDirty) {
           PersistReferences(topic, version, connection);
         }
 
-        if (!topic.VersionHistory.Contains(version)) {
-          topic.VersionHistory.Insert(0, version);
-        }
-
         topic.Attributes.MarkClean(version);
 
       }
@@ -580,20 +499,10 @@ DateTime version
       }
 
       /*------------------------------------------------------------------------------------------------------------------------
-      | Recuse over any children
+      | Close connection
       \-----------------------------------------------------------------------------------------------------------------------*/
-      recurse();
-
-      /*------------------------------------------------------------------------------------------------------------------------
-      | Function: Recurse
-      \-----------------------------------------------------------------------------------------------------------------------*/
-      void recurse() {
-        if (isRecursive) {
-          foreach (var childTopic in topic.Children) {
-            childTopic.Attributes.SetValue("ParentID", topic.Id.ToString(CultureInfo.InvariantCulture));
-            Save(childTopic, isRecursive, connection, unresolvedRelationships, version);
-          }
-        }
+      finally {
+        connection.Close();
       }
 
     }
diff --git a/OnTopic.TestDoubles/StubTopicRepository.cs b/OnTopic.TestDoubles/StubTopicRepository.cs
@@ -5,6 +5,7 @@
 \=============================================================================================================================*/
 using System;
 using System.Collections.Generic;
+using System.Diagnostics.CodeAnalysis;
 using System.Globalization;
 using OnTopic.Attributes;
 using OnTopic.Internal.Diagnostics;
@@ -111,29 +112,15 @@ public override void Refresh(Topic referenceTopic, DateTime since) { }
     | METHOD: SAVE
     \-------------------------------------------------------------------------------------------------------------------------*/
     /// <inheritdoc />
-    public override void Save(Topic topic, bool isRecursive = false) {
+    protected override void Save([NotNull]Topic topic, DateTime version, bool persistRelationships) {
 
       /*------------------------------------------------------------------------------------------------------------------------
-      | Call base method - will trigger any events associated with the save
-      \-----------------------------------------------------------------------------------------------------------------------*/
-      base.Save(topic, isRecursive);
-
-      /*------------------------------------------------------------------------------------------------------------------------
-      | Recurse through children
+      | Assign faux identity
       \-----------------------------------------------------------------------------------------------------------------------*/
       if (topic.IsNew) {
         topic.Id = _identity++;
       }
 
-      /*------------------------------------------------------------------------------------------------------------------------
-      | Recurse through children
-      \-----------------------------------------------------------------------------------------------------------------------*/
-      if (isRecursive) {
-        foreach (var childTopic in topic.Children) {
-          Save(childTopic, isRecursive);
-        }
-      }
-
     }
 
     /*==========================================================================================================================
diff --git a/OnTopic/Repositories/TopicRepository.cs b/OnTopic/Repositories/TopicRepository.cs
