This is an automated email from the ASF dual-hosted git repository. vy pushed a commit to branch fix/docgen-plugin-subclass in repository https://gitbox.apache.org/repos/asf/logging-log4j-tools.git
commit e861a4fd8ab4187f08ad551dc19f66bd5c26a8b9 Author: Volkan Yazıcı <vol...@yazi.ci> AuthorDate: Tue May 7 10:34:25 2024 +0200 Fix handling of subclassed plugins in Log4j Docgen (#117) --- .../docgen/generator/internal/TypeLookup.java | 125 ++++++++++++++++++--- .../docgen/processor/DescriptorGenerator.java | 40 ++++--- src/changelog/.0.x.x/fix-docgen-duplicate-type.xml | 8 ++ 3 files changed, 143 insertions(+), 30 deletions(-) diff --git a/log4j-docgen/src/main/java/org/apache/logging/log4j/docgen/generator/internal/TypeLookup.java b/log4j-docgen/src/main/java/org/apache/logging/log4j/docgen/generator/internal/TypeLookup.java index d32bd00..f87ba02 100644 --- a/log4j-docgen/src/main/java/org/apache/logging/log4j/docgen/generator/internal/TypeLookup.java +++ b/log4j-docgen/src/main/java/org/apache/logging/log4j/docgen/generator/internal/TypeLookup.java @@ -16,15 +16,18 @@ */ package org.apache.logging.log4j.docgen.generator.internal; -import java.util.Iterator; -import java.util.Set; -import java.util.TreeMap; -import java.util.function.Predicate; 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; +import java.util.Iterator; +import java.util.Set; +import java.util.TreeMap; +import java.util.function.Predicate; + public final class TypeLookup extends TreeMap<String, ArtifactSourcedType> { private static final long serialVersionUID = 1L; @@ -42,18 +45,108 @@ public final class TypeLookup extends TreeMap<String, ArtifactSourcedType> { 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); + }); + } + + @SuppressWarnings("StatementWithEmptyBody") + private void mergeScalarTypes(PluginSet pluginSet) { + pluginSet.getScalars().forEach(newType -> { + + final String className = newType.getClassName(); + @Nullable final ArtifactSourcedType oldSourcedType = get(className); + + // If the entry doesn't exist yet, add it + if (oldSourcedType == null) { + final ArtifactSourcedType newSourcedType = ArtifactSourcedType.ofPluginSet(pluginSet, newType); + put(className, 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. + else if (oldSourcedType.type instanceof ScalarType) {} + + // If the entry already exists, but with an unexpected type, fail + else { + conflictingTypeFailure(className, oldSourcedType.type, newType); + } + }); + } + + private static void conflictingTypeFailure(final String className, final Type oldType, final Type newType) { + final String message = String.format( + "`%s` class occurs multiple times with conflicting types: `%s` and `%s`", + className, + oldType.getClass().getSimpleName(), + newType.getClass().getSimpleName()); + throw new IllegalArgumentException(message); + } + + private void mergeAbstractTypes(PluginSet pluginSet) { + pluginSet.getAbstractTypes().forEach(newType -> { + + final String className = newType.getClassName(); + @Nullable final ArtifactSourcedType oldSourcedType = get(className); + + // If the entry doesn't exist yet, add it + if (oldSourcedType == null) { + final ArtifactSourcedType newSourcedType = ArtifactSourcedType.ofPluginSet(pluginSet, newType); + put(className, newSourcedType); + } + + // If the entry already exists and is of expected type, extend it + else if (oldSourcedType.type instanceof AbstractType) { + final AbstractType oldType = (AbstractType) oldSourcedType.type; + newType.getImplementations().forEach(oldType::addImplementation); + } + + // If the entry already exists, but with an unexpected type, fail + else { + conflictingTypeFailure(className, oldSourcedType.type, newType); + } + }); + } + + private void mergePluginTypes(PluginSet pluginSet) { + pluginSet.getPlugins().forEach(newType -> { + + final String className = newType.getClassName(); + @Nullable final ArtifactSourcedType oldSourcedType = get(className); + + // If the entry doesn't exist yet, add it + if (oldSourcedType == null) { + final ArtifactSourcedType newSourcedType = ArtifactSourcedType.ofPluginSet(pluginSet, newType); + put(className, 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. + else if (oldSourcedType.type instanceof AbstractType && !(oldSourcedType.type instanceof PluginType)) { + final ArtifactSourcedType newSourcedType = ArtifactSourcedType.ofPluginSet(pluginSet, newType); + put(className, newSourcedType); + // Preserve old implementations + final AbstractType oldType = (AbstractType) oldSourcedType.type; + oldType.getImplementations().forEach(newType::addImplementation); + } + + // If the entry already exists and is of expected type, extend it + else if (oldSourcedType.type instanceof PluginType) { + final PluginType oldType = (PluginType) oldSourcedType.type; + newType.getImplementations().forEach(oldType::addImplementation); + } + + // If the entry already exists, but with an unexpected type, fail + else { + conflictingTypeFailure(className, oldSourcedType.type, newType); + } }); } diff --git a/log4j-docgen/src/main/java/org/apache/logging/log4j/docgen/processor/DescriptorGenerator.java b/log4j-docgen/src/main/java/org/apache/logging/log4j/docgen/processor/DescriptorGenerator.java index eb9b82a..cd82d7b 100644 --- a/log4j-docgen/src/main/java/org/apache/logging/log4j/docgen/processor/DescriptorGenerator.java +++ b/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 class DescriptorGenerator extends AbstractProcessor { @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 class DescriptorGenerator extends AbstractProcessor { 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()); diff --git a/src/changelog/.0.x.x/fix-docgen-duplicate-type.xml b/src/changelog/.0.x.x/fix-docgen-duplicate-type.xml new file mode 100644 index 0000000..f5a925b --- /dev/null +++ b/src/changelog/.0.x.x/fix-docgen-duplicate-type.xml @@ -0,0 +1,8 @@ +<?xml version="1.0" encoding="UTF-8"?> +<entry xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" + xmlns="https://logging.apache.org/xml/ns" + xsi:schemaLocation="https://logging.apache.org/xml/ns https://logging.apache.org/xml/ns/log4j-changelog-0.xsd" + type="fixed"> + <issue id="117" link="https://github.com/apache/logging-log4j-tools/issues/117"/> + <description format="asciidoc">Fix handling of subclassed plugins in Log4j Docgen</description> +</entry>