serialize http errors

[csantero]

This implements serialization of HttpError objects, (which are are what WebApi passes when an action throws an exception), in a way that is compliant with the json-api spec. I may have over-engineered this a bit, but I wanted it to be testable.

The standard members of the error object are populated as follows:

• id: a GUID
• status: 500
• title: the exception type name
• detail: the exception message

I also wanted to include stack trace and a recursive inner exception. These data points don't really have equivalents in the json-api spec for errors, but I included them anyway as stackTrace and inner.

A few points for further discussion:

1. We probably need a way for the user to specify a level of error verbosity. I imagine that in production, you don't want stack traces (or even perhaps the error title or detail) to leak.
2. Logging. The errors should be logged along with their ID somewhere. I'm not sure exactly how to do that.
3. Of the two tests I added in JsonApiMediaFormaterTests.cs, one is an integration test, and the other is more of a unit test, since it uses a mock ErrorSerializer. I didn't like putting them together, but I wasn't sure how else to organize them.
4. I think the decoupling approach I took with this patch could be applied elsewhere in the formatter. We could strip most of the smarts out of JsonApiFormatter itself, and instead give it dependencies on internal interfaces.
5. I like FluentAssertions so I added it as a test dependency. It's not really necessary, just syntactic sugar, so I can easily remove it if you want (but I do think it's pretty cool).
6. This is not going to merge cleanly with my other PRs, so you should merge them first and I'll solve the conflicts in here as a new diff.

[SphtKr]

This looks great...taking me a while to take it all in, but I think it makes sense. One question generally: Are errors that bubble up always guaranteed to be wrapped in HttpError? Or should we generalize this more to the point where it's possible (perhaps with user-provided serializer classes) to serialize more error types (I'm thinking of ones in inner as well). That's something we can also do as a later refactor, if necessary, so I'm thinking we'll go ahead and merge this for the time being either way (we're still < 1.0 obviously).

A thought on inner: You asked before what use case would be needed for errors being an array in the spec--my guess is this is precisely it. The MO for related objects in JSON API seems to be to flatten them into an array, and reference relationships by id, rather than embedding complex types inside a resource representation (as a general rule anyway). My guess is it might fit the feel of JSON API better to have inner be a reference to the id of the inner exception, and have the exception object actually a sibling of the outer error. It seems the spec doesn't specify this, but I'm just thinking out loud.

On your numbered discussion points:

1. My thought is that we should rely on customErrors in the Web.config to control this--it may already happen in fact, at least insofar as the stack trace and error specifics being contained within the HttpError in the first place.
2. Initial thought is to not build logging in, but provide a hook where a user can be passed any errors that occur (by subclassing ErrorSerializer? providing a lambda function callback?). That way if they want to use NLog or emails or whatever they can.
3. No problem, especially at this stage. I'm not an expert on test architecture, as is probably painfully obvious from the tests written so far.
4. Up for whatever, but I'm not sure exactly what you mean yet.
5. That's great, I'm certainly not picky about adding dependencies to the test project.
6. Got it, I was wondering that. I've been trying to rebuild my laptop (the IA guys said I have to if I want to keep my Mac...grr...) but got hung up on something, I'm installing VS2013 on my old PC in the meantime so I can get your patches and try them against one of my projects. My guess is they'll all be merged as is no problem though--including the OData dependency removal, I just wanted to try them against my own project and it's taking longer than I thought to get back to it, sorry.

[csantero]

The MO for related objects in JSON API seems to be to flatten them into an array, and reference relationships by id, rather than embedding complex types inside a resource representation (as a general rule anyway).

This same thought did occur to me as well. I rejected it for my initial implementation because while it does feel like it resembles the JSON API style, I'm not sure that it really increases developer productivity. Under the current implementation, when I get an error response back from the server, to find the inner exception detail I just have to expand the inner property. Formatting errors like you are suggesting would make it more of a hassle: first you read the id, and then you have to manually find the matching record in the collection. This could be a real pain if the hierarchy is more than 2 oe 3 levels deep. Of course you can write a tool to do this for you, but I often find myself looking at raw responses when developing my API, and I think embedded errors would generally be preferable. I'm willing to change my mind if you feel strongly about it though. =)

Are errors that bubble up always guaranteed to be wrapped in HttpError?

I don't know if it's guaranteed that this happens. But the only ones I've personally seen are HttpError. I am in favor of the concept of pluggable formatters, though, which is basically what I meant by number 4 above. We can let the user inject constructor parameters that implement IErrorSerializer, IResourceSerializer, etc. This relates to the above point in that it would let someone write an IErrorSerializer that flattens the hierarchy rather than embedding inner errors.

My thought is that we should rely on customErrors in the Web.config to control this--it may already happen in fact, at least insofar as the stack trace and error specifics being contained within the HttpError in the first place.

The formatter should be agnostic to global configuration, I don't want to rely on statics there. I see a few possible solutions:

1. Have a boolean property on JsonApiFormatter called SerializeErrorDetails or SendErrorDetails or something. Let the user figure out when it should be true. Easy for us but not as useful.
2. Assuming we choose to let them provide a custom IErrorSerializer, we can just punt and let them do it there. Again, not very helpful, but no more work on our part.
3. Inject an object implementing a new interface IJsonApiFormatterConfiguration. This interface would have a boolean property like in solution 1. The default implementation would look at the static GlobalConfiguration to check the value of customErrors, but you can override it if you want. This solution is the best for the user because it holds their hand by default, but lets them configure custom behavior if they want. It's more work for us, but this is my preferred solution.

Initial thought is to not build logging in, but provide a hook where a user can be passed any errors that occur (by subclassing ErrorSerializer? providing a lambda function callback?). That way if they want to use NLog or emails or whatever they can.

It does seem like error logging is not part of the role of the media type formatter. Maybe the action filter we discussed should be in charge of this. We could then generate the GUID in the filter, and add it as a key to the HttpError (which is just an IDictionary<string, object> after all). Then the ErrorSerializer could fetch the ID from the HttpError object itself, rather than generating its own (although we could have it still generate one if the ID key is missing from the HttpError, for example if the user never added our action filter).

[SphtKr]

Okay, I got everything else merged, can you get the conflicts resolved on this one?

We need to open an issue on error handling/filtering I guess, and continue the conversation there. Short answer: I think what I meant when I suggested relying on customErrors wasn't so much putting using it directly, more that we might should leave inclusion/exclusion/redacting up to the rest of the pipeline, and that it shouldn't be up to the serialization layer...but thinking more about it, we may have less choice, because we can't really punt to later parts of the pipeline--e.g. letting IIS return an HTML error page doesn't work when the client is expecting JSON obviously). So, option 3 is interesting to me if we find other configuration options that need to go in there...which will probably happen. Option 4 also occurred to me: if we have a hook where we allow the user to log an error (as we were saying), we could have that function return an error message: e.g. public HttpError processError(HttpError error) { ... }, in which case the user could return a different (e.g. redacted) error or null, if they wanted the error to not be serialized at all.

Okay that wasn't so short. Like I said, I'll open an issue for this so we can track it separately.

[csantero]

@SphtKr Rebase is finished, and I started #10 to continue the discussion.