Allow username and password to be null in DatabaseConfig #201
Conversation
| @@ -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)) | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :( )
| @@ -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)))) | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@feeblefakie Thank you for reviewing this!
Left some comments for your review. Please check them!
| @@ -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)) | |||
There was a problem hiding this comment.
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.
| @@ -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)))) | |||
There was a problem hiding this comment.
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.
Some databases might not need a username and a password, but they are currently required in
DatabaseConfig. This PR will fix it. Please take a look at this!