Add ISerializationManager for Dependency Injection

[SkyeHoefling]

Fixes: #4086
Related: #4071

Summary

Creates ISerializationManager abstraction that is then implemented by the SerializationController. The new interface is then registered as a transient service with the Dependency Injection container. I believe transient registration makes sense because it does not require any state and meets the definition of stateless service.

@bdukes suggested we add this as PR feedback in #4071

[bdukes]

I forget where we landed on naming, what's the distinction between Manager and Service?

[SkyeHoefling]

I decided to call this a Manager instead of a Service because it isn't really running DNN business rules. I am open to changing the name to ISerializer or IDnnSerializer, but I don't think we should call this a Service

[bdukes]

@ahoefling I added a couple of new methods to ISerializationManager, SerializeValue and DeserializeValue, which should be more useful in #4071.

The only remaining question in my mind is whether we need to copy ISettingsSerializer into DotNetNuke.Abstractions, or if we're okay to leave it where it is (in DotNetNuke.Entities.Modules.Settings). This is the type that custom serializers must implement.

[SkyeHoefling]

@bdukes I reviewed the changes you made and appreciate the new helper methods. I think those will be very valuable.

I am not very familliar with this code in DNN and am curious about the different layers of abstractions we have. It is kind of confusing to me and was wondering if we should flatten it to make it easer for developers to consume. Right now this PR creates a new ISerializationManager many of the APIs depend on using the string serializer which is a object name that gets loaded using reflection.

Instead of just performing a lift and shift, I propose we adjust this API to be ready for the future. We should not be using reflection to instantiate controls or use the custom DNN built reflection cache.

New API Recommendation

I propose we adjust the ISerializer to function similar to the ILogger<T> from Microsoft.Extensions.Logger library. If you are unfamiliar it constructs an ILogger object that is dependent on a generic type. For example if you have a HomeController and you inject ILogger<HomeController> it will properly instantiate an ILogger implementation using the HomeController as the logger reference.

I believe this structure will work very well for our custom ISerializer implementations. Following that pattern the new API structure will be very simple

public interface IValueSerializer<T>
{
    string Serialize(T value);
    T Deserialize(string serializedObject);
}

public interface ISerializer
{
    string Serialize<T>(T value);
    T Deserialize<T>(string serializedObject);
}

Today the logic in the SerializationController is kind of confusing as it violates the Single Responsbility Rule by handling serialization and also acts as a Factory to resolve ISettingsSerializer implementations. These are passed into the string serializer in many of the API calls. To better explain what I am trying to solve let me break down some of the rules as they are in the SerializationController:

Null Serializer

• If the serializer string is null it attempts to use logic within the SerializationController

Valid Serializer

• If the serializer is a valid object, it will use reflection to instantiate the object and invoke the API methods. This is slow and prone to runtime errors. As well as confusing stack traces when something goes wrong.

Currently the DNN library code only uses 1 implementation of the SettingsSerializer that I am aware of which is the ComplextTypeSerializer.

To get this all to work under my new proposal we would need to update the ComplexTypeSerializer to implement the IValueSerializer<T> and then register it with the Dependency Injection Container. This will allow the ISerializer implementation to resolve the generic and use that. If the IValueSerializer<T> is not registered with the Dependency Injection Container it will use the ActivatorUtilities to try and instantiate it. This will allow this code to "just work" with little understandings by developers consuming it.

Here are some example implementations that I am thinking

public class DnnSerializer : ISerializer
{
    protected ISerializer Serializer { get; }
   
    public DnnSerializer(ISerializer<DefaultSerializer> serializer)
    {
        this.Serializer = serializer;
    }

    public string Serialize<T>(T object) => this.Serializer.Serialize(object);

    public T Deserialize<T>(string value) => this.Serializer.Deserialize(value);
}

public class DnnSerializer<T> : DnnSerializer, ISerializer<T>
{
    public DnnSerializer(IServiceProvider container)
        : base(ActivatorUtilities.GetServiceOrCreateInstance<T>(container)
    {
    }
}

public class DefaultSerializer : IValueSerializer<object>
{
    public string Serialize<T>(T object)
    {
        // invokes the current serialization behavior
    }

    public T Deserialize<T>(string value)
    {
        // invokes the current deserialization behavior
    }
}

// This is here to demonstrate a custom serializer
public class ComplexTypeSerializer : IValueSerializer<ComplexType>
{
    public string Serialize<T>(T object)
    {
        // invokes the current serialization behavior
    }

    public T Deserialize<T>(string value)
    {
        // invokes the current deserialization behavior
    }
}

Usage

Below is an example of how this new API will be used in a DnnApiController via constructor injection.

public class HomeController : DnnApiController
{
    protected ISerializer Serializer { get; }

    public HomeController(ISerializer<ComplexTypeSerializer> serializer)
    {
        this.Serializer = serializer;
    }
}

If the code needs to just use the default settings the user can just inject the ISerializer

public class HomeController : DnnApiController
{
    protected ISerializer Serializer { get; }

    public HomeController(ISerializer serializer)
    {
        this.Serializer = serializer;
    }
}

In this proposal I do not like how we have 2 interfaces that are identical in the contract

• ISerializer
• IValueSerializer

I created the 2 different interfaces to ensure we have 1 that implements the actual logic where the other is a facade around the implementation details. I am open to discussing this proposal further to see if we can come to an agreement on something to ultimate publish to this PR.

The end goal is I want to be able to implement an easy interface for serialization and just start using that. Right now as implemented this abstraction becomes very difficult to change or use a custom serializaiton.

cc: @bdukes @mitchelsellers

[bdukes]

I definitely like the approach of adjusting how these pieces fit together. I was also uncomfortable as I was doing the minimal lift-and-shift.

The current SerializationController is primarily used via the SettingsRepository, which is driven by attributes (it's also used to parse Prompt commands, but that doesn't support custom serializers currently). The attribute exposes a string Serializer property which is used to specify a custom serializer to use when saving & loading settings (we should deprecate this and add a Type property, there's no good reason for the property to be a string). The intention here is for the settings infrastructure to be extended by third parties. The ComplexTypeSerializer is just for unit testing this feature, it's not used in production.

Ultimately what this means is that the type of value and type of serializer is often not known at compile time. So while having a generic API is going to work well in some of our new scenarios and will be nice and ergonomic moving forward, there are other scenarios that a generic API doesn't solve. So far as I'm aware, we'll need to be able to support passing Type objects through, in addition to providing type parameters.

I'm trying to design an API which supports what we support now, and somewhere there's going to be some gross reflection code. Should we only support generic overloads in the interface, and the existing APIs can access those via reflection?

public interface ISerializer<T>
{
    string Serialize(T value);
    T Deserialize(string serializedValue);
}

// NOTE: Update DotNetNuke.Entities.Modules.Settings.ISettingsSerializer<T> to inherit from ISerializer<T>

public interface ISerializationManager
{
    string SerializeValue<T>(T value);
    string SerializeValue<T>(T value, ISerializer<T> serializer);

    string SerializeProperty<T>(T containerObject, PropertyInfo property);
    string SerializeProperty<TContainer, TProperty>(TContainer containerObject, PropertyInfo property);

    T DeserializeValue<T>(string serializedValue);
    T DeserializeValue<T>(string serializedValue, ISerializer<T> serializer);

    void DeserializeProperty<T>(T containerObject, PropertyInfo property, string serializedValue);
    void DeserializeProperty<TContainer, TProperty>(TContainer containerObject, PropertyInfo property, string serializedValue, ISerializer<TProperty> serializer);
}

The big piece that helps me to keep in mind is that someone is implementing ISettingsSerializer<ComplexType> right now in order to allow the framework to serialize and deserialize their custom type. They don't need a way to do the serializing/deserializing themselves, they need a way to tell the framework about their custom implementation. Right now, that might look like this:

        public class ModulesSettings
        {
            [ModuleSetting(Serializer = "DotNetNuke.Tests.Core.Entities.Modules.Settings.ComplexTypeSerializer,DotNetNuke.Tests.Core")]
            public ComplexType ComplexProperty { get; set; } = new ComplexType(20, 25);
        }

This should be changed to this:

        public class ModulesSettings
        {
            [ModuleSetting(SerializerType = typeof(ComplexTypeSerializer)]
            public ComplexType ComplexProperty { get; set; } = new ComplexType(20, 25);
        }

And, I suppose, we should switch to using ActivatorUtilities.GetServiceOrCreateInstance to instantiate that type within the SerializationController. But both of those changes can happen in a separate PR, IMO.

[SkyeHoefling]

Thanks for explaining how the ModuleSettingAttribute is used in DNN with the custom serializer as I was not aware. Would the goal be to support a combination of both your proposal and my proposal. This would allow us to have a more modern API structure and allow developers to specify custom Serializer implementations on the ModuleSettings?

[bdukes]

What's the difference between your proposal and my proposal? The major changes that I see are the addition of an overload taking ISerializer<T> and the addition of SerializeProperty and DeserializerProperty. Do you think we shouldn't have those? Or is my proposal missing something that yours includes?

[SkyeHoefling]

I would like to remove the idea of the ISerializationManager and the SerializationController. I think the APIs defined in these are very confusing. The developer should have an interface that is as simple as this

string Serialize<T>(T object);
T Deserialize<T>(string data);

The complicated part is our current implementation for attribute based serialization isn't really a serializer but an IValueConverter. I am coming to this conclusion because it is a 1-way serialization class that is designed to work with a very specific object type.

I see value for all of these things to work together, but I do not think we should tightly couple the ISerializer<T> or what I am calling the IValueConverter<T> with the ISerializer I proposed.

[bdukes]

I like the trimmed down nature of this proposal, but I have a hard time seeing what it looks like for our current settings serialization/deserialization to make use of it. Are you proposing that it wouldn't make use of this interface? Or would use it for properties that don't define a custom serializer/value converter?

Would it look something like this?

foreach (var (property, attribute) in GetProperties(settingsObject))
{
    object propertyValue = GetPropertyValue(settingsObject, property);
    Type propertyType = property.PropertyType;

    string settingValue;
    var customValueConverter = GetValueConverter(attribute.Serializer);
    if (customValueConverter != null)
    {
        settingValue = InvokeConverter(customValueConverter, nameof(IValueConverter.ToString), propertyType, propertyType);
    }
    else
    {
        settingValue = InvokeSerializer(this.serializer, nameof(ISerializer.Serialize), propertyType, propertyType);
    }

    this.moduleController.UpdateModuleSetting(GetSettingName(property, attribute), settingValue);
}

Where the two Invoke methods would be doing reflection to call their corresponding methods with the correct type, and this.serializer is an ISerializer instance resolved via DI.

I think you're right that this trimmed down interface is the right approach for third party devs. But we also need an implementation that serves the needs of the core, and, ideally, centralizes all of the reflection in one place. So, maybe ISerializer is the public-facing implementation in DI, and SerializationController is the creaky, old, Reflection-y implementation used by the core (making use of ISerlializer where it can). Does that sound right/better to you?

[bdukes]

@mitchelsellers @donker any thoughts? @ahoefling am I still missing what you're trying to get at?

[donker]

I'm going to study this more in depth as (module) settings (de-)serialization is a pet peeve of mine. I'd love to see serialization lifted to a higher level, but it's paramount that the settings serialization remains intact. BTW, we keep referring to module settings, but it also implements TabModule settings and Portal Settings. Ideally I'd like to add host settings as well to complete this.

Sample current module settings code can be found here: https://github.com/DNN-Connect/Conference/blob/master/Server/Conference/Common/ModuleSettings.cs

I'm going to do some research to see if I have made my own serializers somewhere.

[donker]

Would it be an option to leave the old Controller as is and deprecate it and instead add a "Manager" next to it? First: you'd avoid any breakages of people who have created their own serializers and second: the naming becomes consistent. I am not a big fan of the interface having a different name from the implementation as it is confusing for developers IMHO. Thoughts, @ahoefling ?

[mitchelsellers]

@ahoefling we are prepping for a 9.8.0 RC next Monday, can we drive this one to ready in that time? I know that @donker has made some adjustments in his own fork per the above

[SkyeHoefling]

I have updated this PR and rebased it on the latest changes in develop. It should be good to go, but with any Dependency Injection PR I strongly recommend we download the PR Installer and validate the happy path works. I'll comment once I run through it as my sign-off for the maintainers

@mitchelsellers thanks for letting me know about the timeline here!

@donker Thank you for making the contributions to the fork of this branch. I pulled them in fixed a few minor issues which I commented as PR feedback so we have the history. This was really a big help as I wouldn't have had the time to get this wrapped up for 9.8.0.