feature: add LogsafeArgName errorprone rule #1459
Conversation
| return buildDescription(tree) | ||
| .setMessage("Arguments with name '" + argName + "' must be marked as unsafe.") | ||
| .addFix(SuggestedFix.builder() | ||
| .replace(tree.getMethodSelect(), "UnsafeArg.of") |
There was a problem hiding this comment.
SuggestedFixes.qualifyType on "com.palantir.logsafe.UnsafeArg" will add the import automagically, or fully qualify the type if there happens to be a foo.bar.UnsafeArg already imported. We can append ".of" to the result.
|
|
||
| public final class LogsafeArgNameTest { | ||
| private CompilationTestHelper compilationHelper; | ||
| private RefactoringValidator refactoringHelper; |
There was a problem hiding this comment.
Slight preference to use factory methods for these, that way we can fluently build the test from the factory.
I recall some of the preconditions tests attempting multiple asserts on a shared helper and getting unexpected results because these guys are stateful!
|
I'd like to merge the associated internal PR first so that excavator will correctly apply the fix on baseline upgrades |
carterkozak
left a comment
carterkozak
left a comment
There was a problem hiding this comment.
lgtm, merge when you're ready.
|
Released 3.36.0 |
Before this PR
Occasionally developers would incorrectly tag a known unsafe parameter (ex. branch, username) as safe.
After this PR
==COMMIT_MSG==
add LogsafeArgName errorprone rule which allows users to specify a list of argument names that must always be tagged as unsafe.
==COMMIT_MSG==
This isn't a perfect solution since developers may use a different name for an unsafe value, but this should help catch some cases.
The expectation is that we will configure the set of unsafe arg names internally.
Possible downsides?
N/A