Fix plugin subclassing in Log4j Docgen (#117, #120)

log4j-docgen/src/main/java/org/apache/logging/log4j/docgen/generator/internal/TypeLookup.java

@@ -23,6 +23,8 @@
 import org.apache.logging.log4j.docgen.AbstractType;
 import org.apache.logging.log4j.docgen.PluginSet;
 import org.apache.logging.log4j.docgen.PluginType;
+import org.apache.logging.log4j.docgen.ScalarType;
+import org.apache.logging.log4j.docgen.Type;
 import org.jspecify.annotations.Nullable;
 
 public final class TypeLookup extends TreeMap<String, ArtifactSourcedType> {
@@ -42,21 +44,109 @@ private TypeLookup(final Iterable<? extends PluginSet> pluginSets, final Predica
 
     private void mergeDescriptors(Iterable<? extends PluginSet> pluginSets) {
         pluginSets.forEach(pluginSet -> {
-            pluginSet.getScalars().forEach(scalar -> {
-                final ArtifactSourcedType sourcedType = ArtifactSourcedType.ofPluginSet(pluginSet, scalar);
-                putIfAbsent(scalar.getClassName(), sourcedType);
-            });
-            pluginSet.getAbstractTypes().forEach(abstractType -> {
-                final ArtifactSourcedType sourcedType = ArtifactSourcedType.ofPluginSet(pluginSet, abstractType);
-                putIfAbsent(abstractType.getClassName(), sourcedType);
-            });
-            pluginSet.getPlugins().forEach(pluginType -> {
-                final ArtifactSourcedType sourcedType = ArtifactSourcedType.ofPluginSet(pluginSet, pluginType);
-                putIfAbsent(pluginType.getClassName(), sourcedType);
-            });
+            mergeScalarTypes(pluginSet);
+            mergeAbstractTypes(pluginSet);
+            mergePluginTypes(pluginSet);
+        });
+    }
+
+    private void mergeScalarTypes(PluginSet pluginSet) {
+        pluginSet.getScalars().forEach(newType -> {
+            final String className = newType.getClassName();
+            final ArtifactSourcedType newSourcedType = ArtifactSourcedType.ofPluginSet(pluginSet, newType);
+            merge(className, newSourcedType, TypeLookup::mergeScalarType);
+        });
+    }
+
+    private static ArtifactSourcedType mergeScalarType(
+            final ArtifactSourcedType oldSourcedType, final ArtifactSourcedType newSourcedType) {
+        // If the entry already exists and is of expected type, we should ideally extend it.
+        // Since Modello doesn't generate `hashCode()`, `equals()`, etc. it is difficult to compare instances.
+        // Hence, we will be lazy, and just assume they are the same.
+        if (oldSourcedType.type instanceof ScalarType) {
+            return oldSourcedType;
+        }
+
+        // If the entry already exists, but with an unexpected type, fail
+        else {
+            throw conflictingTypeFailure(oldSourcedType.type, newSourcedType.type);
+        }
+    }
+
+    private static RuntimeException conflictingTypeFailure(final Type oldType, final Type newType) {
+        final String message = String.format(
+                "`%s` class occurs multiple times with conflicting types: `%s` and `%s`",
+                oldType.getClassName(),
+                oldType.getClass().getSimpleName(),
+                newType.getClass().getSimpleName());
+        return new IllegalArgumentException(message);
+    }
+
+    private void mergeAbstractTypes(PluginSet pluginSet) {
+        pluginSet.getAbstractTypes().forEach(newType -> {
+            final String className = newType.getClassName();
+            final ArtifactSourcedType newSourcedType = ArtifactSourcedType.ofPluginSet(pluginSet, newType);
+            merge(className, newSourcedType, TypeLookup::mergeAbstractType);
+        });
+    }
+
+    private static ArtifactSourcedType mergeAbstractType(
+            final ArtifactSourcedType oldSourcedType, final ArtifactSourcedType newSourcedType) {
+
+        // If the entry already exists and is of expected type, extend it
+        if (oldSourcedType.type instanceof AbstractType) {
+            final AbstractType oldType = (AbstractType) oldSourcedType.type;
+            final AbstractType newType = (AbstractType) newSourcedType.type;
+            newType.getImplementations().forEach(oldType::addImplementation);
+            return oldSourcedType;
+        }
+
+        // If the entry already exists, but with an unexpected type, fail
+        else {
+            throw conflictingTypeFailure(oldSourcedType.type, newSourcedType.type);
+        }
+    }
+
+    private void mergePluginTypes(PluginSet pluginSet) {
+        pluginSet.getPlugins().forEach(newType -> {
+            final String className = newType.getClassName();
+            final ArtifactSourcedType newSourcedType = ArtifactSourcedType.ofPluginSet(pluginSet, newType);
+            merge(className, newSourcedType, TypeLookup::mergePluginType);
         });
     }
 
+    private static ArtifactSourcedType mergePluginType(
+            final ArtifactSourcedType oldSourcedType, final ArtifactSourcedType newSourcedType) {
+
+        // If the entry already exists, but is of `AbstractType`, promote it to `PluginType`.
+        //
+        // The most prominent example to this is `LoggerConfig`, which is a plugin.
+        // Assume `AsyncLoggerConfig` (extending from `LoggerConfig`) is encountered first.
+        // This results in `LoggerConfig` getting registered as an `AbstractType`.
+        // When the actual `LoggerConfig` definition is encountered, the type needs to be promoted to `PluginType`.
+        // Otherwise, `LoggerConfig` plugin definition will get skipped.
+        if (oldSourcedType.type instanceof AbstractType && !(oldSourcedType.type instanceof PluginType)) {
+            final PluginType newType = (PluginType) newSourcedType.type;
+            // Preserve old implementations
+            final AbstractType oldType = (AbstractType) oldSourcedType.type;
+            oldType.getImplementations().forEach(newType::addImplementation);
+            return newSourcedType;
+        }
+
+        // If the entry already exists and is of expected type, extend it
+        else if (oldSourcedType.type instanceof PluginType) {
+            final PluginType oldType = (PluginType) oldSourcedType.type;
+            final PluginType newType = (PluginType) newSourcedType.type;
+            newType.getImplementations().forEach(oldType::addImplementation);
+            return oldSourcedType;
+        }
+
+        // If the entry already exists, but with an unexpected type, fail
+        else {
+            throw conflictingTypeFailure(oldSourcedType.type, newSourcedType.type);
+        }
+    }
+
     private void populateTypeHierarchy(Iterable<? extends PluginSet> pluginSets) {
         pluginSets.forEach(pluginSet -> {
             final Set<PluginType> pluginTypes = pluginSet.getPlugins();

log4j-docgen/src/main/java/org/apache/logging/log4j/docgen/processor/DescriptorGenerator.java

@@ -135,11 +135,11 @@ public class DescriptorGenerator extends AbstractProcessor {
      */
     private static final String IMPOSSIBLE_REGEX = "(?!.*)";
 
-    // Abstract types to process
-    private final Collection<TypeElement> abstractTypesToDocument = new HashSet<>();
+    private final Set<TypeElement> pluginTypesToDocument = new HashSet<>();
 
-    // Scalar types to process
-    private final Collection<TypeElement> scalarTypesToDocument = new HashSet<>();
+    private final Set<TypeElement> abstractTypesToDocument = new HashSet<>();
+
+    private final Set<TypeElement> scalarTypesToDocument = new HashSet<>();
 
     private Predicate<String> classNameFilter;
 
@@ -253,7 +253,8 @@ public SourceVersion getSupportedSourceVersion() {
     @Override
     public boolean process(final Set<? extends TypeElement> unused, final RoundEnvironment roundEnv) {
         // First step: document plugins
-        roundEnv.getElementsAnnotatedWithAny(annotations.getPluginAnnotations()).forEach(this::addPluginDocumentation);
+        populatePluginTypesToDocument(roundEnv);
+        pluginTypesToDocument.forEach(this::addPluginDocumentation);
         // Second step: document abstract types
         abstractTypesToDocument.forEach(this::addAbstractTypeDocumentation);
         // Second step: document scalars
@@ -265,28 +266,39 @@ public boolean process(final Set<? extends TypeElement> unused, final RoundEnvir
         return false;
     }
 
-    private void addPluginDocumentation(final Element element) {
-        try {
+    private void populatePluginTypesToDocument(final RoundEnvironment roundEnv) {
+        roundEnv.getElementsAnnotatedWithAny(annotations.getPluginAnnotations()).forEach(element -> {
             if (element instanceof TypeElement) {
-                final PluginType pluginType = new PluginType();
-                pluginType.setName(annotations.getPluginSpecifiedName(element).orElseGet(() -> element.getSimpleName()
-                        .toString()));
-                pluginType.setNamespace(
-                        annotations.getPluginSpecifiedNamespace(element).orElse("Core"));
-                populatePlugin((TypeElement) element, pluginType);
-                pluginSet.addPlugin(pluginType);
+                pluginTypesToDocument.add((TypeElement) element);
             } else {
                 messager.printMessage(
                         Diagnostic.Kind.WARNING, "Found @Plugin annotation on unexpected element.", element);
             }
+        });
+    }
+
+    private void addPluginDocumentation(final TypeElement element) {
+        try {
+            final PluginType pluginType = new PluginType();
+            pluginType.setName(annotations.getPluginSpecifiedName(element).orElseGet(() -> element.getSimpleName()
+                    .toString()));
+            pluginType.setNamespace(
+                    annotations.getPluginSpecifiedNamespace(element).orElse("Core"));
+            populatePlugin(element, pluginType);
+            pluginSet.addPlugin(pluginType);
         } catch (final Exception error) {
             final String message = String.format("failed to process element `%s`", element);
             throw new RuntimeException(message, error);
         }
     }
 
+    @SuppressWarnings("SuspiciousMethodCalls")
     private void addAbstractTypeDocumentation(final QualifiedNameable element) {
         try {
+            // Short-circuit if the type is already documented as a plugin
+            if (pluginTypesToDocument.contains(element)) {
+                return;
+            }
             final AbstractType abstractType = new AbstractType();
             final ElementImports imports = importsFactory.ofElement(element);
             final String qualifiedClassName = getClassName(element.asType());

log4j-docgen/src/test/resources/DescriptorGeneratorTest/expected-plugins.xml

@@ -123,6 +123,19 @@ It also implements:
 * apiref:example.Appender[],
 * apiref:example.BaseAppender[]</description>
         </plugin>
+        <plugin name="MyAppenderSubclassingAppender" namespace="namespace" className="example.MyAppenderSubclassingAppender">
+            <supertypes>
+                <supertype>example.AbstractAppender</supertype>
+                <supertype>example.Appender</supertype>
+                <supertype>example.BaseAppender</supertype>
+                <supertype>example.MyAppender</supertype>
+                <supertype>java.lang.Object</supertype>
+            </supertypes>
+            <attributes>
+                <attribute name="awesomenessEnabled" type="boolean"/>
+            </attributes>
+            <description>Example plugin to demonstrate the case where a plugin subclasses another plugin.</description>
+        </plugin>
         <plugin name="MyLayout" className="example.MyOldLayout">
             <supertypes>
                 <supertype>example.Layout</supertype>

log4j-docgen/src/test/resources/DescriptorGeneratorTest/java-of-log4j2/MyAppender.java

@@ -45,7 +45,7 @@
  * </ul>
  */
 @Plugin(name = "MyAppender", category = "namespace")
-public final class MyAppender extends AbstractAppender implements Appender {
+public class MyAppender extends AbstractAppender implements Appender {
 
     /**
      * Parent builder with some private fields that are not returned by

log4j-docgen/src/test/resources/DescriptorGeneratorTest/java-of-log4j2/MyAppenderSubclassingAppender.java

@@ -0,0 +1,42 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package example;
+
+import java.util.List;
+import java.util.Set;
+import javax.lang.model.element.TypeElement;
+import org.apache.logging.log4j.core.config.plugins.Plugin;
+import org.apache.logging.log4j.core.config.plugins.PluginAttribute;
+import org.apache.logging.log4j.core.config.plugins.PluginFactory;
+
+/**
+ * Example plugin to demonstrate the case where a plugin subclasses another plugin.
+ *
+ * @see <a href="https://github.com/apache/logging-log4j-tools/issues/117">apache/logging-log4j-tools#117</a>
+ */
+@Plugin(name = "MyAppenderSubclassingAppender", category = "namespace")
+public final class MyAppenderSubclassingAppender extends MyAppender {
+
+    /**
+     * The canonical constructor.
+     */
+    @PluginFactory
+    public static MyAppenderSubclassingAppender newLayout(
+            final @PluginAttribute(value = "awesomenessEnabled", defaultBoolean = true) boolean awesomenessEnabled) {
+        return null;
+    }
+}

log4j-docgen/src/test/resources/DescriptorGeneratorTest/java-of-log4j3/MyAppender.java

@@ -48,7 +48,7 @@
  */
 @Plugin
 @Namespace("namespace")
-public final class MyAppender extends AbstractAppender implements Appender {
+public class MyAppender extends AbstractAppender implements Appender {
 
     /**
      * Parent builder with some private fields that are not returned by

log4j-docgen/src/test/resources/DescriptorGeneratorTest/java-of-log4j3/MyAppenderSubclassingAppender.java

@@ -0,0 +1,43 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package example;
+
+import java.util.List;
+import java.util.Set;
+import org.apache.logging.log4j.plugins.Factory;
+import org.apache.logging.log4j.plugins.Namespace;
+import org.apache.logging.log4j.plugins.Plugin;
+import org.apache.logging.log4j.plugins.PluginAttribute;
+
+/**
+ * Example plugin to demonstrate the case where a plugin subclasses another plugin.
+ *
+ * @see <a href="https://github.com/apache/logging-log4j-tools/issues/117">apache/logging-log4j-tools#117</a>
+ */
+@Plugin
+@Namespace("namespace")
+public final class MyAppenderSubclassingAppender extends MyAppender {
+
+    /**
+     * The canonical constructor.
+     */
+    @Factory
+    public static MyAppenderSubclassingAppender newLayout(
+            final @PluginAttribute(value = "awesomenessEnabled", defaultBoolean = true) boolean awesomenessEnabled) {
+        return null;
+    }
+}