uber
Spring InitializingBean and Objects.requireNonNull()
I'm tryting to integrate NullAway into a project I've just started, that is using Spring Batch.
There is an idiom, where ItemReader has bunch of setters, and some of them might be required to call (I know, constructor would be nicer, but bear with me). The way they force it is by using org.springframework.beans.factory.InitializingBean
public class MyItemReader<T> extends AbstractItemCountingItemStreamItemReader<T> implements ResourceAwareItemReaderItemStream<T>, InitializingBean
{
private RawNestedDataMapper<T> itemDataMapper;
public void setItemDataMapper(final RawNestedDataMapper<T> itemDataMapper)
{
this.itemDataMapper = itemDataMapper;
}
@Override
public void afterPropertiesSet() throws Exception
{
Objects.requireNonNull(itemDataMapper, "itemDataMapper is required");
}
@Nullable
@Override
protected T doRead() throws Exception {
String line = readLine();
if (line == null) {
return null;
}
try {
return itemDataMapper.mapLine(line, lineCount);
} catch (Exception ex) {
throw new FlatFileParseException(ex.getMessage(), ex);
}
}
}
I've read the docs and my understanding and expectation was, that if I mark InitializingBean as initializer
<compilerArgs>
<arg>-XepOpt:NullAway:KnownInitializers=org.springframework.beans.factory.InitializingBean.afterPropertiesSet</arg>
the NullAway will understand that the afterPropertiesSet is a semi-constructor, that is called before any work is done in the class and therefore can check the requirements (for example that the property is initialized) and I won't have to do the check anywhere else in the class.
But instead, it produces following errors
[ERROR] MyItemReader.java:[153,17] [NullAway] initializer method does not guarantee @NonNull field resource is initialized along all control-flow paths (remember to check for exceptions or early returns).
(see http://t.uber.com/nullaway )
[ERROR] MyItemReader.java:[155,32] [NullAway] read of @NonNull field itemDataMapper before initialization
(see http://t.uber.com/nullaway )
If I change the implementation to
@Override
public void afterPropertiesSet() throws Exception
{
if (itemDataMapper == null) {
throw new NullPointerException("itemDataMapper is required");
}
}
it still prints following error
[ERROR] MyItemReader.java:[153,17] [NullAway] initializer method does not guarantee @NonNull field resource is initialized along all control-flow paths (remember to check for exceptions or early returns).
(see http://t.uber.com/nullaway )
maven: effective pom
<plugin>
<artifactId>maven-compiler-plugin</artifactId>
<version>3.8.0</version>
<dependencies>
<dependency>
<groupId>org.codehaus.plexus</groupId>
<artifactId>plexus-compiler-javac-errorprone</artifactId>
<version>2.8.5</version>
<scope>compile</scope>
</dependency>
<dependency>
<groupId>com.google.errorprone</groupId>
<artifactId>error_prone_core</artifactId>
<version>2.3.3</version>
<scope>compile</scope>
</dependency>
</dependencies>
<configuration>
<compilerId>javac-with-errorprone</compilerId>
<forceJavacCompilerUse>true</forceJavacCompilerUse>
<source>1.8</source>
<target>1.8</target>
<compilerArgs>
<arg>-Werror</arg>
<arg>-Xlint:all,-fallthrough,-processing,-serial,-classfile</arg>
<arg>-parameters</arg>
<arg>-XDcompilePolicy=simple</arg>
<arg>-Xep:NullAway:ERROR</arg>
<arg>-XepOpt:NullAway:AnnotatedPackages=com.cogvio.dl</arg>
<arg>-XepOpt:NullAway:TreatGeneratedAsUnannotated=true</arg>
<arg>-XepOpt:NullAway:AcknowledgeRestrictiveAnnotations=true</arg>
<arg>-XepOpt:NullAway:KnownInitializers=org.springframework.beans.factory.InitializingBean.afterPropertiesSet</arg>
</compilerArgs>
<annotationProcessorPaths>
<path>
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-configuration-processor</artifactId>
<version>2.2.0.M1</version>
</path>
<path>
<groupId>org.springframework</groupId>
<artifactId>spring-context-indexer</artifactId>
<version>5.2.0.M1</version>
</path>
<path>
<groupId>com.uber.nullaway</groupId>
<artifactId>nullaway</artifactId>
<version>0.7.2</version>
</path>
</annotationProcessorPaths>
<showWarnings>true</showWarnings>
<parameters>true</parameters>
</configuration>
</plugin>
Hi @fprochazka , thanks for the detailed report!
This is interesting. It's indeed true that XepOpt:NullAway:KnownInitializers is a way to mark methods as being initializers. However, the expectation is that an initializer method will initialize/set the @NonNull fields that remain un-initialized by the constructor. So NullAway is complaining that afterPropertiesSet doesn't actually initialize/set itemDataMapper.
In NullAway's model, the idea is that the initializer will be called after the constructor but before any other method of the object.
Of course, what is happening here is that the method initializes nothing, it just guarantees that after you call it, everything has been initialized by other methods (namely setItemDataMapper which acts like the actual initializer here, in the NullAway sense).
We could make NullAway check which fields can be null on exit from the initializer, rather than require them to be set inside the initializer. That would work in this case, but there is still a more fundamental issue with that model, namely that NullAway would understand that code as:
- The constructor runs first
- Then
afterPropertiesSet - Then any other method, including
setItemDataMapper
So you could write your code as:
public class MyItemReader<T> extends AbstractItemCountingItemStreamItemReader<T> implements ResourceAwareItemReaderItemStream<T>, InitializingBean
{
private RawNestedDataMapper<T> itemDataMapper;
public void setItemDataMapper()
{
this.itemDataMapper = itemDataMapper;
itemDataMapper.foo(); // RUNTIME NPE!!!
}
@Override
public void afterPropertiesSet() throws Exception
{
Objects.requireNonNull(itemDataMapper, "itemDataMapper is required");
}
}
And NullAway would miss the error!
I think what you want is either of these:
- Mark the
setXXXXmethod as@Initializer. This might still require NullAway changes if multiple initializer methods are needed. - Have specific logic to handle Spring InitializingBean as a NullAway handler
- (The most practical short term): Mark all classes implementing
InitializingBeanwith@SuppressWarnings("NullAway.Init")that still gets you most of the benefits of nullness checking itself, but trusts you to take care of field initialization.
In general (3) would be my starting point for any classes with a complex initialization life-cycle (anything beyond constructor + single initializer method).
Does this all make sense?
cc: @msridhar for thoughts
Thank you for the response, I can see now that it's not that simple to solve.
I had to verify, but it turns out I can mark only the specific field (and not the entire class) with @SuppressWarnings("NullAway.Init"), which is good enough for me :)
I'll leave it up to you if you want to keep this issue open for further discussion, I consider this resolved.
Thanks for the report, @fprochazka. The only other relevant thing I can think of are external init annotations, but that doesn't apply here since there is no special annotation on these classes (the convention is based on implementing an interface). Like @lazaroclapp said, if there a strong desire to have built-in support in NullAway for this convention, it could be done with a handler.
