DanielCarter-stack commented on PR #10427:
URL: https://github.com/apache/seatunnel/pull/10427#issuecomment-3831169266
<!-- code-pr-reviewer -->
<!-- cpr:pr_reply_v2_parts {"group": "apache/seatunnel#10427", "part": 1,
"total": 1} -->
### Issue 1: All Tests Disabled
**Location**:
`seatunnel-connectors-v2/connector-jdbc/src/test/java/org/apache/seatunnel/connectors/seatunnel/jdbc/catalog/kingbase/KingbaseCatalogTest.java:21`
```java
@Disabled("Please Test it in your local environment")
class KingbaseCatalogTest {
// All test methods are disabled
}
```
**Problem Description**:
The test class is disabled by the `@Disabled` annotation, preventing CI from
running these tests. This violates the "covered with tests" promise in the PR
Checklist.
**Potential Risks**:
- Unable to verify code correctness in CI
- Potential regression issues cannot be detected in a timely manner
- Reduced code quality assurance
**Scope of Impact**:
- Direct impact: KingbaseCatalog, KingbaseCreateTableSqlBuilder
- Indirect impact: All features depending on Kingbase Catalog
- Affected area: Kingbase Connector
**Severity**: CRITICAL
**Improvement Suggestions**:
```java
// Remove @Disabled or use Testcontainers
@Testcontainers
class KingbaseCatalogTest {
@Container
private static final PostgreSQLContainer<?> kingbaseContainer =
new PostgreSQLContainer<>("kingbase/kes:latest")
.withDatabaseName("test")
.withUsername("kingbase")
.withPassword("kingbase");
@Test
void databaseExists() {
// Test using real containers
}
}
```
**Reason**: Apache projects require all code changes to be covered by
executable tests.
---
### Issue 2: Hardcoded Chinese Exception Messages
**Location**:
`seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/catalog/kingbase/KingbaseCatalog.java:146`
```java
} catch (SQLException e) {
throw new CatalogException("查询数据库是否存在失败: " + databaseName, e);
}
```
**Problem Description**:
Using hardcoded Chinese strings as exception messages does not comply with
internationalization standards. SeaTunnel is an international project, and
error messages should be in English.
**Related Context**:
- Parent class `AbstractJdbcCatalog` uses English error messages
- Other Catalog implementations (such as `PostgresCatalog`) use English
**Potential Risks**:
- International users cannot understand error messages
- Difficult log analysis
- Inconsistent with project standards
**Scope of Impact**:
- Direct impact: KingbaseCatalog.databaseExists()
- Indirect impact: All user code calling this method
- Affected area: Single Connector
**Severity**: MAJOR
**Improvement Suggestions**:
```java
} catch (SQLException e) {
throw new CatalogException(
String.format("Failed to check if database exists: %s",
databaseName),
e
);
}
```
**Reason**: Complies with Apache project internationalization standards.
---
### Issue 3: Missing Null Check
**Location**:
`seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/catalog/kingbase/KingbaseCatalog.java:196-214`
```java
@Override
protected Column buildColumn(ResultSet resultSet) throws SQLException {
String columnName = resultSet.getString("COLUMN_NAME");
String typeName = resultSet.getString("TYPE_NAME");
String fullTypeName = resultSet.getString("FULL_TYPE_NAME");
// ...
Object defaultValue = resultSet.getObject("DEFAULT_VALUE");
boolean isNullable = resultSet.getString("IS_NULLABLE").equals("YES");
// ...
}
```
**Problem Description**:
There is no null check on the return value of `getString()`. If certain
columns in SQL query results are NULL, it will cause `NullPointerException`.
**Related Context**:
- `SELECT_COLUMNS_SQL_TEMPLATE` uses `LEFT JOIN`, which may produce NULL
values
- Especially `column_comment` and `default_value` fields may be NULL
**Potential Risks**:
- When querying columns containing NULL values, NPE will be thrown
- Users cannot retrieve table metadata
- Job failure
**Scope of Impact**:
- Direct impact: KingbaseCatalog.getTable()
- Indirect impact: All operations using Catalog API to query table metadata
- Affected area: Single Connector
**Severity**: MAJOR
**Improvement Suggestions**:
```java
@Override
protected Column buildColumn(ResultSet resultSet) throws SQLException {
String columnName = resultSet.getString("COLUMN_NAME");
if (columnName == null) {
throw new SQLException("COLUMN_NAME cannot be null");
}
String typeName = resultSet.getString("TYPE_NAME");
String fullTypeName = resultSet.getString("FULL_TYPE_NAME");
long columnLength = resultSet.getLong("COLUMN_LENGTH");
long columnPrecision = resultSet.getLong("COLUMN_PRECISION");
int columnScale = resultSet.getInt("COLUMN_SCALE");
String columnComment = resultSet.getString("COLUMN_COMMENT");
Object defaultValue = resultSet.getObject("DEFAULT_VALUE");
String isNullableStr = resultSet.getString("IS_NULLABLE");
boolean isNullable = "YES".equalsIgnoreCase(isNullableStr);
BasicTypeDefine typeDefine = BasicTypeDefine.builder()
.name(columnName)
.columnType(typeName)
.dataType(typeName)
.length(columnLength)
.precision(columnPrecision)
.scale(columnScale)
.nullable(isNullable)
.defaultValue(defaultValue)
.comment(columnComment)
.build();
return KingbaseTypeConverter.INSTANCE.convert(typeDefine);
}
```
**Reason**: Defensive programming to avoid NPE.
---
### Issue 4: SQL Injection Risk (Potential)
**Location**:
`seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/catalog/kingbase/KingbaseCatalog.java:189-192`
```java
@Override
protected String getSelectColumnsSql(TablePath tablePath) {
return String.format(
SELECT_COLUMNS_SQL_TEMPLATE,
tablePath.getSchemaName(),
tablePath.getTableName());
}
```
**Problem Description**:
Using `String.format` to directly concatenate user input into SQL poses SQL
injection risks. Although there are single quotes wrapping it in
`SELECT_COLUMNS_SQL_TEMPLATE`, the input is not escaped.
**Related Context**:
- Other implementations of `AbstractJdbcCatalog` (such as `PostgresCatalog`)
also use the same pattern
- This is a systemic issue, not specific to this PR
- However, this PR introduces this risk point
**Potential Risks**:
- If schema or table names contain single quotes, SQL syntax errors will
occur
- Malicious users may construct special table names for SQL injection attacks
**Scope of Impact**:
- Direct impact: KingbaseCatalog.getSelectColumnsSql()
- Indirect impact: buildColumn(), getTable()
- Affected area: Single Connector
**Severity**: MAJOR
**Improvement Suggestions**:
```java
@Override
protected String getSelectColumnsSql(TablePath tablePath) {
String schemaName = escapeSqlIdentifier(tablePath.getSchemaName());
String tableName = escapeSqlIdentifier(tablePath.getTableName());
return String.format(SELECT_COLUMNS_SQL_TEMPLATE, schemaName, tableName);
}
private String escapeSqlIdentifier(String identifier) {
return identifier.replace("'", "''");
}
```
**Reason**: Prevent SQL injection, comply with secure coding standards.
---
### Issue 5: KB_CLOB Type Sets columnLength Twice
**Location**:
`seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/internal/dialect/kingbase/KingbaseTypeConverter.java:190-194`
```java
case KB_CLOB:
builder.dataType(BasicType.STRING_TYPE);
builder.columnLength(typeDefine.getLength());
builder.columnLength((long) (1024 * 1024 * 1024)); // Second setup
break;
```
**Problem Description**:
`columnLength` is set twice, the second time will overwrite the first value,
causing `typeDefine.getLength()` to be ignored.
**Related Context**:
- `builder` is Lombok's `@Builder` pattern
- The second call will overwrite the first value
- This is an obvious copy-paste error
**Potential Risks**:
- CLOB type length is always set to 1GB
- Ignores the actual length defined in the database
- May lead to inaccurate type mapping
**Scope of Impact**:
- Direct impact: KB_CLOB type conversion
- Indirect impact: CLOB column metadata
- Affected area: Single Connector
**Severity**: MINOR
**Improvement Suggestions**:
```java
case KB_CLOB:
builder.dataType(BasicType.STRING_TYPE);
if (typeDefine.getLength() != null && typeDefine.getLength() > 0) {
builder.columnLength(typeDefine.getLength());
} else {
builder.columnLength((long) (1024 * 1024 * 1024));
}
break;
```
**Reason**: Fix logic error, preserve user-defined length.
---
### Issue 6: MySqlTypeConverter Constant Visibility Modification Affects
Encapsulation
**Location**:
`seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/internal/dialect/mysql/MySqlTypeConverter.java:45-98`
```java
// Before modification
-static final String MYSQL_INT = "INT";
-static final String MYSQL_DATETIME = "DATETIME";
// ... 38 constants
// After modification
+public static final String MYSQL_INT = "INT";
+public static final String MYSQL_DATETIME = "DATETIME";
// ... 38 constants
```
**Problem Description**:
Changing 38 private constants to public constants increases API surface area
and breaks encapsulation. Although this solves Kingbase's needs, it is not the
optimal solution.
**Related Context**:
- Referenced location: KingbaseTypeConverter.java:73-116
- Caller: `case MySqlTypeConverter.MYSQL_INT:`
- Similar references: OracleTypeConverter.ORACLE_NUMBER,
SqlServerTypeConverter.SQLSERVER_DATETIME2
**Potential Risks**:
- These constants now become part of the public API
- Modifying these constants will affect all callers
- Increases maintenance burden for MySQL, Oracle, and SQL Server
implementations
- Violates the "Principle of Least Knowledge"
**Scope of Impact**:
- Direct impact: MySqlTypeConverter, OracleTypeConverter,
SqlServerTypeConverter
- Indirect impact: Other code that may depend on these constants in the
future
- Affected area: Multiple Connectors
**Severity**: MINOR
**Improvement Suggestions**:
```java
// Create new file: JdbcCompatibilityTypeConstants.java
package org.apache.seatunnel.connectors.seatunnel.jdbc.internal.dialect;
public final class JdbcCompatibilityTypeConstants {
// MySQL compatibility types
public static final String MYSQL_INT = "INT";
public static final String MYSQL_DATETIME = "DATETIME";
// ...
// Oracle compatibility types
public static final String ORACLE_NUMBER = "NUMBER";
// ...
// SQLServer compatibility types
public static final String SQLSERVER_DATETIME2 = "DATETIME2";
// ...
private JdbcCompatibilityTypeConstants() {}
}
```
Then reference in Kingbase:
```java
import static
org.apache.seatunnel.connectors.seatunnel.jdbc.internal.dialect.JdbcCompatibilityTypeConstants.*;
case MYSQL_INT:
builder.dataType(BasicType.INT_TYPE);
break;
```
**Reason**: Centrally manage compatibility type constants, reduce coupling,
improve maintainability.
---
### Issue 8: seatunnel-dist/pom.xml Not Updated
**Location**: `seatunnel-dist/pom.xml`
**Problem Description**:
The PR did not update the `seatunnel-dist/pom.xml` file. According to the
Checklist, when adding a new connector, this file needs to be updated.
**Related Context**:
- Checklist requirement: "Update the pom file of seatunnel-dist"
- Kingbase uses the existing connector-jdbc, so may not need independent
dependencies
**Potential Risks**:
- If Kingbase requires specific JDBC driver dependencies, they need to be
declared in the dist pom
- May result in releases missing necessary drivers
**Scope of Impact**:
- Direct impact: SeaTunnel release
- Indirect impact: Users cannot use Kingbase after downloading the release
- Affected area: Build and distribution
**Severity**: MAJOR
**Improvement Suggestions**:
Check if Kingbase JDBC driver dependency needs to be added:
```xml
<!-- 在 seatunnel-dist/pom.xml 中 -->
<dependency>
<groupId>com.kingbase</groupId>
<artifactId>kingbase8</artifactId>
<version>${kingbase.driver.version}</version>
</dependency>
```
**Reason**: Ensure releases include all necessary dependencies.
---
### Issue 9: Incomplete Documentation
**Location**: `docs/en/connectors/source/Kingbase.md`,
`docs/zh/connectors/source/Kingbase.md`
**Problem Description**:
The documentation only adds Connector configuration items, without
explaining how to use Catalog functionality, supported data type mappings, and
special handling for MySQL compatibility mode.
**Related Context**:
- PR title: "Add Kingbase Catalog Support"
- Documentation only updates the Source configuration section
- Missing Catalog API usage examples
**Potential Risks**:
- Users don't know how to use Kingbase Catalog functionality
- Missing type mapping table may cause usage confusion
- Special handling for MySQL compatibility mode is not documented
**Scope of Impact**:
- Direct impact: Kingbase users
- Indirect impact: User support and troubleshooting
- Affected area: Kingbase Connector
**Severity**: MAJOR
**Improvement Suggestions**:
Add in `docs/en/connectors/source/Kingbase.md`:
```markdown
## Catalog Support
Kingbase connector supports catalog functionality for metadata querying and
automatic table creation.
### Supported Data Types
| Kingbase Type | SeaTunnel Type | Notes |
|---------------|----------------|-------|
| INT | INT | MySQL compatibility mode |
| INTEGER | INT | Native type |
| VARCHAR | STRING | |
| NUMBER | DECIMAL | Oracle compatibility mode |
| ... | ... | |
### MySQL Compatibility Mode
When Kingbase is running in MySQL compatibility mode, the connector will
automatically map MySQL types (e.g., INT, DATETIME) to appropriate SeaTunnel
types.
### Example
```hocon
source {
KingbaseCatalog {
catalog {
database = "test_db"
schema = "public"
}
table_path = "public.my_table"
}
}
```
```
** Reason**: Provide complete usage documentation to reduce user learning
costs.
---
# ## Issue 10: label-scope-conf.yml not updated
** Location**: `.github/workflows/labeler/label-scope-conf.yml`
** Problem Description**:
PR 没有更新 `label-scope-conf.yml` 文件。根据 Checklist，添加新 connector 时需要添加 CI label。
** Related Context**:
- Checklist 要求: "Add ci label in label-scope-conf.yml"
- Kingbase 是新添加的 Catalog 支持
- 需要 CI 系统能够识别 Kingbase 相关的 PR
** Potential Risks**:
- Kingbase 相关的 PR 无法自动打上 label
- 可能导致 CI 流程不正确
** Impact Scope**:
- 直接影响: GitHub Actions CI
- 间接影响: PR 自动化处理
- 影响面: CI/CD 流程
** Severity**: MINOR
** Improvement Suggestions**:
在 `.github/workflows/labeler/label-scope-conf.yml` 中添加:
```yaml
connector-jdbc:
- connector-jdbc/**/*
- "**/*kingbase*"
```
**Reason**: Complies with PR Checklist requirements, ensures correct CI
operation.
---
--
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]
