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

[fix] relax @CompileTimeConstant requirement on SafeArg key #119

Merged
merged 2 commits into from
Feb 22, 2019

Conversation

iamdanfox
Copy link
Contributor

Motivation

In 1.8.0 we started requiring SafeArg.of(key, value) to have a compile time constant key. The intention was to prevent people wrapping unknown key-value pairs as SafeArgs, which would make it harder to CR and spot potential data leaks.

Since merging this, we've had a lot of complaints - particularly because it is impossible to suppress the CompileTimeStatic check. This has caused problems for anyone who was dynamically constructing args, leading them to choose one of the following approaches:

  1. just stop taking upgrades
  2. add a line to versions.props to force safe-logging down to <1.8.0
  3. obfuscate their codebase with a subclassed Arg hack:
    /**
     * Like {@link SafeArg}, but works around strict {@link com.google.errorprone.annotations.CompileTimeConstant}
     * argument name restrictions.
     */
    private static final class DynamicSafeArg<T> extends Arg<T> {
        DynamicSafeArg(String name, @Nullable T value) {
            super(name, value);
        }
        @Override
        public boolean isSafeForLogging() {
            return true;
        }
    }

As far as we can tell, this annotation hasn't actually motivated people to replace dynamic usages with non-dynamic usages.

Proposal

Google's error-prone maintainers carefully weigh the cost-benefit of introducing a new check vs the false positives and friction it causes. In this case, I think the friction outweighs the potential upside.

I'd propose turning this into a regular error-prone check, as part of baseline. This would ensure that at the time new code is being written, people will get a warning if they dynamically construct SafeArgs and have to either rethink their approach or justify their suppression of this check.

@iamdanfox iamdanfox requested a review from a team as a code owner February 22, 2019 15:28
@bulldozer-bot bulldozer-bot bot merged commit 1e9839a into develop Feb 22, 2019
@bulldozer-bot bulldozer-bot bot deleted the dfox/relax-compile-time-constant branch February 22, 2019 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants