Copilot commented on code in PR #11165:
URL: https://github.com/apache/seatunnel/pull/11165#discussion_r3472661249
##########
seatunnel-connectors-v2/connector-jdbc/src/test/java/org/apache/seatunnel/connectors/seatunnel/jdbc/catalog/JdbcCatalogFactoryTest.java:
##########
@@ -68,39 +75,42 @@ void testPostgresCatalogValidConfig() {
}
@Test
- void testUrlWithoutDatabaseFails() {
+ void testBlankUrlFails() {
Map<String, Object> cfg = new HashMap<>();
- cfg.put("url", "jdbc:mysql://host:3306");
+ cfg.put("url", "");
cfg.put("username", "root");
cfg.put("password", "pass");
Assertions.assertThrows(OptionValidationException.class, () ->
validate(mysqlRule, cfg));
}
@Test
- void testBlankUrlFails() {
+ void testMissingCredentialsFails() {
Map<String, Object> cfg = new HashMap<>();
- cfg.put("url", "");
- cfg.put("username", "root");
- cfg.put("password", "pass");
+ cfg.put("url", "jdbc:mysql://localhost:3306/mydb");
Assertions.assertThrows(OptionValidationException.class, () ->
validate(mysqlRule, cfg));
}
@Test
- void testMissingUsernameFails() {
+ void testMissingPasswordFails() {
Map<String, Object> cfg = new HashMap<>();
- cfg.put("url", "jdbc:mysql://host:3306/mydb");
- cfg.put("password", "pass");
+ cfg.put("url", "jdbc:mysql://localhost:3306/mydb");
+ cfg.put("username", "root");
Assertions.assertThrows(OptionValidationException.class, () ->
validate(mysqlRule, cfg));
Review Comment:
This test currently asserts that a missing `password` must fail validation,
but the underlying JDBC catalog implementation accepts a null password (it is
only added to connection properties when present). If `PASSWORD` becomes
optional to preserve backward compatibility, this test should be updated
accordingly.
##########
seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/config/JdbcCommonOptions.java:
##########
@@ -162,38 +165,42 @@ public class JdbcCommonOptions {
public static final Option<String> REGION =
Options.key("region").stringType().noDefaultValue().withDescription("region");
- /** @deprecated Use {@link #baseCatalogRule()} instead to avoid shared
mutable state. */
- @Deprecated public static final OptionRule.Builder BASE_CATALOG_RULE =
baseCatalogRule();
-
/**
- * Returns a fresh {@link OptionRule.Builder} with the base validation
rules shared by all JDBC
- * catalog factories (MySQL, PostgreSQL, Oracle, etc.).
+ * Returns a fresh {@link OptionRule.Builder} with the base validation
rules shared by standard
+ * JDBC catalog factories (MySQL, PostgreSQL, etc.) that use generic
{@code host:port/database}
+ * URL format and require username/password authentication.
*
- * <p>These rules are evaluated at <b>submission time</b> via {@code
- * ConfigValidator.validate(factory.optionRule())} in the {@code
FactoryUtil} entry path. They
- * enforce that the JDBC URL contains a database name, and that
username/password are provided.
+ * <p>Catalog factories with non-standard URL formats (SqlServer, Oracle,
SapHana) should use
+ * {@link #baseCatalogRule(ConditionExtension)} with their own URL
validator.
*
- * <p>Individual catalog factories may append additional rules (e.g.
OceanBase requires {@code
- * compatible_mode}) before calling {@code .build()}.
+ * <p>Catalog factories that do not require authentication (e.g. DuckDB)
should define their own
+ * {@code optionRule()} directly.
*/
public static OptionRule.Builder baseCatalogRule() {
+ return baseCatalogRule(new UrlContainsDatabaseValidator());
+ }
+
+ /**
+ * Returns a fresh {@link OptionRule.Builder} with a custom URL validator.
Use this for
+ * databases whose JDBC URL does not follow the standard {@code
host:port/database} format (e.g.
+ * SqlServer, Oracle, SapHana).
+ */
+ public static OptionRule.Builder
baseCatalogRule(ConditionExtension<String> urlValidator) {
return OptionRule.builder()
- .required(URL, Conditions.extension(URL, new
UrlContainsDatabaseValidator()))
+ .required(URL, Conditions.extension(URL, urlValidator))
.required(USERNAME, PASSWORD)
.optional(SCHEMA, DECIMAL_TYPE_NARROWING,
HANDLE_BLOB_AS_STRING);
Review Comment:
`baseCatalogRule` currently marks `PASSWORD` as required, but runtime JDBC
catalogs only enforce a non-blank `username` (see `AbstractJdbcCatalog`
constructor) and treat password as optional (`getConnectionProperties()` only
sets it when non-null). Requiring `password` at optionRule validation time can
break passwordless / trust-auth setups and reintroduce backward-compatibility
issues this PR is trying to fix.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
