java:S5778 should be false positive for trivial getters

Rules and Languages Report False-positive / False-negative...
1

Let’s say I got this test code:

		@Test
		void shouldNotMoveTreeNodeToParentAsSelf() {
			assertThatThrownBy(
					() -> service.moveTreeNodeToNewParent(childNode.getId(), childNode.getId())
			).isInstanceOf(MoveNodeException.class);
		}

There could be times when we are calling trivial getters, like here, because they are methods of one of: Data Transfer Object (DTO), Value Object (VO), Plain Old Java Object (POJO), or JavaBeans. For some, the only reason such getters exists is because the frameworks expect it (i.e., to allow easy serialization and deserialization by frameworks). Otherwise, for example, DTOs are often criticized for not being “purely” object-oriented because they typically violate key principles of Object-Oriented Programming (OOP), specifically encapsulation and behavior, but that’s a longer debate.

What I’m suggesting is that SonarQube should be able to ignore trivial getters that have no behavior, because I don’t think the guesswork argument mentioned in java:S5778 detects false runtime exception is applicable here.

3

Hi Krisztian,

We recognize that S5778 can be quite strict. That said, I don’t think we can implement the idea that you suggested. When performing analysis on a file, we lack access to the underlying implementation of dependencies (unless they are in the same file). Consequently, it is impossible to verify whether a getter is trivial or if it could potentially throw an exception. Even in the example provided, a reader (and the analyzer) cannot be entirely certain that getId() won’t throw an exception without inspecting its source code.

On a positive note, we recently worked on reducing false positives for this rule. The analyzer will now recognize common, non-throwing placeholders such as List.of(), Collections.emptyList(), and so on (SONARJAVA-4426). This should help eliminate many of the false positives generated by this rule.

Best,
Tomasz

4

Remember that before the Java 14+ Enhanced NPE Messages, we were discouraged from calling multiple functions on the same line? So we had to create a lot of temporary variables throughout the Java code base of a project, just so that the JVM can accurate report the source of NPEs in production logs. Even for DTOs or Entities like this:

employee.getPersonalDetails().getEmailAddress().toLowerCase()

(Quoted from Baeldung’s article.)

And this is not a Law of Demeter violation, because this class holds data in a structured way.

With S5778, I’m starting to think that we’re going to have the same issue: the tools limit us in what we can do. A tool should not hinder productivity but aid it. Plus this is test code, not production code: test code always has different priorities, like, I’m afraid, people won’t hesitate to suppress problematic rules on a bigger scale, like, globally.

If a SonarQube rule cannot identify issues accurately enough, then it is inevitably going to cause a significant amount of issues, like it did throughout my test class. As my case shows, even with the improvements you mentioned.

But given how challenging it is to accurately read byte code (I guess even SpotBugs makes false positives), I think we might need to start to think about solving the problem in a completely different way, like popularizing other ways of making developers aware of how much code they should put in a try block. (But this is not the job of the Sonar team, i know.) For example, when developers are taught about the try keyword, they should also be taught about good practices. (And of course they need to understand that modern libraries wrap try-catch blocks into functions like assertThrows or assertThatThrownBy.)

Regardless, I understand your answer, and that you cannot do much about it. At least now it is documented here so we can search for it. Then developers can decide for themselves how far they need to go suppress the false positives.