Skip to content

Commit

Permalink
Avoid "strongly discouraged" put/putAll calls on `java.util.Prope…
Browse files Browse the repository at this point in the history
…rties` (#1366)

`java.util.Properties` extends a `Map<Object, Object>`, which means the
`put`/`putAll` etc have `Object` parameters - however [the Javadoc
says](https://docs.oracle.com/javase/8/docs/api/java/util/Properties.html):
> Because Properties inherits from Hashtable, the put and putAll methods
can be applied to a Properties object. Their use is strongly discouraged
as they allow the caller to insert entries whose keys or values are not
Strings. The setProperty method should be used instead.

[Flyway recently broke with this
convention](flyway/flyway#3866), causing
problems in:
- [Hazelcast due to the unexpected
types](#26310)
- [Scala
wrapper](flyway/flyway#3866 (comment))

[Flyway subsequently resolved
this](flyway/flyway#3866 (comment)).

The intent of this PR is to enhance the codebase to use type-safety to
prevent these errors being introduced in future, rather than ensuring
compliance with this convention.

_Most_ uses of `Properties` in Hazelcast are already `String`s:
- Changing the non-`String` types is out of scope
- When working with `String`s, the more specific
`setProperty`/`getProperty` should be used in preference to
`put`/`putAll`/`get`, _where possible_

GitOrigin-RevId: 25ada108ca08e37af76338d00650780135cef24c
  • Loading branch information
JackPGreen authored and actions-user committed Apr 16, 2024
1 parent 4db0e47 commit 4a84c46
Show file tree
Hide file tree
Showing 46 changed files with 153 additions and 138 deletions.
Expand Up @@ -28,10 +28,10 @@ public DebeziumConfig(@Nonnull String name, @Nonnull String connectorClass) {
Objects.requireNonNull(name, "name");
Objects.requireNonNull(connectorClass, "connectorClass");

properties.put("name", name);
properties.put(CdcSourceP.CONNECTOR_CLASS_PROPERTY, connectorClass);
properties.put("database.history", CdcSourceP.DatabaseHistoryImpl.class.getName());
properties.put("tombstones.on.delete", "false");
properties.setProperty("name", name);
properties.setProperty(CdcSourceP.CONNECTOR_CLASS_PROPERTY, connectorClass);
properties.setProperty("database.history", CdcSourceP.DatabaseHistoryImpl.class.getName());
properties.setProperty("tombstones.on.delete", "false");
}

public void setProperty(String key, Object value) {
Expand Down
Expand Up @@ -42,9 +42,9 @@ public static void runQuery(MySQLContainer<?> container, String query) {

public static Connection getMySqlConnection(String url, String user, String password) throws SQLException {
Properties properties = new Properties();
properties.put("user", user);
properties.put("password", password);
properties.put("useSSL", "false");
properties.setProperty("user", user);
properties.setProperty("password", password);
properties.setProperty("useSSL", "false");

return DriverManager.getConnection(url, properties);
}
Expand Down
Expand Up @@ -16,6 +16,7 @@

package com.hazelcast.jet.kafka.connect;

import com.hazelcast.client.impl.protocol.util.PropertiesUtil;
import com.hazelcast.function.FunctionEx;
import com.hazelcast.jet.kafka.connect.impl.SourceConnectorWrapper;
import com.hazelcast.jet.kafka.connect.impl.processorsupplier.TaskMaxProcessorMetaSupplier;
Expand Down Expand Up @@ -185,8 +186,7 @@ public static StreamSource<SourceRecord> connect(@Nonnull Properties properties)

private static Properties getDefaultProperties(Properties properties) {
// Make new copy
Properties defaultProperties = new Properties();
defaultProperties.putAll(properties);
Properties defaultProperties = PropertiesUtil.clone(properties);

// Populate tasks.max property if necessary
defaultProperties.putIfAbsent("tasks.max", "1");
Expand Down
Expand Up @@ -128,7 +128,7 @@ public void testReadFromNeo4jConnector() {
@Test
public void testDbNotStarted() {
Properties connectorProperties = getConnectorProperties();
connectorProperties.put("neo4j.server.uri", "bolt://localhost:52403");
connectorProperties.setProperty("neo4j.server.uri", "bolt://localhost:52403");
connectorProperties.setProperty("neo4j.retry.backoff.msecs", "5");
connectorProperties.setProperty("neo4j.retry.max.attemps", "1");

Expand Down
Expand Up @@ -212,7 +212,7 @@ public <K, V> KafkaProducer<K, V> getProducer(
Properties props = Util.mergeProps(configProperties, properties);

if (transactionalId != null) {
props.put("transactional.id", transactionalId);
props.setProperty("transactional.id", transactionalId);
}
return new KafkaProducer<>(props);
}
Expand Down
Expand Up @@ -207,7 +207,7 @@ public void should_list_resource_types() {
public void shared_producer_should_not_be_created_with_additional_props() {
kafkaDataConnection = createKafkaDataConnection(kafkaTestSupport);
Properties properties = new Properties();
properties.put("A", "B");
properties.setProperty("A", "B");

assertThatThrownBy(() -> kafkaDataConnection.getProducer(null, properties))
.isInstanceOf(HazelcastException.class)
Expand Down
Expand Up @@ -17,6 +17,7 @@
package com.hazelcast.jet.kafka.impl;

import com.hazelcast.collection.IList;
import com.hazelcast.client.impl.protocol.util.PropertiesUtil;
import com.hazelcast.jet.SimpleTestInClusterSupport;
import com.hazelcast.jet.kafka.HazelcastKafkaAvroDeserializer;
import com.hazelcast.jet.kafka.HazelcastKafkaAvroSerializer;
Expand Down Expand Up @@ -133,14 +134,13 @@ public void writeGenericRecord() {
}

private Properties createProperties() {
Properties properties = new Properties();
Properties properties = PropertiesUtil.fromMap(AVRO_SCHEMA_PROPERTIES);
properties.setProperty(BOOTSTRAP_SERVERS_CONFIG, kafkaTestSupport.getBrokerConnectionString());
properties.setProperty(KEY_DESERIALIZER_CLASS_CONFIG, HazelcastKafkaAvroDeserializer.class.getCanonicalName());
properties.setProperty(VALUE_DESERIALIZER_CLASS_CONFIG, HazelcastKafkaAvroDeserializer.class.getCanonicalName());
properties.setProperty(KEY_SERIALIZER_CLASS_CONFIG, HazelcastKafkaAvroSerializer.class.getCanonicalName());
properties.setProperty(VALUE_SERIALIZER_CLASS_CONFIG, HazelcastKafkaAvroSerializer.class.getCanonicalName());
properties.setProperty(AUTO_OFFSET_RESET_CONFIG, "earliest");
properties.putAll(AVRO_SCHEMA_PROPERTIES);

return properties;
}
Expand Down
Expand Up @@ -259,7 +259,7 @@ private void stressTest(boolean graceful, boolean exactlyOnce) {

@Test
public void test_resumeTransaction() throws Exception {
properties.put("transactional.id", "txn.resumeTransactionTest");
properties.setProperty("transactional.id", "txn.resumeTransactionTest");

// produce items
KafkaProducer<String, String> producer = new KafkaProducer<>(properties);
Expand Down
Expand Up @@ -963,9 +963,9 @@ private void assertDiscoveryConfig(DiscoveryConfig discoveryConfig) {
public void testProperties() {
Properties properties = config.getProperties();
assertNotNull(properties);
assertEquals("5", properties.get(MERGE_FIRST_RUN_DELAY_SECONDS.getName()));
assertEquals("5", properties.get(MERGE_NEXT_RUN_DELAY_SECONDS.getName()));
assertEquals("277", properties.get(PARTITION_COUNT.getName()));
assertEquals("5", properties.getProperty(MERGE_FIRST_RUN_DELAY_SECONDS.getName()));
assertEquals("5", properties.getProperty(MERGE_NEXT_RUN_DELAY_SECONDS.getName()));
assertEquals("277", properties.getProperty(PARTITION_COUNT.getName()));

Config config2 = instance.getConfig();
Properties properties2 = config2.getProperties();
Expand Down
Expand Up @@ -61,13 +61,13 @@ public void toParserConfig(SqlParser.ConfigBuilder configBuilder) {
public CalciteConnectionConfig toConnectionConfig() {
Properties connectionProperties = new Properties();

connectionProperties.put(CalciteConnectionProperty.CASE_SENSITIVE.camelName(), Boolean.toString(caseSensitive));
connectionProperties.put(CalciteConnectionProperty.UNQUOTED_CASING.camelName(), unquotedCasing.toString());
connectionProperties.put(CalciteConnectionProperty.QUOTED_CASING.camelName(), quotedCasing.toString());
connectionProperties.put(CalciteConnectionProperty.QUOTING.camelName(), quoting.toString());
connectionProperties.setProperty(CalciteConnectionProperty.CASE_SENSITIVE.camelName(), Boolean.toString(caseSensitive));
connectionProperties.setProperty(CalciteConnectionProperty.UNQUOTED_CASING.camelName(), unquotedCasing.toString());
connectionProperties.setProperty(CalciteConnectionProperty.QUOTED_CASING.camelName(), quotedCasing.toString());
connectionProperties.setProperty(CalciteConnectionProperty.QUOTING.camelName(), quoting.toString());

// Disable materializations to avoid NPE described in https://github.com/hazelcast/hazelcast/issues/17554
connectionProperties.put(CalciteConnectionProperty.MATERIALIZATIONS_ENABLED.camelName(), Boolean.toString(false));
connectionProperties.setProperty(CalciteConnectionProperty.MATERIALIZATIONS_ENABLED.camelName(), Boolean.toString(false));

return new CalciteConnectionConfigImpl(connectionProperties);
}
Expand Down
Expand Up @@ -112,7 +112,7 @@ private static Properties from(Map<String, String> options) {
String value = entry.getValue();

if (!NON_KAFKA_OPTIONS.contains(key)) {
properties.put(key, value);
properties.setProperty(key, value);
}
}
return properties;
Expand Down
Expand Up @@ -67,12 +67,12 @@ private static void createKafkaCluster() throws Exception {

protected static void createSchemaRegistry() throws Exception {
Properties properties = new Properties();
properties.put("listeners", "http://0.0.0.0:0");
properties.put(SchemaRegistryConfig.KAFKASTORE_BOOTSTRAP_SERVERS_CONFIG,
properties.setProperty("listeners", "http://0.0.0.0:0");
properties.setProperty(SchemaRegistryConfig.KAFKASTORE_BOOTSTRAP_SERVERS_CONFIG,
kafkaTestSupport.getBrokerConnectionString());
// We increase the timeout (default is 500 ms) because when Kafka is under load,
// the schema registry may give "RestClientException: Register operation timed out".
properties.put(SchemaRegistryConfig.KAFKASTORE_TIMEOUT_CONFIG, "5000");
properties.setProperty(SchemaRegistryConfig.KAFKASTORE_TIMEOUT_CONFIG, "5000");
SchemaRegistryConfig config = new SchemaRegistryConfig(properties);
kafkaTestSupport.createSchemaRegistry(config);
}
Expand Down
Expand Up @@ -20,6 +20,7 @@
import com.hazelcast.client.LoadBalancer;
import com.hazelcast.client.config.impl.XmlClientConfigLocator;
import com.hazelcast.client.config.impl.YamlClientConfigLocator;
import com.hazelcast.client.impl.protocol.util.PropertiesUtil;
import com.hazelcast.config.Config;
import com.hazelcast.config.ConfigPatternMatcher;
import com.hazelcast.config.InstanceTrackingConfig;
Expand Down Expand Up @@ -133,8 +134,7 @@ public ClientConfig() {

@SuppressWarnings({"checkstyle:npathcomplexity", "checkstyle:executablestatementcount"})
public ClientConfig(ClientConfig config) {
properties = new Properties();
properties.putAll(config.properties);
properties = PropertiesUtil.clone(config.properties);
clusterName = config.clusterName;
securityConfig = new ClientSecurityConfig(config.securityConfig);
networkConfig = new ClientNetworkConfig(config.networkConfig);
Expand Down Expand Up @@ -274,7 +274,7 @@ public String getProperty(String name) {
* @return configured {@link com.hazelcast.client.config.ClientConfig} for chaining
*/
public ClientConfig setProperty(String name, String value) {
properties.put(name, value);
properties.setProperty(name, value);
return this;
}

Expand Down
Expand Up @@ -298,7 +298,7 @@ protected void fillProperties(Node node, Properties properties) {
NodeList childNodes = node.getChildNodes();
for (int i = 0; i < childNodes.getLength(); i++) {
Node childNode = childNodes.item(i);
properties.put(childNode.getNodeName(), childNode.getNodeValue());
properties.setProperty(childNode.getNodeName(), childNode.getNodeValue());
}
}

Expand Down
Expand Up @@ -17,6 +17,8 @@

package com.hazelcast.client.impl.protocol.util;

import javax.annotation.Nonnull;

import java.util.Map;
import java.util.Properties;
import java.util.stream.Collectors;
Expand All @@ -40,4 +42,8 @@ public static Map<String, String> toMap(Properties properties) {
)
);
}

public static Properties clone(@Nonnull Properties properties) {
return (Properties) properties.clone();
}
}
Expand Up @@ -57,7 +57,7 @@ public T setFactoryClassName(@Nonnull String factoryClassName) {
* @throws NullPointerException if name or value is {@code null}
*/
public T setProperty(String name, String value) {
properties.put(name, value);
properties.setProperty(name, value);
return self();
}

Expand Down
Expand Up @@ -247,7 +247,7 @@ private void fillReplacerProperties(Node node, Properties properties) {
String childName = childNodePair.nodeName();
YamlNode child = childNodePair.childNode();
Object nodeValue = asScalar(child).nodeValue();
properties.put(childName, nodeValue != null ? nodeValue.toString() : "");
properties.setProperty(childName, nodeValue != null ? nodeValue.toString() : "");
}
}
}
Expand Up @@ -56,8 +56,7 @@ public AliasedDiscoveryConfig(String tag, boolean enabled, boolean usePublicIp,
this.tag = tag;
this.enabled = enabled;
this.usePublicIp = usePublicIp;
this.properties = new HashMap<>();
this.properties.putAll(properties);
this.properties = new HashMap<>(properties);
}

/**
Expand Down
8 changes: 4 additions & 4 deletions hazelcast/src/main/java/com/hazelcast/config/Config.java
Expand Up @@ -599,7 +599,7 @@ public Config setProperty(@Nonnull String name, @Nonnull String value) {
throw new IllegalArgumentException("argument 'name' can't be null or empty");
}
isNotNull(value, "value");
properties.put(name, value);
properties.setProperty(name, value);
return this;
}

Expand Down Expand Up @@ -3208,9 +3208,9 @@ public Config setDataConnectionConfigs(Map<String, DataConnectionConfig> dataCon
* <pre>{@code
* Config config = new Config();
* Properties properties = new Properties();
* properties.put("jdbcUrl", jdbcUrl);
* properties.put("username", username);
* properties.put("password", password);
* properties.setProperty("jdbcUrl", jdbcUrl);
* properties.setProperty("username", username);
* properties.setProperty("password", password);
* DataConnectionConfig dataConnectionConfig = new DataConnectionConfig()
* .setName("my-jdbc-data-connection")
* .setType("Jdbc")
Expand Down
Expand Up @@ -19,6 +19,7 @@
import java.util.Objects;
import java.util.Properties;

import com.hazelcast.client.impl.protocol.util.PropertiesUtil;
import com.hazelcast.config.security.IdentityConfig;
import com.hazelcast.internal.nio.ClassLoaderUtil;
import com.hazelcast.security.ICredentialsFactory;
Expand All @@ -44,8 +45,7 @@ public CredentialsFactoryConfig(String className) {
private CredentialsFactoryConfig(CredentialsFactoryConfig credentialsFactoryConfig) {
className = credentialsFactoryConfig.className;
implementation = credentialsFactoryConfig.implementation;
properties = new Properties();
properties.putAll(credentialsFactoryConfig.properties);
properties = PropertiesUtil.clone(credentialsFactoryConfig.properties);
}

public String getClassName() {
Expand Down
Expand Up @@ -16,6 +16,7 @@

package com.hazelcast.config;

import com.hazelcast.client.impl.protocol.util.PropertiesUtil;
import com.hazelcast.dataconnection.DataConnection;
import com.hazelcast.internal.config.ConfigDataSerializerHook;
import com.hazelcast.jet.pipeline.Pipeline;
Expand Down Expand Up @@ -51,19 +52,21 @@ public class DataConnectionConfig implements IdentifiedDataSerializable, NamedCo
private String name;
private String type;
private boolean shared = true;
private Properties properties = new Properties();
private Properties properties;

public DataConnectionConfig() {
properties = new Properties();
}

public DataConnectionConfig(DataConnectionConfig config) {
name = config.name;
type = config.type;
shared = config.shared;
properties.putAll(config.getProperties());
properties = PropertiesUtil.clone(config.getProperties());
}

public DataConnectionConfig(String name) {
this();
this.name = checkNotNull(name, "Name must not be null");
}

Expand Down
Expand Up @@ -16,6 +16,7 @@

package com.hazelcast.config;

import com.hazelcast.client.impl.protocol.util.PropertiesUtil;
import com.hazelcast.internal.config.ConfigDataSerializerHook;
import com.hazelcast.map.MapStore;
import com.hazelcast.nio.ObjectDataInput;
Expand Down Expand Up @@ -64,7 +65,7 @@ public class MapStoreConfig implements IdentifiedDataSerializable, Versioned {
private String factoryClassName;
private Object implementation;
private Object factoryImplementation;
private Properties properties = new Properties();
private Properties properties;
private InitialLoadMode initialLoadMode = InitialLoadMode.LAZY;

/**
Expand All @@ -82,6 +83,7 @@ public enum InitialLoadMode {
}

public MapStoreConfig() {
properties = new Properties();
}

public MapStoreConfig(MapStoreConfig config) {
Expand All @@ -95,7 +97,7 @@ public MapStoreConfig(MapStoreConfig config) {
initialLoadMode = config.getInitialLoadMode();
writeCoalescing = config.isWriteCoalescing();
offload = config.isOffload();
properties.putAll(config.getProperties());
properties = PropertiesUtil.clone(config.getProperties());
}

/**
Expand Down Expand Up @@ -282,7 +284,7 @@ public Object getFactoryImplementation() {
}

public MapStoreConfig setProperty(String name, String value) {
properties.put(name, value);
properties.setProperty(name, value);
return this;
}

Expand Down
Expand Up @@ -16,6 +16,7 @@

package com.hazelcast.config;

import com.hazelcast.client.impl.protocol.util.PropertiesUtil;
import com.hazelcast.collection.QueueStore;
import com.hazelcast.collection.QueueStoreFactory;
import com.hazelcast.internal.config.ConfigDataSerializerHook;
Expand Down Expand Up @@ -77,11 +78,12 @@ public class QueueStoreConfig implements IdentifiedDataSerializable {
private boolean enabled = true;
private String className;
private String factoryClassName;
private Properties properties = new Properties();
private Properties properties;
private QueueStore storeImplementation;
private QueueStoreFactory factoryImplementation;

public QueueStoreConfig() {
properties = new Properties();
}

public QueueStoreConfig(QueueStoreConfig config) {
Expand All @@ -90,7 +92,7 @@ public QueueStoreConfig(QueueStoreConfig config) {
storeImplementation = config.getStoreImplementation();
factoryClassName = config.getFactoryClassName();
factoryImplementation = config.getFactoryImplementation();
properties.putAll(config.getProperties());
properties = PropertiesUtil.clone(config.getProperties());
}

/**
Expand Down Expand Up @@ -207,7 +209,7 @@ public String getProperty(String name) {
* @return this configuration
*/
public QueueStoreConfig setProperty(String name, String value) {
properties.put(name, value);
properties.setProperty(name, value);
return this;
}

Expand Down

0 comments on commit 4a84c46

Please sign in to comment.