Generic mapping for non-collection type

[radiosterne]

These changes fix wrong treatment of non-collection generic types: in upstream version they are treated as generic collections, so if it's instantiated with value type, it is mapped to its underlying type — "Tuple<int, int>" gets mapped to "integer". Here I'm just checking whether type implements ICollection and use underlying type only if it is. Also, basic test is provided. (And I've signed CLA =)

[Mpdreamz]

Hey @radiosterne sorry for not responding earlier, this PR totally flew under my radar. Will have a look tomrorrow, thanks for signing the CLA and including tests!

[gmarz]

@radiosterne Thanks for the PR.

I agree Tuple<int, int> shouldn't map as an integer, and GetUnderlyingType definitely needs to be more robust. However I think I spotted a problem with your proposed change:

Consider the case where we have a generic type like Nullable<int>? You would expect that to map as an integer, however GetUnderlyingType will instead return Nullable as the type since it's generic but not an ICollection. Nullable of course doesn't map to an ES type and thus the property is ignored.

Also, I think it would be better to check for IEnumerable instead of ICollection?

[radiosterne]

Thanks for pointing these issues out, @gmarz !

You're totally right on both accounts, so I've changed my code a bit according to your propositions.

I can't think of any other cases when we should map to underlying type by default (of course, any behaviour can still be overriden by using ElasticProperty attribute, if I'm not mistaken), can you?

[gmarz]

@radiosterne Great, thanks for following up!

These are definitely edge cases, but it would be best to stay as generic as possible. Thinking about this some more...I wonder if this makes the most sense:

private static Type GetUnderlyingType(Type type)
{
    if (type.IsArray)
        return type.GetElementType();

    if (type.IsGenericType && type.GetGenericArguments().Length == 1)
        return type.GetGenericArguments()[0];

    return type;
}

Return the underlying type when the generic only has a single type argument. This takes care of IEnumerable<T>, Nullable<T>, or any FooBar<T>. I think it's safe to say in most cases, we really only care about T and not the thing that wraps it. Otherwise, just return the type.

It will never be perfect, but it's better than just arbitrarily assuming the first type argument of a generic type is the correct type to map as the current implementation does.

Thoughts?

[radiosterne]

@gmarz I have to object against such implementation: actually, when I first encountered this behaviour was with my custom generic type Range<T> :

    public class Range<T> where T : struct
    {
        public T? From { get; set; }
        public T? To { get; set; }
    }

I think, this can be a pretty common usecase, so we should handle arbitrary generic with single type argument as a nested object by default.

But you are, of course, right, we should only map IEnumerable to its underlying type only if it, actually, has one type argument: consider Dictionary<string, int>: it would be a mistake to map it to string =)

I therefore propose to change type.GetGenericArguments().Length >= 1 to type.GetGenericArguments().Length == 1 in my current solution: that would handle most of the cases.

[Mpdreamz]

++ @radiosterne was just typing up a similar response.

We should assume any FooBar<T> is an object not its underlying T. And only destructure to T when its an IEnumerable<T>, or Nullable<T> where the actual typeof(T) only has 1 generic argument.

[gmarz]

Right on 👍

[radiosterne]

I've synced my branch with upstream, so you're good to go!

[gmarz]

Hey @radiosterne I ended up just cherry-picking your 94060e2 commit which was a bit cleaner than merging the entire PR, since your branch is from master and not develop.

Also added a simple unit test for this...didn't think the full blown integration tests were necessary (although really appreciated). Unit tests are also nice because they run as part of our CI. Hopefully this is cool with you...feel free to expand on the tests more if you think we need to.

Thanks again for the PR, this was a great find! Looking forward to more from you :) 👍