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.

Sign up for GitHub

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

Jump to bottom

LogSafePreconditionsMessageFormat disallows slf4j-style format characters #761

Merged
merged 2 commits into from
Aug 21, 2019
Merged

LogSafePreconditionsMessageFormat disallows slf4j-style format characters #761

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
880f66c
LogSafePreconditionsMessageFormat disallows slf4j-style format charac…
carterkozak Aug 20, 2019
06965d8
Add generated changelog entries
carterkozak Aug 20, 2019
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
21 changes: 15 additions & 6 deletions ...one/src/main/java/com/palantir/baseline/errorprone/LogSafePreconditionsMessageFormat.java
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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