From 13c12330fdae3442c90362efa4b1a4403e7be437 Mon Sep 17 00:00:00 2001 From: "justinas.bardauskas" Date: Wed, 11 Jan 2023 14:49:38 +0200 Subject: [PATCH 1/2] bugfix: after openapi specs comparison, all parameters are removed from new spec (right side) because of reused parameters list. - updated code to use copy of given parameters list in diff method - added few tests to cover this case --- .../core/compare/ParametersDiff.java | 32 +++++++------- .../openapidiff/core/OpenApiDiffTest.java | 44 +++++++++++++++++++ 2 files changed, 60 insertions(+), 16 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 c7fb99b08..810b9ced4 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; @@ -16,6 +17,7 @@ /** compare two parameter */ public class ParametersDiff { + private static final RefPointer refPointer = new RefPointer<>(RefType.PARAMETERS); private final Components leftComponents; @@ -47,29 +49,27 @@ public static boolean same(Parameter left, Parameter right) { } public DeferredChanged 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<>(); + 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); - for (Parameter leftPara : left) { - leftPara = refPointer.resolveRef(leftComponents, leftPara, leftPara.get$ref()); + final ChangedParameters changedParameters = new ChangedParameters(wLeft, wRight, context); - Optional rightParam = contains(rightComponents, right, leftPara); - if (!rightParam.isPresent()) { - changedParameters.getMissing().add(leftPara); + 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 builder.buildIsChanged(changedParameters); } 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 88ac57b11..2d52392fe 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(); + } } From 2cdb8375d3abc104a4c7350969e332b4a0c4a610 Mon Sep 17 00:00:00 2001 From: "justinas.bardauskas" Date: Wed, 11 Jan 2023 15:23:06 +0200 Subject: [PATCH 2/2] test fix: path_5.yaml and path_6.yaml identical so their compare result should yield not issues. --- .../openapitools/openapidiff/core/PathDiffTest.java | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) 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 a20d20f64..725fec856 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,8 @@ 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(); } }