How to prevent false positive null pointer warnings, when using CGLIB / Spring AOP?

Question

I'm using Spring AOP and therefore indirectly CGLIB in my Spring MVC controller. Since CGLIB needs an default constructor I included one and my controller now looks like this:

@Controller
public class ExampleController {

    private final ExampleService exampleService;

    public ExampleController(){
        this.exampleService = null;
    }

    @Autowired
    public ExampleController(ExampleService exampleService){
        this.exampleService = exampleService;
    }

    @Transactional
    @ResponseBody
    @RequestMapping(value = "/example/foo")
    public ExampleResponse profilePicture(){
        return this.exampleService.foo(); // IntelliJ reports potential NPE here
    }
}

The problem now is, that IntelliJ IDEA's static code analysis reports a potential NullPointerException, because this.exampleService might be null.

My question is:

How to prevent these false positive null pointer warnings? One solution would be to add assert this.exampleService != null or maybe use Guava's Preconditions.checkNotNull(this.exampleService).

However, this has to be added to each method for each and every field used in this method. I would prefer a solution I could add in a single place. Maybe a annotation on the default constructor or something?

EDIT:

Seems to be fixed with Spring 4, however I'm currently using Spring 3: http://blog.codeleak.pl/2014/07/spring-4-cglib-based-proxy-classes-with-no-default-ctor.html

Does that constructor actually have to be callable, or does it just have to exist? Could you throw an exception from it? — chrylis -cautiouslyoptimistic-, Feb 09 '16 at 10:53
One option is to use AspectJ weaver at compile time or runtime, rather than cglib. Spring docs explain how to do this. One benefit to this approach is that it allows dependency injection and instrumentation of domain objects that would usually be 'anaemic'. Another is to back classes that will be instrumented with an interface, and therefore use dynamic proxy rather than cglib - depending on the nature of the class this may be a good practice anyway. — Jasper Blues, Feb 11 '16 at 11:22
Does issue still exist when a `setter` for `exampleService` is present ? — 11thdimension, Feb 12 '16 at 17:23

score 3 · Accepted Answer · answered Feb 11 '16 at 17:43

3

You can annotate your field (if you are sure that it will really not be null) with:

//import org.jetbrains.annotations.NotNull;
@NotNull
private final ExampleService exampleService;

This will instruct Idea to assume this field to be not-null in all cases. In this case your real constructor will also be annotated automatically by Idea:

public ExampleController(@NotNull ExampleService exampleService){
    this.exampleService = exampleService;
}

answered Feb 11 '16 at 17:43

Gergely Bacso

14,243
2
44
64

I think this is as good as it gets. I was hoping for something like `@IgnoreNotUsed` annotation to add to the argumentless contructor, but this isn't that bad after all... – Tim Büthe Feb 17 '16 at 09:58

Kirill Gamazkov · Answer 2 · 2016-02-17T10:18:05.490

0

IntelliJ IDEA's static code analysis reports a potential NullPointerException

You can switch off these reports for specific fields, variables, methods, etc using @SuppressWarnings({"unchecked", "UnusedDeclaration"}) or a comment. Actually, the IDEA itself can suggest you this solution. See https://www.jetbrains.com/idea/help/suppressing-inspections.html

You can switch warning of for a single line of code:

void foo(java.util.Set set) {
    @SuppressWarnings("unchecked")
    java.util.Set<String> strings = set;
    System.out.println(strings);
}

edited Feb 17 '16 at 10:18

answered Feb 16 '16 at 14:48

Kirill Gamazkov

3,277
1
18
22

Potential NullPointerExceptions-warnings are quite useful and I don't want to deactivate or suppress it altogether – Tim Büthe Feb 16 '16 at 16:59
You can disable it for a specific line of your code, not for the whole exception class – Kirill Gamazkov Feb 17 '16 at 10:14
As I wrote in my question, I could use `assert this.exampleService != null` which far more expressive than `@SuppressWarnings`. However, I don't want to add assertions all over the place. – Tim Büthe Feb 17 '16 at 13:38
BTW assertions are rather bad practice since you can't be really sure that they are turned on for your application – Kirill Gamazkov Feb 17 '16 at 14:37
I disagree, assertions are exactly right in this kind of situation. You tell the compiler, that you're sure about something. At development it gets checked, at runtime it doesn't and you get around performance penalties. – Tim Büthe Feb 17 '16 at 14:44
Right. It's okay in your particular case, It's just not how assertions are meant to be used, in general :) – Kirill Gamazkov Feb 17 '16 at 14:49

score 0 · Answer 3 · answered Feb 16 '16 at 15:38

You could create a default new instance of the ExampleService and assign it in the default constructor rather than assign it to null:

public ExampleController(){
    this.exampleService = new ExampleService();
}

or

public ExampleController(){
    this.exampleService = ExampleServiceFactory.Create();
}

Since this object should never be used in normal operation, it will have no effect, but if the object is used by the framework, or accidentally used directly because of later changes to the code, this will give you more information in a stack trace than a null pointer exception, and this will also solve the error that this.exampleService can be null.

This might require some changes to the ExampleService class, either to allow creating a new instance with default parameters, or to allow creating a new instance that is essentially a shell that does nothing. If it inherits from a base interface type, then a non-functional class can inherit from the same base type specifically as a place-holder. This pattern will also allow you to inject error handling code to provide a clear warning if the application attempts to use a default non-functional instance.

I have found that in languages like Java and C# where almost everything is a pointer, relying on null pointers even in areas where they should never be used makes maintenance more difficult than it should be, since they can often be used accidentally. The underlying virtual machines are designed to have the equivalent of a panic attack whenever code attempts to use a null pointer - I suspect this is because of the legacy of C where null pointers could really mess up the entire running program. Because of this virtual panic attack, they don't provide any useful information that would help diagnose the problem, especially since the value (null) is completely useless for identifying what happened. By avoiding null pointers, and instead specifically designing class hierarchies to determine whether an instantiated object should do any real work, you can avoid the potential problems with null pointers and also make your code easier and safer to maintain.

How to prevent false positive null pointer warnings, when using CGLIB / Spring AOP?

3 Answers3