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

InvocationHandlerDelegation false positive? #1498

Closed
iamdanfox opened this issue Sep 21, 2020 · 3 comments · Fixed by #1499
Closed

InvocationHandlerDelegation false positive? #1498

iamdanfox opened this issue Sep 21, 2020 · 3 comments · Fixed by #1499

Comments

@iamdanfox
Copy link
Contributor

What happened?

With baseline 3.23.0, the following code trips over

protected Object handleInvocation(Object _proxy, Method method, Object[] args) throws Throwable {
            try {
                Object ret = method.invoke(delegate, args);
                log(AuditResult.SUCCESS);
                return ret;
            } catch (InvocationTargetException | UndeclaredThrowableException e) {
                log(AuditUtil.getResultForThrowable(e.getCause()));
                throw e.getCause();
            } catch (Throwable e) {
                log(AuditUtil.getResultForThrowable(e));
                throw e;
            } finally {
                context.clear();
            }
        }

Error produced:

> Task :audit-api:compileJava
/Volumes/git/foundry-audit/audit-api/src/main/java/com/palantir/foundry/audit/AuditLoggingProxy.java:41: warning: [InvocationHandlerDelegation] InvocationHandlers which delegate to another object must catch and unwrap InvocationTargetException, otherwise an UndeclaredThrowableException will be thrown each time the delegate throws an exception.
                Object ret = method.invoke(delegate, args);
                                          ^
  This check is intended to be advisory. It's fine to @SuppressWarnings("InvocationHandlerDelegation") in certain cases, but is usually not recommended.
    (see https://github.com/palantir/gradle-baseline#baseline-error-prone-checks)

What did you want to happen?

I think this code is fine and it's just a straight-up false positive?? (possibly multi-branch catch block?)

@carterkozak
Copy link
Contributor

Why are we unwrapping UndeclaredThrowableException?

@iamdanfox
Copy link
Contributor Author

I guess people do sometimes use this auditing library to audit a big old chain of proxies wrapping proxies (e.g. our SLOs stuff)?

@carterkozak
Copy link
Contributor

Even so, we wouldn't want to throw UndeclaredThrowableException.getCause even if only the cause is audited.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants