AutoMapper / AutoMapper.Extensions.ExpressionMapping

MIT License
143 stars 39 forks source link

ArgumentException for expression parameter on UseAsDataSource().OrderBy with nullable type in destination #97

Closed mycroes closed 4 years ago

mycroes commented 4 years ago

When a DTO has a nullable property for a non-nullable source expression UseAsDataSource throws when trying to OrderBy on the destination property.

The following testcase reproduces this exception:

using System;
using System.Collections.Generic;
using System.Linq;
using System.Linq.Expressions;
using Shouldly;
using Xunit;

namespace AutoMapper.Extensions.ExpressionMapping.UnitTests
{
    public class MappingToNullablePropertyUsingUseAsDataSource
    {
        [Fact]
        public void When_Apply_OrderBy_Clause_Over_Queryable_As_Data_Source()
        {
            // Arrange
            var mapper = CreateMapper();

            var models = new List<Model>()
            {
                new Model {Value = 1},
                new Model {Value = 2}
            };

            var queryable = models.AsQueryable();

            Expression<Func<DTO, int?>> dtoPropertySelector = (dto) => dto.Value;

            // Act
            var result = queryable.UseAsDataSource(mapper).For<DTO>().OrderBy(dtoPropertySelector).ToList();

            // Assert
            result.ShouldNotBeNull();
            result.Count.ShouldBe(2);
        }

        private static IMapper CreateMapper()
        {
            var mapperConfig = new MapperConfiguration(cfg =>
            {
                cfg.CreateMap<Model, DTO>();
            });

            var mapper = mapperConfig.CreateMapper();
            return mapper;
        }

        private class Model
        {
            public int Value { get; set; }
        }

        private class DTO
        {
            public int? Value { get; set; }
        }

    }
}

The error occurs in ExpressionMapper.GetConvertedMethodCall, where parameter dto => dto.Value of type Func<DTO, Nullable<int>> gets converted to Func<Model, int> and the generic type replacement doesn't cover the return type of the Func.

I tried to fix this myself, but I think this requires matching the converted arguments and their types against the generic arguments of the method, for which I couldn't think of a generic approach that wouldn't involve mapping for known types or method signatures only.

mycroes commented 4 years ago

Perhaps it's actually not that hard at all. Currently types are matching on the closed generics, but when taking the open generic types it's possible to match them to the exact types of arguments (at least, I guess it is).

BlaiseD commented 4 years ago

I suspect all you need is the following:

                cfg.CreateMap<Model, DTO>()
                   .ForMember(d => d.Value, opts => opts.MapFrom(s => (int?)s.Value));

e.g. Expression<Func<Model, int>> mapped = mapper.MapExpression<Expression<Func<Model, int>>>(dtoPropertySelector); will give you a more specific exception message. It's different code but I think they fail for similar reasons.

mycroes commented 4 years ago

Hi @BlaiseD,

Actually, that doesn't give any exceptions. We're using UseAsDataSource together with some frontend infra to provide filtering and sorting. In some cases we have a collection of registrations on the entities which have a datetime and event (enum) field. In the table we want to show the latest occurence for some events, so we have a Latest(TEvent event) extension on the registration collection. Now in some cases we have registrations that are always there (event: Created), but most are not guaranteed to be there. Our viewmodel has a DateTimeOffset to accomodate the timestamp of the created registration and a DateTimeOffset? for the scheduled registration. We use the same extension for both properties, which works fine for the actual projection, but it currently breaks on ordering and filtering.

We actually figured a workaround, we're now (ab)using ForAllMemberMaps to fixup mappings from DateTimeOffset to DateTimeOffset? which makes this work for both filtering and ordering (both are broken without the cast).

Anyway, long story short, this is still an issue within the expression mapping. I'm guessing it might even apply to all type conversions between model and dto properties, but we've only been bothered by this issue for DateTimeOffsets.

BlaiseD commented 4 years ago

cfg.CreateMap<Model, DTO>(); does throw for your example for MapExpression. What you're describing is by design. It's been brought up before. We are always open to improvements.

mycroes commented 4 years ago

I'm wondering why you're saying the CreateMap would throw. Besides that it doesn't throw in the test, I see no reason why AutoMapper couldn't map int to int?, the other way around could be more troublesome of course. Also, if it wouldn't work, then what's NullableDestinationMapper supposed to do?

I added my tests using int and int? because it's probably one of the most basic conversions, so it's the best showcase for the issue. It seems I've already thrown you off with the DateTimeOffset reference because the issue you're pointing to is about DateTime to DateTimeOffset conversions (which doesn't relate unfortunately).

So please take another look at this. Can you clarify why this shouldn't work? I can't figure why this shouldn't work except for the simple fact that it's not currently implemented...

BlaiseD commented 4 years ago

Again - this is by design. Here's the error message when you call MapExpression with this configuration cfg.CreateMap<Model, DTO>();.

Expression<Func<Model, int>> mapped = mapper.MapExpression<Expression<Func<Model, int>>>(dtoPropertySelector);

Result Message: System.InvalidOperationException : The source and destination types must be the same for expression mapping between literal types. Source Type: Int32, Source Description: Value, Destination Type: Nullable`1, Destination Property: Value.

PRs are welcome though.

mycroes commented 4 years ago

Ok, I didn't understand the error was supposed to be related to the MapExpression call, which I wasn't using. Also, UseAsDataSource() doesn't present that exception, it will generate an expression with mapped types but then won't infer the correct generic arguments for the method that the expression is passed too., which results in a different exception altogether.

You're saying this behavior is by design, why is it designed to work this way? Is there any reason not to implement this?