Posts tagged with 'aspect'
It's the last part of the series, and I just thought I would wrap things up by talking about unit testing the aspect. I wrote a whole chapter in my book AOP in .NET on unit-testing of aspects. I also was honored to do a live webinar with Gael Fraiteur on unit testing with PostSharp, in which he has a different view on aspect unit testing, so I recommend you check that out too.
I'm not straying too much from the book in this example, so if you're familiar with that chapter, there's not much new for you here.
My plan is to not test the PostSharp aspect directly, but instead have the aspect delegate just about all the functionality to a plain old C# class that's easier to test. The old adage of solving a problem by adding another layer of indirection applies. Doing it this way makes my aspect easier to test, further decouples my application from a specific framework, but does require some more work, so it's not like it's a magic cost-free approach to testing either.
Here is the aspect, all tightly coupled to PostSharp:
And here is my refactored code, which you'll notice results in a higher number of classes, but it is ready to unit test. You may also notice that some of the logic is a bit different than the above aspect. This is because I found some issues and bugs while I was writing unit tests. So, hooray for unit tests! Also notice the "On" static field. I touch on this in the book, but this flag is merely there so that I can unit test the actual services while isolating them from the aspect. Since PostSharp uses a compile-time weaver, this is necessary. If you are using a runtime AOP weaver like Castle DynamicProxy, that's not necessary.
I'll even show you some of my unit tests, which I've written in mspec and JustMock Lite with the help of Sticking Place for testing the SqlException stuff.
Notice that I moved the Exceptionless-specific code behind an interface (IExceptionLogger) as well, to further decouple, improve testability, and also because Exceptionless is just the sort of thing that might actually be swapped out somewhere down the line. I've left out the details of that implementation because it's quite trivial.
So now you are caught up to today: this is the aspect I'm actually using and deploying to a staging site for my cofounder and our designer to use and test. And so far I must say that it's working quite well, but things could always change when it actually hits real customers, of course.
I'm starting to wind down this blog post series. A couple more tweaks and you'll be all caught up with how I'm using this aspect in a system that I'm actively working on right now.
But first, let me address some of the criticism that I've received in comments, reddit, and so on.
Mainly, the criticism about my aspect making the return value be an empty collection (instead of null) if the return type is a collection. I totally expected this criticism, and I definitely think it's fair. I pondered this idea a lot myself before I came to the conclusion that I did, and it's entirely possible that I'll change my mind later. So, if you are following along, and thinking about creating an aspect like this, you should definitely think carefully about doing this; don't just copy and paste my aspect and call it a day.
I think it comes down to a choice: do I want to always be checking for nulls, or when there's a collection can I just assume that it will (at worst) be empty? I based my decision somewhat on the Framework Design Guidelines - my API should return an empty collection (or array) instead of nulls. My thought process was also that this aspect is responsible for the return value being null in the first place (because it's handling the exception and not bubbling it up), and therefore it should make a reasonable attempt at not disrupting normal program flow by returning an empty collection.
This is an opinionated approach, and it may not mesh with what you are trying to do, you or your team's coding style, and where you intend to use this aspect.
Okay, on with the show.
The aspect I've written is able to send an error message up through the stack and eventually to the UI (in my application right now, that's either MVC ModelState or a display element in my Telerik Reporting reports, but it could easily be other UI elements too). Here's the relevant bit of code:
While I'm developing, that's great. I get the exception message and it helps me pinpoint the error. There's no stack trace, but you could add that too if you wanted (or check it out on Exceptionless logs). But there's a problem: I do not want my users (especially any malicious ones) to see detailed error messages like that. So, just add in some configuration and logic to show (or not show) a verbose error message. It's a piece of cake, and it really isn't something that's specific to AOP (other than to show that AOP code can be configured just like any other part of your code). I also want to refactor this to only use Exceptionless when in production: I don't need to log any exceptions that I create locally when I'm in the process of writing code.
Notice that I kinda hand-waved the configuration stuff away into the staic class/method IsThisSite.RunningLocally? The RunningLocally code is simply a check of Web.config (or App.config) using ConfigurationManager.AppSettings. How you manage your configurations is up to you and your team.
Also notice that GetErrorMessage could be customized to show different error messages depending on the exception (or not).
Okay, so I think this aspect is in pretty good shape. It's helpful, cuts down on boilerplate, and implements a very important cross-cutting concern requirement (error handling/logging). But, a) you shouldn't have to take my word for it that it works, and b) what if I want to refactor it? It should be unit tested (which is the next and final post).
In the last post in the series, I moved the error handling aspect into an opinionated one by using Exceptionless and handling certain types of exceptions (like SqlException and NotImplementedException).
In this post, I'm going to go even further down the opinionated road and talk about exception recovery.
My goal is for any exception to be caught and logged, and we're already there. In addition, I'd like the user not to see a yellow screen, so I'm generally not allowing the exception to bubble up, in lieu of a nice error message being put into ModelState (or some other error handling). However, this might not be enough. If the service method throwing an exception has a return value, then it's likely that another error will happen later in execution (maybe due to a null reference). For example:
If the method doesn't throw an exception, everything is fine. If it does throw an exception, there will a null reference when I attempt to use the Count property. What I would prefer is that an empty collection is returned. In fact, this fits with the recommendations from Framework Design Guidelines (page 256) to always return an empty collection (or array) instead of null. I've added a helper method to construct the return value in case this occurs.
What about if the service method is not returning a collection? And what if it's returning a value (like int or DateTime)? It's okay to return a null in the first case. In the second case, it might be okay to throw an exception. I have so few of these in the project I'm working on that I don't worry about it much, but for the sake of completeness, let's just make it return the default value.
I need to be very cautious about what I'm doing, and wrap everything in a try/catch so that I don't end up causing another unhandled exception in my exception handling code. It would be a good idea to create some unit tests to make sure everything is behaving as expected.
Okay, I think one or two more posts will wrap it up for this series. We need to discuss whether or not the exception message should be shown to a user (hopefully you're leaning towards "no").
- This only works for ITerritoryService
- Swallowing exceptions: we should at least be logging the exception, not just ignoring it.
- If the method has a return type, then it's returning null. If it's meant to return a reference type, that's probably fine, but what if it's returning a value type?
- What if it's returning a collection, and the UI assumes that it will get an empty collection instead of a null reference?
- Do we really want to return the exception message to the user?
- What if I forget to use ServiceBase as a base class on a new service?
- Unit tests: how to test this aspect in isolation.
In the last post, I added some compile time validation to make sure that the aspect is being used in the correct places. Today, we're going to look at actually logging the exception.
Remember early on when I said that this aspect is not generally applicable? For this part, it's especially true, since I'll be using a specific logging library + service: Exceptionless.
The application that I'm working on is meant to be deployed to an Azure web site. Because of this, I have to think a little differently about logging. You can still log to the file system, of course, but on a cloud service that may not be the best idea. You could log to table storage on Azure Storage. You could also log to a normal database. However, I wanted to use something that: a) requires very little work, b) provides some useful monitoring tools that I don't have to build, c) is very low cost or free. So, based on my research and an endorsement from Mark Greenway, I decided to explore Exceptionless.
Using Exceptionless is as simple as installing a NuGet package, pasting in your API key, and then using an extension method.
Here's how I could use Exceptionless in my aspect:
This is very similar to what I did initially, but since this project is in development, I made a few adjustments along the way to make my life easier.
SQL exceptions. Especially during development, it's very likely that I'll screw up a SQL query, forget to run a migration script, etc. Additionally, since I'm using constraints in my database, I want to handle the case where a user is trying to delete an entity and a constraint is preventing them. So I modified the aspect some more:
The way I'm checking for "constraint" exceptions feels very hacky to me, but I haven't found a better way yet.
NotImplementedException. Again, since I'm still in active development, often times a NotImplementedException will pop up. For these exceptions, I want them to crash right away, and I also don't need to worry about logging.
Now that SQL exceptions and not-implemented exceptions are handled on their own, any other exception is one that I still want to log and output to the UI in a friendly, non-yellow screen way. During development, I definitely want to see the full exception message, but that's something that will definitely need to be revisited before going into production. Back to the list:
- This only works for ITerritoryService
- Swallowing exceptions: we should at least be logging the exception, not just ignoring it. <- We are now logging exceptions!
- If the method has a return type, then it's returning null. If it's meant to return a reference type, that's probably fine, but what if it's returning a value type?
- What if it's returning a collection, and the UI assumes that it will get an empty collection instead of a null reference?
- Do we really want to return the exception message to the user? <- We'll revisit this later.
- What if I forget to use ServiceBase as a base class on a new service?
In the last post, we started using ServiceBase as a way to more generally address the services and use the callback to report errors to the UI.
What happens if I put the aspect on a method in a class that doesn't inherit from ServiceBase (because either I forgot, or I put an aspect on a class outside of the service layer by mistake)? Exceptions will be swallowed, not reported, and probably cause issues elsewhere in the project.
One approach would be to change the case from "args.Instance as ServiceBase" to "(ServiceBase)args.Instance". At least then, I'll get a runtime error about casting (but only if there's an exception). With PostSharp, we can do better than that.
In fact, remember when I said that PostSharp aspects have to be marked as [Serializable]? This is because, as compile time, PostSharp instantiates those aspect objects and serializes them to be used later at runtime. BUT, since the aspect is being instantiated anyway, PostSharp also does a couple of other optional steps: it can do some compile-time initialization (to avoid runtime reflection, for instance), and it can do some compile-time validation.
That's right, we can put some code in the aspect that can check to make sure that we're applying the aspect correctly. If we aren't, it will give us a compile-time error. We could check, for instance, that the aspect is being applied to a method in a class that inherits ServiceBase.
This code will only be executed at compile-time. If the declaring type (the class that the method is in) does not inherit from ServiceBase, then PostSharp will write out an error at compile time:
So now if I ever forget to use ServiceBase, or I use this aspect in the wrong place, I'll know right when I'm compiling. And because of this validation, I can also confidently change the cast from "args.Instance as ServiceBase" to "(ServiceBase)args.Instance", knowing that if the project compiles, that cast will never fail.
How are we doing:
- This only works for ITerritoryService--what about the other services?
- Swallowing exceptions: we should at least be logging the exception, not just ignoring it.
- If the method has a return type, then it's returning null. If it's meant to return a reference type, that's probably fine, but what if it's returning a value type?
- What if it's returning a collection, and the UI assumes that it will get an empty collection instead of a null reference?
- Do we really want to return the exception message to the user?
- What if I forget to use ServiceBase as a base class on a new service? <- We just took care of this
Next time we'll look at actually logging the exception.