Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement the logger-log4j module using log4j apis directly #756

Merged
merged 3 commits into from
Jul 25, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions .baseline/checkstyle/custom-suppressions.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
<?xml version="1.0"?>
<!DOCTYPE suppressions PUBLIC
"-//Checkstyle//DTD SuppressionFilter Configuration 1.2//EN"
"https://checkstyle.org/dtds/suppressions_1_2.dtd">
<!-- IMPORTANT ECLIPSE NOTE: If you change this file, you must restart Eclipse
for your changes to take effect in its Checkstyle integration. -->
<suppressions>
<!--
Logger facades delegate to logger implementations. Unfortunately we
cannot opt out of specific custom rules, otherwise we would only
opt out of 'BanLoggingImplementations'.
-->
<suppress files="[/\\]logger-.*Bridge\.java" checks="IllegalImport" />
Comment on lines +9 to +13
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with this suppression given this project is explicitly is dealing with loggers, but one workaround would be to use/generate fully qualified type names where used for the things we consider illegal so they aren't imported.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's an option, but it would impact readability of the generated code, so I opted against it.

</suppressions>
5 changes: 5 additions & 0 deletions changelog/@unreleased/pr-756.v2.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
type: improvement
improvement:
description: Implement the `logger-log4j` module using log4j apis directly
links:
- https://github.com/palantir/safe-logging/pull/756
3 changes: 3 additions & 0 deletions gradle.properties
Original file line number Diff line number Diff line change
@@ -1 +1,4 @@
org.gradle.parallel=true
# Avoid formatting copyrights, which are formatted in a
# slightly non-standard way due to javapoet codegen.
com.palantir.baseline-format.copyright=false
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will opt us out of unintentionally formatting generated classes that we check in

1 change: 1 addition & 0 deletions logger-generator/build.gradle
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
dependencies {
implementation 'org.slf4j:slf4j-api'
implementation 'org.apache.logging.log4j:log4j-api'
implementation project(':safe-logging')
implementation 'com.google.errorprone:error_prone_annotations'
implementation 'com.google.code.findbugs:jsr305'
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,14 +48,17 @@ public final class LoggerGenerator {
private static final ClassName LOGGER_NAME = ClassName.get("com.palantir.logsafe.logger", "SafeLogger");
private static final ClassName SLF4J_BRIDGE_NAME =
ClassName.get("com.palantir.logsafe.logger.slf4j", "Slf4jSafeLoggerBridge");

private static final ClassName LOG4J_BRIDGE_NAME =
ClassName.get("com.palantir.logsafe.logger.log4j", "Log4jSafeLoggerBridge");
private static final ClassName THROWABLE_TYPE = ClassName.get(Throwable.class);
private static final String THROWABLE_NAME = "throwable";
private static final TypeName ARG_TYPE =
ParameterizedTypeName.get(ClassName.get(Arg.class), WildcardTypeName.subtypeOf(Object.class));
private static final TypeName ARGS_LIST_TYPE =
ParameterizedTypeName.get(ClassName.get(List.class), WildcardTypeName.subtypeOf(ARG_TYPE));
private static final String DELEGATE = "delegate";
private static final String SLF4J_MARKER = "MARKER";
private static final String MARKER_FIELD = "MARKER";

public static void main(String[] _args) {
JavaFile bridge = generateLoggerBridge();
Expand All @@ -64,6 +67,8 @@ public static void main(String[] _args) {
Goethe.formatAndEmit(implementation, Paths.get("../logger/src/main/java"));
JavaFile slf4jBridge = generateSlf4jBridge();
Goethe.formatAndEmit(slf4jBridge, Paths.get("../logger-slf4j/src/main/java"));
JavaFile log4jBridge = generateLog4jBridge();
Goethe.formatAndEmit(log4jBridge, Paths.get("../logger-log4j/src/main/java"));
}

private static JavaFile generateLoggerImplementation() {
Expand Down Expand Up @@ -182,7 +187,7 @@ private static JavaFile generateSlf4jBridge() {
.build())
.addModifiers(Modifier.FINAL)
.addSuperinterface(BRIDGE_NAME)
.addField(FieldSpec.builder(ClassName.get(org.slf4j.Marker.class), SLF4J_MARKER)
.addField(FieldSpec.builder(ClassName.get(org.slf4j.Marker.class), MARKER_FIELD)
.addModifiers(Modifier.PRIVATE, Modifier.STATIC, Modifier.FINAL)
.initializer("$T.getMarker($S)", org.slf4j.MarkerFactory.class, Safe.class.getName())
.build())
Expand All @@ -205,7 +210,7 @@ private static JavaFile generateSlf4jBridge() {
boolean hasArgList = hasArgList(spec);
boolean hasThrowableArg = hasThrowableArg(spec);
CodeBlock args = Stream.concat(
Stream.of(CodeBlock.of("$N", SLF4J_MARKER)),
Stream.of(CodeBlock.of("$N", MARKER_FIELD)),
spec.parameters.stream().flatMap(param -> {
if (hasArgList && ARGS_LIST_TYPE.equals(param.type)) {
if (hasThrowableArg) {
Expand Down Expand Up @@ -267,6 +272,99 @@ private static boolean requiresSlf4jLevelCheck(MethodSpec spec) {
|| hasArgList(spec);
}

private static JavaFile generateLog4jBridge() {
TypeSpec.Builder loggerBuilder = TypeSpec.classBuilder(LOG4J_BRIDGE_NAME)
.addAnnotation(AnnotationSpec.builder(Generated.class)
.addMember("value", "$S", LoggerGenerator.class.getName())
.build())
.addModifiers(Modifier.FINAL)
.addSuperinterface(BRIDGE_NAME)
.addField(FieldSpec.builder(ClassName.get(org.apache.logging.log4j.Marker.class), MARKER_FIELD)
.addModifiers(Modifier.PRIVATE, Modifier.STATIC, Modifier.FINAL)
.initializer(
"$T.getMarker($S)", org.apache.logging.log4j.MarkerManager.class, Safe.class.getName())
.build())
.addField(FieldSpec.builder(ClassName.get(org.apache.logging.log4j.Logger.class), DELEGATE)
.addModifiers(Modifier.PRIVATE, Modifier.FINAL)
.build())
.addMethod(MethodSpec.constructorBuilder()
.addParameter(
ParameterSpec.builder(ClassName.get(org.apache.logging.log4j.Logger.class), DELEGATE)
.build())
.addStatement(
"this.$1N = $2T.requireNonNull($1N, $3S)",
DELEGATE,
Objects.class,
"Log4j Logger is required")
.build());
for (MethodSpec spec : createLoggerBridge().methodSpecs) {
MethodSpec.Builder methodBuilder = spec.toBuilder();
methodBuilder.modifiers.clear();
boolean isVoidMethod = ClassName.VOID.equals(spec.returnType);
boolean hasArgList = hasArgList(spec);
boolean hasThrowableArg = hasThrowableArg(spec);
CodeBlock args = Stream.concat(
Stream.of(CodeBlock.of("$N", MARKER_FIELD)),
spec.parameters.stream().flatMap(param -> {
if (hasArgList && ARGS_LIST_TYPE.equals(param.type)) {
if (hasThrowableArg) {
return Stream.of(CodeBlock.of("merge($N, $N)", param.name, THROWABLE_NAME));
} else {
return Stream.of(
CodeBlock.of("$N.toArray(new $T[0])", param.name, Object.class));
}
} else if (hasThrowableArg && hasArgList && THROWABLE_TYPE.equals(param.type)) {
return Stream.empty();
}
return Stream.of(CodeBlock.of("$N", param.name));
}))
.collect(CodeBlock.joining(", "));
methodBuilder.addAnnotation(Override.class).addModifiers(Modifier.PUBLIC);
boolean requiresLevelCheck = requiresLog4jLevelCheck(spec);
if (requiresLevelCheck) {
methodBuilder.beginControlFlow(
"if ($N())",
LoggerLevel.valueOf(spec.name.toUpperCase(Locale.ENGLISH))
.isEnabled());
}
methodBuilder.addStatement("$L$N.$N($L)", isVoidMethod ? "" : "return ", DELEGATE, spec.name, args);
if (requiresLevelCheck) {
methodBuilder.endControlFlow();
}
loggerBuilder.addMethod(methodBuilder.build());
}
return JavaFile.builder(
LOG4J_BRIDGE_NAME.packageName(),
loggerBuilder
.addMethod(MethodSpec.methodBuilder("merge")
.addModifiers(Modifier.PRIVATE, Modifier.STATIC)
.returns(ArrayTypeName.of(Object.class))
.addParameter(ParameterSpec.builder(ARGS_LIST_TYPE, "args")
.build())
.addParameter(ParameterSpec.builder(THROWABLE_TYPE, THROWABLE_NAME)
.build())
.addStatement(
"$1T $2N = $3N.toArray(new $4T[$3N.size() + 1])",
ArrayTypeName.of(Object.class),
"result",
"args",
Object.class)
.addStatement("$1N[$1N.length - 1] = $2N", "result", THROWABLE_NAME)
.addStatement("return $N", "result")
.build())
.build())
.skipJavaLangImports(true)
.addFileComment(copyright(2022))
.build();
}

private static boolean requiresLog4jLevelCheck(MethodSpec spec) {
// Always level check prior to delegating to a var-args method
return spec.parameters.size() > 11
// Level check prior to converting args list into an object-array
|| hasArgList(spec);
}

private static boolean hasArgList(MethodSpec spec) {
return spec.parameters.stream().anyMatch(param -> Objects.equals(param.type, ARGS_LIST_TYPE));
}
Expand Down
13 changes: 13 additions & 0 deletions logger-log4j/build.gradle
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
apply plugin: 'com.palantir.external-publish-jar'

dependencies {
api project(':logger-spi')
implementation 'com.google.errorprone:error_prone_annotations'
compileOnly 'org.apache.logging.log4j:log4j-api'
compileOnly 'com.google.code.findbugs:jsr305'
compileOnly 'com.google.auto.service:auto-service-annotations'
annotationProcessor 'com.google.auto.service:auto-service'
}

// No great way to use a block comment for copyright statements
tasks.spotlessJavaCheck.enabled = false
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
/*
* (c) Copyright 2022 Palantir Technologies Inc. All rights reserved.
*
* Licensed 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 com.palantir.logsafe.logger.log4j;

import com.google.auto.service.AutoService;
import com.palantir.logsafe.Safe;
import com.palantir.logsafe.logger.spi.SafeLoggerBridge;
import com.palantir.logsafe.logger.spi.SafeLoggerFactoryBridge;
import org.apache.logging.log4j.LogManager;

@AutoService(SafeLoggerFactoryBridge.class)
public final class Log4JSafeLoggerFactoryBridge implements SafeLoggerFactoryBridge {

private static final int DEFAULT_PRIORITY = 500;

private final int priority;

/** ServiceLoader requires a public no-arg constructor. */
public Log4JSafeLoggerFactoryBridge() {
// priority is relatively high when log4j-core is available on the classpath but low when it is not.
this.priority = isLog4jCoreAvailable() ? DEFAULT_PRIORITY : -DEFAULT_PRIORITY;
}

@Override
public int priority() {
return priority;
}

@Override
public SafeLoggerBridge get(@Safe Class<?> clazz) {
return new Log4jSafeLoggerBridge(LogManager.getLogger(clazz));
}

@Override
public SafeLoggerBridge get(@Safe String name) {
return new Log4jSafeLoggerBridge(LogManager.getLogger(name));
}

private static boolean isLog4jCoreAvailable() {
try {
Class.forName("org.apache.logging.log4j.core.LoggerContext");
return true;
} catch (Throwable ignored) {
return false;
}
}
}
Loading