Query projection with subquery can generate non-compiling expression

[Erythnul]

Hey guys,

I've run into an issue with query projection when subqueries are included.
I've spent some time debugging and seeing if I could get it fixed up, I've got a pull request coming up with a fix that definitely works for my scenario and most probably other ones as well.
The code isn't the most elegant, but I'm not exactly at home in your source so I tried to keep my additions small and easily readable. If you've got a better way of fixing this problem, I'll gladly change the source (or feel free to change it yourself and fix up the pull request before merging).

More details in the reproduction repo:

https://github.com/Erythnul/AutomapperIssue

Source/destination types

//Source
public class Order
{
    public Guid Id { get; set; }
    public Guid CustomerId { get; set; }

    public int? MostImportantOrderLine { get; set; }

    public ICollection<OrderLine> OrderLines { get; set; } = null!;
}
public class OrderLine
{
    public Guid Id { get; set; }
    public Guid OrderId { get; set; }

    public int OrderLineNumber { get; set; }
    public string Description { get; set; } = null!;

    public Order Order { get; set; } = null!;
}

//Destination
public class OrderModel
{
    public OrderSubModel? OrderSubModel { get; set; }
    public OrderLineModel? MostImportantOrderLine { get; set; }
}

public class OrderSubModel
{
    public Guid? OrderId { get; set; }
}

public class OrderLineModel
{
    public Guid OrderLineId { get; set; }
    public string Description { get; set; } = string.Empty;
}

Mapping configuration

new MapperConfiguration(cfg =>
{
    cfg.CreateMap<Entities.Order, Dtos.OrderModel>()
        .ForMember(dst => dst.OrderSubModel, opt => opt.MapFrom(src => src.MostImportantOrderLine != null ? src : null))
        .ForMember(dst => dst.MostImportantOrderLine, opt =>
        {
            opt.MapFrom(src => src.OrderLines.FirstOrDefault(x => x.OrderLineNumber == src.MostImportantOrderLine));
        });

    cfg.CreateMap<Entities.Order, Dtos.OrderSubModel>()
        .ForMember(dst => dst.OrderId, opt => opt.MapFrom(src => src.Id));

    cfg.CreateMap<Entities.OrderLine, Dtos.OrderLineModel>()
        .ForMember(dst => dst.OrderLineId, opt => opt.MapFrom(src => src.Id))
        .ForMember(dst => dst.Description, opt => opt.MapFrom(src => src.Description));
});

Version: 10.1.1

Expected behavior

//projectedQuery.Expression
queryable.Select(
    dtoOrder => new Object_1548421275___MostImportantOrderLine_MostImportantOrderLine_MostImportantOrderLine
    {
        __MostImportantOrderLine = dtoOrder.OrderLines.FirstOrDefault(x => ((int?)x.OrderLineNumber) == dtoOrder.MostImportantOrderLine),
        MostImportantOrderLine = dtoOrder.MostImportantOrderLine,
        Id = dtoOrder.Id
    }).Select(
    dtoLet => new Dtos.OrderModel
    {
        MostImportantOrderLine = (dtoLet.__MostImportantOrderLine == null)
            ? null
            : new Dtos.OrderLineModel
            {
                Description = dtoLet.__MostImportantOrderLine.Description,
                OrderLineId = dtoLet.__MostImportantOrderLine.Id
            },
        OrderSubModel = (((dtoLet.MostImportantOrderLine != null) ? dtoLet : null) == null)
            ? null
            : new Dtos.OrderSubModel
            {
                OrderId = (Guid?)((dtoLet.MostImportantOrderLine != null) ? dtoLet : null).Id
            }
    });

Actual behavior

//projectedQuery.Expression
queryable.Select(
    dtoOrder => new Object_1548421275___MostImportantOrderLine_MostImportantOrderLine_MostImportantOrderLine
    {
        __MostImportantOrderLine = dtoOrder.OrderLines.FirstOrDefault(x => ((int?)x.OrderLineNumber) == dtoOrder.MostImportantOrderLine),
        MostImportantOrderLine = dtoOrder.MostImportantOrderLine
        //, Id = dtoOrder.Id
        // Doesn't collect OrderId into dtoLet, is also not added as possible field to Proxy Object above
    }).Select(
    dtoLet => new Dtos.OrderModel
    {
        MostImportantOrderLine = (dtoLet.__MostImportantOrderLine == null)
            ? null
            : new Dtos.OrderLineModel
            {
                Description = dtoLet.__MostImportantOrderLine.Description,
                OrderLineId = dtoLet.__MostImportantOrderLine.Id
            },
        OrderSubModel = (((dtoLet.MostImportantOrderLine != null) ? dtoOrder : null) == null) //Doesn't compile, should be dtoLet
            ? null
            : new Dtos.OrderSubModel
            {
                OrderId = (Guid?)((dtoLet.MostImportantOrderLine != null) ? dtoOrder : null).Id //Doesn't compile, should be dtoLet
            }
    });

Steps to reproduce

https://github.com/Erythnul/AutomapperIssue

var orders = new[]
{
    new Order
    {
        Id = Guid.Parse("00000000-0000-0000-0000-000000000001"),
        MostImportantOrderLine = 1,
        OrderLines = new List<OrderLine>
        {
            new OrderLine
            {
                Id = Guid.Parse("00000000-0000-0000-0000-000000000101"),
                OrderId = Guid.Parse("00000000-0000-0000-0000-000000000001"),
                OrderLineNumber = 1,
                Description = "ChessSet"
            }
        }
    }
}.AsQueryable();

var projection = orders.ProjectTo<OrderModel>(Configuration);
var orderModel = projection.First(); //throws

[lbargaoanu]

My branch is way ahead of master, any change it's guaranteed to have merge conflicts. I think this should wait.

[lbargaoanu]

Meanwhile you can skip the null check with DoNotAllowNull, unless it's your null check that's causing the issue.

[Erythnul]

@lbargaoanu Thanks for your response. I understand the merge issues. Setting AllowNullDestinationValues to false still generates and invalid expression though:

OrderId = (Guid?)((dtoLet.MostImportantOrderLine != null) ? dtoOrder : null).Id

Yes, my null check is causing an issue, but I do believe that this is an actual bug in the expression evaluation. This scenario is an abstraction of my actual code that will be running in production at some point in the future. The actual code makes more sense, especially considering a legacy database with exotic structures in it, but this code was an easy way to reproduce with recognizable class names.

Waiting for 11.0, while not ideal, is okay if there is no other choice. I see the entire ExpressionBuilder class has been (re)moved in your branch, so I'm not sure where these changes would land in the future. I could have a look around in your branch to see if the issue is already fixed or easily fixable, if that's okay with you?

[lbargaoanu]

You can PR my branch and then you'll be able to use the MyGet build. Let's start with a failing test in the integration tests project.

[Erythnul]

@lbargaoanu I've created a new PR with a failing integration test and then the fix in a separate commit, so you can verify.

Thanks for your time!

[lbargaoanu]

Try the MyGet build.

[github-actions]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.