Havunen / SystemTextJsonPatch

SystemTextJsonPatch is a JSON Patch (JsonPatchDocument) RFC 6902 implementation for .NET using System.Text.Json
MIT License
102 stars 12 forks source link

Throw new exception type when test operation fails #22

Closed kipusoep closed 1 week ago

kipusoep commented 1 year ago

Currently it's hard to distinguish between a failing test operation and other apply issues, because they all throw a JsonPatchException. I would like to return a 409 Conflict http status when a test operation fails, but the only way to identify this is by catching the exception and check if the message contains " is not equal to the test value ". It would be nice if an exception of another type was thrown, for example a JsonPatchTestOperationException.

Havunen commented 1 year ago

Hi, thanks for the issue. I think this is great enhancement to this library but it needs to be done in the next major release to avoid breaking change

Havunen commented 1 year ago

Actually we could just extend the new exception type (JsonPatchTestOperationException) from JsonPatchException

kipusoep commented 1 year ago

Isn't it a breaking change? If you currently catch the JsonPatchException, will it also catch it if the exception is of type JsonPatchTestOperationException?

Havunen commented 1 year ago

Isn't it a breaking change? If you currently catch the JsonPatchException, will it also catch it if the exception is of type JsonPatchTestOperationException?

yes. they both inherit from the same type, but you can also explicitly catch JsonPatchTestOperationException now. I also bumped the major version to v3 and mentioned the possible breaking change in changelog.

kipusoep commented 1 year ago

So I just updated to v3, but when I call ApplyTo() I still get a JsonPatchException instead of the new JsonPatchTestOperationException.

Stacktrace:

   at SystemTextJsonPatch.Exceptions.ExceptionHelper.ThrowJsonPatchException(String message)
   at SystemTextJsonPatch.Adapters.ObjectAdapter.Test(Operation operation, Object objectToApplyTo)
   at SystemTextJsonPatch.JsonPatchDocument`1.ApplyTo(TModel objectToApplyTo, IObjectAdapter adapter)
kipusoep commented 1 year ago

It seems to come from this: https://github.com/Havunen/SystemTextJsonPatch/blob/f6df1c15511d53e431e736f9c9f3259a3c724467/SystemTextJsonPatch/Internal/ErrorReporter.cs#L8

Havunen commented 1 year ago

This should work now in 3.0.1

kipusoep commented 1 year ago

For me it's not there yet. I have a unit test which tests a patch operation with a path that does not exist, I would assume it doesn't throw a JsonPatchTestOperationException but the good old JsonPatchException instead. Because it's not a failing json patch test operation, it's just a replace operation instead.

Havunen commented 1 year ago

Ok, I need to check that

Havunen commented 1 week ago

I refactored the exception handling so that JsonPatchTestOperationException is now thrown only for test -operations failures, but FailedOperation properties are moved to JsonPatchException

This change will be available in the next release