From 5a778eef87849a5cf32609add750609a565dac56 Mon Sep 17 00:00:00 2001 From: vijayvepakomma Date: Thu, 13 May 2021 10:44:31 -0400 Subject: [PATCH 1/2] allow missing security schemes --- cli/pom.xml | 2 +- core/pom.xml | 2 +- .../core/compare/ApiResponseDiff.java | 16 +++++--- .../openapidiff/core/compare/PathsDiff.java | 37 +++++++++--------- .../core/compare/SecurityRequirementDiff.java | 17 ++++++++- .../compare/SecurityRequirementsDiff.java | 33 ++++++++-------- .../core/compare/SecuritySchemeDiff.java | 21 +++++++--- .../ComposedSchemaDiffResult.java | 5 ++- .../openapidiff/core/utils/RefPointer.java | 4 +- .../openapidiff/core/OneOfDiffTest.java | 1 + .../openapidiff/core/PathDiffTest.java | 4 +- .../openapidiff/core/SecurityDiffTest.java | 13 +++---- .../core/compare/ApiResponseDiffTest.java | 38 +++++++++---------- pom.xml | 2 +- 14 files changed, 112 insertions(+), 83 deletions(-) diff --git a/cli/pom.xml b/cli/pom.xml index 5bed4b12..66c18fb2 100644 --- a/cli/pom.xml +++ b/cli/pom.xml @@ -4,7 +4,7 @@ org.openapitools.openapidiff openapi-diff-parent - 2.0.0-SNAPSHOT + 2.0.1-SNAPSHOT openapi-diff-cli diff --git a/core/pom.xml b/core/pom.xml index a3a9d494..261e92e8 100644 --- a/core/pom.xml +++ b/core/pom.xml @@ -4,7 +4,7 @@ org.openapitools.openapidiff openapi-diff-parent - 2.0.0-SNAPSHOT + 2.0.1-SNAPSHOT openapi-diff-core diff --git a/core/src/main/java/org/openapitools/openapidiff/core/compare/ApiResponseDiff.java b/core/src/main/java/org/openapitools/openapidiff/core/compare/ApiResponseDiff.java index 669a4fde..bacdcc00 100644 --- a/core/src/main/java/org/openapitools/openapidiff/core/compare/ApiResponseDiff.java +++ b/core/src/main/java/org/openapitools/openapidiff/core/compare/ApiResponseDiff.java @@ -8,12 +8,11 @@ import java.util.List; import java.util.Map; import java.util.Optional; +import javax.annotation.Nullable; import org.openapitools.openapidiff.core.model.ChangedApiResponse; import org.openapitools.openapidiff.core.model.ChangedResponse; import org.openapitools.openapidiff.core.model.DiffContext; -import javax.annotation.Nullable; - /** Created by adarsh.sharma on 04/01/18. */ public class ApiResponseDiff { private final OpenApiDiff openApiDiff; @@ -22,14 +21,18 @@ public ApiResponseDiff(OpenApiDiff openApiDiff) { this.openApiDiff = openApiDiff; } - public Optional diff(@Nullable ApiResponses left, @Nullable ApiResponses right, DiffContext context) { + public Optional diff( + @Nullable ApiResponses left, @Nullable ApiResponses right, DiffContext context) { MapKeyDiff responseMapKeyDiff = MapKeyDiff.diff(left, right); List sharedResponseCodes = responseMapKeyDiff.getSharedKey(); Map resps = new LinkedHashMap<>(); for (String responseCode : sharedResponseCodes) { openApiDiff .getResponseDiff() - .diff(left != null ? left.get(responseCode) : null, right != null ? right.get(responseCode) : null, context) + .diff( + left != null ? left.get(responseCode) : null, + right != null ? right.get(responseCode) : null, + context) .ifPresent(changedResponse -> resps.put(responseCode, changedResponse)); } ChangedApiResponse changedApiResponse = @@ -39,7 +42,10 @@ public Optional diff(@Nullable ApiResponses left, @Nullable .setChanged(resps); openApiDiff .getExtensionsDiff() - .diff(left != null ? left.getExtensions() : null, right != null ? right.getExtensions() : null, context) + .diff( + left != null ? left.getExtensions() : null, + right != null ? right.getExtensions() : null, + context) .ifPresent(changedApiResponse::setExtensions); return isChanged(changedApiResponse); } diff --git a/core/src/main/java/org/openapitools/openapidiff/core/compare/PathsDiff.java b/core/src/main/java/org/openapitools/openapidiff/core/compare/PathsDiff.java index e3a74345..1642c534 100644 --- a/core/src/main/java/org/openapitools/openapidiff/core/compare/PathsDiff.java +++ b/core/src/main/java/org/openapitools/openapidiff/core/compare/PathsDiff.java @@ -45,25 +45,26 @@ public Optional diff( Optional> result = changedPaths.getIncreased().entrySet().stream() .filter(item -> normalizePath(item.getKey()).equals(template)) - .min((a, b) -> { - if (methodsIntersect(a.getValue(), b.getValue())) { - throw new IllegalArgumentException( + .min( + (a, b) -> { + if (methodsIntersect(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 methodsA = new HashSet<>( - a.getValue().readOperationsMap().keySet()); - methodsA.retainAll(leftPath.readOperationsMap().keySet()); - HashSet methodsB = new HashSet<>( - b.getValue().readOperationsMap().keySet()); - methodsB.retainAll(leftPath.readOperationsMap().keySet()); - return Integer.compare(methodsB.size(), methodsA.size()); - } - }); + } + if (a.getKey().equals(url)) { + return -1; + } else if (b.getKey().equals((url))) { + return 1; + } else { + HashSet methodsA = + new HashSet<>(a.getValue().readOperationsMap().keySet()); + methodsA.retainAll(leftPath.readOperationsMap().keySet()); + HashSet methodsB = + new HashSet<>(b.getValue().readOperationsMap().keySet()); + methodsB.retainAll(leftPath.readOperationsMap().keySet()); + return Integer.compare(methodsB.size(), methodsA.size()); + } + }); if (result.isPresent()) { String rightUrl = result.get().getKey(); PathItem rightPath = changedPaths.getIncreased().remove(rightUrl); diff --git a/core/src/main/java/org/openapitools/openapidiff/core/compare/SecurityRequirementDiff.java b/core/src/main/java/org/openapitools/openapidiff/core/compare/SecurityRequirementDiff.java index 6c058ab0..92cc2a21 100644 --- a/core/src/main/java/org/openapitools/openapidiff/core/compare/SecurityRequirementDiff.java +++ b/core/src/main/java/org/openapitools/openapidiff/core/compare/SecurityRequirementDiff.java @@ -35,11 +35,19 @@ public static SecurityRequirement getCopy(LinkedHashMap> ri private LinkedHashMap> contains( SecurityRequirement right, String schemeRef) { - SecurityScheme leftSecurityScheme = leftComponents.getSecuritySchemes().get(schemeRef); + SecurityScheme leftSecurityScheme = getSecurityScheme(schemeRef, leftComponents); + LinkedHashMap> found = new LinkedHashMap<>(); for (Map.Entry> entry : right.entrySet()) { - SecurityScheme rightSecurityScheme = rightComponents.getSecuritySchemes().get(entry.getKey()); + + SecurityScheme rightSecurityScheme = getSecurityScheme(entry.getKey(), rightComponents); + + if (leftSecurityScheme == null || rightSecurityScheme == null) { + found.put(entry.getKey(), entry.getValue()); + return found; + } + if (leftSecurityScheme.getType() == rightSecurityScheme.getType()) { switch (leftSecurityScheme.getType()) { case APIKEY: @@ -61,6 +69,11 @@ private LinkedHashMap> contains( return found; } + private SecurityScheme getSecurityScheme(String schemeRef, Components components) { + final Map securitySchemeMap = components.getSecuritySchemes(); + return securitySchemeMap == null ? null : securitySchemeMap.get(schemeRef); + } + public Optional diff( SecurityRequirement left, SecurityRequirement right, DiffContext context) { ChangedSecurityRequirement changedSecurityRequirement = diff --git a/core/src/main/java/org/openapitools/openapidiff/core/compare/SecurityRequirementsDiff.java b/core/src/main/java/org/openapitools/openapidiff/core/compare/SecurityRequirementsDiff.java index 30b99a54..1bdca446 100644 --- a/core/src/main/java/org/openapitools/openapidiff/core/compare/SecurityRequirementsDiff.java +++ b/core/src/main/java/org/openapitools/openapidiff/core/compare/SecurityRequirementsDiff.java @@ -5,10 +5,7 @@ import io.swagger.v3.oas.models.Components; import io.swagger.v3.oas.models.security.SecurityRequirement; import io.swagger.v3.oas.models.security.SecurityScheme; -import java.util.ArrayList; -import java.util.List; -import java.util.Map; -import java.util.Optional; +import java.util.*; import java.util.stream.Collectors; import org.apache.commons.collections4.CollectionUtils; import org.apache.commons.lang3.tuple.ImmutablePair; @@ -54,26 +51,26 @@ public boolean same(SecurityRequirement left, SecurityRequirement right) { private List> getListOfSecuritySchemes( Components components, SecurityRequirement securityRequirement) { return securityRequirement.keySet().stream() - .map( - x -> { - Map securitySchemes = components.getSecuritySchemes(); - if (securitySchemes == null) { - throw new IllegalArgumentException("Missing securitySchemes component definition."); - } - - SecurityScheme result = securitySchemes.get(x); - if (result == null) { - throw new IllegalArgumentException("Impossible to find security scheme: " + x); - } - - return result; - }) + .map(key -> getSecurityScheme(components, key)) .map(this::getPair) + .filter(Objects::nonNull) .distinct() .collect(Collectors.toList()); } + private SecurityScheme getSecurityScheme(Components components, String key) { + Map securitySchemes = components.getSecuritySchemes(); + if (securitySchemes == null) { + return null; + } + + return securitySchemes.get(key); + } + private Pair getPair(SecurityScheme securityScheme) { + if (securityScheme == null) { + return null; + } return new ImmutablePair<>(securityScheme.getType(), securityScheme.getIn()); } diff --git a/core/src/main/java/org/openapitools/openapidiff/core/compare/SecuritySchemeDiff.java b/core/src/main/java/org/openapitools/openapidiff/core/compare/SecuritySchemeDiff.java index cf11b0d1..b61dd7fe 100644 --- a/core/src/main/java/org/openapitools/openapidiff/core/compare/SecuritySchemeDiff.java +++ b/core/src/main/java/org/openapitools/openapidiff/core/compare/SecuritySchemeDiff.java @@ -4,10 +4,7 @@ import io.swagger.v3.oas.models.Components; import io.swagger.v3.oas.models.security.SecurityScheme; -import java.util.HashSet; -import java.util.List; -import java.util.Objects; -import java.util.Optional; +import java.util.*; import org.openapitools.openapidiff.core.model.ChangedSecurityScheme; import org.openapitools.openapidiff.core.model.ChangedSecuritySchemeScopes; import org.openapitools.openapidiff.core.model.DiffContext; @@ -36,8 +33,10 @@ public Optional diff( String rightSchemeRef, List rightScopes, DiffContext context) { - SecurityScheme leftSecurityScheme = leftComponents.getSecuritySchemes().get(leftSchemeRef); - SecurityScheme rightSecurityScheme = rightComponents.getSecuritySchemes().get(rightSchemeRef); + SecurityScheme leftSecurityScheme = + getSecurityScheme(leftComponents.getSecuritySchemes(), leftSchemeRef); + SecurityScheme rightSecurityScheme = + getSecurityScheme(rightComponents.getSecuritySchemes(), rightSchemeRef); Optional changedSecuritySchemeOpt = cachedDiff( new HashSet<>(), @@ -52,6 +51,7 @@ public Optional diff( changedSecurityScheme = getCopyWithoutScopes(changedSecurityScheme); if (changedSecurityScheme != null + && leftSecurityScheme != null && leftSecurityScheme.getType() == SecurityScheme.Type.OAUTH2) { isChanged(ListDiff.diff(new ChangedSecuritySchemeScopes(leftScopes, rightScopes))) .ifPresent(changedSecurityScheme::setChangedScopes); @@ -60,6 +60,11 @@ public Optional diff( return isChanged(changedSecurityScheme); } + private SecurityScheme getSecurityScheme( + Map securitySchemes, String leftSchemeRef) { + return securitySchemes == null ? null : securitySchemes.get(leftSchemeRef); + } + @Override protected Optional computeDiff( HashSet refSet, @@ -69,6 +74,10 @@ protected Optional computeDiff( ChangedSecurityScheme changedSecurityScheme = new ChangedSecurityScheme(leftSecurityScheme, rightSecurityScheme); + if (leftSecurityScheme == null || rightSecurityScheme == null) { + return Optional.of(changedSecurityScheme); + } + openApiDiff .getMetadataDiff() .diff(leftSecurityScheme.getDescription(), rightSecurityScheme.getDescription(), context) diff --git a/core/src/main/java/org/openapitools/openapidiff/core/compare/schemadiffresult/ComposedSchemaDiffResult.java b/core/src/main/java/org/openapitools/openapidiff/core/compare/schemadiffresult/ComposedSchemaDiffResult.java index ea681db1..6c1a3ba4 100644 --- a/core/src/main/java/org/openapitools/openapidiff/core/compare/schemadiffresult/ComposedSchemaDiffResult.java +++ b/core/src/main/java/org/openapitools/openapidiff/core/compare/schemadiffresult/ComposedSchemaDiffResult.java @@ -90,7 +90,8 @@ public , X> Optional diff( } } - private Map getSchema(Components components, Map mapping, ComposedSchema composedSchema) { + private Map getSchema( + Components components, Map mapping, ComposedSchema composedSchema) { Map result = new LinkedHashMap<>(); mapping.forEach( (key, value) -> result.put(key, refPointer.resolveRef(components, new Schema(), value))); @@ -106,7 +107,7 @@ private Map getMapping(ComposedSchema composedSchema) { for (Schema schema : composedSchema.getOneOf()) { String ref = schema.get$ref(); if (ref == null) { - continue; + continue; } String schemaName = refPointer.getRefName(ref); if (schemaName == null) { diff --git a/core/src/main/java/org/openapitools/openapidiff/core/utils/RefPointer.java b/core/src/main/java/org/openapitools/openapidiff/core/utils/RefPointer.java index 53c81e0e..4f7125f8 100644 --- a/core/src/main/java/org/openapitools/openapidiff/core/utils/RefPointer.java +++ b/core/src/main/java/org/openapitools/openapidiff/core/utils/RefPointer.java @@ -17,7 +17,7 @@ public T resolveRef(Components components, T t, String ref) { String refName = getRefName(ref); T result = getMap(components).get(refName); if (result == null) { - throw new IllegalArgumentException(String.format("ref '%s' doesn't exist.", ref)); + return null; } return result; } @@ -57,7 +57,7 @@ public String getRefName(String ref) { final String baseRef = getBaseRefForType(refType.getName()); if (!ref.startsWith(baseRef)) { - throw new IllegalArgumentException("Invalid ref: " + ref); + return ref; } return ref.substring(baseRef.length()); } diff --git a/core/src/test/java/org/openapitools/openapidiff/core/OneOfDiffTest.java b/core/src/test/java/org/openapitools/openapidiff/core/OneOfDiffTest.java index cf893c58..af9506cb 100644 --- a/core/src/test/java/org/openapitools/openapidiff/core/OneOfDiffTest.java +++ b/core/src/test/java/org/openapitools/openapidiff/core/OneOfDiffTest.java @@ -44,6 +44,7 @@ public void testComposedSchema() { public void testComposedSchemaDiff() { assertOpenApiAreEquals(OPENAPI_DOC10, OPENAPI_DOC10); } + @Test public void testOneOfDiscrimitatorChanged() { // The oneOf 'discriminator' changed: 'realtype' -> 'othertype': 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 6d136fb2..1c8168c4 100644 --- a/core/src/test/java/org/openapitools/openapidiff/core/PathDiffTest.java +++ b/core/src/test/java/org/openapitools/openapidiff/core/PathDiffTest.java @@ -30,7 +30,9 @@ public void testSameTemplateDifferentMethods() { ChangedOpenApi changedOpenApi = OpenApiCompare.fromLocations(OPENAPI_PATH1, OPENAPI_PATH4); assertThat(changedOpenApi.getNewEndpoints()) .hasSize(1) - .satisfiesExactly(endpoint -> assertThat(endpoint.getOperation().getOperationId()).isEqualTo("deletePet")); + .satisfiesExactly( + endpoint -> + assertThat(endpoint.getOperation().getOperationId()).isEqualTo("deletePet")); assertThat(changedOpenApi.isCompatible()).isTrue(); } } diff --git a/core/src/test/java/org/openapitools/openapidiff/core/SecurityDiffTest.java b/core/src/test/java/org/openapitools/openapidiff/core/SecurityDiffTest.java index 056b9231..08736454 100644 --- a/core/src/test/java/org/openapitools/openapidiff/core/SecurityDiffTest.java +++ b/core/src/test/java/org/openapitools/openapidiff/core/SecurityDiffTest.java @@ -1,7 +1,7 @@ package org.openapitools.openapidiff.core; import static org.assertj.core.api.Assertions.assertThat; -import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.*; import io.swagger.v3.oas.models.security.SecurityRequirement; import org.junit.jupiter.api.Test; @@ -87,11 +87,10 @@ public void testDiffDifferent() { @Test public void testWithUnknownSecurityScheme() { - assertThrows( - IllegalArgumentException.class, - () -> OpenApiCompare.fromLocations(OPENAPI_DOC3, OPENAPI_DOC3)); - assertThrows( - IllegalArgumentException.class, - () -> OpenApiCompare.fromLocations(OPENAPI_DOC4, OPENAPI_DOC4)); + final ChangedOpenApi changedOpenApi = OpenApiCompare.fromLocations(OPENAPI_DOC3, OPENAPI_DOC3); + assertTrue(changedOpenApi.getChangedOperations().isEmpty()); + + final ChangedOpenApi changedOpenApi1 = OpenApiCompare.fromLocations(OPENAPI_DOC4, OPENAPI_DOC4); + assertNotNull(changedOpenApi1); } } diff --git a/core/src/test/java/org/openapitools/openapidiff/core/compare/ApiResponseDiffTest.java b/core/src/test/java/org/openapitools/openapidiff/core/compare/ApiResponseDiffTest.java index 59ad6cac..97b8ab09 100644 --- a/core/src/test/java/org/openapitools/openapidiff/core/compare/ApiResponseDiffTest.java +++ b/core/src/test/java/org/openapitools/openapidiff/core/compare/ApiResponseDiffTest.java @@ -1,27 +1,27 @@ package org.openapitools.openapidiff.core.compare; -import org.junit.jupiter.api.Assertions; -import org.junit.jupiter.api.Test; - import static org.openapitools.openapidiff.core.TestUtils.assertOpenApiBackwardCompatible; import static org.openapitools.openapidiff.core.TestUtils.assertOpenApiBackwardIncompatible; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; class ApiResponseDiffTest extends Assertions { - /*** - * This is a regression test - when no responses were set, we would get an exception - * since the OpenAPI object has a `null` ApiResponses field. - */ - @Test - public void testNoResponseInPrevious() { - // The previous API had no response, so adding a response shape is still compatible. - assertOpenApiBackwardCompatible( - "backwardCompatibility/apiResponse_diff_1.yaml", - "backwardCompatibility/apiResponse_diff_2.yaml", true); + /** + * * This is a regression test - when no responses were set, we would get an exception since the + * OpenAPI object has a `null` ApiResponses field. + */ + @Test + public void testNoResponseInPrevious() { + // The previous API had no response, so adding a response shape is still compatible. + assertOpenApiBackwardCompatible( + "backwardCompatibility/apiResponse_diff_1.yaml", + "backwardCompatibility/apiResponse_diff_2.yaml", + true); - // Removing the response shape is backwards incompatible. - assertOpenApiBackwardIncompatible( - "backwardCompatibility/apiResponse_diff_2.yaml", - "backwardCompatibility/apiResponse_diff_1.yaml"); - } -} \ No newline at end of file + // Removing the response shape is backwards incompatible. + assertOpenApiBackwardIncompatible( + "backwardCompatibility/apiResponse_diff_2.yaml", + "backwardCompatibility/apiResponse_diff_1.yaml"); + } +} diff --git a/pom.xml b/pom.xml index 916a5a63..c6260d3c 100644 --- a/pom.xml +++ b/pom.xml @@ -7,7 +7,7 @@ org.openapitools.openapidiff openapi-diff-parent - 2.0.0-SNAPSHOT + 2.0.1-SNAPSHOT pom openapi-diff-parent From 2477525e475d7d713f9acdf15215e86be6350663 Mon Sep 17 00:00:00 2001 From: vijayvepakomma Date: Thu, 13 May 2021 10:58:51 -0400 Subject: [PATCH 2/2] revert versions to fix CircleCI --- cli/pom.xml | 2 +- core/pom.xml | 2 +- pom.xml | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/cli/pom.xml b/cli/pom.xml index 66c18fb2..5bed4b12 100644 --- a/cli/pom.xml +++ b/cli/pom.xml @@ -4,7 +4,7 @@ org.openapitools.openapidiff openapi-diff-parent - 2.0.1-SNAPSHOT + 2.0.0-SNAPSHOT openapi-diff-cli diff --git a/core/pom.xml b/core/pom.xml index 261e92e8..a3a9d494 100644 --- a/core/pom.xml +++ b/core/pom.xml @@ -4,7 +4,7 @@ org.openapitools.openapidiff openapi-diff-parent - 2.0.1-SNAPSHOT + 2.0.0-SNAPSHOT openapi-diff-core diff --git a/pom.xml b/pom.xml index c6260d3c..916a5a63 100644 --- a/pom.xml +++ b/pom.xml @@ -7,7 +7,7 @@ org.openapitools.openapidiff openapi-diff-parent - 2.0.1-SNAPSHOT + 2.0.0-SNAPSHOT pom openapi-diff-parent