GERONIMO-6692 ensure cyclic schema are using references to avoid infinite loops
authorRomain Manni-Bucau <rmannibucau@gmail.com>
Tue, 5 Feb 2019 09:26:29 +0000 (10:26 +0100)
committerRomain Manni-Bucau <rmannibucau@gmail.com>
Tue, 5 Feb 2019 09:26:29 +0000 (10:26 +0100)
geronimo-openapi-impl/src/main/java/org/apache/geronimo/microprofile/openapi/impl/processor/AnnotationProcessor.java
geronimo-openapi-impl/src/main/java/org/apache/geronimo/microprofile/openapi/impl/processor/SchemaProcessor.java
geronimo-openapi-impl/src/test/java/org/apache/geronimo/microprofile/openapi/impl/processor/SchemaProcessorTest.java

index bd9bb9b..22d52ac 100644 (file)
@@ -483,6 +483,14 @@ public class AnnotationProcessor {
     private void processComponents(final OpenAPI api, final Components components) {
         final org.eclipse.microprofile.openapi.models.Components impl = new ComponentsImpl();
         processCallbacks(impl, components.callbacks());
+        if (components.schemas().length > 0) {
+            impl.schemas(Stream.of(components.schemas())
+                               .map(it -> {
+                                   final String ref = of(it.name()).filter(n -> !n.isEmpty()).orElseGet(it::ref);
+                                   return new SchemaWithRef(ref, mapSchema(impl, it, ref));
+                               })
+                               .collect(toMap(it -> it.ref, it -> it.schema)));
+        }
         if (components.links().length > 0) {
             impl.links(Stream.of(components.links()).collect(
                     toMap(it -> of(it.name()).filter(n -> !n.isEmpty()).orElseGet(it::ref), this::mapLink)));
@@ -512,12 +520,6 @@ public class AnnotationProcessor {
             impl.examples(Stream.of(components.examples()).collect(toMap(
                     it -> of(it.name()).filter(n -> !n.isEmpty()).orElseGet(it::ref), this::mapExample)));
         }
-        if (components.schemas().length > 0) {
-            impl.schemas(Stream.of(components.schemas())
-                    .collect(toMap(
-                            it -> of(it.name()).filter(n -> !n.isEmpty()).orElseGet(it::ref),
-                            it -> mapSchema(impl, it))));
-        }
         if (components.responses().length > 0) {
             final APIResponses responses = new APIResponsesImpl();
             responses.putAll(Stream.of(components.responses())
@@ -529,8 +531,8 @@ public class AnnotationProcessor {
     }
 
     private org.eclipse.microprofile.openapi.models.media.Schema mapSchema(
-            final org.eclipse.microprofile.openapi.models.Components impl, final Schema schema) {
-        return ofNullable(schemaProcessor.mapSchema(impl, schema))
+            final org.eclipse.microprofile.openapi.models.Components impl, final Schema schema, final String ref) {
+        return ofNullable(schemaProcessor.mapSchema(impl, schema, ref))
                 .map(s -> s.externalDocs(mapExternalDocumentation(schema.externalDocs())))
                 .orElse(null);
     }
@@ -751,7 +753,7 @@ public class AnnotationProcessor {
         if (content.encoding().length > 0) {
             Stream.of(content.encoding()).forEach(e -> impl.addEncoding(e.name(), mapEncoding(components, e)));
         }
-        impl.setSchema(schemaProcessor.mapSchema(components, content.schema()));
+        impl.setSchema(schemaProcessor.mapSchema(components, content.schema(), null));
         if (content.examples().length > 0) {
             impl.examples(Stream.of(content.examples()).collect(toMap(
                     it -> of(it.name()).filter(n -> !n.isEmpty()).orElseGet(it::ref), this::mapExample)));
@@ -796,7 +798,7 @@ public class AnnotationProcessor {
         impl.description(header.description());
         impl.allowEmptyValue(header.allowEmptyValue());
         impl.required(header.required());
-        impl.schema(schemaProcessor.mapSchema(components, header.schema()));
+        impl.schema(schemaProcessor.mapSchema(components, header.schema(), null));
         impl.style(org.eclipse.microprofile.openapi.models.headers.Header.Style.SIMPLE);
         return impl;
     }
@@ -832,7 +834,7 @@ public class AnnotationProcessor {
                 .orElse(null));
         impl.allowEmptyValue(parameter.allowEmptyValue());
         impl.allowReserved(parameter.allowReserved());
-        impl.schema(ofNullable(schemaProcessor.mapSchema(components, parameter.schema()))
+        impl.schema(ofNullable(schemaProcessor.mapSchema(components, parameter.schema(), null))
                 .map(s -> s.externalDocs(mapExternalDocumentation(parameter.schema().externalDocs())))
                 .orElseGet(() -> {
                     if (annotatedElement == null) {
@@ -841,7 +843,7 @@ public class AnnotationProcessor {
                     return schemaProcessor.mapSchemaFromClass(components, annotatedElement.getType());
                 }));
         if (impl.getSchema() != null && impl.getSchema().getType() == null && annotatedElement != null) {
-            schemaProcessor.fillSchema(components, annotatedElement.getType(), impl.getSchema());
+            schemaProcessor.fillSchema(components, annotatedElement.getType(), impl.getSchema(), null);
         }
         of(parameter.content()).filter(it -> it.length > 0).map(Stream::of).ifPresent(c -> {
             final ContentImpl content = new ContentImpl();
@@ -1036,4 +1038,15 @@ public class AnnotationProcessor {
             return Tag.class;
         }
     }
+
+    private static class SchemaWithRef {
+        private final String ref;
+        private final org.eclipse.microprofile.openapi.models.media.Schema schema;
+
+        private SchemaWithRef(final String ref,
+                              final org.eclipse.microprofile.openapi.models.media.Schema schema) {
+            this.ref = ref;
+            this.schema = schema;
+        }
+    }
 }
index a0fcf8e..68679a8 100644 (file)
@@ -45,23 +45,35 @@ import org.eclipse.microprofile.openapi.models.Components;
 
 public class SchemaProcessor {
     private final Map<Type, org.eclipse.microprofile.openapi.models.media.Schema> cache = new HashMap<>();
+    private final Map<Class<?>, String> providedRefs = new HashMap<>();
 
     public org.eclipse.microprofile.openapi.models.media.Schema mapSchemaFromClass(
             final org.eclipse.microprofile.openapi.models.Components components, final Type model) {
         final org.eclipse.microprofile.openapi.models.media.Schema cached = cache.get(model);
         if (cached != null) {
-            return cached;
+            return new SchemaImpl().type(cached.getType()).ref(toRef(Class.class.cast(model), null));
         }
-        final SchemaImpl newSchema = new SchemaImpl();
-        if (Class.class.isInstance(model) && !Class.class.cast(model).isPrimitive() && model != String.class) {
-            cache.put(Class.class.cast(model), newSchema);
-        }
-        fillSchema(components, model, newSchema);
-        return newSchema;
+        final SchemaImpl schema = new SchemaImpl();
+        fillSchema(components, model, schema, null);
+        return schema;
+    }
+
+    private String toRef(final Class<?> model, final String providedRef) {
+        return "#/components/schemas/" + toRefName(model, providedRef);
+    }
+
+    // todo: introduce naming strategy? simplename can conflict so this is safer but ugly
+    private String toRefName(final Class<?> model, final String providedRef) {
+        return providedRefs.computeIfAbsent(
+                model, k -> ofNullable(providedRef)
+                        .orElseGet(() -> k.getName().replace('.', '_').replace('$', '_')));
     }
 
-    public void fillSchema(final org.eclipse.microprofile.openapi.models.Components components, final Type model,
-            final org.eclipse.microprofile.openapi.models.media.Schema schema) {
+    public void fillSchema(
+            final org.eclipse.microprofile.openapi.models.Components components,
+            final Type model,
+            final org.eclipse.microprofile.openapi.models.media.Schema schema,
+            final String providedRef) {
         if (Class.class.isInstance(model)) {
             if (boolean.class == model) {
                 schema.type(org.eclipse.microprofile.openapi.models.media.Schema.SchemaType.BOOLEAN);
@@ -85,19 +97,22 @@ public class SchemaProcessor {
                 final Class from = Class.class.cast(model);
                 if (from.isEnum()) {
                     schema.type(org.eclipse.microprofile.openapi.models.media.Schema.SchemaType.STRING)
-                          .enumeration(asList(from.getEnumConstants()))
-                          .nullable(true);
+                                 .enumeration(asList(from.getEnumConstants()))
+                                 .nullable(true);
                 } else {
                     ofNullable(from.getAnnotation(Schema.class)).ifPresent(
-                            s -> sets(components, Schema.class.cast(s), schema));
+                            s -> sets(components, Schema.class.cast(s), schema, null));
 
                     schema.type(org.eclipse.microprofile.openapi.models.media.Schema.SchemaType.OBJECT);
 
                     final org.eclipse.microprofile.openapi.models.media.Schema existingSchema = cache.get(from);
                     if (existingSchema != null && existingSchema != schema) {
-                        schema.setProperties(existingSchema.getProperties());
-                        schema.setRequired(existingSchema.getRequired());
+                        schema.ref(toRef(from, providedRef));
                     } else {
+                        if (cache.putIfAbsent(from, schema) == null) {
+                            components.addSchema(toRefName(from, providedRef), schema);
+                        }
+
                         schema.properties(new HashMap<>());
                         Class<?> current = from;
                         // todo: use getters first then fields, for JSON-B the private only fields must be ignored
@@ -125,7 +140,9 @@ public class SchemaProcessor {
                     schema.type(org.eclipse.microprofile.openapi.models.media.Schema.SchemaType.OBJECT);
                 } else if (pt.getActualTypeArguments().length == 1 && Class.class.isInstance(pt.getActualTypeArguments()[0])) {
                     schema.type(org.eclipse.microprofile.openapi.models.media.Schema.SchemaType.ARRAY);
-                    fillSchema(components, Class.class.cast(pt.getActualTypeArguments()[0]), schema.getItems());
+                    final SchemaImpl items = new SchemaImpl();
+                    fillSchema(components, Class.class.cast(pt.getActualTypeArguments()[0]), items, null);
+                    schema.items(items);
                 } else {
                     schema.type(org.eclipse.microprofile.openapi.models.media.Schema.SchemaType.ARRAY);
                 }
@@ -154,7 +171,7 @@ public class SchemaProcessor {
 
     private org.eclipse.microprofile.openapi.models.media.Schema mapField(final Components components, final Field f) {
         final Schema annotation = f.getAnnotation(Schema.class);
-        return ofNullable(annotation).map(s -> mapSchema(components, s)).orElseGet(() -> {
+        return ofNullable(annotation).map(s -> mapSchema(components, s, null)).orElseGet(() -> {
             final org.eclipse.microprofile.openapi.models.media.Schema schemaFromClass = mapSchemaFromClass(
                     components, f.getGenericType());
             if (annotation != null) {
@@ -176,8 +193,9 @@ public class SchemaProcessor {
                 });
     }
 
-    private void mergeSchema(final Components components, final org.eclipse.microprofile.openapi.models.media.Schema impl,
-            final Schema schema) {
+    private void mergeSchema(final Components components,
+                             final org.eclipse.microprofile.openapi.models.media.Schema impl,
+                             final Schema schema) {
         impl.deprecated(schema.deprecated());
         if (schema.type() != SchemaType.DEFAULT) {
             impl.type(org.eclipse.microprofile.openapi.models.media.Schema.SchemaType.valueOf(schema.type().name()));
@@ -261,7 +279,8 @@ public class SchemaProcessor {
     }
 
     public org.eclipse.microprofile.openapi.models.media.Schema mapSchema(
-            final org.eclipse.microprofile.openapi.models.Components components, final Schema schema) {
+            final org.eclipse.microprofile.openapi.models.Components components, final Schema schema,
+            final String providedRefMapping) {
         if (schema.hidden() || (schema.implementation() == Void.class && schema.name().isEmpty() && schema.ref().isEmpty())) {
             if (schema.type() != SchemaType.DEFAULT) {
                 return new SchemaImpl()
@@ -271,21 +290,24 @@ public class SchemaProcessor {
         }
 
         final SchemaImpl impl = new SchemaImpl();
-        sets(components, schema, impl);
+        sets(components, schema, impl, providedRefMapping);
         return impl;
     }
 
     private void sets(final Components components, final Schema schema,
-            final org.eclipse.microprofile.openapi.models.media.Schema impl) {
+            final org.eclipse.microprofile.openapi.models.media.Schema impl,
+            final String providedRef) {
         if (!schema.ref().isEmpty()) {
             impl.ref(resolveSchemaRef(components, schema.ref()));
         } else {
             if (schema.implementation() != Void.class) {
                 final boolean array = schema.type() == SchemaType.ARRAY;
-                final org.eclipse.microprofile.openapi.models.media.Schema ref = array ? new SchemaImpl() : impl;
-                fillSchema(components, schema.implementation(), ref);
                 if (array) {
-                    impl.items(ref);
+                    final SchemaImpl itemSchema = new SchemaImpl();
+                    fillSchema(components, schema.implementation(), itemSchema, providedRef);
+                    impl.items(itemSchema);
+                } else {
+                    fillSchema(components, schema.implementation(), impl, providedRef);
                 }
             }
             mergeSchema(components, impl, schema);
index b2a0a1f..0c0afb0 100644 (file)
@@ -26,6 +26,7 @@ import java.util.stream.Stream;
 import javax.json.bind.annotation.JsonbProperty;
 
 import org.apache.geronimo.microprofile.openapi.impl.model.ComponentsImpl;
+import org.eclipse.microprofile.openapi.models.Components;
 import org.eclipse.microprofile.openapi.models.media.Schema;
 import org.testng.annotations.Test;
 
@@ -55,18 +56,24 @@ public class SchemaProcessorTest {
 
     @Test
     public void cyclicRef() {
-        final Schema schema = new SchemaProcessor().mapSchemaFromClass(new ComponentsImpl(), SomeClass.class);
+        final Components components = new ComponentsImpl();
+        final Schema schema = new SchemaProcessor().mapSchemaFromClass(components, SomeClass.class);
         assertEquals(3, schema.getProperties().size());
         assertEquals(Schema.SchemaType.STRING, schema.getProperties().get("simple").getType());
         assertSomeClass(schema.getProperties().get("child"));
         final Schema children = schema.getProperties().get("children");
         assertEquals(Schema.SchemaType.ARRAY, children.getType());
         assertSomeRelatedClass(children.getItems());
+        assertEquals(2, components.getSchemas().size());
+        final Schema completeSchema =
+                components.getSchemas().get("org_apache_geronimo_microprofile_openapi_impl_processor_SchemaProcessorTest_SomeClass");
+        assertEquals(3, completeSchema.getProperties().size());
+        assertEquals(Stream.of("simple", "child", "children").collect(toSet()), completeSchema.getProperties().keySet());
     }
 
     private void assertSomeClass(final Schema schema) {
         assertEquals(Schema.SchemaType.OBJECT, schema.getType());
-        assertEquals(Stream.of("simple", "child", "children").collect(toSet()), schema.getProperties().keySet());
+        assertEquals("#/components/schemas/org_apache_geronimo_microprofile_openapi_impl_processor_SchemaProcessorTest_SomeClass", schema.getRef());
     }
 
     private void assertSomeRelatedClass(final Schema schema) {