Skip to content

Commit

Permalink
Merge branch 'feature/TopicFactory-Create-parameters' into develop
Browse files Browse the repository at this point in the history 
Previously, there were two overloads for the `TopicFactory.Create()` method which used competing parameter orders. Not only did the parameter order compete within the overloads, but it also competed with the corresponding parameter order of the `Topic` constructor. This is confusing.

To mitigate this, I consolidated the two `Create()` overloads and, as part of that process, standardized the parameter order. This allows the logic between these methods to be centralized, and maintains consistency across the application.

Technically, this is a breaking change. In practice, however, the vast majority of callers to `Create()` never pass an `Id` and, therefore, won't notice the change. The only production code that's expected to use this overload are concrete implementations of `ITopicRepository.Load()` which interact directly with a persistence store (i.e., not abstract base classes or decorators or test stubs). The only concrete implementation we have is `SqlTopicRepository`, and it actually loads the `parent` topic separately, and thus doesn't break with this change.

That said, a lot of non-production code breaks with this change. Namely, it's common for unit tests—and test doubles—to generate topics using the `TopicFactory` that have both a `parent` (in order to simulate a topic graph) and an `id` (in order to simulate a saved entity, even though the number is arbitrary). As such, the unit tests needed to be updated to reflect this change.

Outside of that, this isn't _expected_ to impact any third-party implementations.
  • Loading branch information
@JeremyCaney
JeremyCaney committed Jan 29, 2021
2 parents fdd25e7 + 48a2ecc commit 69caaa2
Show file tree
Hide file tree
Showing 6 changed files with 34 additions and 90 deletions.
6 changes: 3 additions & 3 deletions OnTopic.TestDoubles/StubTopicRepository.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,7 @@ public class StubTopicRepository : TopicRepository, ITopicRepository {
container = TopicFactory.Create("Attributes", "List", contentType);
container.Attributes.SetBoolean("IsHidden", true);
}
var attribute = (AttributeDescriptor)TopicFactory.Create(attributeKey, editorType, currentAttributeId++, container);
var attribute = (AttributeDescriptor)TopicFactory.Create(attributeKey, editorType, container, currentAttributeId++);
attribute.IsRequired = isRequired;
attribute.IsExtendedAttribute = isExtended;
return attribute;
Expand All @@ -272,7 +272,7 @@ public class StubTopicRepository : TopicRepository, ITopicRepository {
/*------------------------------------------------------------------------------------------------------------------------
| Establish content
\-----------------------------------------------------------------------------------------------------------------------*/
var web = TopicFactory.Create("Web", "Page", 10000, rootTopic);
var web = TopicFactory.Create("Web", "Page", rootTopic, 10000);

CreateFakeData(web, 2, 3);

Expand All @@ -296,7 +296,7 @@ public class StubTopicRepository : TopicRepository, ITopicRepository {
/// </summary>
private void CreateFakeData(Topic parent, int count = 3, int depth = 3) {
for (var i = 0; i < count; i++) {
var topic = TopicFactory.Create(parent.Key + "_" + i, "Page", parent.Id + (int)Math.Pow(10, depth) * i, parent);
var topic = TopicFactory.Create(parent.Key + "_" + i, "Page", parent, parent.Id + (int)Math.Pow(10, depth) * i);
topic.Attributes.SetValue("ParentKey", parent.Key);
topic.Attributes.SetValue("DepthCount", (depth+i).ToString(CultureInfo.InvariantCulture));
if (depth > 0) {
Expand Down
2 changes: 1 addition & 1 deletion OnTopic.Tests/ITopicRepositoryTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ public class ITopicRepositoryTest {
public void Load_ValidUniqueKey_ReturnsCorrectTopic() {

var topic = _topicRepository.Load("Root:Configuration:ContentTypes:Page");
var child = TopicFactory.Create("Child", "ContentType", Int32.MaxValue, topic);
var child = TopicFactory.Create("Child", "ContentType", topic, Int32.MaxValue);

Assert.AreEqual<string>("Page", topic.Key);

Expand Down
4 changes: 2 additions & 2 deletions OnTopic.Tests/SqlTopicRepositoryTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -200,8 +200,8 @@ public class SqlTopicRepositoryTest {
public void LoadTopicGraph_WithDeletedRelationship_RemovesRelationship() {

var topic = TopicFactory.Create("Test", "Container", 1);
var child = TopicFactory.Create("Child", "Container", 2, topic);
var related = TopicFactory.Create("Related", "Container", 3, topic);
var child = TopicFactory.Create("Child", "Container", topic, 2);
var related = TopicFactory.Create("Related", "Container", topic, 3);

child.Relationships.SetTopic("Test", related);

Expand Down
2 changes: 1 addition & 1 deletion OnTopic.Tests/TopicMappingServiceTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -770,7 +770,7 @@ public class TopicMappingServiceTest {
public async Task Map_CircularReference_ReturnsCachedParent() {

var topic = TopicFactory.Create("Test", "Circular", 1);
var childTopic = TopicFactory.Create("ChildTopic", "Circular", 2, topic);
var childTopic = TopicFactory.Create("ChildTopic", "Circular", topic, 2);

var mappedTopic = await _mappingService.MapAsync<CircularTopicViewModel>(topic).ConfigureAwait(false);

Expand Down
36 changes: 18 additions & 18 deletions OnTopic.Tests/TopicQueryingTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,10 +54,10 @@ public class TopicQueryingTest {
public void FindAllByAttribute_ReturnsCorrectTopics() {

var parentTopic = TopicFactory.Create("ParentTopic", "Page", 1);
var childTopic = TopicFactory.Create("ChildTopic", "Page", 5, parentTopic);
var grandChildTopic = TopicFactory.Create("GrandChildTopic", "Page", 20, childTopic);
var grandNieceTopic = TopicFactory.Create("GrandNieceTopic", "Page", 3, childTopic);
var greatGrandChildTopic = TopicFactory.Create("GreatGrandChildTopic", "Page", 7, grandChildTopic);
var childTopic = TopicFactory.Create("ChildTopic", "Page", parentTopic, 5);
var grandChildTopic = TopicFactory.Create("GrandChildTopic", "Page", childTopic, 20);
var grandNieceTopic = TopicFactory.Create("GrandNieceTopic", "Page", childTopic, 3);
var greatGrandChildTopic = TopicFactory.Create("GreatGrandChildTopic", "Page", grandChildTopic, 7);

grandChildTopic.Attributes.SetValue("Foo", "Baz");
greatGrandChildTopic.Attributes.SetValue("Foo", "Bar");
Expand All @@ -79,9 +79,9 @@ public class TopicQueryingTest {
public void FindFirstParent_ReturnsCorrectTopic() {

var parentTopic = TopicFactory.Create("ParentTopic", "Page", 1);
var childTopic = TopicFactory.Create("ChildTopic", "Page", 5, parentTopic);
var grandChildTopic = TopicFactory.Create("GrandChildTopic", "Page", 20, childTopic);
var greatGrandChildTopic = TopicFactory.Create("GreatGrandChildTopic", "Page", 7, grandChildTopic);
var childTopic = TopicFactory.Create("ChildTopic", "Page", parentTopic, 5);
var grandChildTopic = TopicFactory.Create("GrandChildTopic", "Page", childTopic, 20);
var greatGrandChildTopic = TopicFactory.Create("GreatGrandChildTopic", "Page", grandChildTopic, 7);

var foundTopic = greatGrandChildTopic.FindFirstParent(t => t.Id is 5);

Expand All @@ -99,9 +99,9 @@ public class TopicQueryingTest {
public void GetRootTopic_ReturnsRootTopic() {

var parentTopic = TopicFactory.Create("ParentTopic", "Page", 1);
var childTopic = TopicFactory.Create("ChildTopic", "Page", 5, parentTopic);
var grandChildTopic = TopicFactory.Create("GrandChildTopic", "Page", 20, childTopic);
var greatGrandChildTopic = TopicFactory.Create("GreatGrandChildTopic", "Page", 7, grandChildTopic);
var childTopic = TopicFactory.Create("ChildTopic", "Page", parentTopic, 5);
var grandChildTopic = TopicFactory.Create("GrandChildTopic", "Page", childTopic, 20);
var greatGrandChildTopic = TopicFactory.Create("GreatGrandChildTopic", "Page", grandChildTopic, 7);

var rootTopic = greatGrandChildTopic.GetRootTopic();

Expand All @@ -119,7 +119,7 @@ public class TopicQueryingTest {
public void GetByUniqueKey_RootKey_ReturnsRootTopic() {

var parentTopic = TopicFactory.Create("ParentTopic", "Page", 1);
_ = TopicFactory.Create("ChildTopic", "Page", 2, parentTopic);
_ = TopicFactory.Create("ChildTopic", "Page", parentTopic, 2);

var foundTopic = parentTopic.GetByUniqueKey("ParentTopic");

Expand All @@ -138,10 +138,10 @@ public class TopicQueryingTest {
public void GetByUniqueKey_ValidKey_ReturnsTopic() {

var parentTopic = TopicFactory.Create("ParentTopic", "Page", 1);
var childTopic = TopicFactory.Create("ChildTopic", "Page", 5, parentTopic);
var grandChildTopic = TopicFactory.Create("GrandChildTopic", "Page", 20, childTopic);
var greatGrandChildTopic1 = TopicFactory.Create("GreatGrandChildTopic1", "Page", 7, grandChildTopic);
var greatGrandChildTopic2 = TopicFactory.Create("GreatGrandChildTopic2", "Page", 7, grandChildTopic);
var childTopic = TopicFactory.Create("ChildTopic", "Page", parentTopic, 5);
var grandChildTopic = TopicFactory.Create("GrandChildTopic", "Page", childTopic, 20);
var greatGrandChildTopic1 = TopicFactory.Create("GreatGrandChildTopic1", "Page", grandChildTopic, 7);
var greatGrandChildTopic2 = TopicFactory.Create("GreatGrandChildTopic2", "Page", grandChildTopic, 7);

var foundTopic = greatGrandChildTopic1.GetByUniqueKey("ParentTopic:ChildTopic:GrandChildTopic:GreatGrandChildTopic2");

Expand All @@ -160,9 +160,9 @@ public class TopicQueryingTest {
public void GetByUniqueKey_InvalidKey_ReturnsNull() {

var parentTopic = TopicFactory.Create("ParentTopic", "Page", 1);
var childTopic = TopicFactory.Create("ChildTopic", "Page", 5, parentTopic);
var grandChildTopic = TopicFactory.Create("GrandChildTopic", "Page", 20, childTopic);
var greatGrandChildTopic = TopicFactory.Create("GreatGrandChildTopic", "Page", 7, grandChildTopic);
var childTopic = TopicFactory.Create("ChildTopic", "Page", parentTopic, 5);
var grandChildTopic = TopicFactory.Create("GrandChildTopic", "Page", childTopic, 20);
var greatGrandChildTopic = TopicFactory.Create("GreatGrandChildTopic", "Page", grandChildTopic, 7);

var foundTopic = greatGrandChildTopic.GetByUniqueKey("ParentTopic:ChildTopic:GrandChildTopic:GreatGrandChildTopic2");

Expand Down
74 changes: 9 additions & 65 deletions OnTopic/TopicFactory.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,66 +31,6 @@ public static class TopicFactory {
/*==========================================================================================================================
| METHOD: CREATE
\-------------------------------------------------------------------------------------------------------------------------*/
/// <summary>
/// Factory method for creating new strongly-typed instances of the topics class, assuming a strongly-typed subclass is
/// available.
/// </summary>
/// <remarks>
/// When creating new attributes, the <see cref="AttributeValue"/>s for both <see cref="Topic.Key"/> and <see
/// cref="Topic.ContentType"/> will be set to <see cref="AttributeValue.IsDirty"/>, which is required in order to
/// correctly save new topics to the database.
/// </remarks>
/// <param name="key">A string representing the key for the new topic instance.</param>
/// <param name="contentType">A string representing the key of the target content type.</param>
/// <param name="parent">Optional topic to set as the new topic's parent.</param>
/// <exception cref="ArgumentException">
/// Thrown when the class representing the content type is found, but doesn't derive from <see cref="Topic"/>.
/// </exception>
/// <returns>A strongly-typed instance of the <see cref="Topic"/> class based on the target content type.</returns>
/// <requires description="The topic key must be specified." exception="T:System.ArgumentNullException">
/// !String.IsNullOrWhiteSpace(key)
/// </requires>
/// <requires
/// decription="The key should be an alphanumeric sequence; it should not contain spaces or symbols."
/// exception="T:System.ArgumentException">
/// !key.Contains(" ")
/// </requires>
/// <requires description="The content type key must be specified." exception="T:System.ArgumentNullException">
/// !String.IsNullOrWhiteSpace(contentType)
/// </requires>
/// <requires
/// decription="The contentType should be an alphanumeric sequence; it should not contain spaces or symbols."
/// exception="T:System.ArgumentException">
/// !contentType.Contains(" ")
/// </requires>
public static Topic Create(string key, string contentType, Topic? parent = null) {

/*------------------------------------------------------------------------------------------------------------------------
| Validate contracts
\-----------------------------------------------------------------------------------------------------------------------*/
Contract.Requires<ArgumentNullException>(!String.IsNullOrWhiteSpace(key), nameof(key));
Contract.Requires<ArgumentNullException>(!String.IsNullOrWhiteSpace(contentType), nameof(contentType));
TopicFactory.ValidateKey(key);
TopicFactory.ValidateKey(contentType);

/*------------------------------------------------------------------------------------------------------------------------
| Determine target type
\-----------------------------------------------------------------------------------------------------------------------*/
var targetType = TypeLookupService.Lookup(contentType);

Contract.Assume(
targetType,
$"The content type {contentType} could not be located in the ITypeLookupService, and no fallback could be " +
$"identified."
);

/*------------------------------------------------------------------------------------------------------------------------
| Identify the appropriate topic
\-----------------------------------------------------------------------------------------------------------------------*/
return (Topic)Activator.CreateInstance(targetType, key, contentType, parent, -1)!;

}

/// <summary>
/// Factory method for creating new strongly-typed instances of the topics class, assuming a strongly-typed subclass is
/// available. Used for cases where a <see cref="Topic"/> is being deserialized from an existing instance, as indicated
Expand All @@ -103,22 +43,22 @@ public static class TopicFactory {
/// </remarks>
/// <param name="key">A string representing the key for the new topic instance.</param>
/// <param name="contentType">A string representing the key of the target content type.</param>
/// <param name="id">The unique identifier assigned by the data store for an existing topic.</param>
/// <param name="parent">Optional topic to set as the new topic's parent.</param>
/// <param name="id">The unique identifier assigned by the data store for an existing topic.</param>
/// <exception cref="ArgumentException">
/// Thrown when the class representing the content type is found, but doesn't derive from <see cref="Topic"/>.
/// </exception>
/// <returns>A strongly-typed instance of the <see cref="Topic"/> class based on the target content type.</returns>
public static Topic Create(string key, string contentType, int id, Topic? parent = null) {
public static Topic Create(string key, string contentType, Topic? parent = null, int id = -1) {

/*------------------------------------------------------------------------------------------------------------------------
| Validate input
\-----------------------------------------------------------------------------------------------------------------------*/
Contract.Requires<ArgumentNullException>(!String.IsNullOrWhiteSpace(key), nameof(key));
Contract.Requires<ArgumentNullException>(!String.IsNullOrWhiteSpace(contentType), nameof(contentType));
Contract.Requires<ArgumentOutOfRangeException>(id > 0, nameof(id));
TopicFactory.ValidateKey(key);
TopicFactory.ValidateKey(contentType);

ValidateKey(key);
ValidateKey(contentType);

/*------------------------------------------------------------------------------------------------------------------------
| Determine target type
Expand All @@ -138,6 +78,10 @@ public static class TopicFactory {

}

/// <inheritdoc cref="Create(String, String, Topic?, Int32)"/>
public static Topic Create(string key, string contentType, int id) =>
Create(key, contentType, null, id);

/*==========================================================================================================================
| METHOD: VALIDATE KEY
\-------------------------------------------------------------------------------------------------------------------------*/
Expand Down

0 comments on commit 69caaa2

Please sign in to comment.