0
\$\begingroup\$

I have the SQL Products table migrated from MongoDb and I use ASP Core + MediatR. The filtering data comes from frontend Autocompletes where user can select multiple values (3 different ProductGroups + 2 different Suppliers for example). Wanted to ask is my implementation optimal, or did I miss something? Below the code for action:

    [HttpPut("[action]")]
    public async Task<IQueryable<ArticleDto>> Put([FromBody] ProductsQueryAll query)
    {
        var response = await _mediator.Send(query);
        return response;
    }

And below the code for MediatR model and Handler:

 public class ProductsQueryAll : IRequest<IQueryable<ArticleDto>>
    {
        public string[] ProductGroups { get; set; }
        public int[] SupplierIds { get; set; }
        public string[] Categories { get; set; }
    }

    public class ProductsQueryAllHandler : IRequestHandler<ProductsQueryAll, IQueryable<ArticleDto>>
    {
        private readonly DbContext _dbContext;
        private readonly IMapper _mapper;

        public ProductsQueryAllHandler(DbContext dbContext, IMapper mapper)
        {
            _dbContext = dbContext;
            _mapper = mapper ?? throw new ArgumentNullException(nameof(mapper));
        }

        public Task<IQueryable<ArticleDto>> Handle(ProductsQueryAll request, CancellationToken cancellationToken)
        {
            var predicate = PredicateBuilder.True<ArticleDm>();
            if (request.ProductGroups?.Length > 0)
            {
                predicate = predicate.And(x => request.ProductGroups.Any(y => y == x.ProductGroup));
            }
            if (request.SupplierIds?.Length > 0)
            {
                predicate = predicate.And(x => request.SupplierIds.Any(y => y == x.SupplierId));
            }
            if (request.Categories?.Length > 0)
            {
                predicate = predicate.And(x => request.Categories.Any( y => y == x.Category));
            }


            var entityList = _dbContext.Articles
                .AsNoTracking()
                .Where(predicate);

            return Task.FromResult(_mapper.ProjectTo<ArticleDto>(entityList));
        }
    }
\$\endgroup\$
5
  • 2
    \$\begingroup\$ What SQL does it generate? Look at that for possible inefficiencies. And can't this be done by simply using IQueryable, i.e. no need for PredicateBuilder? Also, "ProductsQueryAll" is IMHO a bad name. It looks like a bunch of words jammed together, plus it isn't true: you're not getting "all" since the request contains arrays you use as a filter, and you're getting articles not products. Why not "GetFilteredArticles(Request/Handler)"? \$\endgroup\$
    – BCdotWEB
    Commented Jul 26, 2022 at 11:01
  • \$\begingroup\$ do you have similar filtering requirement on other controllers ? or is it just going to be used on this action only ? \$\endgroup\$
    – iSR5
    Commented Jul 26, 2022 at 11:46
  • \$\begingroup\$ @iSR5 this kind of filtering only on this action \$\endgroup\$ Commented Jul 26, 2022 at 11:49
  • 2
    \$\begingroup\$ @user2771704 then I don't think you need to complicate things, a simple service method with the proper arguments, would be enough, and working with simple IQueryable would be enough as well. use PredicateBuilder if you have complex query. \$\endgroup\$
    – iSR5
    Commented Jul 26, 2022 at 11:56
  • \$\begingroup\$ I see, iSR5 and BCdotWEB thanks for you comments \$\endgroup\$ Commented Jul 26, 2022 at 12:17

1 Answer 1

1
\$\begingroup\$

Small syntax improvements

Foo?.Length > 0 can always be refactored to Foo?.Any()

x => request.SupplierIds.Any(y => y == x.SupplierId) can be somewhat improved by using
x => request.SupplierIds.Contains(x.SupplierId)

I don't particularly like the name ProductsQueryAll. ProductsQuery seems more appropriate. "All" implies that you're going to get all of them, which is not the case since you're allowing filters.
That being said, there may be circumstances that I'm not aware of that help explain why your name could make sense, e.g. to disambiguate it from something very similar.

I'm aware that this is not something everyone agrees on, but I find the _ prefix for private fields superfluous, and simply use camelcasing (context, mapper). That being said, you might be subjected to an existing coding style, at which point I do agree that conformity to the rest of the existing codebase and development team style is more important.

Query logic

Because of the nature of your filtering logic, you could've omitted the predicate builder and instead chained your Where calls directly. I tend to structure this kind of logic like so:

var query = _dbContext.Articles.AsQueryable();

if (request.ProductGroups?.Any())
    query = query.Where(x => request.ProductGroups.Contains(x.ProductGroup));

if (request.SupplierIds?.Any())
    query = query.Where(x => request.SupplierIds.Contains(x.SupplierId));

if (request.Categories?.Any())
    query = query.Where(x => request.Categories.Contains(x.Category));

Returning an IQueryable

This is something you should avoid at all costs. Do not return IQueryable. Your return type should be IEnumerable.

Also note that it is easy to still return an IQueryable even when your return type is IEnumerable (since IQueryable : IEnumerable), but you should actively avoid this. This is done by enumerating your IQueryable, most commonly done via ToList or ToListAsync.

Building this off of my previous suggestion:

public async Task<IEnumerable<ArticleDto>> Handle(ProductsQueryAll request, CancellationToken cancellationToken)
{
    // see previous snippet

    return await query
                   .ProjectTo<ArticleDto>(mapper.ConfigurationProvider)
                   .ToListAsync();
}

Small things to point out:

  • Since you're working asynchronously, you should make this method actually asynchronous so you can benefit the most from it.
  • ProjectTo has two signatures. You used mapper.ProjectTo(query) (oversimplified), but I prefer query.ProjectTo(mapper) (oversimplified) because it's more consistent with the rest of the syntax being used here, because it's always of the form query.Method(...).
\$\endgroup\$
6
  • \$\begingroup\$ Thank you very much for you answer. A little bit confused about avoiding IQueryable. Because in many articles I saw something like So if you working with only in-memory data collection IEnumerable is a good choice but if you want to query data collection which is connected with database IQueryable is a better choice as it reduces network traffic and uses the power of SQL language.` Also often can see statements that IQueryable is more performant (please see webdevelopment.co.nz/post/…) \$\endgroup\$ Commented Jul 28, 2022 at 7:08
  • \$\begingroup\$ So I thought if I load data from big table I thought that for performance better send query to database with IQueryable than use in memory filtering with IEnumerable. Or I get this concept wrong? \$\endgroup\$ Commented Jul 28, 2022 at 7:12
  • \$\begingroup\$ @user2771704: You are correct about the benefits of not loading all data and filtering/sorting/projecting it in-memory; but the handling of an IQueryable should be done only on your persistence layer (DAL), and it should cease to be an IQueryable when it leaves that layer. I surmise that the party who sent the Mediatr request is not part of your DAL, and therefore it should not receive an IQueryable as the result of the request. \$\endgroup\$
    – Flater
    Commented Jul 28, 2022 at 7:18
  • \$\begingroup\$ @user2771704: The problem with an IQueryable is that it is an implementation leak. You can only properly handle an IQueryable when you have concrete knowledge of what data store you're using. The DAL exists specifically to encapsulate the data store knowledge; so that the rest of the application doesn't depend on the specific data storage method. Therefore, only the DAL should be handling a "raw" IQueryable, and by the time the DAl returns its data to a not-DAL consumer, it should've enumerated the IQueryable so that the consumer doesn't have to interact with the IQueryable. \$\endgroup\$
    – Flater
    Commented Jul 28, 2022 at 7:23
  • \$\begingroup\$ Thanks for your comments. I used IQueryable because of Using some additional filtering from Odata with [EnableQuery] attribute in the controller. I will think how to handle this scenario better if possible (probably handle binding manually from ODataRoute). \$\endgroup\$ Commented Jul 28, 2022 at 7:29

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.