Add Oracle Reverse ITs - #4177
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a robust testing framework for Oracle as a target database in the Spanner-to-SourceDB migration template. It adds multiple integration tests covering various scenarios such as data type compatibility, handling of reserved keywords, wide row support, and complex schema mapping via overrides. Additionally, it improves the reliability of Oracle-based integration tests by providing better resource management and DLQ retry validation. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive integration tests and utility classes to support reverse replication from Spanner to Oracle databases, including sharded topologies, dead-letter queue (DLQ) retries, custom transformations, and various datatype mappings. Feedback on these changes highlights several important areas for improvement: addressing potential SQL injection vulnerabilities caused by dynamic SQL string concatenation in the test base and shared container classes, resolving a resource leak where the singleton Oracle resource manager is never closed, replacing magic strings with existing constants, removing or implementing placeholder empty methods, and fixing a state leakage bug in the test cleanup method by ensuring shard-specific test usernames are properly reset.
| "REV_" + | ||
| java.util.UUID.randomUUID().toString().replace("-", "").substring(0, 8).toUpperCase(); | ||
| LOG.info("Creating isolated Oracle user: {}", username); | ||
| jdbcResourceManager.runSQLUpdate("CREATE USER " + username + " IDENTIFIED BY password"); |
There was a problem hiding this comment.
This SQL statement is constructed using string concatenation with the username variable, which is a potential SQL injection vulnerability. Although username is generated from a UUID in this context and is likely safe, it's a security best practice to avoid building queries this way. Please consider using PreparedStatement for executing queries with parameters, or at least sanitizing the input. This concern also applies to the GRANT statements below, and to similar dynamic SQL in runIsolatedSQLQuery (line 622) and createOracleSchema (line 649).
| public static synchronized OracleResourceManager getInstance() { | ||
| if (instance == null) { | ||
| instance = OracleResourceManager.builder("oracle-rev-bulk-db").build(); | ||
| try { | ||
| try (Connection systemConn = | ||
| DriverManager.getConnection(instance.getUri(), "SYSTEM", instance.getPassword()); | ||
| Statement stmt = systemConn.createStatement()) { | ||
| stmt.execute("GRANT DBA TO " + instance.getUsername()); | ||
| LOG.info("Successfully granted DBA to Testcontainers Oracle app user!"); | ||
| } | ||
| } catch (Exception e) { | ||
| LOG.warn("Failed to grant DBA using SYSTEM. CREATE USER might fail.", e); | ||
| } | ||
| } | ||
| return instance; | ||
| } |
There was a problem hiding this comment.
The singleton OracleResourceManager instance created here is never cleaned up. This can lead to resource leaks, as the underlying test container might not be shut down properly after tests complete.
Please consider adding a shutdown hook or a static cleanup method that can be called from a test suite's @AfterAll hook to ensure instance.close() is called.
| try (Connection systemConn = | ||
| DriverManager.getConnection(instance.getUri(), "SYSTEM", instance.getPassword()); | ||
| Statement stmt = systemConn.createStatement()) { | ||
| stmt.execute("GRANT DBA TO " + instance.getUsername()); |
There was a problem hiding this comment.
This GRANT statement is constructed using string concatenation, which is a potential SQL injection vulnerability. While instance.getUsername() is likely safe in this test context as it's generated by the resource manager, it is a security best practice to use parameterized queries or sanitize inputs to prevent SQL injection.
| com.google.cloud.teleport.v2.templates.constants.Constants | ||
| .SOURCE_POSTGRESQL)) | ||
| .SOURCE_POSTGRESQL) | ||
| && !Objects.equals(sourceType, "oracle")) |
There was a problem hiding this comment.
The magic string "oracle" is used here. For consistency and maintainability, please use the existing constant ORACLE_SOURCE_TYPE from com.google.cloud.teleport.v2.spanner.migrations.constants.Constants.
| protected void createOracleTableWithNColumns( | ||
| org.apache.beam.it.jdbc.OracleResourceManager jdbcResourceManager, | ||
| String arg1, | ||
| int arg2, | ||
| String arg3) {} |
There was a problem hiding this comment.
This method createOracleTableWithNColumns is empty. It seems to be a placeholder. Please either implement it or remove it to avoid confusion.
| public static void clearIsolatedUser() { | ||
| testUsername = null; | ||
| } |
There was a problem hiding this comment.
The clearIsolatedUser method only resets testUsername. It should also reset testUsernameShardA and testUsernameShardB to prevent state from leaking between tests using different shards.
public static void clearIsolatedUser() {
testUsername = null;
testUsernameShardA = null;
testUsernameShardB = null;
}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4177 +/- ##
=============================================
+ Coverage 35.83% 63.10% +27.26%
- Complexity 711 2784 +2073
=============================================
Files 250 562 +312
Lines 17131 32614 +15483
Branches 1750 3645 +1895
=============================================
+ Hits 6139 20580 +14441
- Misses 10479 10991 +512
- Partials 513 1043 +530
🚀 New features to boost your workflow:
|
bb34b08 to
89a565e
Compare
e16671e to
8bd93bd
Compare
1 participant
Adding Oracle ITs for Reverse template.