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 fadae7a1..5ffcf774 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 @@ -332,8 +332,21 @@ protected DeferredChanged computeDeferredDiff( 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 { + } + + // A schema with no $ref of its own never reaches the guard above, so a cycle among such schemas + // was followed until the stack overflowed. Guarding on identity rather than on a reference also + // covers schemas whose reference resolveComposedSchema has already cleared. + if (left == null || right == null) { + return computeDiffForReal(refSet, left, right, context); + } + if (!refSet.enter(left, right)) { + return DeferredChanged.empty(); + } + try { return computeDiffForReal(refSet, left, right, context); + } finally { + refSet.leave(left, right); } } 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 index 890d2e6c..3fce7e8a 100644 --- 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 @@ -1,11 +1,13 @@ package org.openapitools.openapidiff.core.model.deferred; +import io.swagger.v3.oas.models.media.Schema; import java.util.HashSet; import org.openapitools.openapidiff.core.compare.CacheKey; public class RecursiveSchemaSet { HashSet leftKeys = new HashSet<>(); HashSet rightKeys = new HashSet<>(); + HashSet schemaPath = new HashSet<>(); public HashSet getLeftKeys() { return leftKeys; @@ -23,4 +25,38 @@ public void put(CacheKey key) { leftKeys.add(key.getLeft()); rightKeys.add(key.getRight()); } + + // Resolving an allOf copies the target's properties into the wrapper and then clears the allOf, + // so a cycle between two such wrappers ends up with no $ref left on either one and is invisible + // to the key-based guards above. Identity is all that still distinguishes them. + public boolean enter(Schema left, Schema right) { + return schemaPath.add(new SchemaPair(left, right)); + } + + public void leave(Schema left, Schema right) { + schemaPath.remove(new SchemaPair(left, right)); + } + + private static final class SchemaPair { + private final Schema left; + private final Schema right; + + private SchemaPair(Schema left, Schema right) { + this.left = left; + this.right = right; + } + + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (!(o instanceof SchemaPair)) return false; + SchemaPair other = (SchemaPair) o; + return left == other.left && right == other.right; + } + + @Override + public int hashCode() { + return 31 * System.identityHashCode(left) + System.identityHashCode(right); + } + } } 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 7f4114c6..5fa09f36 100644 --- a/core/src/test/java/org/openapitools/openapidiff/core/RecursiveSchemaTest.java +++ b/core/src/test/java/org/openapitools/openapidiff/core/RecursiveSchemaTest.java @@ -1,6 +1,7 @@ package org.openapitools.openapidiff.core; import static org.openapitools.openapidiff.core.TestUtils.assertOpenApiAreEquals; +import static org.openapitools.openapidiff.core.TestUtils.assertOpenApiBackwardCompatible; import static org.openapitools.openapidiff.core.TestUtils.assertOpenApiBackwardIncompatible; import org.junit.jupiter.api.Test; @@ -10,6 +11,9 @@ 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"; + private final String OPENAPI_DOC4 = "recursive_allof_model_1.yaml"; + private final String OPENAPI_DOC5 = "recursive_allof_model_2.yaml"; + private final String OPENAPI_DOC6 = "recursive_allof_model_3.yaml"; @Test public void testDiffSame() { @@ -25,4 +29,19 @@ public void testDiffDifferentCyclic() { public void testDiffDifferent() { assertOpenApiBackwardIncompatible(OPENAPI_DOC1, OPENAPI_DOC2); } + + @Test + public void testDiffSameWithAllOfWrappedCycle() { + assertOpenApiAreEquals(OPENAPI_DOC4, OPENAPI_DOC4); + } + + @Test + public void testDiffChangedInsideAllOfWrappedCycle() { + assertOpenApiBackwardIncompatible(OPENAPI_DOC4, OPENAPI_DOC5); + } + + @Test + public void testDiffAllOfWrappedAgainstBareCycle() { + assertOpenApiBackwardCompatible(OPENAPI_DOC4, OPENAPI_DOC6, true); + } } diff --git a/core/src/test/resources/recursive_allof_model_1.yaml b/core/src/test/resources/recursive_allof_model_1.yaml new file mode 100644 index 00000000..336a8a3e --- /dev/null +++ b/core/src/test/resources/recursive_allof_model_1.yaml @@ -0,0 +1,49 @@ +openapi: 3.0.1 +info: + title: allOf-wrapped recursive test + version: '1.0' +servers: + - url: 'http://localhost:8000/' +paths: + /ping: + get: + operationId: ping + responses: + '200': + description: OK + content: + application/json: + schema: + $ref: '#/components/schemas/Entry' +components: + schemas: + Entry: + type: object + properties: + message: + type: string + first: + title: First + allOf: + - $ref: '#/components/schemas/First' + description: Only reachable through an allOf wrapper + First: + type: object + properties: + name: + type: string + second: + title: Second + allOf: + - $ref: '#/components/schemas/Second' + description: Only reachable through an allOf wrapper + Second: + type: object + properties: + name: + type: string + first: + title: First + allOf: + - $ref: '#/components/schemas/First' + description: Closes the cycle without passing through Entry diff --git a/core/src/test/resources/recursive_allof_model_2.yaml b/core/src/test/resources/recursive_allof_model_2.yaml new file mode 100644 index 00000000..8c2c6425 --- /dev/null +++ b/core/src/test/resources/recursive_allof_model_2.yaml @@ -0,0 +1,49 @@ +openapi: 3.0.1 +info: + title: allOf-wrapped recursive test + version: '1.0' +servers: + - url: 'http://localhost:8000/' +paths: + /ping: + get: + operationId: ping + responses: + '200': + description: OK + content: + application/json: + schema: + $ref: '#/components/schemas/Entry' +components: + schemas: + Entry: + type: object + properties: + message: + type: string + first: + title: First + allOf: + - $ref: '#/components/schemas/First' + description: Only reachable through an allOf wrapper + First: + type: object + properties: + name: + type: integer + second: + title: Second + allOf: + - $ref: '#/components/schemas/Second' + description: Only reachable through an allOf wrapper + Second: + type: object + properties: + name: + type: string + first: + title: First + allOf: + - $ref: '#/components/schemas/First' + description: Closes the cycle without passing through Entry diff --git a/core/src/test/resources/recursive_allof_model_3.yaml b/core/src/test/resources/recursive_allof_model_3.yaml new file mode 100644 index 00000000..274a9229 --- /dev/null +++ b/core/src/test/resources/recursive_allof_model_3.yaml @@ -0,0 +1,46 @@ +openapi: 3.0.1 +info: + title: allOf-wrapped recursive test + version: '1.0' +servers: + - url: 'http://localhost:8000/' +paths: + /ping: + get: + operationId: ping + responses: + '200': + description: OK + content: + application/json: + schema: + $ref: '#/components/schemas/Entry' +components: + schemas: + Entry: + type: object + properties: + message: + type: string + first: + $ref: '#/components/schemas/First' + First: + type: object + properties: + name: + type: string + second: + title: Second + allOf: + - $ref: '#/components/schemas/Second' + description: Only reachable through an allOf wrapper + Second: + type: object + properties: + name: + type: string + first: + title: First + allOf: + - $ref: '#/components/schemas/First' + description: Closes the cycle without passing through Entry