Skip to content

Commit 1773113

Browse files
JeremyCaneyJeremyCaney
committed
Set ITopicRepository.Delete()'s isRecursive
In a previous commit, I introduced validation of `TopicRepositoryBase.Delete(…, isRecursive)`, which had not been previously validated (a0cc21c). This was a major bug in that code expected that a topic wouldn't be deleted if it had descendants would, in fact, still delete the topic and all descendants. Oops. While this was fixed, it breaks backward compatibility since `isRecursive` was optional. As such, any code calling into `TopicRepositoryBase.Delete()` without explicitly setting `isRecursive` would not have been expecting an exception in these cases. To "fix" this, I'm setting the `isRecursive` default to `true`. This is technically a breaking change in terms of the interface. But it avoids the bug fix from introducing a breaking change. We'll want to change this back to `false` in OnTopic 5.0.0. Finally, to maintain forward compatibility with OnTopic 5.0.0, and for clarity, I explicitly set the `isRecursive` flag on all calls to `Delete()` from within the `OnTopic.Tests`. As we currently maintain a good understanding of the cases where this is implemented, we have high confidence that this won't impact current implementations. Nevertheless, mea culpa!
1 parent a0cc21c commit 1773113

7 files changed

Lines changed: 16 additions & 9 deletions

File tree

‎OnTopic.Data.Caching/CachedTopicRepository.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -148,7 +148,7 @@ public override int Save(Topic topic, bool isRecursive = false, bool isDraft = f
148148
| METHOD: DELETE
149149
\-------------------------------------------------------------------------------------------------------------------------*/
150150
/// <inheritdoc />
151-
public override void Delete(Topic topic, bool isRecursive = false) => _dataProvider.Delete(topic, isRecursive);
151+
public override void Delete(Topic topic, bool isRecursive = true) => _dataProvider.Delete(topic, isRecursive);
152152

153153
} //Class
154154
} //Namespace

‎OnTopic.Data.Sql/SqlTopicRepository.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -548,7 +548,7 @@ public override void Move(Topic topic, Topic target, Topic? sibling) {
548548
| METHOD: DELETE
549549
\-------------------------------------------------------------------------------------------------------------------------*/
550550
/// <inheritdoc />
551-
public override void Delete(Topic topic, bool isRecursive = false) {
551+
public override void Delete(Topic topic, bool isRecursive = true) {
552552

553553
/*------------------------------------------------------------------------------------------------------------------------
554554
| Delete from memory

‎OnTopic.TestDoubles/DummyTopicRepository.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@ public DummyTopicRepository() : base() { }
5454
| METHOD: DELETE
5555
\-------------------------------------------------------------------------------------------------------------------------*/
5656
/// <inheritdoc />
57-
public override void Delete(Topic topic, bool isRecursive = false) => throw new NotImplementedException();
57+
public override void Delete(Topic topic, bool isRecursive = true) => throw new NotImplementedException();
5858

5959
} //Class
6060
} //Namespace

‎OnTopic.TestDoubles/StubTopicRepository.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -157,7 +157,7 @@ public override void Move(Topic topic, Topic target, Topic? sibling) {
157157
| METHOD: DELETE
158158
\-------------------------------------------------------------------------------------------------------------------------*/
159159
/// <inheritdoc />
160-
public override void Delete(Topic topic, bool isRecursive = false) => base.Delete(topic, isRecursive);
160+
public override void Delete(Topic topic, bool isRecursive = true) => base.Delete(topic, isRecursive);
161161

162162
/*==========================================================================================================================
163163
| METHOD: GET ATTRIBUTES (PROXY)

‎OnTopic.Tests/ITopicRepositoryTest.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -218,7 +218,7 @@ public void Delete_Topic_Removed() {
218218

219219
Assert.AreEqual<int>(2, parent.Children.Count);
220220

221-
_topicRepository.Delete(topic);
221+
_topicRepository.Delete(topic, true);
222222

223223
Assert.AreEqual<int>(1, parent.Children.Count);
224224

‎OnTopic.Tests/TopicRepositoryBaseTest.cs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,7 @@ public void Delete_DerivedTopic_ThrowsException() {
6565

6666
derived.DerivedTopic = child;
6767

68-
_topicRepository.Delete(topic);
68+
_topicRepository.Delete(topic, true);
6969

7070
}
7171

@@ -85,7 +85,7 @@ public void Delete_InternallyDerivedTopic_Succeeds() {
8585

8686
derived.DerivedTopic = child;
8787

88-
_topicRepository.Delete(topic);
88+
_topicRepository.Delete(topic, true);
8989

9090
Assert.AreEqual<int>(0, root.Children.Count);
9191

‎OnTopic/Repositories/ITopicRepository.cs‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -138,16 +138,23 @@ public interface ITopicRepository {
138138
/// <summary>
139139
/// Interface method that deletes the provided topic from the tree
140140
/// </summary>
141+
/// <remarks>
142+
/// Prior to OnTopic 4.5.0, the <paramref name="isRecursive"/> defaulted to <c>false</c>. Unfortunately, a bug in the
143+
/// implementation of <see cref="TopicRepositoryBase.Delete(Topic, Boolean)"/> resulted in this not being validated, and
144+
/// thus it operated <i>as though</i> it were <c>true</c>. This was fixed in OnTopic 4.5.0. As this bug fix potentially
145+
/// breaks prior implementations, however, the default for <paramref name="isRecursive"/> was changed to <c>true</c> in
146+
/// order to maintain backward compatibility. In OnTopic 5.0.0, this will be changed back to <c>false</c>.
147+
/// </remarks>
141148
/// <param name="topic">The topic object to delete.</param>
142149
/// <param name="isRecursive">
143150
/// Boolean indicator nothing whether to recurse through the topic's descendants and delete them as well. If set to false
144-
/// (the default) and the topic has children, including any nested topics, an exception will be thrown.
151+
/// and the topic has children, including any nested topics, an exception will be thrown.
145152
/// </param>
146153
/// <requires description="The topic to delete must be provided." exception="T:System.ArgumentNullException">
147154
/// topic != null
148155
/// </requires>
149156
/// <exception cref="ArgumentNullException">topic</exception>
150-
void Delete(Topic topic, bool isRecursive = false);
157+
void Delete(Topic topic, bool isRecursive = true);
151158

152159
} //Interface
153160
} //Namespace

0 commit comments

Comments
 (0)