From d6383f3a70423c7abbcdb5b50a05619c9ea6fba7 Mon Sep 17 00:00:00 2001 From: Justinas Bardauskas <39775336+justinasbardauskas@users.noreply.github.com> Date: Sun, 26 Feb 2023 12:46:21 +0200 Subject: [PATCH] After specs comparison, all parameters are removed (#454) Co-authored-by: justinas.bardauskas Co-authored-by: Jochen Schalanda --- .../core/compare/ParametersDiff.java | 36 ++++++++------- .../openapidiff/core/OpenApiDiffTest.java | 44 +++++++++++++++++++ .../openapidiff/core/PathDiffTest.java | 13 ++---- 3 files changed, 67 insertions(+), 26 deletions(-) diff --git a/core/src/main/java/org/openapitools/openapidiff/core/compare/ParametersDiff.java b/core/src/main/java/org/openapitools/openapidiff/core/compare/ParametersDiff.java index d01a555b..c12eeaf9 100644 --- a/core/src/main/java/org/openapitools/openapidiff/core/compare/ParametersDiff.java +++ b/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 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 left, List right, DiffContext context) { - DeferredBuilder 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 rightParam = contains(rightComponents, right, leftPara); - if (!rightParam.isPresent()) { - changedParameters.getMissing().add(leftPara); + final List left, final List right, final DiffContext context) { + final DeferredBuilder builder = new DeferredBuilder<>(); + final List wLeft = Optional.ofNullable(left).orElseGet(Collections::emptyList); + final List 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 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)); diff --git a/core/src/test/java/org/openapitools/openapidiff/core/OpenApiDiffTest.java b/core/src/test/java/org/openapitools/openapidiff/core/OpenApiDiffTest.java index 88ac57b1..2d52392f 100644 --- a/core/src/test/java/org/openapitools/openapidiff/core/OpenApiDiffTest.java +++ b/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(); + } } diff --git a/core/src/test/java/org/openapitools/openapidiff/core/PathDiffTest.java b/core/src/test/java/org/openapitools/openapidiff/core/PathDiffTest.java index a20d20f6..858ef520 100644 --- a/core/src/test/java/org/openapitools/openapidiff/core/PathDiffTest.java +++ b/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(); } }