74

I have a repository class that wraps my LINQ to SQL Data Context. The repository class is a business line class that contains all the data tier logic (and caching and such).

Here's my v1 of my repo interface.

public interface ILocationRepository
{
    IList<Location> FindAll();
    IList<Location> FindForState(State state);
    IList<Location> FindForPostCode(string postCode);
}

But to handle paging for FindAll, I'm debating whether or not to expose IQueryable<ILocation> instead of IList to simplify the interface for circumstances such as paging.

What are the pros and cons to exposing IQueryable from the data repo?

Any help is very much appreciated.

CVertex
  • 17,997
  • 28
  • 94
  • 124
  • 8
    How about using IEnumerable in repository interface? – Konstantin Tarkus Apr 05 '09 at 10:19
  • @KonstantinTarkus I'm not for using `IEnumerable` in the Repository or BLL because it may affect the performance in case you didn't pay attention and made many foreach loops to the same list in that case you'll call `GetEnumerator()` many times. – Wahid Bitar Jul 22 '14 at 19:40

3 Answers3

90

The pros; composability:

  • callers can add filters
  • callers can add paging
  • callers can add sorting
  • etc

The cons; non-testability:

  • Your repository is no longer properly unit testable; you can't rely on a: it working, b: what it does;
    • the caller could add a non-translatable function (i.e. no TSQL mapping; breaks at runtime)
    • the caller could add a filter/sort that makes it perform like a dog
  • Since callers expect IQueryable<T> to be composable, it rules out non-composable implementations - or it forces you to write your own query provider for them
  • it means you can't optimize / profile the DAL

For stability, I've taken to not exposing IQueryable<T> or Expression<...> on my repositories. This means I know how the repository behaves, and my upper layers can use mocks without worrying "does the actual repository support this?" (forcing integration tests).

I still use IQueryable<T> etc inside the repository - but not over the boundary. I posted some more thoughts on this theme here. It is just as easy to put paging parameters on the repository interface. You can even use extension methods (on the interface) to add optional paging parameters, so that the concrete classes only have 1 method to implement, but there may be 2 or 3 overloads available to the caller.

Marc Gravell
  • 1,026,079
  • 266
  • 2,566
  • 2,900
  • @Marc, what would you tell about IEnumerable for repository methods? – Konstantin Tarkus Apr 05 '09 at 10:18
  • 5
    If used appropriately, `IEnumerable` would be OK - but mainly for large data. For regular queries, I'd prefer a closed set (array/list/etc). In particular, I'm using MVC at the moment, and I want the **controller** to fetch all data - not defer it until the view - else you can't fully test... – Marc Gravell Apr 05 '09 at 10:37
  • 2
    ...the controller, as you haven't actually proven it fetching data (since `IEnumerable` is usually used with deferred execution). If the repo returns IList (or similar), then you **know** you have got the data. – Marc Gravell Apr 05 '09 at 10:38
  • 1
    @Mark, and.. what's wrong with firing an executin within unit test? Ex.: `Assert.IsTrue(result.Any())` – Konstantin Tarkus Apr 05 '09 at 10:43
  • 2
    With IQueryable, that changes the actual query. With IEnumerable, less so - but still: getting data is the responsibility of the controller, not the view. – Marc Gravell Apr 05 '09 at 12:04
  • 1
    On your last statement about optional paging parameters, and 2 or 3 overloads available to the caller, could you point us to an article on this? – Shawn Mclean Mar 27 '11 at 04:09
  • 1
    Just to add to this, returning IQueryable would seem to break the entire idea of having a repository would it not? You are creating an architectural seam that depends on a data store that properly implements IQueryable. What if for some reason you had to move to a different storage model that didn't play nice with IQueryable at all? An example would be a project I had to port to run on Mono a while back. I had to use old school ADO. If my repos had used IQueryable I'd have had to re-factor application logic because it would have been actually building my queries with LINQ. – CatDadCode Feb 24 '12 at 19:27
  • 1
    3 years late, but I prefer exposing `IQueryable` for as long as possible. The controller can filter what it needs to down further, and the view can filter further if needed, or even reduce the fields it needs from what the controller provides it. Perhaps I don't need every field from the Person class in the view. Why would I want the repository to retrieve them all to put in a Person class when I only really wanted their name? As for testing, the depends on what you are trying to test. You can still test that the repository will return data as requested. – Robert McKee Nov 17 '15 at 20:39
  • Doing an OrderBy/Take/Skip in the view is still possible even on an IList, it is just much less efficient, so it doesn't really help unit test the view, but unless you are writing unit tests for your views, it never tested them anyhow. – Robert McKee Nov 17 '15 at 20:42
  • IQueryable does not prevent testability at all. – Matthew Whited Jul 28 '17 at 13:58
  • @MatthewWhited it doesn't prevent *integration* tests, but it *absolutely* is not suitable for *unit* tests using things like mocks or `AsQueryable()`; by the nature of `IQueryable`, the caller *cannot know* which queries will be supported, and any in-memory mock / etc is going to use the *most generous* implementation, i.e. LINQ-to-Objects (which is *far* more permissive than things like EF or LINQ-to-SQL etc) – Marc Gravell Jul 28 '17 at 16:46
  • `.AsQueryable()` is useless but there is nothing stopping you from checking the Expression trees used in the predicates and so on. IQueryable and the composability of queries far out weight the limitations of IEnumerable/IList/ICollection based respositories. (Which themselves have similar issues with unit testing as you are not testing how the application is actually functioning. – Matthew Whited Jul 28 '17 at 17:05
  • @Matthew checking expression trees is useful if you're checking the compiler or the Queryable extension methods. Usually, though, that isn't a useful test. – Marc Gravell Jul 28 '17 at 17:31
7

As mentioned by previous answer, exposing IQueryable gives access to callers to play with IQueryable itself, which is or it can become dangerous.

Encapsulating business logic's first responsibility is to maintain integrity of your database.

You can continue exposing IList and may be change your parameters as following, this is how we are doing...

public interface ILocationRepository
{
    IList<Location> FindAll(int start, int size);
    IList<Location> FindForState(State state, int start, int size);
    IList<Location> FindForPostCode(string postCode, int start, int size);
}

if size == -1 then return all...

Alternative way...

If you still want to return IQueryable, then you can return IQueryable of List inside your functions.. for example...

public class MyRepository
{
    IQueryable<Location> FindAll()
    {
        List<Location> myLocations = ....;
        return myLocations.AsQueryable<Location>;
        // here Query can only be applied on this
        // subset, not directly to the database
    }
}

First method has an advantage over memory, because you will return less data instead of all.

Akash Kava
  • 39,066
  • 20
  • 121
  • 167
  • 1
    This is not so elegant. This way CVertex must add those parameters (start, size) to EVERY repository method he creates - very bad programming practice. – twk Apr 05 '09 at 09:39
  • Well its better then exposing Data Base Interface which can modify and spoil your data. – Akash Kava Apr 05 '09 at 09:44
  • Only needs to add the paging parameters where they're actually used. And if they're used, its not wasted effort. – Frank Schwieterman Apr 21 '09 at 18:16
  • 2
    Ouch, your second method would be an app killer! It would pull all results into memory and then perform LINQ on them. I never have found a real reason to use `AsQueryable` to be honest. I'm not even sure why it exists. It doesn't suddenly turn your List into an IQueryable and allow you to build expression trees that get sent to your data store, so what's the point? – CatDadCode Feb 24 '12 at 19:32
  • I agree, I wrote this long time back with little knowledge of how EF worked, I didn't mean to load all data in memory. – Akash Kava Feb 24 '12 at 20:24
  • 1
    @AlexFord `AsQueryable` is useful when you get a non-generic `IEnumerable` from a legacy API. – joshperry Feb 13 '13 at 18:14
  • Ah, I suppose that makes sense. – CatDadCode Feb 14 '13 at 16:47
2

I recommend using IEnumerable instead of IList, with it you will have more flexibility.

This way you will be able to get from Db only that portion of data which you really gonna use without extra work done in your repository.

Sample:

// Repository
public interface IRepository
{
    IEnumerable<Location> GetLocations();
}

// Controller
public ActionResult Locations(int? page)
{
    return View(repository.GetLocations().AsPagination(page ?? 1, 10);
}

Which is super clean and simple.

Konstantin Tarkus
  • 37,618
  • 14
  • 135
  • 121
  • Why? What are the trade-offs? – Richard Apr 05 '09 at 09:18
  • 1
    IList FindAll() is not very efficient if you're going to display a paged list to the user (let's say 1000 rows will be fetched from db, but only 10 of them will be shown). With IQueryable you won't have such an issue. – Konstantin Tarkus Apr 05 '09 at 09:22
  • BTW, if you're going to implement paging functionality take a look at existing helper classs in MvcContrib = http://www.codeplex.com/mvccontrib/ (MvcContrib.Pagination namespace) – Konstantin Tarkus Apr 05 '09 at 09:24
  • I am fond of creating verbose repository methods. e.g. `GetRecords(int page, int itemsPerPage)`. Rather than passing the paging off to the controllers. – CatDadCode Feb 24 '12 at 19:35