Skip to content

Commit

Permalink
Set ITopicRepository.Delete()'s isRecursive
Browse files Browse the repository at this point in the history
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!
  • Loading branch information
JeremyCaney committed Sep 8, 2020
1 parent a0cc21c commit 1773113
Show file tree
Hide file tree
Showing 7 changed files with 16 additions and 9 deletions.
2 changes: 1 addition & 1 deletion OnTopic.Data.Caching/CachedTopicRepository.cs
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ public override int Save(Topic topic, bool isRecursive = false, bool isDraft = f
| METHOD: DELETE
\-------------------------------------------------------------------------------------------------------------------------*/
/// <inheritdoc />
public override void Delete(Topic topic, bool isRecursive = false) => _dataProvider.Delete(topic, isRecursive);
public override void Delete(Topic topic, bool isRecursive = true) => _dataProvider.Delete(topic, isRecursive);

} //Class
} //Namespace
2 changes: 1 addition & 1 deletion OnTopic.Data.Sql/SqlTopicRepository.cs
Original file line number Diff line number Diff line change
Expand Up @@ -548,7 +548,7 @@ public override void Move(Topic topic, Topic target, Topic? sibling) {
| METHOD: DELETE
\-------------------------------------------------------------------------------------------------------------------------*/
/// <inheritdoc />
public override void Delete(Topic topic, bool isRecursive = false) {
public override void Delete(Topic topic, bool isRecursive = true) {

/*------------------------------------------------------------------------------------------------------------------------
| Delete from memory
Expand Down
2 changes: 1 addition & 1 deletion OnTopic.TestDoubles/DummyTopicRepository.cs
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ public DummyTopicRepository() : base() { }
| METHOD: DELETE
\-------------------------------------------------------------------------------------------------------------------------*/
/// <inheritdoc />
public override void Delete(Topic topic, bool isRecursive = false) => throw new NotImplementedException();
public override void Delete(Topic topic, bool isRecursive = true) => throw new NotImplementedException();

} //Class
} //Namespace
2 changes: 1 addition & 1 deletion OnTopic.TestDoubles/StubTopicRepository.cs
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ public override void Move(Topic topic, Topic target, Topic? sibling) {
| METHOD: DELETE
\-------------------------------------------------------------------------------------------------------------------------*/
/// <inheritdoc />
public override void Delete(Topic topic, bool isRecursive = false) => base.Delete(topic, isRecursive);
public override void Delete(Topic topic, bool isRecursive = true) => base.Delete(topic, isRecursive);

/*==========================================================================================================================
| METHOD: GET ATTRIBUTES (PROXY)
Expand Down
2 changes: 1 addition & 1 deletion OnTopic.Tests/ITopicRepositoryTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,7 @@ public void Delete_Topic_Removed() {

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

_topicRepository.Delete(topic);
_topicRepository.Delete(topic, true);

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

Expand Down
4 changes: 2 additions & 2 deletions OnTopic.Tests/TopicRepositoryBaseTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ public void Delete_DerivedTopic_ThrowsException() {

derived.DerivedTopic = child;

_topicRepository.Delete(topic);
_topicRepository.Delete(topic, true);

}

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

derived.DerivedTopic = child;

_topicRepository.Delete(topic);
_topicRepository.Delete(topic, true);

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

Expand Down
11 changes: 9 additions & 2 deletions OnTopic/Repositories/ITopicRepository.cs
Original file line number Diff line number Diff line change
Expand Up @@ -138,16 +138,23 @@ public interface ITopicRepository {
/// <summary>
/// Interface method that deletes the provided topic from the tree
/// </summary>
/// <remarks>
/// Prior to OnTopic 4.5.0, the <paramref name="isRecursive"/> defaulted to <c>false</c>. Unfortunately, a bug in the
/// implementation of <see cref="TopicRepositoryBase.Delete(Topic, Boolean)"/> resulted in this not being validated, and
/// 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
/// breaks prior implementations, however, the default for <paramref name="isRecursive"/> was changed to <c>true</c> in
/// order to maintain backward compatibility. In OnTopic 5.0.0, this will be changed back to <c>false</c>.
/// </remarks>
/// <param name="topic">The topic object to delete.</param>
/// <param name="isRecursive">
/// Boolean indicator nothing whether to recurse through the topic's descendants and delete them as well. If set to false
/// (the default) and the topic has children, including any nested topics, an exception will be thrown.
/// and the topic has children, including any nested topics, an exception will be thrown.
/// </param>
/// <requires description="The topic to delete must be provided." exception="T:System.ArgumentNullException">
/// topic != null
/// </requires>
/// <exception cref="ArgumentNullException">topic</exception>
void Delete(Topic topic, bool isRecursive = false);
void Delete(Topic topic, bool isRecursive = true);

} //Interface
} //Namespace

0 comments on commit 1773113

Please # to comment.