Feature: Map constructor parameters #35
Comments
JeremyCaney
added
Area: Mapping
JeremyCaney
added
Status 2: Scheduled
|
Topic associations—including topic references and relationship collections—could both be supported so long as they don’t create a circular reference. One way to handle this would be to track the This could even be done with the existing
|
|
Relationships introduce some challenges in terms of initializing the collection. If it’s a concrete type with an empty constructor, that’s no problem. If it’s an |
JeremyCaney
added
Status 0: Discussion
|
Not overriding null values isn’t ideal for avoiding the double-binding issue. Notably, as raised in the initial description, this risks overriding the initial value if it’s not set to A more sophisticated approach is to track the attribute names, and to bypass This could optionally be stored as a property on |
The `ItemConfiguration` serves the same purpose as the `PropertyConfiguration`, except that it is not specific to `PropertyInfo`, and will also work with e.g. `MethodInfo`, `ConstructorInfo`—and, notable for our purposes, `ParameterInfo` (which doesn't otherwise share a base `MemberInfo` class with the rest of these). This will help facilitate constructor mapping (#35) , which relies on mapping `ParameterInfo` objects instead of `PropertyInfo` objects.
The `IsInitializing` property tracks that a topic is currently _in the process_ of being constructed. Historically, this hasn't been necessary since construction is expected to be a fast process. As we move toward implementing #35, however, construction potentially becomes a time-consuming operation. More importantly, while circular references are permitted with properties—due to the ability to cache the references—this isn't possible with constructors, since the object won't yet be available to cache. As such, the `IsInitializing` property allows us to track whether a circular reference has occurred.
Previously, the `SetProperty()` called out to a number of helper methods—such as `SetCompatibleProperty()`, `SetScalarValue()`, and SetTopicReferenceAsync()`—which not only retrieved the value from `Topic`, but _also_ set the value on the target property. As we move toward #35, we want to update this to prefer "get" semantics, where each helper function is exclusively responsible for _retrieving_ the value, and then `SetPropertyValue()` is exclusively responsible for _setting_ the value on the property. This will allow us, eventually, to introduce an e.g. `SetParameterValue()` method which will do the same thing for the constructor parameters. To assist with this, a lot of the common logic was moved from `SetPropertyValue()` into `GetValue()`. Some of the logic needs to remain in `SetPropertyValue()`, however; for instance, `SetPropertyValue()` shouldn't attempt to set collection property, and it needs to make sure that the target property is, in fact, settable to the source type.
Previously, the `SetCollectionValue()` assumed that the target collection is a concrete type with an empty constructor. If it wasn't already set, it attempts to initialize it as the current type. Ideally, this should support return types that are interfaces as well, such as `IList<>` or its derivatives. To support this, I've introduced a new `InitializeCollection()` method which provides a bit more intelligence here. There remain limitations. This still doesn't support e.g. `IEnumerable<>`, derivatives of `IList<>`, or abstract collection types. But it will, at least, attempt to initialize `IList<>` types. It also offers a central place where we can expand this logic in the future, if we want to. Finally, this also helps pave the way toward #35, which will always need to be able to construct new lists if they occur as constructor parameters. That will ideally need to support `IEnumerable<>` as well, but we'll revisit that later.
Previously, the `MapAsync<>()` required that `T` implement `new()`—i.e., an empty constructor—since that is all it knew how to create. With the introduction of constructor mapping (#35, 3b34265), this limitation no longer exists. `MapAsync<>()` can thus be updated to utilize the `MapAsync(topic, type…)` overload (3b34265). Technically, this is a breaking change in that all implementations of the `ITopicMappingService` will need to be updated to drop the `new()` constraint. As with the previous `ITopicRepository.Load()` update (71552ae), however, we have high confidence that there aren't any implementations of the `ITopicMappingService` outside of the core library, and are thus deploying this as a minor update, instead of waiting for OnTopic 6.0.0, as prescribed by semantic versioning.
This is consistent with other keyed collections. This normally isn't a concern, but becomes one with the introduction of mapped constructor parameters (#35) since the constructor parameters will typically be `camelCase`, whereas the `KeyValuesPair.Key` will typically be `PascalCase`.
While mapped properties permit circular references due to caching, this isn't possible with circular references when mapping constructor parameters, since the source object won't yet have been created—and, thus, won't yet have been cached. In this case, a `TopicMappingException` is expected. This unit test verifies that is the case. This concludes the testing for mapped constructor parameters (#35).
Introduces the ability to map constructor parameters, and not just properties (#35). This allows more sophisticated view models, as well as `record` types using the constructor-only syntax. To support this, the `TopicMappingService` was refactored to use a new `ItemConfiguration` (88e06a7) instead of a `PropertyConfiguration` (88e06a7) so that the helper methods could operate off of either `ParameterInfo` or `PropertyInfo`. Those same methods were updated to use "get" semantics instead of "set" semantics (304525d), thus separating out how they are set to either `SetPropertyAsync()` (304525d) or `GetParameterAsync()` (142d765). With that, the constructor mapping logic was added to the previously introduced `MapAsync(topic, type…)` overload (3b34265), which was first introduced as part of the `[MapAs()]` support (61de3bf). Finally, the code was updated to make this the single source of view model construction in the `TopicMappingService` (ad34a65). This logic is validated via a pair of unit tests (8b3f14e, 4ac3567) using a new `ConstructedTopicViewModel` (3dcbdd6). As part of this update, I also refactored the `MemberDispatcher` to separate the (de)serialization of attribute values to value types into a new `AttributeValueConverter` library (84a2758), and updated the `MappedTopicCache` to have a more focused interface (60ee7d7, 5b3a59c) and to enforce business logic related to instantiating constructors (8f86392, cb41728, b33b60c).
|
To support this, the As part of this update, I also refactored the |
JeremyCaney
added
Status 5: Complete
JeremyCaney
changed the title
OnTopic 5.1.0 is a minor release which introduces support for mapping constructor parameters (#35) and defining what model to use when mapping associations via the `[MapAs()]` attribute (#41). Primarily, however, it is focused on bug fixes, and resolves a number of priority issues, such as an exception which occurs when deleting topics with associations (#47), topics with deleted references being treated as _unresolved_ (#46), and the inability to move a first child to another parent (#76). Finally, it also includes a number of improvements, such as checking type compatibility before mapping an association (#83), migrating the unit tests to xUnit.net (#66), and establishing integration tests for ASP.NET Core (#39). For a full rollup of new features, improvements, and bug fixes, see Pull Request #85.
Currently, the
TopicMappingServiceexclusively 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
IListand 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 theTopicMappingCacheEntry. 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.The text was updated successfully, but these errors were encountered: