From 363489df5f7cee0a8955eac9d846f2346bdb64a8 Mon Sep 17 00:00:00 2001 From: Julien Richard-Foy Date: Sun, 9 Jan 2022 23:49:54 +0100 Subject: [PATCH 1/7] Add failing test --- .../openapidiff/core/AllOfOneOfDiffTest.java | 14 +++ core/src/test/resources/issue-317_1.json | 84 +++++++++++++++++ core/src/test/resources/issue-317_2.json | 91 +++++++++++++++++++ 3 files changed, 189 insertions(+) create mode 100644 core/src/test/java/org/openapitools/openapidiff/core/AllOfOneOfDiffTest.java create mode 100644 core/src/test/resources/issue-317_1.json create mode 100644 core/src/test/resources/issue-317_2.json diff --git a/core/src/test/java/org/openapitools/openapidiff/core/AllOfOneOfDiffTest.java b/core/src/test/java/org/openapitools/openapidiff/core/AllOfOneOfDiffTest.java new file mode 100644 index 000000000..ec1bcfc53 --- /dev/null +++ b/core/src/test/java/org/openapitools/openapidiff/core/AllOfOneOfDiffTest.java @@ -0,0 +1,14 @@ +package org.openapitools.openapidiff.core; + +import static org.assertj.core.api.Assertions.assertThat; + +import org.junit.jupiter.api.Test; +import org.openapitools.openapidiff.core.model.ChangedOpenApi; + +public class AllOfOneOfDiffTest { + @Test + void allOfReferringToOneOfSchemasAreSupported() { + ChangedOpenApi diff = OpenApiCompare.fromLocations("issue-317_1.json", "issue-317_2.json"); + assertThat(diff.isCoreChanged().isUnchanged()); + } +} diff --git a/core/src/test/resources/issue-317_1.json b/core/src/test/resources/issue-317_1.json new file mode 100644 index 000000000..ab6f7357b --- /dev/null +++ b/core/src/test/resources/issue-317_1.json @@ -0,0 +1,84 @@ +{ + "openapi":"3.0.0", + "info":{ + "title":"API", + "version":"0.1.0" + }, + "paths":{ + "/resource":{ + "post":{ + "responses":{ + "200":{ + "description":"Created resource", + "content":{ + "application/json":{ + "schema":{ + "type":"string" + } + } + } + } + }, + "summary":"Create resource", + "requestBody":{ + "content":{ + "application/json":{ + "schema":{ + "$ref":"#/components/schemas/Resource" + } + } + }, + "description":"Definition of the resource" + } + } + } + }, + "components":{ + "schemas":{ + "Resource":{ + "type":"object", + "properties":{ + "assignment":{ + "$ref":"#/components/schemas/Foo" + } + } + }, + "Foo":{ + "oneOf":[ + { + "$ref":"#/components/schemas/Foo.Bar" + }, + { + "$ref":"#/components/schemas/Foo.Baz" + } + ], + "discriminator":{ + "propertyName":"type", + "mapping":{ + "Bar":"#/components/schemas/Foo.Bar", + "Baz":"#/components/schemas/Foo.Baz" + } + } + }, + "Foo.Bar":{ + "type":"object", + "properties":{ + "type":{ + "type":"string" + } + } + }, + "Foo.Baz":{ + "type":"object", + "properties":{ + "type":{ + "type":"string" + } + } + } + }, + "securitySchemes":{ + + } + } +} diff --git a/core/src/test/resources/issue-317_2.json b/core/src/test/resources/issue-317_2.json new file mode 100644 index 000000000..b3d35cde4 --- /dev/null +++ b/core/src/test/resources/issue-317_2.json @@ -0,0 +1,91 @@ +{ + "openapi":"3.0.0", + "info":{ + "title":"API", + "version":"0.1.0" + }, + "paths":{ + "/resource":{ + "post":{ + "responses":{ + "200":{ + "description":"Created resource", + "content":{ + "application/json":{ + "schema":{ + "type":"string" + } + } + } + } + }, + "summary":"Create resource", + "requestBody":{ + "content":{ + "application/json":{ + "schema":{ + "$ref":"#/components/schemas/Resource" + } + } + }, + "description":"Definition of the resource" + } + } + } + }, + "components":{ + "schemas":{ + "Resource":{ + "type":"object", + "properties":{ + "assignment":{ + "default":{ + "type":"Bar" + }, + "allOf":[ + { + "$ref":"#/components/schemas/Foo" + } + ] + } + } + }, + "Foo":{ + "oneOf":[ + { + "$ref":"#/components/schemas/Foo.Bar" + }, + { + "$ref":"#/components/schemas/Foo.Baz" + } + ], + "discriminator":{ + "propertyName":"type", + "mapping":{ + "Bar":"#/components/schemas/Foo.Bar", + "Baz":"#/components/schemas/Foo.Baz" + } + } + }, + "Foo.Bar":{ + "type":"object", + "properties":{ + "type":{ + "type":"string" + } + } + }, + "Foo.Baz":{ + "type":"object", + "properties":{ + "type":{ + "type":"string" + } + } + } + }, + "securitySchemes":{ + + } + } +} From e45c53942776f3fa8c011d4e80d5f7978aee15a0 Mon Sep 17 00:00:00 2001 From: Julien Richard-Foy Date: Mon, 10 Jan 2022 00:10:39 +0100 Subject: [PATCH 2/7] Fix #317 --- .../openapidiff/core/compare/SchemaDiff.java | 10 ++++++++++ 1 file changed, 10 insertions(+) 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 822160ede..9788bea0c 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 @@ -154,6 +154,16 @@ protected static Schema addSchema(Schema schema, Schema fromSchema) { } schema.getExtensions().putAll(fromSchema.getExtensions()); } + if (fromSchema instanceof ComposedSchema && schema instanceof ComposedSchema) { + ComposedSchema composedFromSchema = (ComposedSchema) fromSchema; + ComposedSchema composedSchema = (ComposedSchema) schema; + if (composedFromSchema.getOneOf() != null) { + if (composedSchema.getOneOf() == null) { + composedSchema.setOneOf(new ArrayList<>()); + } + composedSchema.getOneOf().addAll(composedFromSchema.getOneOf()); + } + } if (fromSchema.getDiscriminator() != null) { if (schema.getDiscriminator() == null) { schema.setDiscriminator(new Discriminator()); From b3f1331b5c03e96d281eaebd2747711c566f3669 Mon Sep 17 00:00:00 2001 From: Julien Richard-Foy Date: Thu, 27 Jan 2022 17:43:06 +0100 Subject: [PATCH 3/7] =?UTF-8?q?Address=20reviewer=E2=80=99s=20feedback?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../openapidiff/core/compare/SchemaDiff.java | 28 ++++++++----------- 1 file changed, 12 insertions(+), 16 deletions(-) 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 9788bea0c..35ef9e8e3 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 @@ -82,14 +82,20 @@ public static SchemaDiffResult getSchemaDiffResult( protected static Schema resolveComposedSchema(Components components, Schema schema) { if (schema instanceof ComposedSchema) { ComposedSchema composedSchema = (ComposedSchema) schema; - List allOfSchemaList = composedSchema.getAllOf(); - if (allOfSchemaList != null) { - for (Schema allOfSchema : allOfSchemaList) { - allOfSchema = refPointer.resolveRef(components, allOfSchema, allOfSchema.get$ref()); - allOfSchema = resolveComposedSchema(components, allOfSchema); - schema = addSchema(schema, allOfSchema); + List composedSchemas = new ArrayList<>(); + Optional.ofNullable(composedSchema.getAllOf()).ifPresent(composedSchemas::addAll); + Optional.ofNullable(composedSchema.getOneOf()).ifPresent(composedSchemas::addAll); + Optional.ofNullable(composedSchema.getAnyOf()).ifPresent(composedSchemas::addAll); + + if (!composedSchemas.isEmpty()) { + for (Schema composed : composedSchemas) { + composed = refPointer.resolveRef(components, composed, composed.get$ref()); + composed = resolveComposedSchema(components, composed); + schema = addSchema(schema, composed); } composedSchema.setAllOf(null); + composedSchema.setAnyOf(null); + composedSchema.setOneOf(null); } } return schema; @@ -154,16 +160,6 @@ protected static Schema addSchema(Schema schema, Schema fromSchema) { } schema.getExtensions().putAll(fromSchema.getExtensions()); } - if (fromSchema instanceof ComposedSchema && schema instanceof ComposedSchema) { - ComposedSchema composedFromSchema = (ComposedSchema) fromSchema; - ComposedSchema composedSchema = (ComposedSchema) schema; - if (composedFromSchema.getOneOf() != null) { - if (composedSchema.getOneOf() == null) { - composedSchema.setOneOf(new ArrayList<>()); - } - composedSchema.getOneOf().addAll(composedFromSchema.getOneOf()); - } - } if (fromSchema.getDiscriminator() != null) { if (schema.getDiscriminator() == null) { schema.setDiscriminator(new Discriminator()); From 0fb2f9cd8286951ec5631fe611cb74d99cfae773 Mon Sep 17 00:00:00 2001 From: Julien Richard-Foy Date: Thu, 10 Feb 2022 09:04:43 +0100 Subject: [PATCH 4/7] Check that discriminator is not null before de-referencing it --- .../compare/schemadiffresult/ComposedSchemaDiffResult.java | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) 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 e76aa0155..d5379d11c 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 @@ -9,7 +9,6 @@ import java.util.Map; import java.util.Optional; import java.util.stream.Collectors; -import org.apache.commons.collections4.CollectionUtils; import org.openapitools.openapidiff.core.compare.MapKeyDiff; import org.openapitools.openapidiff.core.compare.OpenApiDiff; import org.openapitools.openapidiff.core.model.ChangedOneOfSchema; @@ -43,8 +42,8 @@ public , X> DeferredChanged diff( ComposedSchema rightComposedSchema = (ComposedSchema) right; DeferredBuilder discriminatorChangedBuilder = new DeferredBuilder<>(); - if (CollectionUtils.isNotEmpty(leftComposedSchema.getOneOf()) - || CollectionUtils.isNotEmpty(rightComposedSchema.getOneOf())) { + if (leftComposedSchema.getDiscriminator() != null + || rightComposedSchema.getDiscriminator() != null) { Discriminator leftDis = leftComposedSchema.getDiscriminator(); Discriminator rightDis = rightComposedSchema.getDiscriminator(); From f90013c41194a5805048cf06127bbb4b82aaba3f Mon Sep 17 00:00:00 2001 From: Julien Richard-Foy Date: Thu, 10 Feb 2022 09:05:21 +0100 Subject: [PATCH 5/7] Prevent resolvedComposedSchema from looping indefinitely by keeping track of the already visited schemas --- .../openapidiff/core/compare/SchemaDiff.java | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) 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 35ef9e8e3..090018fcf 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 @@ -10,11 +10,13 @@ import io.swagger.v3.oas.models.media.Schema; import io.swagger.v3.oas.models.media.XML; import java.util.ArrayList; +import java.util.HashSet; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.Objects; import java.util.Optional; +import java.util.Set; import org.openapitools.openapidiff.core.compare.schemadiffresult.ArraySchemaDiffResult; import org.openapitools.openapidiff.core.compare.schemadiffresult.ComposedSchemaDiffResult; import org.openapitools.openapidiff.core.compare.schemadiffresult.SchemaDiffResult; @@ -79,7 +81,8 @@ public static SchemaDiffResult getSchemaDiffResult( } } - protected static Schema resolveComposedSchema(Components components, Schema schema) { + protected static Schema resolveComposedSchema( + Components components, Schema schema, Set visitedRefs) { if (schema instanceof ComposedSchema) { ComposedSchema composedSchema = (ComposedSchema) schema; List composedSchemas = new ArrayList<>(); @@ -89,9 +92,13 @@ protected static Schema resolveComposedSchema(Components components, Schema composed : composedSchemas) { - composed = refPointer.resolveRef(components, composed, composed.get$ref()); - composed = resolveComposedSchema(components, composed); - schema = addSchema(schema, composed); + if (composed.get$ref() == null || !visitedRefs.contains(composed.get$ref())) { + Set updatedVisitedRefs = new HashSet<>(visitedRefs); + updatedVisitedRefs.add(composed.get$ref()); + composed = refPointer.resolveRef(components, composed, composed.get$ref()); + composed = resolveComposedSchema(components, composed, updatedVisitedRefs); + schema = addSchema(schema, composed); + } } composedSchema.setAllOf(null); composedSchema.setAnyOf(null); @@ -322,8 +329,8 @@ public DeferredChanged computeDiffForReal( left = refPointer.resolveRef(this.leftComponents, left, getSchemaRef(left)); right = refPointer.resolveRef(this.rightComponents, right, getSchemaRef(right)); - left = resolveComposedSchema(leftComponents, left); - right = resolveComposedSchema(rightComponents, right); + left = resolveComposedSchema(leftComponents, left, new HashSet<>()); + right = resolveComposedSchema(rightComponents, right, new HashSet<>()); // If type of schemas are different, just set old & new schema, set changedType to true in // SchemaDiffResult and From 662fa6281f6331dfdcb3a9f3ee39074ca16b6b41 Mon Sep 17 00:00:00 2001 From: Julien Richard-Foy Date: Thu, 10 Feb 2022 10:37:59 +0100 Subject: [PATCH 6/7] fixup! Check that discriminator is not null before de-referencing it --- .../compare/schemadiffresult/ComposedSchemaDiffResult.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) 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 d5379d11c..e76aa0155 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 @@ -9,6 +9,7 @@ import java.util.Map; import java.util.Optional; import java.util.stream.Collectors; +import org.apache.commons.collections4.CollectionUtils; import org.openapitools.openapidiff.core.compare.MapKeyDiff; import org.openapitools.openapidiff.core.compare.OpenApiDiff; import org.openapitools.openapidiff.core.model.ChangedOneOfSchema; @@ -42,8 +43,8 @@ public , X> DeferredChanged diff( ComposedSchema rightComposedSchema = (ComposedSchema) right; DeferredBuilder discriminatorChangedBuilder = new DeferredBuilder<>(); - if (leftComposedSchema.getDiscriminator() != null - || rightComposedSchema.getDiscriminator() != null) { + if (CollectionUtils.isNotEmpty(leftComposedSchema.getOneOf()) + || CollectionUtils.isNotEmpty(rightComposedSchema.getOneOf())) { Discriminator leftDis = leftComposedSchema.getDiscriminator(); Discriminator rightDis = rightComposedSchema.getDiscriminator(); From 00dea6222263f6e51e4186c9fcc5fe095715922a Mon Sep 17 00:00:00 2001 From: Julien Richard-Foy Date: Thu, 10 Feb 2022 10:40:12 +0100 Subject: [PATCH 7/7] Restore initial solution --- .../openapidiff/core/compare/SchemaDiff.java | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) 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 090018fcf..1d1961bf7 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 @@ -87,7 +87,6 @@ protected static Schema resolveComposedSchema( ComposedSchema composedSchema = (ComposedSchema) schema; List composedSchemas = new ArrayList<>(); Optional.ofNullable(composedSchema.getAllOf()).ifPresent(composedSchemas::addAll); - Optional.ofNullable(composedSchema.getOneOf()).ifPresent(composedSchemas::addAll); Optional.ofNullable(composedSchema.getAnyOf()).ifPresent(composedSchemas::addAll); if (!composedSchemas.isEmpty()) { @@ -102,7 +101,6 @@ protected static Schema resolveComposedSchema( } composedSchema.setAllOf(null); composedSchema.setAnyOf(null); - composedSchema.setOneOf(null); } } return schema; @@ -167,6 +165,16 @@ protected static Schema addSchema(Schema schema, Schema fromSchema) { } schema.getExtensions().putAll(fromSchema.getExtensions()); } + if (fromSchema instanceof ComposedSchema && schema instanceof ComposedSchema) { + ComposedSchema composedFromSchema = (ComposedSchema) fromSchema; + ComposedSchema composedSchema = (ComposedSchema) schema; + if (composedFromSchema.getOneOf() != null) { + if (composedSchema.getOneOf() == null) { + composedSchema.setOneOf(new ArrayList<>()); + } + composedSchema.getOneOf().addAll(composedFromSchema.getOneOf()); + } + } if (fromSchema.getDiscriminator() != null) { if (schema.getDiscriminator() == null) { schema.setDiscriminator(new Discriminator());