Fix buildJdbcUrl to apply connection properties #651

seoyoniee · 2025-03-26T09:40:28Z

Description

Fixing buildJdbcUrl to apply connection properties

Additional context and related issues

fixing #650

Release notes

( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
(x) Release notes are required, with the following suggested text:

* Fix buildJdbcUrl to apply connection properties (https://github.com/trinodb/trino-gateway/issues/650)

cla-bot · 2025-03-26T09:40:31Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

seoyoniee · 2025-04-04T00:15:43Z

@cla-bot check

cla-bot · 2025-04-04T00:15:46Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

cla-bot · 2025-04-04T00:15:47Z

The cla-bot has been summoned, and re-checked this pull request!

mosabua · 2025-04-08T01:45:50Z

@cla-bot check

cla-bot · 2025-04-08T01:45:53Z

The cla-bot has been summoned, and re-checked this pull request!

mosabua

I think this requires a pretty static format for the JDBC URL. Can you confirm that this works for MySQL, Oracle, and PostgreSQL as we suggest. At a minimum I think we should add some simple test to verify that the property you reported as failing now works in terms of passing it through. Overall I am ok with this approach though. Might be worth looking in the Trino codebase if we have any similar setup and tests for it.

seoyoniee · 2025-04-17T05:59:54Z

@mosabua
As you pointed out, the code I modified previously had a pretty static format and wouldn't work well for various database, especially Oracle. So I made two modifications:

re-modified buildJdbcUrl to ensure it works correctly even when query parameters contain slashes 083ee64
I also added test code for h2, mysql, Oracle, and PostgreSQL 5c0b9dd

gateway-ha/src/main/java/io/trino/gateway/ha/persistence/JdbcConnectionManager.java

gateway-ha/src/test/java/io/trino/gateway/ha/persistence/TestJdbcConnectionManager.java

mosabua

This looks good to me now. Can you squash commits @seoyoniee ?

Anything else to change @ebyhr ?

ebyhr · 2025-05-15T00:07:43Z

gateway-ha/src/test/java/io/trino/gateway/ha/persistence/TestJdbcConnectionManager.java

+
+final class TestJdbcConnectionManager
+{
+    private JdbcConnectionManager createConnectionManager(String jdbcUrl)


Please move this method to the bottom. We locate helper method after the caller in this project.

ebyhr · 2025-05-15T00:10:57Z

gateway-ha/src/main/java/io/trino/gateway/ha/persistence/JdbcConnectionManager.java

@@ -60,11 +63,26 @@ public Jdbi getJdbi(@Nullable String routingGroupDatabase)
                .registerRowMapper(new RecordAndAnnotatedConstructorMapper());
    }

-    private String buildJdbcUrl(@Nullable String routingGroupDatabase)
+    public String buildJdbcUrl(@Nullable String routingGroupDatabase)


The package-private is sufficient, right? Please add @VisibleForTesting annotation.

yes right! 7d2d2ae

ebyhr · 2025-05-15T00:12:57Z

gateway-ha/src/main/java/io/trino/gateway/ha/persistence/JdbcConnectionManager.java

+            }
+            catch (URISyntaxException e) {
+                throw new RuntimeException(e);
+            }


Please return jdbcUrl early when routingGroupDatabase is null so we can avoid redundant nesting:

if (routingGroupDatabase == null) { return jdbcUrl; } try { ...

ebyhr · 2025-05-15T00:13:25Z

gateway-ha/src/test/java/io/trino/gateway/ha/persistence/TestJdbcConnectionManager.java

+
+final class TestJdbcConnectionManager
+{
+    private JdbcConnectionManager createConnectionManager(String jdbcUrl)


This method can be static.

Chaho12

Can you add some failures in test cases? For incorrect jdbc urls?

Chaho12 · 2025-05-27T14:53:43Z

gateway-ha/src/main/java/io/trino/gateway/ha/persistence/JdbcConnectionManager.java

@@ -60,13 +64,29 @@ public Jdbi getJdbi(@Nullable String routingGroupDatabase)
                .registerRowMapper(new RecordAndAnnotatedConstructorMapper());
    }

-    private String buildJdbcUrl(@Nullable String routingGroupDatabase)
+    @VisibleForTesting
+    String buildJdbcUrl(@Nullable String routingGroupDatabase)
    {
        String jdbcUrl = configuration.getJdbcUrl();


maybe check?

if (jdbcUrl == null) { throw new IllegalArgumentException("JDBC URL cannot be null"); }

Chaho12 · 2025-05-27T15:05:46Z

gateway-ha/src/main/java/io/trino/gateway/ha/persistence/JdbcConnectionManager.java

+        catch (URISyntaxException e) {
+            throw new RuntimeException(e);


Hey @ebyhr WDYT of catching more broder exception like RuntimeException or Exception
Because NPE could happen if jdbcUrl is none, or IndexOutOfBounds if / is not present, or IllegalArgumentException if Path.of(uri.getPath()) is malformed path.

Suggested change

catch (URISyntaxException e) {

throw new RuntimeException(e);

catch (Exception e) { // catch all

Logger.get(getClass()).error(e, "Failed to build JDBC URL for routing group '%s'", routingGroupDatabase);

throw new RuntimeException(e);

}

Chaho12 · 2025-05-27T15:16:08Z

gateway-ha/src/main/java/io/trino/gateway/ha/persistence/JdbcConnectionManager.java

+            return jdbcUrl;
+        }
+        try {
+            int index = jdbcUrl.indexOf("/") + 1;


Maybe add check?

Suggested change

int index = jdbcUrl.indexOf("/") + 1;

int index = jdbcUrl.indexOf("/") + 1;

if (index < 0) {

throw new IllegalArgumentException("Invalid JDBC URL: no '/' found in " + jdbcUrl);

}

cla-bot bot added the cla-signed label Apr 8, 2025

mosabua reviewed Apr 8, 2025

View reviewed changes

xkrogen reviewed Apr 17, 2025

View reviewed changes

gateway-ha/src/main/java/io/trino/gateway/ha/persistence/JdbcConnectionManager.java Outdated Show resolved Hide resolved

ebyhr requested changes Apr 19, 2025

View reviewed changes

ebyhr reviewed May 12, 2025

View reviewed changes

gateway-ha/src/test/java/io/trino/gateway/ha/persistence/TestJdbcConnectionManager.java Show resolved Hide resolved

mosabua approved these changes May 14, 2025

View reviewed changes

Fix buildJdbcUrl to apply connection properties

f38be06

seoyoniee force-pushed the fix-build-jdbc-url branch from 01de0f9 to f38be06 Compare May 14, 2025 23:18

seoyoniee changed the title ~~Fixing buildJdbcUrl to apply connection properties~~ Fix buildJdbcUrl to apply connection properties May 14, 2025

ebyhr reviewed May 15, 2025

View reviewed changes

seoyoniee added 4 commits May 15, 2025 10:23

fixup! Fix buildJdbcUrl to apply connection properties

77dd5fe

fixup! Fix buildJdbcUrl to apply connection properties

7d2d2ae

fixup! Fix buildJdbcUrl to apply connection properties

ba8c71e

fixup! Fix buildJdbcUrl to apply connection properties

8d099e7

Chaho12 reviewed May 27, 2025

View reviewed changes

		catch (URISyntaxException e) {
		throw new RuntimeException(e);

Fix buildJdbcUrl to apply connection properties #651

Are you sure you want to change the base?

Fix buildJdbcUrl to apply connection properties #651

Uh oh!

Conversation

seoyoniee commented Mar 26, 2025

Description

Additional context and related issues

Release notes

Uh oh!

cla-bot bot commented Mar 26, 2025

Uh oh!

seoyoniee commented Apr 4, 2025

Uh oh!

cla-bot bot commented Apr 4, 2025

Uh oh!

cla-bot bot commented Apr 4, 2025

Uh oh!

mosabua commented Apr 8, 2025

Uh oh!

cla-bot bot commented Apr 8, 2025

Uh oh!

mosabua left a comment

Choose a reason for hiding this comment

Uh oh!

seoyoniee commented Apr 17, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mosabua left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Chaho12 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!