How can I tell why/how some LINQ queries work with each other and others don't?

Question

Take the following EF Class:

    public class Person
    {
        public int ID { get; set; }
        public string FirstName { get; set; }
        public string LastName { get; set; }
        public IEnumerable<Property> Property { get; set; }
    }

    public class Property
    {
        public int ID { get; set; }
        public string Name { get; set; }
        public bool Lock { get; set; }
        public Person Person { get; set; }
        public int PersonID
    }

I can pretty much make everything work as expected - including a delete action for Person that also deletes all their property. However, as my code gets more complicated, I want to make the logic slightly more advanced.

In the above example, we have something elsewhere that will set the bool lock for property. In this case, I want to disable delete on person when any property for that person has a lock of true.

The default Delete controller code has:

public async Task<IActionResult> Delete(int? id)
    {
        if (id == null)
        {
            return NotFound();
        }

        var person = await _context.People
            .FirstOrDefaultAsync(m => m.ID == id);
        if (person== null)
        {
            return NotFound();
        }

        return View(person);
    }

And the Delete confirm has:

   public async Task<IActionResult> DeleteConfirmed(int id)
   {
       var person= await _context.people.FindAsync(id);
       _context.people.Remove(person);
       await _context.SaveChangesAsync();
       return RedirectToAction(nameof(Index));
   }

I know the code to do what I want is:

foreach (var item in person.Property)
{
    if item.locked==true
        return("error")
}

Now the fun stars! - The old EF4 virtual keyword on properties I'm used to doesn't work - so, I can't iterate over the property because it is currently null. in most instances, I have to call .include()

On the first delete, this modifies:

    var person = await _context.People
        .FirstOrDefaultAsync(m => m.ID == id);

to

    var person = await _context.People.Include(x=>x.property)
        .FirstOrDefaultAsync(m => m.ID == id);

which seems to work fine.

However, the second one:

    var person = await _context.people.FindAsync(id);

doesn't seem to work. The moment I put the .Include in, it states error CS1061 that there is no definition for FindAsync.

In all honesty, I am not too sure what the need is for two different ways of looking at an ID in the first place... I can only assume that when looking for an ID in the first delete that may not exist, firstordefault is the best and when confirming a delete, find is best.... however, this is what the scaffolding does and I don't feel I know enough to question this.

I however want to be a better developer and would love to understand what is wrong with the code and for future, how do I know what can be combined and what can't as I don't feel I am learning here, I am just randomly trying different things until I find one combination that works.

Answer 1

A few things: I'd consider checking whether the person is Locked before enabling a Delete button, or immediately on clicking the delete button rather than on confirming a delete.

With this code:

var person = await _context.People
   .FirstOrDefaultAsync(m => m.ID == id);
if (person== null)
    return NotFound();

return View(person);

Entities should represent data state, not view state. Returning entities to the view will lead to problems. If lazy loading is supported/enabled this can trigger performance issues when lazy-loads get triggered by serialization, it can also lead to errors due to cyclical references. It exposes more information about your data structure, and data in general that the client does not need (more data over the wire and more information for hackers). Instead, leverage a ViewModel POCO class containing just the data your view needs and use .Select() to populate it.

Next, avoid the crutch of FirstOrDefault. This can lead to unintended bugs remaining hidden. If there is 1 entity expected, use Single or SingleOrDefault. Your application should handle exceptions gracefully and impartially. If someone sends an invalid ID, fail and terminate the session. Basically do not trust the client not to tamper.

var person = await _context.People
   .Select(x => new PersonViewModel
   {
       PersonId = x.ID,
       Name = x.FirstName + " " + x.LastName,
       // etc.
   }).SingleAsync(x => x.ID == id);

return View(person);

When checking data state on the confirm, you receive and ID and want to confirm before issuing the delete, you can query the required detail, but then for a delete you don't need the entire entity provided you trust the ID. Something like this isn't needed:

foreach (var item in person.Property)
{
    if item.locked==true
        return("error")
}

Instead:

var isLocked = context.People.Where(x => x.ID == id)
    .Select(x => x.Property.Any(p => p.isLocked))
    .Single();

This will throw if the person ID isn't found, and return a Bool True of False if any of the Property entries for that person are locked.

From there you can use a simple "trick" to delete an entity without first loading it:

if (!isLocked)
{
   var person = new Person { ID = id };
   context.People.Attach(person);
   context.People.Remove(person);
   context.SaveChanges();
}

Alternatively if you want to load the Person to have access to other properties, such as to create an audit record or may want to display info anyways as part of the error message, then you can substitute the above examples with:

var personData = context.People.Where(x => x.ID == id)
    .Select(x => new 
    {
       Person = x,
       IsLocked = x.Property.Any(p => p.isLocked))
    }).Single();

if (!personData.isLocked)
{
   context.People.Remove(personData.Person);
   context.SaveChanges();
}