From 1a604587908fda29aafadb83eaab3805f3551646 Mon Sep 17 00:00:00 2001 From: I857847 Date: Mon, 19 Jul 2021 10:40:44 -0600 Subject: [PATCH 1/2] Demonstrate performance problems with diff by diffing a large and interconnected schema. This new test generates a large, interconnected api schema with 200 endpoints, 250 model schemas, and references between the models. This generated schema is similar to a real-world schema that we use in production at my job, that failed to diff because it never completed its diff computation. --- .../openapidiff/core/LargeSchemaTest.java | 163 ++++++++++++++++++ 1 file changed, 163 insertions(+) create mode 100644 core/src/test/java/org/openapitools/openapidiff/core/LargeSchemaTest.java diff --git a/core/src/test/java/org/openapitools/openapidiff/core/LargeSchemaTest.java b/core/src/test/java/org/openapitools/openapidiff/core/LargeSchemaTest.java new file mode 100644 index 000000000..2086b016d --- /dev/null +++ b/core/src/test/java/org/openapitools/openapidiff/core/LargeSchemaTest.java @@ -0,0 +1,163 @@ +package org.openapitools.openapidiff.core; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.openapitools.openapidiff.core.TestUtils.assertOpenApiAreEquals; +import static org.openapitools.openapidiff.core.TestUtils.assertOpenApiBackwardIncompatible; +import static org.slf4j.LoggerFactory.getLogger; + +import io.swagger.v3.core.util.Yaml; +import io.swagger.v3.oas.models.Components; +import io.swagger.v3.oas.models.OpenAPI; +import io.swagger.v3.oas.models.Operation; +import io.swagger.v3.oas.models.PathItem; +import io.swagger.v3.oas.models.Paths; +import io.swagger.v3.oas.models.media.ArraySchema; +import io.swagger.v3.oas.models.media.Content; +import io.swagger.v3.oas.models.media.MediaType; +import io.swagger.v3.oas.models.media.ObjectSchema; +import io.swagger.v3.oas.models.media.Schema; +import io.swagger.v3.oas.models.media.StringSchema; +import io.swagger.v3.oas.models.parameters.RequestBody; +import io.swagger.v3.oas.models.responses.ApiResponse; +import io.swagger.v3.oas.models.responses.ApiResponses; +import java.io.File; +import java.io.IOException; +import java.util.ArrayList; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import java.util.Random; +import org.junit.jupiter.api.Test; +import org.openapitools.openapidiff.core.model.ChangedOpenApi; +import org.slf4j.Logger; + +/** Created by jonathan.hess on 2020-07-16. */ +public class LargeSchemaTest { + public static final Logger LOG = getLogger(LargeSchemaTest.class); + + @Test + public void testGeneratedApiSame() { + OpenAPI generated = largeGeneratedApi(); + assertOpenApiAreEquals(generated, generated); + } + + @Test + public void testGeneratedApiDifferent() { + OpenAPI generated = largeGeneratedApi(); + OpenAPI generated2 = largeGeneratedApi(); + assertOpenApiBackwardIncompatible(generated, generated2); + } + + public static void assertOpenApiAreEquals(OpenAPI oldSpec, OpenAPI newSpec) { + ChangedOpenApi changedOpenApi = OpenApiCompare.fromSpecifications(oldSpec, newSpec); + LOG.info("Result: {}", changedOpenApi.isChanged().getValue()); + assertThat(changedOpenApi.getNewEndpoints()).isEmpty(); + assertThat(changedOpenApi.getMissingEndpoints()).isEmpty(); + assertThat(changedOpenApi.getChangedOperations()).isEmpty(); + } + + public static void assertOpenApiBackwardIncompatible(OpenAPI oldSpec, OpenAPI newSpec) { + ChangedOpenApi changedOpenApi = OpenApiCompare.fromSpecifications(oldSpec, newSpec); + LOG.info("Result: {}", changedOpenApi.isChanged().getValue()); + assertThat(changedOpenApi.isDifferent()).isTrue(); + assertThat(changedOpenApi.isIncompatible()).isTrue(); + } + + public OpenAPI largeGeneratedApi() { + final int modelCount = 200; + final int arrayCount = 50; + final int refPerModelCount = 5; + final int endpointCount = 20; + + Random random = new Random(); + + OpenAPI api = new OpenAPI(); + api.setPaths(new Paths()); + api.setComponents(new Components()); + + // create 200 model schemas + Map schemas = new LinkedHashMap<>(); + api.getComponents().setSchemas(schemas); + for (int i = 0; i < modelCount; i++) { + ObjectSchema schema = new ObjectSchema(); + schema.setProperties(new LinkedHashMap<>()); + schema.getProperties().put("name", new StringSchema()); + schema.getProperties().put("description", new StringSchema()); + List required = new ArrayList<>(); + required.add("name"); + schema.setRequired(required); + schemas.put(modelName(i), schema); + } + + // create 50 array schemas + for (int i = modelCount; i < modelCount + arrayCount; i++) { + ArraySchema arraySchema = new ArraySchema(); + arraySchema.setItems(refSchema(i)); + schemas.put(modelName(i), arraySchema); + } + + // list of schema names + + // Create cyclic properties on schemas, make the refs required + schemas.values().stream() + .filter(schema -> schema instanceof ObjectSchema) + .map(schema -> (ObjectSchema) schema) + .forEach( + schema -> { + for (int i = 0; i < refPerModelCount; i++) { + int schemaNumber = random.nextInt(modelCount + arrayCount); + String propertyName = "refTo" + schemaNumber; + schema.getProperties().put(propertyName, refSchema(schemaNumber)); + schema.getRequired().add(propertyName); + } + }); + + // generated endpoints + for (int i = 0; i < endpointCount; i++) { + String path = "/endpoint" + i; + PathItem pathItem = new PathItem(); + Operation operation = new Operation(); + pathItem.post(operation); + + operation.setRequestBody( + new RequestBody() + .content( + new Content() + .addMediaType( + "application/json", new MediaType().schema(refSchema(i % modelCount))))); + ApiResponse responseOk = + new ApiResponses() + .put( + "200", + new ApiResponse() + .content( + new Content() + .addMediaType( + "application/json", + new MediaType().schema(refSchema(i % modelCount))))); + ApiResponses responses = new ApiResponses(); + responses.put("200", responseOk); + operation.setResponses(responses); + api.getPaths().put(path, pathItem); + } + + try { + LOG.info("Printing schema to target/large-api.yaml"); + Yaml.pretty().writeValue(new File("target/large-api.yaml"), api); + } catch (IOException e) { + throw new RuntimeException(e); + } + return api; + } + + private Schema refSchema(final int i) { + String itemModelName = modelName(i); + Schema refSchema = new Schema(); + refSchema.set$ref("#/components/schemas/" + itemModelName); + return refSchema; + } + + private String modelName(final int i) { + return String.format("Model%03d", i); + } +} From 0f23f4c1989998f61a91f488a30caa6f1e8f50b8 Mon Sep 17 00:00:00 2001 From: I857847 Date: Mon, 19 Jul 2021 17:08:24 -0600 Subject: [PATCH 2/2] Conversion to DeferredChanged My employer has an API schema with a lot of deeply nested and recursively referenced objects. We wanted to validate that changes made by developers are backwards compatible. Unfortunately the OpenAPI-Diff tool would run practically forever. There were too many situations where it would have to recompute a diff and could not use the cached result. I implemented an approach that defers computing schema diffs until the entire structure of the API schema has been parsed. This prevents recursive schema definitions from being computed over and over again during parsing. It also ensures that diffs are only computed exactly one time, not recomputed. All this reduces the computational complexity of parsing this big, recursive schema to a manageable time, and avoids recomputing diffs. == Test case I have created a test case: `LargeSchemaTest` that generates a schema similar to the one my employer uses. (Unfortunately our schema is for an internal system and I can't share it.) It will generate similar, but incompatible schemas. These schemas each have: - 250 schemas defined in #/components/schemas, each with 5 properties recursively referencing other schemas defined in #/components schemas. - 100 api endpoints that use those schemas in the RequestBody or ResponseBody. When this test on the `master` branch openapi-diff code, it will not complete. When you profile, you will find that the time is spent in `Changed.isChanged()` which recursively calls other instances of `Changed`. The deep recursion causes an exponential explosion of the number of calls required to compute changed for the whole model. == The solution: Deferring computation of diffs The solution is to break the diff into a two step process: - Step 1: Read the schema and align all the diff computations, deferring computation of actual differences, and avoiding recursive differences. - Step 2: Compute all the differences, avoiding recomputing the recursive differences. This is implemented in [OpenApiDiff.compare()](core/src/main/java/org/openapitools/openapidiff/core/compare/OpenApiDiff.java#L89-L127)] Implementing this was a relatively small change to the code. The `DeferredSchemaCache` holds the cache of SchemaDiffs. It is able to distinguish multiple requests for the same differences. This is the key to avoiding recomputing the same difference multiple times. I replaced all the `Optional diff(...)` with `DeferredChanged diff(...)`, and chose an interface for `DeferredChanged` that matched the `Optional` interface. This minimized the lines of code changed, making it easier to review. Finally, I created a helper object called `DeferredBuilder` which simplifies the task of collecting a bunch of `DeferedChanged` instances together to make composing a change easier to program and read. --- .../openapitools/openapidiff/cli/Main.java | 5 +- .../core/compare/ApiResponseDiff.java | 38 ++-- .../openapidiff/core/compare/ContentDiff.java | 64 ++++--- .../openapidiff/core/compare/HeaderDiff.java | 41 ++-- .../openapidiff/core/compare/HeadersDiff.java | 25 ++- .../openapidiff/core/compare/OpenApiDiff.java | 29 ++- .../core/compare/OperationDiff.java | 118 +++++++++--- .../core/compare/ParameterDiff.java | 43 +++-- .../core/compare/ParametersDiff.java | 16 +- .../openapidiff/core/compare/PathDiff.java | 29 +-- .../openapidiff/core/compare/PathsDiff.java | 53 +++--- .../core/compare/ReferenceDiffCache.java | 28 +-- .../core/compare/RequestBodyDiff.java | 40 ++-- .../core/compare/ResponseDiff.java | 47 +++-- .../openapidiff/core/compare/SchemaDiff.java | 42 ++++- .../core/compare/SecurityRequirementDiff.java | 36 ++-- .../compare/SecurityRequirementsDiff.java | 19 +- .../core/compare/SecuritySchemeDiff.java | 37 ++-- .../ArraySchemaDiffResult.java | 36 ++-- .../ComposedSchemaDiffResult.java | 51 +++-- .../schemadiffresult/SchemaDiffResult.java | 66 ++++--- .../openapidiff/core/model/Change.java | 2 +- .../core/model/ChangedOpenApi.java | 14 +- .../openapidiff/core/model/ChangedSchema.java | 118 ++++++++++-- .../core/model/deferred/DeferredBuilder.java | 106 +++++++++++ .../core/model/deferred/DeferredChanged.java | 38 ++++ .../core/model/deferred/DeferredLogger.java | 71 +++++++ .../model/deferred/DeferredSchemaCache.java | 122 ++++++++++++ .../core/model/deferred/PendingChanged.java | 169 +++++++++++++++++ .../core/model/deferred/RealizedChanged.java | 64 +++++++ .../model/deferred/RecursiveSchemaSet.java | 18 ++ .../model/deferred/SchemaDiffOperation.java | 26 +++ .../openapidiff/core/OneOfDiffTest.java | 1 + .../openapidiff/core/PathDiffTest.java | 4 +- .../openapidiff/core/RecursiveSchemaTest.java | 6 + .../openapidiff/core/SecurityDiffTest.java | 4 +- .../core/compare/ApiResponseDiffTest.java | 38 ++-- .../deferred/DeferredBuilderChangedTest.java | 175 ++++++++++++++++++ .../src/test/resources/recursive_model_3.yaml | 37 ++++ 39 files changed, 1522 insertions(+), 354 deletions(-) create mode 100644 core/src/main/java/org/openapitools/openapidiff/core/model/deferred/DeferredBuilder.java create mode 100644 core/src/main/java/org/openapitools/openapidiff/core/model/deferred/DeferredChanged.java create mode 100644 core/src/main/java/org/openapitools/openapidiff/core/model/deferred/DeferredLogger.java create mode 100644 core/src/main/java/org/openapitools/openapidiff/core/model/deferred/DeferredSchemaCache.java create mode 100644 core/src/main/java/org/openapitools/openapidiff/core/model/deferred/PendingChanged.java create mode 100644 core/src/main/java/org/openapitools/openapidiff/core/model/deferred/RealizedChanged.java create mode 100644 core/src/main/java/org/openapitools/openapidiff/core/model/deferred/RecursiveSchemaSet.java create mode 100644 core/src/main/java/org/openapitools/openapidiff/core/model/deferred/SchemaDiffOperation.java create mode 100644 core/src/test/java/org/openapitools/openapidiff/core/model/deferred/DeferredBuilderChangedTest.java create mode 100644 core/src/test/resources/recursive_model_3.yaml diff --git a/cli/src/main/java/org/openapitools/openapidiff/cli/Main.java b/cli/src/main/java/org/openapitools/openapidiff/cli/Main.java index 6736a5fd7..f9d24d490 100644 --- a/cli/src/main/java/org/openapitools/openapidiff/cli/Main.java +++ b/cli/src/main/java/org/openapitools/openapidiff/cli/Main.java @@ -1,14 +1,11 @@ package org.openapitools.openapidiff.cli; +import io.swagger.v3.parser.core.models.AuthorizationValue; import java.io.File; import java.io.IOException; import java.nio.charset.StandardCharsets; -import java.util.Arrays; import java.util.Collections; import java.util.List; -import java.util.stream.Collectors; - -import io.swagger.v3.parser.core.models.AuthorizationValue; import org.apache.commons.cli.CommandLine; import org.apache.commons.cli.CommandLineParser; import org.apache.commons.cli.DefaultParser; 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 669a4fded..5c5a0bde7 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 @@ -1,18 +1,17 @@ package org.openapitools.openapidiff.core.compare; -import static org.openapitools.openapidiff.core.utils.ChangedUtils.isChanged; - import io.swagger.v3.oas.models.responses.ApiResponse; import io.swagger.v3.oas.models.responses.ApiResponses; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; -import java.util.Optional; +import javax.annotation.Nullable; +import org.openapitools.openapidiff.core.model.Changed; 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; +import org.openapitools.openapidiff.core.model.deferred.DeferredBuilder; +import org.openapitools.openapidiff.core.model.deferred.DeferredChanged; /** Created by adarsh.sharma on 04/01/18. */ public class ApiResponseDiff { @@ -22,14 +21,22 @@ public ApiResponseDiff(OpenApiDiff openApiDiff) { this.openApiDiff = openApiDiff; } - public Optional diff(@Nullable ApiResponses left, @Nullable ApiResponses right, DiffContext context) { + public DeferredChanged diff( + @Nullable ApiResponses left, @Nullable ApiResponses right, DiffContext context) { MapKeyDiff responseMapKeyDiff = MapKeyDiff.diff(left, right); List sharedResponseCodes = responseMapKeyDiff.getSharedKey(); Map resps = new LinkedHashMap<>(); + DeferredBuilder builder = new DeferredBuilder<>(); + for (String responseCode : sharedResponseCodes) { - openApiDiff - .getResponseDiff() - .diff(left != null ? left.get(responseCode) : null, right != null ? right.get(responseCode) : null, context) + builder + .with( + openApiDiff + .getResponseDiff() + .diff( + left != null ? left.get(responseCode) : null, + right != null ? right.get(responseCode) : null, + context)) .ifPresent(changedResponse -> resps.put(responseCode, changedResponse)); } ChangedApiResponse changedApiResponse = @@ -37,10 +44,15 @@ public Optional diff(@Nullable ApiResponses left, @Nullable .setIncreased(responseMapKeyDiff.getIncreased()) .setMissing(responseMapKeyDiff.getMissing()) .setChanged(resps); - openApiDiff - .getExtensionsDiff() - .diff(left != null ? left.getExtensions() : null, right != null ? right.getExtensions() : null, context) + builder + .with( + openApiDiff + .getExtensionsDiff() + .diff( + left != null ? left.getExtensions() : null, + right != null ? right.getExtensions() : null, + context)) .ifPresent(changedApiResponse::setExtensions); - return isChanged(changedApiResponse); + return builder.buildIsChanged(changedApiResponse); } } diff --git a/core/src/main/java/org/openapitools/openapidiff/core/compare/ContentDiff.java b/core/src/main/java/org/openapitools/openapidiff/core/compare/ContentDiff.java index 0c08f9e49..9193238ac 100644 --- a/core/src/main/java/org/openapitools/openapidiff/core/compare/ContentDiff.java +++ b/core/src/main/java/org/openapitools/openapidiff/core/compare/ContentDiff.java @@ -6,9 +6,12 @@ import io.swagger.v3.oas.models.media.Content; import io.swagger.v3.oas.models.media.MediaType; import java.util.*; +import org.openapitools.openapidiff.core.model.Changed; import org.openapitools.openapidiff.core.model.ChangedContent; import org.openapitools.openapidiff.core.model.ChangedMediaType; import org.openapitools.openapidiff.core.model.DiffContext; +import org.openapitools.openapidiff.core.model.deferred.DeferredBuilder; +import org.openapitools.openapidiff.core.model.deferred.DeferredChanged; public class ContentDiff { @@ -18,32 +21,47 @@ public ContentDiff(OpenApiDiff openApiDiff) { this.openApiDiff = openApiDiff; } - public Optional diff(Content left, Content right, DiffContext context) { + public DeferredChanged diff(Content left, Content right, DiffContext context) { + DeferredBuilder builder = new DeferredBuilder(); MapKeyDiff mediaTypeDiff = MapKeyDiff.diff(left, right); List sharedMediaTypes = mediaTypeDiff.getSharedKey(); Map changedMediaTypes = new LinkedHashMap<>(); - for (String mediaTypeKey : sharedMediaTypes) { - MediaType oldMediaType = left.get(mediaTypeKey); - MediaType newMediaType = right.get(mediaTypeKey); - ChangedMediaType changedMediaType = - new ChangedMediaType(oldMediaType.getSchema(), newMediaType.getSchema(), context); - openApiDiff - .getSchemaDiff() - .diff( - new HashSet<>(), - oldMediaType.getSchema(), - newMediaType.getSchema(), - context.copyWithRequired(true)) - .ifPresent(changedMediaType::setSchema); - if (!isUnchanged(changedMediaType)) { - changedMediaTypes.put(mediaTypeKey, changedMediaType); - } - } - return isChanged( - new ChangedContent(left, right, context) - .setIncreased(mediaTypeDiff.getIncreased()) - .setMissing(mediaTypeDiff.getMissing()) - .setChanged(changedMediaTypes)); + + sharedMediaTypes.stream() + .forEach( + (mediaTypeKey) -> { + MediaType oldMediaType = left.get(mediaTypeKey); + MediaType newMediaType = right.get(mediaTypeKey); + + ChangedMediaType changedMediaType = + new ChangedMediaType(oldMediaType.getSchema(), newMediaType.getSchema(), context); + + builder + .with( + openApiDiff + .getSchemaDiff() + .diff( + oldMediaType.getSchema(), + newMediaType.getSchema(), + context.copyWithRequired(true))) + .ifPresent( + (value) -> { + changedMediaType.setSchema(value); + if (!isUnchanged(changedMediaType)) { + changedMediaTypes.put(mediaTypeKey, changedMediaType); + } + }); + }); + + return builder + .build() + .mapOptional( + (value) -> + isChanged( + new ChangedContent(left, right, context) + .setIncreased(mediaTypeDiff.getIncreased()) + .setMissing(mediaTypeDiff.getMissing()) + .setChanged(changedMediaTypes))); } } diff --git a/core/src/main/java/org/openapitools/openapidiff/core/compare/HeaderDiff.java b/core/src/main/java/org/openapitools/openapidiff/core/compare/HeaderDiff.java index 343d044f1..e21f028f4 100644 --- a/core/src/main/java/org/openapitools/openapidiff/core/compare/HeaderDiff.java +++ b/core/src/main/java/org/openapitools/openapidiff/core/compare/HeaderDiff.java @@ -1,14 +1,15 @@ package org.openapitools.openapidiff.core.compare; -import static org.openapitools.openapidiff.core.utils.ChangedUtils.isChanged; - import io.swagger.v3.oas.models.Components; import io.swagger.v3.oas.models.headers.Header; import java.util.HashSet; import java.util.Objects; import java.util.Optional; +import org.openapitools.openapidiff.core.model.Changed; import org.openapitools.openapidiff.core.model.ChangedHeader; import org.openapitools.openapidiff.core.model.DiffContext; +import org.openapitools.openapidiff.core.model.deferred.DeferredBuilder; +import org.openapitools.openapidiff.core.model.deferred.DeferredChanged; import org.openapitools.openapidiff.core.utils.RefPointer; import org.openapitools.openapidiff.core.utils.RefType; @@ -31,16 +32,17 @@ public HeaderDiff(OpenApiDiff openApiDiff) { : null; } - public Optional diff(Header left, Header right, DiffContext context) { + public DeferredChanged diff(Header left, Header right, DiffContext context) { return cachedDiff(new HashSet<>(), left, right, left.get$ref(), right.get$ref(), context); } @Override - protected Optional computeDiff( + protected DeferredChanged computeDiff( HashSet refSet, Header left, Header right, DiffContext context) { left = refPointer.resolveRef(leftComponents, left, left.get$ref()); right = refPointer.resolveRef(rightComponents, right, right.get$ref()); + DeferredBuilder builder = new DeferredBuilder(); ChangedHeader changedHeader = new ChangedHeader(left, right, context) .setRequired(getBooleanDiff(left.getRequired(), right.getRequired())) @@ -49,23 +51,28 @@ protected Optional computeDiff( && Boolean.TRUE.equals(right.getDeprecated())) .setStyle(!Objects.equals(left.getStyle(), right.getStyle())) .setExplode(getBooleanDiff(left.getExplode(), right.getExplode())); - openApiDiff - .getMetadataDiff() - .diff(left.getDescription(), right.getDescription(), context) + builder + .with( + openApiDiff + .getMetadataDiff() + .diff(left.getDescription(), right.getDescription(), context)) .ifPresent(changedHeader::setDescription); - openApiDiff - .getSchemaDiff() - .diff(new HashSet<>(), left.getSchema(), right.getSchema(), context.copyWithRequired(true)) + builder + .with( + openApiDiff + .getSchemaDiff() + .diff(left.getSchema(), right.getSchema(), context.copyWithRequired(true))) .ifPresent(changedHeader::setSchema); - openApiDiff - .getContentDiff() - .diff(left.getContent(), right.getContent(), context) + builder + .with(openApiDiff.getContentDiff().diff(left.getContent(), right.getContent(), context)) .ifPresent(changedHeader::setContent); - openApiDiff - .getExtensionsDiff() - .diff(left.getExtensions(), right.getExtensions(), context) + builder + .with( + openApiDiff + .getExtensionsDiff() + .diff(left.getExtensions(), right.getExtensions(), context)) .ifPresent(changedHeader::setExtensions); - return isChanged(changedHeader); + return builder.buildIsChanged(changedHeader); } private boolean getBooleanDiff(Boolean left, Boolean right) { diff --git a/core/src/main/java/org/openapitools/openapidiff/core/compare/HeadersDiff.java b/core/src/main/java/org/openapitools/openapidiff/core/compare/HeadersDiff.java index f915f634d..44c8d3085 100644 --- a/core/src/main/java/org/openapitools/openapidiff/core/compare/HeadersDiff.java +++ b/core/src/main/java/org/openapitools/openapidiff/core/compare/HeadersDiff.java @@ -6,10 +6,11 @@ import java.util.LinkedHashMap; import java.util.List; import java.util.Map; -import java.util.Optional; import org.openapitools.openapidiff.core.model.ChangedHeader; import org.openapitools.openapidiff.core.model.ChangedHeaders; import org.openapitools.openapidiff.core.model.DiffContext; +import org.openapitools.openapidiff.core.model.deferred.DeferredBuilder; +import org.openapitools.openapidiff.core.model.deferred.DeferredChanged; /** Created by adarsh.sharma on 28/12/17. */ public class HeadersDiff { @@ -19,24 +20,28 @@ public HeadersDiff(OpenApiDiff openApiDiff) { this.openApiDiff = openApiDiff; } - public Optional diff( + public DeferredChanged diff( Map left, Map right, DiffContext context) { MapKeyDiff headerMapDiff = MapKeyDiff.diff(left, right); List sharedHeaderKeys = headerMapDiff.getSharedKey(); Map changed = new LinkedHashMap<>(); + DeferredBuilder builder = new DeferredBuilder<>(); for (String headerKey : sharedHeaderKeys) { Header oldHeader = left.get(headerKey); Header newHeader = right.get(headerKey); - openApiDiff - .getHeaderDiff() - .diff(oldHeader, newHeader, context) + builder + .with(openApiDiff.getHeaderDiff().diff(oldHeader, newHeader, context)) .ifPresent(changedHeader -> changed.put(headerKey, changedHeader)); } - return isChanged( - new ChangedHeaders(left, right, context) - .setIncreased(headerMapDiff.getIncreased()) - .setMissing(headerMapDiff.getMissing()) - .setChanged(changed)); + return builder + .build() + .mapOptional( + (value) -> + isChanged( + new ChangedHeaders(left, right, context) + .setIncreased(headerMapDiff.getIncreased()) + .setMissing(headerMapDiff.getMissing()) + .setChanged(changed))); } } diff --git a/core/src/main/java/org/openapitools/openapidiff/core/compare/OpenApiDiff.java b/core/src/main/java/org/openapitools/openapidiff/core/compare/OpenApiDiff.java index 7b110769d..9fb292156 100644 --- a/core/src/main/java/org/openapitools/openapidiff/core/compare/OpenApiDiff.java +++ b/core/src/main/java/org/openapitools/openapidiff/core/compare/OpenApiDiff.java @@ -8,14 +8,10 @@ import java.util.ArrayList; import java.util.List; import java.util.Map; -import java.util.Optional; import java.util.stream.Collectors; -import org.openapitools.openapidiff.core.model.ChangedExtensions; -import org.openapitools.openapidiff.core.model.ChangedOpenApi; -import org.openapitools.openapidiff.core.model.ChangedOperation; -import org.openapitools.openapidiff.core.model.ChangedPath; -import org.openapitools.openapidiff.core.model.ChangedPaths; -import org.openapitools.openapidiff.core.model.Endpoint; +import org.openapitools.openapidiff.core.model.*; +import org.openapitools.openapidiff.core.model.deferred.DeferredChanged; +import org.openapitools.openapidiff.core.model.deferred.DeferredSchemaCache; import org.openapitools.openapidiff.core.utils.EndpointUtils; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -48,6 +44,7 @@ public class OpenApiDiff { private List missingEndpoints; private List changedOperations; private ChangedExtensions changedExtensions; + private DeferredSchemaCache deferredSchemaCache; /* * @param oldSpecOpenApi @@ -86,17 +83,25 @@ private void initializeFields() { this.oAuthFlowDiff = new OAuthFlowDiff(this); this.extensionsDiff = new ExtensionsDiff(this); this.metadataDiff = new MetadataDiff(this); + this.deferredSchemaCache = new DeferredSchemaCache(this); } private ChangedOpenApi compare() { preProcess(oldSpecOpenApi); preProcess(newSpecOpenApi); - Optional paths = + + // 1st pass scans paths to collect all schemas + DeferredChanged paths = this.pathsDiff.diff( valOrEmpty(oldSpecOpenApi.getPaths()), valOrEmpty(newSpecOpenApi.getPaths())); + + // 2nd pass processes deferred schemas + deferredSchemaCache.process(); + this.newEndpoints = new ArrayList<>(); this.missingEndpoints = new ArrayList<>(); this.changedOperations = new ArrayList<>(); + paths.ifPresent( changedPaths -> { this.newEndpoints = EndpointUtils.convert2EndpointList(changedPaths.getIncreased()); @@ -117,6 +122,7 @@ private ChangedOpenApi compare() { getExtensionsDiff() .diff(oldSpecOpenApi.getExtensions(), newSpecOpenApi.getExtensions()) .ifPresent(this::setChangedExtension); + return getChangedOpenApi(); } @@ -162,7 +168,12 @@ private ChangedOpenApi getChangedOpenApi() { .setNewSpecOpenApi(newSpecOpenApi) .setOldSpecOpenApi(oldSpecOpenApi) .setChangedOperations(changedOperations) - .setChangedExtensions(changedExtensions); + .setChangedExtensions(changedExtensions) + .setChangedSchemas(deferredSchemaCache.getChangedSchemas()); + } + + public DeferredSchemaCache getDeferredSchemaCache() { + return deferredSchemaCache; } public PathsDiff getPathsDiff() { diff --git a/core/src/main/java/org/openapitools/openapidiff/core/compare/OperationDiff.java b/core/src/main/java/org/openapitools/openapidiff/core/compare/OperationDiff.java index 3a9f0f656..fb7f94742 100644 --- a/core/src/main/java/org/openapitools/openapidiff/core/compare/OperationDiff.java +++ b/core/src/main/java/org/openapitools/openapidiff/core/compare/OperationDiff.java @@ -7,50 +7,75 @@ import java.util.List; import java.util.Map; import java.util.Optional; +import java.util.stream.Collectors; +import org.openapitools.openapidiff.core.model.Changed; import org.openapitools.openapidiff.core.model.ChangedOperation; import org.openapitools.openapidiff.core.model.ChangedParameters; import org.openapitools.openapidiff.core.model.DiffContext; +import org.openapitools.openapidiff.core.model.deferred.DeferredBuilder; +import org.openapitools.openapidiff.core.model.deferred.DeferredChanged; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; /** Created by adarsh.sharma on 04/01/18. */ public class OperationDiff { + private static final Logger log = LoggerFactory.getLogger(OperationDiff.class); + private final OpenApiDiff openApiDiff; public OperationDiff(OpenApiDiff openApiDiff) { this.openApiDiff = openApiDiff; } - public Optional diff( + public DeferredChanged diff( Operation oldOperation, Operation newOperation, DiffContext context) { + + DeferredBuilder builder = new DeferredBuilder<>(); ChangedOperation changedOperation = new ChangedOperation(context.getUrl(), context.getMethod(), oldOperation, newOperation); - openApiDiff - .getMetadataDiff() - .diff(oldOperation.getSummary(), newOperation.getSummary(), context) + log.debug( + "Diff operation {} {}", changedOperation.getPathUrl(), changedOperation.getHttpMethod()); + + builder + .with( + openApiDiff + .getMetadataDiff() + .diff(oldOperation.getSummary(), newOperation.getSummary(), context)) .ifPresent(changedOperation::setSummary); - openApiDiff - .getMetadataDiff() - .diff(oldOperation.getDescription(), newOperation.getDescription(), context) + builder + .with( + openApiDiff + .getMetadataDiff() + .diff(oldOperation.getDescription(), newOperation.getDescription(), context)) .ifPresent(changedOperation::setDescription); - openApiDiff - .getMetadataDiff() - .diff(oldOperation.getOperationId(), newOperation.getOperationId(), context) + builder + .with( + openApiDiff + .getMetadataDiff() + .diff(oldOperation.getOperationId(), newOperation.getOperationId(), context)) .ifPresent(changedOperation::setOperationId); changedOperation.setDeprecated( !Boolean.TRUE.equals(oldOperation.getDeprecated()) && Boolean.TRUE.equals(newOperation.getDeprecated())); if (oldOperation.getRequestBody() != null || newOperation.getRequestBody() != null) { - openApiDiff - .getRequestBodyDiff() - .diff( - oldOperation.getRequestBody(), newOperation.getRequestBody(), context.copyAsRequest()) + builder + .with( + openApiDiff + .getRequestBodyDiff() + .diff( + oldOperation.getRequestBody(), + newOperation.getRequestBody(), + context.copyAsRequest())) .ifPresent(changedOperation::setRequestBody); } - openApiDiff - .getParametersDiff() - .diff(oldOperation.getParameters(), newOperation.getParameters(), context) + builder + .with( + openApiDiff + .getParametersDiff() + .diff(oldOperation.getParameters(), newOperation.getParameters(), context)) .ifPresent( params -> { removePathParameters(context.getParameters(), params); @@ -58,25 +83,62 @@ public Optional diff( }); if (oldOperation.getResponses() != null || newOperation.getResponses() != null) { - openApiDiff - .getApiResponseDiff() - .diff(oldOperation.getResponses(), newOperation.getResponses(), context.copyAsResponse()) - .ifPresent(changedOperation::setApiResponses); + builder + .with( + openApiDiff + .getApiResponseDiff() + .diff( + oldOperation.getResponses(), + newOperation.getResponses(), + context.copyAsResponse())) + .ifPresent( + (responses) -> { + log.debug( + "operation " + + changedOperation.getPathUrl() + + " " + + changedOperation.getHttpMethod() + + " setting api responses " + + responses.getChangedElements().stream() + .filter(c -> c != null) + .map(c -> c.isChanged()) + .filter(c -> c != null) + .map(c -> c.toString()) + .collect(Collectors.joining(","))); + changedOperation.setApiResponses(responses); + }); } if (oldOperation.getSecurity() != null || newOperation.getSecurity() != null) { - openApiDiff - .getSecurityRequirementsDiff() - .diff(oldOperation.getSecurity(), newOperation.getSecurity(), context) + builder + .with( + openApiDiff + .getSecurityRequirementsDiff() + .diff(oldOperation.getSecurity(), newOperation.getSecurity(), context)) .ifPresent(changedOperation::setSecurityRequirements); } - openApiDiff - .getExtensionsDiff() - .diff(oldOperation.getExtensions(), newOperation.getExtensions(), context) + builder + .with( + openApiDiff + .getExtensionsDiff() + .diff(oldOperation.getExtensions(), newOperation.getExtensions(), context)) .ifPresent(changedOperation::setExtensions); - return isChanged(changedOperation); + return builder + .build() + .mapOptional( + (value) -> { + Optional changed = isChanged(changedOperation); + log.debug( + "Is changed operation " + + changedOperation.getPathUrl() + + " " + + changedOperation.getHttpMethod() + + " changed: " + + changed.map(c -> c.isChanged()).orElse(null)); + return changed; + }); } public void removePathParameters(Map pathParameters, ChangedParameters params) { diff --git a/core/src/main/java/org/openapitools/openapidiff/core/compare/ParameterDiff.java b/core/src/main/java/org/openapitools/openapidiff/core/compare/ParameterDiff.java index 37d769f1a..6652b0bc4 100644 --- a/core/src/main/java/org/openapitools/openapidiff/core/compare/ParameterDiff.java +++ b/core/src/main/java/org/openapitools/openapidiff/core/compare/ParameterDiff.java @@ -1,14 +1,15 @@ package org.openapitools.openapidiff.core.compare; -import static org.openapitools.openapidiff.core.utils.ChangedUtils.isChanged; - import io.swagger.v3.oas.models.Components; import io.swagger.v3.oas.models.parameters.Parameter; import java.util.HashSet; import java.util.Objects; import java.util.Optional; +import org.openapitools.openapidiff.core.model.Changed; import org.openapitools.openapidiff.core.model.ChangedParameter; import org.openapitools.openapidiff.core.model.DiffContext; +import org.openapitools.openapidiff.core.model.deferred.DeferredBuilder; +import org.openapitools.openapidiff.core.model.deferred.DeferredChanged; import org.openapitools.openapidiff.core.utils.RefPointer; import org.openapitools.openapidiff.core.utils.RefType; @@ -31,16 +32,19 @@ public ParameterDiff(OpenApiDiff openApiDiff) { : null; } - public Optional diff(Parameter left, Parameter right, DiffContext context) { + public DeferredChanged diff( + Parameter left, Parameter right, DiffContext context) { return cachedDiff(new HashSet<>(), left, right, left.get$ref(), right.get$ref(), context); } @Override - protected Optional computeDiff( + protected DeferredChanged computeDiff( HashSet refSet, Parameter left, Parameter right, DiffContext context) { left = refPointer.resolveRef(this.leftComponents, left, left.get$ref()); right = refPointer.resolveRef(this.rightComponents, right, right.get$ref()); + DeferredBuilder builder = new DeferredBuilder<>(); + ChangedParameter changedParameter = new ChangedParameter(right.getName(), right.getIn(), context) .setOldParameter(left) @@ -53,23 +57,28 @@ protected Optional computeDiff( getBooleanDiff(left.getAllowEmptyValue(), right.getAllowEmptyValue())) .setChangeStyle(!Objects.equals(left.getStyle(), right.getStyle())) .setChangeExplode(getBooleanDiff(left.getExplode(), right.getExplode())); - openApiDiff - .getSchemaDiff() - .diff(refSet, left.getSchema(), right.getSchema(), context.copyWithRequired(true)) + builder + .with( + openApiDiff + .getSchemaDiff() + .diff(left.getSchema(), right.getSchema(), context.copyWithRequired(true))) .ifPresent(changedParameter::setSchema); - openApiDiff - .getMetadataDiff() - .diff(left.getDescription(), right.getDescription(), context) + builder + .with( + openApiDiff + .getMetadataDiff() + .diff(left.getDescription(), right.getDescription(), context)) .ifPresent(changedParameter::setDescription); - openApiDiff - .getContentDiff() - .diff(left.getContent(), right.getContent(), context) + builder + .with(openApiDiff.getContentDiff().diff(left.getContent(), right.getContent(), context)) .ifPresent(changedParameter::setContent); - openApiDiff - .getExtensionsDiff() - .diff(left.getExtensions(), right.getExtensions(), context) + builder + .with( + openApiDiff + .getExtensionsDiff() + .diff(left.getExtensions(), right.getExtensions(), context)) .ifPresent(changedParameter::setExtensions); - return isChanged(changedParameter); + return builder.buildIsChanged(changedParameter); } private boolean getBooleanDiff(Boolean left, Boolean right) { 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 f7fa573b2..5271f363e 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 @@ -1,15 +1,16 @@ package org.openapitools.openapidiff.core.compare; -import static org.openapitools.openapidiff.core.utils.ChangedUtils.isChanged; - import io.swagger.v3.oas.models.Components; import io.swagger.v3.oas.models.parameters.Parameter; import java.util.ArrayList; import java.util.List; import java.util.Objects; import java.util.Optional; +import org.openapitools.openapidiff.core.model.Changed; import org.openapitools.openapidiff.core.model.ChangedParameters; import org.openapitools.openapidiff.core.model.DiffContext; +import org.openapitools.openapidiff.core.model.deferred.DeferredBuilder; +import org.openapitools.openapidiff.core.model.deferred.DeferredChanged; import org.openapitools.openapidiff.core.utils.RefPointer; import org.openapitools.openapidiff.core.utils.RefType; @@ -49,8 +50,10 @@ public static boolean same(Parameter left, Parameter right) { && Objects.equals(left.getIn(), right.getIn()); } - public Optional diff( + 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<>(); @@ -65,14 +68,13 @@ public Optional diff( } else { Parameter rightPara = rightParam.get(); right.remove(rightPara); - openApiDiff - .getParameterDiff() - .diff(leftPara, rightPara, context) + builder + .with(openApiDiff.getParameterDiff().diff(leftPara, rightPara, context)) .ifPresent(changedParameters.getChanged()::add); } } changedParameters.getIncreased().addAll(right); - return isChanged(changedParameters); + return builder.buildIsChanged(changedParameters); } } diff --git a/core/src/main/java/org/openapitools/openapidiff/core/compare/PathDiff.java b/core/src/main/java/org/openapitools/openapidiff/core/compare/PathDiff.java index 1562dfbb4..87494521e 100644 --- a/core/src/main/java/org/openapitools/openapidiff/core/compare/PathDiff.java +++ b/core/src/main/java/org/openapitools/openapidiff/core/compare/PathDiff.java @@ -1,14 +1,14 @@ package org.openapitools.openapidiff.core.compare; -import static org.openapitools.openapidiff.core.utils.ChangedUtils.isChanged; - import io.swagger.v3.oas.models.Operation; import io.swagger.v3.oas.models.PathItem; import java.util.List; import java.util.Map; -import java.util.Optional; +import org.openapitools.openapidiff.core.model.Changed; import org.openapitools.openapidiff.core.model.ChangedPath; import org.openapitools.openapidiff.core.model.DiffContext; +import org.openapitools.openapidiff.core.model.deferred.DeferredBuilder; +import org.openapitools.openapidiff.core.model.deferred.DeferredChanged; public class PathDiff { private final OpenApiDiff openApiDiff; @@ -17,12 +17,14 @@ public PathDiff(OpenApiDiff openApiDiff) { this.openApiDiff = openApiDiff; } - public Optional diff(PathItem left, PathItem right, DiffContext context) { + public DeferredChanged diff(PathItem left, PathItem right, DiffContext context) { Map oldOperationMap = left.readOperationsMap(); Map newOperationMap = right.readOperationsMap(); MapKeyDiff operationsDiff = MapKeyDiff.diff(oldOperationMap, newOperationMap); List sharedMethods = operationsDiff.getSharedKey(); + DeferredBuilder builder = new DeferredBuilder(); + ChangedPath changedPath = new ChangedPath(context.getUrl(), left, right, context) .setIncreased(operationsDiff.getIncreased()) @@ -30,15 +32,20 @@ public Optional diff(PathItem left, PathItem right, DiffContext con for (PathItem.HttpMethod method : sharedMethods) { Operation oldOperation = oldOperationMap.get(method); Operation newOperation = newOperationMap.get(method); - openApiDiff - .getOperationDiff() - .diff(oldOperation, newOperation, context.copyWithMethod(method)) + builder + .with( + openApiDiff + .getOperationDiff() + .diff(oldOperation, newOperation, context.copyWithMethod(method))) .ifPresent(changedPath.getChanged()::add); } - openApiDiff - .getExtensionsDiff() - .diff(left.getExtensions(), right.getExtensions(), context) + builder + .with( + openApiDiff + .getExtensionsDiff() + .diff(left.getExtensions(), right.getExtensions(), context)) .ifPresent(changedPath::setExtensions); - return isChanged(changedPath); + + return builder.buildIsChanged(changedPath); } } 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 e3a743455..7e977b6cd 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 @@ -1,14 +1,15 @@ package org.openapitools.openapidiff.core.compare; -import static org.openapitools.openapidiff.core.utils.ChangedUtils.isChanged; - import io.swagger.v3.oas.models.PathItem; import io.swagger.v3.oas.models.Paths; import java.util.*; import java.util.regex.Matcher; import java.util.regex.Pattern; +import org.openapitools.openapidiff.core.model.Changed; import org.openapitools.openapidiff.core.model.ChangedPaths; import org.openapitools.openapidiff.core.model.DiffContext; +import org.openapitools.openapidiff.core.model.deferred.DeferredBuilder; +import org.openapitools.openapidiff.core.model.deferred.DeferredChanged; public class PathsDiff { private static final String REGEX_PATH = "\\{([^/]+)\\}"; @@ -32,8 +33,10 @@ private static List extractParameters(String path) { return params; } - public Optional diff( + public DeferredChanged diff( final Map left, final Map right) { + DeferredBuilder builder = new DeferredBuilder(); + ChangedPaths changedPaths = new ChangedPaths(left, right); changedPaths.getIncreased().putAll(right); @@ -45,25 +48,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); @@ -78,15 +82,14 @@ public Optional diff( DiffContext context = new DiffContext(); context.setUrl(url); context.setParameters(params); - openApiDiff - .getPathDiff() - .diff(leftPath, rightPath, context) + builder + .with(openApiDiff.getPathDiff().diff(leftPath, rightPath, context)) .ifPresent(path -> changedPaths.getChanged().put(rightUrl, path)); } else { changedPaths.getMissing().put(url, leftPath); } }); - return isChanged(changedPaths); + return builder.buildIsChanged(changedPaths); } public static Paths valOrEmpty(Paths path) { diff --git a/core/src/main/java/org/openapitools/openapidiff/core/compare/ReferenceDiffCache.java b/core/src/main/java/org/openapitools/openapidiff/core/compare/ReferenceDiffCache.java index befe8cff9..bc20f8fa5 100644 --- a/core/src/main/java/org/openapitools/openapidiff/core/compare/ReferenceDiffCache.java +++ b/core/src/main/java/org/openapitools/openapidiff/core/compare/ReferenceDiffCache.java @@ -3,26 +3,28 @@ import java.util.HashMap; import java.util.HashSet; import java.util.Map; -import java.util.Optional; +import org.openapitools.openapidiff.core.model.Changed; import org.openapitools.openapidiff.core.model.DiffContext; +import org.openapitools.openapidiff.core.model.deferred.DeferredChanged; +import org.openapitools.openapidiff.core.model.deferred.RealizedChanged; /** Created by adarsh.sharma on 07/01/18. */ -public abstract class ReferenceDiffCache { - private final Map refDiffMap; +public abstract class ReferenceDiffCache { + private final Map> refDiffMap; public ReferenceDiffCache() { this.refDiffMap = new HashMap<>(); } - private Optional getFromCache(CacheKey cacheKey) { - return Optional.ofNullable(refDiffMap.get(cacheKey)); + private DeferredChanged getFromCache(CacheKey cacheKey) { + return refDiffMap.get(cacheKey); } - private void addToCache(CacheKey cacheKey, D changed) { + private void addToCache(CacheKey cacheKey, DeferredChanged changed) { refDiffMap.put(cacheKey, changed); } - public Optional cachedDiff( + public DeferredChanged cachedDiff( HashSet refSet, C left, C right, @@ -32,17 +34,17 @@ public Optional cachedDiff( boolean areBothRefParameters = leftRef != null && rightRef != null; if (areBothRefParameters) { CacheKey key = new CacheKey(leftRef, rightRef, context); - Optional changedFromRef = getFromCache(key); - if (changedFromRef.isPresent()) { + DeferredChanged changedFromRef = getFromCache(key); + if (changedFromRef != null) { return changedFromRef; } else { String refKey = getRefKey(leftRef, rightRef); if (refSet.contains(refKey)) { - return Optional.empty(); + return RealizedChanged.empty(); } else { refSet.add(refKey); - Optional changed = computeDiff(refSet, left, right, context); - addToCache(key, changed.orElse(null)); + DeferredChanged changed = computeDiff(refSet, left, right, context); + addToCache(key, changed); refSet.remove(refKey); return changed; } @@ -56,6 +58,6 @@ protected String getRefKey(String leftRef, String rightRef) { return leftRef + ":" + rightRef; } - protected abstract Optional computeDiff( + protected abstract DeferredChanged computeDiff( HashSet refSet, C left, C right, DiffContext context); } diff --git a/core/src/main/java/org/openapitools/openapidiff/core/compare/RequestBodyDiff.java b/core/src/main/java/org/openapitools/openapidiff/core/compare/RequestBodyDiff.java index 0a1d8c4a0..22d1fe119 100644 --- a/core/src/main/java/org/openapitools/openapidiff/core/compare/RequestBodyDiff.java +++ b/core/src/main/java/org/openapitools/openapidiff/core/compare/RequestBodyDiff.java @@ -1,15 +1,16 @@ package org.openapitools.openapidiff.core.compare; import static java.util.Optional.ofNullable; -import static org.openapitools.openapidiff.core.utils.ChangedUtils.isChanged; import io.swagger.v3.oas.models.media.Content; import io.swagger.v3.oas.models.parameters.RequestBody; import java.util.HashSet; import java.util.Map; -import java.util.Optional; +import org.openapitools.openapidiff.core.model.Changed; import org.openapitools.openapidiff.core.model.ChangedRequestBody; import org.openapitools.openapidiff.core.model.DiffContext; +import org.openapitools.openapidiff.core.model.deferred.DeferredBuilder; +import org.openapitools.openapidiff.core.model.deferred.DeferredChanged; import org.openapitools.openapidiff.core.utils.RefPointer; import org.openapitools.openapidiff.core.utils.RefType; @@ -27,7 +28,7 @@ private static Map getExtensions(RequestBody body) { return ofNullable(body).map(RequestBody::getExtensions).orElse(null); } - public Optional diff( + public DeferredChanged diff( RequestBody left, RequestBody right, DiffContext context) { String leftRef = left != null ? left.get$ref() : null, rightRef = right != null ? right.get$ref() : null; @@ -35,8 +36,10 @@ public Optional diff( } @Override - protected Optional computeDiff( + protected DeferredChanged computeDiff( HashSet refSet, RequestBody left, RequestBody right, DiffContext context) { + DeferredBuilder builder = new DeferredBuilder(); + Content oldRequestContent = new Content(); Content newRequestContent = new Content(); RequestBody oldRequestBody = null; @@ -65,22 +68,25 @@ protected Optional computeDiff( ChangedRequestBody changedRequestBody = new ChangedRequestBody(oldRequestBody, newRequestBody, context) .setChangeRequired(leftRequired != rightRequired); - openApiDiff - .getMetadataDiff() - .diff( - oldRequestBody != null ? oldRequestBody.getDescription() : null, - newRequestBody != null ? newRequestBody.getDescription() : null, - context) + builder + .with( + openApiDiff + .getMetadataDiff() + .diff( + oldRequestBody != null ? oldRequestBody.getDescription() : null, + newRequestBody != null ? newRequestBody.getDescription() : null, + context)) .ifPresent(changedRequestBody::setDescription); - openApiDiff - .getContentDiff() - .diff(oldRequestContent, newRequestContent, context) + builder + .with(openApiDiff.getContentDiff().diff(oldRequestContent, newRequestContent, context)) .ifPresent(changedRequestBody::setContent); - openApiDiff - .getExtensionsDiff() - .diff(getExtensions(left), getExtensions(right), context) + builder + .with( + openApiDiff + .getExtensionsDiff() + .diff(getExtensions(left), getExtensions(right), context)) .ifPresent(changedRequestBody::setExtensions); - return isChanged(changedRequestBody); + return builder.buildIsChanged(changedRequestBody); } } diff --git a/core/src/main/java/org/openapitools/openapidiff/core/compare/ResponseDiff.java b/core/src/main/java/org/openapitools/openapidiff/core/compare/ResponseDiff.java index 0a84de744..b18b77f4a 100644 --- a/core/src/main/java/org/openapitools/openapidiff/core/compare/ResponseDiff.java +++ b/core/src/main/java/org/openapitools/openapidiff/core/compare/ResponseDiff.java @@ -1,13 +1,15 @@ package org.openapitools.openapidiff.core.compare; -import static org.openapitools.openapidiff.core.utils.ChangedUtils.isChanged; - import io.swagger.v3.oas.models.Components; import io.swagger.v3.oas.models.responses.ApiResponse; import java.util.HashSet; import java.util.Optional; +import org.openapitools.openapidiff.core.model.Changed; import org.openapitools.openapidiff.core.model.ChangedResponse; import org.openapitools.openapidiff.core.model.DiffContext; +import org.openapitools.openapidiff.core.model.deferred.DeferredBuilder; +import org.openapitools.openapidiff.core.model.deferred.DeferredChanged; +import org.openapitools.openapidiff.core.model.deferred.RealizedChanged; import org.openapitools.openapidiff.core.utils.RefPointer; import org.openapitools.openapidiff.core.utils.RefType; @@ -30,33 +32,44 @@ public ResponseDiff(OpenApiDiff openApiDiff) { : null; } - public Optional diff(ApiResponse left, ApiResponse right, DiffContext context) { + public DeferredChanged diff( + ApiResponse left, ApiResponse right, DiffContext context) { + if (left == null && right == null) { + return new RealizedChanged(Optional.empty()); + } + if ((left == null && right != null) || (left != null && right == null)) { + return new RealizedChanged(Optional.of(new ChangedResponse(left, right, context))); + } return cachedDiff(new HashSet<>(), left, right, left.get$ref(), right.get$ref(), context); } @Override - protected Optional computeDiff( + protected DeferredChanged computeDiff( HashSet refSet, ApiResponse left, ApiResponse right, DiffContext context) { left = refPointer.resolveRef(leftComponents, left, left.get$ref()); right = refPointer.resolveRef(rightComponents, right, right.get$ref()); + DeferredBuilder builder = new DeferredBuilder<>(); ChangedResponse changedResponse = new ChangedResponse(left, right, context); - openApiDiff - .getMetadataDiff() - .diff(left.getDescription(), right.getDescription(), context) + builder + .with( + openApiDiff + .getMetadataDiff() + .diff(left.getDescription(), right.getDescription(), context)) .ifPresent(changedResponse::setDescription); - openApiDiff - .getContentDiff() - .diff(left.getContent(), right.getContent(), context) + builder + .with(openApiDiff.getContentDiff().diff(left.getContent(), right.getContent(), context)) .ifPresent(changedResponse::setContent); - openApiDiff - .getHeadersDiff() - .diff(left.getHeaders(), right.getHeaders(), context) + builder + .with(openApiDiff.getHeadersDiff().diff(left.getHeaders(), right.getHeaders(), context)) .ifPresent(changedResponse::setHeaders); - openApiDiff - .getExtensionsDiff() - .diff(left.getExtensions(), right.getExtensions(), context) + builder + .with( + openApiDiff + .getExtensionsDiff() + .diff(left.getExtensions(), right.getExtensions(), context)) .ifPresent(changedResponse::setExtensions); - return isChanged(changedResponse); + + return builder.buildIsChanged(changedResponse); } } diff --git a/core/src/main/java/org/openapitools/openapidiff/core/compare/SchemaDiff.java b/core/src/main/java/org/openapitools/openapidiff/core/compare/SchemaDiff.java index 0c25509c5..990029026 100644 --- a/core/src/main/java/org/openapitools/openapidiff/core/compare/SchemaDiff.java +++ b/core/src/main/java/org/openapitools/openapidiff/core/compare/SchemaDiff.java @@ -21,10 +21,13 @@ import org.openapitools.openapidiff.core.compare.schemadiffresult.SchemaDiffResult; import org.openapitools.openapidiff.core.model.ChangedSchema; import org.openapitools.openapidiff.core.model.DiffContext; +import org.openapitools.openapidiff.core.model.deferred.DeferredChanged; +import org.openapitools.openapidiff.core.model.deferred.RealizedChanged; +import org.openapitools.openapidiff.core.model.deferred.RecursiveSchemaSet; import org.openapitools.openapidiff.core.utils.RefPointer; import org.openapitools.openapidiff.core.utils.RefType; -public class SchemaDiff extends ReferenceDiffCache { +public class SchemaDiff { private static final RefPointer refPointer = new RefPointer<>(RefType.SCHEMAS); private static final Map, Class> @@ -274,17 +277,27 @@ private static String getSchemaRef(Schema schema) { return ofNullable(schema).map(Schema::get$ref).orElse(null); } - public Optional diff( + @Deprecated + public DeferredChanged diff( HashSet refSet, Schema left, Schema right, DiffContext context) { + return this.diff(new RecursiveSchemaSet(), left, right, context); + } + + public DeferredChanged diff(Schema left, Schema right, DiffContext context) { + return this.diff(new RecursiveSchemaSet(), left, right, context); + } + + public DeferredChanged diff( + RecursiveSchemaSet refSet, Schema left, Schema right, DiffContext context) { if (left == null && right == null) { - return Optional.empty(); + return new RealizedChanged<>(Optional.empty()); } - return cachedDiff(refSet, left, right, getSchemaRef(left), getSchemaRef(right), context); + return computeDeferredDiff(refSet, left, right, context); } - public Optional getTypeChangedSchema( + public DeferredChanged getTypeChangedSchema( Schema left, Schema right, DiffContext context) { - return Optional.of( + return new RealizedChanged( SchemaDiff.getSchemaDiffResult(openApiDiff) .getChangedSchema() .setOldSchema(left) @@ -293,9 +306,20 @@ public Optional getTypeChangedSchema( .setContext(context)); } - @Override - protected Optional computeDiff( - HashSet refSet, Schema left, Schema right, DiffContext context) { + protected DeferredChanged computeDeferredDiff( + RecursiveSchemaSet refSet, Schema left, Schema right, DiffContext context) { + + CacheKey key = new CacheKey(getSchemaRef(left), getSchemaRef(right), context); + if (key.getLeft() != null && key.getRight() != null) { + return openApiDiff.getDeferredSchemaCache().getOrAddSchema(refSet, key, left, right); + } else { + return computeDiffForReal(refSet, left, right, context); + } + } + + public DeferredChanged computeDiffForReal( + RecursiveSchemaSet refSet, Schema left, Schema right, DiffContext context) { + left = refPointer.resolveRef(this.leftComponents, left, getSchemaRef(left)); right = refPointer.resolveRef(this.rightComponents, right, getSchemaRef(right)); 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 6c058ab07..4e41f077a 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 @@ -1,13 +1,14 @@ package org.openapitools.openapidiff.core.compare; -import static org.openapitools.openapidiff.core.utils.ChangedUtils.isChanged; - 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.*; +import org.openapitools.openapidiff.core.model.Changed; import org.openapitools.openapidiff.core.model.ChangedSecurityRequirement; import org.openapitools.openapidiff.core.model.DiffContext; +import org.openapitools.openapidiff.core.model.deferred.DeferredBuilder; +import org.openapitools.openapidiff.core.model.deferred.DeferredChanged; /** Created by adarsh.sharma on 07/01/18. */ public class SecurityRequirementDiff { @@ -61,8 +62,10 @@ private LinkedHashMap> contains( return found; } - public Optional diff( + public DeferredChanged diff( SecurityRequirement left, SecurityRequirement right, DiffContext context) { + DeferredBuilder builder = new DeferredBuilder(); + ChangedSecurityRequirement changedSecurityRequirement = new ChangedSecurityRequirement(left, right != null ? getCopy(right) : null); @@ -76,22 +79,23 @@ public Optional diff( } else { Optional rightSchemeRef = rightSec.keySet().stream().findFirst(); rightSchemeRef.ifPresent(rightRequirement::remove); - rightSchemeRef - .flatMap( - rightScheme -> - openApiDiff - .getSecuritySchemeDiff() - .diff( - leftEntry.getKey(), - leftEntry.getValue(), - rightScheme, - rightSec.get(rightScheme), - context)) - .ifPresent(changedSecurityRequirement::addChanged); + rightSchemeRef.ifPresent( + rightScheme -> + builder + .with( + openApiDiff + .getSecuritySchemeDiff() + .diff( + leftEntry.getKey(), + leftEntry.getValue(), + rightScheme, + rightSec.get(rightScheme), + context)) + .ifPresent(changedSecurityRequirement::addChanged)); } } rightRequirement.forEach(changedSecurityRequirement::addIncreased); - return isChanged(changedSecurityRequirement); + return builder.buildIsChanged(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 30b99a542..6ba68fdfa 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 @@ -1,7 +1,5 @@ package org.openapitools.openapidiff.core.compare; -import static org.openapitools.openapidiff.core.utils.ChangedUtils.isChanged; - import io.swagger.v3.oas.models.Components; import io.swagger.v3.oas.models.security.SecurityRequirement; import io.swagger.v3.oas.models.security.SecurityScheme; @@ -13,9 +11,11 @@ import org.apache.commons.collections4.CollectionUtils; import org.apache.commons.lang3.tuple.ImmutablePair; import org.apache.commons.lang3.tuple.Pair; -import org.openapitools.openapidiff.core.model.ChangedSecurityRequirement; +import org.openapitools.openapidiff.core.model.Changed; import org.openapitools.openapidiff.core.model.ChangedSecurityRequirements; import org.openapitools.openapidiff.core.model.DiffContext; +import org.openapitools.openapidiff.core.model.deferred.DeferredBuilder; +import org.openapitools.openapidiff.core.model.deferred.DeferredChanged; /** Created by adarsh.sharma on 07/01/18. */ public class SecurityRequirementsDiff { @@ -77,8 +77,11 @@ private Pair getPair(SecurityScheme secu return new ImmutablePair<>(securityScheme.getType(), securityScheme.getIn()); } - protected Optional diff( + protected DeferredChanged diff( List left, List right, DiffContext context) { + + DeferredBuilder builder = new DeferredBuilder(); + left = left == null ? new ArrayList<>() : left; right = right == null ? new ArrayList<>() : getCopy(right); @@ -92,14 +95,14 @@ protected Optional diff( } else { SecurityRequirement rightSec = rightSecOpt.get(); right.remove(rightSec); - Optional diff = - openApiDiff.getSecurityRequirementDiff().diff(leftSecurity, rightSec, context); - diff.ifPresent(changedSecurityRequirements::addChanged); + builder + .with(openApiDiff.getSecurityRequirementDiff().diff(leftSecurity, rightSec, context)) + .ifPresent(changedSecurityRequirements::addChanged); } } right.forEach(changedSecurityRequirements::addIncreased); - return isChanged(changedSecurityRequirements); + return builder.buildIsChanged(changedSecurityRequirements); } private List getCopy(List right) { 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 cf11b0d12..ad011f1fb 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 @@ -7,10 +7,11 @@ import java.util.HashSet; import java.util.List; import java.util.Objects; -import java.util.Optional; import org.openapitools.openapidiff.core.model.ChangedSecurityScheme; import org.openapitools.openapidiff.core.model.ChangedSecuritySchemeScopes; import org.openapitools.openapidiff.core.model.DiffContext; +import org.openapitools.openapidiff.core.model.deferred.DeferredChanged; +import org.openapitools.openapidiff.core.model.deferred.RealizedChanged; /** Created by adarsh.sharma on 11/01/18. */ public class SecuritySchemeDiff extends ReferenceDiffCache { @@ -30,7 +31,7 @@ public SecuritySchemeDiff(OpenApiDiff openApiDiff) { : null; } - public Optional diff( + public DeferredChanged diff( String leftSchemeRef, List leftScopes, String rightSchemeRef, @@ -38,7 +39,7 @@ public Optional diff( DiffContext context) { SecurityScheme leftSecurityScheme = leftComponents.getSecuritySchemes().get(leftSchemeRef); SecurityScheme rightSecurityScheme = rightComponents.getSecuritySchemes().get(rightSchemeRef); - Optional changedSecuritySchemeOpt = + DeferredChanged changedSecuritySchemeOpt = cachedDiff( new HashSet<>(), leftSecurityScheme, @@ -46,22 +47,26 @@ public Optional diff( leftSchemeRef, rightSchemeRef, context); - ChangedSecurityScheme changedSecurityScheme = - changedSecuritySchemeOpt.orElse( - new ChangedSecurityScheme(leftSecurityScheme, rightSecurityScheme)); - changedSecurityScheme = getCopyWithoutScopes(changedSecurityScheme); - - if (changedSecurityScheme != null - && leftSecurityScheme.getType() == SecurityScheme.Type.OAUTH2) { - isChanged(ListDiff.diff(new ChangedSecuritySchemeScopes(leftScopes, rightScopes))) - .ifPresent(changedSecurityScheme::setChangedScopes); - } - return isChanged(changedSecurityScheme); + return changedSecuritySchemeOpt.map( + (changedSecuritySchemeOptional) -> { + ChangedSecurityScheme changedSecurityScheme = + changedSecuritySchemeOptional.orElse( + new ChangedSecurityScheme(leftSecurityScheme, rightSecurityScheme)); + changedSecurityScheme = getCopyWithoutScopes(changedSecurityScheme); + + if (changedSecurityScheme != null + && leftSecurityScheme.getType() == SecurityScheme.Type.OAUTH2) { + isChanged(ListDiff.diff(new ChangedSecuritySchemeScopes(leftScopes, rightScopes))) + .ifPresent(changedSecurityScheme::setChangedScopes); + } + + return changedSecurityScheme; + }); } @Override - protected Optional computeDiff( + protected DeferredChanged computeDiff( HashSet refSet, SecurityScheme leftSecurityScheme, SecurityScheme rightSecurityScheme, @@ -107,7 +112,7 @@ protected Optional computeDiff( .diff(leftSecurityScheme.getExtensions(), rightSecurityScheme.getExtensions(), context) .ifPresent(changedSecurityScheme::setExtensions); - return Optional.of(changedSecurityScheme); + return new RealizedChanged(changedSecurityScheme); } private ChangedSecurityScheme getCopyWithoutScopes(ChangedSecurityScheme original) { diff --git a/core/src/main/java/org/openapitools/openapidiff/core/compare/schemadiffresult/ArraySchemaDiffResult.java b/core/src/main/java/org/openapitools/openapidiff/core/compare/schemadiffresult/ArraySchemaDiffResult.java index cefa18e3f..8c6357ab7 100644 --- a/core/src/main/java/org/openapitools/openapidiff/core/compare/schemadiffresult/ArraySchemaDiffResult.java +++ b/core/src/main/java/org/openapitools/openapidiff/core/compare/schemadiffresult/ArraySchemaDiffResult.java @@ -3,11 +3,11 @@ import io.swagger.v3.oas.models.Components; import io.swagger.v3.oas.models.media.ArraySchema; import io.swagger.v3.oas.models.media.Schema; -import java.util.HashSet; -import java.util.Optional; import org.openapitools.openapidiff.core.compare.OpenApiDiff; import org.openapitools.openapidiff.core.model.ChangedSchema; import org.openapitools.openapidiff.core.model.DiffContext; +import org.openapitools.openapidiff.core.model.deferred.DeferredChanged; +import org.openapitools.openapidiff.core.model.deferred.RecursiveSchemaSet; /** Created by adarsh.sharma on 18/12/17. */ public class ArraySchemaDiffResult extends SchemaDiffResult { @@ -16,8 +16,8 @@ public ArraySchemaDiffResult(OpenApiDiff openApiDiff) { } @Override - public , X> Optional diff( - HashSet refSet, + public , X> DeferredChanged diff( + RecursiveSchemaSet refSet, Components leftComponents, Components rightComponents, T left, @@ -25,15 +25,23 @@ public , X> Optional diff( DiffContext context) { ArraySchema leftArraySchema = (ArraySchema) left; ArraySchema rightArraySchema = (ArraySchema) right; - super.diff(refSet, leftComponents, rightComponents, left, right, context); - openApiDiff - .getSchemaDiff() - .diff( - refSet, - leftArraySchema.getItems(), - rightArraySchema.getItems(), - context.copyWithRequired(true)) - .ifPresent(changedSchema::setItems); - return isApplicable(context); + + DeferredChanged superSchemaDiff = + super.diff(refSet, leftComponents, rightComponents, left, right, context) + .flatMap( + (changeSchemaOptional) -> { + DeferredChanged itemsDiff = + openApiDiff + .getSchemaDiff() + .diff( + refSet, + leftArraySchema.getItems(), + rightArraySchema.getItems(), + context.copyWithRequired(true)); + itemsDiff.ifPresent(changedSchema::setItems); + return itemsDiff; + }); + + return superSchemaDiff.mapOptional(schemaOptional -> isApplicable(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 ea681db13..d55e0271d 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 @@ -4,7 +4,6 @@ import io.swagger.v3.oas.models.media.ComposedSchema; import io.swagger.v3.oas.models.media.Discriminator; import io.swagger.v3.oas.models.media.Schema; -import java.util.HashSet; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; @@ -16,6 +15,10 @@ import org.openapitools.openapidiff.core.model.ChangedOneOfSchema; import org.openapitools.openapidiff.core.model.ChangedSchema; import org.openapitools.openapidiff.core.model.DiffContext; +import org.openapitools.openapidiff.core.model.deferred.DeferredBuilder; +import org.openapitools.openapidiff.core.model.deferred.DeferredChanged; +import org.openapitools.openapidiff.core.model.deferred.RealizedChanged; +import org.openapitools.openapidiff.core.model.deferred.RecursiveSchemaSet; import org.openapitools.openapidiff.core.utils.RefPointer; import org.openapitools.openapidiff.core.utils.RefType; @@ -28,16 +31,19 @@ public ComposedSchemaDiffResult(OpenApiDiff openApiDiff) { } @Override - public , X> Optional diff( - HashSet refSet, + public , X> DeferredChanged diff( + RecursiveSchemaSet refSet, Components leftComponents, Components rightComponents, T left, T right, DiffContext context) { + if (left instanceof ComposedSchema) { ComposedSchema leftComposedSchema = (ComposedSchema) left; ComposedSchema rightComposedSchema = (ComposedSchema) right; + DeferredBuilder discriminatorChangedBuilder = new DeferredBuilder<>(); + if (CollectionUtils.isNotEmpty(leftComposedSchema.getOneOf()) || CollectionUtils.isNotEmpty(rightComposedSchema.getOneOf())) { @@ -56,7 +62,7 @@ public , X> Optional diff( changedSchema.setNewSchema(right); changedSchema.setDiscriminatorPropertyChanged(true); changedSchema.setContext(context); - return Optional.of(changedSchema); + return new RealizedChanged<>(Optional.of(changedSchema)); } Map leftMapping = getMapping(leftComposedSchema); @@ -72,25 +78,36 @@ public , X> Optional diff( leftSchema.set$ref(leftMapping.get(key)); Schema rightSchema = new Schema(); rightSchema.set$ref(rightMapping.get(key)); - Optional changedSchema = - openApiDiff - .getSchemaDiff() - .diff(refSet, leftSchema, rightSchema, context.copyWithRequired(true)); - changedSchema.ifPresent(schema -> changedMapping.put(key, schema)); + discriminatorChangedBuilder + .with( + openApiDiff + .getSchemaDiff() + .diff(refSet, leftSchema, rightSchema, context.copyWithRequired(true))) + .ifPresent(schema -> changedMapping.put(key, schema)); } - changedSchema.setOneOfSchema( - new ChangedOneOfSchema(leftMapping, rightMapping, context) - .setIncreased(mappingDiff.getIncreased()) - .setMissing(mappingDiff.getMissing()) - .setChanged(changedMapping)); + + discriminatorChangedBuilder.whenSet( + (composedSchemas) -> { + changedSchema.setOneOfSchema( + new ChangedOneOfSchema(leftMapping, rightMapping, context) + .setIncreased(mappingDiff.getIncreased()) + .setMissing(mappingDiff.getMissing()) + .setChanged(changedMapping)); + }); } - return super.diff(refSet, leftComponents, rightComponents, left, right, context); + + return discriminatorChangedBuilder + .build() + .flatMap( + (values) -> + super.diff(refSet, leftComponents, rightComponents, left, right, context)); } else { return openApiDiff.getSchemaDiff().getTypeChangedSchema(left, right, context); } } - 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 +123,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/compare/schemadiffresult/SchemaDiffResult.java b/core/src/main/java/org/openapitools/openapidiff/core/compare/schemadiffresult/SchemaDiffResult.java index 24f178c65..d2be3725e 100644 --- a/core/src/main/java/org/openapitools/openapidiff/core/compare/schemadiffresult/SchemaDiffResult.java +++ b/core/src/main/java/org/openapitools/openapidiff/core/compare/schemadiffresult/SchemaDiffResult.java @@ -10,8 +10,12 @@ import org.openapitools.openapidiff.core.compare.MapKeyDiff; import org.openapitools.openapidiff.core.compare.OpenApiDiff; import org.openapitools.openapidiff.core.model.Change; +import org.openapitools.openapidiff.core.model.Changed; import org.openapitools.openapidiff.core.model.ChangedSchema; import org.openapitools.openapidiff.core.model.DiffContext; +import org.openapitools.openapidiff.core.model.deferred.DeferredBuilder; +import org.openapitools.openapidiff.core.model.deferred.DeferredChanged; +import org.openapitools.openapidiff.core.model.deferred.RecursiveSchemaSet; import org.openapitools.openapidiff.core.model.schema.*; public class SchemaDiffResult { @@ -28,13 +32,16 @@ public SchemaDiffResult(String type, OpenApiDiff openApiDiff) { this.changedSchema.setType(type); } - public , X> Optional diff( - HashSet refSet, + public , X> DeferredChanged diff( + RecursiveSchemaSet refSet, Components leftComponents, Components rightComponents, V left, V right, DiffContext context) { + + DeferredBuilder builder = new DeferredBuilder<>(); + ChangedEnum changedEnum = ListDiff.diff(new ChangedEnum<>(left.getEnum(), right.getEnum(), context)); changedSchema @@ -53,36 +60,42 @@ public , X> Optional diff( .setReadOnly(new ChangedReadOnly(left.getReadOnly(), right.getReadOnly(), context)) .setWriteOnly(new ChangedWriteOnly(left.getWriteOnly(), right.getWriteOnly(), context)) .setMaxLength(new ChangedMaxLength(left.getMaxLength(), right.getMaxLength(), context)); - openApiDiff - .getExtensionsDiff() - .diff(left.getExtensions(), right.getExtensions(), context) + builder + .with( + openApiDiff + .getExtensionsDiff() + .diff(left.getExtensions(), right.getExtensions(), context)) .ifPresent(changedSchema::setExtensions); - openApiDiff - .getMetadataDiff() - .diff(left.getDescription(), right.getDescription(), context) + builder + .with( + openApiDiff + .getMetadataDiff() + .diff(left.getDescription(), right.getDescription(), context)) .ifPresent(changedSchema::setDescription); Map leftProperties = left.getProperties(); Map rightProperties = right.getProperties(); MapKeyDiff propertyDiff = MapKeyDiff.diff(leftProperties, rightProperties); for (String key : propertyDiff.getSharedKey()) { - openApiDiff - .getSchemaDiff() - .diff( - refSet, - leftProperties.get(key), - rightProperties.get(key), - required(context, key, right.getRequired())) + builder + .with( + openApiDiff + .getSchemaDiff() + .diff( + refSet, + leftProperties.get(key), + rightProperties.get(key), + required(context, key, right.getRequired()))) .ifPresent( changedSchema1 -> changedSchema.getChangedProperties().put(key, changedSchema1)); } - compareAdditionalProperties(refSet, left, right, context); + compareAdditionalProperties(refSet, left, right, context, builder); changedSchema .getIncreasedProperties() .putAll(filterProperties(Change.Type.ADDED, propertyDiff.getIncreased(), context)); changedSchema .getMissingProperties() .putAll(filterProperties(Change.Type.REMOVED, propertyDiff.getMissing(), context)); - return isApplicable(context); + return builder.build().mapOptional((values) -> isApplicable(context)); } protected Optional isApplicable(DiffContext context) { @@ -125,7 +138,11 @@ private DiffContext required(DiffContext context, String key, List requi } private void compareAdditionalProperties( - HashSet refSet, Schema leftSchema, Schema rightSchema, DiffContext context) { + RecursiveSchemaSet refSet, + Schema leftSchema, + Schema rightSchema, + DiffContext context, + DeferredBuilder builder) { Object left = leftSchema.getAdditionalProperties(); Object right = rightSchema.getAdditionalProperties(); if (left instanceof Schema || right instanceof Schema) { @@ -137,7 +154,7 @@ private void compareAdditionalProperties( .setOldSchema(leftAdditionalSchema) .setNewSchema(rightAdditionalSchema); if (left != null && right != null) { - Optional addPropChangedSchemaOP = + DeferredChanged addPropChangedSchemaOP = openApiDiff .getSchemaDiff() .diff( @@ -145,9 +162,16 @@ private void compareAdditionalProperties( leftAdditionalSchema, rightAdditionalSchema, context.copyWithRequired(false)); - apChangedSchema = addPropChangedSchemaOP.orElse(apChangedSchema); + builder + .with(addPropChangedSchemaOP) + .whenSet( + (optional) -> { + ChangedSchema apc = optional.orElse(apChangedSchema); + isChanged(apc).ifPresent(changedSchema::setAddProp); + }); + } else { + isChanged(apChangedSchema).ifPresent(changedSchema::setAddProp); } - isChanged(apChangedSchema).ifPresent(changedSchema::setAddProp); } } diff --git a/core/src/main/java/org/openapitools/openapidiff/core/model/Change.java b/core/src/main/java/org/openapitools/openapidiff/core/model/Change.java index 604aaffe9..a688336e6 100644 --- a/core/src/main/java/org/openapitools/openapidiff/core/model/Change.java +++ b/core/src/main/java/org/openapitools/openapidiff/core/model/Change.java @@ -22,7 +22,7 @@ public static Change removed(T oldValue) { public enum Type { ADDED, CHANGED, - REMOVED; + REMOVED } public Change(final T oldValue, final T newValue, final Type type) { diff --git a/core/src/main/java/org/openapitools/openapidiff/core/model/ChangedOpenApi.java b/core/src/main/java/org/openapitools/openapidiff/core/model/ChangedOpenApi.java index 99c6a387d..dca98a5f9 100644 --- a/core/src/main/java/org/openapitools/openapidiff/core/model/ChangedOpenApi.java +++ b/core/src/main/java/org/openapitools/openapidiff/core/model/ChangedOpenApi.java @@ -13,6 +13,7 @@ public class ChangedOpenApi implements ComposedChanged { private List newEndpoints; private List missingEndpoints; private List changedOperations; + private List changedSchemas; private ChangedExtensions changedExtensions; public List getDeprecatedEndpoints() { @@ -27,7 +28,9 @@ public List getDeprecatedEndpoints() { @Override public List getChangedElements() { - return Stream.concat(changedOperations.stream(), Stream.of(changedExtensions)) + return Stream.concat( + Stream.concat(changedOperations.stream(), Stream.of(changedExtensions)), + changedSchemas.stream()) .collect(Collectors.toList()); } @@ -68,6 +71,10 @@ public ChangedExtensions getChangedExtensions() { return this.changedExtensions; } + public List getChangedSchemas() { + return changedSchemas; + } + public ChangedOpenApi setOldSpecOpenApi(final OpenAPI oldSpecOpenApi) { this.oldSpecOpenApi = oldSpecOpenApi; return this; @@ -98,6 +105,11 @@ public ChangedOpenApi setChangedExtensions(final ChangedExtensions changedExtens return this; } + public ChangedOpenApi setChangedSchemas(final List changedSchemas) { + this.changedSchemas = changedSchemas; + return this; + } + @Override public boolean equals(Object o) { if (this == o) return true; diff --git a/core/src/main/java/org/openapitools/openapidiff/core/model/ChangedSchema.java b/core/src/main/java/org/openapitools/openapidiff/core/model/ChangedSchema.java index 0dfd49ae5..c353e84a9 100644 --- a/core/src/main/java/org/openapitools/openapidiff/core/model/ChangedSchema.java +++ b/core/src/main/java/org/openapitools/openapidiff/core/model/ChangedSchema.java @@ -2,10 +2,7 @@ import io.swagger.v3.oas.models.PathItem; import io.swagger.v3.oas.models.media.Schema; -import java.util.LinkedHashMap; -import java.util.List; -import java.util.Map; -import java.util.Objects; +import java.util.*; import java.util.stream.Collectors; import java.util.stream.Stream; import org.openapitools.openapidiff.core.model.schema.ChangedEnum; @@ -39,32 +36,98 @@ public class ChangedSchema implements ComposedChanged { protected ChangedSchema addProp; private ChangedExtensions extensions; + // Flags to avoid recursive calls to isChanged() and getChangedElements() + private boolean gettingChangedElements = false; + private boolean gettingIsChanged = false; + + // cached results for isChanged() + private DiffResult changed; + private DiffResult coreChanged; + + // cached results for getChangedElements() + private List changedElements; + public ChangedSchema() { increasedProperties = new LinkedHashMap<>(); missingProperties = new LinkedHashMap<>(); changedProperties = new LinkedHashMap<>(); } + private void clearChangedCache() { + this.changed = null; + this.coreChanged = null; + this.changedElements = null; + } + + @Override + public DiffResult isChanged() { + if (gettingIsChanged) { + return DiffResult.NO_CHANGES; + } + + gettingIsChanged = true; + if (this.changed == null) { + DiffResult elementsResult = + DiffResult.fromWeight( + getChangedElements().stream() + .filter(Objects::nonNull) + .map(Changed::isChanged) + .mapToInt(DiffResult::getWeight) + .max() + .orElse(0)); + DiffResult result; + if (isCoreChanged().getWeight() > elementsResult.getWeight()) { + result = isCoreChanged(); + } else { + result = elementsResult; + } + this.changed = result; + } + gettingIsChanged = false; + + return this.changed; + } + @Override public List getChangedElements() { - return Stream.concat( - changedProperties.values().stream(), - Stream.of( - description, - readOnly, - writeOnly, - items, - oneOfSchema, - addProp, - enumeration, - required, - maxLength, - extensions)) - .collect(Collectors.toList()); + if (gettingChangedElements) { + return Collections.emptyList(); + } + + gettingChangedElements = true; + if (changedElements == null) { + List changed = + Stream.concat( + changedProperties.values().stream(), + Stream.of( + description, + readOnly, + writeOnly, + items, + oneOfSchema, + addProp, + enumeration, + required, + maxLength, + extensions)) + .collect(Collectors.toList()); + this.changedElements = changed; + } + gettingChangedElements = false; + + return this.changedElements; } @Override public DiffResult isCoreChanged() { + if (this.coreChanged == null) { + this.coreChanged = calculateCoreChanged(); + } + + return this.coreChanged; + } + + private DiffResult calculateCoreChanged() { if (!changedType && (oldSchema == null && newSchema == null || oldSchema != null && newSchema != null) && !changeFormat @@ -208,96 +271,115 @@ public ChangedSchema setType(final String type) { } public ChangedSchema setChangedProperties(final Map changedProperties) { + clearChangedCache(); this.changedProperties = changedProperties; return this; } public ChangedSchema setIncreasedProperties(final Map increasedProperties) { + clearChangedCache(); this.increasedProperties = increasedProperties; return this; } public ChangedSchema setMissingProperties(final Map missingProperties) { + clearChangedCache(); this.missingProperties = missingProperties; return this; } public ChangedSchema setChangeDeprecated(final boolean changeDeprecated) { + clearChangedCache(); this.changeDeprecated = changeDeprecated; return this; } public ChangedSchema setDescription(final ChangedMetadata description) { + clearChangedCache(); this.description = description; return this; } public ChangedSchema setChangeTitle(final boolean changeTitle) { + clearChangedCache(); this.changeTitle = changeTitle; return this; } public ChangedSchema setRequired(final ChangedRequired required) { + clearChangedCache(); this.required = required; return this; } public ChangedSchema setChangeDefault(final boolean changeDefault) { + clearChangedCache(); this.changeDefault = changeDefault; return this; } public ChangedSchema setEnumeration(final ChangedEnum enumeration) { + clearChangedCache(); this.enumeration = enumeration; return this; } public ChangedSchema setChangeFormat(final boolean changeFormat) { + clearChangedCache(); this.changeFormat = changeFormat; return this; } public ChangedSchema setReadOnly(final ChangedReadOnly readOnly) { + clearChangedCache(); this.readOnly = readOnly; return this; } public ChangedSchema setWriteOnly(final ChangedWriteOnly writeOnly) { + clearChangedCache(); this.writeOnly = writeOnly; return this; } public ChangedSchema setChangedType(final boolean changedType) { + clearChangedCache(); this.changedType = changedType; return this; } public ChangedSchema setMaxLength(final ChangedMaxLength maxLength) { + clearChangedCache(); this.maxLength = maxLength; return this; } public ChangedSchema setDiscriminatorPropertyChanged(final boolean discriminatorPropertyChanged) { + clearChangedCache(); this.discriminatorPropertyChanged = discriminatorPropertyChanged; return this; } public ChangedSchema setItems(final ChangedSchema items) { + clearChangedCache(); this.items = items; return this; } public ChangedSchema setOneOfSchema(final ChangedOneOfSchema oneOfSchema) { + clearChangedCache(); this.oneOfSchema = oneOfSchema; return this; } public ChangedSchema setAddProp(final ChangedSchema addProp) { + clearChangedCache(); this.addProp = addProp; return this; } public ChangedSchema setExtensions(final ChangedExtensions extensions) { + clearChangedCache(); this.extensions = extensions; return this; } diff --git a/core/src/main/java/org/openapitools/openapidiff/core/model/deferred/DeferredBuilder.java b/core/src/main/java/org/openapitools/openapidiff/core/model/deferred/DeferredBuilder.java new file mode 100644 index 000000000..959e7625e --- /dev/null +++ b/core/src/main/java/org/openapitools/openapidiff/core/model/deferred/DeferredBuilder.java @@ -0,0 +1,106 @@ +package org.openapitools.openapidiff.core.model.deferred; + +import static org.openapitools.openapidiff.core.utils.ChangedUtils.isChanged; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; +import java.util.Optional; +import java.util.function.Consumer; +import java.util.stream.Collectors; +import java.util.stream.IntStream; +import java.util.stream.Stream; +import org.openapitools.openapidiff.core.model.Changed; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +public class DeferredBuilder implements Consumer> { + private static final Logger log = LoggerFactory.getLogger(DeferredBuilder.class); + + private final List> deferredValues = new ArrayList<>(); + private final List>>>> whenSet = new ArrayList<>(); + + public Optional with(Optional value) { + return value; + } + + public DeferredChanged with(DeferredChanged value) { + deferredValues.add(value); + return value; + } + + public DeferredBuilder add(DeferredChanged value) { + deferredValues.add(value); + return this; + } + + public DeferredBuilder addAll(List> values) { + deferredValues.addAll(values); + return this; + } + + public DeferredBuilder addAll(Stream> values) { + deferredValues.addAll(values.collect(Collectors.toList())); + return this; + } + + public DeferredBuilder whenSet(Consumer>>> consumer) { + whenSet.add(consumer); + return this; + } + + @Override + public void accept(DeferredChanged value) { + deferredValues.add(value); + } + + public DeferredChanged>> build() { + if (deferredValues.isEmpty()) { + return DeferredChanged.empty(); + } + + log.debug("Building collected deferred {}", DeferredLogger.logValue(deferredValues)); + + final PendingChanged>> changed = new PendingChanged(); + whenSet.forEach(changed::whenSet); + + Optional[] values = new Optional[deferredValues.size()]; + + IntStream.range(0, deferredValues.size()) + .forEach( + (i) -> { + DeferredChanged deferredItem = deferredValues.get(i); + deferredItem.whenSet( + (value) -> { + values[i] = value; + log.debug( + "Collected deferred item set this={}, item={}, values = {}", + this, + DeferredLogger.logValue(value), + DeferredLogger.logValue(values)); + if (isFull(values)) { + log.debug( + "Collected deferred triggering complete this={}, values = {}", + this, + DeferredLogger.logValue(values)); + changed.setValue(Optional.of(Arrays.asList(values))); + } + }); + }); + + return changed; + } + + public DeferredChanged buildIsChanged(V changed) { + return build().flatMap((values) -> (DeferredChanged) DeferredChanged.of(isChanged(changed))); + } + + private static boolean isFull(Object[] values) { + for (int i = 0; i < values.length; i++) { + if (values[i] == null) { + return false; + } + } + return true; + } +} diff --git a/core/src/main/java/org/openapitools/openapidiff/core/model/deferred/DeferredChanged.java b/core/src/main/java/org/openapitools/openapidiff/core/model/deferred/DeferredChanged.java new file mode 100644 index 000000000..c2aecf979 --- /dev/null +++ b/core/src/main/java/org/openapitools/openapidiff/core/model/deferred/DeferredChanged.java @@ -0,0 +1,38 @@ +package org.openapitools.openapidiff.core.model.deferred; + +import java.util.Optional; +import java.util.function.Consumer; +import java.util.function.Function; +import javax.annotation.Nullable; +import org.openapitools.openapidiff.core.model.ChangedResponse; + +public interface DeferredChanged { + + void ifPresent(Consumer consumer); + + void whenSet(Consumer> consumer); + + DeferredChanged map(Function, Q> consumer); + + DeferredChanged mapOptional(Function, Optional> consumer); + + DeferredChanged flatMap(Function, DeferredChanged> consumer); + + static DeferredChanged empty() { + return RealizedChanged.empty(); + } + + static DeferredChanged ofNullable(@Nullable T value) { + return new RealizedChanged(value); + } + + static DeferredChanged of(Optional changed) { + return new RealizedChanged(changed); + } + + boolean isPresent(); + + boolean isValueSet(); + + T get(); +} diff --git a/core/src/main/java/org/openapitools/openapidiff/core/model/deferred/DeferredLogger.java b/core/src/main/java/org/openapitools/openapidiff/core/model/deferred/DeferredLogger.java new file mode 100644 index 000000000..90020188e --- /dev/null +++ b/core/src/main/java/org/openapitools/openapidiff/core/model/deferred/DeferredLogger.java @@ -0,0 +1,71 @@ +package org.openapitools.openapidiff.core.model.deferred; + +import java.util.Arrays; +import java.util.Collection; +import java.util.Optional; +import java.util.stream.Collectors; +import java.util.stream.Stream; +import org.openapitools.openapidiff.core.model.Changed; +import org.openapitools.openapidiff.core.model.ComposedChanged; + +public class DeferredLogger { + + public static Object logValue(Object value) { + return new Object() { + public String toString() { + return valueToString(value); + } + }; + } + + public static String optionalToString(Optional value) { + if (value == null) { + return "null"; + } else { + return value.map((v) -> "Optional[" + valueToString(v) + "]").orElse("Optional[empty]"); + } + } + + public static String changedToString(Changed value) { + if (value instanceof ComposedChanged) { + return "Changed: " + value.getClass() + " (composed) "; + } else { + return "Changed: " + value.getClass() + " " + value.isChanged(); + } + } + + public static String streamToString(Stream values) { + return "[" + values.map(v -> valueToString(v)).collect(Collectors.joining(", ")) + "]"; + } + + public static String deferredChangeToString(DeferredChanged deferredChanged) { + if (deferredChanged.isValueSet()) { + if (deferredChanged.isPresent()) { + Object value = deferredChanged.get(); + return valueToString(value); + } else { + return deferredChanged.toString(); + } + } else { + return deferredChanged.toString(); + } + } + + public static String valueToString(Object value) { + if (value == null) { + return "null"; + } else if (value instanceof Changed) { + return changedToString((Changed) value); + } else if (value instanceof Optional) { + return optionalToString((Optional) value); + } else if (value instanceof DeferredChanged) { + return deferredChangeToString((DeferredChanged) value); + } else if (value.getClass().isArray()) { + return streamToString(Arrays.stream((Object[]) value)); + } else if (value instanceof Collection) { + return streamToString(((Collection) value).stream()); + } else { + return value.toString(); + } + } +} diff --git a/core/src/main/java/org/openapitools/openapidiff/core/model/deferred/DeferredSchemaCache.java b/core/src/main/java/org/openapitools/openapidiff/core/model/deferred/DeferredSchemaCache.java new file mode 100644 index 000000000..2033e0cf5 --- /dev/null +++ b/core/src/main/java/org/openapitools/openapidiff/core/model/deferred/DeferredSchemaCache.java @@ -0,0 +1,122 @@ +package org.openapitools.openapidiff.core.model.deferred; + +import io.swagger.v3.oas.models.media.Schema; +import java.util.*; +import java.util.function.Supplier; +import java.util.stream.Collectors; +import org.openapitools.openapidiff.core.compare.CacheKey; +import org.openapitools.openapidiff.core.compare.OpenApiDiff; +import org.openapitools.openapidiff.core.model.Changed; +import org.openapitools.openapidiff.core.model.ChangedSchema; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** Holds schema created by jonathan.hess@gmail.com */ +public class DeferredSchemaCache { + private static final Logger log = LoggerFactory.getLogger(DeferredSchemaCache.class); + + private Map cache = new LinkedHashMap<>(); + private Queue processingQueue = new ArrayDeque<>(); + // private Queue deferredOperations = new ArrayDeque<>(); + + private final OpenApiDiff openApiDiff; + + public DeferredSchemaCache(OpenApiDiff openApiDiff) { + this.openApiDiff = openApiDiff; + } + + public SchemaDiffOperation addSchema( + RecursiveSchemaSet refSet, CacheKey key, Schema left, Schema right) { + if (!cache.containsKey(key)) { + log.debug("Added schema cache {}", key); + SchemaDiffOperation operation = + new SchemaDiffOperation(openApiDiff, refSet, key, left, right); + cache.put(key, operation); + processingQueue.add(key); + return operation; + } else { + return cache.get(key); + } + } + + public DeferredChanged getOrAddSchema( + RecursiveSchemaSet refSet, CacheKey key, Schema left, Schema right) { + // don't allow recursive references to schemas + if (refSet.contains(key)) { + log.debug("getOrAddSchema recursive call aborted {} ", key); + return DeferredChanged.empty(); + } + + refSet.put(key); + SchemaDiffOperation operation; + if (cache.containsKey(key)) { + operation = cache.get(key); + log.debug("getOrAddSchema cached {} {}", key, operation.diffResult); + } else { + operation = addSchema(refSet, key, left, right); + log.debug("getOrAddSchema added {} {}", key, operation.diffResult); + } + return operation.diffResult; + } + + public void process() { + processSchemaQueue(); + // while(! deferredOperations.isEmpty()) { + // processSchemaQueue(); + // DeferredOperation op = deferredOperations.poll(); + // if(op != null) { + // log.debug("Processing deferred {}", op); + // op.process(); + // } + // } + } + + public void processSchemaQueue() { + PendingChanged.logResolved(); + while (!processingQueue.isEmpty()) { + CacheKey key = processingQueue.poll(); + if (key != null) { + log.debug("Processing schema {}", key); + SchemaDiffOperation operation = cache.get(key); + DeferredChanged realValue = + operation + .openApiDiff + .getSchemaDiff() + .computeDiffForReal( + operation.refSet, operation.left, operation.right, key.getContext()); + operation.processed = true; + realValue.whenSet( + (value) -> { + log.debug("Schema processed {} {}", key, DeferredLogger.logValue(value)); + operation.diffResult.setValue(value); + }); + log.debug("Processing schema started {}", key); + } + PendingChanged.logResolved(); + } + } + + public Collection getOperations() { + return cache.values(); + } + + public List getChangedSchemas() { + return cache.values().stream() + .filter(op -> op.processed && op.diffResult.isPresent()) + .map(op -> op.diffResult.get()) + .collect(Collectors.toList()); + } + + private static class DeferredOperation { + private PendingChanged pending = new PendingChanged<>(); + private final Supplier> supplier; + + public DeferredOperation(Supplier> supplier) { + this.supplier = supplier; + } + + public void process() { + pending.setValue(supplier.get()); + } + } +} diff --git a/core/src/main/java/org/openapitools/openapidiff/core/model/deferred/PendingChanged.java b/core/src/main/java/org/openapitools/openapidiff/core/model/deferred/PendingChanged.java new file mode 100644 index 000000000..1881f2ee2 --- /dev/null +++ b/core/src/main/java/org/openapitools/openapidiff/core/model/deferred/PendingChanged.java @@ -0,0 +1,169 @@ +package org.openapitools.openapidiff.core.model.deferred; + +import java.util.ArrayList; +import java.util.List; +import java.util.Optional; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.function.Consumer; +import java.util.function.Function; +import javax.annotation.Nullable; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +public class PendingChanged implements DeferredChanged { + private static final Logger log = LoggerFactory.getLogger(PendingChanged.class); + + private List> ifPresentConsumers = new ArrayList<>(); + private List>> whenSetConsumers = new ArrayList<>(); + + @Nullable private T value; + private Optional valueOptional = Optional.empty(); + private boolean valueIsPresent; + private boolean valueSet; + + private static final AtomicInteger deferredCounter = new AtomicInteger(); + private static final AtomicInteger resolvedCounter = new AtomicInteger(); + + @Override + public void ifPresent(Consumer consumer) { + if (valueSet) { + if (valueIsPresent) { + consumer.accept(value); + } + } else { + ifPresentConsumers.add(consumer); + } + } + + public void setValue(Optional value) { + if (!valueSet) { + this.valueSet = true; + this.valueIsPresent = value.isPresent(); + this.value = value.orElse(null); + this.valueOptional = value; + + log.debug("set {}", DeferredLogger.logValue(this.value)); + + if (this.valueIsPresent) { + ifPresentConsumers.forEach(c -> c.accept(this.value)); + } + + whenSetConsumers.forEach(c -> c.accept(this.valueOptional)); + + } else { + throw new IllegalStateException( + "PendingChanged may not be set more than once. Value was already set."); + } + } + + public boolean isPresent() { + return valueSet && valueIsPresent; + } + + public T get() { + return valueOptional.get(); + } + + public boolean isValueSet() { + return valueSet; + } + + public void whenSet(Consumer> consumer) { + if (valueSet) { + consumer.accept(valueOptional); + } else { + whenSetConsumers.add(consumer); + } + } + + public DeferredChanged map(Function, Q> function) { + return mapOptional((v) -> Optional.ofNullable(function.apply(v))); + } + + public DeferredChanged mapOptional(Function, Optional> function) { + if (valueSet) { + Optional result = function.apply(this.valueOptional); + log.debug( + "map resolved {} {} -> {}", + function, + DeferredLogger.logValue(this.value), + DeferredLogger.logValue(result)); + return new RealizedChanged(result); + } else { + final PendingChanged mappedChanged = new PendingChanged(); + log.debug("map deferred {} ? -> ?", function); + deferredCounter.incrementAndGet(); + whenSet( + (value) -> { + Optional result = function.apply(this.valueOptional); + log.debug( + "map resolved {} {} -> {}", + function, + DeferredLogger.logValue(this.value), + DeferredLogger.logValue(result)); + resolvedCounter.incrementAndGet(); + mappedChanged.setValue(result); + }); + return mappedChanged; + } + } + + public DeferredChanged flatMap(Function, DeferredChanged> function) { + if (valueSet) { + DeferredChanged nextDeferred = function.apply(this.valueOptional); + log.debug("flat map deferred {} {} -> ?", function, DeferredLogger.logValue(this.value)); + deferredCounter.incrementAndGet(); + nextDeferred.whenSet( + (nextValue) -> { + log.debug( + "flat map resolved {} {} -> {}", + function, + DeferredLogger.logValue(this.value), + DeferredLogger.logValue(nextValue)); + resolvedCounter.incrementAndGet(); + }); + return nextDeferred; + } else { + final PendingChanged mappedChanged = new PendingChanged<>(); + log.debug("flat map deferred {} ? -> ?", function); + deferredCounter.incrementAndGet(); + whenSet( + (value) -> { + DeferredChanged nextDeferred = function.apply(value); + nextDeferred.whenSet( + (nextValue) -> { + log.debug( + "flat map deferred {} {} -> {}", + function, + DeferredLogger.logValue(this.value), + DeferredLogger.logValue(nextValue)); + resolvedCounter.incrementAndGet(); + mappedChanged.setValue(nextValue); + }); + log.debug("flat map resolved {} {} -> ?", function, DeferredLogger.logValue(value)); + }); + return mappedChanged; + } + } + + @Override + public String toString() { + return "PendingChanged{" + + "value=" + + DeferredLogger.logValue(value) + + ", valueSet=" + + valueSet + + ", ifPresentConsumers.size=" + + ifPresentConsumers.size() + + ", whenSetConsumers.size=" + + whenSetConsumers.size() + + '}'; + } + + public static void logResolved() { + int deferred = deferredCounter.get(); + int resolved = resolvedCounter.get(); + log.debug( + "Outstanding: {} Deferred: {} Resolved {}", deferred - resolved, deferred, resolved); + } +} diff --git a/core/src/main/java/org/openapitools/openapidiff/core/model/deferred/RealizedChanged.java b/core/src/main/java/org/openapitools/openapidiff/core/model/deferred/RealizedChanged.java new file mode 100644 index 000000000..fccf66846 --- /dev/null +++ b/core/src/main/java/org/openapitools/openapidiff/core/model/deferred/RealizedChanged.java @@ -0,0 +1,64 @@ +package org.openapitools.openapidiff.core.model.deferred; + +import java.util.Optional; +import java.util.function.Consumer; +import java.util.function.Function; + +public class RealizedChanged implements DeferredChanged { + private final Optional value; + + public RealizedChanged(T value) { + this.value = Optional.ofNullable(value); + } + + public RealizedChanged(Optional value) { + this.value = value; + } + + @Override + public void ifPresent(Consumer consumer) { + value.ifPresent(consumer); + } + + @Override + public void whenSet(Consumer> consumer) { + consumer.accept(value); + } + + public static RealizedChanged empty() { + return new RealizedChanged(Optional.empty()); + } + + @Override + public boolean isPresent() { + return value.isPresent(); + } + + @Override + public boolean isValueSet() { + return true; + } + + @Override + public T get() { + return value.get(); + } + + public DeferredChanged map(Function, Q> function) { + return new RealizedChanged(function.apply(this.value)); + } + + @Override + public DeferredChanged mapOptional(Function, Optional> consumer) { + return new RealizedChanged<>(consumer.apply(this.value)); + } + + public DeferredChanged flatMap(Function, DeferredChanged> function) { + return function.apply(this.value); + } + + @Override + public String toString() { + return "RealizedChanged{" + "value=" + value + '}'; + } +} diff --git a/core/src/main/java/org/openapitools/openapidiff/core/model/deferred/RecursiveSchemaSet.java b/core/src/main/java/org/openapitools/openapidiff/core/model/deferred/RecursiveSchemaSet.java new file mode 100644 index 000000000..079139db2 --- /dev/null +++ b/core/src/main/java/org/openapitools/openapidiff/core/model/deferred/RecursiveSchemaSet.java @@ -0,0 +1,18 @@ +package org.openapitools.openapidiff.core.model.deferred; + +import java.util.HashSet; +import org.openapitools.openapidiff.core.compare.CacheKey; + +public class RecursiveSchemaSet { + HashSet leftKeys = new HashSet<>(); + HashSet rightKeys = new HashSet<>(); + + public boolean contains(CacheKey key) { + return leftKeys.contains(key.getLeft()) || rightKeys.contains(key.getRight()); + } + + public void put(CacheKey key) { + leftKeys.add(key.getLeft()); + leftKeys.add(key.getRight()); + } +} diff --git a/core/src/main/java/org/openapitools/openapidiff/core/model/deferred/SchemaDiffOperation.java b/core/src/main/java/org/openapitools/openapidiff/core/model/deferred/SchemaDiffOperation.java new file mode 100644 index 000000000..512b47a43 --- /dev/null +++ b/core/src/main/java/org/openapitools/openapidiff/core/model/deferred/SchemaDiffOperation.java @@ -0,0 +1,26 @@ +package org.openapitools.openapidiff.core.model.deferred; + +import io.swagger.v3.oas.models.media.Schema; +import org.openapitools.openapidiff.core.compare.CacheKey; +import org.openapitools.openapidiff.core.compare.OpenApiDiff; +import org.openapitools.openapidiff.core.model.ChangedSchema; + +public class SchemaDiffOperation { + final OpenApiDiff openApiDiff; + final RecursiveSchemaSet refSet; + final CacheKey key; + final Schema left; + final Schema right; + boolean processed; + + PendingChanged diffResult = new PendingChanged<>(); + + SchemaDiffOperation( + OpenApiDiff openApiDiff, RecursiveSchemaSet refSet, CacheKey key, Schema left, Schema right) { + this.openApiDiff = openApiDiff; + this.refSet = refSet; + this.key = key; + this.left = left; + this.right = right; + } +} 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 cf893c588..af9506cb4 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 6d136fb25..1c8168c41 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/RecursiveSchemaTest.java b/core/src/test/java/org/openapitools/openapidiff/core/RecursiveSchemaTest.java index 5d93c61dc..0f6d003ac 100644 --- a/core/src/test/java/org/openapitools/openapidiff/core/RecursiveSchemaTest.java +++ b/core/src/test/java/org/openapitools/openapidiff/core/RecursiveSchemaTest.java @@ -10,12 +10,18 @@ public class RecursiveSchemaTest { private final String OPENAPI_DOC1 = "recursive_model_1.yaml"; private final String OPENAPI_DOC2 = "recursive_model_2.yaml"; + private final String OPENAPI_DOC3 = "recursive_model_3.yaml"; @Test public void testDiffSame() { assertOpenApiAreEquals(OPENAPI_DOC1, OPENAPI_DOC1); } + @Test + public void testDiffDifferentCyclic() { + assertOpenApiBackwardIncompatible(OPENAPI_DOC1, OPENAPI_DOC3); + } + @Test public void testDiffDifferent() { assertOpenApiBackwardIncompatible(OPENAPI_DOC1, OPENAPI_DOC2); 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 056b92319..71d89f4ec 100644 --- a/core/src/test/java/org/openapitools/openapidiff/core/SecurityDiffTest.java +++ b/core/src/test/java/org/openapitools/openapidiff/core/SecurityDiffTest.java @@ -91,7 +91,7 @@ public void testWithUnknownSecurityScheme() { IllegalArgumentException.class, () -> OpenApiCompare.fromLocations(OPENAPI_DOC3, OPENAPI_DOC3)); assertThrows( - IllegalArgumentException.class, - () -> OpenApiCompare.fromLocations(OPENAPI_DOC4, OPENAPI_DOC4)); + IllegalArgumentException.class, + () -> OpenApiCompare.fromLocations(OPENAPI_DOC4, OPENAPI_DOC4)); } } 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 59ad6cac7..97b8ab094 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/core/src/test/java/org/openapitools/openapidiff/core/model/deferred/DeferredBuilderChangedTest.java b/core/src/test/java/org/openapitools/openapidiff/core/model/deferred/DeferredBuilderChangedTest.java new file mode 100644 index 000000000..2168e367c --- /dev/null +++ b/core/src/test/java/org/openapitools/openapidiff/core/model/deferred/DeferredBuilderChangedTest.java @@ -0,0 +1,175 @@ +package org.openapitools.openapidiff.core.model.deferred; + +import java.util.Optional; +import java.util.concurrent.atomic.AtomicBoolean; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.openapitools.openapidiff.core.model.schema.ChangedReadOnly; +import org.openapitools.openapidiff.core.model.schema.ChangedWriteOnly; + +public class DeferredBuilderChangedTest { + + private PendingChanged changed; + private Optional whenSet; + private ChangedWriteOnly mappedValue; + private Optional chainedValue; + + @BeforeEach + public void beforeEach() { + whenSet = Optional.empty(); + mappedValue = null; + chainedValue = Optional.empty(); + changed = new PendingChanged<>(); + } + + @Test + public void testPendingChangedValueSetBeforeListeners() { + PendingChanged changed = new PendingChanged(); + changed.setValue(Optional.of("Hello")); + ChangedAssertion assertion = new ChangedAssertion(changed); + assertion.assertSet(true); + } + + @Test + public void testPendingChangedValueSetAfterListeners() { + PendingChanged changed = new PendingChanged(); + ChangedAssertion assertion = new ChangedAssertion(changed); + changed.setValue(Optional.of("Hello")); + assertion.assertSet(true); + } + + @Test + public void testPendingChangedValueEMpty() { + PendingChanged changed = new PendingChanged(); + ChangedAssertion assertion = new ChangedAssertion(changed); + changed.setValue(Optional.empty()); + assertion.assertSet(false); + } + + @Test + public void testRealizedChange() { + RealizedChanged changed = new RealizedChanged<>("hello"); + ChangedAssertion assertion = new ChangedAssertion(changed); + assertion.assertSet(true); + } + + @Test + public void testRealizedChangeEmpty() { + RealizedChanged changed = new RealizedChanged<>(Optional.empty()); + ChangedAssertion assertion = new ChangedAssertion(changed); + assertion.assertSet(false); + } + + private static class ChangedAssertion { + AtomicBoolean map = new AtomicBoolean(false); + AtomicBoolean flatMap = new AtomicBoolean(false); + AtomicBoolean mapOptional = new AtomicBoolean(false); + AtomicBoolean whenSet = new AtomicBoolean(false); + AtomicBoolean ifPresent = new AtomicBoolean(false); + + public ChangedAssertion(DeferredChanged changed) { + changed.mapOptional( + (value) -> { + mapOptional.set(true); + return Optional.empty(); + }); + changed.map( + (value) -> { + map.set(true); + return Optional.empty(); + }); + changed.flatMap( + (value) -> { + flatMap.set(true); + return DeferredChanged.empty(); + }); + changed.whenSet( + (value) -> { + whenSet.set(true); + }); + changed.ifPresent( + (value) -> { + ifPresent.set(true); + }); + } + + public void assertSet(boolean expectedIfPresent) { + Assertions.assertTrue(mapOptional.get(), "mapOptional"); + Assertions.assertTrue(map.get(), "map"); + Assertions.assertTrue(flatMap.get(), "flatMap"); + Assertions.assertTrue(whenSet.get(), "whenSet"); + Assertions.assertEquals(expectedIfPresent, ifPresent.get(), "ifPresent"); + } + + public void assertNotSet() { + Assertions.assertFalse(mapOptional.get(), "mapOptional"); + Assertions.assertFalse(map.get(), "map"); + Assertions.assertFalse(flatMap.get(), "flatMap"); + Assertions.assertFalse(whenSet.get(), "whenSet"); + Assertions.assertFalse(ifPresent.get(), "ifPresent"); + } + } + + @Test + public void testFlatMap() { + PendingChanged flatMapPending = new PendingChanged<>(); + + changed.whenSet((value) -> this.whenSet = value); + + DeferredChanged chainedChanged = + changed.flatMap( + (value) -> { + System.out.println("Flatmap called"); + return flatMapPending; + }); + chainedChanged.whenSet(value -> chainedValue = value); + + DeferredChanged mappedDeferred = + changed.map(value -> new ChangedWriteOnly(false, true, null)); + mappedDeferred.ifPresent(v -> mappedValue = v); + + Assertions.assertFalse(whenSet.isPresent()); + Assertions.assertFalse(chainedValue.isPresent()); + Assertions.assertNull(mappedValue); + + changed.setValue(Optional.of(new ChangedReadOnly(false, true, null))); + Assertions.assertTrue(whenSet.isPresent()); + Assertions.assertNotNull(mappedValue); + Assertions.assertFalse(chainedValue.isPresent()); + + flatMapPending.setValue(Optional.of(new ChangedWriteOnly(false, true, null))); + Assertions.assertTrue(whenSet.isPresent()); + Assertions.assertTrue(chainedValue.isPresent()); + } + + @Test + public void testDeferredBuilderEmpty() { + DeferredBuilder builder = new DeferredBuilder(); + ChangedAssertion builderAssertion = new ChangedAssertion(builder.build()); + builderAssertion.assertSet(false); + } + + @Test + public void testDeferredBuilderAllRealized() { + DeferredBuilder builder = new DeferredBuilder(); + builder.add(new RealizedChanged("hello")); + builder.add(new RealizedChanged("bye")); + ChangedAssertion builderAssertion = new ChangedAssertion(builder.build()); + builderAssertion.assertSet(true); + } + + @Test + public void testDeferredBuilderPending() { + PendingChanged changed = new PendingChanged(); + + DeferredBuilder builder = new DeferredBuilder(); + builder.add(new RealizedChanged("hello")); + builder.add(changed); + ChangedAssertion builderAssertion = new ChangedAssertion(builder.build()); + builderAssertion.assertNotSet(); + + changed.setValue(Optional.of("hello")); + builderAssertion.assertSet(true); + } +} diff --git a/core/src/test/resources/recursive_model_3.yaml b/core/src/test/resources/recursive_model_3.yaml new file mode 100644 index 000000000..eea603965 --- /dev/null +++ b/core/src/test/resources/recursive_model_3.yaml @@ -0,0 +1,37 @@ +openapi: 3.0.1 +info: + title: recursive test + version: '1.0' +servers: + - url: 'http://localhost:8000/' +paths: + /ping: + get: + operationId: ping + responses: + '200': + description: OK + content: + text/plain: + schema: + $ref: '#/components/schemas/B' +components: + schemas: + B: + type: object + properties: + message: + type: string + details: + type: array + items: + $ref: '#/components/schemas/C' + C: + type: object + properties: + message: + type: string + details: + type: array + items: + $ref: '#/components/schemas/B'