terrymanu commented on PR #38901: URL: https://github.com/apache/shardingsphere/pull/38901#issuecomment-4785573469
### Summary **Review Result: Mergeable** Reason: The PR is scoped to Oracle JDBC URL parsing and correctly preserves query properties for the Oracle URL forms already handled by `OracleConnectionPropertiesParser`. The production change is covered by focused unit cases, and scoped local verification plus repository style gates passed. ### Evidence - `database/connector/dialect/oracle/src/main/java/org/apache/shardingsphere/database/connector/oracle/jdbcurl/OracleConnectionPropertiesParser.java:50` strips the query part before applying the existing Oracle URL matchers, so the hostname/port/catalog parsing path remains the same for no-query URLs. - `OracleConnectionPropertiesParser.java:51` reuses `StandardJdbcUrlParser#parseQueryProperties` instead of adding a separate query parser. - `OracleConnectionPropertiesParser.java:55` passes parsed query properties into both thin-style and connect-descriptor `ConnectionProperties`. - `database/connector/dialect/oracle/src/test/java/org/apache/shardingsphere/database/connector/oracle/jdbcurl/OracleConnectionPropertiesParserTest.java:74` and `:92` add direct coverage for query properties on thin-style and connect-descriptor Oracle URLs, while existing no-query cases remain covered. - Oracle JDBC documentation describes URL query parameters after `?` for EZConnect URLs, so preserving these properties is consistent with Oracle JDBC URL behavior: https://docs.oracle.com/en/database/oracle/oracle-database/21/jajdb/oracle/jdbc/OracleDriver.html ### Review Details Reviewed Scope: Latest PR head `5eb3e99ce727d6b8526121abdde1c287b5e8263a`; base `ede2451d30c6affbc6078d8df4365bcaa91e5242`; local merge-base `464662d666bb8b67b618dc336887850ae146199c`. GitHub `/pulls/38901/files` matched the local triple-dot file list. Reviewed files: - `database/connector/dialect/oracle/src/main/java/org/apache/shardingsphere/database/connector/oracle/jdbcurl/OracleConnectionPropertiesParser.java` - `database/connector/dialect/oracle/src/test/java/org/apache/shardingsphere/database/connector/oracle/jdbcurl/OracleConnectionPropertiesParserTest.java` Not Reviewed Scope: Full repository build, live Oracle driver/database smoke testing, and unrelated modules beyond the existing query-property consumption paths. Broader CI/E2E jobs were still in progress and were not awaited because this connector-only change had sufficient code, test, and scoped local verification evidence. Verification: - `python3 .codex/skills/review-pr/scripts/build_review_inventory.py --base-ref apache/master --head-ref apache/pr/38901 --github-files ... --format markdown` exited `0`; GitHub file list matched local scope. - `./mvnw -pl database/connector/dialect/oracle -am -DskipITs -Dspotless.skip=true -Dtest=OracleConnectionPropertiesParserTest -Dsurefire.failIfNoSpecifiedTests=false test` exited `0`; `BUILD SUCCESS`. - `./mvnw -pl database/connector/dialect/oracle -am -Pcheck -DskipTests spotless:check checkstyle:check` exited `0`; `BUILD SUCCESS`. - GitHub checks observed for the latest head: `Check - License`, `Check - CheckStyle`, and `Check - Spotless` completed successfully. Release Note / User Docs: Not required. This preserves existing Oracle JDBC URL query properties in parsed metadata and does not introduce a new user-facing configuration key, migration step, or compatibility break. -- 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]
