Parameter data detection fix when PathParams changed (#353)

Fixes #353

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

@@ -72,14 +72,15 @@ public DeferredChanged<ChangedOperation> diff(
           .ifPresent(changedOperation::setRequestBody);
     }
 
+    ParametersDiffResult parametersDiffResult =
+            openApiDiff.getParametersDiff().diff(oldOperation.getParameters(), newOperation.getParameters(), context);
     builder
-        .with(
-            openApiDiff
-                .getParametersDiff()
-                .diff(oldOperation.getParameters(), newOperation.getParameters(), context))
+        .with(parametersDiffResult.deferredChanged)
         .ifPresent(
             params -> {
-              removePathParameters(context.getParameters(), params);
+              if (!parametersDiffResult.sameOperationsDiffSchema) {
+                removePathParameters(context.getParameters(), params);
+              }
               changedOperation.setParameters(params);
             });
 

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

@@ -6,6 +6,8 @@
 import java.util.List;
 import java.util.Objects;
 import java.util.Optional;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
 import org.openapitools.openapidiff.core.model.Changed;
 import org.openapitools.openapidiff.core.model.ChangedParameters;
 import org.openapitools.openapidiff.core.model.DiffContext;
@@ -14,6 +16,15 @@
 import org.openapitools.openapidiff.core.utils.RefPointer;
 import org.openapitools.openapidiff.core.utils.RefType;
 
+class ParametersDiffResult {
+  protected DeferredChanged<ChangedParameters> deferredChanged;
+  protected boolean sameOperationsDiffSchema;
+
+  public ParametersDiffResult(DeferredChanged<ChangedParameters> deferredChanged, boolean sameOperationsDiffSchema) {
+    this.deferredChanged = deferredChanged;
+    this.sameOperationsDiffSchema = sameOperationsDiffSchema;
+  }
+}
 /** compare two parameter */
 public class ParametersDiff {
   private static final RefPointer<Parameter> refPointer = new RefPointer<>(RefType.PARAMETERS);
@@ -46,9 +57,7 @@ public static boolean same(Parameter left, Parameter right) {
         && Objects.equals(left.getIn(), right.getIn());
   }
 
-  public DeferredChanged<ChangedParameters> diff(
-      List<Parameter> left, List<Parameter> right, DiffContext context) {
-
+  public ParametersDiffResult diff(List<Parameter> left, List<Parameter> right, DiffContext context) {
     DeferredBuilder<Changed> builder = new DeferredBuilder<>();
     ChangedParameters changedParameters =
         new ChangedParameters(left, right != null ? new ArrayList<>(right) : null, context);
@@ -70,7 +79,59 @@ public DeferredChanged<ChangedParameters> diff(
       }
     }
     changedParameters.getIncreased().addAll(right);
+    return new ParametersDiffResult(
+            builder.buildIsChanged(changedParameters),
+            pathUnchangedParametersChanged(changedParameters, context)
+    );
+  }
+
+  public boolean pathUnchangedParametersChanged(
+      ChangedParameters changedParameters, DiffContext context) {
+    if (!pathUnchanged(changedParameters, context)) return false;
+    // If missing and increased parameter count is different, it's a new endpoint
+    if (changedParameters.getMissing().size() != changedParameters.getIncreased().size())
+      return false;
+    // Go through each missing Parameter and compare it to newly added Parameters
+    for (Parameter parameter : changedParameters.getMissing()) {
+      // Speedy Check. Use the map already created in changedParameters to check if missing param is linked to newParam
+      String newParameterName = context.getParameters().get(parameter.getName());
+      if (newParameterName.isEmpty()) return false;
+
+      Optional<Parameter> newParameter =
+          changedParameters.getIncreased().stream()
+              .filter(p -> p.getName().equals(newParameterName))
+              .findFirst();
+      if (!newParameter.isPresent()) return false;
 
-    return builder.buildIsChanged(changedParameters);
+      // Check if  the old and new Parameters match . IF so, return TRUE
+      Parameter newParameterRealized = newParameter.get();
+      newParameterRealized.setName(parameter.getName()); // Make names similar
+      boolean samePathDifferentParameter = !newParameterRealized.equals(parameter);
+      newParameterRealized.setName(newParameterName); // Important:: MUST Reset the name as this is not a copy
+      return samePathDifferentParameter;
+    }
+    return false;
+  }
+
+  public boolean pathUnchanged(ChangedParameters changedParameters, DiffContext context) {
+    final String REGEX_PATH = "\\{([^/]+)}";
+    String oldUrl = context.getLeftUrl();
+    String newUrl = context.getRightUrl();
+    ArrayList<String> oldUrlPathParams = matchedItems(oldUrl, REGEX_PATH);
+    ArrayList<String> newUrlPathParams = matchedItems(newUrl, REGEX_PATH);
+    // Path Param count doesn't match or param-less path doesn't match or param is changed --> It's a new endpoint
+    return oldUrlPathParams.size() == newUrlPathParams.size()
+            && changedParameters.getChanged().isEmpty()
+            && oldUrl.replaceAll(REGEX_PATH, "").equals(newUrl.replaceAll(REGEX_PATH, ""));
+  }
+
+  public ArrayList<String> matchedItems(String string, String regex) {
+    Matcher matcher = Pattern.compile(regex).matcher(string);
+    ArrayList<String> matchedItems = new ArrayList<>();
+    while (matcher.find()) {
+      String item = matcher.group();
+      matchedItems.add(item.substring(0, matcher.group().length() - 1).replaceFirst("\\{", ""));
+    }
+    return matchedItems;
   }
 }

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

@@ -36,8 +36,13 @@ public DeferredChanged<ChangedPath> diff(PathItem left, PathItem right, DiffCont
           .with(
               openApiDiff
                   .getOperationDiff()
-                  .diff(oldOperation, newOperation, context.copyWithMethod(method)))
-          .ifPresent(changedPath.getChanged()::add);
+                  .diff(oldOperation, newOperation,
+                          context.copyWithMethod(method).copyWithLeftRightUrls(
+                                  context.getLeftUrl(),
+                                  context.getRightUrl()
+                          )
+                  )
+          ).ifPresent(changedPath.getChanged()::add);
     }
     builder
         .with(

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

@@ -85,6 +85,7 @@ public DeferredChanged<ChangedPaths> diff(
                 DiffContext context = new DiffContext();
                 context.setUrl(url);
                 context.setParameters(params);
+                context.setLeftAndRightUrls(url, rightUrl);
                 builder
                     .with(openApiDiff.getPathDiff().diff(leftPath, rightPath, context))
                     .ifPresent(path -> changedPaths.getChanged().put(rightUrl, path));

core/src/main/java/org/openapitools/openapidiff/core/model/DiffContext.java

@@ -14,6 +14,8 @@ public class DiffContext {
   private boolean response;
   private boolean request;
   private Boolean required;
+  private String leftUrl;
+  private String rightUrl;
 
   public DiffContext() {
     parameters = new HashMap<>();
@@ -37,6 +39,10 @@ public DiffContext copyAsResponse() {
     return copy().setResponse();
   }
 
+  public DiffContext copyWithLeftRightUrls(String leftUrl, String rightUrl) {
+    return copy().setLeftAndRightUrls(leftUrl, rightUrl);
+  }
+
   private DiffContext setRequest() {
     this.request = true;
     this.response = false;
@@ -104,6 +110,16 @@ private DiffContext setRequired(boolean required) {
     return this;
   }
 
+  public DiffContext setLeftAndRightUrls(String leftUrl, String rightUrl) {
+    this.leftUrl = leftUrl;
+    this.rightUrl = rightUrl;
+    return this;
+  }
+
+  public String getLeftUrl() { return this.leftUrl; }
+
+  public String getRightUrl() { return this.rightUrl; }
+
   @Override
   public boolean equals(Object o) {
     if (this == o) return true;
@@ -131,6 +147,8 @@ public int hashCode() {
         .append(response)
         .append(request)
         .append(required)
+        .append(leftUrl)
+        .append(rightUrl)
         .toHashCode();
   }
 }

core/src/test/java/org/openapitools/openapidiff/core/PathParameterSchemaDiffTest.java

@@ -0,0 +1,53 @@
+package org.openapitools.openapidiff.core;
+
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.api.Test;
+import org.openapitools.openapidiff.core.model.ChangedOpenApi;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class PathParameterSchemaDiffTest {
+  final String TEST_MSG_1 =
+      "Testing: \n"
+          + "1. Same path but different pathParameters\n"
+          + "2. different parameters in the parameters: section\n"
+          + "3. Parameters have different schema\n"
+          + "eg:\n"
+          + "old path   -- students/{id}\n"
+          + "old schema -- id: integer\n"
+          + "new path   -- students/{username}\n"
+          + "new schema -- username: string";
+
+  final String TEST_MSG_2 =
+      "Testing: \n"
+          + "1. Same path but different pathParameters\n"
+          + "2. different parameters in the parameters: section\n"
+          + "3. Parameters have same schema\n";
+
+  @Test
+  @DisplayName(
+      "Same Path, different PathParams, Params in the `Parameters`: match pathParam, Different Schema")
+  public void pathSamePathParamsDiffParamSameAsInPathButSchemaDiff() {
+    final Logger logger = LoggerFactory.getLogger(PathParameterSchemaDiffTest.class);
+    logger.info(TEST_MSG_1);
+    String OPENAPI_DOC1 = "path_parameter_diff_param_schema_diff_old.yaml";
+    String OPENAPI_DOC2 = "path_parameter_diff_param_schema_diff_new.yaml";
+    ChangedOpenApi diff = OpenApiCompare.fromLocations(OPENAPI_DOC1, OPENAPI_DOC2);
+    Assertions.assertTrue(diff.isDifferent());
+    Assertions.assertFalse(diff.isCompatible());
+  }
+
+  @Test
+  @DisplayName(
+      "Same Path, different PathParams, Params in the `Parameters`: match pathParam, same Schema")
+  public void pathSamePathParamsDiffParamNameDiffSchemaSame() {
+    final Logger logger = LoggerFactory.getLogger(PathParameterSchemaDiffTest.class);
+    logger.info(TEST_MSG_2);
+    String OPENAPI_DOC1 = "path_parameter_diff_param_name_diff_old.yaml";
+    String OPENAPI_DOC2 = "path_parameter_diff_param_name_diff_new.yaml";
+    ChangedOpenApi diff = OpenApiCompare.fromLocations(OPENAPI_DOC1, OPENAPI_DOC2);
+    Assertions.assertFalse(diff.isDifferent());
+    Assertions.assertTrue(diff.isCompatible());
+  }
+}

core/src/test/resources/path_parameter_diff_param_name_diff_new.yaml

@@ -0,0 +1,48 @@
+openapi: 3.0.0
+info:
+  title: Sample API
+  description: Optional multiline or single-line description in [CommonMark](http://commonmark.org/help/) or HTML.
+  version: 0.1.9
+servers:
+  - url: http://api.example.com/v1
+    description: Example api server
+
+paths:
+
+  /student/{username}:
+    get:
+      parameters:
+        - in: path
+          name: username   # Note the name is the same as in the path
+          required: true
+          schema:
+            type: string
+            minimum: 1
+
+
+      summary: Returns data about a student
+      responses:
+        '200': # status code
+          description: A JSON Object student data
+          content:
+            application/json:
+              schema:
+                type: object
+                items:
+                  $ref: '#/components/schemas/Student'
+
+components:
+  schemas:
+    Student:
+      type: object
+      properties:
+        id:
+          type: integer
+          example: "1"
+        username:
+          type: string
+          example: "alloy_horizon"
+          pattern: A..Z a..z 0..9 . _ -
+        name:
+          type: string
+          example: "Alloy Horizon"

core/src/test/resources/path_parameter_diff_param_name_diff_old.yaml

@@ -0,0 +1,48 @@
+openapi: 3.0.0
+info:
+  title: Sample API
+  description: Optional multiline or single-line description in [CommonMark](http://commonmark.org/help/) or HTML.
+  version: 0.1.9
+servers:
+  - url: http://api.example.com/v1
+    description: Example api server
+
+paths:
+
+  /student/{id}:
+    get:
+      parameters:
+        - in: path
+          name: id   # Note the name is the same as in the path
+          required: true
+          schema:
+            type: string
+            minimum: 1
+
+
+      summary: Returns data about a student
+      responses:
+        '200': # status code
+          description: A JSON Object student data
+          content:
+            application/json:
+              schema:
+                type: object
+                items:
+                  $ref: '#/components/schemas/Student'
+
+components:
+  schemas:
+    Student:
+      type: object
+      properties:
+        id:
+          type: integer
+          example: "1"
+        username:
+          type: string
+          example: "alloy_horizon"
+          pattern: A..Z a..z 0..9 . _ -
+        name:
+          type: string
+          example: "Alloy Horizon"

core/src/test/resources/path_parameter_diff_param_schema_diff_new.yaml

@@ -0,0 +1,48 @@
+openapi: 3.0.0
+info:
+  title: Sample API
+  description: Optional multiline or single-line description in [CommonMark](http://commonmark.org/help/) or HTML.
+  version: 0.1.9
+servers:
+  - url: http://api.example.com/v1
+    description: Example api server
+
+paths:
+
+  /student/{username}:
+    get:
+      parameters:
+        - in: path
+          name: username   # Note the name is the same as in the path
+          required: true
+          schema:
+            type: string
+            minimum: 1
+
+
+      summary: Returns data about a student
+      responses:
+        '200': # status code
+          description: A JSON Object student data
+          content:
+            application/json:
+              schema:
+                type: object
+                items:
+                  $ref: '#/components/schemas/Student'
+
+components:
+  schemas:
+    Student:
+      type: object
+      properties:
+        id:
+          type: integer
+          example: "1"
+        username:
+          type: string
+          example: "alloy_horizon"
+          pattern: A..Z a..z 0..9 . _ -
+        name:
+          type: string
+          example: "Alloy Horizon"