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

Closes #1882 - Enable multi value pattern matching for DateTime #1925

Merged

Conversation

klaasdellschaft
Copy link
Contributor

  • Adding failing test cases
  • Apply a fix for the failing test cases

- Adding failing test cases
- Apply a fix for the failing test cases
@klaasdellschaft
Copy link
Contributor Author

This PR fixes #1882 I added several test cases that were originally failing, and which are then fixed by this PR.

The problem described in #1882 occurs when doing any kind of DateTime matching on potentially multi valued fields like query parameters or headers. If the actual request does not contain any such value (i.e. query parameter or header value is missing), then the absent multi value field is translated into a list that contains a single null value. Thus, the different DateTimePatterns need to be able to gracefully handle the situation where the value that is passed to AbstractDateTimePattern.match(String value) is null.

From my point of view, an open question is which distance is to be expected when comparing an expected DateTime to a missing DateTime. I decided for keeping the currently computed value where distance == -1.0 if the actual value is absent.

@klaasdellschaft klaasdellschaft changed the title Fixes #1882 - Enable multi value pattern matching for DateTime Closes #1882 - Enable multi value pattern matching for DateTime Jul 12, 2022
@tomakehurst
Copy link
Member

Hey @klaasdellschaft thanks for submitting this.

Generally it looks great. The main change that's needed is that a non-match should result in distance 1.0 rather than a negative, since distance is normalised to between 0 and 1, and where we can't realistically do a distance calculation we set the value to 1 e.g. https://github.com/wiremock/wiremock/blob/master/src/main/java/com/github/tomakehurst/wiremock/matching/MatchesJsonPathPattern.java#L53

Also, please could you run ./gradlew spotlessApply and re-push so that the formatting errors are cleared?

@klaasdellschaft
Copy link
Contributor Author

Hi @tomakehurst thanks for the feedback. Pushed my changes to address your review. If you like, I can also squash the two commits before you merge.

@klaasdellschaft klaasdellschaft force-pushed the 1882-datetime-matching-multi-value branch from 2de57a3 to 27310d2 Compare July 14, 2022 07:41
@klaasdellschaft
Copy link
Contributor Author

Hi @tomakehurst after I saw the failing pipeline, I applied the spotless check again. Overall, I had to call gradlew spotlessApply several times in a row, until it finally settled on a stable source code output. Is that known behavior of the spotless check?

@tomakehurst
Copy link
Member

That's strange, you should only have to apply it once. I've never seen it need more than one run. Was there any particular pattern to the changes it was making?

Or is there any chance your IDE was reverting/auto-formatting stuff in the background or something like that?

@klaasdellschaft
Copy link
Contributor Author

The "unstableness" was related to the reordering and reorganizing of the import statements. I did not watch out for a particular pattern.

IDE auto-formatting may have influenced the commit / push (I will check and adapt my "on-commit" settings in the IDE) but I observed the unstableness on the Gradle command line where the IDE should not have jumped in because it did not see the changes yet. I will double and triple check now so that hopefully the next try of passing the spotless check in the pipeline is my last try.

- Use "1.0" as match distance where realistically no distance can be calculated
- Apply Spotless checks
@klaasdellschaft klaasdellschaft force-pushed the 1882-datetime-matching-multi-value branch from 27310d2 to 5d92eea Compare July 14, 2022 13:55
@klaasdellschaft
Copy link
Contributor Author

OK, now I did the gradlew check after commit and before push, so I'm confident that it should now also pass the checks in the pipeline. Sorry for bothering you with the several retries of getting the formatting right.

@tomakehurst tomakehurst merged commit bfbbd8c into wiremock:master Jul 19, 2022
@tomakehurst
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants