Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow username and password to be null in DatabaseConfig #201

Merged
merged 2 commits into from
May 20, 2021
Merged

Allow username and password to be null in DatabaseConfig #201

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5cf92cb
Allow username and password to be null in DatabaseConfig
brfrn169 May 17, 2021
083ef83
Merge branch 'master' into allow-username-password-to-be-null
feeblefakie May 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 6 additions & 8 deletions src/main/java/com/scalar/db/config/DatabaseConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,8 @@ public class DatabaseConfig {
private final Properties props;
private List<String> contactPoints;
private int contactPort;
private String username;
private String password;
private Optional<String> username;
private Optional<String> password;
private Class<? extends DistributedStorage> storageClass;
private Optional<String> namespacePrefix;
private Isolation isolation = Isolation.SNAPSHOT;
Expand Down Expand Up @@ -91,8 +91,6 @@ protected void load() {

if (storageClass != MultiStorage.class) {
checkNotNull(props.getProperty(CONTACT_POINTS));
checkNotNull(props.getProperty(USERNAME));
checkNotNull(props.getProperty(PASSWORD));

contactPoints = Arrays.asList(props.getProperty(CONTACT_POINTS).split(","));
if (props.getProperty(CONTACT_PORT) == null) {
Expand All @@ -101,8 +99,8 @@ protected void load() {
contactPort = Integer.parseInt(props.getProperty(CONTACT_PORT));
checkArgument(contactPort > 0);
}
username = props.getProperty(USERNAME);
password = props.getProperty(PASSWORD);
username = Optional.ofNullable(props.getProperty(USERNAME));
password = Optional.ofNullable(props.getProperty(PASSWORD));

if (Strings.isNullOrEmpty(props.getProperty(NAMESPACE_PREFIX))) {
namespacePrefix = Optional.empty();
Expand All @@ -129,11 +127,11 @@ public int getContactPort() {
return contactPort;
}

public String getUsername() {
public Optional<String> getUsername() {
return username;
}

public String getPassword() {
public Optional<String> getPassword() {
return password;
}

Expand Down
4 changes: 2 additions & 2 deletions src/main/java/com/scalar/db/storage/cassandra/ClusterManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,8 +108,8 @@ void build() {

@VisibleForTesting
Cluster getCluster(DatabaseConfig config) {
if (config.getUsername() != null && config.getPassword() != null) {
builder.withCredentials(config.getUsername(), config.getPassword());
if (config.getUsername().isPresent() && config.getPassword().isPresent()) {
builder.withCredentials(config.getUsername().get(), config.getPassword().get());
}
return builder.build();
}
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/com/scalar/db/storage/cosmos/Cosmos.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ public Cosmos(DatabaseConfig config) {
this.client =
new CosmosClientBuilder()
.endpoint(config.getContactPoints().get(0))
.key(config.getPassword())
.key(config.getPassword().orElse(null))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any way it can work well without a password?
If there is no way, I'm wondering if it should check it here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The key method throws NullPointerException with an error message ("'key' cannot be null.") when the password is EMPTY. I thought it was okay because we basically delegate the configuration parameter check to the db client side (in this case CosmosClientBuilder). An invalid password (including null) will cause something error on CosmosClient side.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! Ok, if it is delegating the check to the builder, then I think it is fine. (I want the builder to throw IllegalArgumentException though :( )

.directMode()
.consistencyLevel(ConsistencyLevel.STRONG)
.buildClient();
Expand Down
3 changes: 2 additions & 1 deletion src/main/java/com/scalar/db/storage/dynamo/Dynamo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,8 @@ public Dynamo(DatabaseConfig config) {
DynamoDbClient.builder()
.credentialsProvider(
StaticCredentialsProvider.create(
AwsBasicCredentials.create(config.getUsername(), config.getPassword())))
AwsBasicCredentials.create(
config.getUsername().orElse(null), config.getPassword().orElse(null))))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If both are null, does it assume that the code uses credentials stored in a file (like ~/aws/config) implicitly?
I'm just wondering if there is a case where it works without a username and a password.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If any of them is null, the DynamoDbClient builder will throw NullPointerException with a message. I thought it was okay as the same reason above. BTW, the current implementation doesn't check username and password, so if username or password is empty, we will also face NullPointerException.

.region(Region.of(config.getContactPoints().get(0)))
.build();

Expand Down
10 changes: 5 additions & 5 deletions src/main/java/com/scalar/db/storage/jdbc/JdbcUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,11 @@
package com.scalar.db.storage.jdbc;

import com.microsoft.sqlserver.jdbc.SQLServerDriver;
import oracle.jdbc.OracleDriver;
import org.apache.commons.dbcp2.BasicDataSource;

import java.sql.Connection;
import java.sql.Driver;
import java.sql.SQLException;
import oracle.jdbc.OracleDriver;
import org.apache.commons.dbcp2.BasicDataSource;

public final class JdbcUtils {
private JdbcUtils() {}
Expand Down Expand Up @@ -41,8 +40,9 @@ public static BasicDataSource initDataSource(JdbcDatabaseConfig config, boolean
dataSource.setDriver(getDriverClass(jdbcUrl));

dataSource.setUrl(jdbcUrl);
dataSource.setUsername(config.getUsername());
dataSource.setPassword(config.getPassword());

config.getUsername().ifPresent(dataSource::setUsername);
config.getPassword().ifPresent(dataSource::setPassword);

if (transactional) {
dataSource.setDefaultAutoCommit(false);
Expand Down
82 changes: 66 additions & 16 deletions src/test/java/com/scalar/db/config/DatabaseConfigTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
import com.scalar.db.storage.cassandra.Cassandra;
import com.scalar.db.storage.cosmos.Cosmos;
import com.scalar.db.transaction.consensuscommit.SerializableStrategy;
import java.util.Arrays;
import java.util.Collections;
import java.util.Properties;
import org.junit.Test;

Expand All @@ -29,10 +29,52 @@ public void constructor_PropertiesWithoutPortGiven_ShouldLoadProperly() {
DatabaseConfig config = new DatabaseConfig(props);

// Assert
assertThat(config.getContactPoints()).isEqualTo(Arrays.asList(ANY_HOST));
assertThat(config.getContactPoints()).isEqualTo(Collections.singletonList(ANY_HOST));
assertThat(config.getContactPort()).isEqualTo(0);
assertThat(config.getUsername()).isEqualTo(ANY_USERNAME);
assertThat(config.getPassword()).isEqualTo(ANY_PASSWORD);
assertThat(config.getUsername().isPresent()).isTrue();
assertThat(config.getUsername().get()).isEqualTo(ANY_USERNAME);
assertThat(config.getPassword().isPresent()).isTrue();
assertThat(config.getPassword().get()).isEqualTo(ANY_PASSWORD);
assertThat(config.getStorageClass()).isEqualTo(Cassandra.class);
}

@Test
public void constructor_PropertiesWithoutUsernameGiven_ShouldLoadProperly() {
// Arrange
Properties props = new Properties();
props.setProperty(DatabaseConfig.CONTACT_POINTS, ANY_HOST);
props.setProperty(DatabaseConfig.CONTACT_PORT, Integer.toString(ANY_PORT));
props.setProperty(DatabaseConfig.PASSWORD, ANY_PASSWORD);

// Act
DatabaseConfig config = new DatabaseConfig(props);

// Assert
assertThat(config.getContactPoints()).isEqualTo(Collections.singletonList(ANY_HOST));
assertThat(config.getContactPort()).isEqualTo(ANY_PORT);
assertThat(config.getUsername().isPresent()).isFalse();
assertThat(config.getPassword().isPresent()).isTrue();
assertThat(config.getPassword().get()).isEqualTo(ANY_PASSWORD);
assertThat(config.getStorageClass()).isEqualTo(Cassandra.class);
}

@Test
public void constructor_PropertiesWithoutPasswordGiven_ShouldLoadProperly() {
// Arrange
Properties props = new Properties();
props.setProperty(DatabaseConfig.CONTACT_POINTS, ANY_HOST);
props.setProperty(DatabaseConfig.CONTACT_PORT, Integer.toString(ANY_PORT));
props.setProperty(DatabaseConfig.USERNAME, ANY_USERNAME);

// Act
DatabaseConfig config = new DatabaseConfig(props);

// Assert
assertThat(config.getContactPoints()).isEqualTo(Collections.singletonList(ANY_HOST));
assertThat(config.getContactPort()).isEqualTo(ANY_PORT);
assertThat(config.getUsername().isPresent()).isTrue();
assertThat(config.getUsername().get()).isEqualTo(ANY_USERNAME);
assertThat(config.getPassword().isPresent()).isFalse();
assertThat(config.getStorageClass()).isEqualTo(Cassandra.class);
}

Expand All @@ -49,10 +91,12 @@ public void constructor_PropertiesWithPortGiven_ShouldLoadProperly() {
DatabaseConfig config = new DatabaseConfig(props);

// Assert
assertThat(config.getContactPoints()).isEqualTo(Arrays.asList(ANY_HOST));
assertThat(config.getContactPoints()).isEqualTo(Collections.singletonList(ANY_HOST));
assertThat(config.getContactPort()).isEqualTo(ANY_PORT);
assertThat(config.getUsername()).isEqualTo(ANY_USERNAME);
assertThat(config.getPassword()).isEqualTo(ANY_PASSWORD);
assertThat(config.getUsername().isPresent()).isTrue();
assertThat(config.getUsername().get()).isEqualTo(ANY_USERNAME);
assertThat(config.getPassword().isPresent()).isTrue();
assertThat(config.getPassword().get()).isEqualTo(ANY_PASSWORD);
}

@Test
Expand Down Expand Up @@ -98,10 +142,12 @@ public void constructor_PropertiesWithCosmosGiven_ShouldLoadProperly() {
DatabaseConfig config = new DatabaseConfig(props);

// Assert
assertThat(config.getContactPoints()).isEqualTo(Arrays.asList(ANY_HOST));
assertThat(config.getContactPoints()).isEqualTo(Collections.singletonList(ANY_HOST));
assertThat(config.getContactPort()).isEqualTo(0);
assertThat(config.getUsername()).isEqualTo(ANY_USERNAME);
assertThat(config.getPassword()).isEqualTo(ANY_PASSWORD);
assertThat(config.getUsername().isPresent()).isTrue();
assertThat(config.getUsername().get()).isEqualTo(ANY_USERNAME);
assertThat(config.getPassword().isPresent()).isTrue();
assertThat(config.getPassword().get()).isEqualTo(ANY_PASSWORD);
assertThat(config.getStorageClass()).isEqualTo(Cosmos.class);
}

Expand Down Expand Up @@ -135,10 +181,12 @@ public void constructor_PropertiesWithIsolationLevelGiven_ShouldLoadProperly() {
DatabaseConfig config = new DatabaseConfig(props);

// Assert
assertThat(config.getContactPoints()).isEqualTo(Arrays.asList(ANY_HOST));
assertThat(config.getContactPoints()).isEqualTo(Collections.singletonList(ANY_HOST));
assertThat(config.getContactPort()).isEqualTo(0);
assertThat(config.getUsername()).isEqualTo(ANY_USERNAME);
assertThat(config.getPassword()).isEqualTo(ANY_PASSWORD);
assertThat(config.getUsername().isPresent()).isTrue();
assertThat(config.getUsername().get()).isEqualTo(ANY_USERNAME);
assertThat(config.getPassword().isPresent()).isTrue();
assertThat(config.getPassword().get()).isEqualTo(ANY_PASSWORD);
assertThat(config.getIsolation()).isEqualTo(Isolation.SERIALIZABLE);
}

Expand Down Expand Up @@ -173,10 +221,12 @@ public void constructor_PropertiesWithSerializableStrategyGiven_ShouldLoadProper
DatabaseConfig config = new DatabaseConfig(props);

// Assert
assertThat(config.getContactPoints()).isEqualTo(Arrays.asList(ANY_HOST));
assertThat(config.getContactPoints()).isEqualTo(Collections.singletonList(ANY_HOST));
assertThat(config.getContactPort()).isEqualTo(0);
assertThat(config.getUsername()).isEqualTo(ANY_USERNAME);
assertThat(config.getPassword()).isEqualTo(ANY_PASSWORD);
assertThat(config.getUsername().isPresent()).isTrue();
assertThat(config.getUsername().get()).isEqualTo(ANY_USERNAME);
assertThat(config.getPassword().isPresent()).isTrue();
assertThat(config.getPassword().get()).isEqualTo(ANY_PASSWORD);
assertThat(config.getSerializableStrategy()).isEqualTo(SerializableStrategy.EXTRA_WRITE);
}

Expand Down
27 changes: 16 additions & 11 deletions src/test/java/com/scalar/db/storage/jdbc/JdbcDatabaseConfigTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,12 @@
package com.scalar.db.storage.jdbc;

import com.scalar.db.config.DatabaseConfig;
import org.junit.Test;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;

import com.scalar.db.config.DatabaseConfig;
import java.util.Collections;
import java.util.Properties;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
import org.junit.Test;

public class JdbcDatabaseConfigTest {

Expand Down Expand Up @@ -41,8 +40,10 @@ public void constructor_AllPropertiesGiven_ShouldLoadProperly() {
// Assert
assertThat(config.getContactPoints()).isEqualTo(Collections.singletonList(ANY_JDBC_URL));
assertThat(config.getContactPort()).isEqualTo(0);
assertThat(config.getUsername()).isEqualTo(ANY_USERNAME);
assertThat(config.getPassword()).isEqualTo(ANY_PASSWORD);
assertThat(config.getUsername().isPresent()).isTrue();
assertThat(config.getUsername().get()).isEqualTo(ANY_USERNAME);
assertThat(config.getPassword().isPresent()).isTrue();
assertThat(config.getPassword().get()).isEqualTo(ANY_PASSWORD);
assertThat(config.getNamespacePrefix().isPresent()).isTrue();
assertThat(config.getNamespacePrefix().get()).isEqualTo(ANY_NAMESPACE_PREFIX + "_");
assertThat(config.getStorageClass()).isEqualTo(JdbcDatabase.class);
Expand Down Expand Up @@ -72,8 +73,10 @@ public void constructor_AllPropertiesGiven_ShouldLoadProperly() {
// Assert
assertThat(config.getContactPoints()).isEqualTo(Collections.singletonList(ANY_JDBC_URL));
assertThat(config.getContactPort()).isEqualTo(0);
assertThat(config.getUsername()).isEqualTo(ANY_USERNAME);
assertThat(config.getPassword()).isEqualTo(ANY_PASSWORD);
assertThat(config.getUsername().isPresent()).isTrue();
assertThat(config.getUsername().get()).isEqualTo(ANY_USERNAME);
assertThat(config.getPassword().isPresent()).isTrue();
assertThat(config.getPassword().get()).isEqualTo(ANY_PASSWORD);
assertThat(config.getNamespacePrefix().isPresent()).isTrue();
assertThat(config.getNamespacePrefix().get()).isEqualTo(ANY_NAMESPACE_PREFIX + "_");
assertThat(config.getStorageClass()).isEqualTo(JdbcDatabase.class);
Expand Down Expand Up @@ -129,8 +132,10 @@ public void constructor_PropertiesWithWrongStorage_ShouldThrowIllegalArgumentExc
// Assert
assertThat(config.getContactPoints()).isEqualTo(Collections.singletonList(ANY_JDBC_URL));
assertThat(config.getContactPort()).isEqualTo(0);
assertThat(config.getUsername()).isEqualTo(ANY_USERNAME);
assertThat(config.getPassword()).isEqualTo(ANY_PASSWORD);
assertThat(config.getUsername().isPresent()).isTrue();
assertThat(config.getUsername().get()).isEqualTo(ANY_USERNAME);
assertThat(config.getPassword().isPresent()).isTrue();
assertThat(config.getPassword().get()).isEqualTo(ANY_PASSWORD);
assertThat(config.getNamespacePrefix().isPresent()).isTrue();
assertThat(config.getNamespacePrefix().get()).isEqualTo(ANY_NAMESPACE_PREFIX + "_");
assertThat(config.getStorageClass()).isEqualTo(JdbcDatabase.class);
Expand Down
12 changes: 8 additions & 4 deletions src/test/java/com/scalar/db/storage/multistorage/MultiStorageConfigTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,17 +47,21 @@ public void constructor_AllPropertiesGiven_ShouldLoadProperly() {
assertThat(c.getStorageClass()).isEqualTo(Cassandra.class);
assertThat(c.getContactPoints()).isEqualTo(Collections.singletonList("localhost"));
assertThat(c.getContactPort()).isEqualTo(7000);
assertThat(c.getUsername()).isEqualTo("cassandra");
assertThat(c.getPassword()).isEqualTo("cassandra");
assertThat(c.getUsername().isPresent()).isTrue();
assertThat(c.getUsername().get()).isEqualTo("cassandra");
assertThat(c.getPassword().isPresent()).isTrue();
assertThat(c.getPassword().get()).isEqualTo("cassandra");
assertThat(c.getNamespacePrefix().isPresent()).isTrue();
assertThat(c.getNamespacePrefix().get()).isEqualTo("prefix_");
assertThat(config.getDatabaseConfigMap().containsKey("mysql")).isTrue();
c = config.getDatabaseConfigMap().get("mysql");
assertThat(c.getStorageClass()).isEqualTo(JdbcDatabase.class);
assertThat(c.getContactPoints())
.isEqualTo(Collections.singletonList("jdbc:mysql://localhost:3306/"));
assertThat(c.getUsername()).isEqualTo("root");
assertThat(c.getPassword()).isEqualTo("mysql");
assertThat(c.getUsername().isPresent()).isTrue();
assertThat(c.getUsername().get()).isEqualTo("root");
assertThat(c.getPassword().isPresent()).isTrue();
assertThat(c.getPassword().get()).isEqualTo("mysql");
assertThat(c.getNamespacePrefix().isPresent()).isFalse();

assertThat(config.getTableStorageMap().size()).isEqualTo(3);
Expand Down