Fix flood json string can not be null or empty from mapping matcher

[emilianoalvarez91]

When using body patterns and matchesJsonPath with either $. or $[?. there are several occasions that wiremock throws a warning with the error:
Warning: JSON path expression {Expression} failed to match document 'null' because of error 'json string can not be null or empty'
This floods the entire wiremock logs leaving verbose pretty useless.

References

#1817

Submitter checklist

• The PR request is well described and justified, including the body and the references
• The PR title represents the desired changelog entry
• The repository's code style is followed (see the contributing guide)
• Test coverage that demonstrates that the change works as expected
• For new features, there's necessary documentation in this pull request or in a subsequent PR to wiremock.org

[oleg-nenashev]

Looks like it's quite urgent based on votes.
Would be nice to improve propagation.

[oleg-nenashev]

Definitely needs a fix. Leaving to @tomakehurst to comments on the implementation style, the NPE approach is a matter for discussion

[emilianoalvarez91]

@oleg-nenashev thanks. Yes the reason in which I was explicit with the NPE is that I want to guarantee that specific error and capture it accordingly in isAdvancedMatch, otherwise if I rethrow the initial error in getExpressionResult, there's no guarantee that isAdvancedMatch function captures the same error in the future. If @tomakehurst you have a better suggestion, I'm happy to make the change.

[tomakehurst]

I'm not keen on catching NPE. Something like this might be better (around line 94):

if (expressionResult == null || expressionResult.isEmpty()) {
        if (AbsentPattern.class.isAssignableFrom(valuePattern.getClass())) {
          expressionResult = ListOrSingle.of((String) null);
        } else {
          return MatchResult.noMatch();
        }
      }

[tomakehurst]

We could just drop this completely. I'm not sure it's really helping, and I plan to remove all of these at some point anyway now that we've got sub-events.

[tomakehurst]

An additional test case or two would definitely be a good idea also

[emilianoalvarez91]

@tomakehurst I followed your suggestion of removing the notifier and by that we don't need to catch the specific error. Do you think we should go in this direction or should I rework the other solution?

[oleg-nenashev]

I would put the message to the debug-level logging, but otherwise +1

[tomakehurst]

LGTM.

I suspect we now also have this problem in terms of stacking up a loads of sub-events for the same reason, but we can address this separately.

[oleg-nenashev]

I suspect we now also have this problem in terms of stacking up a loads of sub-events for the same reason, but we can address this separately.

Yeah, very likely