1
\$\begingroup\$

Given a DbSet<Client> from an Entity Framework 6 DbContext, I need to count the number of related Tickets of each type: Open, Responded, and Resolved.

The below code works, and results in only one query (according to DbContext.Database.Log), as desired. However, the fact that the call to GroupBy results in an IGrouping<Client, ICollection<Ticket>>, which is also an IEnumerable<ICollection<Ticket>>, requires me to call FirstOrDefault(), even though there is always only one ICollection<Ticket>.

I've never used GroupBy before, so I'm willing to bet I'm just misusing it.

public IEnumerable<DashboardItemViewModel> GetItems(IQueryable<Client> clients)
{
    return clients
        .Where(cli => cli.IsActive)
        .GroupBy(cli => cli, cli => cli.Tickets)
        .Select(grp => new DashboardItemViewModel
        {
            Id = grp.Key.Id,
            Name = grp.Key.Name,
            Open = grp.FirstOrDefault().Count(t => t.Status == StatusType.Open && !t.IsArchived),
            Responded = grp.FirstOrDefault().Count(t => t.Status == StatusType.Responded && !t.IsArchived),
            Resolved = grp.FirstOrDefault().Count(t => t.Status == StatusType.Resolved && !t.IsArchived),
        });
}

public enum StatusType
{
    Open,
    Responded,
    Resolved,
};

public class DashboardItemViewModel
{
    public int Id { get; set; }
    public string Name { get; set; }
    public int Open { get; set; }
    public int Responded { get; set; }
    public int Resolved { get; set; }
}

public class Client
{
    public int Id { get; set; }
    public string Name { get; set; }
    public bool IsActive { get; set; }
    public virtual ICollection<Ticket> Tickets { get; set; }
}

public class Ticket
{
    public int Id { get; set; }
    public StatusType Status { get; set; }
    public bool IsArchived { get; set; }
    public virtual Client Client { get; set; }
}
\$\endgroup\$
  • \$\begingroup\$ @tinstaafl I don't see how that's relevant. \$\endgroup\$ – Sinjai Jan 24 at 0:14
  • \$\begingroup\$ @tinstaafl Obviously my question is unclear. What can I explain? "How to retrieve a single result" and "counting related records with GroupBy" seem like very different things. \$\endgroup\$ – Sinjai Jan 24 at 0:19
  • \$\begingroup\$ The below code works -- Really? Doesn't GroupBy(cli => cli, cli => cli.Tickets) thow an exception? That would mean that clients is not an IQueryable originating from a DbSet but actually is an IEnumerable. Both with EF6 and EF-core, grouping by a collection should throw an exception. BTW, please indicate which EF version this is. \$\endgroup\$ – Gert Arnold Jan 24 at 16:14
  • \$\begingroup\$ @GertArnold Really. Wouldn't it only be grouping by a collection if a collection is the key? It's EF6, I'll edit the post when I get a chance. \$\endgroup\$ – Sinjai Jan 24 at 16:18
  • \$\begingroup\$ Yes you're right, I missed the cli => cli. \$\endgroup\$ – Gert Arnold Jan 24 at 16:22
2
\$\begingroup\$

You don't need the grouping at the Client level. No grouping at all, actually. Don't worry, this is a common mistake when people want grouping/aggregation in child collections. Per client, the Tickets can be counted:

return clients
    .Where(cli => cli.IsActive)
    .Select(cli => new DashboardItemViewModel
    {
        Id = cli.Id,
        Name = cli.Name,
        Open = cli.Tickets.Count(t => t.Status == StatusType.Open && !t.IsArchived),
        Responded = cli.Tickets.Count(t => t.Status == StatusType.Responded && !t.IsArchived),
        Resolved = cli.Tickets.Count(t => t.Status == StatusType.Resolved && !t.IsArchived),
    });

Or in query syntax, so we can benefit from the let statement:

return from cli in clients
    where cli.IsActive
    let activeTickets = cli.Tickets.Where(t => !t.IsArchived)
    select new DashboardItemViewModel
    {
        Id = cli.Id,
        Name = cli.Name,
        Open = activeTickets.Count(t => t.Status == StatusType.Open),
        Responded = activeTickets.Count(t => t.Status == StatusType.Responded),
        Resolved = activeTickets.Count(t => t.Status == StatusType.Resolved)
    };
\$\endgroup\$
  • \$\begingroup\$ Makes sense, thanks. Do you have any idea if SQL Server will optimize those counts, i.e. loop once (like this rather than once for each StatusType (like this)? If not, there's the possibility of doing the counting myself, rather than asking the database to... Also, is the benefit of let just clarity? \$\endgroup\$ – Sinjai Jan 24 at 19:10
  • \$\begingroup\$ I can't see your links, but let doesn't improve the generated SQL. It's for clarity and for preventing repetitive code. The generated SQL will contain three COUNT subqueries. In a similar query I see that each subquey is executed separately. If there's a performance issue you could try to improve that by fetching an anon. type with StatusTypes = activeTickets.Select(t => tStatusType) and do grouping/counting of StatusTypes in memory. Can't tell if that will be any better. \$\endgroup\$ – Gert Arnold Jan 24 at 20:04
  • \$\begingroup\$ ... If there are many tickets per client the amount of data going over the wire may be significantly larger than a (maybe) slower counting query returning 3 numbers per client. \$\endgroup\$ – Gert Arnold Jan 24 at 20:07
  • \$\begingroup\$ Sorry about that, not sure what's wrong with hastebin. The links were just examples to make sure what I was talking about was clear, but I think it's safe to say you understand. Thanks for your help, I'll go ahead and mark this as the solution. \$\endgroup\$ – Sinjai Jan 24 at 20:45
1
\$\begingroup\$

Your code does not work the way you think it does.

.GroupBy(cli => cli, cli => cli.Tickets)

This is not grouping anything because you are using the entire object as a key and since you are not providing any comparer for them each group will have exactly one item, itself.

.Select(grp =>

Then from each one-itemed-group you select its single item with

grp.FirstOrDefault().Count(t => t.Status == StatusType.Open

This is probably very inefficient because the grouping isn't grouping anything and works like a SELECT * FROM Table.

Instead you should rather be doing this. Select tickets from active clients, group them by their Status, calculate their Count and then create the view-model.

    var ticketGroups = tickets
        .Where(ticket => ticket.Client.IsActive)        
        .GroupBy(ticket => ticket.Status, tickets => tickets.Count())
        .ToList();

    return new DashboardItemViewModel
    {
        Id = grp.Key.Id,
        Name = grp.Key.Name,
        Open =  ticketGroups.SingleOrDefault(tg => tg.Key == StatusType.Open && !tg.IsArchived),
        Responded = ticketGroups.SingleOrDefault(tg => tg.Status == StatusType.Responded && !tg.IsArchived),
        Resolved = ticketGroups.SingleOrDefault(tg => tg.Status == StatusType.Resolved && !tg.IsArchived),
    });
\$\endgroup\$
  • \$\begingroup\$ Your code doesn't use a DbSet<Client> at all and doesn't return an IEnumerable<DashboardItemViewModel>. Could you please update your answer or explain your reasoning? \$\endgroup\$ – Sinjai Jan 24 at 14:47

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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