Bean Validation should be enabled

java

(Jens Bannmann) #1

Hi all,

I would like to propose the following new rule.


Rule description & motivation
Bean Validation can be triggered programmatically on an object, but it will also run automatically for fields of objects being validated and for any method parameters. However, in both cases, the corresponding variable must be annotated with javax.validation.Valid. This can easily be forgotten.

This rule should create an issue for each

  • parameter of a public method (excluding constructors)
  • field with bean validation constraints
  • field in a class where other fields have bean validation constraints

that is not annotated javax.validation.Valid and where the type refers to a class with bean validation constraints.

This rule should support standard constraint annotations (such as @NotNull) as well as custom ones (annotations which have the javax.validation.Constraint annotation).

Impact to keep this code as it is
Not annotating the element with @Valid means bean validation will not be triggered, but readers may overlook this omission and assume the object will be validated.

Example Code

import javax.validation.Valid;
import javax.validation.constraints.NotNull;

public class User {
  @NotNull
  private String name;
}

public class Group {
  @NotNull
  private List<User> users; // noncompliant, User instances are not validated
}

public class Email {
  @Valid
  @NotNull
  private List<User> recipients; // compliant
}

public class MyService {
  public void login(User user) { // noncompliant, parameter is not validated
  }

  public void logout(@Valid User user) { // compliant
  }

  protected void logAction(User user) { // compliant
  }
}

References

Type
Code Smell

Tags
suspicious


Best regards


(Jens Bannmann) #2

Hi all!

I’m interested in implementing this rule inside SonarJava. Before I work on it and open a pull request, please state whether the rule as described above seems acceptable.

Best regards,
Jens


(Alexandre Gigleux) #4

Hello Jens,

As it is, I think the scope of the rule is too wide as it will apply to all public class and I believe there are lot of situations where you just need a bean and you don’t want all this validation layer.

What about limiting the rule to Spring @Controller , @Service , and @Repository classes?

Thanks


(Jens Bannmann) #5

Hi Alexandre!

In our projects, we could indeed enable that rule for the entire project as we only annotate our “data transfer object” classes which are not used in the lower layers. However, I agree that for other teams it may not be that simple.

On the other hand, your suggestion will only work for teams who use Spring. For us, the the rule would instead have to look for JAX-RS @Path annotations.

However, I strongly advise against doing that as Bean Validation is not at all specific to HTTP! My suggestion is to leave this to the users: add a note to the rule description saying something like “note that this rule will check all classes. if it should only look at certain parts/layers, use SonarQube’s ‘Restrict Scope of Coding Rules’ functionality.” When a team decides to activate this rule, they can make an informed decision that suits their code base.

What do you think?

Best regards,
Jens


(Alexandre Gigleux) #6

Hello Jens,

Let’s try with your suggestion to use “Restrict Scope of Coding Rules” and see even if I believe it’s kind of advance feature that not a lot of SQ users are aware of.

I created the rule specification (https://jira.sonarsource.com/browse/RSPEC-5128) and its linked implementation ticket (https://jira.sonarsource.com/browse/SONARJAVA-3007) so you can push your PR.

Regards


Rule to detect Unused Public classes or methods
(Jens Bannmann) #7

Hi all,

I created pull request #2446 for this.

Best regards,
Jens


(Lubinson) #8

In my java project, I have a java bean which have the @NotNull annotation. But actually we don’t use validator to check the bean input. So the field is possible null. And then in some place we check the field != null then sonar submit a bug Change this condition so that it does not always evaluate to "true".

I think sonar should not restrict based on mutable facts, e.g. annotation which may be implemented or not implemented, or result may be different. At least should not submit a major bug.

BRs.


(Jens Bannmann) #9

Hi Lubinson,

I’m pretty sure SonarSource employees will ask you which @NotNull annotation exactly you use. Please post the fully qualified class name of the annotation (i.e. the import ....NotNull line).

Also, when reporting suspected bugs or suggestions, always include the key of the respective SonarJava rule - for example, the rule this thread is about is called S5128. In SonarQube/SonarCloud, the rule key is displayed in the upper right corner of the rule description window.

BR
Jens


(Lubinson) #10

@bannmann

I just made a simple sample to reproduce this. Pls take a look. I found the rule id is s2583.

package test;

import javax.validation.Valid;
import javax.validation.constraints.NotNull;

public class MyBean {

  public String name = null;
  
  public static void main(String[] args) {
    MyBean myBean = new MyBean();
    if (myBean.getName() != null){
      System.out.println("I am not null");
    }else {
      System.out.println("I am null");
    }

  }

  @NotNull
  @Valid
  public String getName() {
    return name;
  }

  public void setName(String name) {
    this.name = name;
  }

}

Actually we can use this Bean inside our code to get/set without validation or use validation if it is needed, that’s depends on use cases.

Thanks.