Skip to content

Commit ca9c6ab

Browse files
solarhesssolarhess
authored
Make large, recursive schemas diff-able by deferring computation of diffs (#249)
* 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. * 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.
1 parent bda81cc commit ca9c6ab

40 files changed

+1685
-354
lines changed

‎cli/src/main/java/org/openapitools/openapidiff/cli/Main.java

+1-4
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,11 @@
11
package org.openapitools.openapidiff.cli;
22

3+
import io.swagger.v3.parser.core.models.AuthorizationValue;
34
import java.io.File;
45
import java.io.IOException;
56
import java.nio.charset.StandardCharsets;
6-
import java.util.Arrays;
77
import java.util.Collections;
88
import java.util.List;
9-
import java.util.stream.Collectors;
10-
11-
import io.swagger.v3.parser.core.models.AuthorizationValue;
129
import org.apache.commons.cli.CommandLine;
1310
import org.apache.commons.cli.CommandLineParser;
1411
import org.apache.commons.cli.DefaultParser;

‎core/src/main/java/org/openapitools/openapidiff/core/compare/ApiResponseDiff.java

+25-13
Original file line numberDiff line numberDiff line change
@@ -1,18 +1,17 @@
11
package org.openapitools.openapidiff.core.compare;
22

3-
import static org.openapitools.openapidiff.core.utils.ChangedUtils.isChanged;
4-
53
import io.swagger.v3.oas.models.responses.ApiResponse;
64
import io.swagger.v3.oas.models.responses.ApiResponses;
75
import java.util.LinkedHashMap;
86
import java.util.List;
97
import java.util.Map;
10-
import java.util.Optional;
8+
import javax.annotation.Nullable;
9+
import org.openapitools.openapidiff.core.model.Changed;
1110
import org.openapitools.openapidiff.core.model.ChangedApiResponse;
1211
import org.openapitools.openapidiff.core.model.ChangedResponse;
1312
import org.openapitools.openapidiff.core.model.DiffContext;
14-
15-
import javax.annotation.Nullable;
13+
import org.openapitools.openapidiff.core.model.deferred.DeferredBuilder;
14+
import org.openapitools.openapidiff.core.model.deferred.DeferredChanged;
1615

1716
/** Created by adarsh.sharma on 04/01/18. */
1817
public class ApiResponseDiff {
@@ -22,25 +21,38 @@ public ApiResponseDiff(OpenApiDiff openApiDiff) {
2221
this.openApiDiff = openApiDiff;
2322
}
2423

25-
public Optional<ChangedApiResponse> diff(@Nullable ApiResponses left, @Nullable ApiResponses right, DiffContext context) {
24+
public DeferredChanged<ChangedApiResponse> diff(
25+
@Nullable ApiResponses left, @Nullable ApiResponses right, DiffContext context) {
2626
MapKeyDiff<String, ApiResponse> responseMapKeyDiff = MapKeyDiff.diff(left, right);
2727
List<String> sharedResponseCodes = responseMapKeyDiff.getSharedKey();
2828
Map<String, ChangedResponse> resps = new LinkedHashMap<>();
29+
DeferredBuilder<Changed> builder = new DeferredBuilder<>();
30+
2931
for (String responseCode : sharedResponseCodes) {
30-
openApiDiff
31-
.getResponseDiff()
32-
.diff(left != null ? left.get(responseCode) : null, right != null ? right.get(responseCode) : null, context)
32+
builder
33+
.with(
34+
openApiDiff
35+
.getResponseDiff()
36+
.diff(
37+
left != null ? left.get(responseCode) : null,
38+
right != null ? right.get(responseCode) : null,
39+
context))
3340
.ifPresent(changedResponse -> resps.put(responseCode, changedResponse));
3441
}
3542
ChangedApiResponse changedApiResponse =
3643
new ChangedApiResponse(left, right, context)
3744
.setIncreased(responseMapKeyDiff.getIncreased())
3845
.setMissing(responseMapKeyDiff.getMissing())
3946
.setChanged(resps);
40-
openApiDiff
41-
.getExtensionsDiff()
42-
.diff(left != null ? left.getExtensions() : null, right != null ? right.getExtensions() : null, context)
47+
builder
48+
.with(
49+
openApiDiff
50+
.getExtensionsDiff()
51+
.diff(
52+
left != null ? left.getExtensions() : null,
53+
right != null ? right.getExtensions() : null,
54+
context))
4355
.ifPresent(changedApiResponse::setExtensions);
44-
return isChanged(changedApiResponse);
56+
return builder.buildIsChanged(changedApiResponse);
4557
}
4658
}

‎core/src/main/java/org/openapitools/openapidiff/core/compare/ContentDiff.java

+41-23
Original file line numberDiff line numberDiff line change
@@ -6,9 +6,12 @@
66
import io.swagger.v3.oas.models.media.Content;
77
import io.swagger.v3.oas.models.media.MediaType;
88
import java.util.*;
9+
import org.openapitools.openapidiff.core.model.Changed;
910
import org.openapitools.openapidiff.core.model.ChangedContent;
1011
import org.openapitools.openapidiff.core.model.ChangedMediaType;
1112
import org.openapitools.openapidiff.core.model.DiffContext;
13+
import org.openapitools.openapidiff.core.model.deferred.DeferredBuilder;
14+
import org.openapitools.openapidiff.core.model.deferred.DeferredChanged;
1215

1316
public class ContentDiff {
1417

@@ -18,32 +21,47 @@ public ContentDiff(OpenApiDiff openApiDiff) {
1821
this.openApiDiff = openApiDiff;
1922
}
2023

21-
public Optional<ChangedContent> diff(Content left, Content right, DiffContext context) {
24+
public DeferredChanged<ChangedContent> diff(Content left, Content right, DiffContext context) {
25+
DeferredBuilder<Changed> builder = new DeferredBuilder<Changed>();
2226

2327
MapKeyDiff<String, MediaType> mediaTypeDiff = MapKeyDiff.diff(left, right);
2428
List<String> sharedMediaTypes = mediaTypeDiff.getSharedKey();
2529
Map<String, ChangedMediaType> changedMediaTypes = new LinkedHashMap<>();
26-
for (String mediaTypeKey : sharedMediaTypes) {
27-
MediaType oldMediaType = left.get(mediaTypeKey);
28-
MediaType newMediaType = right.get(mediaTypeKey);
29-
ChangedMediaType changedMediaType =
30-
new ChangedMediaType(oldMediaType.getSchema(), newMediaType.getSchema(), context);
31-
openApiDiff
32-
.getSchemaDiff()
33-
.diff(
34-
new HashSet<>(),
35-
oldMediaType.getSchema(),
36-
newMediaType.getSchema(),
37-
context.copyWithRequired(true))
38-
.ifPresent(changedMediaType::setSchema);
39-
if (!isUnchanged(changedMediaType)) {
40-
changedMediaTypes.put(mediaTypeKey, changedMediaType);
41-
}
42-
}
43-
return isChanged(
44-
new ChangedContent(left, right, context)
45-
.setIncreased(mediaTypeDiff.getIncreased())
46-
.setMissing(mediaTypeDiff.getMissing())
47-
.setChanged(changedMediaTypes));
30+
31+
sharedMediaTypes.stream()
32+
.forEach(
33+
(mediaTypeKey) -> {
34+
MediaType oldMediaType = left.get(mediaTypeKey);
35+
MediaType newMediaType = right.get(mediaTypeKey);
36+
37+
ChangedMediaType changedMediaType =
38+
new ChangedMediaType(oldMediaType.getSchema(), newMediaType.getSchema(), context);
39+
40+
builder
41+
.with(
42+
openApiDiff
43+
.getSchemaDiff()
44+
.diff(
45+
oldMediaType.getSchema(),
46+
newMediaType.getSchema(),
47+
context.copyWithRequired(true)))
48+
.ifPresent(
49+
(value) -> {
50+
changedMediaType.setSchema(value);
51+
if (!isUnchanged(changedMediaType)) {
52+
changedMediaTypes.put(mediaTypeKey, changedMediaType);
53+
}
54+
});
55+
});
56+
57+
return builder
58+
.build()
59+
.mapOptional(
60+
(value) ->
61+
isChanged(
62+
new ChangedContent(left, right, context)
63+
.setIncreased(mediaTypeDiff.getIncreased())
64+
.setMissing(mediaTypeDiff.getMissing())
65+
.setChanged(changedMediaTypes)));
4866
}
4967
}

‎core/src/main/java/org/openapitools/openapidiff/core/compare/HeaderDiff.java

+24-17
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,15 @@
11
package org.openapitools.openapidiff.core.compare;
22

3-
import static org.openapitools.openapidiff.core.utils.ChangedUtils.isChanged;
4-
53
import io.swagger.v3.oas.models.Components;
64
import io.swagger.v3.oas.models.headers.Header;
75
import java.util.HashSet;
86
import java.util.Objects;
97
import java.util.Optional;
8+
import org.openapitools.openapidiff.core.model.Changed;
109
import org.openapitools.openapidiff.core.model.ChangedHeader;
1110
import org.openapitools.openapidiff.core.model.DiffContext;
11+
import org.openapitools.openapidiff.core.model.deferred.DeferredBuilder;
12+
import org.openapitools.openapidiff.core.model.deferred.DeferredChanged;
1213
import org.openapitools.openapidiff.core.utils.RefPointer;
1314
import org.openapitools.openapidiff.core.utils.RefType;
1415

@@ -31,16 +32,17 @@ public HeaderDiff(OpenApiDiff openApiDiff) {
3132
: null;
3233
}
3334

34-
public Optional<ChangedHeader> diff(Header left, Header right, DiffContext context) {
35+
public DeferredChanged<ChangedHeader> diff(Header left, Header right, DiffContext context) {
3536
return cachedDiff(new HashSet<>(), left, right, left.get$ref(), right.get$ref(), context);
3637
}
3738

3839
@Override
39-
protected Optional<ChangedHeader> computeDiff(
40+
protected DeferredChanged<ChangedHeader> computeDiff(
4041
HashSet<String> refSet, Header left, Header right, DiffContext context) {
4142
left = refPointer.resolveRef(leftComponents, left, left.get$ref());
4243
right = refPointer.resolveRef(rightComponents, right, right.get$ref());
4344

45+
DeferredBuilder<Changed> builder = new DeferredBuilder<Changed>();
4446
ChangedHeader changedHeader =
4547
new ChangedHeader(left, right, context)
4648
.setRequired(getBooleanDiff(left.getRequired(), right.getRequired()))
@@ -49,23 +51,28 @@ protected Optional<ChangedHeader> computeDiff(
4951
&& Boolean.TRUE.equals(right.getDeprecated()))
5052
.setStyle(!Objects.equals(left.getStyle(), right.getStyle()))
5153
.setExplode(getBooleanDiff(left.getExplode(), right.getExplode()));
52-
openApiDiff
53-
.getMetadataDiff()
54-
.diff(left.getDescription(), right.getDescription(), context)
54+
builder
55+
.with(
56+
openApiDiff
57+
.getMetadataDiff()
58+
.diff(left.getDescription(), right.getDescription(), context))
5559
.ifPresent(changedHeader::setDescription);
56-
openApiDiff
57-
.getSchemaDiff()
58-
.diff(new HashSet<>(), left.getSchema(), right.getSchema(), context.copyWithRequired(true))
60+
builder
61+
.with(
62+
openApiDiff
63+
.getSchemaDiff()
64+
.diff(left.getSchema(), right.getSchema(), context.copyWithRequired(true)))
5965
.ifPresent(changedHeader::setSchema);
60-
openApiDiff
61-
.getContentDiff()
62-
.diff(left.getContent(), right.getContent(), context)
66+
builder
67+
.with(openApiDiff.getContentDiff().diff(left.getContent(), right.getContent(), context))
6368
.ifPresent(changedHeader::setContent);
64-
openApiDiff
65-
.getExtensionsDiff()
66-
.diff(left.getExtensions(), right.getExtensions(), context)
69+
builder
70+
.with(
71+
openApiDiff
72+
.getExtensionsDiff()
73+
.diff(left.getExtensions(), right.getExtensions(), context))
6774
.ifPresent(changedHeader::setExtensions);
68-
return isChanged(changedHeader);
75+
return builder.buildIsChanged(changedHeader);
6976
}
7077

7178
private boolean getBooleanDiff(Boolean left, Boolean right) {

‎core/src/main/java/org/openapitools/openapidiff/core/compare/HeadersDiff.java

+15-10
Original file line numberDiff line numberDiff line change
@@ -6,10 +6,11 @@
66
import java.util.LinkedHashMap;
77
import java.util.List;
88
import java.util.Map;
9-
import java.util.Optional;
109
import org.openapitools.openapidiff.core.model.ChangedHeader;
1110
import org.openapitools.openapidiff.core.model.ChangedHeaders;
1211
import org.openapitools.openapidiff.core.model.DiffContext;
12+
import org.openapitools.openapidiff.core.model.deferred.DeferredBuilder;
13+
import org.openapitools.openapidiff.core.model.deferred.DeferredChanged;
1314

1415
/** Created by adarsh.sharma on 28/12/17. */
1516
public class HeadersDiff {
@@ -19,24 +20,28 @@ public HeadersDiff(OpenApiDiff openApiDiff) {
1920
this.openApiDiff = openApiDiff;
2021
}
2122

22-
public Optional<ChangedHeaders> diff(
23+
public DeferredChanged<ChangedHeaders> diff(
2324
Map<String, Header> left, Map<String, Header> right, DiffContext context) {
2425
MapKeyDiff<String, Header> headerMapDiff = MapKeyDiff.diff(left, right);
2526
List<String> sharedHeaderKeys = headerMapDiff.getSharedKey();
2627

2728
Map<String, ChangedHeader> changed = new LinkedHashMap<>();
29+
DeferredBuilder<ChangedHeader> builder = new DeferredBuilder<>();
2830
for (String headerKey : sharedHeaderKeys) {
2931
Header oldHeader = left.get(headerKey);
3032
Header newHeader = right.get(headerKey);
31-
openApiDiff
32-
.getHeaderDiff()
33-
.diff(oldHeader, newHeader, context)
33+
builder
34+
.with(openApiDiff.getHeaderDiff().diff(oldHeader, newHeader, context))
3435
.ifPresent(changedHeader -> changed.put(headerKey, changedHeader));
3536
}
36-
return isChanged(
37-
new ChangedHeaders(left, right, context)
38-
.setIncreased(headerMapDiff.getIncreased())
39-
.setMissing(headerMapDiff.getMissing())
40-
.setChanged(changed));
37+
return builder
38+
.build()
39+
.mapOptional(
40+
(value) ->
41+
isChanged(
42+
new ChangedHeaders(left, right, context)
43+
.setIncreased(headerMapDiff.getIncreased())
44+
.setMissing(headerMapDiff.getMissing())
45+
.setChanged(changed)));
4146
}
4247
}

‎core/src/main/java/org/openapitools/openapidiff/core/compare/OpenApiDiff.java

+20-9
Original file line numberDiff line numberDiff line change
@@ -8,14 +8,10 @@
88
import java.util.ArrayList;
99
import java.util.List;
1010
import java.util.Map;
11-
import java.util.Optional;
1211
import java.util.stream.Collectors;
13-
import org.openapitools.openapidiff.core.model.ChangedExtensions;
14-
import org.openapitools.openapidiff.core.model.ChangedOpenApi;
15-
import org.openapitools.openapidiff.core.model.ChangedOperation;
16-
import org.openapitools.openapidiff.core.model.ChangedPath;
17-
import org.openapitools.openapidiff.core.model.ChangedPaths;
18-
import org.openapitools.openapidiff.core.model.Endpoint;
12+
import org.openapitools.openapidiff.core.model.*;
13+
import org.openapitools.openapidiff.core.model.deferred.DeferredChanged;
14+
import org.openapitools.openapidiff.core.model.deferred.DeferredSchemaCache;
1915
import org.openapitools.openapidiff.core.utils.EndpointUtils;
2016
import org.slf4j.Logger;
2117
import org.slf4j.LoggerFactory;
@@ -48,6 +44,7 @@ public class OpenApiDiff {
4844
private List<Endpoint> missingEndpoints;
4945
private List<ChangedOperation> changedOperations;
5046
private ChangedExtensions changedExtensions;
47+
private DeferredSchemaCache deferredSchemaCache;
5148

5249
/*
5350
* @param oldSpecOpenApi
@@ -86,17 +83,25 @@ private void initializeFields() {
8683
this.oAuthFlowDiff = new OAuthFlowDiff(this);
8784
this.extensionsDiff = new ExtensionsDiff(this);
8885
this.metadataDiff = new MetadataDiff(this);
86+
this.deferredSchemaCache = new DeferredSchemaCache(this);
8987
}
9088

9189
private ChangedOpenApi compare() {
9290
preProcess(oldSpecOpenApi);
9391
preProcess(newSpecOpenApi);
94-
Optional<ChangedPaths> paths =
92+
93+
// 1st pass scans paths to collect all schemas
94+
DeferredChanged<ChangedPaths> paths =
9595
this.pathsDiff.diff(
9696
valOrEmpty(oldSpecOpenApi.getPaths()), valOrEmpty(newSpecOpenApi.getPaths()));
97+
98+
// 2nd pass processes deferred schemas
99+
deferredSchemaCache.process();
100+
97101
this.newEndpoints = new ArrayList<>();
98102
this.missingEndpoints = new ArrayList<>();
99103
this.changedOperations = new ArrayList<>();
104+
100105
paths.ifPresent(
101106
changedPaths -> {
102107
this.newEndpoints = EndpointUtils.convert2EndpointList(changedPaths.getIncreased());
@@ -117,6 +122,7 @@ private ChangedOpenApi compare() {
117122
getExtensionsDiff()
118123
.diff(oldSpecOpenApi.getExtensions(), newSpecOpenApi.getExtensions())
119124
.ifPresent(this::setChangedExtension);
125+
120126
return getChangedOpenApi();
121127
}
122128

@@ -162,7 +168,12 @@ private ChangedOpenApi getChangedOpenApi() {
162168
.setNewSpecOpenApi(newSpecOpenApi)
163169
.setOldSpecOpenApi(oldSpecOpenApi)
164170
.setChangedOperations(changedOperations)
165-
.setChangedExtensions(changedExtensions);
171+
.setChangedExtensions(changedExtensions)
172+
.setChangedSchemas(deferredSchemaCache.getChangedSchemas());
173+
}
174+
175+
public DeferredSchemaCache getDeferredSchemaCache() {
176+
return deferredSchemaCache;
166177
}
167178

168179
public PathsDiff getPathsDiff() {

0 commit comments

Comments
 (0)