Posts tagged with 'postsharp'
Peter Lorimer has built the ASPeKT AOP framework. This episode is sponsored by Uncall.
Show Notes:
-
Other AOP frameworks discussed:
-
We use a bit of AOP terminology. If you need a refresher, check out the terminology posts that cover weaving, cross cutting concern, aspect, advice, pointcut, and join point
-
Book: Adaptive Code via C# by Gary McLean Hall
-
Mono.Cecil is not an AOP framework, but it is a tool to manipulate IL (aka MSIL aka CIL)
-
Unparalleled Adventure blog by Peter Lorimer
Want to be on the next episode? You can! All you need is the willingness to talk about something technical.
Want to be a sponsor? Check out my sponsorship gig on Fiverr
Welcome to another "Weekly Concerns". This is a post-a-week series of interesting links, relevant to programming and programmers. You can check out previous Weekly Concerns posts in the archive.
- 5 Things You Should Stop Doing With jQuery. It's a sensational title, but read all the way to the end before you judge.
- PostSharp 3.2 Preview 2 is available, with improvements that help you write multi-threaded code (among other things).
- Direct casting vs "as" casting in C#
- Check out the new and improved WordPress Code Reference.
If you have an interesting link that you'd like to see in Weekly Concerns, leave a comment or contact me.
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:
[Serializable] | |
[MulticastAttributeUsage(MulticastTargets.Method, TargetMemberAttributes = MulticastAttributes.Public)] | |
public class ServiceErrorInterceptor : MethodInterceptionAspect | |
{ | |
public override bool CompileTimeValidate(MethodBase method) | |
{ | |
if (!typeof (ServiceBase).IsAssignableFrom(method.DeclaringType)) | |
{ | |
var declaringType = method.DeclaringType; | |
var declaringTypeName = "Unknown"; | |
if (declaringType != null) | |
declaringTypeName = declaringType.FullName; | |
Message.Write(method, SeverityType.Error, "SEI01", "The target type (" + declaringTypeName + ") does not implement ServiceBase."); | |
return false; | |
} | |
return base.CompileTimeValidate(method); | |
} | |
public override void OnInvoke(MethodInterceptionArgs args) | |
{ | |
try | |
{ | |
args.Proceed(); | |
} | |
catch (SqlException ex) | |
{ | |
var obj = (ServiceBase) args.Instance; | |
if (ex.Message.Contains("The DELETE statement conflicted with the REFERENCE constraint")) | |
obj.AddValidationError("This item can't be deleted because it is still used by other items."); | |
else | |
{ | |
obj.AddValidationError(GetErrorMessage(ex)); | |
if (!IsThisSite.RunningLocally) | |
ex.ToExceptionless().Submit(); | |
} | |
ErrorReturn(args); | |
} | |
catch (NotImplementedException) | |
{ | |
// if something's not implemented, just throw it up | |
throw; | |
} | |
catch (Exception ex) | |
{ | |
if (!IsThisSite.RunningLocally) | |
ex.ToExceptionless().Submit(); | |
var obj = (ServiceBase)args.Instance; | |
obj.AddValidationError(GetErrorMessage(ex)); | |
ErrorReturn(args); | |
} | |
} | |
private string GetErrorMessage(Exception ex) | |
{ | |
if(IsThisSite.RunningLocally) | |
return "There was a database error. Please try again later. (" + ex.Message + "), (" + ex.GetType().FullName + ")"; | |
return "There was a database error. Please try again later."; | |
} | |
void ErrorReturn(MethodInterceptionArgs args) | |
{ | |
try | |
{ | |
var methodInfo = args.Method as MethodInfo; | |
if (methodInfo == null) | |
return; | |
if (typeof (IEnumerable).IsAssignableFrom(methodInfo.ReturnType) || methodInfo.ReturnType.IsValueType) | |
if (methodInfo.ReturnType != typeof (void)) | |
args.ReturnValue = Activator.CreateInstance(methodInfo.ReturnType); | |
else | |
args.ReturnValue = null; | |
} | |
catch | |
{ | |
// don't want any errors to take place trying to build the correct return value | |
// so just swallow them, and let args.ReturnValue stay whatever it is | |
} | |
} | |
} |
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.
[Serializable] | |
[MulticastAttributeUsage(MulticastTargets.Method, TargetMemberAttributes = MulticastAttributes.Public)] | |
public class ServiceErrorInterceptor : MethodInterceptionAspect | |
{ | |
[NonSerialized] | |
ServiceErrorConcern _concern; | |
public static bool On = true; | |
public override bool CompileTimeValidate(MethodBase method) | |
{ | |
if (!typeof (ServiceBase).IsAssignableFrom(method.DeclaringType)) | |
{ | |
var declaringType = method.DeclaringType; | |
var declaringTypeName = "Unknown"; | |
if (declaringType != null) | |
declaringTypeName = declaringType.FullName; | |
Message.Write(method, SeverityType.Error, "SEI01", "The target type (" + declaringTypeName + ") does not implement ServiceBase."); | |
return false; | |
} | |
return base.CompileTimeValidate(method); | |
} | |
public override void RuntimeInitialize(MethodBase method) | |
{ | |
if (!On) return; | |
// I'm newing up directly instead of using StructureMap/IoC | |
// because my project structure would require some extra hoops | |
// but I plan to do that work when it makes sense | |
// so think of this as a technical debt decision | |
_concern = new ServiceErrorConcern(new ExceptionlessLogger()); | |
} | |
public override void OnInvoke(MethodInterceptionArgs args) | |
{ | |
if (!On) | |
{ | |
args.Proceed(); | |
return; | |
} | |
_concern.OnInvoke(new MethodConcernArgs(args)); | |
} | |
} |
public interface IMethodConcernArgs | |
{ | |
void Proceed(); | |
object Instance { get; } | |
MethodBase Method { get; } | |
object ReturnValue { get; set; } | |
} | |
public class MethodConcernArgs : IMethodConcernArgs | |
{ | |
readonly MethodInterceptionArgs _args; | |
public MethodConcernArgs(MethodInterceptionArgs args) | |
{ | |
_args = args; | |
} | |
public void Proceed() | |
{ | |
_args.Proceed(); | |
} | |
public object Instance | |
{ | |
get { return _args.Instance; } | |
} | |
public MethodBase Method | |
{ | |
get { return _args.Method; } | |
} | |
public object ReturnValue | |
{ | |
get { return _args.ReturnValue; } | |
set { _args.ReturnValue = value; } | |
} | |
} |
public class ServiceErrorConcern | |
{ | |
readonly IExceptionLogger _exceptionLogger; | |
public ServiceErrorConcern(IExceptionLogger exceptionLogger) | |
{ | |
_exceptionLogger = exceptionLogger; | |
} | |
public void OnInvoke(IMethodConcernArgs args) | |
{ | |
try | |
{ | |
args.Proceed(); | |
} | |
catch (SqlException ex) | |
{ | |
var obj = (ServiceBase)args.Instance; | |
if (ex.Message.Contains("The DELETE statement conflicted with the REFERENCE constraint")) | |
obj.AddValidationError("This item can't be deleted because it is still used by other items."); | |
else | |
{ | |
obj.AddValidationError(GetErrorMessage(ex)); | |
_exceptionLogger.Log(ex); | |
} | |
ErrorReturn(args); | |
} | |
catch (NotImplementedException) | |
{ | |
// if something's not implemented, just throw it up | |
throw; | |
} | |
catch (Exception ex) | |
{ | |
_exceptionLogger.Log(ex); | |
var obj = (ServiceBase)args.Instance; | |
obj.AddValidationError(GetErrorMessage(ex)); | |
ErrorReturn(args); | |
} | |
} | |
string GetErrorMessage(Exception ex) | |
{ | |
if (ex is SqlException) | |
{ | |
if (IsThisSite.RunningLocally) | |
return "There was a database error. Please try again later. (" + ex.Message + "), (" + ex.GetType().FullName + ")"; | |
return "There was a database error. Please try again later."; | |
} | |
if (IsThisSite.RunningLocally) | |
return "There was an error. Please try again later. (" + ex.Message + "), (" + ex.GetType().FullName + ")"; | |
return "There was an error. Please try again later."; | |
} | |
// if the return type is ienumerable, then I want the return type to be an empty collection | |
// if the return type is value, then I want the return to be default value | |
// if the return type is void, don't touch the return value | |
// if the return type is reference (should cover everything else), then return should be null | |
void ErrorReturn(IMethodConcernArgs args) | |
{ | |
try | |
{ | |
var methodInfo = args.Method as MethodInfo; | |
if (methodInfo == null) | |
return; | |
if (methodInfo.ReturnType == typeof(string)) | |
args.ReturnValue = null; | |
else if (typeof(IEnumerable).IsAssignableFrom(methodInfo.ReturnType)) | |
args.ReturnValue = Activator.CreateInstance(methodInfo.ReturnType); | |
else if (methodInfo.ReturnType.IsValueType) | |
args.ReturnValue = Activator.CreateInstance(methodInfo.ReturnType); | |
else if (methodInfo.ReturnType != typeof (void)) | |
args.ReturnValue = null; | |
} | |
catch | |
{ | |
// don't want any errors to take place trying to build the correct return value | |
// so just swallow them, and let args.ReturnValue stay whatever it is | |
} | |
} | |
} |
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.
// while I'm not actually relying on | |
// PostSharp or any of my real service classes anywhere in my unit tests | |
// I still need a service class to stand in and receive errors | |
// that I can test assertions against | |
public class FakeService : ServiceBase | |
{ | |
public List<string> Errors { get; private set; } | |
public FakeService() | |
{ | |
Errors = new List<string>(); | |
this.AddValidationError = x => Errors.Add(x); | |
} | |
} |
public class When_invoking_a_service_method_with_sql_exception | |
{ | |
static ServiceErrorConcern _concern; | |
static IMethodConcernArgs _mockArgs; | |
static FakeService _fakeService; | |
static SqlException _sqlException; | |
static IExceptionLogger _mockLogger; | |
Establish context = () => | |
{ | |
_mockLogger = Mock.Create<IExceptionLogger>(); | |
_concern = new ServiceErrorConcern(_mockLogger); | |
_mockArgs = Mock.Create<IMethodConcernArgs>(); | |
_fakeService = new FakeService(); | |
Mock.Arrange(() => _mockArgs.Instance).Returns(_fakeService); | |
_sqlException = SqlExceptionHelper.CreateException(); | |
Mock.Arrange(() => _mockArgs.Proceed()).Throws(_sqlException); | |
}; | |
Because of = () => | |
{ | |
_concern.OnInvoke(_mockArgs); | |
}; | |
It should_callback_with_a_single_constraint_error_message = () => | |
_fakeService.Errors.Count.ShouldEqual(1); | |
It should_callback_with_an_appropriate_error_message = () => | |
_fakeService.Errors[0].ShouldContain("There was a database error. Please try again later."); | |
It should_log_the_exception = () => | |
Mock.Assert(() => _mockLogger.Log(Arg.Is(_sqlException)), Occurs.Once()); | |
} |
public class When_invoking_a_service_method_with_sql_constraint_exception | |
{ | |
static ServiceErrorConcern _concern; | |
static IMethodConcernArgs _mockArgs; | |
static FakeService _fakeService; | |
static IExceptionLogger _mockLogger; | |
static SqlException _sqlException; | |
Establish context = () => | |
{ | |
_mockLogger = Mock.Create<IExceptionLogger>(); | |
_concern = new ServiceErrorConcern(_mockLogger); | |
_mockArgs = Mock.Create<IMethodConcernArgs>(); | |
_fakeService = new FakeService(); | |
Mock.Arrange(() => _mockArgs.Instance).Returns(_fakeService); | |
_sqlException = SqlExceptionHelper.CreateException("xxx The DELETE statement conflicted with the REFERENCE constraint xxx"); | |
Mock.Arrange(() => _mockArgs.Proceed()).Throws(_sqlException); | |
}; | |
Because of = () => | |
{ | |
_concern.OnInvoke(_mockArgs); | |
}; | |
It should_callback_with_a_single_constraint_error_message = () => | |
_fakeService.Errors.Count.ShouldEqual(1); | |
It should_callback_with_an_appropriate_error_message = () => | |
_fakeService.Errors[0].ShouldEqual("This item can't be deleted because it is still used by other items."); | |
It should_not_log_the_exception = () => | |
Mock.Assert(() => _mockLogger.Log(Arg.IsAny<Exception>()), Occurs.Never()); | |
} |
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:
public override void OnInvoke(MethodInterceptionArgs args) | |
{ | |
try | |
{ | |
args.Proceed(); | |
} | |
catch (SqlException ex) | |
{ | |
ex.ToExceptionless().Submit(); | |
var obj = (ServiceBase) args.Instance; | |
if (ex.Message.Contains("The DELETE statement conflicted with the REFERENCE constraint")) | |
obj.AddValidationError("This item can't be deleted because it is still used by other items."); | |
else | |
obj.AddValidationError("There was a database error. Please try again later. (" + ex.Message + "), (" + | |
ex.GetType().FullName + ")"); | |
} | |
catch (NotImplementedException) | |
{ | |
// if something's not implemented, just throw it up | |
throw; | |
} | |
catch (Exception ex) | |
{ | |
ex.ToExceptionless().Submit(); | |
var obj = (ServiceBase)args.Instance; | |
obj.AddValidationError("There was an error. Please try again later. (" + ex.Message + "), (" + ex.GetType().Name + ")"); | |
} | |
} |
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.
public override void OnInvoke(MethodInterceptionArgs args) | |
{ | |
try | |
{ | |
args.Proceed(); | |
} | |
catch (SqlException ex) | |
{ | |
var obj = (ServiceBase) args.Instance; | |
if (ex.Message.Contains("The DELETE statement conflicted with the REFERENCE constraint")) | |
obj.AddValidationError("This item can't be deleted because it is still used by other items."); | |
else | |
{ | |
obj.AddValidationError(GetErrorMessage(ex)); | |
if (!IsThisSite.RunningLocally) | |
ex.ToExceptionless().Submit(); | |
} | |
ErrorReturn(args); | |
} | |
catch (NotImplementedException) | |
{ | |
// if something's not implemented, just throw it up | |
throw; | |
} | |
catch (Exception ex) | |
{ | |
if (!IsThisSite.RunningLocally) | |
ex.ToExceptionless().Submit(); | |
var obj = (ServiceBase)args.Instance; | |
obj.AddValidationError(GetErrorMessage(ex)); | |
ErrorReturn(args); | |
} | |
} | |
private string GetErrorMessage(Exception ex) | |
{ | |
if(IsThisSite.RunningLocally) | |
return "There was an error. Please try again later. (" + ex.Message + "), (" + ex.GetType().FullName + ")"; | |
return "There was an error. Please try again later."; | |
} |
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:
public ActionResult MyAction() | |
{ | |
var listResults = _service.GetAllResults(); // an exception here gets logged, but execution continues | |
var model = new MyViewModel(); | |
model.NumResults = listResults.Count; // listResults is null, NullReferenceExcpetion here | |
return View(model); | |
} |
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.
public void OnInvoke(MethodInterceptionArgs args) | |
{ | |
try | |
{ | |
args.Proceed(); | |
} | |
catch (Exception ex) | |
{ | |
// ... snip ... | |
ErrorReturn(args); | |
} | |
} | |
void ErrorReturn(MethodInterceptionArgs args) | |
{ | |
try | |
{ | |
var methodInfo = args.Method as MethodInfo; | |
if (methodInfo == null) // cast failed, so just give up | |
return; | |
else if (typeof(IEnumerable).IsAssignableFrom(methodInfo.ReturnType)) | |
args.ReturnValue = Activator.CreateInstance(methodInfo.ReturnType); | |
} | |
catch | |
{ | |
// don't want any errors to take place trying to build the correct return value | |
// so just swallow them, and let args.ReturnValue stay whatever it is | |
} | |
} |
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.
void ErrorReturn(MethodInterceptionArgs args) | |
{ | |
try | |
{ | |
var methodInfo = args.Method as MethodInfo; | |
if (methodInfo == null) | |
return; | |
if (methodInfo.ReturnType == typeof(string)) // if it's string, just return a null | |
args.ReturnValue = null; | |
else if (typeof(IEnumerable).IsAssignableFrom(methodInfo.ReturnType)) | |
args.ReturnValue = Activator.CreateInstance(methodInfo.ReturnType); | |
else if (methodInfo.ReturnType.IsValueType) // for value types, return default | |
args.ReturnValue = Activator.CreateInstance(methodInfo.ReturnType); | |
else if (methodInfo.ReturnType != typeof (void)) // anything else, return null | |
args.ReturnValue = null; | |
} | |
catch | |
{ | |
// don't want any errors to take place trying to build the correct return value | |
// so just swallow them, and let args.ReturnValue stay whatever it is | |
} | |
} |
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.