Enum serialization

[berlotte]

Hi,

Not sure if it is an issue or just me doing something wrong. I'm trying to insert an object that has an Enum property

public enum ItemStatus {Active, Disabled, Deleted}

[DataContract]
[KnownType(typeof(ItemStatus))]
public class MyGreatObject{
    [DataMember(Name = "id", EmitDefaultValue = false)]
    public int Id{get;set;}

    [DataMember]
    public string Name{get;set;}        

    [DataMember]
    public ItemStatus ActiveStatus{get;set;}

    public MyGreatObject(){}
}

If I leave the Enum declaration as-is (not decorated with serialization/WCF attributes), when trying to Insert() my object, I get the following error:

Unhandled Exception:
System.AggregateException:  ---> System.NotSupportedException: Datum converter is not available for type Common.ItemStatus
  at RethinkDb.DatumConverters.DatumConverterFactoryExtensions.Get[ItemStatus] (IDatumConverterFactory datumConverterFactory, IDatumConverterFactory rootDatumConverterFactory) [0x0004c] in /home/matt/dev/Monitor/Monitor/lib/rethinkdb-net/rethinkdb-net/DatumConverters/DatumConverterFactoryExtensions.cs:31 
  at RethinkDb.DatumConverters.DatumConverterFactoryExtensions.Get[ItemStatus] (IDatumConverterFactory datumConverterFactory) [0x00014] in /home/matt/dev/Monitor/Monitor/lib/rethinkdb-net/rethinkdb-net/DatumConverters/DatumConverterFactoryExtensions.cs:18 
  at DataContractDatumConverterFactory.Common.Host.ConvertObject (Common.Host obj) [0x00000] in <filename unknown>:0 
  at RethinkDb.QueryTerm.InsertQuery`1[Common.Host].GenerateTerm (IDatumConverterFactory datumConverterFactory) [0x00066] in /home/matt/dev/Monitor/Monitor/lib/rethinkdb-net/rethinkdb-net/QueryTerm/InsertQuery.cs:35 
  at RethinkDb.Connection+<RunAsync>c__async6`1[RethinkDb.DmlResponse].MoveNext () [0x00061] in /home/matt/dev/Monitor/Monitor/lib/rethinkdb-net/rethinkdb-net/Connection.cs:429 
--- End of stack trace from previous location where exception was thrown ---
  at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw () [0x0000b] in /home/abuild/rpmbuild/BUILD/mono-3.2.1/mcs/class/corlib/System.Runtime.ExceptionServices/ExceptionDispatchInfo.cs:62 
  at System.Runtime.CompilerServices.TaskAwaiter`1[TResult].GetResult () [0x00000] in <filename unknown>:0 
  at RethinkDb.ConnectionFactories.ReliableConnectionFactory+ReliableConnectionWrapper+<RetryRunAsync>c__asyncF`1[RethinkDb.DmlResponse].MoveNext () [0x00053] in /home/matt/dev/Monitor/Monitor/lib/rethinkdb-net/rethinkdb-net/ConnectionFactories/ReliableConnectionFactory.cs:65 
  --- End of inner exception stack trace ---
  at System.Threading.Tasks.Task`1[TResult].get_Result () [0x00000] in <filename unknown>:0 
  at RethinkDb.TaskUtilities.ExecuteSynchronously[DmlResponse] (System.Func`1 taskDelegate) [0x00016] in /home/matt/dev/Monitor/Monitor/lib/rethinkdb-net/rethinkdb-net/TaskUtilities.cs:49 
  at RethinkDb.SynchronousApiExtensions.Run[DmlResponse] (IConnection connection, IScalarQuery`1 queryObject) [0x00021] in /home/matt/dev/Monitor/Monitor/lib/rethinkdb-net/rethinkdb-net/SynchronousApiExtensions.cs:33 
  at Monitor.Rethink.Save[Host] (Common.Host obj) [0x00014] in /home/matt/dev/Monitor/Monitor/Repositories/DAL/RethinkDB.cs:64 

• Now, if I decorate my Enum declaration with [DataContract], I get the following:

Unhandled Exception:
System.AggregateException:  ---> System.ArgumentNullException: Argument cannot be null.
Parameter name: obj
  at (wrapper managed-to-native) System.Reflection.Emit.ModuleBuilder:getToken (System.Reflection.Emit.ModuleBuilder,object,bool)
  at System.Reflection.Emit.ModuleBuilder.GetToken (System.Reflection.MemberInfo member, Boolean create_open_instance) [0x00000] in /home/abuild/rpmbuild/BUILD/mono-3.2.1/mcs/class/corlib/System.Reflection.Emit/ModuleBuilder.cs:678 
  at System.Reflection.Emit.ModuleBuilderTokenGenerator.GetToken (System.Reflection.MemberInfo member, Boolean create_open_instance) [0x00000] in /home/abuild/rpmbuild/BUILD/mono-3.2.1/mcs/class/corlib/System.Reflection.Emit/ModuleBuilder.cs:981 
  at System.Reflection.Emit.ILGenerator.Emit (OpCode opcode, System.Reflection.ConstructorInfo con) [0x00000] in /home/abuild/rpmbuild/BUILD/mono-3.2.1/mcs/class/corlib/System.Reflection.Emit/ILGenerator.cs:520 
  at RethinkDb.DatumConverters.DataContractDatumConverterFactory.DefineConvertDatum[ItemStatus] (System.Reflection.Emit.TypeBuilder type, System.Reflection.FieldInfo datumConverterFactoryField) [0x00120] in /home/matt/dev/Monitor/Monitor/lib/rethinkdb-net/rethinkdb-net/DatumConverters/DataContractDatumConverterFactory.cs:127 
  at RethinkDb.DatumConverters.DataContractDatumConverterFactory.Create[ItemStatus] () [0x000d6] in /home/matt/dev/Monitor/Monitor/lib/rethinkdb-net/rethinkdb-net/DatumConverters/DataContractDatumConverterFactory.cs:71 
  at System.Lazy`1[T].InitValue () [0x00000] in <filename unknown>:0 
--- End of stack trace from previous location where exception was thrown ---
  at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw () [0x0000b] in /home/abuild/rpmbuild/BUILD/mono-3.2.1/mcs/class/corlib/System.Runtime.ExceptionServices/ExceptionDispatchInfo.cs:62 
  at System.Runtime.CompilerServices.TaskAwaiter`1[TResult].GetResult () [0x00000] in <filename unknown>:0 
  at RethinkDb.ConnectionFactories.ReliableConnectionFactory+ReliableConnectionWrapper+<RetryRunAsync>c__asyncF`1[RethinkDb.DmlResponse].MoveNext () [0x00053] in /home/matt/dev/Monitor/Monitor/lib/rethinkdb-net/rethinkdb-net/ConnectionFactories/ReliableConnectionFactory.cs:65 
  --- End of inner exception stack trace ---
  at System.Threading.Tasks.Task`1[TResult].get_Result () [0x00000] in <filename unknown>:0 
  at RethinkDb.TaskUtilities.ExecuteSynchronously[DmlResponse] (System.Func`1 taskDelegate) [0x00016] in /home/matt/dev/Monitor/Monitor/lib/rethinkdb-net/rethinkdb-net/TaskUtilities.cs:49 
  at RethinkDb.SynchronousApiExtensions.Run[DmlResponse] (IConnection connection, IScalarQuery`1 queryObject) [0x00021] in /home/matt/dev/Monitor/Monitor/lib/rethinkdb-net/rethinkdb-net/SynchronousApiExtensions.cs:33 
  at Monitor.Rethink.Save[Host] (Common.Host obj) [0x00014] in /home/matt/dev/Monitor/Monitor/Repositories/DAL/RethinkDB.cs:64 

• My 'obj' is obviously not null, I successfully dump it just before calling Insert(). If remove the Enum property in MyGreatObject, insert works fine.

I'm using current master on Linux/Mono 3.2.1 What am I doing wrong?

Thanks for your help!

[mfenniak]

Hi @berlotte. You're not doing anything wrong; rethinkdb-net doesn't currently support enums. However, it would be a great addition to have. There would be a few implementation details that we'd need to define before we could add enum support; if you're looking to use them, perhaps you have an opinion on some of these details:

• How would they be stored on the server-side, as text (eg. "Active"), or as a numeric value (eg. 0, 1, 2)?
• Client -> Server, how do you handle enum values that aren't defined? (eg. (ItemStatus)3416516)
• Server -> Client, same question, how do you handle enum values that aren't defined?
• Do you have any need for [Flags] enums, or just standard single-value enums? [Flags] enums would be more complicated to support in the text-style serialization, and more difficult to validate if we use strictly defined-enum-only serialization.

By the way, it is possible to add enum support without library changes, as our datum converters are intended to be customizable. You'd need to:

• Implement IDatumConverter on a class (derived from AbstractDatumConverter makes it easier)
• Implement an IDatumConverterFactory that returns an instance of the above class when inspected for the type ItemStatus (derived from AbstractDatumConverterFactory makes it easier)
• Change the connection's DatumConverterFactory to a new AggregateDatumConverterFactory that includes your new DatumConverterFactory, and the original DatumConverterFactory value.

[berlotte]

Thanks Mathieu for replying so fast!

As I do code mostly on my personal time (I am certainly not a professional and experienced developer), my opinion will just be what I have in my head without any kind of deep analysis.

Since Rethinkdb-net seems to be following serialization semantics, there is the solution to mimic what others do. Most Json serializers seem to write enums as integers (Json.net, Servicestack serializer, Microsoft's DataContractJsonSerializer...). I'm talking about Json since I read that this is the format RethinkDB follows (http://www.rethinkdb.com/docs/comparison-tables/). The advantage I can see with Ints is that they save storage space, are mayber faster and easier to index and query(?). Flags storage and retrieval should be easier too I guess? About what to do when a value is unexpected, I can run tests with other libraires (Json, or .Net serializer) and see how they behave, or maybe an ArgumentOutOfRangeException would be clear enough.

I'd like to help but I'm not really sure I can understand your code well enough to do so. If adding code to Rethinkdb-net to handle Enums is "only" a matter of adding proper type checking and appropriate behavior when an enum is met, I can try to have a look, where do most of the types cheking and conversion/serializing occur in the library?

By the way, thanks for providing and maintaining this driver!

[karlgrz]

Hey @berlotte,

Appreciate the input!

If adding code to Rethinkdb-net to handle Enums is "only" a matter of adding proper type checking and appropriate behavior when an enum is met, I can try to have a look, where do most of the types cheking and conversion/serializing occur in the library?

That's basically it. If you take a look at some of the other DatumConverterFactory classes, that's pretty much all they do. If you give it a shot I'd be happy to help out / provide input.

[berlotte]

Hi,

I gave it a try and finally got a somewhat working thing.

using System;

namespace RethinkDb.DatumConverters
{
    public class EnumDatumConverterFactory : AbstractDatumConverterFactory
    {
        public static readonly EnumDatumConverterFactory Instance = new EnumDatumConverterFactory();

        public EnumDatumConverterFactory()
        {
        }

        public override bool TryGet<T>(IDatumConverterFactory rootDatumConverterFactory, out IDatumConverter<T> datumConverter)

        {
            datumConverter = null;
            if (typeof(T).IsEnum)
                datumConverter = (IDatumConverter<T>)EnumDatumConverter<T>.Instance.Value;
            return datumConverter != null;
        }
    }

    public class EnumDatumConverter<T> : AbstractValueTypeDatumConverter<T>
    {
        public static readonly Lazy<EnumDatumConverter<T>> Instance = new Lazy<EnumDatumConverter<T>>(() => new EnumDatumConverter<T>());

        #region IDatumConverter<Enum> Members

        public override T ConvertDatum(Spec.Datum datum)
        {
            return (T)Enum.ToObject(typeof(T) , (int)datum.r_num);
        }

        public override Spec.Datum ConvertObject(T en)
        {                
            int val = Convert.ToInt32(en);
            return new Spec.Datum() { type = Spec.Datum.DatumType.R_NUM, r_num =  val };
        }

        #endregion
    }
}

I kept the idea of only serializing/deserialzing from/to Ints, though I was wrong yesterday : after checking, the ServiceStack Json serializer convert Enums to strings by default. Having them serialized as Ints is possible but left as an option.

The above code works for the following kinds of enums:

    public enum ItemStatus {
        Active, 
        Disabled, 
        Deleted
    }

    public enum DummyChar : byte{
        One=2,
        Two=4,
        Three=6
    }

    [Flags]
    public enum DummyFlag {
        One=1,
        Two=2,
        Three=4
    }

It does NOT for Enums decorated with Serialization options such as:

    [DataContract]
    public enum DummyWithValues {
        [EnumMember(Value="10")]Ten,
        [EnumMember(Value="20")]Twenty,
        [EnumMember(Value="30")]Thirty
    }

Because, even if I'm not sure, Rethinkdb-net consider them as objects since they have these DataContract-style attributes. What I did (as a poor workaround) was to make DataContractDatumConverterFactory refuse to process Enums:

using System;
using System.Reflection;
using System.Reflection.Emit;
using System.Runtime.Serialization;

namespace RethinkDb.DatumConverters
{
    public class DataContractDatumConverterFactory : AbstractDatumConverterFactory
    {
        public static readonly DataContractDatumConverterFactory Instance = new DataContractDatumConverterFactory();
        private static readonly object dynamicModuleLock = new object();
        private static AssemblyBuilder assemblyBuilder = null;
        private static ModuleBuilder dynamicModule = null;

        private DataContractDatumConverterFactory()
        {
        }

        public override bool TryGet<T>(IDatumConverterFactory rootDatumConverterFactory, out IDatumConverter<T> datumConverter)
        {
            if (rootDatumConverterFactory == null)
                throw new ArgumentNullException("rootDatumConverterFactory");

            datumConverter = null;

            var dataContractAttribute = typeof(T).GetCustomAttribute<DataContractAttribute>();
            // *** REFUSE to process decorated Enums ***
            if (dataContractAttribute == null || typeof(T).IsEnum)
                return false;

            Type datumConverterType = TypeCache<T>.Instance.Value;
            datumConverter = (IDatumConverter<T>)Activator.CreateInstance(datumConverterType, rootDatumConverterFactory);
            return true;
        }
[.....]

I then get my annotated enum properly (de)serialized. How should we handle this special case?

As you can see I didn't implement any range checking (so (MyEnum)3259871 would probably be accepted since I read somewhere that Enum.ToObject() doesn't check that either). For now I'd like your feedback to know if I'm on the right track or if Enums support should be handled a totally different way.

I saw that generic collections are not yet supported, so if I can contribute in this area too, let me know (wouldn't 'List' support code be quite close to what the code for Arrays is?)

Thanks to both of you for your help!

[karlgrz]

This is great work, @berlotte! Would you be able to package this up as a pull request?

I can't speak for @mfenniak, but this looks pretty good to me. I think it's a fair trade off to limit Enums in the DataContractDatumConverterFactory for now. I have a feeling there will be a couple more cases similar to this that will come up. I think at that point, if it's causing us friction, we can address it. Good job!

As for generic List support, you are correct, it would be very similar to Array converter, I think.

[mfenniak]

Looks good, @berlotte. I would suggest adding a check in ConvertDatum to verify that the datum received is actually a number. And if you could create a fork and a pull-request with this code, as @karlgrz requested, that'd be great. Thanks!

I'm not too concerned about not supporting DataContractAttribute on enum types, so your change there is fine too.

[berlotte]

Hi,

@karlgrz : thanks for your support!

@mfenniak : Thanks, I will add checks based on what is done for UnsignedIntDatumConverter. Will create a pull request after doing a bit of cleaning.

While waiting for your feedback, I started working on Generic collections support. I have something working for types implementing IList<T>, will see if can have it handle types of a broader scope than simply IList<T>. It works fine even serializing lists of objects, but I'm not happy with the implementation I got, especially regarding performance, so I'll really need your advice. Will send you code probably by tomorrow!

[berlotte]

Hi!

I finally made the code to ensure that Enum value is an int by simply calling PrimitiveDatumConverterFactory.IntDatumConverter.Instance.Value.ConvertDatum() I thought it was a good idea since avoids useless code duplication, what do you think?

I started working on a converter for IEnumerable, and to allow converting of the largest number of generic collections I ended up doing almost the same thing than the already existing Array converter. After realizing that, I simply merged the two. Now my IEnumerableTDatumConverterFactory replaces and extends the ArrayDatumConverterFactory. Again, is it a good idea? Could you have a look at my code here and tell me if you know of optimizations (I'm calling Activator.CreateInstance() to create a generic collection but it's probably not the fastest solution)? Note : ran multiple tests with Arrays, List<int>, List<string>, List<complex_object>, Queue<string>, Stack<string>, HashSet<string>, all work fine.

Tried to run the unit tests, and they all pass. If one day my code is mergeable, then I'll add a few unit tests for collections and enums.

Also, I started working on IDictionary<TKey,TValue> support, and tried using R_OBJECT and assocpairs. Seemed a good idea at first, but -Serialization works, I see the data in the db table, but reading the r_object doesn't work (emtpy content?) -assocpair key must me a string, don't know if it's a Rethinkdb limitation. That means that after hacking a bit, the string key could store any IDictionary key that has a simple type (int, string, char...), but not complex ones. However that should address most of users needs, I personally never felt the need to store complex types in dictionary keys. Again, what do you think?

I forked your repo and pushed my changes at https://github.com/berlotte/rethinkdb-net

Thanks for your help and advice!

[mfenniak]

Hi @berlotte,

First of all, I really appreciate your enthusiasm and interest in improving this project! :-) Now... please don't take any of my criticism too harshly, you're helping move this project in a great direction.

Using the PrimitiveDatumConverterFactory's IntDatumConverter for the enum type seems like a good idea.

I'd appreciate it if you could create separate branches for each of these changes, so that they could be evaluated and merged independently. The more work you do in one branch, the harder it is to do thorough code reviews and accept the patches. The enum work should be separate from the enumerable work, and the dictionary work should be separate again.

I'm a little hesitant about any datum converter that supports converting interfaces, like IEnumerable, IList, and IDictionary. How are you planning to construct those objects when converting a datum to an interface? Is the implementation of that interface going to be something configurable or optional? For example, an IDatumConverter<IList>'s ConvertDatum method could theoretically return a System.Collections.Generic.List, or a System.ArraySegment, or an int[], or any user-defined class that implements IList or IList... what should it actually do? IDictionary and IEnumerable have many more core-framework options, and they're even more likely to be unexpected types. It might be interesting to research some other serialization systems and see how they handle this issue.

Much of your code doesn't match the same coding style being used by the rest of this project. I think the main difference is that you're using tabs in all your source files, and this project uses four-space indentation throughout. This type of inconsistency doesn't seem like a big deal on one hand, but I try to avoid it because it creates future scenarios where it's difficult to merge changes from different contributors.

Anything doing Console.WriteLine will need to be removed. :-) It's not good form for a library to spit stuff out on the console, and it's especially problematic if someone uses this library to build console applications.

Also, it'd be great to have unit tests for these new datum converters. I'd certainly merge them without unit tests (I'm actually more pedantic about indentation!), but it might help remove your need to throw Console.WriteLine's in there if you're confident that they're working correctly via unit tests.

[mfenniak]

Oh, I missed your comment about IDictionary and R_OBJECT. There are a huge number of issues with using IDictionary to support objects; many library changes would be involved beyond a datum converter. I think this needs a high-level design that incorporates how it would eventually be worked into query expressions, otherwise we'd be taking a stab in the dark on this one.

[berlotte]

Hi,

Thanks @mfenniak for your constructive feedback, I appreciate that a lot.

I'd appreciate it if you could create separate branches for each of these changes, so that they could be evaluated and merged independently. The more work you do in one branch, the harder it is to do thorough code reviews and accept the patches. The enum work should be separate from the enumerable work, and the dictionary work should be separate again. Sure, will do, totally understand that. For now my fork is only there in its current state to allow you to have a look at what I did and discuss about that.

I'm a little hesitant about any datum converter that supports converting interfaces, like IEnumerable, IList, and IDictionary. How are you planning to construct those objects when converting a datum to an interface? Is the implementation of that interface going to be something configurable or optional? I don't think I understand your concerns, and I'm sorry about that. I use Activator.CreateInstance() to re-construct the serialized member according to <T>'s type. If it's a List<T>, then a List<T> will be returned, Stack<T> if the property was a Stack<T> and so on. I respect the original type of the serialized property. I did a very wide assumption that allows this Converter to work with most of System.Collections.Generics ; the converter accepts (and work with) -everything being a Generic and an IEnumerable, but NOT a Dictionary -and only those having a new(IEnumerable) constructor.

This allows having List, Queue, Stack, HashSet, Collection (and maybe others) working "out of the box", because they are all generic collections implementing IEnumerable and having a constructor accepting an IEnumerable as a parameter. What do you think that should be configurable or optional? I'm all open, it's just that I don't understand :)

It might be interesting to research some other serialization systems and see how they handle this issue.

Looking at other implementations (Mongodb C# driver and ServiceStack Json serializer), they are more restrictive and separate collections handling in 2 or 3 different converters, but they work basically the same way, using Activator to reconstruct the serialized type, and sometimes they have 'hardcoded' converters for very frequent Generics : for example, Mongodb has a special if/else condition to check if the generic collection was a List<T> and returns immediately (a List<T>) if so, as an optimization. ServiceStack Json even has more specialized conditional checks to quickly return if ICollection or ICollection, and fallback to slow path/activator.CreateInstance else. While it's probably faster than my implementation, I'm only looking at something more generic at the moment.

Could my implementation have some limits? More than probably, as other implementations do. If someone implements its own IEnumerable class with a weird new(IEnumerable) constructor that does nothing instead of filling the initial collection backing store, then my converter wil fail miserably. But I'm here to learn, so again, your advice is really needed :) If it appears that we have to do a different converter for every king of Generic collection, and throw&forget my converter, I'm fine with that!

Much of your code doesn't match the same coding style being used by the rest of this project. And I'm very sorry, since my tree was there just fo you to look at my code and comment, I didn't even pay attention to this kind of details. Should be fixed for Enum and IEnumerableT converters.

Anything doing Console.WriteLine will need to be removed. :-) It's not good form for a library to spit stuff out on the console, and it's especially problematic if someone uses this library to build console applications. Of course. The code I published on my fork wasn't aiming at being directly mergeable, I'm only looking for discussion and comments for now.

Also, thanks for your comments about Dictionary, will forget it for now then.

Also, it'd be great to have unit tests for these new datum converters. I'd certainly merge them without unit tests (I'm actually more pedantic about indentation!), but it might help remove your need to throw Console.WriteLine's in there if you're confident that they're working correctly via unit tests. I'm really willing to add unit tests, once I know what in my code could be officially merged and what should be thrown and forgotten. Totally understand how important are unit tests!

Oter than that, I cancelled my IEnumerable and Array converters merge, they are now 2 distinct converters again.

[mfenniak]

Here's a more succinct description of my question about the enumerable converter. :-)

[DataContract]
public class Person
{
    [DataMember]
    public string Name;
    [DataMember]
    public IList<Location> Residences;
}

When I do this:

var berlotte = conn.Run(
    Query.Db("test")
    .Table<Person>("people")
    .Filter(p => p.Name == "berlotte")
);

What type is berlotte.Residences? How was it created? How did rethinkdb-net determine that type?

[berlotte]

@mfenniak , very good point.

Again, I looked at what Mongodb C# driver does : for ICollection<T>, IList<T> and IEnumerable<T>, it returns a List<T> (link : https://github.com/mongodb/mongo-csharp-driver/blob/master/MongoDB.Bson/Serialization/Serializers/EnumerableSerializer.cs ) It makes sense since List<T> inherits both ICollection<T> and IEnumerable<T>.

So basically I went exactly the same way and ended up with another new converter, GenericsCollInterfaceConverter. Feel free to have a look at my forked tree.

Are things finally starting to take shape, or should I give up with these 2 new Generics converters? Thanks,

[berlotte]

Short and simple answer : it means that berlotte.Residences is a List<Location>

[karlgrz]

My apologies for being absent yesterday, my job has kept me quite busy the past few days!

@berlotte I applaud the effort you have put in thus far! This stuff is looking great!

Short and simple answer : it means that berlotte.Residences is a List

It's a little more complicated than that, which I think is why @mfenniak brought it up. Given only the data that is stored in the db, how do you determine that it should be List? You make an arbitrary decision to return that type, for one of many reasons.

The point is there isn't an elegant way in the framework to determine exactly which type is coming down the wire unless you try to deserialize it OR you provide some kind of type metadata alongside the data.

What happens if you are trying to utilize a different framework? Why cast to some arbitrary implementation when IEnumerable might be sufficient?

Your work brings up a great discussion point about how to handle this kind of implementation detail. I think we've been kind of punting it off until now, but it might be time to figure out how to solve that problem, it's only going to keep coming up down the road.

Again, @berlotte, this is great, and I'm really glad you put in all this hard work! It sounds like you're getting very close to wrapping this up.

[tetious]

Perhaps the correct approach in this situation would be to make IEnumerableTDatumConverterFactory have an optional concrete type override?

I think the principle of least surprise would not be violated if it used a List by default, and making it flexible enough to be overridden when needed might be sufficient for the corner cases. I suspect most folks will want and expect it to be a List.

[mfenniak]

@tetious's approach sounds like what I'd want to see. I would imagine the entire creation of the concrete type instance to be virtual; eg. protected virtual IList<T> CreateList(T[] objects) { return new List<T>(objects); }. That approach (a) allows the use of different concrete types, and (b) doesn't require using Activator.CreateInstance, and (c) doesn't require that the concrete type have a constructor w/ an IEnumerable argument, as not all implementations of IList would be guaranteed to have that constructor.

[berlotte]

Hi @karlgrz and @tetious ,

Thanks for the positive feedback!

It's a little more complicated than that, which I think is why @mfenniak brought it up. Given only the data that is stored in the db, how do you determine that it should be List? You make an arbitrary decision to return that type, for one of many reasons. Yes, in this exact situation, we have no mean to set a precise type. I understand your concern, but to make it 'just work', I looked at what MongoDB did, and their choice of returning a List is, in my opinion, the best compromise. I did the same way they do : if we have an interface, accept to (de)serialize it if, and only if, this interface is a generic IList, ICollection or IEnumerable. Else, the user is left alone for now. I'm sure we could/should have some ways to pass more guidance to Rethinkdb-Net, by overriding, or a member attribute such as [MapTo(typeof(List<>))] public IList GreatStuff{get;set;} ... and so on. I'm all for it.

The point is there isn't an elegant way in the framework to determine exactly which type is coming down the wire unless you try to deserialize it OR you provide some kind of type metadata alongside the data. Yes, I guess that's why other drivers took this kind fo compromise too

Why cast to some arbitrary implementation when IEnumerable might be sufficient I chose the collection implementing all of the 3 interfaces I consider in this converter. It allows the user to use transparently what's returned after a db query.

Thanks for bringing this interesting discussion. And, if this pure-interface converter is so controversial, why not just ignore and forget it? I'm not here to force anyone to merge changes in the official driver :)

[karlgrz]

I don't think we should ignore and forget it, I want to use it :-)

I am all for this implementation, I just wanted to bring up more discussion around it, that's all. It seems to be a good compromise of functionality and expressiveness.

:+1:

[berlotte]

@mfenniak : Could you explain how we could avoid calling Activator.createInstance? I'd love to figure it out, but couldn't find an easy way with my poor Generic and Reflection skills.

[thinkong]

can this please be implemented? actually i was looking for some kind of container for an array that may vary in size.... like.. list<> or arraylist<>

maybe i just can't find these ... are these implemented?

[mfenniak]

@berlotte, I've taken your initial Enum datum converter implementation, added some unit tests, and synced it in. I've created issue #145 to continue any discussion and implementation work on the generic collections.

[mfenniak]

Hi @thinkong. The ability to use List<> has now been implemented in the newly released rethinkdb-net 0.6.0.0.

[thinkong]

@mfenniak thank you for the ability.. so.. can i just use the as [Datamember] or.. do i have to implement them as showed here

[mfenniak]

@thinkong, you can just add [DataMember] to a List property or field, as shown here: https://github.com/mfenniak/rethinkdb-net/blob/master/rethinkdb-net-test/Integration/TestObject.cs#L19