Skip to content

Commit f7d36f0

Browse files
JeremyCaneyJeremyCaney
committed
Merge branch 'feature/IsDirty-improvements' into develop
Made multiple improvements to how `IsDirty()` and `MarkDirty()` are handled to make them more intelligent. Notably, this includes checks to see if the source `Topic` or, where appropriate, target `Topic` `IsNew` to prevent those items from being marked as `!IsDirty`, even if they are explicitly requested to be marked as clean by e.g. a call to `MarkClean()`, `TrackedItem<T>.IsDirty`, or `SetTopic()`. While these scenarios should not occur, this logic helps prevent inadvertently orphaning values that we _know_ have not been saved, either because the source topic itself has not been saved, or because they point to topics that have not been saved. This effects `TrackedCollection<T>` (85c386f, b0787c6, cbd49c1, 6c6a1b5), `TopicRelationshipMultiMap` (85c386f, 34a80cb, 1222bd0, 2cd0c5f, 249d049) and `Topic` (af53497, dc08f2e). By making it easier to assess whether a `IEnumerable<Topic>` collection has any `IsNew` topics via the new `IEnumerable<Topic>.AnyNew()` extension method (d551442), this helps support the above for `TopicReferenceCollection` and `TopicRelationshipMultiMap`, thus allowing the `MarkClean()` logic to be moved from `ITopicRepository` implementations—such as `SqlTopicRepository`—and into the base `TopicRepository` class, thus centralizing that logic (2dd8e03). In addition, this updates `Topic` to use `DirtyKeyCollection` (61fe810) and `ITrackDirtyKeys` (e6d15ab) for more granular tracking of _which_ core attributes have been modified (e.g., `Key`, `ContentType`, or `Parent`), which in turn permits `TopicRepository.Save()` to determine if `TopicRepository.Move()` should be called (f5ba7a8), for cases where a saved topic also has a new parent (ebd6d3c). This fixes a bug that was introduced when `ParentID` was removed from `Topic.Attributes`, since there was no longer a way to determine if a `Topic` had been moved within the topic graph without that update being persisted to the database.
2 parents b01216e + 6c6a1b5 commit f7d36f0

File tree

10 files changed

+356
-69
lines changed

10 files changed

+356
-69
lines changed

‎OnTopic.Data.Sql/SqlTopicRepository.cs‎

Lines changed: 13 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -4,15 +4,12 @@
44
| Project Topics Library
55
\=============================================================================================================================*/
66
using System;
7-
using System.Collections.Generic;
87
using System.Data;
9-
using System.Data.SqlTypes;
108
using System.Diagnostics.CodeAnalysis;
119
using System.Globalization;
1210
using System.Linq;
1311
using System.Text;
1412
using Microsoft.Data.SqlClient;
15-
using OnTopic.Collections;
1613
using OnTopic.Data.Sql.Models;
1714
using OnTopic.Internal.Diagnostics;
1815
using OnTopic.Querying;
@@ -491,15 +488,13 @@ bool persistRelationships
491488
);
492489

493490
if (persistRelationships && areRelationshipsDirty) {
494-
PersistRelations(topic, version, connection);
491+
PersistRelationships(topic, version, connection);
495492
}
496493

497494
if (persistRelationships && areReferencesDirty) {
498495
PersistReferences(topic, version, connection);
499496
}
500497

501-
topic.Attributes.MarkClean(version);
502-
503498
}
504499

505500
/*------------------------------------------------------------------------------------------------------------------------
@@ -617,14 +612,14 @@ protected override sealed void DeleteTopic(Topic topic) {
617612
}
618613

619614
/*==========================================================================================================================
620-
| METHOD: PERSIST RELATIONS
615+
| METHOD: PERSIST RELATIONSHIPS
621616
\-------------------------------------------------------------------------------------------------------------------------*/
622617
/// <summary>
623618
/// Internal method that saves topic relationships to the n:n mapping table in SQL.
624619
/// </summary>
625620
/// <param name="topic">The topic object whose relationships should be persisted.</param>
626621
/// <param name="connection">The SQL connection.</param>
627-
private static void PersistRelations(Topic topic, DateTime version, SqlConnection connection) {
622+
private static void PersistRelationships(Topic topic, DateTime version, SqlConnection connection) {
628623

629624
/*------------------------------------------------------------------------------------------------------------------------
630625
| Return blank if the topic has no relations.
@@ -641,33 +636,26 @@ private static void PersistRelations(Topic topic, DateTime version, SqlConnectio
641636
\---------------------------------------------------------------------------------------------------------------------*/
642637
foreach (var key in topic.Relationships.Keys) {
643638

644-
var relatedTopics = topic.Relationships.GetTopics(key);
645-
var topicId = topic.Id.ToString(CultureInfo.InvariantCulture);
646-
var savedTopics = relatedTopics.Where(t => !t.IsNew).Select<Topic, int>(m => m.Id);
647-
648639
using var targetIds = new TopicListDataTable();
649640
using var command = new SqlCommand("UpdateRelationships", connection) {
650641
CommandType = CommandType.StoredProcedure
651642
};
652643

653-
foreach (var targetTopicId in savedTopics) {
654-
targetIds.AddRow(targetTopicId);
644+
foreach (var targetTopic in topic.Relationships.GetTopics(key)) {
645+
if (!targetTopic.IsNew) {
646+
targetIds.AddRow(targetTopic.Id);
647+
}
655648
}
656649

657650
// Add Parameters
658-
command.AddParameter("TopicID", topicId);
651+
command.AddParameter("TopicID", topic.Id.ToString(CultureInfo.InvariantCulture));
659652
command.AddParameter("RelationshipKey", key);
660653
command.AddParameter("RelatedTopics", targetIds);
661654
command.AddParameter("Version", version);
662655
command.AddParameter("DeleteUnmatched", topic.Relationships.IsFullyLoaded);
663656

664657
command.ExecuteNonQuery();
665658

666-
//Reset isDirty, assuming there aren't any unresolved references
667-
if (savedTopics.Count() == relatedTopics.Count) {
668-
topic.Relationships.MarkClean(key);
669-
}
670-
671659
}
672660

673661
}
@@ -704,29 +692,25 @@ private static void PersistReferences(Topic topic, DateTime version, SqlConnecti
704692
\-----------------------------------------------------------------------------------------------------------------------*/
705693
try {
706694

707-
var topicId = topic.Id.ToString(CultureInfo.InvariantCulture);
708695
using var references = new TopicReferencesDataTable();
709696
using var command = new SqlCommand("UpdateReferences", connection) {
710697
CommandType = CommandType.StoredProcedure
711698
};
712699

713-
foreach (var relatedTopic in topic.References.Where(t => !t.Value?.IsNew?? false)) {
714-
references.AddRow(relatedTopic.Key, relatedTopic.Value!.Id);
700+
foreach (var relatedTopic in topic.References) {
701+
if (!relatedTopic.Value?.IsNew?? false) {
702+
references.AddRow(relatedTopic.Key, relatedTopic.Value!.Id);
703+
}
715704
}
716705

717706
// Add Parameters
718-
command.AddParameter("TopicID", topicId);
707+
command.AddParameter("TopicID", topic.Id.ToString(CultureInfo.InvariantCulture));
719708
command.AddParameter("ReferencedTopics", references);
720709
command.AddParameter("Version", version);
721710
command.AddParameter("DeleteUnmatched", topic.References.IsFullyLoaded);
722711

723712
command.ExecuteNonQuery();
724713

725-
//Reset isDirty, assuming there aren't any unresolved references
726-
if (references.Rows.Count == topic.References.Count) {
727-
topic.References.MarkClean();
728-
}
729-
730714
}
731715

732716
/*------------------------------------------------------------------------------------------------------------------------

‎OnTopic.Tests/AttributeValueCollectionTest.cs‎

Lines changed: 52 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -302,7 +302,7 @@ public void SetValue_ValueChanged_IsDirty() {
302302
[TestMethod]
303303
public void Clear_ExistingValues_IsDirty() {
304304

305-
var topic = TopicFactory.Create("Test", "Container");
305+
var topic = TopicFactory.Create("Test", "Container", 1);
306306

307307
topic.Attributes.SetValue("Foo", "Bar", false);
308308

@@ -323,7 +323,7 @@ public void Clear_ExistingValues_IsDirty() {
323323
[TestMethod]
324324
public void SetValue_ValueUnchanged_IsNotDirty() {
325325

326-
var topic = TopicFactory.Create("Test", "Container");
326+
var topic = TopicFactory.Create("Test", "Container", 1);
327327

328328
topic.Attributes.SetValue("Fah", "Bar", false);
329329
topic.Attributes.SetValue("Fah", "Bar");
@@ -361,7 +361,7 @@ public void IsDirty_DirtyValues_ReturnsTrue() {
361361
[TestMethod]
362362
public void IsDirty_DeletedValues_ReturnsTrue() {
363363

364-
var topic = TopicFactory.Create("Test", "Container");
364+
var topic = TopicFactory.Create("Test", "Container", 1);
365365

366366
topic.Attributes.SetValue("Foo", "Bar");
367367
topic.Attributes.Remove("Foo");
@@ -421,7 +421,7 @@ public void IsDirty_ExcludeLastModified_ReturnsFalse() {
421421
[TestMethod]
422422
public void IsDirty_MarkClean_UpdatesLastModified() {
423423

424-
var topic = TopicFactory.Create("Test", "Container");
424+
var topic = TopicFactory.Create("Test", "Container", 1);
425425
var version = DateTime.Now.AddDays(5);
426426

427427
topic.Attributes.SetValue("Baz", "Foo");
@@ -443,7 +443,7 @@ public void IsDirty_MarkClean_UpdatesLastModified() {
443443
[TestMethod]
444444
public void IsDirty_MarkClean_ReturnsFalse() {
445445

446-
var topic = TopicFactory.Create("Test", "Container");
446+
var topic = TopicFactory.Create("Test", "Container", 1);
447447

448448
topic.Attributes.SetValue("Foo", "Bar");
449449
topic.Attributes.SetValue("Baz", "Foo");
@@ -467,7 +467,7 @@ public void IsDirty_MarkClean_ReturnsFalse() {
467467
[TestMethod]
468468
public void IsDirty_MarkAttributeClean_ReturnsFalse() {
469469

470-
var topic = TopicFactory.Create("Test", "Container");
470+
var topic = TopicFactory.Create("Test", "Container", 1);
471471

472472
topic.Attributes.SetValue("Foo", "Bar");
473473
topic.Attributes.MarkClean("Foo");
@@ -476,6 +476,52 @@ public void IsDirty_MarkAttributeClean_ReturnsFalse() {
476476

477477
}
478478

479+
/*==========================================================================================================================
480+
| TEST: IS DIRTY: ADD CLEAN ATTRIBUTE TO NEW TOPIC: RETURNS TRUE
481+
\-------------------------------------------------------------------------------------------------------------------------*/
482+
/// <summary>
483+
/// Populates a <see cref="AttributeValueCollection"/> associated with an <see cref="Topic.IsNew"/> <see cref="Topic"/>
484+
/// with a <see cref="AttributeValue"/> that is not marked as <see cref="TrackedItem{T}.IsDirty"/> and then confirms that
485+
/// <see cref="TrackedCollection{TItem, TValue, TAttribute}.IsDirty()"/> returns <c>true</c>.
486+
/// </summary>
487+
[TestMethod]
488+
public void IsDirty_AddCleanAttributeToNewTopic_ReturnsTrue() {
489+
490+
var topic = TopicFactory.Create("Test", "Container");
491+
492+
topic.Attributes.Add(
493+
new() {
494+
Key = "Foo",
495+
Value = "Bar",
496+
IsDirty = false
497+
}
498+
);
499+
500+
Assert.IsTrue(topic.Attributes.IsDirty());
501+
502+
}
503+
504+
/*==========================================================================================================================
505+
| TEST: IS DIRTY: MARK NEW TOPIC AS CLEAN: RETURNS TRUE
506+
\-------------------------------------------------------------------------------------------------------------------------*/
507+
/// <summary>
508+
/// Populates a <see cref="AttributeValueCollection"/> associated with an <see cref="Topic.IsNew"/> <see cref="Topic"/>
509+
/// with a <see cref="AttributeValue"/> and then confirms that <see cref="TrackedCollection{TItem, TValue, TAttribute}.
510+
/// IsDirty(String)"/> returns <c>true</c> for that attribute after calling <see cref="TrackedCollection{TItem, TValue,
511+
/// TAttribute}.MarkClean(String, DateTime?)"/>.
512+
/// </summary>
513+
[TestMethod]
514+
public void IsDirty_MarkNewTopicAsClean_ReturnsTrue() {
515+
516+
var topic = TopicFactory.Create("Test", "Container");
517+
518+
topic.Attributes.SetValue("Foo", "Bar");
519+
topic.Attributes.MarkClean();
520+
521+
Assert.IsTrue(topic.Attributes.IsDirty());
522+
523+
}
524+
479525
/*==========================================================================================================================
480526
| TEST: SET VALUE: INVALID VALUE: THROWS EXCEPTION
481527
\-------------------------------------------------------------------------------------------------------------------------*/

‎OnTopic.Tests/TopicReferenceCollectionTest.cs‎

Lines changed: 24 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
using OnTopic.Associations;
99
using OnTopic.Tests.Entities;
1010
using OnTopic.Collections.Specialized;
11+
using System.Collections.ObjectModel;
1112

1213
namespace OnTopic.Tests {
1314

@@ -55,8 +56,8 @@ public void Add_NewReference_IsDirty() {
5556
[TestMethod]
5657
public void SetValue_NewReference_NotDirty() {
5758

58-
var topic = TopicFactory.Create("Topic", "Page");
59-
var reference = TopicFactory.Create("Reference", "Page");
59+
var topic = TopicFactory.Create("Topic", "Page", 1);
60+
var reference = TopicFactory.Create("Reference", "Page", 2);
6061

6162
topic.References.SetValue("Reference", reference, false);
6263

@@ -110,6 +111,27 @@ public void Clear_ExistingReferences_IsDirty() {
110111

111112
}
112113

114+
/*==========================================================================================================================
115+
| TEST: ADD: NEW TOPIC: IS DIRTY
116+
\-------------------------------------------------------------------------------------------------------------------------*/
117+
/// <summary>
118+
/// Assembles a new <see cref="TopicReferenceCollection"/> and adds a new <see cref="Topic"/> reference using <see cref="
119+
/// KeyedCollection{TKey, TItem}.InsertItem(Int32, TItem)"/> with <see cref="TrackedItem{T}.IsDirty"/> set to <c>false
120+
/// </c>, confirming that <see cref="TrackedCollection{TItem, TValue, TAttribute}.IsDirty()"/> remains <c>true</c> since
121+
/// the target <see cref="Topic"/> is unsaved.
122+
/// </summary>
123+
[TestMethod]
124+
public void Add_NewTopic_IsDirty() {
125+
126+
var topic = TopicFactory.Create("Topic", "Page", 1);
127+
var reference = TopicFactory.Create("Reference", "Page");
128+
129+
topic.References.SetValue("Reference", reference, false);
130+
131+
Assert.IsTrue(topic.References.IsDirty());
132+
133+
}
134+
113135
/*==========================================================================================================================
114136
| TEST: ADD: NEW REFERENCE: INCOMING RELATIONSHIP SET
115137
\-------------------------------------------------------------------------------------------------------------------------*/

‎OnTopic.Tests/TopicRelationshipMultiMapTest.cs‎

Lines changed: 46 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -192,9 +192,9 @@ public void SetTopic_IsDirty() {
192192
[TestMethod]
193193
public void SetTopic_IsDuplicate_IsNotDirty() {
194194

195-
var topic = TopicFactory.Create("Test", "Page");
195+
var topic = TopicFactory.Create("Test", "Page", 1);
196196
var relationships = new TopicRelationshipMultiMap(topic);
197-
var related = TopicFactory.Create("Topic", "Page");
197+
var related = TopicFactory.Create("Topic", "Page", 2);
198198

199199
relationships.SetTopic("Related", related);
200200
relationships.MarkClean();
@@ -336,5 +336,49 @@ public void ClearTopics_NoTopics_IsNotDirty() {
336336

337337
}
338338

339+
/*==========================================================================================================================
340+
| TEST: SET TOPIC: NEW PARENT: IS DIRTY
341+
\-------------------------------------------------------------------------------------------------------------------------*/
342+
/// <summary>
343+
/// Adds an existing <see cref="Topic"/> to a <see cref="TopicRelationshipMultiMap"/> associated with a <see cref="Topic.
344+
/// IsNew"/> <see cref="Topic"/> and confirms that <see cref="TopicRelationshipMultiMap.IsDirty()"/> returns <c>true</c>
345+
/// even if <see cref="TopicRelationshipMultiMap.SetTopic(String, Topic, Boolean?, Boolean)"/> is called with the <c>
346+
/// markDirty</c> parameter set to <c>false</c>.
347+
/// </summary>
348+
[TestMethod]
349+
public void SetTopic_NewParent_IsDirty() {
350+
351+
var topic = TopicFactory.Create("Test", "Page");
352+
var relationships = new TopicRelationshipMultiMap(topic);
353+
var related = TopicFactory.Create("Topic", "Page", 1);
354+
355+
relationships.SetTopic("Related", related, false);
356+
357+
Assert.IsTrue(relationships.IsDirty());
358+
359+
}
360+
361+
/*==========================================================================================================================
362+
| TEST: SET TOPIC: NEW TOPIC: IS DIRTY
363+
\-------------------------------------------------------------------------------------------------------------------------*/
364+
/// <summary>
365+
/// Adds a new <see cref="Topic"/> to a <see cref="TopicRelationshipMultiMap"/> associated with an existing <see cref="
366+
/// Topic"/> and confirms that <see cref="TopicRelationshipMultiMap.IsDirty()"/> returns <c>true</c> even if <see cref="
367+
/// TopicRelationshipMultiMap.SetTopic(String, Topic, Boolean?, Boolean)"/> is called with the <c>markDirty</c> parameter
368+
/// set to <c>false</c>.
369+
/// </summary>
370+
[TestMethod]
371+
public void SetTopic_NewTopic_IsDirty() {
372+
373+
var topic = TopicFactory.Create("Test", "Page", 1);
374+
var relationships = new TopicRelationshipMultiMap(topic);
375+
var related = TopicFactory.Create("Topic", "Page");
376+
377+
relationships.SetTopic("Related", related, false);
378+
379+
Assert.IsTrue(relationships.IsDirty());
380+
381+
}
382+
339383
} //Class
340384
} //Namespace

‎OnTopic.Tests/TopicTest.cs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -397,8 +397,8 @@ public void IsDirty_ChangeCollections_ReturnsTrue() {
397397
[TestMethod]
398398
public void MarkClean_ChangeCollection_ResetIsDirty() {
399399

400-
var topic = TopicFactory.Create("Topic", "Page");
401-
var related = TopicFactory.Create("Related", "Page");
400+
var topic = TopicFactory.Create("Topic", "Page", 1);
401+
var related = TopicFactory.Create("Related", "Page", 2);
402402

403403
topic.Attributes.SetValue("Related", related.Key);
404404
topic.References.SetValue("Related", related);

‎OnTopic/Associations/TopicRelationshipMultiMap.cs‎

Lines changed: 20 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
using System;
77
using OnTopic.Collections.Specialized;
88
using OnTopic.Internal.Diagnostics;
9+
using OnTopic.Querying;
910
using OnTopic.Repositories;
1011

1112
namespace OnTopic.Associations {
@@ -187,7 +188,7 @@ internal void SetTopic(string relationshipKey, Topic topic, bool? markDirty, boo
187188
var wasDirty = _dirtyKeys.IsDirty(relationshipKey);
188189
if (!topics.Contains(topic)) {
189190
_storage.Add(relationshipKey, topic);
190-
if (markDirty.HasValue && !markDirty.Value && !wasDirty) {
191+
if (!_parent.IsNew && !topic.IsNew && markDirty.HasValue && !markDirty.Value && !wasDirty) {
191192
MarkClean(relationshipKey);
192193
}
193194
else {
@@ -245,10 +246,26 @@ internal void SetTopic(string relationshipKey, Topic topic, bool? markDirty, boo
245246
| METHOD: MARK CLEAN
246247
\-------------------------------------------------------------------------------------------------------------------------*/
247248
/// <inheritdoc/>
248-
public void MarkClean() => _dirtyKeys.MarkClean();
249+
public void MarkClean() {
250+
if (_parent.IsNew) {
251+
return;
252+
}
253+
foreach (var relationship in _storage) {
254+
if (!relationship.Values.AnyNew()) {
255+
_dirtyKeys.MarkClean(relationship.Key);
256+
}
257+
}
258+
}
249259

250260
/// <inheritdoc/>
251-
public void MarkClean(string key) => _dirtyKeys.MarkClean(key);
261+
public void MarkClean(string key) {
262+
if (_parent.IsNew) {
263+
return;
264+
}
265+
if (Contains(key) && !_storage[key].Values.AnyNew()) {
266+
_dirtyKeys.MarkClean(key);
267+
}
268+
}
252269

253270
} //Class
254271
} //Namespace

0 commit comments

Comments
 (0)