...
In the `ConfigEntry` class, the `value` fields field can be `null`. However, the users can easily assume it can't be and encounter NullPointerException since the type is not explicit. Thus we want to change its type to `Optional<String>` to make it explicit that it might not be set. The users can write code like `leaderConfig.value().length()` which throws a null pointer exception.
Furthermore, in our implementation here the `AlterConfigsRequest.ConfigEntry` value is requires not be null, however there is nothing preventing `configEntry.value()` to not returning null. Similar cases can also be found in here.
Public Interfaces
We plan to change the `value` field of the class to be `Optional<String>` to make it explicitly to the caller that it might well be null. And change the `value()` method to return an `Optional<String>` as well.
public String value() {
return value;
}
=>
public Optional<String> value() {
return value;
}
...
Considering the following configurations:
All of the above contain default empty string values and there is nothing prevent them to be set to null. Having read the documentation the caller might easily think that the value of the above configuration can't be null given it's initial value is empty string. Consequently, they forget the null check and hit NullPointerException when these configuration are indeed `null`. The users can write code like `leaderConfig.value().length() == 0` which throws a NullPointerException in execution.
Public Interfaces
We plan to add a new method `valueOptional()` and mark the `value()` method as deprecated.
Code Block | ||||
---|---|---|---|---|
| ||||
public Optional<String> valueOptional() { |
...
return value; |
...
} |
Proposed Changes
We plan to change the type of the value field to be an `Optional<String>` which requires us to also change the getter, `equals` and `hashCode` methods.
Code Block | ||||
---|---|---|---|---|
| ||||
private final String value; => private final Optional<String> value; |
...
@Override public boolean equals(Object o) { |
...
if (this == o) |
...
return true; |
...
if (o == null || getClass() != o.getClass()) |
...
return false; |
...
ConfigEntry that = (ConfigEntry) o; |
...
return this.name.equals(that.name) && |
...
this.value.equals(that.value) && |
...
// CHANGED this.isSensitive == that.isSensitive && |
...
this.isReadOnly == that.isReadOnly && |
...
this.source == that.source && |
...
Objects.equals(this.synonyms, that.synonyms); |
...
} |
...
@Override |
...
public int hashCode() { |
...
final int prime = 31; |
...
int result = 1; |
...
result = prime * result + name.hashCode(); |
...
result = prime * result + value.hashCode(); |
...
// CHANGED result = prime * result + (isSensitive ? 1 : 0); |
...
result = prime * result + (isReadOnly ? 1 : 0); |
...
result = prime * result + source.hashCode(); |
...
result = prime * result + synonyms.hashCode(); |
...
return result; |
...
} |
Compatibility, Deprecation, and Migration Plan
...