Skip to content

Commit

Permalink
LogSafePreconditionsMessageFormat disallows slf4j-style format charac…
Browse files Browse the repository at this point in the history 
…ters (#761)

LogSafePreconditionsMessageFormat disallows slf4j-style format characters
  • Loading branch information
@carterkozak @bulldozer-bot
carterkozak authored and bulldozer-bot[bot] committed Aug 21, 2019
1 parent 04b6cae commit fdf3b88
Show file tree
Hide file tree
Showing 4 changed files with 74 additions and 142 deletions.
21 changes: 15 additions & 6 deletions ...one/src/main/java/com/palantir/baseline/errorprone/LogSafePreconditionsMessageFormat.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
link = "https://github.com/palantir/gradle-baseline#baseline-error-prone-checks",
linkType = LinkType.CUSTOM,
severity = SeverityLevel.ERROR,
summary = "logsafe Preconditions.checkX() methods should not have print-f style formatting.")
summary = "logsafe Preconditions.checkX() methods should not have print-f or slf4j style formatting.")
public final class LogSafePreconditionsMessageFormat extends PreconditionsMessageFormat {

private static final long serialVersionUID = 1L;
Expand All @@ -50,11 +50,20 @@ public LogSafePreconditionsMessageFormat() {

@Override
protected Description matchMessageFormat(MethodInvocationTree tree, String message, VisitorState state) {
if (!message.contains("%s")) {
return Description.NO_MATCH;
if (message.contains("%s")) {
return buildDescription(tree)
.setMessage("Do not use printf-style formatting in logsafe Preconditions. "
+ "Logsafe exceptions provide a simple message and key-value pairs of arguments, "
+ "no interpolation is performed.")
.build();
}

return buildDescription(tree).setMessage(
"Do not use printf-style formatting in logsafe Preconditions.").build();
if (message.contains("{}")) {
return buildDescription(tree)
.setMessage("Do not use slf4j-style formatting in logsafe Preconditions. "
+ "Logsafe exceptions provide a simple message and key-value pairs of arguments, "
+ "no interpolation is performed.")
.build();
}
return Description.NO_MATCH;
}
}
63 changes: 50 additions & 13 deletions ...rc/test/java/com/palantir/baseline/errorprone/LogSafePreconditionsMessageFormatTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@

public final class LogSafePreconditionsMessageFormatTests extends PreconditionsTests {

private static final String DIAGNOSTIC = "Do not use printf-style formatting";
private static final String PRINTF_DIAGNOSTIC = "Do not use printf-style formatting";
private static final String SLF4J_DIAGNOSTIC = "Do not use slf4j-style formatting";

private CompilationTestHelper compilationHelper;

Expand All @@ -37,38 +38,74 @@ public CompilationTestHelper compilationHelper() {
}

@Test
public void testCheckArgument() {
failLogSafe(DIAGNOSTIC, "Preconditions.checkArgument(param != \"string\", \"message %s\","
public void testCheckArgument_printf() {
failLogSafe(PRINTF_DIAGNOSTIC, "Preconditions.checkArgument(param != \"string\", \"message %s\","
+ " UnsafeArg.of(\"long\", 123L));");
}

@Test
public void testCheckArgument_multipleArgs() {
failLogSafe(DIAGNOSTIC, "Preconditions.checkArgument(param != \"string\", \"message %s %s\","
public void testCheckArgument_multipleArgs_printf() {
failLogSafe(PRINTF_DIAGNOSTIC, "Preconditions.checkArgument(param != \"string\", \"message %s %s\","
+ " UnsafeArg.of(\"char1\", 'a'), UnsafeArg.of(\"char2\", 'b'));");
}

@Test
public void testCheckState() {
failLogSafe(DIAGNOSTIC, "Preconditions.checkState(param != \"string\", \"message %s\","
public void testCheckState_printf() {
failLogSafe(PRINTF_DIAGNOSTIC, "Preconditions.checkState(param != \"string\", \"message %s\","
+ " UnsafeArg.of(\"long\", 123L));");
}

@Test
public void testCheckState_multipleArgs() {
failLogSafe(DIAGNOSTIC, "Preconditions.checkState(param != \"string\", \"message %s %s\","
public void testCheckState_multipleArgs_printf() {
failLogSafe(PRINTF_DIAGNOSTIC, "Preconditions.checkState(param != \"string\", \"message %s %s\","
+ " UnsafeArg.of(\"char1\", 'a'), UnsafeArg.of(\"char2\", 'b'));");
}

@Test
public void testCheckNotNull() {
failLogSafe(DIAGNOSTIC, "Preconditions.checkNotNull(param, \"message %s\","
public void testCheckNotNull_printf() {
failLogSafe(PRINTF_DIAGNOSTIC, "Preconditions.checkNotNull(param, \"message %s\","
+ " UnsafeArg.of(\"long\", 123L));");
}

@Test
public void testCheckNotNull_multipleArgs() {
failLogSafe(DIAGNOSTIC, "Preconditions.checkNotNull(param, \"message %s %s\","
public void testCheckNotNull_multipleArgs_printf() {
failLogSafe(PRINTF_DIAGNOSTIC, "Preconditions.checkNotNull(param, \"message %s %s\","
+ " UnsafeArg.of(\"char1\", 'a'), UnsafeArg.of(\"char2\", 'b'));");
}

@Test
public void testCheckArgument_slf4j() {
failLogSafe(SLF4J_DIAGNOSTIC, "Preconditions.checkArgument(param != \"string\", \"message {}\","
+ " UnsafeArg.of(\"long\", 123L));");
}

@Test
public void testCheckArgument_multipleArgs_slf4j() {
failLogSafe(SLF4J_DIAGNOSTIC, "Preconditions.checkArgument(param != \"string\", \"message {} {}\","
+ " UnsafeArg.of(\"char1\", 'a'), UnsafeArg.of(\"char2\", 'b'));");
}

@Test
public void testCheckState_slf4j() {
failLogSafe(SLF4J_DIAGNOSTIC, "Preconditions.checkState(param != \"string\", \"message {}\","
+ " UnsafeArg.of(\"long\", 123L));");
}

@Test
public void testCheckState_multipleArgs_slf4j() {
failLogSafe(SLF4J_DIAGNOSTIC, "Preconditions.checkState(param != \"string\", \"message {} {}\","
+ " UnsafeArg.of(\"char1\", 'a'), UnsafeArg.of(\"char2\", 'b'));");
}

@Test
public void testCheckNotNull_slf4j() {
failLogSafe(SLF4J_DIAGNOSTIC, "Preconditions.checkNotNull(param, \"message {}\","
+ " UnsafeArg.of(\"long\", 123L));");
}

@Test
public void testCheckNotNull_multipleArgs_slf4j() {
failLogSafe(SLF4J_DIAGNOSTIC, "Preconditions.checkNotNull(param, \"message {} {}\","
+ " UnsafeArg.of(\"char1\", 'a'), UnsafeArg.of(\"char2\", 'b'));");
}
}
127 changes: 4 additions & 123 deletions baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/PreconditionsTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -156,137 +156,18 @@ public final void validLogSafe() throws Exception {
" void f(boolean bArg, int iArg, Object oArg) {",
" Preconditions.checkArgument(bArg);",
" Preconditions.checkArgument(bArg, \"message\");",
" Preconditions.checkArgument(bArg, \"message {}\", UnsafeArg.of(\"char\", 'a'));",
" Preconditions.checkArgument(bArg, \"message {}\", UnsafeArg.of(\"int\", 123));",
" Preconditions.checkArgument(bArg, \"message {}\", UnsafeArg.of(\"long\", 123L));",
" Preconditions.checkArgument(bArg, \"message {}\", UnsafeArg.of(\"string\", \"msg\"));",
" Preconditions.checkArgument(bArg, \"message {} {}\", UnsafeArg.of(\"char1\", 'a'),"
" Preconditions.checkArgument(bArg, \"message {char}\", UnsafeArg.of(\"char\", 'a'));",
" Preconditions.checkArgument(bArg, \"message {char1 {char2}\", UnsafeArg.of(\"char1\", 'a'),"
+ " UnsafeArg.of(\"char2\", 'b'));",
" Preconditions.checkArgument(bArg, \"message {} {}\", UnsafeArg.of(\"char\", 'a'),"
+ " UnsafeArg.of(\"int\", 123));",
" Preconditions.checkArgument(bArg, \"message {} {}\", UnsafeArg.of(\"char\", 'a'),"
+ " UnsafeArg.of(\"long\", 123L));",
" Preconditions.checkArgument(bArg, \"message {} {}\", UnsafeArg.of(\"char\", 'a'),"
+ " UnsafeArg.of(\"string\", \"msg\"));",
" Preconditions.checkArgument(bArg, \"message {} {}\", UnsafeArg.of(\"int\", 123),"
+ " UnsafeArg.of(\"char\", 'a'));",
" Preconditions.checkArgument(bArg, \"message {} {}\", UnsafeArg.of(\"int\", 123),"
+ " UnsafeArg.of(\"int\", 123));",
" Preconditions.checkArgument(bArg, \"message {} {}\", UnsafeArg.of(\"int\", 123),"
+ " UnsafeArg.of(\"long\", 123L));",
" Preconditions.checkArgument(bArg, \"message {} {}\", UnsafeArg.of(\"int\", 123),"
+ " UnsafeArg.of(\"string\", \"msg\"));",
" Preconditions.checkArgument(bArg, \"message {} {}\", UnsafeArg.of(\"long\", 123L),"
+ " UnsafeArg.of(\"char\", 'a'));",
" Preconditions.checkArgument(bArg, \"message {} {}\", UnsafeArg.of(\"long\", 123L),"
+ " UnsafeArg.of(\"int\", 123));",
" Preconditions.checkArgument(bArg, \"message {} {}\", UnsafeArg.of(\"long\", 123L),"
+ " UnsafeArg.of(\"long\", 123L));",
" Preconditions.checkArgument(bArg, \"message {} {}\", UnsafeArg.of(\"long\", 123L),"
+ " UnsafeArg.of(\"string\", \"msg\"));",
" Preconditions.checkArgument(bArg, \"message {} {}\", UnsafeArg.of(\"string\", \"msg\"),"
+ " UnsafeArg.of(\"char\", 'a'));",
" Preconditions.checkArgument(bArg, \"message {} {}\", UnsafeArg.of(\"string\", \"msg\"),"
+ " UnsafeArg.of(\"int\", 123));",
" Preconditions.checkArgument(bArg, \"message {} {}\", UnsafeArg.of(\"string\", \"msg\"),"
+ " UnsafeArg.of(\"long\", 123L));",
" Preconditions.checkArgument(bArg, \"message {} {}\", UnsafeArg.of(\"string1\", \"msg\"),"
+ " UnsafeArg.of(\"string2\", \"msg\"));",
" Preconditions.checkArgument(bArg, \"message {} {} {}\","
+ " UnsafeArg.of(\"string1\", \"msg\"), UnsafeArg.of(\"string2\", \"msg\"),"
+ " UnsafeArg.of(\"string3\", \"msg\"));",
" Preconditions.checkArgument(bArg, \"message {} {} {} {}\","
+ " UnsafeArg.of(\"string1\", \"msg\"), UnsafeArg.of(\"string2\", \"msg\"),"
+ " UnsafeArg.of(\"string3\", \"msg\"), UnsafeArg.of(\"string4\", \"msg\"));",
"",
" Preconditions.checkState(iArg > 0);",
" Preconditions.checkState(iArg > 0, \"message\");",
" Preconditions.checkState(iArg > 0, \"message {}\", UnsafeArg.of(\"char\", 'a'));",
" Preconditions.checkState(iArg > 0, \"message {}\", UnsafeArg.of(\"int\", 123));",
" Preconditions.checkState(iArg > 0, \"message {}\", UnsafeArg.of(\"long\", 123L));",
" Preconditions.checkState(iArg > 0, \"message {}\", UnsafeArg.of(\"string\", \"msg\"));",
" Preconditions.checkState(iArg > 0, \"message {} {}\", UnsafeArg.of(\"char1\", 'a'),"
" Preconditions.checkState(iArg > 0, \"message {char}\", UnsafeArg.of(\"char\", 'a'));",
" Preconditions.checkState(iArg > 0, \"message {char1} {char2}\", UnsafeArg.of(\"char1\", 'a'),"
+ " UnsafeArg.of(\"char2\", 'b'));",
" Preconditions.checkState(iArg > 0, \"message {} {}\", UnsafeArg.of(\"char\", 'a'),"
+ " UnsafeArg.of(\"int\", 123));",
" Preconditions.checkState(iArg > 0, \"message {} {}\", UnsafeArg.of(\"char\", 'a'),"
+ " UnsafeArg.of(\"long\", 123L));",
" Preconditions.checkState(iArg > 0, \"message {} {}\", UnsafeArg.of(\"char\", 'a'),"
+ " UnsafeArg.of(\"string\", \"msg\"));",
" Preconditions.checkState(iArg > 0, \"message {} {}\", UnsafeArg.of(\"int\", 123),"
+ " UnsafeArg.of(\"char\", 'a'));",
" Preconditions.checkState(iArg > 0, \"message {} {}\", UnsafeArg.of(\"int\", 123),"
+ " UnsafeArg.of(\"int\", 123));",
" Preconditions.checkState(iArg > 0, \"message {} {}\", UnsafeArg.of(\"int\", 123),"
+ " UnsafeArg.of(\"long\", 123L));",
" Preconditions.checkState(iArg > 0, \"message {} {}\", UnsafeArg.of(\"int\", 123),"
+ " UnsafeArg.of(\"string\", \"msg\"));",
" Preconditions.checkState(iArg > 0, \"message {} {}\", UnsafeArg.of(\"long\", 123L),"
+ " UnsafeArg.of(\"char\", 'a'));",
" Preconditions.checkState(iArg > 0, \"message {} {}\", UnsafeArg.of(\"long\", 123L),"
+ " UnsafeArg.of(\"int\", 123));",
" Preconditions.checkState(iArg > 0, \"message {} {}\", UnsafeArg.of(\"long\", 123L),"
+ " UnsafeArg.of(\"long\", 123L));",
" Preconditions.checkState(iArg > 0, \"message {} {}\", UnsafeArg.of(\"long\", 123L),"
+ " UnsafeArg.of(\"string\", \"msg\"));",
" Preconditions.checkState(iArg > 0, \"message {} {}\", UnsafeArg.of(\"string\", \"msg\"),"
+ " UnsafeArg.of(\"char\", 'a'));",
" Preconditions.checkState(iArg > 0, \"message {} {}\", UnsafeArg.of(\"string\", \"msg\"),"
+ " UnsafeArg.of(\"int\", 123));",
" Preconditions.checkState(iArg > 0, \"message {} {}\", UnsafeArg.of(\"string\", \"msg\"),"
+ " UnsafeArg.of(\"long\", 123L));",
" Preconditions.checkState(iArg > 0, \"message {} {}\", UnsafeArg.of(\"string1\", \"msg\"),"
+ " UnsafeArg.of(\"string2\", \"msg\"));",
" Preconditions.checkState(iArg > 0, \"message {} {} {}\","
+ " UnsafeArg.of(\"string1\", \"msg\"), UnsafeArg.of(\"string2\", \"msg\"),"
+ " UnsafeArg.of(\"string3\", \"msg\"));",
" Preconditions.checkState(iArg > 0, \"message {} {} {} {}\","
+ " UnsafeArg.of(\"string1\", \"msg\"), UnsafeArg.of(\"string2\", \"msg\"),"
+ " UnsafeArg.of(\"string3\", \"msg\"), UnsafeArg.of(\"string4\", \"msg\"));",
"",
" Preconditions.checkNotNull(oArg);",
" Preconditions.checkNotNull(oArg, \"message\");",
" Preconditions.checkNotNull(oArg, \"message {}\", UnsafeArg.of(\"char\", 'a'));",
" Preconditions.checkNotNull(oArg, \"message {}\", UnsafeArg.of(\"int\", 123));",
" Preconditions.checkNotNull(oArg, \"message {}\", UnsafeArg.of(\"long\", 123L));",
" Preconditions.checkNotNull(oArg, \"message {}\", UnsafeArg.of(\"string\", \"msg\"));",
" Preconditions.checkNotNull(oArg, \"message {} {}\", UnsafeArg.of(\"char1\", 'a'),"
+ " UnsafeArg.of(\"char2\", 'b'));",
" Preconditions.checkNotNull(oArg, \"message {} {}\", UnsafeArg.of(\"char\", 'a'),"
+ " UnsafeArg.of(\"int\", 123));",
" Preconditions.checkNotNull(oArg, \"message {} {}\", UnsafeArg.of(\"char\", 'a'),"
+ " UnsafeArg.of(\"long\", 123L));",
" Preconditions.checkNotNull(oArg, \"message {} {}\", UnsafeArg.of(\"char\", 'a'),"
+ " UnsafeArg.of(\"string\", \"msg\"));",
" Preconditions.checkNotNull(oArg, \"message {} {}\", UnsafeArg.of(\"int\", 123),"
+ " UnsafeArg.of(\"char\", 'a'));",
" Preconditions.checkNotNull(oArg, \"message {} {}\", UnsafeArg.of(\"int\", 123),"
+ " UnsafeArg.of(\"int\", 123));",
" Preconditions.checkNotNull(oArg, \"message {} {}\", UnsafeArg.of(\"int\", 123),"
+ " UnsafeArg.of(\"long\", 123L));",
" Preconditions.checkNotNull(oArg, \"message {} {}\", UnsafeArg.of(\"int\", 123),"
+ " UnsafeArg.of(\"string\", \"msg\"));",
" Preconditions.checkNotNull(oArg, \"message {} {}\", UnsafeArg.of(\"long\", 123L),"
+ " UnsafeArg.of(\"char\", 'a'));",
" Preconditions.checkNotNull(oArg, \"message {} {}\", UnsafeArg.of(\"long\", 123L),"
+ " UnsafeArg.of(\"int\", 123));",
" Preconditions.checkNotNull(oArg, \"message {} {}\", UnsafeArg.of(\"long\", 123L),"
+ " UnsafeArg.of(\"long\", 123L));",
" Preconditions.checkNotNull(oArg, \"message {} {}\", UnsafeArg.of(\"long\", 123L),"
+ " UnsafeArg.of(\"string\", \"msg\"));",
" Preconditions.checkNotNull(oArg, \"message {} {}\", UnsafeArg.of(\"string\", \"msg\"),"
+ " UnsafeArg.of(\"char\", 'a'));",
" Preconditions.checkNotNull(oArg, \"message {} {}\", UnsafeArg.of(\"string\", \"msg\"),"
+ " UnsafeArg.of(\"int\", 123));",
" Preconditions.checkNotNull(oArg, \"message {} {}\", UnsafeArg.of(\"string\", \"msg\"),"
+ " UnsafeArg.of(\"long\", 123L));",
" Preconditions.checkNotNull(oArg, \"message {} {}\", UnsafeArg.of(\"string1\", \"msg\"),"
+ " UnsafeArg.of(\"string2\", \"msg\"));",
" Preconditions.checkNotNull(oArg, \"message {} {} {}\", UnsafeArg.of(\"string1\", \"msg\"),"
+ " UnsafeArg.of(\"string2\", \"msg\"), UnsafeArg.of(\"string3\", \"msg\"));",
" Preconditions.checkNotNull(oArg, \"message {} {} {} {}\","
+ " UnsafeArg.of(\"string1\", \"msg\"), UnsafeArg.of(\"string2\", \"msg\"),"
+ " UnsafeArg.of(\"string3\", \"msg\"), UnsafeArg.of(\"string4\", \"msg\"));",
" }",
"}")
.doTest();
Expand Down
5 changes: 5 additions & 0 deletions changelog/@unreleased/pr-761.v2.yml
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
type: improvement
improvement:
description: LogSafePreconditionsMessageFormat disallows slf4j-style format characters
links:
- https://github.com/palantir/gradle-baseline/pull/761

0 comments on commit fdf3b88

Please sign in to comment.