Skip to content

Commit e703de5

Browse files
JeremyCaneyJeremyCaney
committed
Merge branch 'improvement/TopicRepositoryBase-refactoring' into develop
This is a major refactoring of the `TopicRepository` base class. Most notably, this includes: - Renaming `TopicRepositoryBase` to `TopicRepository` (f0d9d44). - Separating the events logic into a new, bare bones `ObservableTopicRepository` base class (fb663a2), including new `OnTopic{Event}()` methods for raising and/or handling events in derived classes (0e57644), and setup the `TopicRepository` (c8afd13) and `DummyTopicRepository` (3ce23f6) to derive from it. - Establishing a new `TopicRepositoryDecorator` base class for `ITopicRepository` decorators (799e2b6), with implementation for the `CachedTopicRepository` decorator (7c7afbf). - Moved the logic for establishing a shared version, recursing over children, and attempting to pick up `unresolvedTopics` (i.e., `Topic.Relationships` and `Topic.References` that weren't yet saved when a `Topic` referencing them was first saved) from `SqlTopicRepository` to `TopicRepository` (fdd0025, ) - Moved core implementation logic from `TopicRepository.{Operation}` to a new `abstract` `TopicRepository.{Operation}Topic()` method (e.g., `SaveTopic()`, `DeleteTopic()`, `MoveTopic()`) to help protect the core business logic and provide more control over the order of operations (e.g., validating logic prior to the implementation, raising the event after the implementation) (116c8eb). - Throw new `ReferentialIntegryException` on `TopicRepository.Save()` if there are any `unresolvedTopics` after an initial attempt to resolve orphaned references and relationships (6f55b2a). This also includes some minor bugfixes and cleanup, such as: - Removing a legacy hack for picking up unresolved `DerivedTopic` references (d631b3f). - Marking `Topic.Relationships` and `Topic.References` as `!IsFullyLoaded` if there's an exception when loading a specific version (06bf859).
2 parents 9ef227c + 934594b commit e703de5

File tree

12 files changed

+675
-303
lines changed

12 files changed

+675
-303
lines changed

‎OnTopic.Data.Caching/CachedTopicRepository.cs‎

Lines changed: 10 additions & 73 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@
55
\=============================================================================================================================*/
66
using System;
77
using OnTopic.Internal.Diagnostics;
8-
using OnTopic.Metadata;
98
using OnTopic.Querying;
109
using OnTopic.Repositories;
1110

@@ -22,39 +21,30 @@ namespace OnTopic.Data.Caching {
2221
/// for an actual data access class.
2322
/// </remarks>
2423

25-
public class CachedTopicRepository : TopicRepositoryBase, ITopicRepository {
24+
public class CachedTopicRepository : TopicRepositoryDecorator {
2625

2726
/*==========================================================================================================================
2827
| VARIABLES
2928
\-------------------------------------------------------------------------------------------------------------------------*/
30-
private readonly ITopicRepository _dataProvider;
3129
private readonly Topic _cache;
3230

3331
/*==========================================================================================================================
3432
| CONSTRUCTOR
3533
\-------------------------------------------------------------------------------------------------------------------------*/
3634
/// <summary>
37-
/// Instantiates a new instance of the CachedTopicRepository with a dependency on an underlying ITopicRepository in order
38-
/// to provide necessary data access.
35+
/// Instantiates a new instance of the <see cref="CachedTopicRepository"/> with a dependency on an underlying <see cref="
36+
/// ITopicRepository"/> in order to provide necessary data access.
3937
/// </summary>
40-
/// <param name="dataProvider">A concrete instance of an ITopicRepository, which will be used for data access.</param>
41-
/// <returns>A new instance of the CachedTopicRepository.</returns>
42-
public CachedTopicRepository(ITopicRepository dataProvider) : base() {
43-
44-
/*------------------------------------------------------------------------------------------------------------------------
45-
| Validate input
46-
\-----------------------------------------------------------------------------------------------------------------------*/
47-
Contract.Requires(dataProvider, "A concrete implementation of an ITopicRepository is required.");
48-
49-
/*------------------------------------------------------------------------------------------------------------------------
50-
| Set values locally
51-
\-----------------------------------------------------------------------------------------------------------------------*/
52-
_dataProvider = dataProvider;
38+
/// <param name="topicRepository">
39+
/// A concrete instance of an <see cref="ITopicRepository"/>, which will be used for data access.
40+
/// </param>
41+
/// <returns>A new instance of a <see cref="CachedTopicRepository"/>.</returns>
42+
public CachedTopicRepository(ITopicRepository topicRepository) : base(topicRepository) {
5343

5444
/*------------------------------------------------------------------------------------------------------------------------
5545
| Ensure topics are loaded
5646
\-----------------------------------------------------------------------------------------------------------------------*/
57-
var rootTopic = _dataProvider.Load();
47+
var rootTopic = TopicRepository.Load();
5848

5949
Contract.Assume(
6050
rootTopic,
@@ -69,34 +59,6 @@ public CachedTopicRepository(ITopicRepository dataProvider) : base() {
6959

7060
}
7161

72-
/*==========================================================================================================================
73-
| EVENT PASSTHROUGHS
74-
\-------------------------------------------------------------------------------------------------------------------------*/
75-
76-
/// <inheritdoc/>
77-
public override event EventHandler<DeleteEventArgs>? DeleteEvent {
78-
add => _dataProvider.DeleteEvent += value;
79-
remove => _dataProvider.DeleteEvent -= value;
80-
}
81-
82-
/// <inheritdoc/>
83-
public override event EventHandler<MoveEventArgs>? MoveEvent {
84-
add => _dataProvider.MoveEvent += value;
85-
remove => _dataProvider.MoveEvent -= value;
86-
}
87-
88-
/// <inheritdoc/>
89-
public override event EventHandler<RenameEventArgs>? RenameEvent {
90-
add => _dataProvider.RenameEvent += value;
91-
remove => _dataProvider.RenameEvent -= value;
92-
}
93-
94-
/*==========================================================================================================================
95-
| GET CONTENT TYPE DESCRIPTORS
96-
\-------------------------------------------------------------------------------------------------------------------------*/
97-
/// <inheritdoc />
98-
public override ContentTypeDescriptorCollection GetContentTypeDescriptors() => _dataProvider.GetContentTypeDescriptors();
99-
10062
/*==========================================================================================================================
10163
| METHOD: LOAD
10264
\-------------------------------------------------------------------------------------------------------------------------*/
@@ -149,34 +111,9 @@ public override event EventHandler<RenameEventArgs>? RenameEvent {
149111
/*------------------------------------------------------------------------------------------------------------------------
150112
| Return appropriate topic
151113
\-----------------------------------------------------------------------------------------------------------------------*/
152-
return _dataProvider.Load(topicId, version, referenceTopic?? _cache);
114+
return TopicRepository.Load(topicId, version, referenceTopic?? _cache);
153115

154116
}
155117

156-
/*==========================================================================================================================
157-
| METHOD: REFRESH
158-
\-------------------------------------------------------------------------------------------------------------------------*/
159-
/// <inheritdoc/>
160-
public override void Refresh(Topic referenceTopic, DateTime since) => _dataProvider.Refresh(referenceTopic, since);
161-
162-
/*==========================================================================================================================
163-
| METHOD: SAVE
164-
\-------------------------------------------------------------------------------------------------------------------------*/
165-
/// <inheritdoc />
166-
public override void Save(Topic topic, bool isRecursive = false) =>
167-
_dataProvider.Save(topic, isRecursive);
168-
169-
/*==========================================================================================================================
170-
| METHOD: MOVE
171-
\-------------------------------------------------------------------------------------------------------------------------*/
172-
/// <inheritdoc />
173-
public override void Move(Topic topic, Topic target, Topic? sibling) => _dataProvider.Move(topic, target, sibling);
174-
175-
/*==========================================================================================================================
176-
| METHOD: DELETE
177-
\-------------------------------------------------------------------------------------------------------------------------*/
178-
/// <inheritdoc />
179-
public override void Delete(Topic topic, bool isRecursive = true) => _dataProvider.Delete(topic, isRecursive);
180-
181118
} //Class
182119
} //Namespace

‎OnTopic.Data.Sql/SqlTopicRepository.cs‎

Lines changed: 27 additions & 113 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
using System.Linq;
1313
using System.Text;
1414
using Microsoft.Data.SqlClient;
15+
using OnTopic.Collections;
1516
using OnTopic.Data.Sql.Models;
1617
using OnTopic.Internal.Diagnostics;
1718
using OnTopic.Querying;
@@ -28,7 +29,7 @@ namespace OnTopic.Data.Sql {
2829
/// <remarks>
2930
/// Concrete implementation of the <see cref="OnTopic.Repositories.IDataRepository"/> class.
3031
/// </remarks>
31-
public class SqlTopicRepository : TopicRepositoryBase, ITopicRepository {
32+
public class SqlTopicRepository : TopicRepository, ITopicRepository {
3233

3334
/*==========================================================================================================================
3435
| PRIVATE VARIABLES
@@ -253,6 +254,10 @@ public override Topic Load(int topicId, DateTime version, Topic? referenceTopic
253254
| Catch exception
254255
\-----------------------------------------------------------------------------------------------------------------------*/
255256
catch (SqlException exception) {
257+
if (topic is not null) {
258+
topic.Relationships.IsFullyLoaded = false;
259+
topic.References.IsFullyLoaded = false;
260+
}
256261
throw new TopicRepositoryException($"Topics failed to load: '{exception.Message}'", exception);
257262
}
258263

@@ -335,79 +340,16 @@ public override void Refresh(Topic referenceTopic, DateTime since) {
335340
/*==========================================================================================================================
336341
| METHOD: SAVE
337342
\-------------------------------------------------------------------------------------------------------------------------*/
338-
/// <inheritdoc />
339-
public override void Save([NotNull]Topic topic, bool isRecursive = false) {
340-
341-
/*------------------------------------------------------------------------------------------------------------------------
342-
| Establish dependencies
343-
\-----------------------------------------------------------------------------------------------------------------------*/
344-
var version = DateTime.UtcNow;
345-
var unresolvedTopics = new List<Topic>();
346-
347-
using var connection = new SqlConnection(_connectionString);
348-
349-
connection.Open();
350-
351-
/*------------------------------------------------------------------------------------------------------------------------
352-
| Handle first pass
353-
\-----------------------------------------------------------------------------------------------------------------------*/
354-
Save(topic, isRecursive, connection, unresolvedTopics, version);
355-
356-
/*------------------------------------------------------------------------------------------------------------------------
357-
| Attempt to resolve outstanding relationships
358-
\-----------------------------------------------------------------------------------------------------------------------*/
359-
foreach (var unresolvedTopic in unresolvedTopics) {
360-
Save(unresolvedTopic, false, connection, unresolvedTopics, version);
361-
}
362-
363-
/*------------------------------------------------------------------------------------------------------------------------
364-
| Close shared connection
365-
\-----------------------------------------------------------------------------------------------------------------------*/
366-
connection.Close();
367-
368-
}
369-
370-
/// <summary>
371-
/// The private overload of the <see cref="Save"/> method provides support for sharing the <see cref="SqlConnection"/>
372-
/// between multiple requests, and maintaining a list of <paramref name="unresolvedRelationships"/>.
373-
/// </summary>
374-
/// <remarks>
375-
/// <para>
376-
/// When recursively saving a topic graph, it is conceivable that references to other topics—such as <see
377-
/// cref="Topic.Relationships"/> or <see cref="Topic.DerivedTopic"/>—can't yet be persisted because the target <see
378-
/// cref="Topic"/> hasn't yet been saved, and thus the <see cref="Topic.Id"/> is still set to <c>-1</c>. To mitigate
379-
/// this, the <paramref name="unresolvedRelationships"/> allows this private overload to keep track of unresolved
380-
/// relationships. The public <see cref="Save(Topic, Boolean, Boolean)"/> overload uses this list to resave any topics
381-
/// that include such references. This adds some overhead due to the duplicate <see cref="Save"/>, but helps avoid
382-
/// potential data loss when working with complex topic graphs.
383-
/// </para>
384-
/// <para>
385-
/// The connection sharing probably doesn't provide that much of a gain in that .NET does a good job of connection
386-
/// pooling. Nevertheless, there is some overhead to opening a new connection, so sharing an open connection when we
387-
/// doing a recursive save could potentially provide some performance benefit.
388-
/// </para>
389-
/// </remarks>
390-
/// <param name="topic">The source <see cref="Topic"/> to save.</param>
391-
/// <param name="isRecursive">Determines whether or not to recursively save <see cref="Topic.Children"/>.</param>
392-
/// <param name="connection">The open <see cref="SqlConnection"/> to use for executing <see cref="SqlCommand"/>s.</param>
393-
/// <param name="unresolvedRelationships">A list of <see cref="Topic"/>s with unresolved topic references.</param>
394-
private void Save(
343+
/// <inheritdoc/>
344+
protected override sealed void SaveTopic(
395345
[NotNull]Topic topic,
396-
bool isRecursive,
397-
SqlConnection connection,
398-
List<Topic> unresolvedRelationships,
399-
DateTime version
346+
DateTime version,
347+
bool persistRelationships
400348
) {
401349

402-
/*------------------------------------------------------------------------------------------------------------------------
403-
| Call base method - will trigger any events associated with the save
404-
\-----------------------------------------------------------------------------------------------------------------------*/
405-
base.Save(topic, isRecursive);
406-
407350
/*------------------------------------------------------------------------------------------------------------------------
408351
| Define variables
409352
\-----------------------------------------------------------------------------------------------------------------------*/
410-
var areReferencesResolved = true;
411353
var isTopicDirty = topic.IsDirty();
412354
var areRelationshipsDirty = topic.Relationships.IsDirty();
413355
var areReferencesDirty = topic.References.IsDirty();
@@ -440,7 +382,6 @@ DateTime version
440382
| Bypass is not dirty
441383
\-----------------------------------------------------------------------------------------------------------------------*/
442384
if (!isDirty) {
443-
recurse();
444385
return;
445386
}
446387

@@ -494,29 +435,15 @@ DateTime version
494435
/*------------------------------------------------------------------------------------------------------------------------
495436
| Establish database connection
496437
\-----------------------------------------------------------------------------------------------------------------------*/
438+
using var connection = new SqlConnection(_connectionString);
497439
var procedureName = topic.IsNew? "CreateTopic" : "UpdateTopic";
498440

441+
connection.Open();
442+
499443
using var command = new SqlCommand(procedureName, connection) {
500444
CommandType = CommandType.StoredProcedure
501445
};
502446

503-
/*------------------------------------------------------------------------------------------------------------------------
504-
| Handle unresolved references
505-
>-------------------------------------------------------------------------------------------------------------------------
506-
| If it's a recursive save and there are any unresolved relationships, come back to this after the topic graph has been
507-
| saved; that ensures that any relationships within the topic graph have been saved and can be properly persisted. The
508-
| same can be done for DerivedTopics references, which are effectively establish a 1:1 relationship.
509-
\-----------------------------------------------------------------------------------------------------------------------*/
510-
if (
511-
isRecursive &&
512-
( topic.Relationships.Any(r => r.Values.Any(t => t.Id < 0)) ||
513-
topic.References.Values.Any(t => t.Id < 0)
514-
)
515-
) {
516-
unresolvedRelationships.Add(topic);
517-
areReferencesResolved = false;
518-
}
519-
520447
/*------------------------------------------------------------------------------------------------------------------------
521448
| Establish query parameters
522449
\-----------------------------------------------------------------------------------------------------------------------*/
@@ -553,18 +480,14 @@ DateTime version
553480
"The call to the CreateTopic stored procedure did not return the expected 'Id' parameter."
554481
);
555482

556-
if (areReferencesResolved && areRelationshipsDirty) {
483+
if (persistRelationships && areRelationshipsDirty) {
557484
PersistRelations(topic, version, connection);
558485
}
559486

560-
if (areReferencesResolved && areReferencesDirty) {
487+
if (persistRelationships && areReferencesDirty) {
561488
PersistReferences(topic, version, connection);
562489
}
563490

564-
if (!topic.VersionHistory.Contains(version)) {
565-
topic.VersionHistory.Insert(0, version);
566-
}
567-
568491
topic.Attributes.MarkClean(version);
569492

570493
}
@@ -580,34 +503,25 @@ DateTime version
580503
}
581504

582505
/*------------------------------------------------------------------------------------------------------------------------
583-
| Recuse over any children
506+
| Close connection
584507
\-----------------------------------------------------------------------------------------------------------------------*/
585-
recurse();
586-
587-
/*------------------------------------------------------------------------------------------------------------------------
588-
| Function: Recurse
589-
\-----------------------------------------------------------------------------------------------------------------------*/
590-
void recurse() {
591-
if (isRecursive) {
592-
foreach (var childTopic in topic.Children) {
593-
childTopic.Attributes.SetValue("ParentID", topic.Id.ToString(CultureInfo.InvariantCulture));
594-
Save(childTopic, isRecursive, connection, unresolvedRelationships, version);
595-
}
596-
}
508+
finally {
509+
connection.Close();
597510
}
598511

599512
}
600513

601514
/*==========================================================================================================================
602-
| METHOD: MOVE
515+
| METHOD: MOVE TOPIC
603516
\-------------------------------------------------------------------------------------------------------------------------*/
604517
/// <inheritdoc />
605-
public override void Move(Topic topic, Topic target, Topic? sibling) {
518+
protected override void MoveTopic(Topic topic, Topic target, Topic? sibling) {
606519

607520
/*------------------------------------------------------------------------------------------------------------------------
608-
| Delete from memory
521+
| Validate parameters
609522
\-----------------------------------------------------------------------------------------------------------------------*/
610-
base.Move(topic, target, sibling);
523+
Contract.Requires(topic, nameof(topic));
524+
Contract.Requires(target, nameof(target));
611525

612526
/*------------------------------------------------------------------------------------------------------------------------
613527
| Establish database connection
@@ -649,15 +563,15 @@ public override void Move(Topic topic, Topic target, Topic? sibling) {
649563
}
650564

651565
/*==========================================================================================================================
652-
| METHOD: DELETE
566+
| METHOD: DELETE TOPIC
653567
\-------------------------------------------------------------------------------------------------------------------------*/
654568
/// <inheritdoc />
655-
public override void Delete(Topic topic, bool isRecursive = false) {
569+
protected override void DeleteTopic(Topic topic) {
656570

657571
/*------------------------------------------------------------------------------------------------------------------------
658-
| Delete from memory
572+
| Validate parameters
659573
\-----------------------------------------------------------------------------------------------------------------------*/
660-
base.Delete(topic, isRecursive);
574+
Contract.Requires(topic, nameof(topic));
661575

662576
/*------------------------------------------------------------------------------------------------------------------------
663577
| Delete from database

0 commit comments

Comments
 (0)