C0urante commented on a change in pull request #11333:
URL: https://github.com/apache/kafka/pull/11333#discussion_r720508381
##########
File path:
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/AbstractHerder.java
##########
@@ -576,8 +576,12 @@ private static ConfigInfos validateClientOverrides(String
connName,
return new ConfigInfos(connectorClass.toString(), errorCount, new
ArrayList<>(groups), configInfoList);
}
- // public for testing
public static ConfigInfos generateResult(String connType, Map<String,
ConfigKey> configKeys, List<ConfigValue> configValues, List<String> groups) {
+ return generateResult(connType, configKeys, configValues, groups,
Collections.emptyMap());
+ }
Review comment:
Since this is no longer used in the main code base, could we remove it
from this class and, where necessary, replace it either with a call to the new
variant created below, or a testing-only wrapper function that has the same
body as this one?
##########
File path:
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/entities/ConfigInfo.java
##########
@@ -60,6 +60,6 @@ public int hashCode() {
@Override
public String toString() {
- return "[" + configKey.toString() + "," + configValue.toString() + "]";
+ return "[" + configKey + "," + configValue + "]";
Review comment:
Nice cleanup 👍
##########
File path:
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/AbstractHerder.java
##########
@@ -603,9 +607,39 @@ public static ConfigInfos generateResult(String connType,
Map<String, ConfigKey>
}
configInfoList.add(new ConfigInfo(configKeyInfo, configValueInfo));
}
+
+ final List<ConfigValueInfo> nullConfigValues =
buildConfigValueInfoWithNullValues(connectorProps);
+ nullConfigValues.forEach(configWithNullValue ->
addError(configInfoList, configWithNullValue));
+ errorCount += nullConfigValues.size();
+
return new ConfigInfos(connType, errorCount, groups, configInfoList);
}
+ private static void addError(List<ConfigInfo> configInfoList,
ConfigValueInfo configWithNullValue) {
+ final Optional<ConfigInfo> predefinedConfig =
getConfigInfoForPredefinedConfig(configInfoList, configWithNullValue.name());
+ if (predefinedConfig.isPresent()) {
+ predefinedConfig.get()
+ .configValue()
+ .errors()
+ .addAll(configWithNullValue.errors());
+ } else {
+ configInfoList.add(new ConfigInfo(null, configWithNullValue));
+ }
+ }
+
+ private static Optional<ConfigInfo>
getConfigInfoForPredefinedConfig(List<ConfigInfo> configInfoList, String
nullConfigName) {
+ return configInfoList.stream()
+ .filter(configInfo ->
configInfo.configValue().name().equals(nullConfigName))
+ .findAny();
+ }
+
+ private static List<ConfigValueInfo>
buildConfigValueInfoWithNullValues(Map<String, String> connectorProps) {
+ return connectorProps.entrySet().stream()
+ .filter(configEntry -> configEntry.getValue() == null)
+ .map(configEntry -> new ConfigValueInfo(configEntry.getKey(),
String.format("Null Value Not Allowed for key %s.", configEntry.getKey())))
Review comment:
I believe that including the name of the property in the error message
is redundant as that information will be available already in the REST response.
I also think we may want to be clearer about the error message here. Users
can't supply null values, but developers (by specifying `null` as the default
value for a property in a `ConfigDef`, for example) definitely can, and we may
want to make it clear which variety we're prohibiting.
What do you think about this?
```suggestion
.map(configEntry -> new
ConfigValueInfo(configEntry.getKey(), "The JSON literal `null` may not be used
in connector configurations"))
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
