DanielCarter-stack commented on PR #10499:
URL: https://github.com/apache/seatunnel/pull/10499#issuecomment-3916857258
<!-- code-pr-reviewer -->
<!-- cpr:pr_reply_v2_parts {"group": "apache/seatunnel#10499", "part": 1,
"total": 1} -->
## Issue 1: Test assertion for getUpsertStatement may be inaccurate
**Location**: `DmdbDialectTest.java:79-87`
**Related context**:
- Parent class method: `JdbcDialect.java:118-120` (tableIdentifier default
implementation)
- Implementation class: `DmdbDialect.java:160-168` (tableIdentifier override
implementation)
- Method under test: `DmdbDialect.java:81-141` (getUpsertStatement)
**Issue description**:
In the SQL of the test assertion, the database part (`testdb`) is not
enclosed in quotes, but according to the implementation of the
`DmdbDialect.tableIdentifier` method, it calls the parent class's
`quoteDatabaseIdentifier` (which returns the original string by default). The
test should verify this behavior, rather than assuming that the database name
will be surrounded by quotes. Although the current implementation does not add
quotes to database names, the test should explicitly reflect this behavior,
rather than passing by coincidence.
**Potential risks**:
- If the implementation of `quoteDatabaseIdentifier` changes in the future,
the test may not detect it in time
- Maintainers may misunderstand the quoting rules for database names in
Dameng database
**Impact scope**:
- Direct impact: `DmdbDialectTest` test class
- Indirect impact: Understanding of SQL generation logic for Dameng database
**Severity**: MINOR
**Improvement suggestion**:
```java
@Test
public void testGetUpsertStatement() {
DmdbDialect dialect = new DmdbDialect(FieldIdeEnum.ORIGINAL.getValue());
String database = "testdb";
String tableName = "users";
String[] fieldNames = {"id", "name", "age"};
String[] uniqueKeyFields = {"id"};
Optional<String> upsertSQL =
dialect.getUpsertStatement(database, tableName, fieldNames,
uniqueKeyFields);
Assertions.assertTrue(upsertSQL.isPresent());
String sql = upsertSQL.get();
Assertions.assertTrue(sql.contains("MERGE INTO"));
Assertions.assertTrue(sql.contains("TARGET"));
Assertions.assertTrue(sql.contains("SOURCE"));
Assertions.assertTrue(sql.contains("WHEN MATCHED THEN"));
Assertions.assertTrue(sql.contains("UPDATE SET"));
Assertions.assertTrue(sql.contains("WHEN NOT MATCHED THEN"));
Assertions.assertTrue(sql.contains("INSERT"));
// Note: Database names will not be surrounded by quotes (because
quoteDatabaseIdentifier is not overridden)
Assertions.assertEquals(
" MERGE INTO testdb.\"users\" TARGET"
+ " USING (SELECT :id \"id\", :name \"name\", :age
\"age\") SOURCE"
+ " ON (TARGET.\"id\"=SOURCE.\"id\") "
+ " WHEN MATCHED THEN"
+ " UPDATE SET TARGET.\"name\"=SOURCE.\"name\",
TARGET.\"age\"=SOURCE.\"age\""
+ " WHEN NOT MATCHED THEN"
+ " INSERT (\"id\", \"name\", \"age\") VALUES
(SOURCE.\"id\", SOURCE.\"name\", SOURCE.\"age\")",
sql);
}
```
**Rationale**:
Add explicit comments explaining why database names are not quoted, making
the test intent clearer.
---
## Issue 2: junit-bom dependency configuration error in pom.xml
**Location**: `pom.xml:261-269`
**Related context**:
- Parent POM: `/pom.xml` (junit5.version = 5.9.0)
- Sibling dependency: `pom.xml:271-277` (junit-jupiter)
- Sibling dependency: `pom.xml:456-459` (junit-jupiter-api)
**Issue description**:
When declaring `junit-bom` in the `dependencyManagement` section,
`scope=import` is set, which is correct usage. However, `junit-bom` should not
be actually referenced in the `dependencies` section. The purpose of a BOM is
to uniformly manage dependency versions and should not be introduced as an
actual dependency. Although the current configuration will not cause build
failures, this is a configuration error that may cause confusion.
**Potential risks**:
- May cause IDE or Maven plugin warnings
- Other developers may misunderstand the usage of BOM
- May result in unnecessary BOM dependencies appearing in the dependency tree
**Impact scope**:
- Direct impact: `connector-jdbc/pom.xml` configuration
- Indirect impact: All projects that depend on this module
**Severity**: MINOR
**Improvement suggestion**:
Keep the configuration in `dependencyManagement`, but ensure that the BOM is
not referenced in `dependencies`:
```xml
<dependencyManagement>
<dependencies>
<!-- 其他依赖管理 -->
<dependency>
<groupId>org.junit</groupId>
<artifactId>junit-bom</artifactId>
<version>${junit5.version}</version>
<type>pom</type>
<scope>import</scope>
</dependency>
</dependencies>
</dependencyManagement>
<dependencies>
<!-- 只引用实际的依赖，不要引用 BOM -->
<dependency>
<groupId>org.junit.jupiter</groupId>
<artifactId>junit-jupiter</artifactId>
<scope>test</scope>
</dependency>
<!-- 不需要引用 junit-bom -->
</dependencies>
```
**Rationale**:
BOM should only be used in `dependencyManagement` and should not appear in
`dependencies`.
---
## Issue 3: postgresql testcontainers dependency was added by mistake
**Location**: `pom.xml:293-299`
**Related context**:
- Sibling dependency: `pom.xml:255-273` (testcontainers core dependency)
- Test class: `AbstractDamengContainerTest.java:50` (uses Dameng image)
- Test class: `DamengDialectContainerTest.java` (Dameng database test)
**Issue description**:
In the Dameng database connector's pom.xml, the `testcontainers-postgresql`
dependency was added, which is completely unrelated. Dameng database uses its
own JDBC driver and Docker image, and does not need PostgreSQL's test
container. This may be an error caused by copying and pasting from other test
configurations.
**Potential risks**:
- Increases unnecessary dependencies, increasing build time and classpath
size
- May cause version conflicts
- May mislead other developers into thinking this is a required dependency
**Impact scope**:
- Direct impact: `connector-jdbc/pom.xml` dependency configuration
- Indirect impact: Build performance
**Severity**: MINOR
**Improvement suggestion**:
Remove this dependency:
```xml
<!-- 删除这个依赖 -->
<!-- <dependency> -->
<!-- <groupId>org.testcontainers</groupId> -->
<!-- <artifactId>postgresql</artifactId> -->
<!-- <version>${testcontainer.version}</version> -->
<!-- <scope>test</scope> -->
<!-- </dependency> -->
```
**Rationale**:
Dameng database tests do not need PostgreSQL test container support.
---
## Issue 4: Using non-official Docker images for testing
**Location**: `AbstractDamengContainerTest.java:50`
**Related context**:
- Usage: `AbstractDamengContainerTest.java:62-73` (start container)
- Test cases: `DamengDialectContainerTest.java` (all test methods)
**Issue description**:
The Docker image used by the test `onlyoffice/damengdb:8.1.2` comes from the
onlyoffice organization, not the official Dameng organization. Need to verify:
1. The maintenance status and quality of the image
2. Whether the image version is consistent with the production environment
3. Whether there is an official or more reliable image available
4. Whether the image's license is suitable for use in Apache projects
**Potential risks**:
- The image may be removed or updated, causing test failures
- The image may contain security vulnerabilities
- The image behavior may be inconsistent with the production environment's
Dameng database
- License issues
**Impact scope**:
- Direct impact: All integration tests
- Indirect impact: CI/CD stability
**Severity**: MAJOR
**Improvement suggestion**:
1. Investigate whether there is an official Dameng database Docker image
2. Document the image source and selection reason in JavaDoc
3. Add image availability checks and retry logic
4. Consider using `@EnabledIfSystemProperty` or
`@DisabledIfEnvironmentVariable` to control test execution
```java
/**
* Base class for Dameng (DM8) tests using Testcontainers.
*
* <p>Note: This test uses the onlyoffice/damengdb:8.1.2 Docker image as
it's one of the
* few publicly available DM8 images. Official Dameng images require
registration and
* are not suitable for automated CI/CD. Tests may be disabled if Docker is
unavailable.
*
* @see <a href="https://hub.docker.com/r/onlyoffice/damengdb";>Docker Hub</a>
*/
@DisabledOnOs(OS.WINDOWS)
@Testcontainers
public abstract class AbstractDamengContainerTest {
// ...
}
```
**Rationale**:
Explicitly document the image source and limitations so that maintainers and
users understand the potential risks.
---
## Issue 5: Duplicate configuration of maven-compiler-plugin
**Location**: `pom.xml:477-486`
**Related context**:
- Root POM: `/pom.xml` (java.version = 1.8 already defined)
- Parent POM possibly: already configured maven-compiler-plugin
**Issue description**:
In connector-jdbc's pom.xml, `maven-compiler-plugin` is redefined, which is
unnecessary. The root POM has already set `java.version`,
`maven.compiler.source` and `maven.compiler.target`, and child modules should
inherit these configurations rather than redefine them.
**Potential risks**:
- May conflict with parent POM configuration
- Violates the DRY principle
- If the root POM's Java version is updated, child modules need to be
updated synchronously
**Impact scope**:
- Direct impact: Compilation configuration of `connector-jdbc`
- Indirect impact: Consistency with other modules in the project
**Severity**: MINOR
**Improvement suggestion**:
Remove the duplicate configuration:
```xml
<!-- 删除这个 build 配置 -->
<!-- <build> -->
<!-- <plugins> -->
<!-- <plugin> -->
<!-- <groupId>org.apache.maven.plugins</groupId> -->
<!-- <artifactId>maven-compiler-plugin</artifactId> -->
<!-- <configuration> -->
<!-- <source>${java.version}</source> -->
<!-- <target>${java.version}</target> -->
<!-- </configuration> -->
<!-- </plugin> -->
<!-- </plugins> -->
<!-- </build> -->
```
**Rationale**:
Should inherit the compiler configuration from the parent POM, avoiding
duplication and inconsistency.
---
## Issue 6: Integration tests lack error handling for container startup
failures
**Location**: `AbstractDamengContainerTest.java:60-79`
**Related context**:
- Cleanup method: `AbstractDamengContainerTest.java:81-87`
- Test cases: All tests in `DamengDialectContainerTest`
**Issue description**:
The test code uses `@BeforeAll` to start the container, but does not handle
container startup failures. If Docker is unavailable or image pull fails, all
tests will fail. Lacks retry logic and more friendly error messages.
**Potential risks**:
- In environments without Docker, test failure messages are unclear
- Temporary network issues may cause the entire test suite to fail
- May produce zombie containers
**Impact scope**:
- Direct impact: Stability of all integration tests
- Indirect impact: CI/CD reliability
**Severity**: MINOR
**Improvement suggestion**:
Add error handling and assumptions:
```java
@BeforeAll
static void startContainer() {
try {
DM_CONTAINER =
new GenericContainer<>(DockerImageName.parse(DM_IMAGE))
.withExposedPorts(DM_PORT)
.withEnv("PAGE_SIZE", "16")
.withEnv("LD_LIBRARY_PATH", "/opt/dmdbms/bin")
.withEnv("EXTENT_SIZE", "32")
.withEnv("BLANK_PAD_MODE", "1")
.withEnv("LOG_SIZE", "1024")
.withEnv("UNICODE_FLAG", "1")
.withEnv("INSTANCE_NAME", "dm8_test")
.waitingFor(Wait.forLogMessage(".*SYSTEM IS
READY.*\\n", 1))
.withStartupTimeout(Duration.ofMinutes(5));
DM_CONTAINER.start();
log.info(
"Dameng container started at: {}:{}",
DM_CONTAINER.getHost(),
DM_CONTAINER.getMappedPort(DM_PORT));
} catch (Exception e) {
log.error("Failed to start Dameng container. Integration tests will
be skipped.", e);
// You can set a flag for the test method to check and skip
// Or use org.junit.AssumptionViolatedException
throw new org.junit.AssumptionViolatedException(
"Dameng container not available, skipping integration
tests", e);
}
}
@AfterAll
static void stopContainer() {
if (DM_CONTAINER != null) {
try {
DM_CONTAINER.stop();
log.info("Dameng container stopped");
} catch (Exception e) {
log.warn("Failed to stop Dameng container", e);
}
}
}
```
**Rationale**:
Using AssumptionViolatedException allows the test framework to skip tests
rather than fail, which is more useful in CI environments.
---
## Issue 7: needsQuotesWithDefaultValue test coverage incomplete
**Location**: `DmdbDialectTest.java:197-225`
**Related context**:
- Method under test: `DmdbDialect.java:333-349` (needsQuotesWithDefaultValue)
- Tested constants: `DmdbTypeConverter.java:64-73` (data type constants)
**Issue description**:
Comparing with the implementation of
`DmdbDialect.needsQuotesWithDefaultValue`, the test only covers some types:
- Tested: VARCHAR, CHAR, CLOB, TEXT, INTEGER, DECIMAL
- Not tested: CHARACTER, VARCHAR2, NVARCHAR, LONGVARCHAR, LONG
Although code coverage tools may show coverage (because of the default
branch), explicitly testing each type is safer.
**Potential risks**:
- If someone modifies the switch statement, they may not discover handling
changes for certain types
- Incomplete code documentation
**Impact scope**:
- Direct impact: `DmdbDialectTest` test completeness
- Indirect impact: Understanding of Dameng database data type handling
**Severity**: MINOR
**Improvement suggestion**:
Add missing type tests:
```java
@Test
public void testNeedsQuotesWithDefaultValue() {
DmdbDialect dialect = new DmdbDialect(FieldIdeEnum.ORIGINAL.getValue());
// String types that need quotes
Assertions.assertTrue(dialect.needsQuotesWithDefaultValue(
BasicTypeDefine.builder().name("col").dataType("VARCHAR").build()));
Assertions.assertTrue(dialect.needsQuotesWithDefaultValue(
BasicTypeDefine.builder().name("col").dataType("CHAR").build()));
Assertions.assertTrue(dialect.needsQuotesWithDefaultValue(
BasicTypeDefine.builder().name("col").dataType("CHARACTER").build()));
Assertions.assertTrue(dialect.needsQuotesWithDefaultValue(
BasicTypeDefine.builder().name("col").dataType("CLOB").build()));
Assertions.assertTrue(dialect.needsQuotesWithDefaultValue(
BasicTypeDefine.builder().name("col").dataType("TEXT").build()));
Assertions.assertTrue(dialect.needsQuotesWithDefaultValue(
BasicTypeDefine.builder().name("col").dataType("VARCHAR2").build()));
Assertions.assertTrue(dialect.needsQuotesWithDefaultValue(
BasicTypeDefine.builder().name("col").dataType("NVARCHAR").build()));
Assertions.assertTrue(dialect.needsQuotesWithDefaultValue(
BasicTypeDefine.builder().name("col").dataType("LONGVARCHAR").build()));
Assertions.assertTrue(dialect.needsQuotesWithDefaultValue(
BasicTypeDefine.builder().name("col").dataType("LONG").build()));
// Numeric types that don't need quotes
Assertions.assertFalse(dialect.needsQuotesWithDefaultValue(
BasicTypeDefine.builder().name("col").dataType("INTEGER").build()));
Assertions.assertFalse(dialect.needsQuotesWithDefaultValue(
BasicTypeDefine.builder().name("col").dataType("DECIMAL").build()));
Assertions.assertFalse(dialect.needsQuotesWithDefaultValue(
BasicTypeDefine.builder().name("col").dataType("BIGINT").build()));
}
```
**Rationale**:
Complete coverage of all data types improves test completeness and
maintainability.
---
## Issue 8: acceptsURL method lacks boundary tests
**Location**: `DmdbDialectFactoryTest.java:35-44`
**Related context**:
- Method under test: `DmdbDialectFactory.java:37-39` (acceptsURL)
- Production code calls: `JdbcDialectFactory` auto-discovery mechanism
**Issue description**:
The `acceptsURL` method only tests normal cases and lacks tests for boundary
and exceptional cases:
- null values
- Empty strings
- Case variations
- URLs with parameters
Although the implementation is simple (just `url.startsWith("jdbc:dm:")`),
these boundary tests can prevent problems from being introduced by future
accidental modifications.
**Potential risks**:
- If someone modifies the implementation to `url.equals("jdbc:dm:")` or
other unsafe implementations, tests will not detect it
- null values may cause NullPointerException
**Impact scope**:
- Direct impact: `DmdbDialectFactoryTest` test quality
- Indirect impact: Robustness of URL parsing
**Severity**: MINOR
**Improvement suggestion**:
Add boundary tests:
```java
@Test
public void testAcceptsURL() {
DmdbDialectFactory factory = new DmdbDialectFactory();
// Normal case
Assertions.assertTrue(factory.acceptsURL("jdbc:dm://localhost:5236"));
Assertions.assertTrue(factory.acceptsURL("jdbc:dm:localhost:5236"));
// Case sensitivity test (current implementation should pass)
Assertions.assertTrue(factory.acceptsURL("JDBC:DM://localhost:5236"));
Assertions.assertTrue(factory.acceptsURL("jdbc:DM://localhost:5236"));
// URL with parameters
Assertions.assertTrue(factory.acceptsURL("jdbc:dm://localhost:5236?param=value"));
// Reject other databases
Assertions.assertFalse(factory.acceptsURL("jdbc:mysql://localhost:3306"));
Assertions.assertFalse(factory.acceptsURL("jdbc:postgresql://localhost:5432"));
Assertions.assertFalse(factory.acceptsURL("jdbc:oracle:thin:@localhost:1521"));
// Edge case
Assertions.assertFalse(factory.acceptsURL(null));
Assertions.assertFalse(factory.acceptsURL(""));
Assertions.assertFalse(factory.acceptsURL("jdbc:dm"));
Assertions.assertFalse(factory.acceptsURL("jdbc:dm:"));
}
```
**Rationale**:
Complete testing of boundary cases improves code robustness.
---
## Issue 9: Test classes lack JavaDoc documentation
**Location**:
- `DmdbDialectFactoryTest.java:26`
- `DmdbDialectTest.java:30`
- `AbstractDamengContainerTest.java:48`
- `DamengDialectContainerTest.java:39`
**Related context**:
- Classes under test: `DmdbDialect`, `DmdbDialectFactory`
**Issue description**:
Test classes lack class-level JavaDoc documentation explaining:
- The purpose and scope of the tests
- Test dependencies and prerequisites
- Limitations of the tests
For complex integration tests, documentation is particularly important.
**Potential risks**:
- New developers may struggle to understand test intent
- Increased maintenance costs
- Test limitations are not obvious
**Impact scope**:
- Direct impact: Maintainability of test code
- Indirect impact: Team collaboration efficiency
**Severity**: MINOR
**Improvement suggestion**:
Add JavaDoc:
```java
/**
* Unit tests for {@link DmdbDialectFactory}.
*
* <p>Tests cover:
* <ul>
* <li>Factory name verification</li>
* <li>URL acceptance logic (jdbc:dm: protocol)</li>
* <li>Dialect instance creation with different field identifier
configurations</li>
* </ul>
*/
public class DmdbDialectFactoryTest {
// ...
}
/**
* Unit tests for {@link DmdbDialect} SQL generation and identifier handling.
*
* <p>Tests verify:
* <ul>
* <li>SQL statement generation (UPSERT, INSERT, UPDATE, DELETE,
EXISTS)</li>
* <li>Table and identifier quoting with case sensitivity options</li>
* <li>Default value handling for different data types</li>
* <li>Converter and mapper instantiation</li>
* </ul>
*
* <p>Tests use mock data and do not require a running database.
*/
public class DmdbDialectTest {
// ...
}
```
**Rationale**:
Improve the readability and maintainability of test code.
---
## Issue 10: Integration tests lack tags, preventing selective execution
**Location**:
- `AbstractDamengContainerTest.java:48` (`@Testcontainers`)
- `DamengDialectContainerTest.java:39` (extends
`AbstractDamengContainerTest`)
**Related context**:
- Unit tests: `DmdbDialectTest.java`, `DmdbDialectFactoryTest.java` (do not
require Docker)
**Issue description**:
Integration tests (requiring Docker) and unit tests (not requiring external
dependencies) are not distinguished by JUnit 5's `@Tag` annotation. This means:
1. Cannot use Maven commands to run unit tests alone
2. In environments without Docker, all tests will fail
3. CI environments must configure Docker to run tests
**Potential risks**:
- Developers cannot run any tests in local environments without Docker
- Increased CI environment configuration complexity
- Increased test execution time (integration tests are usually slower)
**Impact scope**:
- Direct impact: Test execution efficiency
- Indirect impact: Development experience and CI/CD configuration
**Severity**: MAJOR
**Improvement suggestion**:
Use `@Tag` to distinguish test types:
```java
/**
* Base class for Dameng (DM8) integration tests using Testcontainers.
*
* <p>These tests require:
* <ul>
* <li>Docker runtime environment</li>
* <li>Network access to pull Docker images</li>
* <li>~500MB disk space for the DM8 image</li>
* </ul>
*
* <p>To skip integration tests:
* <pre>
* mvn test -DexcludedGroups=integration
* </pre>
*/
@Slf4j
@DisabledOnOs(OS.WINDOWS)
@Testcontainers
@Tag("integration")
@Tag("docker")
public abstract class AbstractDamengContainerTest {
// ...
}
```
Also, add tags for unit tests:
```java
/**
* Unit tests for {@link DmdbDialect}.
*/
@Tag("unit")
public class DmdbDialectTest {
// ...
}
/**
* Unit tests for {@link DmdbDialectFactory}.
*/
@Tag("unit")
public class DmdbDialectFactoryTest {
// ...
}
```
Then configure in `pom.xml`:
```xml
<build>
<plugins>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-surefire-plugin</artifactId>
<configuration>
<groups>unit</groups>
<!-- 或者在 CI 中 -->
<!-- <excludedGroups>integration</excludedGroups> -->
</configuration>
</plugin>
</plugins>
</build>
```
**Rationale**:
Distinguish integration tests from unit tests to improve test flexibility
and development experience.
---
### VI. Overall Assessment Conclusion
**Overall evaluation**: This is a **valuable test enhancement PR** that adds
necessary test coverage for the Dameng database Connector. The overall code
quality is good, but there are some configuration and design issues that need
improvement.
**Can merge**: **Conditionally approved** - It is recommended to fix the
following MAJOR-level issues before merging:
1. **Issue 3**: Remove the incorrect postgresql testcontainers dependency
2. **Issue 4**: Add Docker image source documentation or consider official
images
3. **Issue 10**: Add `@Tag` to distinguish integration tests from unit tests
**Suggested post-merge improvements** (can be handled in subsequent PRs):
1. **Issues 1-2, 5-9**: MINOR-level code quality and documentation
improvements
**Strengths**:
- Fills the testing gap for the Dameng database Connector
- Uses modern testing frameworks (JUnit 5, Testcontainers)
- Tests cover core SQL generation logic
- Code style is consistent with the project
**Key areas needing improvement**:
- Maven dependency configuration needs cleanup (Issues 2, 3, 5)
- Integration test architecture needs optimization (Issues 4, 6, 10)
- Test coverage needs improvement (Issues 7, 8)
- Documentation needs improvement (Issue 9)
**Test coverage recommendations**:
- Current unit test coverage is approximately 60-70% (estimated)
- Recommend adding boundary tests and exception scenario tests
- Recommend adding performance tests (e.g., batch SQL generation)
--
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]
