SLF4J_MANUALLY_PROVIDED_MESSAGE should be able to fail even if only getMessage() and no cause

[vorburger]

We should have a new check here which detects this kind of non-sense I'm seeing suprisingly often:

LOG.error("bla bla bla {}", e.getMessage())

as well as e.getStackTrace() and e.getLocalizedMessage() (anything else?), because I cannot really think of any good reason why you would want those methods of Exception in log args (can you?).

If I find the time may be I'll contribute it in the coming days.

[vorburger]

Wait a second, SLF4J_MANUALLY_PROVIDED_MESSAGE already does this for e.getMessage() and e.getLocalizedMessage() ... all that is left to do here is to do the same for e.getStackTrace().

[vorburger]

Whoa... wait, I spent some time to dig into the code (UsingGet[Localized]Message, UsingGet[Localized]MessageTest and ManualMessageDetector), and found out that in #31 there was work to intentionally only flag this as an error:

LOG.error("bla bla bla {}", e.getMessage(), e)

but not this, as the README actually explains if I had RTFM:

LOG.error("bla bla bla {}", e.getMessage())

At least in our project, this is IMHO not really what we want to check for... I can't think of a reason we would only want the getMessage() and not the e; we do want to catch use of only getMessage() because we find it to be done by developers not understanding the SLF4j API, not ever intentionally used as in #31. Perhaps it could be made configurable?

So I've change this issue's title to clarify what this is now about, and create a completely separate new issue #70 re. getStackTrace().

[vorburger]

@KengoTODA can the Detectors be made configurable (how, example?), or would it be simpler to just have two separate detectors / bug patterns for enforcing the getMessage() with/without cause style?

[maggu2810]

Hm, AFAIK it is used in projects where the message of an exception contains some useful information and the code that catch the exception knows that only the message itself could be interesting but it does not make sense to log the whole stack trace etc.

[vorburger]

@maggu2810 yeah it seems like there are clearly different requirements in different projects. So how would you (and @KengoTODA) feel about it if I simply proposed an additional detector named e.g. SLF4J_MANUALLY_PROVIDED_MESSAGE_ALWAYS (note the _ALWAYS suffix... it would share and not copy/paste the code, of course) which would flag up ANY use of getMessage() even if there is no e? Because that is the case I would like to detect in our community (opendaylight.org; a server side thing, no UI, so if there a Throwable we typically want to always log that, never just its Message). Projects could include/exclude whichever of these two detectors is more suitable in their respective environment.

[maggu2810]

If it make sense for projects to forbid the calling of any member function for exception in the slf4j I don't care as long as it is optional.

But does your community forbid it for logging only or in general for the exception usage?

How do you want to detect:

final String msg = ex.getMessage();
logger.warn("Catched an error: {}", msg, ex);

Wouldn't it make more sense to use PMD and its AvoidPrintStackTrace rule and a similar one for your other functions you don't want to be called?

[vorburger]

I don't care as long as it is optional

do you happen to know how to make a Detector excluded by default?

forbid it for logging only or in general for the exception usage

our main concern is for corret logging only; generally saying "thou shall never use Throwable.getMessage()" probably doesn't make sense IMHO.

How do you want to detect:

I guess you cannot really, but if someone really wants to "cheat" any tool, he typically somehow always can anyway, of course?

use PMD and its AvoidPrintStackTrace rule and a similar one

Introducing another tool such as PMD is not that trivial in a large community... build time impact needs to be assessed, IDE support ideally needs to be guaranteed... we would rather get more out of FindBugs (SpotBugs) which, after years, we have now adopted, instead of adding more tools to the zoo... 😸

I am motivated to contribute a PR for this, after #72 is in.