feat: optionally fail request if several mappings match #2263

tomasbjerre · 2023-07-12T14:53:24Z

This PR adds a feature that, if enabled, will fail requests if several mappings are matched.

I have found myself, several times, trying to understand why some mapping is not working and later come the the conclusion that some other mapping was serving the request. This would help me reduce time spent on such problems.

References

wiremock/wiremock.org#127

Would also solve #2565

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

Requesting changes, There is a bunch of API breaking changes that are IMHO not needed. QUite opposite, maybe it is better to have a withFailIfMultipleMappingsMatch(boolean) notation and to take a default value from a static final variable that we could make customizable later once WireMock gets full configuration engine

I also mentioned a bunch or refactorings in the code. No strong opinion about them

oleg-nenashev · 2023-07-17T11:27:50Z

src/main/java/com/github/tomakehurst/wiremock/core/Options.java

@@ -136,4 +136,6 @@ default Function<Extensions, NotMatchedRenderer> getNotMatchedRendererFactory()
  Set<String> getTemplatePermittedSystemKeys();

  boolean getTemplateEscapingDisabled();
+
+  boolean failIfMultipleMappingsMatch();


Needs a default return if we do not want to break binary compatibility of a public interface

Also should be failIfMultipleMappingsMatch() to follow the pattern

I think this is fixed

oleg-nenashev · 2023-07-17T11:29:16Z

src/main/java/com/github/tomakehurst/wiremock/core/WireMockApp.java

@@ -139,7 +140,8 @@ public WireMockApp(
      Map<String, ResponseDefinitionTransformerV2> v2transformers,
      Map<String, RequestMatcherExtension> requestMatchers,
      FileSource rootFileSource,
-      Container container) {
+      Container container,
+      boolean failIfMultipleMappingsMatch) {


Breaks binary compatibility too. Maybe it is time to start switching to the public withFailIfMultipleMappingsMatch(boolean) pattern for new fields that are not critical to set?

Fixed.

So WiremockApp has 2 constructors, perhaps the one taking Options is newer and the one taking booleans should be marked as @Deprecated? Anyway, I added the withFailIfMultipleMappingsMatch.

Works for me.

oleg-nenashev · 2023-07-17T11:31:58Z

src/main/java/com/github/tomakehurst/wiremock/stubbing/AbstractStubMappings.java


    subEvents.forEach(initialServeEvent::appendSubEvent);

-    scenarios.onStubServed(matchingMapping);
+    this.scenarios.onStubServed(matchingMapping);


A bunch of unrelated refactorings, but not blocking the PR because of it. I see no strict need to use this. in the code or in the code style though

Sorry, this was a save action. Removing those changes.

oleg-nenashev · 2023-07-17T11:32:39Z

src/main/java/com/github/tomakehurst/wiremock/stubbing/StoreBackedStubMappings.java

@@ -33,14 +33,16 @@ public StoreBackedStubMappings(
      Map<String, ResponseDefinitionTransformer> transformers,
      Map<String, ResponseDefinitionTransformerV2> v2transformers,
      BlobStore filesBlobStore,
-      List<StubLifecycleListener> stubLifecycleListeners) {
+      List<StubLifecycleListener> stubLifecycleListeners,
+      boolean failIfMultipleMappingsMatch) {


breaking change too

src/main/java/com/github/tomakehurst/wiremock/stubbing/InMemoryStubMappings.java

tomakehurst · 2023-07-17T15:17:16Z

Let's be careful about merging this one as it's changing the most performance-sensitive part of the codebase

tomasbjerre · 2023-07-17T17:04:43Z

Perhaps some of the API comments here would have been avoided if the things that are considered API is in its own Gradle project, as discussed in #2266 .

tomasbjerre · 2023-07-17T17:05:39Z

Requesting changes, There is a bunch of API breaking changes that are IMHO not needed. QUite opposite, maybe it is better to have a withFailIfMultipleMappingsMatch(boolean) notation and to take a default value from a static final variable that we could make customizable later once WireMock gets full configuration engine

I also mentioned a bunch or refactorings in the code. No strong opinion about them

I'm not sure I understand what you mean here exactly.

oleg-nenashev

All key comments are addressed. I guess now we need a call on the performance overhead, but I have no objections otherwise

P.S: We have no perf tests to do measurements

oleg-nenashev · 2023-07-18T11:16:25Z

src/main/java/com/github/tomakehurst/wiremock/core/Options.java

@@ -136,4 +136,8 @@ default Function<Extensions, NotMatchedRenderer> getNotMatchedRendererFactory()
  Set<String> getTemplatePermittedSystemKeys();

  boolean getTemplateEscapingDisabled();
+
+  default boolean failIfMultipleMappingsMatch() {


This class uses mixed patterns but I guess getSomething() dominates. Should it be getFailIfMultipleMappingsMatch() . No strong opinion

feat: optionally fail request if several mappings match

87814eb

tomasbjerre requested a review from a team as a code owner July 12, 2023 14:53

tomasbjerre mentioned this pull request Jul 12, 2023

docs: optionally fail request if several mappings match wiremock/wiremock.org#127

Closed

3 tasks

oleg-nenashev requested changes Jul 17, 2023

View reviewed changes

oleg-nenashev added the enhancement label Jul 17, 2023

tomasbjerre added 2 commits July 17, 2023 18:22

chore: reverting automated refactoring of AbstractStubMappings

2fd1703

chore: keeping it binary compatible

38037e2

oleg-nenashev reviewed Jul 18, 2023

View reviewed changes

chore: getFailIfMultipleMappingsMatch

e1ecd4d

oleg-nenashev approved these changes Jul 24, 2023

View reviewed changes

oleg-nenashev requested a review from tomakehurst July 24, 2023 08:14

oleg-nenashev added the needs-tom Tom's Train Project :) label Jul 24, 2023

tomasbjerre mentioned this pull request Mar 29, 2024

Detect overlapping mock json in wiremock #2565

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: optionally fail request if several mappings match #2263

feat: optionally fail request if several mappings match #2263

tomasbjerre commented Jul 12, 2023 •

edited

oleg-nenashev left a comment

oleg-nenashev Jul 17, 2023

oleg-nenashev Jul 17, 2023

tomasbjerre Jul 17, 2023

oleg-nenashev Jul 17, 2023

tomasbjerre Jul 17, 2023

oleg-nenashev Jul 18, 2023

oleg-nenashev Jul 17, 2023

tomasbjerre Jul 17, 2023

oleg-nenashev Jul 17, 2023

tomasbjerre Jul 17, 2023

tomakehurst commented Jul 17, 2023

tomasbjerre commented Jul 17, 2023

tomasbjerre commented Jul 17, 2023

oleg-nenashev left a comment •

edited

oleg-nenashev Jul 18, 2023

tomasbjerre Jul 18, 2023

feat: optionally fail request if several mappings match #2263

Are you sure you want to change the base?

feat: optionally fail request if several mappings match #2263

Conversation

tomasbjerre commented Jul 12, 2023 • edited

References

Submitter checklist

oleg-nenashev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomakehurst commented Jul 17, 2023

tomasbjerre commented Jul 17, 2023

tomasbjerre commented Jul 17, 2023

oleg-nenashev left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomasbjerre commented Jul 12, 2023 •

edited

oleg-nenashev left a comment •

edited