Release/5.2.0 #108
Merged
Release/5.2.0 #108
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The `MemberDispatcher` was written to work with `TypeMemberInfoCollection` as well as direct calls to the .NET Reflection libraries. The introduction of `TypeAccessor` (f246997) and `MemberAccessor` (eb7fc4a) offers a faster, more efficient model, but necessitates that all requests for getting and setting members go through `MemberAccessor`. Those tasks will be handled in future updates. As a first effort at achieving this, I've retrofitted the `MemberDispatcher` to work with `TypeAccessor` via the `TypeAccessorCache`, while maintaining the previous interface for backward compatibility. This isn't optimal as, ideally, we want to get rid of the `(Type)MemberInfoCollection` and migrate the `MemberDispatcher` to a set of extension methods off of `TypeAccessor`. As such, this just provides the first, immediate step at integration. Much of this is pretty basic modifications to the underlying calls. The only major change is to `GetMembers()` since a) it operates off of a `T`, whereas `TypeAccessor` operates off of the `MemberTypes` enum, b) it supports constructors, whereas `TypeAccessor` does not, and c) it returns the (legacy) `MemberInfoCollection<>`, which ideally we'll want to deprecate.
In the unexpected case that a value being set to a property is not compatible with the target type, an `ArgumentException` is thrown by the reflection library. When using a delegate, the `InvalidCastException` is instead thrown. As this is a low-level, unexpected error that reflect a design flaw in e.g. a view model, it's not a concern that this changed (i.e., it's not worth rethrowing it as a `ArgumentException`). As such, I'm just updating the test to comply with the expectation.
This maintains consistency with the previous `MemberInfoCollection`, which also ensured uniqueness when adding members. Typically this isn't an issue, except in cases where e.g. a `new` member is added, hiding an inherited member. In that case, we only want the first result returned—which should be the `new` version. This is an outside use case that's not a primary concern, but we obviously want to avoid duplicate key exceptions being thrown in this scenario.
Currently, the `MemberDispatcher` is set locally as a cache on `TopicPropertyDispatcher`, `TopicMappingService, and `ReverseTopicMappingService`. As part of that, callers may optionally set an `_attributeFlag` via the primary constructor, thus restricting updates to members that have a given flag. Currently, this is only used by the `TopicPropertyDispatcher`. As we prepare to move `MemberDispatcher` to a set of extension methods off of `TypeAccessor`, we need to find another way of relaying this information to these calls. This not only adds those parameters, but also introduces optional overloads so that they can be specified as generic types. Note: For consistency, this is currently available on all `Has*()` methods and `Get*()` methods. It is only currently _used_ on `HasSettableProperty()`, however. As a result, we may want to revisit this in the future to determine if it's really necessary.
In a previous update, I retrofitted `MemberDispatcher` to operate off of `TypeAccessor` (and the underlying `MemberDispatcher`) instead of relying on `(Type)MemberInfoCollection` and going directly to the .NET Reflection classes when necessary. With this done, it makes more sense for these methods to be treated as extensions of the `TypeMember` class—since they all rely upon it—instead of instantiating a new class that goes through the `TypeAccessorMemberCache`. This also provides a more intuitive naming scheme, since the legacy `MemberDispatcher` was a bit ambiguous with the new `MemberAccessor`. The continues to maintain a separation between properties and methods. As this is no longer needed in the underlying library, we may want to revisit this at some point, and especially since these aren't currently validated as properties or methods. We can revisit this later.
This results in slightly shorter, cleaner code for situations where the type is known at compile time.
Previously, the `TopicMappingService`, `ReverseTopicMappingService`, and ``TopicPropertyDispatcher` all relied on the `MemberDispatcher`. As that has been refactored into the new `TypeAccessor` extensions, these classes all required updates to comply with the new interface. For the most part, this was a pretty straight-forward migration. There remain a few cases where this doesn't work as well, and we may want to revisit later—such as where we're passing `MemberAccessor` objects inside of `TopicMappingService`, but the `TypeAccessor` needs to be retrieved since that's where the mapping-specific `Set*Value()` methods are located. These work, but they may call for revisiting the interface to avoid this scenario.
With the `MemberDispatcher` refactored as extensions off of `TypeAccessor`, the legacy `TypeMemberInfoCollectionTest`—which, confusingly, primarily evaluated the `MemberDispatcher` class—has been renamed to `TypeAccessorExtensionsTest`, and updated to work off of that new class. As part of this, I removed the `GetMember()` tests as this method is no longer used, and doesn't necessitate tests. Most of these migrations were straight-forward updates to the new syntax, and have no functional impact. Much of the change in code is actually just rewrapping the XML docs.
The `CanRead` and `CanWrite` property names are consistent with the underlying `PropertyInfo` properties which serve the same purpose. This also frees up the identifiers `IsGettable()` and `IsSettable()` for a more intelligent method which can factor in conversion and, optionally, target type.
…hods Previously, the `SetMethodValue()` and `SetPropertyValue()` _implicitly_ converted string values to compatible types (such as `int`, `bool`, and `DateTime`). This can potentially lead to unexpected scenarios if unexpected. While this is the expected—and long established—behavior, I decided to make it opt-in since it's something that should be a conscious decision by a developer to support. This also allows the `SetPropertyValue()` and `SetMethodValue()` methods to optionally operate as faster, less-presumptuous, lower level operations, if needed. That will be supported in a subsequent update which moves most of this logic down to `MemberAccessor`'s `SetValue()` method.
Previously, the business logic for handling compatibility and conversions was handled at the `TypeAccessorExtensions` level—e.g., through `SetPropertyValue()`, `GetMethodValue()`, &c. This left the `MemberAccessor`'s `SetValue()` and `GetValue()` to be fast, low-level implementations without much business logic or validation. In practice, however, all requests in the application require this, and forcing them to go through the `TypeAccessor` extensions was confusing—and especially when there might already be an instance of a `MemberAccessor` available. Further, with a little bit of optimization, this business logic can be made fairly performant by e.g. checking type compatibility before attempting a conversion, and excluding `null`s from conversion, instead checking `IsNullable`. As part of this, I've incorporated the new `allowConversion` paramter (5cd1dcb) into `SetValue()`.
With the migration of the business logic back down to the lower-level `MemberAccessor`'s `SetValue()` method (a43434d), we are now able to throw the previously expected `ArgumentException` (via `Contract.Assume()`) for the rare case where the object doesn't match the `MemberAccessor.MemberInfo.DeclaringType`. This undoes one of the updates made as part of 56792e9.
This was missed as part of 5cd1dcb.
Now that the business logic for `SetPropertyValue()` has been moved to the underlying `MemberAccessor.SetValue()` method, the `TopicMappingService`'s `SetPropertyAsync()` method can call `SetValue()` directly, since it already has a reference to `MemberAccessor`. This remains a bit sloppy as it still needs to get a reference to the `TypeAccessor` from the `TypeAccessorCache` in order to validate `HasSettableProperty()`. To fix that, we need to move the core of this logic down to `MemberAccessor`. That will come in a subsequent update.
Since the `SetValue()` method now handles a lot of the business logic, we can simplify the logic for `SetPropertyAsync()`'s calls by no longer needing the validate compatibility first via the `TypeAccessor.HasSettableProperty()`. All of this will just be repeated in `SetValue()` anyway—and, by design, will fail silently if it's not compatible—so we can just to a basic check against `CanWrite` to confirm it's an appropriate path. (Even this last check isn't strictly necessary; this could be an `else` statement without any further conditions. But as these are quick checks, it helps reinforce what values won't get written at this higher level, and is thus more intuitive to read.)
`HasGettableProperty()`, `HasGettableMethod()`, `HasSettableProperty()`, and `HasSettableMethod()` specify `Property` or `Method` as part of their signature. This isn't strictly necessary, as the underlying `GetValue()` and `SetValue()` on `MemberAccessor` will work against _either_ a property _or_ a method. As such, it remains a bit unclear—outside of semantic precision—whether we still need to maintain these overloads. Until then, however, they should validate what they claim to validate. Previously, if a gettable method name was passed to `HasGettableProperty()`, for instance, it would return true, since there was an `IsGettable` `MemberAccessor` with that name. With this update, it explicitly verifies that it is, in fact, a property or a method.
In a previous update, I updated the `Has*ettable*()` methods (e.g., `HasSettableProperty()` to validate that the member type is, in fact, a property or a method (44ead0e). I've extended this validation to the `*et*Value()` methods (e.g., `SetPropertyValue()`) as well. This isn't technically required, but so long as we're maintaining the superficial difference between properties and methods, this distinction should be enforced. Note that the setter will throw an exception if it's not found, whereas the getter will fail silently.
The ability to validate the `targetType` and `attributeFlag` used to be required since the `GetPropertyValue()` and `GetMethodValue()` previously called `HasGettableProperty()` and `HasGettableMethod()` respectively. Those were removed as part of the refactoring to the underlying `MemberAccessor.GetValue()` (a43434d), which now recreates just the aspects of the validation logic which are necessary for retrieving a value. If it is a concern that the value be compatible with a particular `targetType` or `attributeFlag`, then callers can (and should) continue to call `HasGettableProperty()` and `HasGettableMethod()` to ensure compatibility. Otherwise, however, this is not the responsibility of `GetPropertyValue()` and `GetMethodValue()`, and doesn't affect their functionality.
The `TypeMemberInfoCollection` was used as a collection of `MemberInfoCollection<>` as a means of caching type information. This has been effectively replaced by the `TypeAccessorCache`, and is no longer used. As part of this, I also removed two vestigial unit tests that were still evaluating its functionality.
The `MemberInfoCollection` offered a convenience shortcut to `MemberInfoCollection<T>` without needing to specify a `MemberInfo` type. This was never actually used—and with the focus being on e.g. `TypeAccessorCache`, it's unlikely it'll be used in the future. For now, we're keeping the strongly typed `MemberInfoCollection<T>`, which can be used if there's a need for such a collection. (Though we'll likely revisit whether this is needed as well in the future.) As part of this, I also fixed a couple of XML Doc references that were _intended_ to reference the generic version, but inadvertently references _this_ version.
The legacy `MemberInfoCollection<T>` which `MemberDispatcher` relied upon included all members, including constructors, as part of its collection. With the migration to `MemberAccessor`, however, the `TypeAccessor` that replaces `MemberInfoCollection<T>` doesn't track constructors. This is because it doesn't have enough information to create delegates for constructors, nor do the semantics of `MemberAccessor` really work with constructors (e.g., `GetValue()`, `CanWrite`, &c.). As part of the migration from `MemberDispatcher`, the `TypeAccessor` inherited a `GetMembers<T>()` method that is exclusively used for getting constructors via the legacy `MemberInfoCollection<T>`. Since this is the only current need, a more elegant approach is to simply offer a purpose-built convenience method to retrieve the _primary_ constructor (currently defined as the first public, instance constructor).
The `GetMembers<T>()` overload was inherited from `MemberDispatcher`. The `T` type parameter represented a child of `MemberInfo`, and was a way of sorting a `MemberInfoCollection<T>`. With the introduction of the `MemberAccessor` wrapper for `MemberInfo`, this way of filtering was deprioritized in favor of using `MemberTypes`, as is done on the primary `GetMembers()` methods on `TypeAccessor`. Given this, `GetMembers<T>()` shouldn't be needed anymore. The one case where it was still used was with the `TopicMappingService`'s support for mapping constructors. When `MemberDispatcher` was migrated to `TypeAccessor` extensions, a hack was put in place to support this specific scenario. Now, we have a purpose-built `GetPrimaryConstructor()` to satisfy this condition, and can thus get rid of this entire `GetMembers<T>()` overload, instead preferring the primary overloads from `TypeAccessor`.
With the renaming of `CanRead` and `CanWrite` (ee5db7d), the indentation of this code block became malformed. Oops.
In a previous commit, I got rid of the `GetMembers<T>()` extension (68f582d) as it was attempting—poorly!—to satisfy legacy requirements based on an organization of `MemberInfo` instances grouped by `MemberInfoCollection<T>` collections. There remains a need to extract `MemberInfo` instances from a `TypeAccessor`, however, and optionally filter by type. This is primarily used by the `BindingModelValidator`, which needs an enumeration of `PropertyInfo` objects. As part of this, I've retrofitted the two areas that relied on this to use the new overload. This satisfies the basic condition of the legacy extension method, but with much cleaner code, and without a dependency on the legacy `MemberInfoCollection<T>`.
The `GetMembers_Property_ReturnsProperties` unit test, which now evaluates the new `GetMembers<T>()` overload (c85de0a), was relying on `MemberInfoCollection<T>` not as a necessary subject of testing, but as a convenience for evaluating the results of a collection. This is mitigated by using XUnit's `Assert.Contains()` method instead.
With the last few commits, there's no longer any dependency on the legacy `MemberInfoCollection<T>` class. It does offer a potential _convenience_ as a keyed collection of `MemberInfo` objects, but as this isn't strictly necessary anymore—and, indeed, we're no longer actively using it—it doesn't make sense to keep it around, and especially as an internal class. Should we, at a later date, rediscover a need for something like this, it's easy enough to resurrect—or simply recreate based on the precise needs at that time. Until then, this is the last vestigial element of the legacy `MemberDispatcher` and `TypeMemberInfoCollection` system that's now been replaced by `TypeAccessor` and `MemberAccessor`. As part of this, removed a few legacy unit tests that were part of the `TypeAccessor` extensions test (a location they didn't really fit into anymore).
In preparation for merging `TypeAccessorExtensions` into `TypeAccessor` and `TypeAccessorExtensionsTest` into `TyepAccessorTest`, delete both `TypeAccessor` and `TypeAccessorTest`. These will be recreated by renaming `TypeAccessorExtensions(Test)` to `TypeAccessor(Test)` as a merged file.
This is more consistent with the order in e.g. `TypeAccessor`, `MemberAccessor`, and other classes, with getters (e.g., `GetPropertyValue()`) before setters (e.g., `SetPropertyValue()`); hassers (e.g., `HasSettableProperty()`) before actions (e.g., `SetPropertyValue()`); and properties (e.g., `GetPropertyValue()`) before methods (e.g., `GetMethodValue()`). This will make the merge into `TypeAccessor` more legible.
Previously, more opinionated members of `TypeAccessor`—i.e., which did conversion, or handling of type checking—were placed in extension methods, allowing `TypeAccessor` to remain focused on just providing a basic interface to `MemberAccessor` instances. This allowed low-level access through `TypeAccessor`/`MemberAccessor`, with higher-level, and more cautious access through `TypeAccessor` extensions. In practice, nearly _all_ access was going through `TypeAccessor` extensions. And, recently, much of that type checking and conversion were moved down to `MemberAccessor`. As such, this distinction no longer makes sense. To support that, `TypeAccessorExtensions` is being merged into `TypeAccessor`.
Fixed a couple of minor errors in the XML Doc metadata which will cause problems when compiling the XML documentation for use with e.g. DocFx, as proposed in #29.
Previously, some view model defaults were set on both the property's auto-value, as well as the `AttributeDictionary` constructor. This ensures that the constructor instead falls back to the auto-value. While this isn't strictly required, it helps avoid scenarios where the default value is updated in one place, but accidentally overlooked in the other.
Previously, some view model defaults were set on both the property's auto-value, as well as the `AttributeDictionary` constructor. This ensures that the constructor instead falls back to the auto-value. While this isn't strictly required, it helps avoid scenarios where the default value is updated in one place, but accidentally overlooked in the other.
When applying an attribute, the `Attribute` extension is implied, and thus doesn't need to be included. While I was at it, I moved the `[Guid()]` attribute to the bottom, below `[InternalsVisibleTo()]`, for consistency with other `AssymblyInfo` classes.
This was previously applied to most scenarios (e6bd2b7), but missed in this scenario.
This is more descriptive, consistent with other similar methods, and is aligned with the existing comment header.
Made a number of minor code updates in preparation for the final deployment. This includes simplifying attribute names and catching an errant `==` where `is` is preferred.
This will be the first release of 2022, so the copyright date should reflect that.
Updated `GitVersion` from 5.8.1 to 5.10.3. As this is a build-only dependency, it won't affect downstream consumers.
Updated the Microsoft .NET Test SDK from 17.0.0 to 17.2.0.
Updated ASP.NET Core test framework from 6.0.1 to 6.0.6.
Updated xUnit test runner from 2.4.3 to 2.4.5. As this is a test-only dependency, it won't affect downstream consumers.
Updated Visual Studio Validation from 17.0.34 to 17.0.53. As this is a design-time dependency, it won't affect downstream consumers.
Updated all NuGet packages that don't affect downstream consumers. These include build dependencies, such as `GitVersion`, test dependencies, such as the Microsoft .NET Test SDK and `xUnit`, as well as design-time dependencies, such as the Visual Studio Validation utility. While I was at it, I also bumped the copyright date to 2022, in preparation for the first release of the year.
This ensures that a topic with the same key as another topic cannot exist within the same parent scope. This is already enforced by the Topic Library itself, but not by the SQL schema. This helps prevent inadvertent conflicts from being introduced when manually updating the SQL database, which would result in an exception when loading the data via an `ITopicRepository` implementation. This resolves #106.
This ensures that a topic with the same key as another topic cannot exist within the same parent scope. This is already enforced by the Topic Library itself, but not by the SQL schema. This helps prevent inadvertent conflicts from being introduced when manually updating the SQL database, which would result in an exception when loading the data via an `ITopicRepository` implementation. This resolves #106.
The name `AttributeDictionary` is more consistent with our overall naming conventions. However, it conflicts with the out-of-the-box `AttributeDictionary` class in the `Microsoft.AspNetCore.Mvc.ViewFeatures` namespace. As this is commonly introduced as a global namespace for model libraries, this introduces a potential conflict which requires fully-qualifying classes from either the corresponding Microsoft or the OnTopic namespace. To mitigate that, I'm renaming `AttributeDictionary` to `AttributeValueDictionary`. This is similar to the out-of-the-box `RouteValueDictionary` which refers to route parameter keys and values, but uses the name `RouteValue`, not `RouteParameter` or even `Route`. That isn't an entirely satisfying precedent as it's pretty anomalous within the BCL, but it's also a popular enough class that the convention doesn't feel _entirely_ arbitrary, and especially by comparison to other options.
With the renaming of the `AttributeDictionary` to the `AttributeValueDictionary` (38c54c3), the test project's `AttributeDictionaryConstructorTopicViewModel` is now renamed to `AttributeValueDictionaryConstructorTopicViewModel`. This class name was already a mouthful, but now it _really_ is!
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.
Previously, `OnTopic.Attributes` couldn't be (easily) introduced as a global using directive (c9f6b17, #93) because of a naming conflict with `AttributeDictionary`, which existed in both `OnTopic.Attributes` and `Microsoft.AspNetCore.Mvc.ViewFeatures`. With `AttributeDictionary` renamed to `AttributeValueDictionary` (0b62fe0, #107), this can be reintroduced.
With `OnTopic.Attributes` added as a global using directive for both `OnTopic` and `OnTopic.Tests` (95b8276), we can now remove all local using directives for it.
With `AttributeDictionary` renamed to `AttributeValueDictionary` (#107) the potential naming conflict with the out-of-the-box `AttributeValueDictionary` is removed, and we can thus (re)introduce `OnTopic.Attributes` as a global using directive, and remove the local using directives across `OnTopic` and `OnTopic.Tests` (07d46ec).
Renamed `AttributeValueDictionary` back to `AttributeDictionary`, thus reverting 38c54c3. It seems this decision was premature, and we can mitigate the naming conflicts without renaming the class, since the out-of-the-box `ViewFeatures` namespace is almost exclusively used in views, and the `AttributeDictionary` is almost exclusively used in view models. This effectively undoes #107.
Renamed `AttributeValueDictionary` back to `AttributeDictionary`, thus reverting 38c54c3. This decision was made prematurely, and we can mitigate the naming conflicts without renaming the class. This is because the out-of-the-box `ViewFeatures` namespace is almost exclusively used in views, and the `AttributeDictionary` is almost exclusively used in view models. This effectively undoes #107, while still allowing `OnTopic.Attributes` to be added as a global using directive, where appropriate.
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.
This pull request was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
OnTopic 5.2.0 introduces support for response caching (#89), a new
ErrorControllerfor HTTP error codes (#91), and improvements toITopicMappingServiceperformance via optionalAttributeDictionaryconstructors (#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).Features
TopicControllerresponse caching #89 Integrated response caching via OnTopic-based cache profilesErrorControllerto simplify ASP.NET Core error handling #91 Including fallback to parent HTTP codesIDictionary<>constructor for manual deserialization #99 Support forAttributeDictionaryconstructors on view models to improve performance ofITopicMappingServiceImprovements
CreateDelegate()#90 Includes significant refactoring of theOnTopic.Internal.Reflectionclasses around theTypeAccessor(Cache), instead of(Type)MemberInfoCollection(reference)ErrorController: Bypass for static files #101 Enables lightweight response codes for embedded resources instead of a friendly error pageIsCompatible: CheckMemberAccessoragainst expectedTopic#102 Cache compatible member associations between view models and strongly typed topics (e.g.,ContentTypeDescriptor)Topicstable can't have a duplicate key for an attribute #106 Provides database-level enforcement via a SQL constraintCode Changes
GitVersionand Microsoft's test libraries