Fix for #823 (#824)

* Fix for #823

* Fixes #823

* docs: enhance README.md with detailed explanation of path matching behavior

Author: DrSatyr

README.md

@@ -193,6 +193,21 @@ public class Main {
 }
 ```
 
+### Path Matching while comparing two OpenAPI paths
+
+Path matching controls how paths from the old and new specs are paired during comparison (PathsDiff.java). The default matcher (DefaultPathMatcher) obfuscates path parameter names, meaning `/users/{id}` matches `/users/{userId}`. Default matcher fails on ambiguous signatures if spec contains few paths semantically identical. In case this behaviour is not fitting your use case, you can implement your own matching strategy.
+
+You can plug in a custom matcher via `OpenApiDiffOptions` implementing the `PathMatcher` interface.:
+
+```java
+OpenApiDiffOptions options = OpenApiDiffOptions
+    .builder()
+    .pathMatcher(new MyCustomPathMatcher())
+    .build();
+
+ChangedOpenApi diff = OpenApiCompare.fromLocations(oldSpec, newSpec, null, options);
+```
+
 ### Render difference
 
 ---

core/src/main/java/org/openapitools/openapidiff/core/compare/OpenApiDiffOptions.java

@@ -6,9 +6,12 @@
 import org.apache.commons.configuration2.CompositeConfiguration;
 import org.apache.commons.configuration2.YAMLConfiguration;
 import org.apache.commons.configuration2.ex.ConfigurationException;
+import org.openapitools.openapidiff.core.compare.matchers.DefaultPathMatcher;
+import org.openapitools.openapidiff.core.compare.matchers.PathMatcher;
 
 public class OpenApiDiffOptions {
   private final CompositeConfiguration config;
+  private PathMatcher pathMatcher;
 
   private OpenApiDiffOptions(CompositeConfiguration config) {
     this.config = config;
@@ -18,6 +21,10 @@ public CompositeConfiguration getConfig() {
     return config;
   }
 
+  public PathMatcher getPathMatcher() {
+    return pathMatcher != null ? pathMatcher : new DefaultPathMatcher();
+  }
+
   public static Builder builder() {
     return new Builder();
   }
@@ -45,5 +52,10 @@ public Builder configProperty(String propKey, String propVal) {
     public OpenApiDiffOptions build() {
       return built;
     }
+
+    public Builder pathMatcher(PathMatcher matcher) {
+      built.pathMatcher = matcher;
+      return this;
+    }
   }
 }

core/src/main/java/org/openapitools/openapidiff/core/compare/PathsDiff.java

@@ -1,13 +1,10 @@
 package org.openapitools.openapidiff.core.compare;
 
-import io.swagger.v3.oas.models.Operation;
 import io.swagger.v3.oas.models.PathItem;
 import io.swagger.v3.oas.models.Paths;
-import io.swagger.v3.oas.models.parameters.Parameter;
 import java.util.*;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
-import java.util.stream.IntStream;
 import org.openapitools.openapidiff.core.model.Changed;
 import org.openapitools.openapidiff.core.model.ChangedPaths;
 import org.openapitools.openapidiff.core.model.DiffContext;
@@ -22,135 +19,62 @@ public PathsDiff(OpenApiDiff openApiDiff) {
     this.openApiDiff = openApiDiff;
   }
 
-  private static String normalizePath(String path) {
-    return path.replaceAll(REGEX_PATH, "{}");
-  }
-
-  private static List<String> extractParameters(String path) {
-    ArrayList<String> params = new ArrayList<>();
-    Pattern pattern = Pattern.compile(REGEX_PATH);
-    Matcher matcher = pattern.matcher(path);
-    while (matcher.find()) {
-      params.add(matcher.group(1));
-    }
-    return params;
-  }
-
   public DeferredChanged<ChangedPaths> diff(
       final Map<String, PathItem> left, final Map<String, PathItem> right) {
     DeferredBuilder<Changed> builder = new DeferredBuilder<>();
 
     ChangedPaths changedPaths = new ChangedPaths(left, right, openApiDiff.getOptions());
     changedPaths.getIncreased().putAll(right);
 
-    left.keySet()
+    left.entrySet()
         .forEach(
-            (String url) -> {
-              PathItem leftPath = left.get(url);
-              String template = normalizePath(url);
+            pathEntry -> {
+              String leftUrl = pathEntry.getKey();
+              PathItem leftPath = pathEntry.getValue();
               Optional<Map.Entry<String, PathItem>> result =
-                  changedPaths.getIncreased().entrySet().stream()
-                      .filter(item -> normalizePath(item.getKey()).equals(template))
-                      .min(
-                          (a, b) -> {
-                            if (methodsAndParametersIntersect(a.getValue(), b.getValue())) {
-                              throw new IllegalArgumentException(
-                                  "Two path items have the same signature: " + template);
-                            }
-                            if (a.getKey().equals(url)) {
-                              return -1;
-                            } else if (b.getKey().equals((url))) {
-                              return 1;
-                            } else {
-                              HashSet<PathItem.HttpMethod> methodsA =
-                                  new HashSet<>(a.getValue().readOperationsMap().keySet());
-                              methodsA.retainAll(leftPath.readOperationsMap().keySet());
-                              HashSet<PathItem.HttpMethod> methodsB =
-                                  new HashSet<>(b.getValue().readOperationsMap().keySet());
-                              methodsB.retainAll(leftPath.readOperationsMap().keySet());
-                              return Integer.compare(methodsB.size(), methodsA.size());
-                            }
-                          });
+                  openApiDiff
+                      .getOptions()
+                      .getPathMatcher()
+                      .find(pathEntry, changedPaths.getIncreased());
               if (result.isPresent()) {
                 String rightUrl = result.get().getKey();
                 PathItem rightPath = changedPaths.getIncreased().remove(rightUrl);
                 Map<String, String> params = new LinkedHashMap<>();
-                if (!url.equals(rightUrl)) {
-                  List<String> oldParams = extractParameters(url);
+                if (!leftUrl.equals(rightUrl)) {
+                  List<String> oldParams = extractParameters(leftUrl);
                   List<String> newParams = extractParameters(rightUrl);
                   for (int i = 0; i < oldParams.size(); i++) {
                     params.put(oldParams.get(i), newParams.get(i));
                   }
                 }
                 DiffContext context = new DiffContext(openApiDiff.getOptions());
-                context.setUrl(url);
+                context.setUrl(leftUrl);
                 context.setParameters(params);
-                context.setLeftAndRightUrls(url, rightUrl);
+                context.setLeftAndRightUrls(leftUrl, rightUrl);
                 builder
                     .with(openApiDiff.getPathDiff().diff(leftPath, rightPath, context))
                     .ifPresent(path -> changedPaths.getChanged().put(rightUrl, path));
               } else {
-                changedPaths.getMissing().put(url, leftPath);
+                changedPaths.getMissing().put(leftUrl, leftPath);
               }
             });
     return builder.buildIsChanged(changedPaths);
   }
 
+  private List<String> extractParameters(String path) {
+    ArrayList<String> params = new ArrayList<>();
+    Pattern pattern = Pattern.compile(REGEX_PATH);
+    Matcher matcher = pattern.matcher(path);
+    while (matcher.find()) {
+      params.add(matcher.group(1));
+    }
+    return params;
+  }
+
   public static Paths valOrEmpty(Paths path) {
     if (path == null) {
       path = new Paths();
     }
     return path;
   }
-
-  /**
-   * @param a a path form the open api spec
-   * @param b another path from the same open api spec
-   * @return <code>true</code> in case both paths are of the same method AND their templated
-   *     parameters are of the same type; <code>false</code> otherwise
-   */
-  private static boolean methodsAndParametersIntersect(PathItem a, PathItem b) {
-    Set<PathItem.HttpMethod> methodsA = a.readOperationsMap().keySet();
-    for (PathItem.HttpMethod method : b.readOperationsMap().keySet()) {
-      if (methodsA.contains(method)) {
-        Operation left = a.readOperationsMap().get(method);
-        Operation right = b.readOperationsMap().get(method);
-        if (left.getParameters().size() == right.getParameters().size()) {
-          return parametersIntersect(left.getParameters(), right.getParameters());
-        }
-        return false;
-      }
-    }
-    return false;
-  }
-
-  /**
-   * Checks if provided parameter pairs are equal by type and format
-   *
-   * @param left parameters from the first compared method
-   * @param right parameters from the second compared method
-   * @return <code>true</code> in case each parameter pair is of the same type; <code>false</code>
-   *     otherwise
-   */
-  private static boolean parametersIntersect(List<Parameter> left, List<Parameter> right) {
-    int parametersSize = left.size();
-    long intersectedParameters =
-        IntStream.range(0, left.size())
-            .filter(i -> parametersTypeEquals(left.get(i), right.get(i)))
-            .count();
-    return parametersSize == intersectedParameters;
-  }
-
-  /**
-   * Checks if provided parameter pair is equal by type and format
-   *
-   * @param left parameter from the first compared method
-   * @param right parameter from the second compared method
-   * @return <code>true</code> in case parameter pair is of the same type; <code>false</code>
-   *     otherwise
-   */
-  private static boolean parametersTypeEquals(Parameter left, Parameter right) {
-    return Objects.equals(left.getSchema().getType(), right.getSchema().getType())
-        && Objects.equals(left.getSchema().getFormat(), right.getSchema().getFormat());
-  }
 }

core/src/main/java/org/openapitools/openapidiff/core/compare/matchers/DefaultPathMatcher.java

@@ -0,0 +1,82 @@
+package org.openapitools.openapidiff.core.compare.matchers;
+
+import io.swagger.v3.oas.models.Operation;
+import io.swagger.v3.oas.models.PathItem;
+import io.swagger.v3.oas.models.parameters.Parameter;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Optional;
+import java.util.Set;
+import java.util.stream.IntStream;
+
+/** Default implementation of PathMatcher */
+public class DefaultPathMatcher implements PathMatcher {
+
+  private static final String REGEX_PATH = "\\{([^/{}]+)}";
+
+  @Override
+  public Optional<Map.Entry<String, PathItem>> find(
+      Map.Entry<String, PathItem> what, Map<String, PathItem> candidates) {
+    String leftUrl = what.getKey();
+    PathItem leftPath = what.getValue();
+
+    final String template = normalizePath(leftUrl);
+    return candidates.entrySet().stream()
+        .filter(item -> normalizePath(item.getKey()).equals(template))
+        .min(
+            (a, b) -> {
+              if (methodsAndParametersIntersect(a.getValue(), b.getValue())) {
+                throw new IllegalArgumentException(
+                    "Two path items have the same signature: " + template);
+              }
+              if (a.getKey().equals(leftUrl)) {
+                return -1;
+              } else if (b.getKey().equals(leftUrl)) {
+                return 1;
+              } else {
+                HashSet<PathItem.HttpMethod> methodsA =
+                    new HashSet<>(a.getValue().readOperationsMap().keySet());
+                methodsA.retainAll(leftPath.readOperationsMap().keySet());
+                HashSet<PathItem.HttpMethod> methodsB =
+                    new HashSet<>(b.getValue().readOperationsMap().keySet());
+                methodsB.retainAll(leftPath.readOperationsMap().keySet());
+                return Integer.compare(methodsB.size(), methodsA.size());
+              }
+            });
+  }
+
+  private static String normalizePath(String path) {
+    return path.replaceAll(REGEX_PATH, "{}");
+  }
+
+  private static boolean methodsAndParametersIntersect(PathItem a, PathItem b) {
+    Set<PathItem.HttpMethod> methodsA = a.readOperationsMap().keySet();
+    for (PathItem.HttpMethod method : b.readOperationsMap().keySet()) {
+      if (methodsA.contains(method)) {
+        Operation left = a.readOperationsMap().get(method);
+        Operation right = b.readOperationsMap().get(method);
+        if (left.getParameters().size() == right.getParameters().size()) {
+          return parametersIntersect(left.getParameters(), right.getParameters());
+        }
+        return false;
+      }
+    }
+    return false;
+  }
+
+  private static boolean parametersIntersect(List<Parameter> left, List<Parameter> right) {
+    int parametersSize = left.size();
+    long intersectedParameters =
+        IntStream.range(0, left.size())
+            .filter(i -> parametersTypeEquals(left.get(i), right.get(i)))
+            .count();
+    return parametersSize == intersectedParameters;
+  }
+
+  private static boolean parametersTypeEquals(Parameter left, Parameter right) {
+    return Objects.equals(left.getSchema().getType(), right.getSchema().getType())
+        && Objects.equals(left.getSchema().getFormat(), right.getSchema().getFormat());
+  }
+}

core/src/main/java/org/openapitools/openapidiff/core/compare/matchers/PathMatcher.java

@@ -0,0 +1,19 @@
+package org.openapitools.openapidiff.core.compare.matchers;
+
+import io.swagger.v3.oas.models.PathItem;
+import java.util.Map;
+import java.util.Optional;
+
+/** Strategy to find a matching path. */
+public interface PathMatcher {
+
+  /**
+   * Finds a matching path entry.
+   *
+   * @param what entry of the path to find
+   * @param candidates map of right spec paths to search in
+   * @return Optional entry of the matching right path
+   */
+  Optional<Map.Entry<String, PathItem>> find(
+      Map.Entry<String, PathItem> what, Map<String, PathItem> candidates);
+}