ontopiccms / ontopic-library Goto Github PK
View Code? Open in Web Editor NEW.NET/C# client library for Ignia's OnTopic CMS.
.NET/C# client library for Ignia's OnTopic CMS.
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
TABLOCK
when creating new Topics
record to allow parallelization of Save()
UpdateTopic
@ExtendedAttributes
if value isn’t set, thus allowing update only.ExtendedAttributes
against ExtendedAttributeIndex
and cast @ExtendedAttributes
to XML
instead of pulling value upfront and converting to VARCHAR
.NullAttributes
against AttributeIndex
instead of performing nested join.NullAttributes
PersistRelations
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.
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.
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, 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.
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.
In particular, the following are areas that are difficult to meaningfully unit test but trivial to setup integration tests for:
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.
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.
/Views/ContentTypes/{ContentType}[.{View}].cshtml
/Views/{ContentType}/{ContentType|View}.cshtml
/Views/{Controller}/{Action}.cshtml
/Views/Shared/{View}.cshtml
/{rootTopic}/{path}
/{area}/{path}
/{area?}/{controller}/{action?}/{id?}
/{area}/{action?}/{id?}
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.
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 offalse
isn't being added to callers of repositories that don't define it.
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 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.
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.
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.
There are two primary challenges with this.
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.
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.
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.
IList
and aren’t set to their default value—except that would likely introduce problems with properties initialized to a specific value.[DisableMapping]
on the property itself to manually prevent this situation.MapProperties()
. This could even be on the TopicMappingCacheEntry
. That would depend on the parameter names matching their target properties.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.Generally, it's preferred that attributes we explicitly defined as AttributeDescriptor
s 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
.
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.
There are some challenges to this which would need to be addressed:
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:
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 theContentTypeDescriptor
but not theTopic
. With arbitrary attributes, however, we end up with the opposite scenario.
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
.
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.
Parent
reference would need to be excluded from a Rollback()
.TopicIndex
to combine the latest ParentId
with [dbo].[Topics]
data. Or could Parent
just be assembled just like other topic references?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.
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.
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.
This generally works fine. Problems are introduced, however, when the following conditions are true:
Import()
using the OnTopic-Data-Exchange library), andContentTypeDescriptor
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 whenContentTypeDescriptor
s orAttributeDescriptor
s areSave()
d,Delete()
d, orMove()
d. But this doesn't address situations where a newTopic
in a topic graph is being committed as part of a recursiveSave()
which will subsequently include newContentTypeDescriptor
orAttributeDescriptor
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
orConfiguration
before establishing theContainer
content type? This affects other scenarios as well, however.
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, asCachedTopicRepository
does. As such, it can't do a fast local lookup on a canonical source. Still, theTopic
being passed toSave()
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.
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. aGetTopicByUniqueKey()
extension method so it can be reused elsewhere. This would be comparable to theCachedTopicRepository.GetTopic()
method.
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.
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.
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
.
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.
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.
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:
ValidateTopic()
function to centralize checks for null
, IsDisabled
, IsHidden
, and List
.PopulateChildTopics()
since it is no longer a primary entry point.[AllowHidden]
attribute on properties to bypass the check for IsHidden
.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.
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).
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.
NamedTopicCollection.IsDirty
RelatedTopicCollection.IsDirty()
RelatedTopicCollection.SetTopic(…, isDirty)
SetTopic(…, isDirty)
on SqlTopicRepository.Load()
NamedTopicCollection.IsDirty
on SqlTopicRepository.Save()
Save()
if !RelatedTopicCollection.IsDirty()
and !AttributeValueCollection.IsDirty()
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.
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.
This would require renaming deleted topics to avoid key conflicts, and storing attributes necessary for resurrecting the topics (e.g., OriginalKey
, OriginalParentID
).
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 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.
Descendants should be kept as part of the deleted topic, as this
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.
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.
Each ContentTypeDescriptor
object cached as part of the TopicRepositoryBase.GetContentTypeDescriptors()
method has a locally cached rollup of AttributeDescriptor
s.
Note: This is very similar to #16, which pertains to
ContentTypeDescriptor
updates instead ofAttributeDescriptor
updates; there may be overlap in the implementation.
This is satisfactory for most cases. Issues are introduced, however, when adding or removing AttributeDescriptor
s, 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.
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 AttributeDescriptor
s) or Move()
(for ContentTypeDescriptor
s).
Note: This may also need to be recursive, since
AttributeDescriptors
inheritAttributeDescriptor
values from parentContentTypeDescriptor
s.
AttributeDescriptor
s, so trying to identify what is an AttributeDescriptor
based on the ContentType
string alone won't be effective.
Parent
is a List
with the Key
of Attributes
and the grandparent is a ContentTypeDescriptor
.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;
}
}
}
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.
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, 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.)
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.
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.
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.
[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)*
Note: The
TopicCollection
andReadOnlyTopicCollection
cannot be marked as[Obsolete]
as they have been subsequently replaced with non-keyed collections with the same names.
AttributeTypeDescriptor
removed; AttributeDescriptor
should be used instead (7b00274)RelatedTopicCollection
replaced with TopicMultiMap
(66ccd9d)
NamedTopicCollection
replaced with KeyValuesPair<T>
(66ccd9d)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)
TopicRepository
INavigationViewModel.CurrentKey
replaced with CurrentWebPath
(8fd4d80, 31475d2)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:
ITopicViewModel.IsHidden
removed, since hidden associations are not mapped (68a4e83)TopicFactory.Create()
parameter order updated, with id
moved after parent
(69caaa2)ITopicRepository
(and implementations)
INavigationTopicViewModel<T>.IsSelected(uniqueKey)
parameter renamed to webPath
(7a6e7ef)The following cannot be included because the new types are implicitly compatible with the old types, thus introducing an ambiguous reference.
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.
This has two impacts.
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 theUpdateReferences
stored procedure.
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 onSave()
.
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, 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.
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.
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.
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
).
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.
[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
.Create an OnTopic AppSettings
configuration store that can be used to store global settings such as (non-secret) API keys or site-wide preferences.
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)Topic
classes, if it's not a secret (see below)Startup
, if it's not a secretSome sites even use a combination of each of these.
Given the nature of OnTopic as a semi-structured data source, it makes sense for OnTopic sites to be configured via Topic
s. 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.
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 storeTopicAppSettings
(IConfiguration
) class for storing, retrieving settingsTopicConfiguration
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.
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.
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, ContentTypeDescriptor
s are composed of AttributeDescriptor
s, 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.
AttributeDescriptor
to e.g., FieldDescriptor
.(Extended)Attributes
to e.g., ScalarValues
.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.
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 theReturnCode
itself is intended to indicate a problem with the stored procedure or its execution.
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.
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.
The TopicViewResultExecutor
contains logic for identifying the (optional) View
from the following locations:
Accept
headersView
propertyContentType
propertyThe 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, 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.
(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.
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 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.
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.
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 AttributeValue
s that don't start with LastModified
.
There are a couple of challenges to this that are worth considering.
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.
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.
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.
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).
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
.
This has a number of downsides:
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.
OnTopic
library? Or could it integrate with the existing Topic
entity? I assume the former.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, 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.
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.
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()
).
AttributeValue.IsExtendedAttribute
AttributeValueCollection.SetValue(…, isExtendedAttribute)
SetValue(…, isExtendedAttribute)
from SqlTopicRepository.Load()
TopicRepositoryBase.IsExtendedAttributeMismatch()
IsExtendedAttributeMismatch()
into GetAttributes()
IsExtendedAttributeMismatch()
into Save()
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.
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
).
The TopicRepositoryBase
class has a default implementation of GetContentTypeDescriptors()
which loads the ContentTypeDescriptor
s via Load()
and then caches them locally. This collection is not modified until the application is reset, even if ContentTypeDescriptor
s are added or removed.
This works fine when making most day-to-day edits. Issues are introduced, however, when adding or removing ContentTypeDescriptor
s, 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.
To mitigate this, ideally we should update the GetContentTypeDescriptors()
cache any time
Save()
is called with a new ContentTypeDescriptor
, orDelete()
is called with an existing ContentTypeDescriptor
.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, 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.
Root:Configuration
is only needed if there's write access, and can otherwise be handled via TopicRepository
.Root
? Or should these always be children of Root
?CachedAreaTopicRepository
CachedTopicRepository
rootTopicKey
constructor parameterprotected string RootTopicKey
propertyA declarative, efficient, and flexible JavaScript library for building user interfaces.
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
An Open Source Machine Learning Framework for Everyone
The Web framework for perfectionists with deadlines.
A PHP framework for web artisans
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
Some thing interesting about web. New door for the world.
A server is a program made to process requests and deliver data to clients.
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
Some thing interesting about visualization, use data art
Some thing interesting about game, make everyone happy.
We are working to build community through open source technology. NB: members must have two-factor auth.
Open source projects and samples from Microsoft.
Google ❤️ Open Source for everyone.
Alibaba Open Source for everyone
Data-Driven Documents codes.
China tencent open source team.