Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Throw new exception type when test operation fails #22

Open
kipusoep opened this issue Jul 24, 2023 · 9 comments
Open

Throw new exception type when test operation fails #22

kipusoep opened this issue Jul 24, 2023 · 9 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@kipusoep
Copy link

kipusoep commented Jul 24, 2023

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 Havunen added the enhancement New feature or request label Jul 24, 2023
@Havunen
Copy link
Owner

Havunen commented Jul 24, 2023

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
Copy link
Owner

Havunen commented Aug 14, 2023

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

@kipusoep
Copy link
Author

kipusoep commented Aug 21, 2023

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
Copy link
Owner

Havunen commented Aug 21, 2023

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
Copy link
Author

kipusoep commented Aug 21, 2023

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)

@Havunen Havunen reopened this Aug 21, 2023
@kipusoep
Copy link
Author

kipusoep commented Aug 21, 2023

It seems to come from this:

public static readonly Action<JsonPatchError> Default = error => ExceptionHelper.ThrowJsonPatchException(error.ErrorMessage);

@Havunen
Copy link
Owner

Havunen commented Sep 17, 2023

This should work now in 3.0.1

@Havunen Havunen closed this as completed Sep 17, 2023
@kipusoep
Copy link
Author

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 Havunen reopened this Sep 22, 2023
@Havunen
Copy link
Owner

Havunen commented Sep 22, 2023

Ok, I need to check that

@Havunen Havunen added the help wanted Extra attention is needed label May 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants