UserStore.FindByIdAsync should use Users property rather than UsersSet

[thomaslevesque]

Most methods in UserStore use the Users property, which can be overridden in derived classes. FindByIdAsync, however, uses UsersSet directly, which cannot be overridden.

This can be an issue if the TUser class has navigation properties and the DbContext doesn't have lazy-loading enabled, because there isn't a single place where the necessary Includes can be added on the DbSet. The Users property can be overridden like this:

public override IQueryable<ApplicationUser> Users => Context.Users.Include(u => u.Whatever);

which will affect most methods, but not FindByIdAsync, so I have to override that as well:

public override Task<ApplicationUser> FindByIdAsync(string userId, CancellationToken cancellationToken = new CancellationToken())
{
    cancellationToken.ThrowIfCancellationRequested();
    this.ThrowIfDisposed();
    var databaseId = this.ConvertIdFromString(userId);
    return Context.Users
        .Include(u => u.Whatever)
        .SingleOrDefaultAsync(m => m.Id == databaseId, cancellationToken);
}

This is inconsistent and a bit annoying... Couldn't FindByIdAsync be based on Users like the other methods?

[HaoK]

This is because FindById is using FindAsync which is only available on DbSet. So you will have to override FindById to change what this method does. This was originally done for performance reasons I think. @ajcvickers is this a pattern that is preferred for faster key lookups? This was a change I made a long time ago at Diego's request.

[ajcvickers]

@HaoK Find looks to see if the entity is already being tracked first, and returns it without executing a query if it is found. Otherwise it executes the query. Mostly this is just perf, but it can behave differently if the entity has not been saved to the database yet, since in that case the database query would not return it. As long as that's not a concern I think it would not be unreasonable to change this to use a query instead of Find.

[thomaslevesque]

I see, thanks for the explanation. Anyway, I gave up on the idea of eagerly loading associations from the UserStore, it was causing unexpected side effects.

[HaoK]

Ok closing this issue for now then