Mapping: Support `IDictionary<>` constructor for manual deserialization
Closed this issue · 3 comments
Optionally provide support for a constructor that accepts an IDictionary<string, string> of key, value pairs representing the values of the Topic.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 an IDictionary<string, stirng> parameter. In addition, the private MapAsync() overload that creates the initial object will need to be updated to conditionally pass mapAssociationsOnly to SetProperty(), possibly by setting a property on the MappedTopicCacheEntry. Alternatively, the check for the IDictionary<> can be handled in MapAsync(), simplifying how mapAssociationsOnly is 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 tracking AssociationTypes and 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.
This was implemented through a specialized class, AttributeDictionary (31ee953), which ensures the correct signature is used (e.g., no ambiguity between IDictionary<string, string> and the correct IDictionary<string, string?>), but also offers a number of strongly typed GetValue() methods for requesting a value if the key exists, and otherwise returning null, thus simplifying the implementation (8e09fc5). In addition, I exposed an AttributeDictionary constructor for every model in OnTopic.ViewModels; these may not be necessary, and especially for light-weight models, but having it in place ensures that new models can derive from the out-of-the-box models, while relying on the base() constructor(s) to take care of setting their own properties (f137ca1). This is covered by a new unit test (6b8ed99).
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 AttributeDictionary (a781356), but it's not compensated by savings in bypassing e.g. SetPropertyAsync(). Accurately determining whether there will be a successful mapping will likely cost more than it would save, so instead we established a threshold of five source attributes to enable the AttributeDictionary (e1abb1d), based on basic load tests for evaluating difference scenarios (2e157e5). This accounts for the fact that we expect most Topics in standard configurations to have 2-3 topics that won't be mapped (i.e., Title, LastModified, and LastModifiedBy).
Likewise, the threshold expects five or more IsConvertible properties on the target model—though, in practice, this isn't a guarantee that there will be a match since the ITopicViewModel interface itself has more than five IsConvertible properties that aren't expected to map to any attributes (e.g., Id, Key, ContentType, WebPath). In the future, we may rely on the caching afforded by TypeAccessor to more intelligently assess how many of these exist, and set the threshold accordingly. In the meanwhile, however, we have reasonable confidence that a model with fewer than five properties probably won't benefit from the use of AttributeDictionary, so this provides a good floor.
That all said, in practice, these all pertain to edge cases, typically with regards to associations using lightweight [MapAs()] models, since most PageTopicViewModels will always have more than five source attributes and target properties.
There is already a class named AttributeDictionary in Microsoft.AspNetCore.Mvc.ViewFeatures namespace (reference). Unfortunately, the ViewFeatures namespace is often appropriate to introduce as a global namespace for view models. That introduces a naming conflict in the exact domain we expect to be using our AttributeDictionary.
The AttributeDictionary naming convention makes perfect sense for our needs. But to disambiguate it, we may want to rename it to AttributeValueDictionary. This isn't ideal, as it's inconsistent with most other attribute-related naming conventions in OnTopic as well as dictionary names in .NET. That said, there is 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. That's not especially convincing, but as a popular class it at least provides some familiarity or justification for this convention, instead of it being entirely arbitrary.