2

I'm working on a JSF project using CDI. We are using SonarQube to manage the code quality. One of the issues that popped up after running a scan on our project is S3306: "Constructor injection should be used instead of field injection".

It was triggered by injects we are using in our beans, for example:

@Named
@ViewScoped
public class AccountsController extends AbstractController<Account> {

  @Inject
  private AccountsFacade accountsFacade;

  public AccountsController() {
    super(Account.class);
  }

  ...
}

Injected are facades like:

@Stateless
public class AccountsFacade extends AbstractFacade<Account> {

  @PersistenceContext(unitName = "...")
  private EntityManager entityManager;

  public AccountsFacade() {
    super(Account.class);
  }

  ...
}

The info on this issue provided by SonarQube:

Field injection seems like a tidy way to get your classes what they need to do their jobs, but it's really a NullPointerException waiting to happen unless all your class constructors are private. That's because any class instances that are constructed by callers, rather than instantiated by the Spring framework, won't have the ability to perform the field injection.

Instead @Inject should be moved to the constructor and the fields required as constructor parameters.

This rule raises an issue when classes with non-private constructors (including the default constructor) use field injection.

The suggested solution:

class MyComponent {

  private final MyCollaborator collaborator;

  @Inject
  public MyComponent(MyCollaborator collaborator) {
    Assert.notNull(collaborator, "MyCollaborator must not be null!");
    this.collaborator = collaborator;
  }

  public void myBusinessMethod() {
    collaborator.doSomething();
  }
}

Since managed beans are created using a constructor with no arguments, is there any way to comply with the SonarQube rule?

Edit: I think this is relevant: we are using CDI. I'm not sure if the previous link (Oracle documentation) applies here.

Edit 2: I just tried the suggested solution after I've read documentation on WELD injection points. But that gives me this error:

WELD-001435: Normal scoped bean class ...AccountsController is not proxyable because it has no no-args constructor

Edit 3: The error in edit 2 is indeed (see comments at the question) caused by the @Inject of the AccountsController in an other controller. See also the answer.

Jasper de Vries
  • 19,370
  • 6
  • 64
  • 102
  • Are you sure it should be `@Inject public MyComponent(MyCollaborator collaborator) {...}` and not `public MyComponent(@Inject MyCollaborator collaborator) {...}` ? See also http://stackoverflow.com/questions/19381846/why-use-constructor-over-setter-injection-in-cdi – Kukeltje Nov 25 '16 at 13:41
  • Hmmmm.... contradictory information... (never used the constructor way)... – Kukeltje Nov 25 '16 at 13:43
  • 1
    Your example seems indeed right... Weird and btw... `Spring framework` in the sonarcube docs? Hmmmm Maybe override the sonarcube rule ;-) – Kukeltje Nov 25 '16 at 13:47
  • Why I stated the 'override the rule' is that in the link I posted it talks explicitly about the @PostConstruct to prevent NPE's... So maybe this rules is not really applicable here – Kukeltje Nov 25 '16 at 13:53
  • I you think you encountered a false positive not applicable in some context or that the doc is misleading please reach out to sonarqube@googlegroups.com to discuss this so the java team could improve the rule. – benzonico Nov 25 '16 at 14:19
  • @JasperdeVries: But it is still weird that the oracle docs talk about constructor injection and when you use that, weld complains. – Kukeltje Nov 25 '16 at 15:08
  • @benzonico: Then very few people (if at all) using CDI use SonarQube... – Kukeltje Nov 25 '16 at 15:09
  • @JasperdeVries Sorry, I meant the weld docs where you referred to. They have the same example, yet in real life it fails – Kukeltje Nov 25 '16 at 15:12
  • @Kukeltje Hmm.. my bean is also injected into an other bean.. that injection might cause the weld error. I'll create a clean project next week to eliminate some variables... – Jasper de Vries Nov 25 '16 at 15:15
  • @Kukeltje Or the rule is deactivated, but if we don't have the feedback we can't improve the rule to make it valuable in CDI context. – benzonico Nov 25 '16 at 16:55
  • Could indeed be. Bad users to not give feedback if the do use it but just disable this rule ;-). I will certainly also give SonarQube a try on my code! – Kukeltje Nov 25 '16 at 17:19
  • Can you provide version of JSF and which JEE version you are using? With recent versions of JSF in JEE 7 you should be able to use all Cdi features in JSF beans – Tibor Blenessy Nov 26 '16 at 07:00
  • JEE 7, JSF 2.2 (bundled with Payara 4.1.1 163) – Jasper de Vries Nov 26 '16 at 14:51
  • @benzonico I found the cause and a solution. Still not sure if this rule is helpful for CDI beans. I can't think of a scenario where one would manually instantiate such a bean. Also, I must agree with Kukeltje that it is weird to explicitly mention Spring in the documentation. It feels like that this rule was ment to apply to Spring applications. – Jasper de Vries Nov 30 '16 at 09:23

1 Answers1

1

The suggested solution does work for injecting @Stateless classes into CDI beans with Payara 4.1.1 163. I have been experiencing problems with GlassFish 4.1 though.

Additionally, when you apply this solution when injecting beans into other beans like:

@Named
@ViewScoped
public class OtherController extends AbstractController<Other> {

  private final AccountsController accountsController;

  @Inject
  public OtherController(AccountsController accountsController) {
    Assert.notNull(accountsController, "accountsController must not be null!");
    this.accountsController = accountsController;
  }

}

.. you'll get this error (as mentioned in edit 2 of the question):

WELD-001435: Normal scoped bean class ...AccountsController is not proxyable because it has no no-args constructor

N.B. AccountsController was modified as suggested.

I resolved this issue by using OmniFaces Beans:

@Named
@ViewScoped
public class OtherController extends AbstractController<Other> {

  private final AccountsController accountsController;

  public OtherController() {
    this.accountsController = Beans.getInstance(AccountsController.class);
  }

}

... or, for something completely different, you could also just suppress the warning on your (CDI) managed beans:

@SuppressWarnings("squid:S3306")
Jasper de Vries
  • 19,370
  • 6
  • 64
  • 102
  • 1
    I'm not a CDI / WELD expert, but it feels like an issue there. In that perspective this is a workaround. In the perspective of complying with S3306 this is a solution. – Jasper de Vries Nov 29 '16 at 22:54