KevinDockx / JsonPatch

JSON Patch (JsonPatchDocument) RFC 6902 implementation for .NET
MIT License
173 stars 29 forks source link

Improve error handling according to spec #14

Closed KevinDockx closed 9 years ago

KevinDockx commented 9 years ago

=> split from #13 - by @sergkr

Here's another (potential) consideration: error handling. Error handling probably deserves its own separate issue, as there are a lot of considerations there, but since it will affect the decision of whether to use a custom formatter or not (which we're currently discussing), it's probably worth mentioning a couple things about it here.

In the error handling portion of the JSON Patch spec, it refers you to RFC5789, where it says if the user sends a malformed patch document, the server should return an HTTP 400 (Bad Request) response. Ideally, this is something that you should do in your library (so the user of the library shouldn't even have to worry about this). And just to be clear, we're not necessarily talking about just malformed JSON here - they might be sending you valid JSON, but not a valid JSON patch document (e.g., sending an object instead of an array at the root).

Currently, if they send a malformed patch document, you're just throwing a JsonPatchException, and I think that's probably fine for now. Even if you go the custom formatter route, you would still have a hard time implementing what the spec says, because you don't have access to the HTTP context (request/response) inside a formatter:

http://aspnetwebstack.codeplex.com/workitem/82

At least you don't have easy access - because you actually can get to it as explained here (which is also alluded to in that codeplex issue). But that's something you can only do inside a formatter, as far as I know - so it might be another argument for going the custom formatter route.

However, error handling is a complex topic - the user of the library may want to do error logging, and maybe have full control over the response, in which case you would be doing them a major disfavor if you implement the spec to the letter without making it configurable. If you're going to be helping out with adding JSON Patch support to MVC 6 (I'm aware that's under consideration right now), you may want to touch base with the team on what's the best way to approach this.

KevinDockx commented 9 years ago

Looked into this; when the JsonPatchDocument itself is malformed, the library will throw an exception stating it's malformed. When an operation can 't be applied, the library will throw an error stating that.

But what's going to be returned to the consumer of the API, that's up to the consumer of the library; ie: this will follow the same principle as is generally followed by frameworks like Web API. If you look at the way a REST-full API should be implemented (return NotFound, Internal Server Error, Created, ... when applicable), the throwing of those status codes is up to the developer. Web API provides the means to do this. Likewise, the library will provide enough information so the consumer can throw BadRequest etc, but returning the correct status code itself is up to the consumer of the library.