After specs comparison, all parameters are removed (#454)

Co-authored-by: justinas.bardauskas <justinas.bardauskas@multitude.com>
Co-authored-by: Jochen Schalanda <jochen@schalanda.name>

Author: 3 people

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

@@ -3,6 +3,7 @@
 import io.swagger.v3.oas.models.Components;
 import io.swagger.v3.oas.models.parameters.Parameter;
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.List;
 import java.util.Objects;
 import java.util.Optional;
@@ -28,6 +29,7 @@ public ParametersDiffResult(
 }
 /** compare two parameter */
 public class ParametersDiff {
+
   private static final RefPointer<Parameter> refPointer = new RefPointer<>(RefType.PARAMETERS);
 
   private final Components leftComponents;
@@ -59,28 +61,28 @@ public static boolean same(Parameter left, Parameter right) {
   }
 
   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);
-    if (null == left) left = new ArrayList<>();
-    if (null == right) right = new ArrayList<>();
-
-    for (Parameter leftPara : left) {
-      leftPara = refPointer.resolveRef(leftComponents, leftPara, leftPara.get$ref());
-
-      Optional<Parameter> rightParam = contains(rightComponents, right, leftPara);
-      if (!rightParam.isPresent()) {
-        changedParameters.getMissing().add(leftPara);
+      final List<Parameter> left, final List<Parameter> right, final DiffContext context) {
+    final DeferredBuilder<Changed> builder = new DeferredBuilder<>();
+    final List<Parameter> wLeft = Optional.ofNullable(left).orElseGet(Collections::emptyList);
+    final List<Parameter> wRight =
+        Optional.ofNullable(right).map(ArrayList::new).orElseGet(ArrayList::new);
+
+    final ChangedParameters changedParameters = new ChangedParameters(wLeft, wRight, context);
+
+    for (Parameter leftParam : wLeft) {
+      leftParam = refPointer.resolveRef(leftComponents, leftParam, leftParam.get$ref());
+      Optional<Parameter> rightParamOpt = contains(rightComponents, wRight, leftParam);
+      if (!rightParamOpt.isPresent()) {
+        changedParameters.getMissing().add(leftParam);
       } else {
-        Parameter rightPara = rightParam.get();
-        right.remove(rightPara);
+        Parameter rightParam = rightParamOpt.get();
+        wRight.remove(rightParam);
         builder
-            .with(openApiDiff.getParameterDiff().diff(leftPara, rightPara, context))
+            .with(openApiDiff.getParameterDiff().diff(leftParam, rightParam, context))
             .ifPresent(changedParameters.getChanged()::add);
       }
     }
-    changedParameters.getIncreased().addAll(right);
+    changedParameters.getIncreased().addAll(wRight);
     return new ParametersDiffResult(
         builder.buildIsChanged(changedParameters),
         pathUnchangedParametersChanged(changedParameters, context));

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

@@ -3,6 +3,9 @@
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.openapitools.openapidiff.core.TestUtils.assertOpenApiAreEquals;
 
+import io.swagger.parser.OpenAPIParser;
+import io.swagger.v3.oas.models.OpenAPI;
+import io.swagger.v3.parser.core.models.ParseOptions;
 import java.io.FileWriter;
 import java.io.IOException;
 import java.nio.file.Path;
@@ -11,6 +14,7 @@
 import org.junit.jupiter.api.io.TempDir;
 import org.openapitools.openapidiff.core.model.ChangedOpenApi;
 import org.openapitools.openapidiff.core.model.ChangedOperation;
+import org.openapitools.openapidiff.core.model.DiffResult;
 import org.openapitools.openapidiff.core.model.Endpoint;
 import org.openapitools.openapidiff.core.output.HtmlRender;
 import org.openapitools.openapidiff.core.output.JsonRender;
@@ -21,6 +25,9 @@ public class OpenApiDiffTest {
   private final String OPENAPI_DOC1 = "petstore_v2_1.yaml";
   private final String OPENAPI_DOC2 = "petstore_v2_2.yaml";
   private final String OPENAPI_EMPTY_DOC = "petstore_v2_empty.yaml";
+  private final String OPENAPI_DOC3 = "petstore_openapi3.yaml";
+
+  private static final OpenAPIParser PARSER = new OpenAPIParser();
 
   @Test
   public void testEqual() {
@@ -104,4 +111,41 @@ public void testDiffAndJson(@TempDir Path tempDir) throws IOException {
     }
     assertThat(path).isNotEmptyFile();
   }
+
+  /** Testing that repetitive specs comparisons has to produce consistent result. */
+  @Test
+  public void testComparisonConsistency() {
+    final OpenAPI oldSpec = loadSpecFromFile(OPENAPI_DOC3);
+    final OpenAPI newSpec = loadSpecFromFile(OPENAPI_DOC3);
+
+    final ChangedOpenApi diff1 = OpenApiCompare.fromSpecifications(oldSpec, newSpec);
+    assertThat(diff1.isChanged()).isEqualTo(DiffResult.NO_CHANGES);
+    assertThat(diff1.getNewEndpoints()).isEmpty();
+    assertThat(diff1.getMissingEndpoints()).isEmpty();
+    assertThat(diff1.getChangedOperations()).isEmpty();
+
+    final ChangedOpenApi diff2 = OpenApiCompare.fromSpecifications(oldSpec, newSpec);
+    assertThat(diff2.isChanged()).isEqualTo(DiffResult.NO_CHANGES);
+    assertThat(diff2.getNewEndpoints()).isEmpty();
+    assertThat(diff2.getMissingEndpoints()).isEmpty();
+    assertThat(diff2.getChangedOperations()).isEmpty();
+  }
+
+  @Test
+  public void testSpecObjectsAreNotChangesAfterComparison() {
+    final OpenAPI oldSpec = loadSpecFromFile(OPENAPI_DOC3);
+    final OpenAPI newSpec = loadSpecFromFile(OPENAPI_DOC3);
+
+    OpenApiCompare.fromSpecifications(oldSpec, newSpec);
+    OpenApiCompare.fromSpecifications(oldSpec, newSpec);
+
+    final OpenAPI expectedOldSpec = loadSpecFromFile(OPENAPI_DOC3);
+    final OpenAPI expectedNewSpec = loadSpecFromFile(OPENAPI_DOC3);
+    assertThat(oldSpec).isEqualTo(expectedOldSpec);
+    assertThat(newSpec).isEqualTo(expectedNewSpec);
+  }
+
+  private static OpenAPI loadSpecFromFile(String specFile) {
+    return PARSER.readLocation(specFile, null, new ParseOptions()).getOpenAPI();
+  }
 }

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

@@ -2,7 +2,6 @@
 
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.junit.jupiter.api.Assertions.assertThrows;
-import static org.junit.jupiter.api.Assertions.fail;
 import static org.openapitools.openapidiff.core.TestUtils.assertOpenApiAreEquals;
 
 import org.junit.jupiter.api.Test;
@@ -42,13 +41,9 @@ public void testSameTemplateDifferentMethods() {
   @Test
   public void testDiffWithSimilarBeginningPaths() {
     ChangedOpenApi changedOpenApi = OpenApiCompare.fromLocations(OPENAPI_PATH5, OPENAPI_PATH6);
-    try {
-      ChangedOpenApi diff =
-          OpenApiCompare.fromSpecifications(
-              changedOpenApi.getOldSpecOpenApi(), changedOpenApi.getNewSpecOpenApi());
-      assertThat(diff.getChangedOperations()).hasSize(4);
-    } catch (IllegalArgumentException e) {
-      fail(e.getMessage());
-    }
+    ChangedOpenApi diff =
+        OpenApiCompare.fromSpecifications(
+            changedOpenApi.getOldSpecOpenApi(), changedOpenApi.getNewSpecOpenApi());
+    assertThat(diff.getChangedOperations()).isEmpty();
   }
 }