mirror of
https://github.com/wiremock/WireMock.Net.git
synced 2026-01-11 22:30:41 +01:00
Why does JsonMatcher accept an invalid JSON string? #291
Reference in New Issue
Block a user
Delete Branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Originally created by @lobsteropteryx on GitHub (Aug 6, 2020).
It appears that the
JsonMatcherconstructor accepts an invalid JSON string, and swallows the parsing exception.Is there a valid use case for this behavior? As far as I can tell, it still won't properly match an invalid JSON string. For me, the preferred behavior would be to let the caller know right away that the string is not valid JSON.
I would be happy to submit a pull request with this change, but I wanted to make sure I'm not missing something.
Thank you much, this is a fantastic tool that we use every day!
@StefH commented on GitHub (Aug 7, 2020):
@lobsteropteryx
Thanks for the question.
I think that more places in the code do a try-catch-swallow. So it's best approach to detect all these places and use a new configuration setting
bool? ThrowOnException.What do you think?
@lobsteropteryx commented on GitHub (Aug 7, 2020):
I will have to dig a little deeper to see where else that might happen, but a boolean setting would be fine.
Another possibility might be to try to parse the string immediately when constructing the
JsonMatcher. That would limit the blast radius, since I believe it's only created two places internally.Is this something you would like help with?
@StefH commented on GitHub (Aug 7, 2020):
I see your point.
This code
8ae0abb023/src/WireMock.Net/Matchers/JsonMatcher.cs (L91)uses theValue, but if indeed the conversion from thatobject ValuetoJToken jtokenValueis done in the constructor, any errors are found earlier.And if this code fails:
For exception I can use that configuration setting bool? ThrowOnException.
@StefH commented on GitHub (Aug 7, 2020):
Parsing the code in the constructor : you can try this in preview MyGet :
WireMock.Net.1.2.17-ci-13680.nupkg@StefH commented on GitHub (Aug 8, 2020):
Hello @lobsteropteryx,
A new preview version can be found on MyGet:
WireMock.Net.1.2.17-ci-13684. Can you please test?Set the
ThrowExceptionWhenMatcherFailsin theWireMockServerSettingsto true.https://github.com/WireMock-Net/WireMock.Net/pull/500
@StefH commented on GitHub (Aug 12, 2020):
@lobsteropteryx Did you have time to test this new setting?
@lobsteropteryx commented on GitHub (Aug 12, 2020):
Hi @StefH!
I've done some testing, and I like the new behavior--parsing the string immediately, and raising an exception for invalid JSON.
You might have an issue with the
ThrowExceptionparameter; as it stands right now, you'll still get the same exception raised whetherThrowExceptionis true or false, since you're callingConvertValueToJTokenhere: https://github.com/WireMock-Net/WireMock.Net/blob/JsonMatcherFixes/src/WireMock.Net/Matchers/JsonMatcher.cs#L98I actually think this is correct, and perhaps the
ThrowExceptionparameter isn't needed; I can't think of a situation where I wouldn't want the exception to be thrown right away.@StefH commented on GitHub (Aug 12, 2020):
I also think that throwing exception in constructor is correct. You get this exception immediately when adding a invalid mapping.
Later today I will merge PR and create new official release.