Mapping: Support IDictionary<> constructor for manual deserialization
#99
Comments
JeremyCaney
added
Area: Mapping
Ideally, models will optionally accept a dictionary of attributes so that scalar properties can be quickly and easily assigned without relying on reflection, in order to improve performance. The `AttributeDictionary` class will not only provide a friendlier signature than e.g. `IDictionary<string, string>`, but will also provide a friendlier `GetValue()` method for getting a value if there's a match, and otherwise returning `null`. This will aid in supporting Feature #99.
The `AsAttributeDictionary()` will convert the current `AttributeCollection` into a lighter weight `AttributeDictionary`. Filters out attributes which have special getter logic in `Topic`. Optionally, the `inheritFromBase` method allows values to be inherited from derived topics. This will aid in supporting #99.
Updated the private `MapAsync()` overload to identify if the model exposes a constructor with the `AttributeDictionary` (31ee953) and, if it does, populate it with `AsAttributeDictionary()` (a781356). In this case, bypass other constructor processing, and then bypass `SetPropertyAsync()` for all attributes mapped via the constructor. This, of course, relies on the implementer of the constructor to correctly set the properties. This is the core implementation of Issue #99.
To aid in retrieving strongly types values from the `AttributeDictionary`, I've added convenience methods for e.g. `GetBooleanValue()`, `GetInteger()`, &c., each corresponding to the convertible types on the `AttributeValueConverter`. This will make it easier to implement logic in view models. In addition, since `AttributeValueConverter` is currently `internal`, this allows external libraries—including `OnTopic.ViewModels`—to benefit from the the conversion logic without having to have direct access to the library. This aids in the implementation of Feature #99.
The models in `OnTopic.ViewModels` are intended to be the standard reference models used for out-of-the-box implementations of OnTopic, and should be optimized for readability and performance, while also demonstrating best practices. To this end, I've added support for the new `AttributeDictionary` constructor to each of these, thus allowing them to bypass reflection for standard scalar properties. Equally importantly, while not all implementors will choose to create `AttributeDictionary` constructors for their own view models, we want to support those who do, and do so by basing their implementation on the `OnTopic.ViewModels` library. By ensuring there's an `AttributeDictionary` constructor on every model—even models that aren't used very often and might not otherwise benefit from this functionality—we make it possible for any model in `OnTopic.ViewModels` to be subclassed and to have an `AttributeDictionary` constructor defined on that derived type without needing to implement properties of all base types. This should improve both performance of the `OnTopic.ViewModels` when used directly. while offering flexibility for implementors to fully adopt `AttributeDictionary` constructors on their own derived view models, should they choose to do so. This completes the basic implementation of the `AttributeDictionary` support for the `TopicMappingService` as required by Feature #99. That said, we still need to extend our unit tests to cover `AttributeDictionary` and the new `MapAsync()` functionality.
…onary` As per Feature #99, we now support mapping models that have a constructor with a single `AttributeDictionary` parameter (72ca032). In these cases, scalar properties in the `AttributeCollection` will be set via constructor, thus bypassing unnecessary calls to reflection. The new unit test confirms that this works by ensuring that such a constructor correctly sets a `Subtitle` property, without interfering with the standard handling for `Title` and `LastModified` (which are handled via compatible properties).
Added support for constructing models using a lightweight `AttributeDictionary`, as an alternative to using reflection for basic scalar properties. This puts control over basic construction in the hands of the model developer, and should be faster for scenarios where there are a lot of scalar properties. Enabled creation of an `AttributeDictionary` via new `AsAttributeDictionary()` method on the heavier duty `AttributeCollection` class, and integrated support for constructing models which accept an `AttributeDictionary` into `TopicMappingService`. This satisfies the functional requirements #99, though we'll still need to introduce unit tests for this behavior.
Introduced unit tests for the `AttributeDictionary` support (f137ca1), thus completing the requirements for Feature #99. This includes unit tests for the `AttributeDictionary` class (11894b7), the `AsAttributeDictionary()` method (d4b77c1), and the constructor support for `AttributeDictionary` within `TopicMappingService` (6b8ed99).
Established a threshold of attributes (of five) before invoking the `AttributeDictionary` constructor during the `TopicMappingService`, if available (per Feature #99). There is overhead to establishing a new `AttributeDictionary` via, especially, `AttributeCollection.AsAttributeDictionary(true)`. This pays off after just one or two mapped attributes. However, most topics have multiple attributes which will not be mapped (e.g., `LastModifiedBy`) or are excluded (e.g., `LastModified`, `Title`), and thus shouldn't be counted. As a result, setting the threshold at five generally achieves a useful heuristic around improving performance without worrying about the overhead. As part of this, I introduced "unit tests" which can be configured to aid in identifying where this threshold exists, and whether or not updates to the `TopicMappingService` are performant or not.
|
This was implemented through a specialized class, |
|
As part of the initial testing of this feature, it was determined that this is actually slower if there aren't any attributes that map (e1abb1d), as it adds overhead of establishing a new Likewise, the threshold expects five or more That all said, in practice, these all pertain to edge cases, typically with regards to associations using lightweight |
JeremyCaney
added
Status 5: Complete
The `AttributeDictionaryConstructorTopicViewModel` provides a more specialized model for evaluating the `AttributeDictionary` mapping capabilities added to the `TopicMappingService` (see #99). This builds off of the existing `PageTopicViewModel` by including both a `MappedProperty` (which will get mapped by the constructor) and an `UnmappedProperty` (which will not get mapped by the constructor). This allows us to confirm that the `AttributeDictionary` constructor did, in fact, get called, since otherwise the `UnmappedProperty` would get picked up by `SetPropertyAsync()`. As part of this, I also extended the attributes in the test to ensure that it isn't falling under the recently introduced threshold for honoring the `AttributeDictionary` constructor (2e157e5).
The `AttributeDictionary` constructor (#99) adds some overhead to mapping operations if there aren't enough mapped attributes. To mitigate this, a threshold was previously established of five or more attributes or convertible properties (2e157e5). This was meant to account for the fact that we expect upwards of five compatible properties on the base `TopicViewModel`. With the introduction of the new `MaybeCompatible` property (6662e18, 359eaae, 1e2dc53), however, we can use a more intelligent threshold by only evaluating non-compatible properties. In practice, some compatible properties will _still_ get mapped by the `AttributeDictionary` handling and, thus, potentially benefit from this (e.g., `View`, `IsHidden`). Mapping those will be comparatively fast, however, since they're compatible properties, so measuring off of the incompatible members remains a useful heuristic.
Introduced a new property, `MaybeCompatible`, to the `ItemMetadata` which attempts to determine if an item has a corresponding member on the `Topic` class (or a derivative with a matching name) and, if so, whether it is either a compatible or convertible data type. This can't guarantee compatibility since, potentially, a `Topic` may be derived, and the source topic type may not match the target model naming convention. Since derived topics are very rarely used by either the OnTopic library or customer implementations, however, this isn't a major concern. In the meanwhile, this allows us to limit more careful analysis of compatible and convertible types in the `TopicMappingService` by relying on `MaybeCompatible` to determine if further evaluation makes sense. This allows us to improve the threshold used for the `AttributeDictionary` constructor (Feature #99), thus evaluating whether there are any members that do _not_ correspond to properties on `Topic`. This is more intelligent than the previous approach, which simply evaluated the number of properties, since the base `TopicViewModel` class (if it's used) exceeded the threshold due to properties that map to `Topic` members, but not necessarily attributes (e.g., `Id`, `Key`, `ContentType`, `WebPath`). As a result, I was able to reduce the threshold from five to three, while actually making it more selective. Finally, the `TypeAccessor` was updated to exclude members inherited from `object` or added dynamically by the C# compiler to `record` objects. This helps reduce false positives in the above thresholds while also greatly reducing the number of cached properties that will never be used.
|
There is already a class named The |
When we first introduced the `AttributeValueDictionary` (#99), we inadvertently introduced a naming conflict with the `AttributeDictionary` in the `Microsoft.AspNetCore.Mvc.ViewFeatures` namespace. Unfortunately, the `ViewFeatures` namespace is often appropriate to introduce as a global namespace for view models, resulting in a naming conflict. To disambiguate it, I have renamed it to `AttributeValueDictionary`. This isn't ideal, but there's at least some precedence for this in the ASP.NET Core MVC `RouteValueDictionary`, which is a mapping of route parameter names to route parameter values. As part of this, I also renamed associated tests and test fixtures.
The `AttributeDictionary` class (OnTopicCMS/OnTopic-Library#99) introduces a conflict with the `AttributeDictionary` in `Microsoft.AspNetCore.Mvc.ViewFeatures`, which is often used as a namespace in model libraries (OnTopicCMS/OnTopic-Library#107). To disambiguate this, it's been renamed to `AttributeValueDictionary`. The editor's view models have been updated to utilize this new naming convention.
The `AttributeDictionary` class (OnTopicCMS/OnTopic-Library#99) introduces a conflict with the `AttributeDictionary` in `Microsoft.AspNetCore.Mvc.ViewFeatures`, which is often used as a namespace in model libraries (OnTopicCMS/OnTopic-Library#107). To disambiguate this, it's been renamed to `AttributeValueDictionary` in OnTopic Library 5.2.0 Alpha 255 (b243853). The editor's view models have been updated to utilize this new naming convention (1ca4eb4).
Introduced a description, example, and list of considerations for using the `AttributeDictionary` constructor in view models for use with the `ITopicMappingService`, thus documenting #99.
…elop Introduced a description, example, and list of considerations for using the `AttributeDictionary` constructor in view models for use with the `ITopicMappingService`, thus documenting #99.
OnTopic 5.2.0 introduces support for response caching (#89), a new `ErrorController` for HTTP error codes (#91), and improvements to `ITopicMappingService` performance via optional `AttributeDictionary` constructors (#99), updates to the internal reflection libraries (#90), and caching of compatible properties (#102). In addition, it also includes a number of internal code improvements, such as adoption of new C# 10 capabilities (#96), including global using directives (#93).
Optionally provide support for a constructor that accepts an
IDictionary<string, string>ofkey,valuepairs representing the values of theTopic.Attributes. This would allow view models to manually bind their primary properties. This would require more development effort upfront, but would potentially be faster than relying on reflection, and especially for commonly used objects.Implementation
Most of this logic can be implemented in the
GetParameterAsync()method by offering special handling for anIDictionary<string, stirng>parameter. In addition, the privateMapAsync()overload that creates the initial object will need to be updated to conditionally passmapAssociationsOnlytoSetProperty(), possibly by setting a property on theMappedTopicCacheEntry. Alternatively, the check for theIDictionary<>can be handled inMapAsync(), simplifying howmapAssociationsOnlyis set.Limitations
This would simply support attribute binding, and would leave complex properties (i.e., topic references, parent mappings) and collections (i.e., relationships) to the mapping service—though, of course, each of those associations may include their own
IDictionary<>constructor. This is because trackingAssociationTypesand mapping an entire topic graph adds a potentially prohibitive level of of complication for implementers, while also adding a non-trivial amount of logic to the model objects. By contrast, supporting scalar properties addresses the vast majority of needs with minimal implementation overhead.The text was updated successfully, but these errors were encountered: