BadPractice15 - SpotBugsExtensionForSpringFrameWork/CS5098 GitHub Wiki

Bug patterns name: NON_FINAL_SETTER_METHOD_KEYWORD_IN_ABSTRACT_CLASS Short Description: When we use @Autowired on a setter method in an abstract class, we should use the final keyword.

Description

Setter injection is possible in an abstract class, but it's risky if we don't use the final keyword for the setter method, so that the subclass can't override the setter method. Otherwise, the annotation won't work as we expect. The application may not be stable if a subclass overrides the setter method (TBH, not sure about what is the unexpected scenario).

//Bad Case
public abstract class BallService {

    private LogRepository logRepository;

    @Autowired
    public void setLogRepository(LogRepository logRepository) { // where is final keyword?
        this.logRepository = logRepository;
    }
}
//Good Case
public abstract class BallService {

    private LogRepository logRepository;

    @Autowired
    public final void setLogRepository(LogRepository logRepository) { // Good
        this.logRepository = logRepository;
    }
}

Evidence

Here's a minimal example to show the problem:

@SpringBootApplication
@RestController
public class DemoApplication extends AutowiredBase {

    public static void main(String[] args) {
        SpringApplication.run(DemoApplication.class, args);
    }

    @GetMapping
    public String get() {
        return timeProvider.getTime();
    }

    @Override
    public void setTimeProvider(final TimeProvider timeProvider) {
        // never called, because this method is not autowired
        super.setTimeProvider(timeProvider);
    }

}

@Component
class TimeProvider {
    public String getTime() {
        return Instant.now().toString();
    }
}

class AutowiredBase {
    protected TimeProvider timeProvider;

    @Autowired
    public void setTimeProvider(final TimeProvider timeProvider) {
        System.out.println("Autowiring time provider");
        this.timeProvider = timeProvider;
    }
}

DemoApplication#setTimeProvider will never be called, therefore calling the get() method will result in an NPE. If you comment the overridden method and leave the base method, everything works as expected.

Reference List