FP squid:S1192 logging statements

java

(Alix Warnke) #1

Error observed
Violations when duplicating string literals in logging statements

Steps to reproduce

Add three logging statements, such as:

logger.warning("Failed to load service");
logger.warning("Failed to load service");
logger.warning("Failed to load service");

Previously reported here: https://stackoverflow.com/questions/44561234/scope-of-rule-string-literals-should-not-be-duplicated-squids1192 and here: https://groups.google.com/forum/#!topic/sonarqube/B1iUm_LJz60

I think this is a very serious false positive as the severity of the rule is very high (CRITICAL) whereas the statements in this ticket, I could argue, are not even issues (apart from increasing the standard code duplication metric).
The precondition for this rule is that there might be business logic attached to the string literal value.
If the string literal value only exists within the scope of a logging statement I don’t see the problem.


(Andrei Epure) #3

Hello Alix,

The description of rule 1192 specifies

Duplicated string literals make the process of refactoring error-prone, since you must be sure to update all occurrences.
On the other hand, constants can be referenced from many places, but only need to be updated in a single place.

In other words, this is a critical code-smell (maintainability), not a critical bug (reliability, business logic). Also, in my experience, logging statements can become quite important when investigating live-site incidents.

If you believe that string duplication in logging statements is not an issue in the context of your application, you can disable this rule and create a custom one (see our custom java rules guide) for your specific needs.

Thank you.
Andrei


(Alix Warnke) #4

Thank you for your comments, Andrei.

Looking beyond mere maintainability, I see that this rule does make sense to apply even for reliability/business logic as there might be logic associated to a particular string literal value.

Creating a custom rule is very much an option but I think that the problem is more general than that and would argue that maybe this rule needs to be split in two (one for code-smell, which I would perhaps disable, and one for reliability/business logic which does not complain for logging statements and that I would not disable)

Regards
Alix


(Michal Domagala) #5

Hello,
I also vote for splitting the rule in two.

String literals vs constants is trade-off between readability and maintainability. When you read a code, logged message as literal is more readable.

Let consider instruction log.info("Incoming message: {}", m) If I repeated it few times in my code sonar will detect duplication. I don’t want disable the rule because beyond logging context it is useful. I want keep my code readable: that version log.info(INCOMING_MESSAGE, m) looks terrible.

I will do:
log.info("Incoming message: {}", m);
log.info("Incoming message: {}.", m);
log.info("Incoming message : {}", m);

It means that effectively it was false positive: although logged message template was identical, it was accidental. I can modify each message without breaking the contract. Why? Because there is no contract. As there is no contract about message structure, there is no maintainability issue here.

Vox populi for change: https://stackoverflow.com/questions/49447377/sonarqube-custom-rule-string-literal-should-not-be-duplicated-ignored-in-conte

Conclusion:
s1192 is useful but in some context developer must choose between unreadable code or stupid String modification. It would be nice to configure somehow scope of the rule in Sonar

Regards
Mike


(Hilal Emeksiz) #6

Hi
@Andrei_Epure
How about sql statements our developers claim that applying this rule decreases code readability.which clues can you give me to convince developers who write sql statements in java code? I looked for owasp, however I coulnd’t find a clue from taht side.


(Andrei Epure) #7

Thank you, @Hilal_Emeksiz, for the message.

OWASP is about writing secure code, not maintainable code.

I recommend you this very good answer on codereview.stackexchange regarding exactly this problem. It suggests to use a library or a query builder to construct queries. In case this would not work for your particular use-case, our platform is extensible and:


(Hilal Emeksiz) #8

thank you @Andrei_Epure


(Joachim Lous) #9

Those workarounds work, but they remain workarounds and do not adress the root problem, which is a real false positive.

Some may prefer assembling SQL with code, but explicit SQL is still a valid practice, and frequently more maintainable than alternatives due to its clarity.

When I insert line breaks in my queries to increase readability and, yes, maintainability, this rule claims that the line break caused a drastic reduction in maintanability bacause they all, say use “select” . This is just patently false. Yes, I could replace "SELECT " with a constant, but there is no sane reason to do so, except to appease the tool.

If making the rule more context aware is too costly or error prone, then fine, but say so. Don’t just deny the problem exists and pretend this is the users’ fault. Rules cannot be allowed to be sacred.


(Andrei Epure) #10

Hi @jolous

I did not suggest it’s the users’ fault. In both my answers above, I mentioned that the context (logging or SQL statements) may make this rule noisy, case in which the rule can be disabled inside SonarQube and the users can create a separate rule. Our platform is extensible and the existing rule is open source ( StringLiteralDuplicatedCheck), so you can modify it according to your needs as a custom rule.

We currently do not have plans to add exceptions inside this rule because it would induce a high maintenance cost. Thank you for your understanding.