9

Many of the bugs I've been fixing lately are a result of null references when accessing navigation properties of objects loaded using entity framework. I believe there must be a flaw in how I'm designing my methods. Here's an example...

A Task contains Many Roles, each Role references a User.

public class Role
{
    public int Id;
    public int User_Id;
    public string Type;
}

public class User
{
    public int Id
    public string Name;
}    

public class Task
{
    public int Id;
    public string Name;
    public string Status;
    public List<Role> Roles;
}

Considering that I would have queried my context like this by mistake and not loaded User...

var task = context.Tasks.Include(x=>x.Roles).FirstOrDefault;

And then I call this method...

public void PrintTask(Task task)
{
    Console.WriteLine(task.Name);
    Console.WriteLine(task.Status);

    foreach(var r in task.Roles)
    {
        Console.WriteLine(r.User.Name); //This will throw NRE because User wasn't loaded
    }
}

I may have built this method with every intention to load Roles and User but next time I use it I may forget that I need both. Ideally the method definition should tell me what data is necessary, but even if I pass in both Task and Roles, I'm still missing Roles->User.

What's the proper way to reference these relationships and be sure that they're loaded in something like this print method? I'm interested in a better design, so "Use Lazy Loading" isn't the answer I'm looking for.

Thanks!

EDIT:

I know I can load the task like this...

var task = context.Tasks.Include(x=>x.Roles.Select(z=>z.User)).FirstOrDefault();

What I want to know is how do I design my method so that when I come back and use it 6 months from now I know what data needs to be loaded in my entities? The method definition doesn't indicate what is necessary to use it. Or how to I block against these NullReferences. There has to be a better design.

BZink
  • 7,106
  • 10
  • 34
  • 54

3 Answers3

2

You can use the Select extension method to eager load Users.

var task = context.Tasks.Include(x => x.Roles)
             .Include(x => x.Roles.Select(r => r.User))
             .FirstOrDefault();

Edit:

There are few ways that I can think of to avoid the NRE

  • Integration test using a SQL Server CE/Express database. Unit testing with fake contexts will not work correctly.
  • Loading the entities close to where they are consumed. So that the Includes are near to where the entities are used.
  • Passing DTOs/ViewModels to the upper layers without passing the entities.
Eranga
  • 31,383
  • 5
  • 88
  • 92
  • Sorry, I didn't make it clear that I knew how to load the user. And in fact you can do that in one Include statement, not two. What I'm dealing with is using methods where it's not apparent what references are needed. So if I do load the entity wrong, and use the method, I get the null reference. – BZink Dec 20 '11 at 02:19
1

User should be lazily loaded in your loop—just note though that this is a classic select N + 1 problem that you should fix with another Include.

I think the root problem is either that this particular Role doesn't have a User, or that this particular Role's User has null set for its Name. You'll need to check both for null in your loop

foreach(var r in task.Roles)
{
    if (r.User != null)
        Console.WriteLine(r.User.Name ?? "Name is null"); 
}
Community
  • 1
  • 1
Adam Rackis
  • 79,454
  • 49
  • 255
  • 377
1

Very good question. Here are some possible solutions that, while they don't enforce the avoidance of NREs, they'll provide clues to the caller that they need to Include things:

The first option is to not have your method access a non-guaranteed property of an entity; rather, force the caller to pass both entities:

public void PrintTask(Task task, User taskUser)
{
    // ...
}

Another option is to name the parameter of your method such that it will clue the caller as to what is required:

public void PrintTask(Task taskWithUser)
{
    // ...
}
Jacob
  • 72,750
  • 22
  • 137
  • 214
  • Thanks. I think you validated my thoughts that there was no pure solution to this. I try to keep property access as simple as possible, and pass them individually when possible. – BZink Dec 21 '11 at 15:28