Closed HappyNomad closed 6 years ago
Exceptions are nice when you want to just "stop everything and give up," but are not so great for "normal control flow of errors," which is probably the intention of the code here. That is, it's perfectly normal for some mismatches to take place when performing a JSON PATCH operation, and using exceptions to flow that has its own set of problems. For example, exceptions are not "contractual" - that is, there's no verifiable way to know that an exception gets thrown, you have to read docs or read code. But having an explicit parameter makes it clear you should expect possible failures, and by what mechanism.
Earlier I also thought there may be some special circumstances here that necessitate the pattern over exceptions. But after having dug into the code, I see that "stop everything and give up," is what the code does anyway. That is, if an invalid condition is met, then processing of the JSON Patch document stops.
@HappyNomad I think there's still a difference with exceptions in that an exception can sort of "skip over" stack frames until someone does a catch
, and then even that catch
would catch all exceptions, even a random NullReferenceException
or ArgumentException
, whereas by flowing errors in a more structured way helps guarantee that the system is dutifully tracking only the types of errors it wishes to track.
@Eilon There's already a JsonPatchException
class defined in the Microsoft.AspNetCore.JsonPatch.Exceptions
namespace. Once all the out string errorMessage
s bubble up the call stack to ObjectAdapter
, this exception is what's thrown anyway. Replacing a stack of Try
calls and their out string errorMessage
parameters with a simple catch ( JsonPatchException )
would not catch a "random NullReferenceException or ArgumentException". Rather, it would facilitate "flowing errors in a more structured way" and thus "guarantee that the system is dutifully tracking only the types of errors it wishes to track".
I just happened to come across this code:
In spite of those numerous nested Try
methods and their out string errorMessage
parameters, the error is simply converted to a JsonPatchException
which is thrown in ObjectAdapter
and caught as you see in this code. All those nested Try
methods and out string errorMessage
parameters are for naught.
@HappyNomad I think that one way or another it was a design decision (even if not the greatest one), so it would be difficult to change it without making a breaking change. Most of what I said earlier is from memory and general thoughts.
While implementing JSON Patch
IAdapter
s, I encountered a coding pattern that I'm unaccustomed to. Instead of throwing and catching exceptions, the library hasout string errorMessage
parameters everywhere. That's what exceptions are for, so this pattern doesn't seem to belong.