Code review for an abstract repository implementation

I have a repository abstract class that encapsulates pretty much all of the CRUD functionality:

public abstract class DataRepository<T> : IRepository<T>
where T : class
{
    public DataContext Context { get; private set; }
    public TransactionScope Transaction { get; private set; }

    /// <summary>
    /// A <see cref="bool"/> function that compares the keys for fetching a single item, for example: 
    /// return item1.Id == item2.Id (as an anonymous delegate).
    /// </summary>
    public Func<T, T, bool> KeyCompare { get; private set; }

    /// <summary>
    /// Creates a new data repository.
    /// </summary>
    /// <param name="context"></param>
    /// <param name="scope"></param>
    /// <param name="keyCompare">
    /// A <see cref="bool"/> function that compares the keys for fetching a single item, for example: 
    /// return item1.Id == item2.Id (as an anonymous delegate).
    /// </param>
    public DataRepository(DataContext context, TransactionScope scope, Func<T, T, bool> keyCompare)
    {
        Context = context;
        Transaction = scope;
        KeyCompare = keyCompare;
    }

    public virtual T Item(T item)
    {
        return Items().SingleOrDefault(e => KeyCompare(e, item));
    }

    public virtual IEnumerable<T> Items()
    {
        return DataTable.AsEnumerable();
    }

    protected virtual Table<T> DataTable { get { return Context.GetTable<T>(); } }

    /// <summary>
    /// A method that updates the non-key fields of an existing entity with those of specified <see cref="item"/>.
    /// Called by the <see cref="Save"/> method.
    /// </summary>
    /// <param name="existing">The existing record to update.</param>
    /// <param name="item">A <see cref="T"/> object containing the values to update <see cref="existing"/> object with.</param>
    /// <returns></returns>
    protected abstract void UpdateExisting(T existing, T item);

    /// <summary>
    /// A method that updates an existing item or creates a new one, as needed.
    /// </summary>
    /// <param name="item">The entity containing the values to be saved.</param>
    public virtual void Save(T item)
    {
        var existing = Item(item);
        if (existing != null)
        {
            UpdateExisting(existing, item);
        }
        else
        {
            DataTable.InsertOnSubmit(item);
        }

        Context.SubmitChanges();
    }

    /// <summary>
    /// A method that saves all specified items (creates new, updates existing).
    /// </summary>
    /// <param name="items">The entities to be saved.</param>
    public virtual void Save(IEnumerable<T> items)
    {
        foreach (var item in items)
        {
            Save(item);
        }
    }

    /// <summary>
    /// A method that deletes specified item.
    /// </summary>
    /// <param name="item"></param>
    public virtual void Delete(T item)
    {
        var existing = Item(item);
        if (existing != null)
        {
            DataTable.DeleteOnSubmit(existing);
        }

        Context.SubmitChanges();
    }

    public virtual void Delete(IEnumerable<T> items)
    {
        var selection = Items().Where(e => items.Any(item => KeyCompare(e, item)));
        DataTable.DeleteAllOnSubmit(selection);

        Context.SubmitChanges();
    }
}

The KeyCompare property is used like this in the derived classes, so that the base class knows how to isolate a single item in the repository (not all "entities" have a "Id" property, and some keys span multiple columns - this solution attempts to resolve that particular point):

public AuthInfoRepository(DataContext context, TransactionScope scope)
    : base(context, scope, (item1, item2) => { return item1.Id == item2.Id;})
{ }

This KeyCompare property is really the cornerstone that allows the derived classes to merely implement the UpdateExisting method, like this:

protected override void UpdateExisting(AuthInfo existing, AuthInfo item)
{
    existing.AuthId = item.AuthId;
    existing.ActiveDirectoryGroup = item.ActiveDirectoryGroup;
}

The rest (actual CRUD) is all handled by the base class. With this abstract repository I have been implementing the concrete ones in minutes if not seconds, writing only the code that is specific to each implementation. Finest DRY I've ever written.

The DataRepository deals with SQL Server, so I needed yet another implementation for mocking, which I've called ListRepository and does pretty much exactly the same thing (except Context and Transaction properties both return null). I think the constructor's signature is all I need to post here:

public ListRepository(IEnumerable<T> items, Func<T, T, bool> keyCompare)

So for testing, I can bind IReporitory<T> to this ListRepository while production code binds to DataRepository:

Bind<IRepository<AuthInfo>>().To<ListRepository<AuthInfo>>()
    .WithConstructorArgument("items", _mockAuthInfo)
    .WithConstructorArgument("keyCompare", (Func<AuthInfo, AuthInfo, bool>)((item1, item2) => item1.Id == item2.Id));

While this is all great (just about to start unit testing it), there are a couple points that I'd still like to address:

• Should I externalize the calls to Context.SubmitChanges()? I don't like that the method gets called at every iteration, both when saving and when deleting multiple items.
• While this is certainly great DRY, I'm worried the virtual methods violate YAGNI - I don't see me overriding any of those in any implmentation in the near future. Then the class doesn't need to be abstract and could still provide a virtual implementation of UpdateExisting, but that would make implementing the class less obvious than the abstract method. Where's the line drawn?

UPDATE

Given the great answer I got, I reworked the repository interface, looking at Linq to Entities' ObjectSet for an inspiration:

public interface IRepository<T> where T : class
{
    /// <summary>
    /// A function that compares all key fields of two <see cref="T"/> items to determine equality.
    /// </summary>
    Expression<Func<T, T, bool>> KeyCompare { get; }

    /// <summary>
    /// Gets all items in the repository (without fetching them).
    /// </summary>
    /// <returns></returns>
    IQueryable<T> Select();

    T Item(T itemKeys);

    /// <summary>
    /// Gets the number of items held in the repository.
    /// </summary>
    int Count { get; }

    /// <summary>
    /// Adds specified item to the repository. Updates item if it already exists.
    /// </summary>
    /// <param name="item">The item to save.</param>
    void Save(T item);

    /// <summary>
    /// Adds specified items to the repository. Updates existing items.
    /// </summary>
    /// <param name="items">The items to save.</param>
    void Save(IEnumerable<T> items);

    /// <summary>
    /// Removes specified item from repository.
    /// </summary>
    /// <param name="item"></param>
    void Delete(T itemKey);

    /// <summary>
    /// Removes specified items from repository.
    /// </summary>
    /// <param name="items"></param>
    void Delete(IEnumerable<T> itemKeys);
}

And then I managed to get KeyCompare to work with LinqToSQL, by implementing an ExpressionVisitor:

public class PartialApplier : ExpressionVisitor
{
    private readonly ConstantExpression value;
    private readonly ParameterExpression replace;

    private PartialApplier(ParameterExpression replace, object value)
    {
        this.replace = replace;
        this.value = Expression.Constant(value, value.GetType());
    }

    public override Expression Visit(Expression node)
    {
        var parameter = node as ParameterExpression;
        if (parameter != null && parameter.Equals(replace))
        {
            return value;
        }
        else return base.Visit(node);
    }

    public static Expression<Func<T2,TResult>> PartialApply<T,T2,TResult>(Expression<Func<T,T2,TResult>> expression, T value)
    {
        var result = new PartialApplier(expression.Parameters.First(), value).Visit(expression.Body);

        return (Expression<Func<T2,TResult>>)Expression.Lambda(result, expression.Parameters.Skip(1));
    }
}

This way I could use Expression<Func<T, T, bool>> KeyCompare the way I intended it:

public abstract class RepositoryBase<T> : IRepository<T>
    where T : class
{
    public abstract IQueryable<T> Select();
    public abstract void Save(T item);
    public abstract void Delete(T item);
    public abstract void Delete(IEnumerable<T> items);

    public virtual Expression<Func<T, T, bool>> KeyCompare { get; private set; }
    public virtual int Count { get { return Select().Count(); } }

    protected RepositoryBase(Expression<Func<T, T, bool>> keyCompare)
    {
        KeyCompare = keyCompare;
    }

    public virtual T Item(T item)
    {
        var applied = PartialApplier.PartialApply(KeyCompare, item);
        var result = (Select().SingleOrDefault(applied));

        return result;
    }

    /// <summary>
    /// A method that updates the non-key fields of an existing entity with those of specified <see cref="item"/>.
    /// Called by the <see cref="Save"/> method.
    /// </summary>
    /// <param name="existing">The existing record to update.</param>
    /// <param name="item">A <see cref="T"/> object containing the values to update <see cref="existing"/> object with.</param>
    /// <returns></returns>
    protected abstract void UpdateExisting(T existing, T item);

    /// <summary>
    /// A method that saves all specified items.
    /// </summary>
    /// <param name="items">The entities to be saved.</param>
    public virtual void Save(IEnumerable<T> items)
    {
        foreach (var item in items)
        {
            Save(item);
        }
    }
}

The ListRepository was no issue, but needed a bit of reworking to properly derive from RepositoryBase:

/// <summary>
/// A non-persistent repository implementation for mocking the repository layer.
/// </summary>
/// <typeparam name="T"></typeparam>
public class ListRepository<T> : RepositoryBase<T>
    where T : class
{
    private readonly List<T> _items = new List<T>();

    public ListRepository(IEnumerable<T> items, Expression<Func<T, T, bool>> keyCompare)
        : base(keyCompare)
    {
        _items.AddRange(items);
    }

    public override IQueryable<T> Select()
    { 
        return _items.AsQueryable(); 
    }

    public override void Save(T item)
    {
        var existing = Item(item);
        if (existing != null)
        {
            _items.Remove(item);
        }

        _items.Add(item);
    }

    public override void Save(IEnumerable<T> items)
    {
        foreach (var item in items)
        {
            Save(item);
        }
    }

    public override void Delete(T item)
    {
        _items.Remove(item);
    }

    public override void Delete(IEnumerable<T> items)
    {
        foreach (var item in items)
        {
            Delete(Item(item));
        }
    }

    /// <summary>
    /// Not implemented.
    /// </summary>
    /// <param name="existing"></param>
    /// <param name="item"></param>
    protected override void UpdateExisting(T existing, T item)
    {
        throw new NotImplementedException(); // list repo just wipes existing data
    }
}

And then the DataRepository could look like this:

public abstract class DataRepository<T> : RepositoryBase<T>
    where T : class
{
    private DataContext _context;

    public DataRepository(DataContext context, Expression<Func<T, T, bool>> keyCompare)
        : base(keyCompare)
    {
        _context = context;
    }

    public override IQueryable<T> Select()
    {
        return _context.GetTable<T>().AsQueryable();
    }

    /// <summary>
    /// A method that updates an existing item or creates a new one, as needed.
    /// </summary>
    /// <param name="item">The entity containing the values to be saved.</param>
    public override void Save(T item)
    {
        var existing = Item(item);
        if (existing != null)
        {
            UpdateExisting(existing, item);
        }
        else
        {
            _context.GetTable<T>().InsertOnSubmit(item);
        }

        _context.SubmitChanges();
    }

    /// <summary>
    /// A method that deletes specified item.
    /// </summary>
    /// <param name="item"></param>
    public override void Delete(T item)
    {
        var existing = Item(item);
        if (existing != null)
        {
            _context.GetTable<T>().DeleteOnSubmit(existing);
        }

        _context.SubmitChanges();
    }

    public override void Delete(IEnumerable<T> items)
    {
        foreach (var item in items)
        {
            Delete(item);
        }
    }

    protected override void UpdateExisting(T existing, T item)
    {
        throw new NotImplementedException(); // must override in derived classes - not obvious I know
    }
}

So, given a DataContext with only a simple object (called DataRepositoryIntegrationTest in this case), I could create a concrete implementation to test it all out:

public class TestRepository : DataRepository<DataRepositoryIntegrationTest>
{
    public TestRepository(DataContext context)
        : base(context,  (item1, item2) => item1.Id == item2.Id)
    { }

    protected override void UpdateExisting(DataRepositoryIntegrationTest existing, DataRepositoryIntegrationTest item)
    {
        existing.Value = item.Value;
        existing.DateUpdated = DateTime.Now;
    }
}

And all the tests pass! Are there any issues I'm not seeing? The TransactionScope was moved to unit of work abstraction level and the DataContext only gets created if it's a DataRepository asking for it. Everything just feels right at home to me!