paiden / Nett

.Net library for TOML
MIT License
223 stars 27 forks source link

Is it possible to convert from interfaces or genric types. #59

Closed ldbenoit closed 5 years ago

ldbenoit commented 6 years ago

Hey, is it possible to use interface for ConfigureType? I have a generic class and I'd like to serialize it as a dictionary. Unfortunately I can't get converter to recognize it unless I explicitly set it with its type arguments. E.g. This works but has to be set for every type.

var tomlConfig = TomlSettings.Create(cfg => cfg
    .ConfigureType<ConfigObject<Setting>>(type => type
        .WithConversionFor<TomlTable>(convert => convert
            .ToToml((obj, table) => obj.ForEach(x => table.Add(x.Key, x.Value)))
            .FromToml(table => table.ToDictionary()))));

Doesnt work.

var tomlConfig = TomlSettings.Create(cfg => cfg
    .ConfigureType<IConfigObject>(type => type
        .WithConversionFor<TomlTable>(convert => convert
            .ToToml((obj, table) => obj.ForEach(x => table.Add(x.Key, x.Value)))
            .FromToml(table => table.ToDictionary()))));
paiden commented 6 years ago

The following test works fine for me:

        private interface IGenType
        {
        }

        private class GenTypeWithInterfaceHost
        {
            public IGenType X { get; set; } = new GenTypeWithInterface<string>();
        }

        private class GenTypeWithInterface<T> : IGenType
        {
            public T Value { get; set; } = default(T);
        }

and the test

            var obj = new GenTypeWithInterfaceHost();
            obj.X = new GenTypeWithInterface<string>() { Value = "It works" };

            var cfg = TomlSettings.Create(c => c
                .ConfigureType<IGenType>(ct => ct
                    .WithConversionFor<TomlTable>(conv => conv
                        .ToToml((o, t) => t.Add("Value", ((GenTypeWithInterface<string>)o).Value))
                        .FromToml(t => new GenTypeWithInterface<string>() { Value = t.Get<string>("Value") + " really!" }))));

            var tml = Toml.WriteString(obj, cfg);
            var read = Toml.ReadString<GenTypeWithInterfaceHost>(tml, cfg);

            // Assert
            ((GenTypeWithInterface<string>)read.X).Value.Should().Be("It works really!");

As far as I can tell this test should do the same thing as you described. Can you provide more details?

ldbenoit commented 6 years ago

Hey, your example doesn't work for me. The conversion never actually gets called, but even if it did it's not quite what I was thinking. What I was thinking would be something like:

        private class GenTypeWithInterfaceHost
        {
            public GenTypeWithInterface<string> stringX { get; set; } 
            public GenTypeWithInterface<int> intX { get; set; } 
            public GenTypeWithInterface<double> doubleX { get; set; } 
        }

and converted each to Dictionary<string, object> through an interface. The reason I want to do this is mostly because of the way Nett serializes my class. For example, compare the way these are serialized.

    public class Example
    {       
        public ConfigObject<Setting> MyObj { get; set;}
        public Dictionary<string, Setting> MyObjDict { get => MyObj; set => MyObj = value;}
    }

[[MyObj]]
Key = "Setting1"

[MyObj.Value]
Name = "A Setting"
IsSomething = false
AnAmount = 20.0
[[MyObj]]
Key = "Setting2"

[MyObj.Value]
Name = "Another Setting"
IsSomething = true
AnAmount = 45.0

[MyObjDict]

[MyObjDict.Setting1]
Name = "A Setting"
IsSomething = false
AnAmount = 20.0

[MyObjDict.Setting2]
Name = "Another Setting"
IsSomething = true
AnAmount = 45.0

The dictionary is much nicer, when I have nested objects it can get really ugly with 'MyObj.Value.Value.Value'. If I use TomlIgnore attribute on MyObj above it has the exact behavior I want but it would obviously be nicer to have a converter for this.

paiden commented 6 years ago

There seems to be an issue with the converter check that causes this scenario to fail, although it should work. I'm still investigating the exact cause.

class Base {}
class Gen<T> : Base{}
class Root { Gen<string> X {} }

config.ConfigureType<Base>(...)

But, if there is no non-generic base type, you have to configure each explicit generic type and that is expected behavior.

ldbenoit commented 6 years ago

Hey, I thought about that but, unless I am misunderstanding, it has the same problem and wont be caught by the converter. I had a look at the code and I suppose the problem lies here:

public bool CanConvertFrom(Type t) => t == StaticFromType;

public bool CanConvertTo(Type t) => t == StaticToType || t.IsAssignableFrom(StaticToType);

CanConvertFrom will fail because the class ins't equal to the interface. It would work with:

public bool CanConvertFrom(Type t) => t == StaticFromType || StaticFromType.IsAssignableFrom(t);

CanConvertTo will fail because the class isnt assignable from an interface. StaticToType.IsAssignableFrom(t) would fix things for me but that would probably cause more problems,

Thanks for your help, I think I see the error in my thinking on this one. With that said, is it possible to have classes that inherit from IDictionary<> be serialized similar to a Dictionary<>. That's what got me started on this rabbit hole to begin with. Thanks again.

paiden commented 6 years ago

Yes the problem lies in this line.

During writing tests and debugging this stuff, I also remembered that not including the inheritance chain in the check the other way round (the is assignable this way is for a Nett internal use case) was by design.

I wanted to force the user to explicitly state the converter usage for each type.

But I see the validity of your use case. I'm just not sure that the converter is the right mechanism to use here. It feels somewhat like a work around to me to use converters for that.

If I understand correctly, you have wrapper ConfigObject around your real config data of type T. So when serializing/deserializing you want to omit the T Value {get;set;} property and directly serialize the T instead. If so, I think this will be another feature of the config, that I may make less strict than the converter stuff (Converters are strict regarding the types for a good reason, otherwise a misconfigured TomlConfig can create really hard to debug serialization bugs).

I'm still not 100% sure what you mean with the serialize dictionary statement. Maybe a full code sample (and probably failing unit test) would help me to understand it better/quicker.

ldbenoit commented 6 years ago

Ya, I see that converter isn't really the right place for this now.

The ConfigObject is similar to a typed ExpandoObject. It inherits DynamicObject and IDictionary<string,object> and has conversions to/from Dictionary<string, object> and Dictionary<string, T>. Ideally it would be serialized like a dictionary but as you can see above it separates it and makes it sort of hard to read.

[[MyObj]]
Key = "Setting1"

[MyObj.Value]
Name = "A Setting"

instead of

[MyObjDict.Setting1]
Name = "A Setting"

It's not a big deal and I'm fine leaving it at that. Thanks for all your help in understanding this.

paiden commented 6 years ago

So I finally found some time to analyze the issue a little bit further. Unfortunately still I'm confused about it.

In commit 385f52b I wrote a unit test that should reproduce you scenario. And in fact Nett already directly serializes objects that implement IDictionary.

So the output of the test is the expected shorter form:

RootSetting = "SomeVal"
[Sect]
Prop = "PropValue"

instead of

RootSetting = "SomeVal"
[Sect]
[Sect.Data]
Prop = "PropValue"

So I'm not really sure why Nett serializes your object differently. So once more the question, can you provide a small repro project, test etc.?