Return Custom Message When ApplyUpdatesTo Throws An Exception

[broderickt2]

In my scenario, ApplyUpdatesTo throws an exception if someone inputs an invalid enum. The returned error message starts like this "Failed to set value at path "/bill_every" while performing "replace" operation" and can be confusing, so I am wanting to return a custom error message. Is there a way to possibly override ApplyUpdatesTo or maybe wrap it in a try catch to create a more generic or larger implementation?

I tried creating a new class that inherits from JsonPatchDocument as shown below:

public class JsonPatchFormatter<TEntity>: JsonPatch.JsonPatchDocument<TEntity> where TEntity : class, new()
{
	public new void ApplyUpdatesTo(TEntity entity)
	{
		try
		{
			base.ApplyUpdatesTo(entity);
		}
		catch (Exception ex)
		{
			throw new Exception("whatever");
		}
		
	}
}

I then tried using the new class like below:

endpoints.MapMethods("/some-resource/{id}", new[] { "PATCH" }, async (Guid id, JsonPatchFormatter<SomeDto> model) =>
{
    var objectToUpdate = repository.GetById(id);
    patchData.ApplyUpdatesTo(objectToUpdate);
	...
});

but then I ran into an issue with the media type formatter as shown below. I think JsonPatchDocument isn't working quite the same like this.
"No MediaTypeFormatter is available to read an object of type 'JsonPatchFormatter`1' from content with media type 'application/json-patch+json'."

Thanks in advance for your time!

[broderickt2]

@myquay I wasn't able to accomplish the above (overriding ApplyUpdatesTo) and after further research determined that modifying the method SetValueFromPath with some custom logic would resolve this.

For further context, I was trying to return the enums that could be used if an end API user inputted an invalid value. For an API field that has a nullable enum type, if a user submitted an incorrect value (one not in the enum), he or she could get a response something like below (depending on how this package is integrated with your API responses):

"Failed to set value at path \"/my_field\" while performing \"replace\" operation: Error converting value \"my_bad_enum_value\" to type 'System.Nullable`1[path/directory]'. Path '', line 1, position 9."

This could be confusing and potentially a security issue (debatable of course) by exposing the internal directory. Or for those less tech savvy, it is just not super clear. Passing invalid values to the API should not normally happen though especially if you expose all of the valid values of the enum in your documentation.

With the changes I did, now SetValueFromPath can potentially throw an exception something like this (which is more clear in my opinion):

Invalid value \"bad_enum_value\" for Foo. Possible values are: FirstEnum,SecondEnum,ThirdEnum

@myquay I am not sure if others would like this change or if it makes sense for the direction of this repo. Let me know what you think. I'd be happy to open a pull request if anything.

[myquay]

Hi @broderickt2,

Thanks for taking the time to write up the issue you're having.

The security issue

Internal exceptions should be managed via. the frameworks error handling features: https://learn.microsoft.com/en-us/aspnet/core/fundamentals/minimal-apis/handle-errors?view=aspnetcore-7.0 and not exposed directly to the end user - that's outside the scope of this library.

The error message

In regard to the actual error message returned from the library it's hard to get a message that's suitable for all applications and it isn't localised etc - so I think tinkering with that that isn't the correct path to take.

However, that's not to say it can't be improved. I think a good approach to that is to return additional contextual information in the exception thrown so the error handler in a specific API implementation has all the information required to generate a suitable error message for its particular use case.

Just off the top of my head something like:

public class JsonPatchOperationException : JsonPatchException {

/// Additional properties that can be used to generate a message like 
/// 'Invalid value \"bad_enum_value\" for Foo. Possible values are: FirstEnum,SecondEnum,ThirdEnum'. 
/// E.g., OperationType, Path, Target Type, Supplied Value etc...

}

Which can be thrown in place of the base JsonPatchException in the SetValueFromPath method.

Then the downstream API error handler can catch errors of this type and has the required information from the library to generate a useful error message without baking a particular message into the library itself.

Let me know how that sounds and if it covers the use case you've described. I'd be more than happy to work with you on getting the change through - take a stab at an initial implementation and open a PR and we can go from there.

[broderickt2]

@myquay Thanks for the prompt response and explaining a lot of extra information. I did some research on my end and you are correct in that I should manage the exception on my end and not rely on this library to return custom exception messages.

I think improving the exceptions that are returned from this library is a great idea. I got some ideas implemented on my local machine but cannot create a branch to push to. Do I need to be a collaborator to create a branch and pull request? Or are you able to create a branch I can push to? I thought about forking the repo and then creating a pull request upstream but don't think this will work from what I am reading here due to my access.

[myquay]

What you read there is the process, fork the repository and when you're happy with the changes submit a pull request 😊

[broderickt2]

@myquay That worked! I got a pull request here when you get a moment.

[broderickt2]

@myquay Thanks for merging in the pull request. Left one last question here for you.

[myquay]

@broderickt2 Thanks for the contribution and taking the time to clean up those namespaces.

[broderickt2]

@myquay Thanks for reviewing both of my pull requests and merging them in. I am excited to use the new release!