peteroupc / CBOR

A C# implementation of Concise Binary Object Representation (RFC 8949).
The Unlicense
206 stars 29 forks source link

CBORObject.ToObject when destination type contains IReadOnlyXxxx properties #54

Closed finnkam closed 3 years ago

finnkam commented 3 years ago

When the destination type has e.g. IReadOnlyCollection properties CBOR.Object.ToObject fails with a CBORException.

For example:

    public class Tests
    {
        [Test]
        public void ToObjectWithReadOnly()
        {
            var foo = new Foo
            {
                Bars = new List<Bar> {new Bar {Strings = new List<string> {"Mumbo", "Jumbo"}}}
            };

            var stream = new MemoryStream();
            CBORObject.FromObject(foo).WriteTo(stream);
            stream.Position = 0;
            CBORObject.Read(stream).ToObject(foo.GetType()); // Throws
        }
    }

    public class Foo
    {
        public IReadOnlyCollection<Bar> Bars { get; set; }
    }

    public class Bar
    {
        public IReadOnlyList<string> Strings { get; set; }
    }

Changing the Foo and Bar property types to List makes it work. I would however prefer to use IReadOnlyCollection/List if possible. Would it be possible to support IReadOnlyCollection/List in much the same way as (I)List is supported today?

peteroupc commented 3 years ago

With the current version you can define a custom ICBORToFromConverter<IReadOnlyList>, ICBORToFromConverter<IReadOnlyCollection>, etc., register the converters to a CBORTypeMapper, and pass the CBORTypeMapper to the ToObject method. For an example, see the README.

finnkam commented 3 years ago

Thanks for the fast response. :)

IReadOnlyCollection and IReadOnlyList are generic and take a type parameter. So I can't do exactly what you suggested.

I sketched the test into this:

    public class Tests
    {
        [Test]
        public void ToObjectWithReadOnly()
        {
            var foo = new Foo
            {
                Bars = new List<Bar> {new Bar {String = "Mumbo"}}
            };

            var typeMapper = new CBORTypeMapper();
            typeMapper.AddTypeName(typeof(Foo).FullName);
            typeMapper.AddTypeName(typeof(Bar).FullName);
            typeMapper.AddConverter(typeof(IReadOnlyCollection<Bar>), new ReadOnlyCollectionBarConverter(typeMapper));

            var stream = new MemoryStream();
            CBORObject.FromObject(foo, typeMapper).WriteTo(stream);
            stream.Position = 0;
            CBORObject.Read(stream).ToObject(foo.GetType(), typeMapper);
        }
    }

    public class Foo
    {
        public IReadOnlyCollection<Bar> Bars { get; set; }
    }

    public class Bar
    {
        public string String { get; set; }
    }

    public class ReadOnlyCollectionBarConverter : ICBORToFromConverter<IReadOnlyCollection<Bar>>
    {
        private CBORTypeMapper tm;

        public ReadOnlyCollectionBarConverter(CBORTypeMapper tm)
        {
            this.tm = tm;
        }

        public CBORObject ToCBORObject(IReadOnlyCollection<Bar> obj)
        {
            var array = CBORObject.NewArray();
            foreach (var item in obj)
            {
                array.Add(CBORObject.FromObject(item, tm));
            }

            return array;
        }

        public IReadOnlyCollection<Bar> FromCBORObject(CBORObject obj)
        {
            return obj.Values.Select(x => x.ToObject<Bar>(tm)).ToList();
        }
    }

It works (I think). But the "cost" seems to be a converter class per IReadOnlyCollection/List property instance in my model plus registration. It would sure be nice if IReadOnlyCollection/List was supported out of the box like (I)List. :)

I think I'm just going to use IList and avoid the converters.

peteroupc commented 3 years ago

I've tried your test code and discovered that it works without problems if you make the converter itself generic, as in this example: public class ReadOnlyCollectionBarConverter<TType> : ICBORToFromConverter<IReadOnlyCollection<TType>> (and replace Bar in that class's code with TType).

finnkam commented 3 years ago

Thanks for the followup.

You are right. The generic converter class reduces the count of converters from N to 1. But there still is the need for maintaining the type registrations. It would be nice to not having to worry about those (nudge, nudge). ;)

If a IReadOnlyCollection registration is missing I get this somewhat implicit exception:

PeterO.Cbor.CBORException : Exception of type 'PeterO.Cbor.CBORException' was thrown.
  ----> PeterO.Cbor.CBORException : Exception of type 'PeterO.Cbor.CBORException' was thrown.
   at PeterO.Cbor.PropertyMap.ObjectWithProperties(Type t, IEnumerable`1 keysValues, CBORTypeMapper mapper, PODOptions options, Int32 depth)
   at PeterO.Cbor.PropertyMap.TypeToObject(CBORObject objThis, Type t, CBORTypeMapper mapper, PODOptions options, Int32 depth)
   at PeterO.Cbor.CBORObject.ToObject(Type t, CBORTypeMapper mapper)
   at TestProject1.Tests.ToObjectWithReadOnly()
--CBORException
   at PeterO.Cbor.PropertyMap.TypeToObject(CBORObject objThis, Type t, CBORTypeMapper mapper, PODOptions options, Int32 depth)
   at PeterO.Cbor.PropertyMap.ObjectWithProperties(Type t, IEnumerable`1 keysValues, CBORTypeMapper mapper, PODOptions options, Int32 depth)

Working test code updated:

    public class Tests
    {
        [Test]
        public void ToObjectWithReadOnly()
        {
            var foo = new Foo
            {
                Bars = new List<Bar> {new Bar {Strings = new List<string>{ "Mumbo"}} }
            };

            var typeMapper = new CBORTypeMapper();
            typeMapper.AddTypePrefix(typeof(Foo).Namespace);
            typeMapper.AddConverter(typeof(IReadOnlyCollection<Bar>), new ReadOnlyCollectionConverter<Bar>(typeMapper));
            typeMapper.AddConverter(typeof(IReadOnlyCollection<string>), new ReadOnlyCollectionConverter<string>(typeMapper));

            var stream = new MemoryStream();
            CBORObject.FromObject(foo, typeMapper).WriteTo(stream);
            stream.Position = 0;
            CBORObject.Read(stream).ToObject(foo.GetType(), typeMapper);
        }
    }

    public class Foo
    {
        public IReadOnlyCollection<Bar> Bars { get; set; }
    }

    public class Bar
    {
        public IReadOnlyCollection<string> Strings { get; set; }
    }

    public class ReadOnlyCollectionConverter<T> : ICBORToFromConverter<IReadOnlyCollection<T>>
    {
        private CBORTypeMapper tm;

        public ReadOnlyCollectionConverter(CBORTypeMapper tm)
        {
            this.tm = tm;
        }

        public CBORObject ToCBORObject(IReadOnlyCollection<T> obj)
        {
            var array = CBORObject.NewArray();
            foreach (var item in obj)
            {
                array.Add(CBORObject.FromObject(item, tm));
            }

            return array;
        }

        public IReadOnlyCollection<T> FromCBORObject(CBORObject obj)
        {
            return obj.Values.Select(x => x.ToObject<T>(tm)).ToList();
        }
    }