1

Should all linq queries which return an entity be surrounded with a try catch?

For example:

phase.Container = containers.Where(cont => cont.ContainerId == phase.ContainerId).SingleOrDefault();

This will not cause the application to crash, but will cause an exception if the Container object is not present.

EDIT

Even doing this will still cause the view to have an exception:

bool schedValid = false;
foreach (var s in schedules)
{
 if (s.ScheduleId == phase.ScheduleId) schedValid = true;
}
if (schedValid)
{
 phase.Schedule = schedules.SingleOrDefault(sc => sc.ScheduleId == phase.ScheduleId);
}
else
{
 phase.Schedule = null;
}

Edit 2

The reason for all this effort is that if a parent object is deleted then the children objects which contain data should still be allowed to be viewed even though their parent is gone. An argument can be made that the parent object should not be allowed to be deleted. Fair enough, but there is no point of keeping it around when the children have become more relevant than the parent and for whatever reason the parent no longer is required to be kept. At a later time, the children may be re-assigned to a different parent object.

Travis J
  • 81,153
  • 41
  • 202
  • 273
  • What exception does it cause? – mellamokb Mar 23 '12 at 19:49
  • 2
    This doesn't answer your question, but you could write the same thing shorter as `phase.Container = containers.SingleOrDefault(cont => cont.ContainerId == phase.ContainerId);` – M.Babcock Mar 23 '12 at 19:52
  • @mellamokb - The problem arises when phase has a foreign key which was valid at one point, but is no longer valid due to the associated record of Container being removed from the database. In this case, the exception throw is an objectcontext dispose exception. – Travis J Mar 23 '12 at 19:58
  • Seems like if you're getting with foreign key restraints, you've got problems that no amount of try/catch will solve. I would look for the root of the foreign key problems, and try to fix it there. In normal circumstances, a SingleOrDefault shouldn't cause any problems. – Eric Andres Mar 23 '12 at 20:12

3 Answers3

3

This is a response to the question in the title, not the specific example given in the body of the question.

"It depends"

What are you going to do when the exception happens? Can you reasonably respond a database error? If not, don't catch the exception. If you can reasonable respond in such a way that allows the rest of the application to continue execution, then catch the exception.

See this great post from Eric Lippert for more details about exception handling.
http://blogs.msdn.com/b/ericlippert/archive/2008/09/10/vexing-exceptions.aspx

cadrell0
  • 17,109
  • 5
  • 51
  • 69
1

The only exception I could see being thrown by your code snippet (assuming it compiles) is an NRE. If phase or containers are null, or if containers contains null values, you will end up with a NullReferenceException.

I would anticipate those in code though to avoid having to use a try/catch by checking their values explicitly before use:

if (phase != null && containers != null)
    phase.Container = containers
                        .Where(cont => cont != null)
                        .SingleOrDefault(cont => cont.ContainerId == phase.ContainerId);

Exceptions are expensive... do whatever you can to avoid them.

UPDATE

Given the fact that you're current design intentionally orphans records and you can't really trust your database, I'd recommend using try/catch blocks around all of your code...

A bad database/application design requires equally bad code.

The right answer here is to fix your design.

M.Babcock
  • 18,753
  • 6
  • 54
  • 84
  • `'((new System.Collections.Generic.Mscorlib_CollectionDebugView(vm.Phases)).Items[56]).Container' threw an exception of type 'System.ObjectDisposedException'` Is the exception thrown when the Container selected is not present, but there are containers in the list. – Travis J Mar 23 '12 at 20:05
  • Is that saying that `containers` was disposed or `phase` was disposed? – M.Babcock Mar 23 '12 at 20:08
  • The suggested change you make here still throws the same exception. – Travis J Mar 23 '12 at 20:09
  • The exception is a little odd, because the lists were already generated. Once the list has been generated it expects this association to exist. When it finds that there is a valid foreign key in phase for a container, but that the container list does not have that container, it wants to re-query the database. This is where the exception is thrown. – Travis J Mar 23 '12 at 20:10
  • The question referenced by Peekay is a plausible explanation for the exception and offers a possible way to avoid this exception. Use `containers.ToList().Where(...` to eager load the contents of your collection. – M.Babcock Mar 23 '12 at 20:24
  • Not sure how else to say this because I have said so many times that the foreign object is missing...The contents of the collection are complete, there is an object *phyiscally missing* from the database that **cannot** be loaded in that it does not exist. Is this a common problem? No. Can it happen? Yes, it is an extreme outlier but it still must be handled. – Travis J Mar 23 '12 at 20:40
  • I got it now, sorry for sending you on a wild goose chase. If you can't trust your data source, then yes you will have to use try/catch more than I would generally suggest. Long term, I'd look into a database redesign to avoid these sorts of issues. Any database that knowingly orphans valuable records is problematic. – M.Babcock Mar 23 '12 at 20:58
  • I wouldn't say it is orphaning records though. Let me explain. Once mature (after 5 years), the database will have many records. Using employees as an example, the database doesn't need to have a list of thousands of employees when there are only currently 100 working. These employees should be able to be excluded from the database without causing records they are a part of to be deleted. Those records may still be of use, and if not, then those may be removed at convenience as well. – Travis J Mar 23 '12 at 21:24
  • But you can't trust your foreign key, so it is orphaning the records. You'll either need to fix your key issues or use a different mechanism to retrieve the records. – M.Babcock Mar 23 '12 at 21:26
  • The problem is with the belief that all records should be kept. Wells Fargo Payroll has this fallacy and it makes their service awful. Same with the Hartford who cannot seem to find anything in the swamp of crap they keep around. I do not think that every single piece of data should be kept for all of eternity just because it has been used in other places. Is this a problem in design? Yes, industry wide. – Travis J Mar 23 '12 at 21:26
  • The foreign keys indicated that there was a record present at one time. I could have the foreign object then track weather or not it was useful at one point and then indicate that no, it is no longer useful and has been shallow deleted. This would still keep a large amount of useless data around. – Travis J Mar 23 '12 at 21:28
  • You need a database trigger to clear your key when the referenced record has been deleted. Then just be sure to ignore the records who's keys are null. – M.Babcock Mar 23 '12 at 21:29
  • Clearing these records will still cause an exception in the view. lol. EF *expects* this record to be complete, EF: "how else could it have been committed?" -> EF: "It must need to be lazily loaded, I better go get it from the objectcontext" -> exception. – Travis J Mar 23 '12 at 21:47
1

Well, it'll throw an exception, as long as you manage your exceptions somewhere down the stack, then you probably don't need to wrap every LINQ you use.

You could handle common exceptions that can be thrown by LINQ with default/broad message and handle more precises error around your LINQ.

Here are the list of known exceptions that can be thrown taken from this question :

  • SqlException
  • ChangeConflictException
  • DuplicateKeyException
  • ForeignKeyReferenceAlreadyHasValueException
  • OutOfMemoryException (when not correctly disposing the DataContext)

Known that these errors will be thrown, you can most likely find a bottle-neck where it'd be interesting to catch those errors and deal with them accordingly, if it's a User Application, then maybe showing an Error Box with some friendly text to describe the type of error (Ex: Foreign Key Exception means that the record is still being used somewhere).

If you know a precise LINQ expression is giving you trouble, then you can single handley Wrap it with a Try/Catch and manage it accordingly.

Community
  • 1
  • 1
Alexandre
  • 4,382
  • 2
  • 23
  • 33
  • The expression causes a possible error in this situation - one which is not listed in your list: objectcontext disposed. Left unhandled, it would be possible in the future for it to arise. "If you know a precise LINQ expression is giving you trouble, then you can single handley Wrap it with a Try/Catch and manage it accordingly." Does this imply that I should have to wrap every occurance of this expression with a try catch? – Travis J Mar 23 '12 at 20:15
  • 1
    The error you are getting should and can be avoided. In this case, a Try/Catch is the wrong way to deal with the issue because it can be avoided. I'm not familiar with LINQ enough to help you, but I'd like to redirect you to this SO [Question](http://stackoverflow.com/questions/4206634/system-objectdisposedexception-the-objectcontext-instance-has-been-disposed-and), hopefully you'll be able to fix it! – Alexandre Mar 23 '12 at 20:20
  • I am actually including all properties correctly, which is what the link entails (and implies to use of .Include() I believe). The objectcontext disposed exception is strange, because even if the objectcontext was not disposed, there is no way that a deleted object would be able to be somehow magically created. – Travis J Mar 23 '12 at 20:23
  • It's very hard to tell what is happening given we only have 2 lines of code. I would encourage you to ensure all propreties are loaded and not tied, it seems very clear the exception is thrown because of DataContext not being done loading, which can be avoided by waiting for it to finish loading. As I said, my LINQ knowledge is very limited, if you still think Try/Catch is the way to go, then you should ask yourself what would you do when the exception is thrown? How do you handle the exception? Etc... – Alexandre Mar 23 '12 at 20:29
  • All objects are loaded. One object cannot be loaded in that it does not exist, anywhere. – Travis J Mar 23 '12 at 20:44