Handle parameterized projection in $filter predicate mapping expressions

[orty]

Hi,

I discovered an issue when trying to use this library in the following scenario:

with a mapping configured as follows

this.CreateMap<MyDbModel, MyApiModel>()
    .ForMember(
        dest => dest.IsFavorite,
        opt => opt.MapFrom(
            src => src.Favorites.Any(
                fav => userId.HasValue && fav.UserId == userId.Value)))
....

where userId is injected as a projection settings, like so:

AutoMapper.AspNet.OData.QueryableExtensions.GetQuery(
                query,
                mapper,
                oDataQueryOptions,
                new QuerySettings()
                {
                    ProjectionSettings = new ProjectionSettings()
                    {
                        Parameters = new { userId = this.UserId!.Value }
                    }
                }));

even though the value of userId can be retrieved in the final projection of the results, in cannot be used within a predicate, for example, if we issue the following OData query:

api/myapimodel?$filter=isFavorite eq true

The reason for it, as it appeared to me, is that the value of userId is not injected along with the AutoMapperConfig.<>DisplayClass(...).userId closure in the f predicate here: https://github.com/AutoMapper/AutoMapper.Extensions.OData/blob/68ff04d5d8caf40c3ee887fb694383af375de7f9/AutoMapper.AspNetCore.OData.EFCore/QueryableExtensions.cs#L152C8-L156

I did this very quick and dirty fix, and tested it in our own environment. I did not want to duplicate AutoMapper logic for injecting projection parameters, so I did it the reflection way. Please, feel free to redirect me to a prefered method if you need to, but I would really appreciate your feedback on this.

Thanks

[BlaiseD]

What happens if you filter after mapper.ProjectTo here. Maybe there's simpler fix if you start with the way ProjectTo would work by itself and proceed from there?

[BlaiseD]

You'd also want to add a test which fails before the fix and passes after the fix.

[orty]

Thanks for your advice. I will try to do the filter the other way around.

Edit: Unfortunately, as I was worried about, the filter cannot be applied after the projection because then the expression cannot be translated to SQL. The predicate is being applied to the projected object instead of the source DbSet.

How do you recommend me to implement a test which would fail without the fix ?

[BlaiseD]

Thanks for your advice. I will try to do the filter the other way around.

Edit: Unfortunately, as I was worried about, the filter cannot be applied after the projection because then the expression cannot be translated to SQL. The predicate is being applied to the projected object instead of the source DbSet.

How do you recommend me to implement a test which would fail without the fix ?

We update the expression after projection here for filtering and sorting of expansions. Maybe consider something similar which would execute in this scenario only.

The test is the same as for all tests - fails in the current version and works with the new code.

[orty]

Thanks for spotted the UpdateQueryableExpression method, it made me think of another test, that I did, and realized unfortunately that even for $expanded properties, custom projection settings are not applied when calculating the projection.

Hence, with the scenario I gave in my first comment, if you have another entity:

public class MyOtherApiModel
{
    ... 
    public MyApiModel TheMyApiModel { get; set; }
    ...
}

and run api/myotherapimodel?$expand=theMyApiModel the isFavorite property will always be false.

The fact is, as soon as the userId.HasValue && fav.UserId == userId.Value projection expression is evaluated, it results in a constant false value (because userId is null in this context), which then results on SQL side in a 1 = 0 expression (or something similar, to express a falsy value).

I will try to take a closer look to how the projection is calculated in the UpdateQueryableExpression to see if an equivalent fix can be made, and then think about a cleaner fix than the one I originally proposed.

Sorry to ask again, but I do not understand how I should implement a test which fails "before the fix". Should I issue a first PR with only the new unit test demonstrating my issue, for you to run your validation pipelines on it and see for yourself that it does not work as is, or is it something else ?

Again, many thanks for your time.

[BlaiseD]

Does code like the following solve your use case?

        private static IQueryable<TModel> GetQuery<TModel, TData>(this IQueryable<TData> query,
            IMapper mapper,
            Expression<Func<TModel, bool>> filter = null,
            Expression<Func<IQueryable<TModel>, IQueryable<TModel>>> queryFunc = null,
            IEnumerable<Expression<Func<TModel, object>>> includeProperties = null,
            ProjectionSettings projectionSettings = null)
        {
            Expression<Func<TData, bool>> f = mapper.MapExpression<Expression<Func<TData, bool>>>(filter);
            Func<IQueryable<TData>, IQueryable<TData>> mappedQueryFunc = mapper.MapExpression<Expression<Func<IQueryable<TData>, IQueryable<TData>>>>(queryFunc)?.Compile();

            if (filter != null)
                query = query.Where(f);

            IQueryable<TModel> modelQuery = mappedQueryFunc != null
                    ? mapper.ProjectTo(mappedQueryFunc(query), projectionSettings?.Parameters, GetIncludes())
                    : mapper.ProjectTo(query, projectionSettings?.Parameters, GetIncludes());

            if (projectionSettings?.Parameters != null && filter != null)
                modelQuery = modelQuery.Where(filter);

            return modelQuery;

            Expression<Func<TModel, object>>[] GetIncludes() => includeProperties?.ToArray() ?? new Expression<Func<TModel, object>>[] { };
        }

By a test I mean a unit test like one of these. It should fail against v4.0.1 and pass with your new code.

[orty]

Unfortunately, it does not, I just tried this exact implementation with our use case and what happens is that:

1. upon evaluation of Expression<Func<TData, bool>> f = mapper.MapExpression<Expression<Func<TData, bool>>>(filter); the f expression is written using the closures from the AutoMapper.Mapper parameters.
2. when the query = query.Where(f); expression evaluates, the closure has no value provided, hence, in our case, as we safely check if our parameter .HasValue, the predicate which uses the closure is evaluated to false.

Running another .Where clause on the query after that adds the proper condition to the SQL predicate, but the AND (@__TypedProperty_1 = CAST(0 AS bit))) (with @__TypedProperty_1 = 1) part just falsifies the whole SQL query. Let me issue another pull request with the unit test you requested.

If I were to guess, I would say that the proper place to put the change would be into the Mapper.MapExpression method itself, by allowing it to accept the projection parameters as an optional argument.

[BlaiseD]

Filtering is recommended before projection. Looks like you're confusing constants in expressions with projection parameters which happens later. If you do need to filter after projection you'll have to add a flag to the ProjectionSettings to exclude the first filter and use the second one.

The tests in the other PR fail because the values are set here. The query settings come later and will not apply to the filter (which works against the database classes not the projected one.)

[orty]

Did I misunderstood the point of projection parameters ? When using the "vanilla" AutoMapper library, the currentUserFavoriteCategory variable, for instance, is translated into the SQL query from its value provided in the projection parameters. This allows to express a DTO property calculated from column(s) in DB and a contextual value, I thought this was what the projection parameters were for.

Anyway, even if my example (which does not fully represent my business case) is closer from a filtering function, when it comes to only express such a DTO's calculated property, your OData extension is completely able to perform the translation at first level, but fails when inside an $expand. I would like to have the same level of functionalities over both the $filter and $expand query options as I do with plain AutoMapper.

[BlaiseD]

I think we've gone about this backwards. Better to open an issue with a sample we can run and see fail. A link to a failing test in your repository is probably the easiest way.