Created IHostSettingsService for Dependency Injection

[SkyeHoefling]

Related to #3985

Summary

This does not close the linked work item, but is required before I can submit a pull request.

Added an IHostController an abstraction of the HostController that will be used with Dependency Injection. The DotNetNuke.Library code for this is a small sample of what I have been thinking would be a good approach for migrating controllers and entities from DotNetNuke.Library to DotNetNuke.Abstractions.

[SkyeHoefling]

I am working on the unit tests for this one, but wanted to submit the PR so the business rules could get reviewed. There are a bunch of mocking issues with unit tests.

We should wait until #3988 is merged prior to merging this. I would like to rebase on that to ensure we have a good merge in the unit test files.

[mitchelsellers]

Initial pass review overall looks good on this, I think we need to be sure to coordinate AND focus on being sure that we have true performance & memory management done on DI channels before we recommend this route.

[mitchelsellers]

This should be public

[SkyeHoefling]

I extracted the new IConfigurationSetting interface into the abstractions project. It is not possible to have 2 identical methods in a class with different return types. The compiler reports errors as it doesn't know how to handle it. Explicit interface implementations like this is a way to solve it. Considering the new interface will only be used for Dependency Injection I believe this change is safe.

Once we move the deprecated method Dictionary<string, ConfigurationSetting> GetSettings() we can remove the explicit declaration and make this public

[SkyeHoefling]

Thanks for the review @mitchelsellers . I would like us to hold off on merging this until we complete #3988 as the unit testing work done in that PR will help this one. When you get a chance could you review that one, I am interested in your thoughts on the new interfaces I added.

[SkyeHoefling]

Now that #3988 is merged, this PR needs to be rebased on develop. There is a lot of unit test work from that PR that this PR will need to update to follow the pattern. I can now dedicate my time to working towards the code review feedback

[bdukes]

@ahoefling I've updated your branch with the rebase. Hope it helps!

[SkyeHoefling]

@bdukes thanks for the assist, managing the rebase gave me a great head start on fixing all the broken unit tests. I have updated the PR with the requested changes and fixed all the unit tests. It was quite satisfying updating unit test mocks to use the new IHostController vs the old way of using HostController.RegisterInstance.

Changes

• Fixed all failing unit tests, updated mocks
• Updated HostController.Instance to offload state management to the Dependency Injection Container
• Updated modified unit tests to following coding standards and guideline recommendations from stylecop

What's Next

I have checked all the builds locally and all the unit tests are passing. I have not ran the generated installer build yet, but plan to do it on tomorrow (8/20/2020) when I get a chance. If someone wants to grab the install file and report back, that would save me some time and be a big help!

[SkyeHoefling]

I just tested the installer and it appears to work without issue

[SkyeHoefling]

Since we are having conversations about migrating items such as HostController -> IHostSettingService to follow modern .NET conventions and best practices. Should we talk about the strategy for migrating Entity Models. In this PR there is the ConfigurationSetting, which maps to the HostSettings table. If I were building a greenfield application with that structure I would be creating an Entity class that maps 1:1 to that table. I would consider creating a business layer model or Data Transfer Object to build a bridge between the business layer and the database. If we want DNN to migrate towards an ultimate solution of Entity Framework Core, it would be useful to document what is appropriate.

Perhaps something like this

public class HostSettingEntity
{
    public int Id { get; set; }
    public string Key { get; set; }
    public string Value { get; set; }
    public bool IsSecured { get; set; }
}

public class HostSetting
{
    public string Key { get; set; }
    public string Value { get; set; }
}

In this example I created a HostSettingEntity which maps 1:1 to the columns in the database table. We don't need to suffix it with Entity, I did that to illustrate the differences between entity and business model. When the IHostSettingsService queries the data it would read the data from the IHostSettingEntity and then it would transfer the data to HostSettings which would be returned. In this scenario the IHostSettingsService abstracts some of the rules such as encryption.

[SkyeHoefling]

I thought about this a little bit more and wanted to share some additional thoughts. Right now we are dealing with the HostController which uses the ConfigurationSetting to store key-value-pairs. While it doesn't appear that the ConfigurationSetting is used outside of the HostSetting table the concept applies to many tables in DNN. We could extract the current interface that we already have IConfigurationSetting and then create concrete entities for each table that uses the IConfigurationSetting

public class HostSetting : IConfigurationSetting 
{
    public int Id { get; set; }

    // interface members
    public string Key { get; set; }
    public string Value { get; set; }
    public bool IsSecured { get; set; }
}

public class PortalSetting : IConfigurationSetting
{
    public int PortalSettingId { get; set; }
    public int PortalId { get; set; }

    // interface members
    public string Key { get; set; }
    public string Value { get; set; }
    public bool IsSecured { get; set; }
}

public class ModuleSetting : IConfigurationSetting
{
    public int ModuleId { get; set; }

    // interface members
    public string Key { get; set; }
    public string Value { get; set; }
    public bool IsSecured { get; set; }
}

public class RoleSetting : IConfigurationSetting
{
    public int RoleSettingId { get; set; }
    public int RoleId { get; set; }
    // interface members
    public string Key { get; set; }
    public string Value { get; set; }
    public bool IsSecured { get; set; }
}

// there are some omitted tables

This will generate a high level of interoperability with lower level data-access as we modernize this part of the platform. It would be very powerful if all of these tables used a very similar polymorphic Business Layer that only overrides the default rules if it needs to add additional properties besides the Primary Key. In the example of RoleSetting there is a RoleSettingId and a RoleId. In that case there would be an override in the child class to handle setting those properties

[bdukes]

I really like these thoughts and being able to have a discussion around them.

Regarding the entity vs. business model idea, in my mind the entity class would be an implementation detail which isn't exposed via any of the abstractions, so the details of it don't need to necessarily conform to some standard. Whatever intermediate classes are used by data access shouldn't impact the design of the interface. That said, if we find a model/approach we like, we might as well be consistent with it (while leaving room to deviate where it makes sense).

I think there's probably value in creating a single interface for the various types settings, for example I could imagine an API similar to PortalModuleBase.Settings that returns an ILookup<string, IConfigurationSetting>. We're already treating various settings similarly in ISettingsRepository<T> and ISettingsSerializer<T>. If we're going to do that, however, it probably makes sense to expose some sort of scope, so that given a collection of IConfigurationSetting instances, I can tell the difference between a module setting and a tab module setting that have the same key. Do you have thoughts on this @donker?

[SkyeHoefling]

@bdukes
Adding to your point that entities don't need to conform to a standard since it is an implementation detail. Perhaps we add a DotNetNuke.Abstractions.Models namespace. This would conform to all the public business models that we would be using and this is the folder/namespace that all the new interface models and enums would exist in. That will allow us to abstract things without needing to create a DotNetNuke.Data project right now.

This will also give us some more time to digest these ideas prior to making any strong opinions on a DotNetNuke.Data project, business models, and entities.

Currently the DNN Entities that exist in DotNetNuke.Library.Entities are really business models in my opinion. They don't give you direct access to the database and the controllers handle reading/writing to the database. With all of this in mind, I think it makes sense to still continue extracting interfaces for the abstraction project. I would argue that these should all exist in the DotNetNuke.Abstractions.Models folder I was mentioning above. Then when it is time to build a DotNetNuke.Data project we can have that used by the necessary business layer for reading/writing data.

[bdukes]

So, would you recommend moving DotNetNuke.Abstractions.Users.IUserInfo to DotNetNuke.Abstractions.Models.IUserInfo? I think I would lean more towards keeping it with the rest of the Users namespace. @mitchelsellers thoughts? Or am I misunderstanding your idea, @ahoefling? Thanks!

[mitchelsellers]

I would tend to agree with @bdukes on this one, I would really like to try and keep like elements together, so I think that putting any sort of models into a separate namespace would be confusing, and additionally would clutter that namespace greatly in the future.

[SkyeHoefling]

@bdukes @mitchelsellers
This makes sense, I made the recommendation as a convention I have seen in many MVC and MVVM apps in the .NET ecosystem. A downfall of a models folder is when you get 20+ objects in that namespace. As you pointed out the grouping makes it easier to find things.

I'm thinking of moving IConfigurationSetting from

DotNetNuke.Abstractions.IConfigurationSetting

to

DotNetNuke.Abstractions.Application.IConfigurationSetting

I think this would follow our guidelines as it is a model used at the application level. Our current pattern appears to be keeping service related interfaces at the root level and models in the folders. Do you all have any thoughts on this?

[mitchelsellers]

I really like the use of "Application" to hold those larger level items, honestly, in some ways the IHostController might be good as well to go there.

Might we get a quick call setup @ahoefling and @bdukes might be faster to just chat through this a bit?

[bdukes]

I have a little time this morning (but @ahoefling just messaged me that he was switching projects, so he may not have time).

I agree that IHostSettingsService makes sense to go into the Application namespace.

My question with IConfigurationSetting is whether it's really going to only apply to host settings, or if we'll reuse it for other types of settings. If it's only for host settings, let's call it DotNetNuke.Abstractions.Application.IHostSetting, otherwise DotNetNuke.Abstrations.Settings.IConfigurationSetting (or without .Settings in the namespace if we can't come up with more settings-specific interfaces we'll want).

[SkyeHoefling]

I have no problem moving

DotNetNuke.Abstractions.IHostSettingService

to

DotNetNuke.Abstractions.Application.IHostSettingService

I originally moved it to the root namespace as a place to put all the services. Considering the service is focused on "Application" level business rules it does make sense to put it back. (I think it was originally in there when I first submitted the PR and I moved it out to try it this way).

I don't have time for a call today unfortunately, but I can continue the dialogue on github

[SkyeHoefling]

@bdukes
I really like the settings namespace

DotNetNuke.Abstrations.Settings.IConfigurationSetting

I would argue that it should keep the prefix of Setting because it would be confused with the IConfiguration object which reads the AppSettings from the appsettings.json or web.config

[mitchelsellers]

I agree with the above and like the .Settings namespace as well

[SkyeHoefling]

Perfect! I'll make the following changes to the PR later today

Move: IHostSettingService -> into the DotNetNuke.Abstractions.Application namespace
Move: IConfigurationSetting -> into the DotNetNuke.Abstractions.Settings namespace

I'll do an audit of our changes and make sure I have no other questions. Once I push the update I'll request a re-review from both of you (@bdukes and @mitchelsellers). This should give us an opportunity to make sure there are no confusions or last minute questions on the change

[SkyeHoefling]

• Updated the IHostSettingsService and IConfigurationSetting with the new namespace recommendation
• Rebased branch on develop branch
• Requested re-review from @mitchelsellers and @bdukes

[valadas]

Is this ready to merge @mitchelsellers @bdukes ? And if so, in 9.7.1 or we hold it for 9.8.0 ?

[mitchelsellers]

@bdukes @ahoefling thoughts?

I think it is ok, as it isn't breaking

[valadas]

@mitchelsellers sounds, good, merging...

[SkyeHoefling]

I think we have good test coverage on this and it doesn't introduce breaking changes. 🎉