Added additional url params in JdbcDatabaseContainer (#1802) #1874

eaxdev · 2019-09-15T18:58:29Z

Added the ability to add additional parameters to the URL in JDBC containers

Added the ability to add additional parameters to the URL (https://dev.mysql.com/doc/connector-j/8.0/en/connector-j-reference-configuration-properties.html)

rnorth · 2019-09-16T21:16:32Z

Thanks @eaxdev!

I'm curious about a couple of things.

The connection string that you're modifying is usually only used during Testcontainers initial readiness checks for the container and running of init scripts. The expectated usage of the current API is that your normal code would call getJdbcUrl(), append any query params you wish, and obtain a connection yourself. The createConnection(String) method can also be used, and accepts query params in urlencoded form.

So firstly, what is the use case that means that the current API is not sufficient? I'm afraid I'm not quite clear on this yet.

Secondly, if we do this, is there a specific reason not to support it for other JDBC container types? It seems that it would almost be easier to support it all-round!

eaxdev · 2019-09-17T09:06:46Z

Hi, @rnorth!

Yes, I also think that it is necessary to add this logic in the getJdbcUrl() method. And this is more correct. But as @knutwannheden described in issue Cannot connect to MySQL with time zone #1764 when the container is initialization, it also needs specific properties from the URL, such as serverTimezone, without this in its case the container does not start. Therefore, the same settings are required and in the constructUrlForConnection method.
Yes, that's a good idea. I could also do this for for other JDBC container types that support parameters in the URL.

knutwannheden · 2019-09-17T13:41:49Z

Some comments from my side:

I would indeed expect the logic to be implemented directly in JdbcDatabaseContainer. I now already had to extend the MySQLContainer and the MariaDBContainer for the same reason.
I also think the logic should be implemented in constructUrlForConnection(String), because it also needs to work when called by JdbcDatabaseContainer#waitUntilContainerStarted(), where createConnection(String) is called with the empty string as the argument.
I think there should be Javadoc for withUrlParam() which makes it clear whether the arguments are expected to be encoded or not (expecting encoded values probably makes more sense, as some JDBC drivers may not decode according to the URL specification?).

rnorth · 2019-09-23T20:29:59Z

Thanks for your input @knutwannheden - I agree with your points.

@eaxdev, would you be OK to follow up with changes?
Thank you both!

eaxdev · 2019-09-24T06:42:59Z

Hi, @rnorth! Yes, I'm ready to follow up with changes. But still I want to clarify the final requirements:

Add this logic to two methods: getJdbcUrl() and constructUrlForConnection().
Add this logic to other JDBC container types, not just MySQL.

Did I understand the problem correctly? Thanks!

knutwannheden · 2019-10-09T04:17:34Z

@rnorth It looks like @eaxdev is still waiting for some input here. Just in case you missed it. Thanks.

… constructUrlParameters() (#1802)

eaxdev · 2019-10-16T20:03:14Z

Hi, @rnorth!

I added support it for other JDBC container types. See, please. Thank!

knutwannheden · 2019-10-16T20:19:01Z

modules/jdbc/src/main/java/org/testcontainers/containers/JdbcDatabaseContainer.java


+    public SELF withUrlParam(String paramName, String paramValue) {
+        throw new UnsupportedOperationException();


Why can't the implementation of this method be pulled up here?

Yes, that's a good idea. I will fix it.

This is done. I did it.

rnorth · 2019-10-17T19:00:19Z

We seem to have some test failures around the MS SQL Server container - any ideas?

eaxdev · 2019-10-17T19:05:59Z

@rnorth , Yes, Yes. I already got it. I'm already fixing...

…e end url (#1802)

eaxdev · 2019-10-18T06:21:35Z

@rnorth, I fixed it. Now all tests pass

knutwannheden · 2019-10-25T18:27:51Z

modules/postgresql/src/main/java/org/testcontainers/containers/PostgreSQLContainer.java

@@ -46,6 +46,8 @@ public PostgreSQLContainer(final String dockerImageName) {
    @Override
    protected void configure() {
        addExposedPort(POSTGRESQL_PORT);
+        // Disable Postgres driver use of java.util.logging to reduce noise at startup time
+        withUrlParam("loggerLevel", "OFF");


I haven't investigated what exactly this does, but it looks a bit weird. I can understand if by default the logging level should not be DEBUG or so, but I would definitely like to see errors and probably also warnings by default (i.e. without having to investigate how to change this manually).

Please disregard if I missed something.

It used to be in the getJdbcUrl method like this:

return "jdbc:postgresql://" + getContainerIpAddress() + ":" + getMappedPort(POSTGRESQL_PORT) + "/" + databaseName + "?loggerLevel=OFF";

I just moved it to the configure() method.

I'm guessing the logger was turned off on purpose to remove the noise at the start of the container.

knutwannheden · 2019-10-25T18:41:00Z

modules/oracle-xe/src/main/java/org/testcontainers/containers/OracleContainer.java

@@ -90,6 +90,11 @@ public OracleContainer withPassword(String password) {
        return self();
    }

+    @Override
+    public OracleContainer withUrlParam(String paramName, String paramValue) {


Since some DBs like Oracle don't support URL parameters, but do support Properties parameters, I think it is worth considering renaming this method to withParam() or withJdbcParam(), so that client code doesn't have to call withUrlParameter() for some DBs and something like withPropertiesParam() for others (assuming that will be also supported in the future). I just think this would be worth considering.

I think this is not entirely correct. What do you think about that, @rnorth ? Thx!

Sorry to take so long to come back to this PR to break the tie...

I think the current form is reasonable; Oracle does not support JDBC URL parameters (in the normal sense of a URL parameter, anyway). Also, we don't support setting connection properties for the Oracle driver.

It's good to think about the future case of being agnostic of whether the connection params are set in the URL or as properties. However, I think that:

the params themselves are so database-specific that it's rarely going to be possible to write them generically, and

trying to support both URL params and properties via one method is going to tie us in knots if we ever actually support pass-through of properties - we're not going to know which params go where.

Makes sense. Let's tackle that problem with connection properties separately. From my POV this commit looks good to go!

knutwannheden · 2019-11-29T18:54:40Z

Any status update on this PR? Would be nice if it could get integrated in some form.

stale · 2020-04-11T07:15:37Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you believe this is a mistake, please reply to this comment to keep it open. If there isn't one already, a PR to fix or at least reproduce the problem in a test case will always help us get back on track to tackle this.

kiview · 2020-04-11T09:13:11Z

@rnorth WDYT on how to proceed here? You were the most involved one.

eaxdev · 2020-05-15T12:24:58Z

@rnorth Which status of this PR? Is something wrong with him?

rnorth · 2020-05-16T19:48:35Z

Eugh, sorry I let this slip through the cracks. I'll check out the merge conflicts and resolve.

…_param

…1874)" This reverts commit 206c5f6.

#2777)

…1802) (testcontainers#1874)

…abaseContainer" (testcontainers#2777)

eaxdev requested review from bsideup, kiview and rnorth as code owners September 15, 2019 18:58

eaxdev changed the title ~~Add additional url params in MySQLContainer (#1802)~~ Added additional url params in MySQLContainer (#1802) Sep 15, 2019

Add additional url params in MySQLContainer (#1802)

0af8b12

Added the ability to add additional parameters to the URL (https://dev.mysql.com/doc/connector-j/8.0/en/connector-j-reference-configuration-properties.html)

rnorth added 2 commits October 8, 2019 21:00

Merge branch 'master' into eaxdev_add_with_url_param

456d477

Merge branch 'master' into eaxdev_add_with_url_param

00536ce

eaxdev added 6 commits October 16, 2019 21:20

Change additional url params in MySQLContainer. Create common method:…

e55eb25

… constructUrlParameters() (#1802)

Add additional url params in CockroachContainer (#1802)

5417319

Add additional url params in Db2Container (#1802)

8af81ac

Add additional url params in MSSQLServerContainer (#1802)

96c955e

Add additional url params in MariaDBContainer (#1802)

709ad1e

Add additional url params in PostgreSQLContainer (#1802)

722f32b

knutwannheden reviewed Oct 16, 2019

View reviewed changes

Fix typo in test (#1802)

062070f

eaxdev added 2 commits October 17, 2019 23:01

Move common method withUrlParam() to parent abstract class (#1802)

2022e5d

Fixed build url params in DB2 container. DB2 require ';' symbol in th…

701a1b0

…e end url (#1802)

eaxdev changed the title ~~Added additional url params in MySQLContainer (#1802)~~ Added additional url params in JdbcDatabaseContainer (#1802) Oct 18, 2019

witjem mentioned this pull request Oct 18, 2019

#1802: added JdbcDatabaseContainer#withUrlParam #1962

Closed

Merge branch 'master' into eaxdev_add_with_url_param

21f63e2

knutwannheden reviewed Oct 25, 2019

View reviewed changes

rnorth self-assigned this Nov 1, 2019

Merge branch 'master' into eaxdev_add_with_url_param

7eadac4

stale bot added the stale label Apr 11, 2020

stale bot removed the stale label Apr 11, 2020

rnorth added 3 commits May 16, 2020 20:59

Merge remote-tracking branch 'origin/master' into eaxdev_add_with_url…

d89543e

…_param

Merge remote-tracking branch 'origin/master' into eaxdev_add_with_url…

128b69f

…_param

Reinstate tests lost during merge

fd56078

rnorth approved these changes May 16, 2020

View reviewed changes

rnorth added 2 commits May 21, 2020 16:06

Merge branch 'master' into eaxdev_add_with_url_param

f005def

Fix erroneous assertion

4f1d18f

rnorth added the modules/jdbc label May 21, 2020

rnorth added this to the next milestone May 21, 2020

rnorth merged commit 206c5f6 into testcontainers:master May 21, 2020

rnorth modified the milestones: 1.14.2, next May 21, 2020

rnorth added a commit that referenced this pull request May 21, 2020

Revert "Added additional url params in JdbcDatabaseContainer (#1802) (#…

7edccb1

…1874)" This reverts commit 206c5f6.

This was referenced May 21, 2020

Revert "Added additional url params in JdbcDatabaseContainer (#1802)" #2775

Merged

Reinstate #1874 "Added additional url params in JdbcDatabaseContainer" #2777

Merged

Prevent Local gradle build cache collisions #2778

Merged

rnorth added a commit that referenced this pull request May 22, 2020

Reinstate #1874 "Added additional url params in JdbcDatabaseContainer" (

f401b1c

#2777)

quincy pushed a commit to quincy/testcontainers-java that referenced this pull request May 28, 2020

Added additional url params in JdbcDatabaseContainer (testcontainers#…

cb7ecf6

…1802) (testcontainers#1874)

quincy pushed a commit to quincy/testcontainers-java that referenced this pull request May 28, 2020

Reinstate testcontainers#1874 "Added additional url params in JdbcDat…

325610e

…abaseContainer" (testcontainers#2777)

eaxdev deleted the eaxdev_add_with_url_param branch June 21, 2020 19:42

kiview mentioned this pull request Oct 4, 2022

New JdbcDatabaseContainer#withUrlParam(String, String) method #1802

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added additional url params in JdbcDatabaseContainer (#1802) #1874

Added additional url params in JdbcDatabaseContainer (#1802) #1874

eaxdev commented Sep 15, 2019 •

edited

rnorth commented Sep 16, 2019

eaxdev commented Sep 17, 2019

knutwannheden commented Sep 17, 2019

rnorth commented Sep 23, 2019

eaxdev commented Sep 24, 2019

knutwannheden commented Oct 9, 2019

eaxdev commented Oct 16, 2019

knutwannheden Oct 16, 2019

eaxdev Oct 17, 2019

eaxdev Oct 17, 2019

rnorth commented Oct 17, 2019

eaxdev commented Oct 17, 2019

eaxdev commented Oct 18, 2019

knutwannheden Oct 25, 2019

eaxdev Oct 26, 2019

knutwannheden Oct 25, 2019

eaxdev Oct 26, 2019

rnorth Dec 23, 2019

knutwannheden Jan 12, 2020

knutwannheden commented Nov 29, 2019

stale bot commented Apr 11, 2020

kiview commented Apr 11, 2020

eaxdev commented May 15, 2020

rnorth commented May 16, 2020


		public SELF withUrlParam(String paramName, String paramValue) {
		throw new UnsupportedOperationException();

Added additional url params in JdbcDatabaseContainer (#1802) #1874

Added additional url params in JdbcDatabaseContainer (#1802) #1874

Conversation

eaxdev commented Sep 15, 2019 • edited

rnorth commented Sep 16, 2019

eaxdev commented Sep 17, 2019

knutwannheden commented Sep 17, 2019

rnorth commented Sep 23, 2019

eaxdev commented Sep 24, 2019

knutwannheden commented Oct 9, 2019

eaxdev commented Oct 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnorth commented Oct 17, 2019

eaxdev commented Oct 17, 2019

eaxdev commented Oct 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knutwannheden commented Nov 29, 2019

stale bot commented Apr 11, 2020

kiview commented Apr 11, 2020

eaxdev commented May 15, 2020

rnorth commented May 16, 2020

eaxdev commented Sep 15, 2019 •

edited