kirktrue commented on code in PR #20134:
URL: https://github.com/apache/kafka/pull/20134#discussion_r2195815877
##########
clients/src/main/java/org/apache/kafka/common/utils/Utils.java:
##########
@@ -1469,7 +1470,19 @@ public static <K, V> Map<K, V> filterMap(final Map<K, V>
map, final Predicate<En
* @return a map including all elements in properties
*/
public static Map<String, Object> propsToMap(Properties properties) {
- return castToStringObjectMap(properties);
+ try {
+ final Enumeration<?> enumeration = properties.propertyNames();
+ Map<String, Object> props = new HashMap<>();
+ while (enumeration.hasMoreElements()) {
+ Object key = enumeration.nextElement();
+ String keyString = (String) key;
+ Object value = (properties.get(keyString) != null) ?
properties.get(keyString) : properties.getProperty(keyString);
+ props.put(keyString, value);
+ }
+ return props;
+ } catch (Exception e) {
+ throw new ConfigException("Key must be a string.");
+ }
Review Comment:
Rather than casting the key to a `String`, my preference would be to
proactively check the type, just like `castToStringObjectMap()` does:
```suggestion
final Enumeration<?> enumeration = properties.propertyNames();
Map<String, Object> map = new HashMap<>();
while (enumeration.hasMoreElements()) {
Object key = enumeration.nextElement();
if (key instanceof String) {
String k = (String) key;
map.put(k, properties.getProperty(k));
} else {
throw new ConfigException(String.valueOf(key),
properties.get(key), "Key must be a string.");
}
}
return props;
```
##########
clients/src/test/java/org/apache/kafka/clients/producer/KafkaProducerTest.java:
##########
@@ -579,7 +579,7 @@ public void testConstructorWithNotStringKey() {
ConfigException ce = assertThrows(
ConfigException.class,
() -> new KafkaProducer<>(props, new StringSerializer(), new
StringSerializer()));
- assertTrue(ce.getMessage().contains("not string key"), "Unexpected
exception message: " + ce.getMessage());
+ assertTrue(ce.getMessage().contains("must be a string"), "Unexpected
exception message: " + ce.getMessage());
Review Comment:
Does this need to be changed? The existing code is checking that the error
message contains the value `not string key` which is the literal value for
property `1` that's added on line 578.
I suspect the reason it needed to be changed is because the
`ConfigException` that is thrown from `propsToMap()` doesn't pass in the
erroneous property name and value.
--
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
