From e861a4fd8ab4187f08ad551dc19f66bd5c26a8b9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Volkan=20Yaz=C4=B1c=C4=B1?= Date: Tue, 7 May 2024 10:34:25 +0200 Subject: [PATCH 1/6] Fix handling of subclassed plugins in Log4j Docgen (#117) --- .../docgen/generator/internal/TypeLookup.java | 125 +++++++++++++++--- .../docgen/processor/DescriptorGenerator.java | 40 ++++-- .../.0.x.x/fix-docgen-duplicate-type.xml | 8 ++ 3 files changed, 143 insertions(+), 30 deletions(-) create mode 100644 src/changelog/.0.x.x/fix-docgen-duplicate-type.xml 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 d32bd00c..f87ba025 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 { private static final long serialVersionUID = 1L; @@ -42,18 +45,108 @@ private TypeLookup(final Iterable pluginSets, final Predica private void mergeDescriptors(Iterable 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 eb9b82a6..cd82d7bf 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 abstractTypesToDocument = new HashSet<>(); + private final Set pluginTypesToDocument = new HashSet<>(); - // Scalar types to process - private final Collection scalarTypesToDocument = new HashSet<>(); + private final Set abstractTypesToDocument = new HashSet<>(); + + private final Set scalarTypesToDocument = new HashSet<>(); private Predicate classNameFilter; @@ -253,7 +253,8 @@ public SourceVersion getSupportedSourceVersion() { @Override public boolean process(final Set 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 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()); 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 00000000..f5a925ba --- /dev/null +++ b/src/changelog/.0.x.x/fix-docgen-duplicate-type.xml @@ -0,0 +1,8 @@ + + + + Fix handling of subclassed plugins in Log4j Docgen + From 6d3610faab63a34e20a3bf97cfb1c2a2430eca83 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Volkan=20Yaz=C4=B1c=C4=B1?= Date: Tue, 7 May 2024 10:39:48 +0200 Subject: [PATCH 2/6] Fix Spotless failures --- .../docgen/generator/internal/TypeLookup.java | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 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 f87ba025..098ed225 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,6 +16,10 @@ */ 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; @@ -23,11 +27,6 @@ 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 { private static final long serialVersionUID = 1L; @@ -54,7 +53,6 @@ private void mergeDescriptors(Iterable pluginSets) { @SuppressWarnings("StatementWithEmptyBody") private void mergeScalarTypes(PluginSet pluginSet) { pluginSet.getScalars().forEach(newType -> { - final String className = newType.getClassName(); @Nullable final ArtifactSourcedType oldSourcedType = get(className); @@ -67,7 +65,8 @@ private void mergeScalarTypes(PluginSet pluginSet) { // 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) {} + else if (oldSourcedType.type instanceof ScalarType) { + } // If the entry already exists, but with an unexpected type, fail else { @@ -87,7 +86,6 @@ private static void conflictingTypeFailure(final String className, final Type ol private void mergeAbstractTypes(PluginSet pluginSet) { pluginSet.getAbstractTypes().forEach(newType -> { - final String className = newType.getClassName(); @Nullable final ArtifactSourcedType oldSourcedType = get(className); @@ -112,7 +110,6 @@ else if (oldSourcedType.type instanceof AbstractType) { private void mergePluginTypes(PluginSet pluginSet) { pluginSet.getPlugins().forEach(newType -> { - final String className = newType.getClassName(); @Nullable final ArtifactSourcedType oldSourcedType = get(className); From cd401aa515892db012a13ef10fffd4f8762bd146 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Volkan=20Yaz=C4=B1c=C4=B1?= Date: Tue, 7 May 2024 13:34:58 +0200 Subject: [PATCH 3/6] Fix failing tests --- ...ng.log4j.core.appender.SocketAppender.adoc | 127 +++++++++++++++++- ...ogging.log4j.core.config.LoggerConfig.adoc | 75 ++++++++++- ...gging.log4j.core.config.LoggersPlugin.adoc | 2 +- ...e.logging.log4j.core.filter.MapFilter.adoc | 53 +++++++- ...e.logging.log4j.core.lookup.MapLookup.adoc | 8 +- ...ore.pattern.ThrowablePatternConverter.adoc | 8 +- 6 files changed, 267 insertions(+), 6 deletions(-) diff --git a/log4j-docgen/src/test/resources/DocumentationGeneratorTest/complex/docs/log4j-core/org.apache.logging.log4j.core.appender.SocketAppender.adoc b/log4j-docgen/src/test/resources/DocumentationGeneratorTest/complex/docs/log4j-core/org.apache.logging.log4j.core.appender.SocketAppender.adoc index 5268f96b..a68b7c94 100644 --- a/log4j-docgen/src/test/resources/DocumentationGeneratorTest/complex/docs/log4j-core/org.apache.logging.log4j.core.appender.SocketAppender.adoc +++ b/log4j-docgen/src/test/resources/DocumentationGeneratorTest/complex/docs/log4j-core/org.apache.logging.log4j.core.appender.SocketAppender.adoc @@ -16,7 +16,7 @@ limitations under the License. //// [#org_apache_logging_log4j_core_appender_SocketAppender] -= `org.apache.logging.log4j.core.appender.SocketAppender` += Socket Class:: `org.apache.logging.log4j.core.appender.SocketAppender` Provider:: `org.apache.logging.log4j:log4j-core` @@ -26,6 +26,131 @@ An Appender that delivers events over socket connections. Supports both TCP and UDP. +[#org_apache_logging_log4j_core_appender_SocketAppender-XML-snippet] +== XML snippet +[source, xml] +---- + + + + + + + +---- + +[#org_apache_logging_log4j_core_appender_SocketAppender-attributes] +== Attributes + +Optional attributes are denoted by `?`-suffixed types. + +[cols="1m,1m,1m,5"] +|=== +|Name|Type|Default|Description + +|advertise +|boolean? +| +a| + +|bufferedIo +|boolean? +| +a| + +|bufferSize +|int? +| +a| + +|connectTimeoutMillis +|int? +| +a| + +|host +|String? +| +a| + +|ignoreExceptions +|boolean? +| +a| + +|immediateFail +|boolean? +| +a| + +|immediateFlush +|boolean? +| +a| + +|name +|String +| +a| + +|port +|int? +| +a| + +|protocol +|xref:../log4j-core/org.apache.logging.log4j.core.net.Protocol.adoc[Protocol]? +| +a| + +|reconnectDelayMillis +|int? +| +a| + +|=== + +[#org_apache_logging_log4j_core_appender_SocketAppender-components] +== Nested components + +Optional components are denoted by `?`-suffixed types. + +[cols="1m,1m,5"] +|=== +|Tag|Type|Description + +|property +|xref:../log4j-core/org.apache.logging.log4j.core.config.Property.adoc[Property]? +a| + +| +|xref:../log4j-core/org.apache.logging.log4j.core.Filter.adoc[Filter]? +a| + +| +|xref:../log4j-core/org.apache.logging.log4j.core.Layout.adoc[Layout]? +a| + +|SocketOptions +|xref:../log4j-core/org.apache.logging.log4j.core.net.SocketOptions.adoc[SocketOptions]? +a| + +|Ssl +|xref:../log4j-core/org.apache.logging.log4j.core.net.ssl.SslConfiguration.adoc[SslConfiguration]? +a| + +|=== [#org_apache_logging_log4j_core_appender_SocketAppender-implementations] == Known implementations diff --git a/log4j-docgen/src/test/resources/DocumentationGeneratorTest/complex/docs/log4j-core/org.apache.logging.log4j.core.config.LoggerConfig.adoc b/log4j-docgen/src/test/resources/DocumentationGeneratorTest/complex/docs/log4j-core/org.apache.logging.log4j.core.config.LoggerConfig.adoc index b47395ed..4fb693ea 100644 --- a/log4j-docgen/src/test/resources/DocumentationGeneratorTest/complex/docs/log4j-core/org.apache.logging.log4j.core.config.LoggerConfig.adoc +++ b/log4j-docgen/src/test/resources/DocumentationGeneratorTest/complex/docs/log4j-core/org.apache.logging.log4j.core.config.LoggerConfig.adoc @@ -16,7 +16,7 @@ limitations under the License. //// [#org_apache_logging_log4j_core_config_LoggerConfig] -= `org.apache.logging.log4j.core.config.LoggerConfig` += logger Class:: `org.apache.logging.log4j.core.config.LoggerConfig` Provider:: `org.apache.logging.log4j:log4j-core` @@ -24,6 +24,79 @@ Provider:: `org.apache.logging.log4j:log4j-core` Logger object that is created via configuration. +[#org_apache_logging_log4j_core_config_LoggerConfig-XML-snippet] +== XML snippet +[source, xml] +---- + + + + + +---- + +[#org_apache_logging_log4j_core_config_LoggerConfig-attributes] +== Attributes + +Optional attributes are denoted by `?`-suffixed types. + +[cols="1m,1m,1m,5"] +|=== +|Name|Type|Default|Description + +|additivity +|Boolean? +| +a| + +|includeLocation +|String? +| +a| + +|level +|xref:../log4j-core/org.apache.logging.log4j.Level.adoc[Level]? +| +a| + +|levelAndRefs +|String? +| +a| + +|name +|String +| +a| + +|=== + +[#org_apache_logging_log4j_core_config_LoggerConfig-components] +== Nested components + +Optional components are denoted by `?`-suffixed types. + +[cols="1m,1m,5"] +|=== +|Tag|Type|Description + +|AppenderRef +|xref:../log4j-core/org.apache.logging.log4j.core.config.AppenderRef.adoc[AppenderRef]? +a| + +|property +|xref:../log4j-core/org.apache.logging.log4j.core.config.Property.adoc[Property]? +a| + +| +|xref:../log4j-core/org.apache.logging.log4j.core.Filter.adoc[Filter]? +a| + +|=== [#org_apache_logging_log4j_core_config_LoggerConfig-implementations] == Known implementations diff --git a/log4j-docgen/src/test/resources/DocumentationGeneratorTest/complex/docs/log4j-core/org.apache.logging.log4j.core.config.LoggersPlugin.adoc b/log4j-docgen/src/test/resources/DocumentationGeneratorTest/complex/docs/log4j-core/org.apache.logging.log4j.core.config.LoggersPlugin.adoc index 6ad8a91d..23d2fbc2 100644 --- a/log4j-docgen/src/test/resources/DocumentationGeneratorTest/complex/docs/log4j-core/org.apache.logging.log4j.core.config.LoggersPlugin.adoc +++ b/log4j-docgen/src/test/resources/DocumentationGeneratorTest/complex/docs/log4j-core/org.apache.logging.log4j.core.config.LoggersPlugin.adoc @@ -40,7 +40,7 @@ Optional components are denoted by `?`-suffixed types. |=== |Tag|Type|Description -| +|logger |xref:../log4j-core/org.apache.logging.log4j.core.config.LoggerConfig.adoc[LoggerConfig]? a|An array of Loggers. diff --git a/log4j-docgen/src/test/resources/DocumentationGeneratorTest/complex/docs/log4j-core/org.apache.logging.log4j.core.filter.MapFilter.adoc b/log4j-docgen/src/test/resources/DocumentationGeneratorTest/complex/docs/log4j-core/org.apache.logging.log4j.core.filter.MapFilter.adoc index 7f1aaa48..a0ae030e 100644 --- a/log4j-docgen/src/test/resources/DocumentationGeneratorTest/complex/docs/log4j-core/org.apache.logging.log4j.core.filter.MapFilter.adoc +++ b/log4j-docgen/src/test/resources/DocumentationGeneratorTest/complex/docs/log4j-core/org.apache.logging.log4j.core.filter.MapFilter.adoc @@ -16,7 +16,7 @@ limitations under the License. //// [#org_apache_logging_log4j_core_filter_MapFilter] -= `org.apache.logging.log4j.core.filter.MapFilter` += MapFilter Class:: `org.apache.logging.log4j.core.filter.MapFilter` Provider:: `org.apache.logging.log4j:log4j-core` @@ -24,6 +24,57 @@ Provider:: `org.apache.logging.log4j:log4j-core` A Filter that operates on a Map. +[#org_apache_logging_log4j_core_filter_MapFilter-XML-snippet] +== XML snippet +[source, xml] +---- + + + +---- + +[#org_apache_logging_log4j_core_filter_MapFilter-attributes] +== Attributes + +Optional attributes are denoted by `?`-suffixed types. + +[cols="1m,1m,1m,5"] +|=== +|Name|Type|Default|Description + +|onMatch +|xref:../log4j-core/org.apache.logging.log4j.core.Filter.Result.adoc[Result]? +| +a| + +|onMismatch +|xref:../log4j-core/org.apache.logging.log4j.core.Filter.Result.adoc[Result]? +| +a| + +|operator +|String? +| +a| + +|=== + +[#org_apache_logging_log4j_core_filter_MapFilter-components] +== Nested components + +Optional components are denoted by `?`-suffixed types. + +[cols="1m,1m,5"] +|=== +|Tag|Type|Description + +|KeyValuePair +|xref:../log4j-core/org.apache.logging.log4j.core.util.KeyValuePair.adoc[KeyValuePair]? +a| + +|=== [#org_apache_logging_log4j_core_filter_MapFilter-implementations] == Known implementations diff --git a/log4j-docgen/src/test/resources/DocumentationGeneratorTest/complex/docs/log4j-core/org.apache.logging.log4j.core.lookup.MapLookup.adoc b/log4j-docgen/src/test/resources/DocumentationGeneratorTest/complex/docs/log4j-core/org.apache.logging.log4j.core.lookup.MapLookup.adoc index 70511a52..ab1ebb44 100644 --- a/log4j-docgen/src/test/resources/DocumentationGeneratorTest/complex/docs/log4j-core/org.apache.logging.log4j.core.lookup.MapLookup.adoc +++ b/log4j-docgen/src/test/resources/DocumentationGeneratorTest/complex/docs/log4j-core/org.apache.logging.log4j.core.lookup.MapLookup.adoc @@ -16,7 +16,7 @@ limitations under the License. //// [#org_apache_logging_log4j_core_lookup_MapLookup] -= `org.apache.logging.log4j.core.lookup.MapLookup` += map Class:: `org.apache.logging.log4j.core.lookup.MapLookup` Provider:: `org.apache.logging.log4j:log4j-core` @@ -24,6 +24,12 @@ Provider:: `org.apache.logging.log4j:log4j-core` A map-based lookup. +[#org_apache_logging_log4j_core_lookup_MapLookup-XML-snippet] +== XML snippet +[source, xml] +---- + +---- [#org_apache_logging_log4j_core_lookup_MapLookup-implementations] == Known implementations diff --git a/log4j-docgen/src/test/resources/DocumentationGeneratorTest/complex/docs/log4j-core/org.apache.logging.log4j.core.pattern.ThrowablePatternConverter.adoc b/log4j-docgen/src/test/resources/DocumentationGeneratorTest/complex/docs/log4j-core/org.apache.logging.log4j.core.pattern.ThrowablePatternConverter.adoc index b67917ed..7bee906b 100644 --- a/log4j-docgen/src/test/resources/DocumentationGeneratorTest/complex/docs/log4j-core/org.apache.logging.log4j.core.pattern.ThrowablePatternConverter.adoc +++ b/log4j-docgen/src/test/resources/DocumentationGeneratorTest/complex/docs/log4j-core/org.apache.logging.log4j.core.pattern.ThrowablePatternConverter.adoc @@ -16,7 +16,7 @@ limitations under the License. //// [#org_apache_logging_log4j_core_pattern_ThrowablePatternConverter] -= `org.apache.logging.log4j.core.pattern.ThrowablePatternConverter` += ThrowablePatternConverter Class:: `org.apache.logging.log4j.core.pattern.ThrowablePatternConverter` Provider:: `org.apache.logging.log4j:log4j-core` @@ -24,6 +24,12 @@ Provider:: `org.apache.logging.log4j:log4j-core` Outputs the Throwable portion of the LoggingEvent as a full stack trace unless this converter's option is 'short', where it just outputs the first line of the trace, or if the number of lines to print is explicitly specified. +[#org_apache_logging_log4j_core_pattern_ThrowablePatternConverter-XML-snippet] +== XML snippet +[source, xml] +---- + +---- [#org_apache_logging_log4j_core_pattern_ThrowablePatternConverter-implementations] == Known implementations From 17fee979e7792312ecc64f9219d6ecadd961d186 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Volkan=20Yaz=C4=B1c=C4=B1?= Date: Tue, 7 May 2024 13:52:31 +0200 Subject: [PATCH 4/6] Add tests cases covering plugins subclassing plugins --- .../expected-plugins.xml | 13 ++++++ .../java-of-log4j2/MyAppender.java | 2 +- .../MyAppenderSubclassingAppender.java | 42 ++++++++++++++++++ .../java-of-log4j3/MyAppender.java | 2 +- .../MyAppenderSubclassingAppender.java | 43 +++++++++++++++++++ 5 files changed, 100 insertions(+), 2 deletions(-) create mode 100644 log4j-docgen/src/test/resources/DescriptorGeneratorTest/java-of-log4j2/MyAppenderSubclassingAppender.java create mode 100644 log4j-docgen/src/test/resources/DescriptorGeneratorTest/java-of-log4j3/MyAppenderSubclassingAppender.java diff --git a/log4j-docgen/src/test/resources/DescriptorGeneratorTest/expected-plugins.xml b/log4j-docgen/src/test/resources/DescriptorGeneratorTest/expected-plugins.xml index 3cdd37b2..39f6806c 100644 --- a/log4j-docgen/src/test/resources/DescriptorGeneratorTest/expected-plugins.xml +++ b/log4j-docgen/src/test/resources/DescriptorGeneratorTest/expected-plugins.xml @@ -123,6 +123,19 @@ It also implements: * apiref:example.Appender[], * apiref:example.BaseAppender[] + + + example.AbstractAppender + example.Appender + example.BaseAppender + example.MyAppender + java.lang.Object + + + + + Example plugin to demonstrate the case where a plugin subclasses another plugin. + example.Layout diff --git a/log4j-docgen/src/test/resources/DescriptorGeneratorTest/java-of-log4j2/MyAppender.java b/log4j-docgen/src/test/resources/DescriptorGeneratorTest/java-of-log4j2/MyAppender.java index 493ff00c..37a47ef7 100644 --- a/log4j-docgen/src/test/resources/DescriptorGeneratorTest/java-of-log4j2/MyAppender.java +++ b/log4j-docgen/src/test/resources/DescriptorGeneratorTest/java-of-log4j2/MyAppender.java @@ -45,7 +45,7 @@ * */ @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 diff --git a/log4j-docgen/src/test/resources/DescriptorGeneratorTest/java-of-log4j2/MyAppenderSubclassingAppender.java b/log4j-docgen/src/test/resources/DescriptorGeneratorTest/java-of-log4j2/MyAppenderSubclassingAppender.java new file mode 100644 index 00000000..8bc13a62 --- /dev/null +++ b/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 apache/logging-log4j-tools#117 + */ +@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; + } +} diff --git a/log4j-docgen/src/test/resources/DescriptorGeneratorTest/java-of-log4j3/MyAppender.java b/log4j-docgen/src/test/resources/DescriptorGeneratorTest/java-of-log4j3/MyAppender.java index 1318ed20..30246206 100644 --- a/log4j-docgen/src/test/resources/DescriptorGeneratorTest/java-of-log4j3/MyAppender.java +++ b/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 diff --git a/log4j-docgen/src/test/resources/DescriptorGeneratorTest/java-of-log4j3/MyAppenderSubclassingAppender.java b/log4j-docgen/src/test/resources/DescriptorGeneratorTest/java-of-log4j3/MyAppenderSubclassingAppender.java new file mode 100644 index 00000000..9a7b5a57 --- /dev/null +++ b/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 apache/logging-log4j-tools#117 + */ +@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; + } +} From 2cfeeb64cccb7f60d018c4cf8a4056f437c850ba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Volkan=20Yaz=C4=B1c=C4=B1?= Date: Tue, 7 May 2024 14:38:42 +0200 Subject: [PATCH 5/6] Use `Map#merge(K,V,BiFunction)` --- .../docgen/generator/internal/TypeLookup.java | 136 +++++++++--------- 1 file changed, 68 insertions(+), 68 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 098ed225..1a592ecc 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 @@ -50,101 +50,101 @@ private void mergeDescriptors(Iterable pluginSets) { }); } - @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); - } + final ArtifactSourcedType newSourcedType = ArtifactSourcedType.ofPluginSet(pluginSet, newType); + merge(className, newSourcedType, TypeLookup::mergeScalarType); + }); + } - // 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) { - } + 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 { - conflictingTypeFailure(className, oldSourcedType.type, newType); - } - }); + // If the entry already exists, but with an unexpected type, fail + else { + throw conflictingTypeFailure(oldSourcedType.type, newSourcedType.type); + } } - private static void conflictingTypeFailure(final String className, final Type oldType, final Type newType) { + 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`", - className, + oldType.getClassName(), oldType.getClass().getSimpleName(), newType.getClass().getSimpleName()); - throw new IllegalArgumentException(message); + return new IllegalArgumentException(message); } private void mergeAbstractTypes(PluginSet pluginSet) { pluginSet.getAbstractTypes().forEach(newType -> { final String className = newType.getClassName(); - @Nullable final ArtifactSourcedType oldSourcedType = get(className); + final ArtifactSourcedType newSourcedType = ArtifactSourcedType.ofPluginSet(pluginSet, newType); + merge(className, newSourcedType, TypeLookup::mergeAbstractType); + }); + } - // If the entry doesn't exist yet, add it - if (oldSourcedType == null) { - final ArtifactSourcedType newSourcedType = ArtifactSourcedType.ofPluginSet(pluginSet, newType); - put(className, newSourcedType); - } + private static ArtifactSourcedType mergeAbstractType( + final ArtifactSourcedType oldSourcedType, final ArtifactSourcedType 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 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 { - conflictingTypeFailure(className, oldSourcedType.type, newType); - } - }); + // 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(); - @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); - } + final ArtifactSourcedType newSourcedType = ArtifactSourcedType.ofPluginSet(pluginSet, newType); + merge(className, newSourcedType, TypeLookup::mergePluginType); + }); + } - // 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); - } + 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; - newType.getImplementations().forEach(oldType::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; + 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 { - conflictingTypeFailure(className, oldSourcedType.type, newType); - } - }); + // If the entry already exists, but with an unexpected type, fail + else { + throw conflictingTypeFailure(oldSourcedType.type, newSourcedType.type); + } } private void populateTypeHierarchy(Iterable pluginSets) { From 2c05adeca358ce8be28e278ac467eec22b19964f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Volkan=20Yaz=C4=B1c=C4=B1?= Date: Tue, 7 May 2024 14:41:15 +0200 Subject: [PATCH 6/6] Improve changelog entry --- ...docgen-duplicate-type.xml => fix-docgen-plugin-subclass.xml} | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) rename src/changelog/.0.x.x/{fix-docgen-duplicate-type.xml => fix-docgen-plugin-subclass.xml} (82%) diff --git a/src/changelog/.0.x.x/fix-docgen-duplicate-type.xml b/src/changelog/.0.x.x/fix-docgen-plugin-subclass.xml similarity index 82% rename from src/changelog/.0.x.x/fix-docgen-duplicate-type.xml rename to src/changelog/.0.x.x/fix-docgen-plugin-subclass.xml index f5a925ba..5de37e22 100644 --- a/src/changelog/.0.x.x/fix-docgen-duplicate-type.xml +++ b/src/changelog/.0.x.x/fix-docgen-plugin-subclass.xml @@ -3,6 +3,6 @@ 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"> - + Fix handling of subclassed plugins in Log4j Docgen