Merge branch 'feature/unresolved-reference-handling' into develop

JeremyCaney · JeremyCaney · commit 913b68dc31d4 · 2020-05-26T17:44:51.000-07:00
When recursively saving a topic graph, it is possible for a topic being saved to reference a new topic in the graph that hasn't yet been saved. This prevents it from resolving that reference. I.e., while the _object model_ is able to model that reference, it can't be _persisted_ to the underlying SQL database since the target topic doesn't yet have a `Topic.Id`.

Previously, this required calling `Save()` twice in this scenario. Now, a new private overload allows a `List&lt;Topic&gt;` to be maintained of `unresolvedTopics`. Then, once the entire graph has been saved, `Save()` will save each individual topic with unresolved references, thus ensuring that they are properly persisted. This works for both `Topic.Relationships` and `Topic.DerivedTopic` references.

There are limitations to this implementation. It isn't able to resolve _implicit_ topic pointers (i.e., attributes with an `Id` extension) since those aren't mapped via the object model, but rather runtime interpretation. As such, when working with the **OnTopic Data Transfer** library, which includes special handling for implicit topic pointers, `Save()` must be called twice (alongside `Import()`). That's expected, and there isn't really a clean workaround.

Second, obviously any references that are outside the scope of the current recursive `Save()` operation will continue to be unresolved. This shouldn't happen in most use cases, but could occur with some custom configuration scripts. That's also expected, and not something we plan on resolving.

Still, this addresses the most common and otherwise unintuitive issue, without adding much overhead to most `Save()` operations. (Obviously, the overhead will depend on how many unresolved references there are.)

While I was at it, I also added support for _explicit_ connection handling on recursive saves. This probably provide minimal improvement over performance, but it prevents the need to open and close a connection for every topic on a recursive `Save()` of a large topic graph.
diff --git a/OnTopic.Data.Sql/SqlTopicRepository.cs b/OnTopic.Data.Sql/SqlTopicRepository.cs
@@ -4,6 +4,7 @@
 | Project       Topics Library
 \=============================================================================================================================*/
 using System;
+using System.Collections.Generic;
 using System.Data;
 using System.Data.SqlTypes;
 using System.Diagnostics.CodeAnalysis;
@@ -264,6 +265,68 @@ public override Topic Load(int topicId, DateTime version) {
     /// <inheritdoc />
     public override int Save([NotNull]Topic topic, bool isRecursive = false, bool isDraft = false) {
 
+      /*------------------------------------------------------------------------------------------------------------------------
+      | Establish dependencies
+      \-----------------------------------------------------------------------------------------------------------------------*/
+      var unresolvedTopics      = new List<Topic>();
+      var connection            = new SqlConnection(_connectionString);
+
+      connection.Open();
+
+      /*------------------------------------------------------------------------------------------------------------------------
+      | Handle first pass
+      \-----------------------------------------------------------------------------------------------------------------------*/
+      var topicId = Save(topic, isRecursive, isDraft, connection, unresolvedTopics);
+
+      /*------------------------------------------------------------------------------------------------------------------------
+      | Attempt to resolve outstanding relationships
+      \-----------------------------------------------------------------------------------------------------------------------*/
+      foreach (var unresolvedTopic in unresolvedTopics) {
+        Save(unresolvedTopic, false, isDraft, connection, new List<Topic>());
+      }
+
+      /*------------------------------------------------------------------------------------------------------------------------
+      | Return value
+      \-----------------------------------------------------------------------------------------------------------------------*/
+      connection.Close();
+      connection.Dispose();
+      return topicId;
+
+    }
+
+    /// <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="isDraft">Determines if the <see cref="Topic"/> should be saved as a draft version.</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 int Save(
+      [NotNull]Topic topic,
+      bool isRecursive,
+      bool isDraft,
+      SqlConnection connection,
+      List<Topic> unresolvedRelationships
+    ) {
+
       /*------------------------------------------------------------------------------------------------------------------------
       | Call base method - will trigger any events associated with the save
       \-----------------------------------------------------------------------------------------------------------------------*/
@@ -355,19 +418,31 @@ void addUnmatchedAttribute(string key) {
       | Establish database connection
       \-----------------------------------------------------------------------------------------------------------------------*/
       var isNew                 = topic.Id == -1;
-      var connection            = new SqlConnection(_connectionString);
       var procedureName         = isNew? "CreateTopic" : "UpdateTopic";
       var command               = new SqlCommand(procedureName, connection) {
         CommandType             = CommandType.StoredProcedure
       };
       var version               = new SqlDateTime(DateTime.Now);
+      var areReferencesResolved = true;
+
+      /*------------------------------------------------------------------------------------------------------------------------
+      | 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.DerivedTopic?.Id < 0 || topic.Relationships.Any(r => r.Any(t => t.Id < 0)))) {
+        unresolvedRelationships.Add(topic);
+        areReferencesResolved = false;
+      }
 
       /*------------------------------------------------------------------------------------------------------------------------
       | Establish query parameters
       \-----------------------------------------------------------------------------------------------------------------------*/
       if (!isNew) {
         command.AddParameter("TopicID", topic.Id);
-        command.AddParameter("DeleteRelationships", true);
+        command.AddParameter("DeleteRelationships", areReferencesResolved);
       }
       else if (topic.Parent != null) {
         command.AddParameter("ParentID", topic.Parent.Id);
@@ -382,7 +457,6 @@ void addUnmatchedAttribute(string key) {
       \-----------------------------------------------------------------------------------------------------------------------*/
       try {
 
-        connection.Open();
         command.ExecuteNonQuery();
 
         topic.Id                = command.GetReturnCode();
@@ -392,7 +466,9 @@ void addUnmatchedAttribute(string key) {
           "The call to the CreateTopic stored procedure did not return the expected 'Id' parameter."
         );
 
-        PersistRelations(topic, connection, true);
+        if (areReferencesResolved) {
+          PersistRelations(topic, connection, true);
+        }
 
         topic.VersionHistory.Insert(0, version.Value);
 
@@ -402,6 +478,7 @@ void addUnmatchedAttribute(string key) {
       | Catch exception
       \-----------------------------------------------------------------------------------------------------------------------*/
       catch (SqlException exception) {
+        connection?.Dispose();
         throw new TopicRepositoryException(
           $"Failed to save Topic '{topic.Key}' ({topic.Id}) via '{_connectionString}': '{exception.Message}'",
           exception
@@ -413,7 +490,6 @@ void addUnmatchedAttribute(string key) {
       \-----------------------------------------------------------------------------------------------------------------------*/
       finally {
         command?.Dispose();
-        connection?.Dispose();
         attributes.Dispose();
       }
 
@@ -423,7 +499,7 @@ void addUnmatchedAttribute(string key) {
       if (isRecursive) {
         foreach (var childTopic in topic.Children) {
           childTopic.Attributes.SetValue("ParentID", topic.Id.ToString(CultureInfo.InvariantCulture));
-          Save(childTopic, isRecursive, isDraft);
+          Save(childTopic, isRecursive, isDraft, connection, unresolvedRelationships);
         }
       }
 
@@ -633,14 +709,13 @@ private static string PersistRelations(Topic topic, SqlConnection connection, bo
       finally {
         command?.Dispose();
         targetIds.Dispose();
-        //Since the SQL connection is being passed in, do not close connection; this allows command pooling.
+        //Since the SQL connection is being passed in, do not close connection; this allows connection pooling.
       }
 
       /*------------------------------------------------------------------------------------------------------------------------
       | Return the relationship attributes to append to the XML attributes (unless skipXml is set to true)
       \-----------------------------------------------------------------------------------------------------------------------*/
-      if (skipXml) return "";
-      else return CreateRelationshipsXml(topic);
+      return skipXml? "" : CreateRelationshipsXml(topic);
 
     }
 
