The ITopicRepository interface defines the isRecursive parameter on the Delete() method as optional, with a default value of false. Unfortunately, the C# compiler doesn't necessitate that implementations use the same default—or even that they apply a default at all. This can cause issues because, according to the interface, Delete(topic) should be accepted, but according to the implementations it is not. Derived topics—such as ObservableTopicRepository and TopicRepositoryDecorator—should explicitly set isRecursive as an optional parameter defaulting to false.

Note: This is due to optional parameters in C# being a bit of a hack. They actually get compiled as required parameters, and the defaults are injected into calling code at compile time. As such, what's really happening is that the isRecursive parameter is always required, but the default value of false isn't being added to callers of repositories that don't define it.

Background

The TopicRepositoryBase class has a default implementation of GetContentTypeDescriptors() which loads the ContentTypeDescriptors via Load() and then caches them locally. This collection is not modified until the application is reset, even if ContentTypeDescriptors are added or removed.

Problem

This works fine when making most day-to-day edits. Issues are introduced, however, when adding or removing ContentTypeDescriptors, as these aren't added to the GetContentTypeDescriptors() cache. As a result, these changes won't be reflected in e.g. the OnTopic Editor's interface until after an application reset. Further, exceptions will be thrown if attempting to commit instance of these content types to the database, since e.g. SqlTopicRepository.Save() won't be able to validate them via the GetContentTypeDescriptors() cache.

Solution

To mitigate this, ideally we should update the GetContentTypeDescriptors() cache any time

1. Save() is called with a new ContentTypeDescriptor, or
2. Delete() is called with an existing ContentTypeDescriptor.

Implementation

The following represents pseudocode for how this might be implemented:

public abstract class TopicRepositoryBase {
  public void Save(Topic topic, bool isRecursive = false) {
    …
    if (topic.ContentType == "ContentTypeDescriptor" && topic.Id < 0) {
      _contentTypeDescriptors.Add(topic);
    }
    …
  }
  public void Delete(Topic topic, bool isRecursive) {
    …
    if (topic.ContentType == "ContentTypeDescriptor") {
      _contentTypeDescriptors.Remove(topic);
      //Need to remove all descendants as well
    }
  }
}

Currently, a number of implementations which aren’t required by the core OnTopic library are included in the OnTopic project. We should consider migrating these to separate projects (and, thus, packages) within the same repository, similar to e.g., SqlTopicRepository.

Obviously, the interfaces, base classes, and any shared classes would remain in the OnTopic project.

Candidates

• (Cached)TopicMappingService
• (Cached)HierarchicalTopicMappingService
• ReverseTopicMappingService

Positively, this would better separate ancillary libraries. Negatively, it would increase the number of packages, even though most would be needed by typical projects, and it’s unlikely that we’d end up with alternate implementations that might need to be swapped out.

The TopicViewLocationExpander defines a number of custom conventions for loading views based on the Topic.ContentType as well as an optional View (which can be defined in a variety of locations). Currently, if the view exists in both /Areas/{Area}/Views and /Views, however, the latter takes precedence. This is unintuitive and unexpected; we'd expect the view closest to the current controller to take precedence.

A parent is, fundamentally, a topic reference, but it is currently stored on the [dbo].[Topics] table, and not accessible via Topic.References.

Storing it as a topic reference would be more consistent with other topic references and offer better referential integrity, but it would also introduce some specific challenges.

Challenges

• Topic references are versioned, but we don't want a rollback to revert the location in the topic graph. As such, the Parent reference would need to be excluded from a Rollback().
• May need to reintroduce a TopicIndex to combine the latest ParentId with [dbo].[Topics] data. Or could Parent just be assembled just like other topic references?

Currently, the ReverseTopicMappingService will bypass any IAssociatedTopicBindingModel instances whose UniqueKey is null or empty. But if a UniqueKey is empty, that should be used to delete any existing topic references that have the same Key. That's how attributes work in the ReverseTopicMappingService*, but not topic references. This isn't necessarily a bug, but it's definitely an unintuitive and unnecessary limitation that should be resolved.

The relevant code is within the SetReference() method of the ReverseTopicMappingService.

* At least assuming the attribute isn’t backed by a property which requires enforcement of business logic (#65).

Background

Currently, when calling ITopicRepository.Save()—and especially SqlTopicRepository.Save()—all attributes flagged as AttributeDescriptor.IsExtendedAttribute are sent to the UpdateTopic stored procedure's @ExtendedAttributesXml parameter, but only AttributeValue.IsDirty attributes are sent via the @Attributes parameter.

Problem

This introduces a wrinkle when supporting moved attributes—i.e., attribute values that haven't changed, but whose storage location has been reconfigured via AttributeDescriptor.IsExtendedAttribute since the Topic was last Save()d. This can potentially result in data loss, since the ExtendedAttributesXml is always saved (thus deleting the attribute from the IsExtendedAttribute store), but only changed attributes are stored in the indexed attribute store.

Solution

To handle this, we should ideally factor in configuration mismatches as part of Save(). This would require tracking where attributes came from (e.g., via a AttributeValue.IsExtendedAttribute property) so that we can detect mismatches with the configuration (via the AttributeDescriptor.IsExtendedAttribute property). Then, any mismatched attributes would be sent to their respective store, regardless of whether or not they have been changed (i.e., independent of IsDirty()).

Implementation

• AttributeValue.IsExtendedAttribute
• AttributeValueCollection.SetValue(…, isExtendedAttribute)
• Call SetValue(…, isExtendedAttribute) from SqlTopicRepository.Load()
• TopicRepositoryBase.IsExtendedAttributeMismatch()
• Factor IsExtendedAttributeMismatch() into GetAttributes()
• Factor IsExtendedAttributeMismatch() into Save()

Currently, while we have decent unit tests for much of the core functionality, the actual SqlTopicRepository itself is not subjected to testing—and it would be difficult to mitigate that, given the nature of the dependencies. Integration tests offer an alternative.

Priority

The scope of impact for this is comparatively small compared to e.g., #39. Much of the business logic is already tested via the TopicRepository base class and the SqlDataReader extension methods. And the underlying database logic is tested by the SQL unit tests. Given that, the priority of this might be pretty low.

Performance

The biggest concern with this is performance, as well as the dependency on a database server. As with the SQL unit tests, this may be best handled exclusively on the localhost using a local test database which isn’t run during the CI/CD pipeline, and may even be unloaded during normal operation.

Git Repository

With the SqlTopicRepository, database schema, unit tests, and proposed integration tests, we’re looking at four projects that are closely related to one another, but necessarily loosely coupled from the core library. Does it make sense to separate these out into their own git repository? This makes coordinating versioning harder. But it would be especially useful if e.g., we later introduce another core repository implementation (e.g., MongoTopicRepository).

Currently, AttributeDescriptor.ModelType is set based on a mapping between EditorType and the ModelType enum. This is clumsy and error prone. With the new strongly typed derivatives of AttributeDescriptor, a smarter approach is to allow this to be set to a default, and then overwritten in derivative attributes (e.g., RelationshipAttribute) as appropriate.

When the NoIndex attribute is set, typical implementations exclude the page from indexing by adding a noindex meta tag to the page header, as one would expect. For sites using the OnTopic.ViewModels library, this is driven by the PageTopicViewModel.NoIndex property.

Currently, however, the SitemapController not only excludes topics marked with NoIndex, as one would expect—but additionally excludes all of their descendents. This potentially results entire segments of the topic tree being excluded from the sitemap.

Since this isn’t applied in site templates (e.g., via PageTopicViewModel), these downstream pages still get indexed, they’re just not represented in the sitemap. That’s confusing, and undermines the value of the sitemap as a comprehensive index of the site structure.

Solution

At minimum, the SitemapController should function the same as the PageTopicViewModel.NoIndex property. If there is a need to support a recursive NoIndex attribute, then that should be added separately (e.g., NoIndexRecursive or NoIndexTree).

Establish a reference configuration as a JSON file on a new GitHub repository, allowing adopters to import the basic schema.

Optionally, break up into smaller pieces that map to optional features, which might include extended content types such as e.g., PageGroup, ContentList, Search, and Video.

Attributes can be null when returned from the database. This happens when a previous attribute was deleted; the new version will use a null value. When loading values as part of e.g. ITopicRepository.Refresh() or Rollback(), this should result in deleting any existing attribute values, if present. Currently, this does not happen.

Impact

If the null values are skipped, any previous values will be retained in memory. There may also be circumstances where this could result in those values being inadvertently persisted to the database—though, fortunately, this will generally be avoided by the fact that Save() doesn't include clean indexed attributes when calling the SaveTopic stored procedure.

Cause

The SetIndexedAttributes() extension method skips over null values. That was fine when this was only used to create new topics. With the OnTopic 5.0.0 support for passing a referenceTopic to Load(), however, this potentially prevents existing topics from being properly updated with new attributes. This is most notable with ITopicRepository.Refresh(), since the goal is explicitly to update existing topics with new values.

Currently, strings can be set to null, but other potentially nullable types cannot—including references, as well as value types such as int? or bool?. In this case, the call to SetPropertyValue() will be ignored entirely, and any preexisting value will be retained. This could result in unexpected, if not buggy, behavior. To remedy this, ideally the MemberDispatcher should be able to detect nullable types and, if a type is nullable, then permit the null value. If the type is not nullable, we should consider falling back to default.

The TopicViewResultExecutor contains logic for identifying the (optional) View from the following locations:

• Query string,
• HTTP Accept headers
• Controller's action name
• Topic's View property
• Topic's ContentType property

The evaluation of the controller's action name, however, is not currently working as expected, resulting in buggy behavior. In particular, it fails if there isn't a View query string or an Accept HTTP header that have already been evaluated for a valid view name, and failed. Typically, there will be an Accept HTTP header, though one might be missing during e.g. AJAX calls.

Currently, the OnTopic Editor has a basic read-only JSON interface, as well as some built-in actions for handling updates, but it's highly specific to the Editor and doesn't support other clients.

Ideally, we'd establish a new e.g. TopicJsonController that exposes a full REST API that can be used by not only the OnTopic Editor, but also any other clients that need AJAX access to the OnTopic database.

Questions

• Can this be registered as part of the OnTopic Editor so it doesn't need to be independently configured?
• Should this support both read and write operations? Or just read operations? Or both, via separate services?
• If it supports write operations, how should authorization be handled?
• How would we handle sensitive data such as user records? Would these be hidden by an attribute that necessitates authentication?
• Should this utilize the OnTopic Data Transfer library as a standard interchange format? If so, it makes sense as a separate repository.

When calling StubTopicRepository.Load(topic, version) or Load(topicId, version, referenceTopic), the TopicLoaded event is correctly raised, but doesn't contain the version parameter. That's because these overloads call into Load(uniqueKey, isRecursive), and it's that overload which is raising the event. To fix this, the Load(uniqueKey, isRecursive) logic will likely need to be moved into its own private function, which each of the public overloads responsible for raising their own event.

Background

The byline (LastModifiedBy) and dateline (LastModified) attributes are set apart in that they're automatically calculated. This means they have the potential to change every time Save() is called, even if no other attributes were modified.

Solution

On the OnTopic Data Exchange library, we mitigate this by only updating these values if other attributes have been modified. Ideally, we'd incorporate this into the library itself so it can be used by other clients, such as the OnTopic Editor. In the best case scenario, this would be handled transparently by Save() without any downstream libraries even needing to be aware of it.

Implementation

Currently, there's an IsDirty() method on AttributeValueCollection, which accepts an attributeKey parameter to check whether an individual AttributeValue is dirty (i.e., has been modified). We could extend this to include the following overloads:

public bool IsDirty() { … }
public bool IsDirty(bool excludeLastModified) { … }

In these overloads, the first would check to see if any AttributeValue is dirty. If excludeLastModified is flagged, then this would check any AttributeValues that don't start with LastModified.

Challenges

There are a couple of challenges to this that are worth considering.

Extended Attributes

Do we need to account for indexed vs. extended? Perhaps not, because a) LastModified and LastModifiedBy are both indexed, and b) extended attributes have their own deduplication handling in the UpdateTopic stored procedure. Also, even if no (other) indexed attributes are being updated, we still want to write LastModified and LastModifiedBy if any extended attributes are updated, so the decision as to whether or not to write them shouldn't be conditional on storage.

Naming Conventions

This implementation will affect any attributes that begin with LastModified. Are we confident that this won't bring in false positives in the future? I think this is safe. Given the semantics of the name, it's reasonable to assume that even if we did, they would be specific to that update and should thus be conditional upon other changes. E.g., if we added a LastModifiedNotes attribute, that would only make sense if other modifications were made.

Non-Attribute Updates

Just because no other attributes have been updated doesn't mean that the topic hasn't been updated. Most notably, the relationships could be modified. Currently, however, we don't have an elegant way to track these changes via the interface (i.e., there isn't a RelatedTopicCollection.IsDirty() equivalent). For now, this is considered an edge case, but we may want to revisit it in the future.

ASP.NET Core relies on a number of framework conventions that make it cumbersome to unit test—and which, in some cases, might even limit the benefits of unit testing. This makes them well-suited for integration tests.

Scope

In particular, the following are areas that are difficult to meaningfully unit test but trivial to setup integration tests for:

• Routing and associated extension methods
• Dynamic selection of views based on contextual data
• Searching for views on the file system

Performance

The most expensive part of this process is loading a comprehensive database. Given that, the optimal solution will be to establish a stub ITopicRepository with hard-coded data which can be loaded into memory.

Host

Integration testing works hand in-hand with a host project. We already have a host project. But it typically operates off of a live SqlTopicRepository in order to evaluate against real-world conditions. Can this be modified to conditionally use a stub repository, possibly by selecting an alternateStartup? Or should it be a separate, lighter weight project intended exclusively as a test harness? Likely the latter.

Tests

• Define view by:
  • Query String
  • Header
  • Topic
• Find views in:
  • /Views/ContentTypes/{ContentType}[.{View}].cshtml
  • /Views/{ContentType}/{ContentType|View}.cshtml
  • /Views/{Controller}/{Action}.cshtml
  • /Views/Shared/{View}.cshtml
• Route by:
  • /{rootTopic}/{path}
  • /{area}/{path}
  • /{area?}/{controller}/{action?}/{id?}
  • /{area}/{action?}/{id?}

Git Repository

If we combine the core library, the hosts, the unit tests, and the integration tests, we’re looking at 3-5 projects which are closely related, but only loosely dependent on the other core projects. Does it make sense to move these to a new git repository? That makes good organizational sense, but makes it harder to coordinate versioning, metapackage inclusion, deeply integrated features, and testing. The latter might be solvable with conditional local project dependencies, possibly via a separate reference host repository.

Typically, a ReadOnlyKeyedTopicCollection<T> will be initialized with an innerCollection. If it isn't, then it will always be empty. There are times, however, when an empty collection is expected—such as when no results are found in a search operation. In that case, the ReadOnlyKeyedCollection<T> should be able to be initialized without an innerCollection. The innerCollection parameter is already marked as optional, but if that is relied upon then a guard clause will throw an exception.

The use of a recursive query in the topics_DeleteTopic sproc to determine the list of topics to be deleted can result in a race condition if multiple topics are simultaneously deleted. It's highly unlikely that this will happen under normal workflow conditions. That said, it can easily happen in a bulk delete scenario. A lock is the preferred solution, but as a quick fix storing the topic list in a local variable inside the stored procedure should avoid this scenario.

When Topic.References.Clear() is called, the underlying implementation on the TopicReferenceCollection has logic to loop through each topic reference and remove the reciprocal relationship from the targets' IncomingRelationship collection. This logic isn't firing, however, as the base.ClearItems() is being called first, and thus the loop is finding no items in the current collection.

Currently, the TopicRepository.GetAttributes() method will include attributes under its isDirty check if their IsExtendedAttribute property doesn't match the IsExtendedProperty value of the AttributeDescriptor, even if the AttributeRecord is otherwise clean. This usually happens if the location that an attribute is configured to save has been updated since the attribute was last saved. By detecting these as dirty, we ensure they will get updated with the next Save().

Confusingly, these attributes will also be returned if isDirty is set to false. That doesn't hurt anything in our current logic, but it could lead to potential bugs in the future. If nothing else, it's inconsistent and unintuitive; an attribute should be able to be clean and dirty at the same time. Basically, the difference here is whether we're measure attributes that are either explicitly or implicitly dirty, or just explicitly dirty.

While we're at it, we may want to evaluate how these are returned along with isExtendedAttribute. If an AttributeRecord was not loaded with IsExtendedAttribute, but its AttributeDescriptor is set to IsExtendedAttribute, should it be returned with the isExtendedAttribute parameter? I think it should be. Basically, isExtendedAttribute should follow the AttributeDescriptor, and ignore the AttributeRecord. (It may already do this, but we should confirm.)

Currently, topic references are stored as attributes with a Key ending with Id and a Value representing the target Topic.Id. This prevents any type of referential integrity from being maintained via foreign key constraints and introduces complexities for handling Import() and Export() operations with the OnTopic Data Transfer library.

Fundamentally, though, these are just relationships, and so we may be able to simply to store them as such. The main difference is that they’re (usually) 1:1 relationships. These can still be stored in Topic.Relationships (and the Relationships table), but we’d likely need some type of special entry point for saving and retrieving these so there isn’t a need to e.g.:

topic.Relationships.GetTopics(“DerivedTopic”).FirstOrDefault()

Alternatively, this could be stored in a new data structure as e.g., Topic.References.

Once this is complete, we can update the DerivedTopic handling in TopicRepositoryBase.Delete() to look for IncomingRelationships.

If we want to support existing references—such as DerivedTopic or Parent—then we’ll want to introduce a basic EnforceBusinessLogic() function with a reference back to the parent Topic. This will be similar to, but simpler than, the AttributeValueCollection implementation, as there aren’t any ancillary parameters we need to contend with, unless we later choose to version these.

Currently, ContentTypeDescriptors are composed of AttributeDescriptors, which describe Attributes—but also Relationships and TopicReferences. This is a bit confusing. This is especially true since References, in particular, operate just like Attributes, except that they’re Topic-valued.

Options

• Rename AttributeDescriptor to e.g., FieldDescriptor.
• Rename (Extended)Attributes to e.g., ScalarValues.
• Rename TopicReferences to e.g., TopicAttribute or ReferenceAttributes.

Consider renaming some of these to establish a more consistent vocabulary between metadata and the Topic entity object model.

OnTopic 5.0.0 includes a lot of breaking changes, including types, members, and parameters that have been renamed or removed. To aid in migration, we should ensure that these have a placeholders for their previous signature marked with [Obsolete]. This will make it easier to migrate code by providing clear instructions to implementations referring to the legacy signatures.

Contents

• Types
  • Renamed
  • Removed
  • Replaced
• Members
  • Renamed
  • Removed
• Parameters
• Not Possible (or Necessary?

Types

Renamed

• [Follow(Relationships)] renamed to [Include(AssociationTypes)] (7c150d8)
  • Relationships renamed to AssociationTypes (78600fd)
• [Relationship(RelationshipType)] renamed to [Collection(CollectionType)] (f84ae66)
  • RelationshipType renamed to CollectionType (1daf799)
• AttributeValueCollection renamed to AttributeCollection (f51407a)
  • AttributeValue renamed to AttributeRecord (f51407a)
• {Delete}EventArgs renamed to Topic{Delete}EventArgs (37b38f5)
• TopicCollection to KeyedTopicCollection (826b93c)*
  • TopicCollection<T> to KeyedTopicCollection<T> (826b93c)
  • ReadOnlyTopicCollection to ReadOnlyKeyedTopicCollection (826b93c)*
    • ReadOnlyTopicCollection<T> to ReadOnlyKeyedTopicCollection<T> (826b93c)

Note: The TopicCollection and ReadOnlyTopicCollection cannot be marked as [Obsolete] as they have been subsequently replaced with non-keyed collections with the same names.

Removed

• AttributeTypeDescriptor removed; AttributeDescriptor should be used instead (7b00274)

Replaced

• RelatedTopicCollection replaced with TopicMultiMap (66ccd9d)
  • NamedTopicCollection replaced with KeyValuesPair<T> (66ccd9d)

Members

Renamed

• Topic.DerivedTopic renamed to BaseTopic (2486ab2)
• AttributeKeyAttribute.Value renamed to Key
• TopicRepository.GetContentTypeDescriptors(ContentTypeDescriptor) renamed to SetContentTypeDescriptors() (a078fc8)
• Topic.Relationships (was a RelatedTopicCollection, now a TopicRelationshipMultiMap) (66ccd9d)
  • SetTopic() renamed to SetValue() (34a8c52)
  • GetAllTopics() renamed to GetAllValues() (34a8c52)
  • GetTopics() renamed to GetValues() (34a8c52)
  • RemoveTopic() renamed to Remove() (34a8c52)
  • ClearTopics() renamed to Clear() (34a8c52)
• TopicRepository
  • RenameEvent renamed to TopicRenamed (37b38f5)
  • DeleteEvent renamed to TopicDeleted (37b38f5)
  • MoveEvent renamed to TopicMoved (37b38f5)
• INavigationViewModel.CurrentKey replaced with CurrentWebPath (8fd4d80, 31475d2)

Removed

• Topic.Description removed, in favor of Attributes.GetValue() (6c3a2e9)
• ReadOnlyTopicCollection.FromList() removed, in favor of constructor (b013e38)
• ReadOnlyKeyedTopicCollection<T>.FromList() removed, in favor of constructor (b013e38)
• StaticTypeLookupService.DefaultType removed, in favor of Lookup() fallbacks (ab4253d)
• IRouteBuilder.MapTopicRoute() removed, in favor of IEndpointRouteBuilder.MapTopicRoute() (6be993b, 826b93c)
• ….GetTopic() was moved to KeyedCollection (826b93c), no longer supported by:
  • TopicCollection (826b93c)
  • ReadOnlyTopicCollection (826b93c)
• ITopicViewModel.IsHidden removed, since hidden associations are not mapped (68a4e83)

Parameters

• TopicFactory.Create() parameter order updated, with id moved after parent (69caaa2)
• ITopicRepository (and implementations)
  • Load() parameter isRecursive moved after new referenceTopic parameter (704c8de)
  • Save() parameter isDraft parameter removed (59a7716)
• INavigationTopicViewModel<T>.IsSelected(uniqueKey) parameter renamed to webPath (7a6e7ef)

Not Possible (or Necessary?)

The following cannot be included because the new types are implicitly compatible with the old types, thus introducing an ambiguous reference.

• ITopicMappingService
  • MapAsync(Topic, Relationships) updated to `MapAsync(Topic, AssociationTypes) (78600fd)
  • MapAsync(…, ConcurrentDictionary<int, object>) updated to MapAsync(…, MappedTopicCache) (d8e3c68)

The TrackedRecord<T>.Value property is nullable, but when constructing a new TrackedRecord<T> via the constructor, the value parameter is required—both in terms of its nullability annotation, as well as an explicit guard clause. The same is true of the derived TopicReferenceRecord and AttributeRecord constructors.

In practice, we generally prefer creating these via e.g. TrackedRecord<T>.SetValue(), which doesn't use the constructor, and can set a null value. But this mismatch isn't consistent with the data model. As such, the constructors should be updated to maintain parity with the underlying property return types they represent.

The ContentTypeDescriptor.IsTypeOf() method allows a caller to determine if a content type inherits from another content type in the topic tree. It will return true if an ascendant ContentTypeDescriptor has a Key matching the supplied contentTypeName. If not, however, it is also defaulting to true. This makes the method effectively useless. Fortunately, this method is rarely used—but if it is, it isn't working as expected.

Background

Each ContentTypeDescriptor object cached as part of the TopicRepositoryBase.GetContentTypeDescriptors() method has a locally cached rollup of AttributeDescriptors.

Note: This is very similar to #16, which pertains to ContentTypeDescriptor updates instead of AttributeDescriptor updates; there may be overlap in the implementation.

Problem

This is satisfactory for most cases. Issues are introduced, however, when adding or removing AttributeDescriptors, as these new values won't be found. As a result, these changes won't be reflected in e.g. the OnTopic Editor's interface until after an application reset. Further, exceptions will be thrown if attempting to commit instance of these attributes to the database, since e.g. SqlTopicRepository.Save() won't be able to validate them via the GetContentTypeDescriptors() cache.

Solution

To mitigate this, we should update the AttributeDescriptors collection on the ContentTypeDescriptor anytime an AttributeDescriptor is added or removed from the Attributes nested topic collection. This will include calls to Save() or Delete() (for AttributeDescriptors) or Move() (for ContentTypeDescriptors).

Note: This may also need to be recursive, since AttributeDescriptors inherit AttributeDescriptor values from parent ContentTypeDescriptors.

Challenges

• There are now multiple types of AttributeDescriptors, so trying to identify what is an AttributeDescriptor based on the ContentType string alone won't be effective.
  • We might mitigate this by instead evaluating cases where the Parent is a List with the Key of Attributes and the grandparent is a ContentTypeDescriptor.

Implementation

The following represents pseudocode for how this might be implemented:

public abstract class TopicRepositoryBase {

  private bool IsAttributeDescriptor(Topic topic) =>
    topic.Parent.ContentType == "List" &&
    topic.Parent.Key == "Attributes" && 
    topic.Parent.Parent.ContentType == "ContentTypeDescriptor";

  public void Save(Topic topic, bool isRecursive = false) {
    …
    if (IsAttributeDescriptor(topic) && topic.Id < 0) {
      topic.Parent.Parent.AttributeDescriptors = null;
    }
    …
  }

  public void Delete(Topic topic, bool isRecursive) {
    …
    if (IsAttributeDescriptor(topic)) {
      topic.Parent.Parent.AttributeDescriptors = null;
    }
  }

  public void Move(Topic topic, Topic target, Topic? sibling) {
    …
    if (topic.ContentType == "ContentTypeDescriptor" && topic.Parent != target) {
      topic.Parent.Parent.AttributeDescriptors = null;
    }
  }

}

When calling Topic.References.SetValue() with a null reference, any previous reciprocal relationships should be removed from the previous target topic's IncomingRelationships. There is logic in the TopicReferenceCollection.SetItem() method to handle this, but it isn't working correctly.

When calling ITopicRepository.Delete(), any topic references on the current or descendent topics should be deleted as part of the operation. While this will be done by e.g. the DeleteTopic stored procedure (if using the SqlTopicRepository), this should also be done in the in-memory topic graph in order to ensure that any topics they are pointing to no longer maintain a reference to the deleted topics via IncomingRelationships. There is already logic in place to handle this, but it throws an exception.

Cause

The Delete() method loops through all items in the Topic.References collection, and removes them, thus not only removing them from the current collection, but also any IncomingRelationships that point back to the current topic. By removing them, however, the collection being iterated against is modified, resulting in an exception being thrown.

Currently, if a PageGroup is requested, the [ValidateTopic] will redirect to the first child that IsVisible(). If there are no children that satisfy that condition, it will attempt to redirect to a null path, which throws an exception. Instead, we should prefer to return a 403 Forbidden response, which is similar to attempting to browse a directory without a default document in it.

The ITopicMappingService‘s use of Relationship for e.g., [Follow()] introduces a conflict with e.g. Topic.Relationships, leading to Relationships.Relationships.

Relatedly, the semantics of mapping these “relationships” are inconsistent, with [Follow(Relationships)] for annotating view models, CrawlRelationships for exposing that metadata on PropertyConfiguration, and relationships for the corresponding MapAsync() parameter.

To mitigate this, revisit the semantics and distinctions between competing concepts and nomenclature in the ITopicRepository‘s annotations.

Renames

[Relationship(RelationshipType)]

• [Relationship()] to [Collection()].
• RelationshipType to CollectionType.
• RelationshipsMap to AssociationsMap.
• PropertyConfiguration‘s RelationshipKey to CollectionKey.
• PropertyConfiguration‘s RelationshipType to CollectionType.

[Follow(Relationships)]

• [Follow()] to e.g. [Include()].
• PropertyConfiguration‘s CrawlRelationships to Include.
• Relationships enum to AssociationTypes.
• MapAsync(relationships) to MapAsync(associations).
• CacheEntry‘s e.g., GetMissingRelationships() to GetMissingAssociations().
• CacheEntry ‘s Relationships to Associations.
• IRelatedTopicBindingModel to IAssociatedTopicBindingModel.

When an existing topic is loaded (i.e., a topic with an Id), they should be not be marked as IsDirty(). This works if a new Topic has its Key or ContentType set (both of which are required parameters). If the Parent property is set, however, then the Topic is being reported as IsDirty(). This means it will saved as part of a recursive Save() even if no other changes have been made, which can have a significant performance impact on recursive saves, such as imports using the OnTopic Data Transfer library.

Background

Currently, ITopicRepository implementations rely on cached data in the TopicRepositoryBase's GetContentTypeDescriptors() method to validate metadata upon Save(). Notably, this is used by the SqlTopicRepository implementation to determine if an attribute should be stored as an indexed or an extended attribute.

Problem

This generally works fine. Problems are introduced, however, when the following conditions are true:

1. An entire topic graph is being saved (e.g., as part of an Import() using the OnTopic-Data-Exchange library), and
2. The topic graph includes new ContentTypeDescriptor or AttributeDescriptor instances not stored in the database.

Note: Issues #16 and #17 both seek to address a similar problem by updating the GetContentTypeDescriptors() cache when ContentTypeDescriptors or AttributeDescriptors are Save()d, Delete()d, or Move()d. But this doesn't address situations where a new Topic in a topic graph is being committed as part of a recursive Save() which will subsequently include new ContentTypeDescriptor or AttributeDescriptor instances.

Note: An obvious example of this is when bootstrapping a new database by importing a reference version of the OnTopic schema via a JSON file. How do you save the Root or Configuration before establishing the Container content type? This affects other scenarios as well, however.

Solution

This likely requires more consideration, but one option is to reactively update the GetContentTypeDescriptors() cache anytime the cache fails to deliver the a ContentTypeDescriptor or AttributeDescriptor by attempting to find that value in the current topic graph.

Note: The SqlTopicRepository doesn't maintain a local cached instance of the entire topic graph, as CachedTopicRepository does. As such, it can't do a fast local lookup on a canonical source. Still, the Topic being passed to Save() is presumably part of a larger topic graph, and thus new versions of content types may be able to be extracted from it. This would generally be expected, for example, in cases using the OnTopic-Data-Exchange library.

Implementation

Assuming the above solution makes sense, one possible implementation of this might look something like the following:

public class TopicRepositoryBase {
  …
  protected ContentTypeDescriptorCollection GetContentTypeDescriptors(Topic topic) =>
    GetContentTypeDescriptors(GetRootContentType(topic));

  private ContentTypeDescriptorCollection GetContentTypeDescriptors(ContentTypeDescriptor? topic) {
    // Centralized logic from current GetContentTypeDescriptors() method
  }

  private ContentTypeDescriptor GetRootContentType(Topic topic, string ) {
    while (topic.Parent == null) {
      topic = topic.Parent;
    }
    var configuration = topic.Children.GetTopic("Configuration");
    return configuration?.Children.GetTopic("ContentTypes") as ContentTypeDescriptor;
  }
  …
}

Note: It may be valuable to try generalizing and centralizing the GetRootContentType() as e.g. a GetTopicByUniqueKey() extension method so it can be reused elsewhere. This would be comparable to the CachedTopicRepository.GetTopic() method.

Currently, the CachedTopicRepository will load the entire Root topic and all descendants. It will not load any topics outside of the Root topic, nor can it be restricted to a single section of the topic graph.

Consider implementing functionality that allows the scope of the CachedTopicRepository to be restricted to a particular area. This would allow e.g. subdomains to load just a segment of the tree without needing access to the entire tree.

Considerations

• The Root:Configuration is only needed if there's write access, and can otherwise be handled via TopicRepository.
• The OnTopic Editor will need access to all topics, so editors can maintain the entire tree.
• In Dependency Injection, constructor parameters should be required, not optional.
• Should we ever allow topics to be created outside of Root? Or should these always be children of Root?

Proposal: CachedAreaTopicRepository

• Derives from CachedTopicRepository
• Exposes a required rootTopicKey constructor parameter
• Sets a protected string RootTopicKey property

Problem

Currently, whenever ITopicRepository.Save() is called, relationships are deleted (e.g., via the @DeleteRelationships parameter on the UpdateTopic stored procedure) and then recreated (via the PersistRelations stored procedure)—even if no relationships were modified.

Solution

Ideally, we'd track whether the relationships collection had been modified, as we already do with AttributeValueCollection.IsDirty(). That way, we can conditionally disable this, which would reduce the number of unnecessary (and expensive) database calls made when doing a recursive Save() on a large topic graph (e.g., during an Import() with the OnTopic Data Transfer library).

Nice to Have

When combined with AttributeValueCollection.IsDirty(), this would also potentially allow us to bypass the UpdateTopic entirely, which currently passes, at minimum, the full XML for @ExtendedAttributesXml, in order to evaluate if the extended attributes collection has changed.

Implementation

• NamedTopicCollection.IsDirty
• RelatedTopicCollection.IsDirty()
• RelatedTopicCollection.SetTopic(…, isDirty)
• Call SetTopic(…, isDirty) on SqlTopicRepository.Load()
• Disable NamedTopicCollection.IsDirty on SqlTopicRepository.Save()
• Bypass Save() if !RelatedTopicCollection.IsDirty() and !AttributeValueCollection.IsDirty()

Create an OnTopic AppSettings configuration store that can be used to store global settings such as (non-secret) API keys or site-wide preferences.

Current Approach

Today, customers store application settings in a variety of places:

• config files (such as web.config or appsettings.config)
• secrets.json files (in development) and Azure App Service Settings (in production)
• Individual Topic classes, if it's not a secret (see below)
• Startup, if it's not a secret

Some sites even use a combination of each of these.

Topic-Based Approach

Given the nature of OnTopic as a semi-structured data source, it makes sense for OnTopic sites to be configured via Topics. Today, this is done on a per Topic basic, typically as a subclass of Page. That's fine for one-off pages, such as a /Search page storing an API key, but it's impractical for values that need to be shared across multiple pages, as configuration settings are only available in that context.

Ideal Approach

Ideally, we'll instead approach this through something like the following:

• ContentTypeDescriptor: AppSettings (LookupList)
  • TopicList: Settings of AppSettingsItem (LookupListItem)
  • AllowedContentTypes: AppSettings (thus allowing namespacing)
• Root:Configuration:AppSettings as root store
• TopicAppSettings (IConfiguration) class for storing, retrieving settings
• TopicConfiguration class for mapping TopicAppSettings to a value object and passing it to e.g. AddTopicSupport().

This way, any custom application setting can easily be added to OnTopic without needing to create a custom ContentTypeDescriptor, and those application settings can easily be namespaced in case a particular context (such as /Search) has multiple settings.

The CachedTopicRepository and StubTopicRepository include code for validating that the version passed to the Load(topicId, version) method has occurred in the past. But the Topic.VersionHistory defaults to UTC, whereas the validation code defaults to the local timezone. This isn't an issue in a production environment since servers should have their timezone set to UTC anyway. But it can prevent topics from being rolled back on development machines.

The OnTopic Library has a rich collection of inline documentation in the form of XML Docs. This is currently used by Visual Studio for IntelliSense, but not much beyond that.

Ideally, this would be used to generate HTML documentation which would then be published publicly.

Open Questions

• Library: DocFX?
• Location: GitHub Pages?

Currently, the TopicMappingService exclusively maps properties, and requires an empty constructor. It would add flexibility, and especially for records, if constructor parameters could also be mapped.

Challenges

There are two primary challenges with this.

Double Mapping

If a property is mapped via the constructor, how do we prevent it from being double mapped? This is especially true with records, where the constructor parameter might be used to define a property.

Order of Operations

Currently, the object is initialized, immediately added to cache, and then mapped. This would require assembling a collection of mapped values first and assigning them in unison, instead of assigning them as they're evaluated, and only then adding the object to the cache.

Circular References

Extrapolated out, the above means that topic associations might be evaluated prior to the root object(s) being created. This will introduce problems with circular references since the original topic can't be mapped until (constructor) dependencies are mapped.

Considerations

Double Mapping

• Ignore mapping properties if they aren’t an IList and aren’t set to their default value—except that would likely introduce problems with properties initialized to a specific value.
• Require [DisableMapping] on the property itself to manually prevent this situation.
• Track mapped constructor parameters in a configuration object passed down to MapProperties(). This could even be on the TopicMappingCacheEntry. That would depend on the parameter names matching their target properties.

Circular References

The circular dependency issue could be addressed by limiting what annotations can be applied to parameters. Specifically, this could exclude:

• [Include()], so no associations are mapped,
• [MapToParent], since no parent object will be available.

Currently, associations always rely on the MapAsync(source, associations) overload when mapping associated topics for collections and topic references. A consequence of that is that they effectively mandate that that they are mapped to models using the {ContentType}TopicViewModel convention.

This can be expensive given that, often, associations are only needed for a couple of properties needed to render a link to that topic. In those cases, it would be preferred to map to a lightweight object that only contains the necessary metadata, such as Title, ShortTitle, WebPath (I.e., INavigable) and possibly ImageUri and Description.

Ideally, this would be accomplished by introducing a new attribute, such as [MapTo()] or [MappingType()] and/or even a new property of [Collection()] which which allows the association to specify what type it should be mapped to. This could improve performance on pages with a lot of associations, but which only need a small amount of data.

When SqlTopicRepository,Load() can't find a Topic, it throws a TopicNotFoundException. When CachedTopicRepository.Load() can't find a topic, it returns null. In fact, the signature for Load() on SqlTopicRepository returns a Topic, whereas the signature for Load() on CachedTopicRepository (and, incidentally, ITopicRepository) returns a Topic?.

There are good arguments for returning null vs. throwing an exception here. But the behavior and the return type nullability should be consistent—or, at least, clearly documented. That said, updating this behavior is a breaking change, and so this may need to wait until OnTopic 6.0.0.

Similar to attributes (see #45), topic references can be null when returned from the database. This happens when a previous topic reference was deleted; the new version will use a null value. Currently, topics with these null topic references are being treated as unresolved references.

Impact

This has two impacts.

1. When loading values as part of e.g. ITopicRepository.Refresh() or Rollback(), any existing in-memory topic reference values, if present, are not being deleted.

   Note: There may also be circumstances where this could result in those values being inadvertently persisted to the database—though, fortunately, this will generally be avoided by the fact that Save() doesn't include clean topic references when calling the UpdateReferences stored procedure.

2. More importantly, because the Topic.References collection will be set to !IsFullyLoaded, any topic references that are deleted will not be removed from the database when calling UpdateReferences.

   Note: This is due to a safeguard which prevents collections that Load() was not able to fully load from deleting the orphaned references on Save().

Cause

The SetReferences() extension method treats a null reference as an unresolved reference. But while a reference could be null because it was unresolved, it could also be null because it was explicitly deleted. The SetReferences() extension method should be updated to differentiate between null values and unresolved references.

Currently, an entire topic graph needs to be loaded—typically via the CachedTopicRepository—with all data, and references to in-memory objects, to ensure that the graph can be navigated and mapped via association properties, such as Parent, Children, References, and Relationships.

Consequences

This has a number of downsides:

• A large amount of data must be loaded up-front and/or persisted in a cache.
• We cannot evict stale or rarely referenced items from the cache.
• We cannot have any lazy loading, such that associations get loaded on demand.
• OnTopic is not optimized for very large data sets, or distributed read/write databases.
• OnTopic is not optimized for e.g. mobile devices that have limited memory.

Alternatives

An alternative would be to expose a light-weight Topic entity, similar to the TopicData interchange class, which tracks the unique keys of its associations, and then combine it with a service that retrieves those objects by key on demand—either from a data store, or from a cache.

Questions

• Would this be an entirely separate OnTopic library? Or could it integrate with the existing Topic entity? I assume the former.
• Would callers need to handle looking up associations? Or would the topic entity accept e.g. a repository as an argument to a method?
• Could this be implemented via an ITopicMappingService which operates off of an e.g. ITopic{Record?}Repository to dynamically query the database based on the needs of a record?

This is likely to be a massive change in how OnTopic operates. It may be better to accept its limitations and focus instead on its strengths, instead of reinventing much of the library to support scenarios other CMS concepts are better suited for.

Currently, SqlTopicRepository.Save() operations are slower than expected. There is room for a handful of optimizations to the associated stores procedures to help remedy that.

CreateTopic

• Use TABLOCK when creating new Topics record to allow parallelization of Save()

UpdateTopic

• Ignore @ExtendedAttributes if value isn’t set, thus allowing update only.
• Join ExtendedAttributes against ExtendedAttributeIndex and cast @ExtendedAttributes to XML instead of pulling value upfront and converting to VARCHAR.
• Join NullAttributes against AttributeIndex instead of performing nested join.
• Or, consider pulling latest attributes once upfront and joining against that for NullAttributes

PersistRelations

• Allow missing relationships to be removed via join, instead of deleting and recreating each time.
• Optionally disable to above to support adding relationships only

Background

Generally, it's preferred that attributes we explicitly defined as AttributeDescriptors and associated with a ContentTypeDescriptor in order to maintain and help enforce a reference schema. There are cases, however, where it may be useful to assign one-off attributes to a topic programmatically, even though it may not warrant the overhead of adding an AttributeDescriptor.

Problem

While this is currently supported via the SQL database, these values cannot be directly updated using the SqlTopicRepository. This is because attributes are currently validated against the ContentTypeDescriptor to determine if they should be stored as indexed or extended attributes in the database. Anything that falls outside of that is simply ignored; they aren't deleted, but changes and additions aren't persisted to the database, either.

Challenges

There are some challenges to this which would need to be addressed:

Data Storage

Should arbitrary attributes be treated as indexed or extended? Currently, in the database, they're usually treated as indexed, which makes them easy to find and update. But if they're over 255 characters, that will result in truncation. There are two main options:

1. Default to extended. This is the safest bet, but most arbitrary attributes will likely be smaller, and thus we're losing the benefits of them being indexed.
2. Conditionally store them based on length. In this case, most will be stored as indexed attributes, but it can lead to a confusing condition where the same attribute might be stored in different locations for different topics.

Deletions

How can arbitrary attribute deleted programmatically? Currently, deletions are handled via the UnmatchedAttributes collection which looks for mismatches against the ContentTypeDescriptor and submits those to the database. The best way to address this might be to include any empty or null attributes—even if they don't match the ContentTypeDescriptor—in the UnmatchedAttributes collection.

Note: This exposes a semantic ambiguity with the term "unmatched". In UnmatchedAttributes it means they're defined on the ContentTypeDescriptor but not the Topic. With arbitrary attributes, however, we end up with the opposite scenario.

When a topic is renamed (i.e., the Key is changed on an existing topic), and then that topic is saved, the ITopicRepository.TopicRenamed event should be raised. This is not currently happening.

Cause

This is caused by the order of operations in the base TopicRepository.Save() implementation. It resets the OriginalKey prior to evaluating the TopicRenamed event—but the OriginalKey is needed to evaluate if the topic has been renamed. As a result, this condition is never evaluated as true.

Some stored procedures are expected to return a ReturnCode output parameter. If this value is not present, stored procedures calling GetReturnCode() will throw an exception due to the referencing of the null Value field. This interferes with the existing functionality which defaults to returning -1 if a value cannot be parsed or identified. While stored procedures expected to return a ReturnCode should, in fact, return a ReturnCode, this also shouldn't cause an exception.

Note: If callers are validating the return code, then the fallback of -1 should alert them to the fact that there is a problem with the stored procedure. As such, this isn't needed for validation since the ReturnCode itself is intended to indicate a problem with the stored procedure or its execution.

Currently, deleted topics are permanently deleted, including all versions and associations. Instead, consider moving them to a hidden area so they can be recovered later.

Considerations

Key Conflicts

This would require renaming deleted topics to avoid key conflicts, and storing attributes necessary for resurrecting the topics (e.g., OriginalKey, OriginalParentID).

Storage

These could be stored in e.g., Root:Deleted. But then they will be fully loaded and cached by ITopicRepository.Load(). An alternative would be to store them in a Deleted root topic. This way, a list of deleted topic could be retrieved using e.g., ITopicRepository.GetDeletedTopics(), which only lists the e.g., unique key of each top-level deleted topic. This avoids issues such as special handling for Move(), Save(), and Rollback() in the OnTopic Editor. In this model, a RestoreTopic stored procedure would be similar to LoadTopic, but would additionally return incoming associations.

Associations

Associations with the deleted topic graph pose a problem. The best option is to delete the associations via versioning, so we still have a historical record of them. This would require, at minimum, a RestoreTopic stored procedure which deletes the last association to and from each restored topic—which will be its deletion. It will then need to return these associations so that they can be restored in memory.

Note: This record needs to be deleted so it’s not treated as a version moving forward.

Recursive Delete

Descendants should be kept as part of the deleted topic, as this

1. allows the entire tree to be restored, and
2. it isn’t possible to restore topics if their parent is missing.

API

If it weren’t for associations, this could all be handled superficially via the OnTopic Editor. Given the associations issues, though, this will likely require e.g.,

• ITopicRepository.Restore(uniqueKey)
• [dbo].[RestoreTopic]

It could also include an API for permanent deletion of (individual?) topics, but my inclination is to handle that manually, similar to version compression, at least for the initial version.

A previous commit, #885d9a6d, introduced a bypass for IsVisible() when adding topics to a collection via TopicMappingService. This introduced problems since some customers have a requirement to provide an index of topics that are hidden from the navigation using IsHidden. For that reason, this changed was rolled back in #cccf94c.

Ideally, we will reintroduce this with a more careful implementation. This should include:

• A ValidateTopic() function to centralize checks for null, IsDisabled, IsHidden, and List.
• Possibly refactoring of PopulateChildTopics() since it is no longer a primary entry point.
• A [AllowHidden] attribute on properties to bypass the check for IsHidden.