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.

Sign up for GitHub

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

Jump to bottom

Stop returning 500s for unmatched path patterns #2339

Merged
merged 1 commit into from
Aug 31, 2023
Merged

Stop returning 500s for unmatched path patterns #2339

merged 1 commit into from
Aug 31, 2023

Conversation

Mahoney
Copy link
Collaborator

@Mahoney Mahoney commented Aug 30, 2023

The Diff rendering did not handle the SubExpressionException thrown in
e.g. MatchesJsonPathPattern. This makes it catch that exception and
output it in the diff.

References

Reinstates #2336 after revert in #2338

Submitter checklist

  • Recommended: Join WireMock Slack to get any help in #help-contributing or a project-specific channel like #wiremock-java
  • 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

@Mahoney Mahoney requested a review from a team as a code owner August 30, 2023 08:41
tomakehurst
tomakehurst reviewed Aug 30, 2023
View reviewed changes
src/test/resources/exception-diff-sample_json-path.txt Outdated
ANY | ANY
/thing | /thing
|
$.accountNum [equalTo] 1234 | Warning: JSON path expression '$.accountNum' failed to<<<<< Body does not match
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like here, on the Request side it should just say (empty) rather than rendering an error message. We shouldn't really treat it as an error as when scanning a whole set of stubs matching a request with an empty body it's perfectly legit to test it against a stub that requires one.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK - I've reworked it so the diff rendered outputs no body if there is no body, and the body itself if the expression does not match, which seems to be the standard way.

@Mahoney
Stop returning 500s for unmatched path patterns
e95a0de 
The Diff rendering did not handle the `SubExpressionException` thrown in
e.g. `MatchesJsonPathPattern`. This makes it catch that exception and
output a useful value in the diff.
@oleg-nenashev
Copy link
Member

Is it a regression @Mahoney ?

@Mahoney
Copy link
Collaborator Author

Mahoney commented Aug 30, 2023 •
edited

Yes, looks like it is a regression

Not sure since when...

@Mahoney
Copy link
Collaborator Author

Mahoney commented Aug 30, 2023

It's a pretty old regression
It was working in 75a053b on 2018-11-07
it was broken at latest in 4a887a9 on 2021-04-29

Haven't bisected enough to work out when exactly it broke between those two

oleg-nenashev
oleg-nenashev approved these changes Aug 30, 2023
View reviewed changes
Copy link
Member

@oleg-nenashev oleg-nenashev left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I think it does not matter too much for such an old one

@Mahoney
Copy link
Collaborator Author

Mahoney commented Aug 30, 2023

It's actually quite nasty - if you create a stub using this matcher, you get a 500 for any request that doesn't match a stub instead of a 404 with a nice diff.

@oleg-nenashev
Copy link
Member

oleg-nenashev commented Aug 30, 2023 via email

@Mahoney Mahoney merged commit 5273a43 into master Aug 31, 2023
7 checks passed
@Mahoney Mahoney deleted the fix-exception-in-diff branch August 31, 2023 11:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomakehurst tomakehurst tomakehurst approved these changes

@oleg-nenashev oleg-nenashev oleg-nenashev approved these changes

Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Mahoney @oleg-nenashev @tomakehurst