Skip to content

feat: introduce validateFields method to SourceConnectionConfig and implement validation for all source configurations #3956

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

Open
pratickchokhani wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: introduce validateFields method to SourceConnectionConfig and implement validation for all source configurations #3956

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 2 additions & 1 deletion ...n/src/main/java/com/google/cloud/teleport/v2/spanner/migrations/shard/CassandraShard.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,8 @@ public CassandraShard(OptionsMap optionsMap) {
extractAndSetHostAndPort();
}

private void validateFields() {
@Override
public void validateFields() {
if (getContactPoints() == null || getContactPoints().isEmpty()) {
throw new IllegalArgumentException("CONTACT_POINTS cannot be null or empty.");
}
Expand Down
33 changes: 33 additions & 0 deletions ...ner-common/src/main/java/com/google/cloud/teleport/v2/spanner/migrations/shard/Shard.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,4 +197,37 @@ public int hashCode() {
secretManagerUri,
dbNameToLogicalShardIdMap);
}

public void validateFields() {
if (logicalShardId == null) {
throw new IllegalArgumentException("logicalShardId cannot be null");
}
if (host == null) {
throw new IllegalArgumentException("host cannot be null");
}
if (port == null) {
throw new IllegalArgumentException("port cannot be null");
}
if (user == null) {
throw new IllegalArgumentException("user cannot be null");
}
if (password == null) {
throw new IllegalArgumentException("password cannot be null");
}
if (dbName == null) {
throw new IllegalArgumentException("dbName cannot be null");
}
if (namespace == null) {
throw new IllegalArgumentException("namespace cannot be null");
}
if (secretManagerUri == null) {
throw new IllegalArgumentException("secretManagerUri cannot be null");

@bharadwaj-aditya bharadwaj-aditya Jul 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

secret manager or password can be null. It is possible to pass only one of them.

Can you please verify the logic here ?

@pratickchokhani pratickchokhani Jul 1, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We have integration tests for both secretManagerUri and password. Unless someone sets the value as null manually in the JSON or HOCON string, these values will not be null.

Hence, this check is added.

@bharadwaj-aditya bharadwaj-aditya Jul 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

2 questions

  1. Is there a scenario where someone would want to set these fields explicitly as null and not empty strings? Maybe for password ?
  2. Does the integration test handle the scenario where all 4 combinations of these 2 parameters are set ?

}
if (connectionProperties == null) {
throw new IllegalArgumentException("connectionProperties cannot be null");
}
if (dbNameToLogicalShardIdMap == null) {
throw new IllegalArgumentException("dbNameToLogicalShardIdMap cannot be null");
}
}
Comment thread
pratickchokhani marked this conversation as resolved.
}
16 changes: 16 additions & 0 deletions .../com/google/cloud/teleport/v2/spanner/migrations/source/config/AstraConnectionConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,4 +54,20 @@ public String getAstraDbRegion() {
public void setAstraDbRegion(String astraDbRegion) {
this.astraDbRegion = astraDbRegion;
}

@Override
public void validateFields() {
if (databaseId == null) {
throw new IllegalArgumentException("databaseId cannot be null");
}
if (astraToken == null) {
throw new IllegalArgumentException("astraToken cannot be null");
}
if (keySpace == null) {
throw new IllegalArgumentException("keySpace cannot be null");
}
if (astraDbRegion == null) {
throw new IllegalArgumentException("astraDbRegion cannot be null");
}
}
Comment thread
pratickchokhani marked this conversation as resolved.
}
7 changes: 7 additions & 0 deletions .../google/cloud/teleport/v2/spanner/migrations/source/config/CassandraConnectionConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,4 +29,11 @@ public CassandraConnectionConfig(OptionsMap optionsMap) {
public OptionsMap getOptionsMap() {
return optionsMap;
}

@Override
public void validateFields() {
if (optionsMap == null) {
throw new IllegalArgumentException("optionsMap cannot be null");
}
}
}
10 changes: 10 additions & 0 deletions ...n/java/com/google/cloud/teleport/v2/spanner/migrations/source/config/JdbcShardConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,4 +33,14 @@ public List<Shard> getShardConfigs() {
public void setShardConfigs(List<Shard> shardConfigs) {
this.shardConfigs = shardConfigs;
}

@Override
public void validateFields() {
if (shardConfigs == null || shardConfigs.isEmpty()) {
Comment thread
pratickchokhani marked this conversation as resolved.
throw new IllegalArgumentException("shardConfigs cannot be null or empty");
}
for (Shard shard : shardConfigs) {
shard.validateFields();
}
}
}
15 changes: 11 additions & 4 deletions ...ava/com/google/cloud/teleport/v2/spanner/migrations/source/config/SourceConfigParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,27 +69,34 @@ public SourceConnectionConfig parseConfiguration(
String sourceTypeStr, String sourceConfigFilePath) throws Exception {

SourceType sourceType = SourceType.parseSourceType(sourceTypeStr);
switch (SourceType.parseSourceType(sourceTypeStr)) {
SourceConnectionConfig sourceConfig;
switch (sourceType) {
case CASSANDRA:
// Maps directly to the DataStax OptionsMap
return new CassandraConnectionConfig(
CassandraDriverConfigLoader.getOptionsMapFromFile(sourceConfigFilePath));
sourceConfig =
new CassandraConnectionConfig(
CassandraDriverConfigLoader.getOptionsMapFromFile(sourceConfigFilePath));
break;
case ASTRA_DB:
String astraFileContent = FileLoader.readConfigFilePath(sourceConfigFilePath);
Map<String, Object> astraConfigMap = parseConfigToConfigMap(astraFileContent);
return mapper.convertValue(astraConfigMap, AstraConnectionConfig.class);
sourceConfig = mapper.convertValue(astraConfigMap, AstraConnectionConfig.class);
break;
case MYSQL:
case PG:
String jdbcFileContent = FileLoader.readConfigFilePath(sourceConfigFilePath);
Map<String, Object> jdbcConfigMap = parseConfigToConfigMap(jdbcFileContent);
JdbcShardConfig jdbcShardConfig = mapper.convertValue(jdbcConfigMap, JdbcShardConfig.class);
jdbcShardConfig.validateFields();
// Returns ordered list of shards
jdbcShardConfig.getShardConfigs().sort(Comparator.comparing(Shard::getLogicalShardId));
resolveShardSecret(jdbcShardConfig, sourceConfigFilePath);
return jdbcShardConfig;
default:
throw new IllegalArgumentException("Unsupported source type: " + sourceType);
}
sourceConfig.validateFields();
return sourceConfig;
}

/**
Expand Down
9 changes: 8 additions & 1 deletion ...com/google/cloud/teleport/v2/spanner/migrations/source/config/SourceConnectionConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,4 +23,11 @@
*
* <p>This interface will be expanded in Phase 2 during the platformization phase.
*/
public interface SourceConnectionConfig {}
public interface SourceConnectionConfig {

/**
* Validates that the source configuration is valid. Throws IllegalArgumentException if the
* configuration is invalid.
*/
void validateFields();

@bharadwaj-aditya bharadwaj-aditya Jul 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If the validation is implemented at the source, then it can be private.

This method should not be required.

@pratickchokhani pratickchokhani Jul 1, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This only validates the null checks and basic checks. It is called by the SourceConfigParser before returning the config.

Interface fields cannot be private.

@bharadwaj-aditya bharadwaj-aditya Jul 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If validate is called in each of the flows right after reading the config, then this validation is not really achieving anything. Especially if the code of what to validate is within the source implementation.

The value of the validate would be if it lies outside of the source implementation. Can you think of a scenario where the source specific validation can not be done while reading the source config ?

}
31 changes: 31 additions & 0 deletions ...com/google/cloud/teleport/v2/spanner/migrations/source/config/SourceConfigParserTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import static org.mockito.Mockito.when;

import com.google.cloud.teleport.v2.spanner.migrations.shard.Shard;
import com.google.cloud.teleport.v2.spanner.migrations.utils.CassandraDriverConfigLoader;
import com.google.cloud.teleport.v2.spanner.migrations.utils.ISecretManagerAccessor;
import com.google.cloud.teleport.v2.spanner.migrations.utils.JarFileReader;
import com.typesafe.config.ConfigException;
Expand Down Expand Up @@ -381,4 +382,34 @@ public void testParseConfiguration_UnsupportedSourceType() {
() -> parser.parseConfiguration("unsupported_db", "dummy/path.json"));
assertEquals("Unsupported source type: unsupported_db", exception.getMessage());
}

@Test
public void testParseConfiguration_Cassandra_ValidationFails() throws Exception {
try (MockedStatic<CassandraDriverConfigLoader> mockLoader =
mockStatic(CassandraDriverConfigLoader.class)) {
String testGcsPath = "gs://smt-test-bucket/cassandraConfig.conf";
mockLoader
.when(() -> CassandraDriverConfigLoader.getOptionsMapFromFile(testGcsPath))
.thenReturn(null);

Exception exception =
assertThrows(
IllegalArgumentException.class,
() -> parser.parseConfiguration("cassandra", testGcsPath));
assertEquals("optionsMap cannot be null", exception.getMessage());
}
}

@Test
public void testParseConfiguration_Jdbc_EmptyShardConfigs() throws Exception {
File tempFile = tempFolder.newFile("jdbc-empty-shards-config.json");
String jdbcJson = "{\n \"shardConfigs\": []\n}";
Files.writeString(tempFile.toPath(), jdbcJson);

Exception exception =
assertThrows(
IllegalArgumentException.class,
() -> parser.parseConfiguration("mysql", tempFile.getAbsolutePath()));
assertEquals("shardConfigs cannot be null or empty", exception.getMessage());
}
}
Loading