Fix extraneous use of connection string useSSL flag

[rnorth]

This is a proposed fix for #568, hopefully to release quickly.

[iNikem]

You forgot the queryString . Probably should be getJdbcUrl() + queryString + separator + "useSSL=false". Or may be even

String url = super.constructUrlForConnection(queryString);
String separator = url.contains("?") ? "&" : "?";
return url + separator + "useSSL=false";

[rnorth]

Doh, I’m a fool 😱. Thanks for the catch.

…

On Wed, 31 Jan 2018 at 20:58 Nikita Salnikov-Tarnovski < ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In modules/mysql/src/main/java/org/testcontainers/containers/MySQLContainer.java <#569 (comment)> : > @@ -54,6 +54,12 @@ public String getJdbcUrl() { return "jdbc:mysql://" + getContainerIpAddress() + ":" + getMappedPort(MYSQL_PORT) + "/" + databaseName; } + @OverRide + protected String constructUrlForConnection(String queryString) { + String separator = queryString.contains("?") ? "&" : "?"; + return getJdbcUrl() + separator + "useSSL=false"; You forgot the queryString. Probably should be getJdbcUrl() + queryString + separator + "useSSL=false". Or may be even String url = super.constructUrlForConnection(queryString); String separator = url.contains("?") ? "&" : "?"; return url + separator + "useSSL=false"; — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#569 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAIET6Xf02H9jB6FndjSiZrsSKG2Ppwdks5tQNQEgaJpZM4R0nuo> .

[npetzall]

Did you consider the withDefaultQueryString approach?
Since no subclassing would be need to remove the override that is suggested.
If that would be desired, say when this added possibility of override were to be re-used.

[kiview]

LGTM, my remark can be tackeled in another change.

[kiview]

I wonder if the real problem lies in line 145. I'm not a JDBC expert, but shouldn't we provide those flags in the Properties object, like info.setProperty("useSSL", "false");? AFAIK this would make this condfigs database driver implementation independent, at least regarding the way, the url is constructed.

I'm totally fine with this fix ATM, but I think we should refactor this part in a follow up, ideally once we've changed to the monorepo, so we can easily test for regressions.

[npetzall]

I got inspired by your comment, but after testing this with Hive-jdbc i got sad.
I'm not gonna force you into supporting each and every corner use case. Since it's implementation specific. Ergo I should probably submit a ticket to hive-jdbc developers.

But there is no guarantee that all information in info.properties will have affect. I assume that the reason is that most end-user applications accepts a String to make the connection, usually with user/pass fields, but not a key-value form for setting the info properties.

So the best bet is that parameters in ConnectionString will work and that info properties might work.

And just to show some of the horror with hive-jdbc connection string.
Hive JDBC URL: jdbc:hive2://:/dbName;sess_var_list?hive_conf_list#hive_var_list

schema:host:port/path;matrix?query#fragment

[iNikem]

I would like to remind about the original problem and to call the power of YAGNI :) The problem was that using MySQL containers was inconvenient due to irrelevant ssl warnings. There is no feature requests for default query string nor for any new kind of connection configuration. So in my opinion the only thing that must be done here, is disable SSL in MySQL container. That's it.

[npetzall]

Original issue was that TC jdbc URL the useSSL parameter was ignored which it still will be during waitStrategy.

[iNikem]

No, it will not. During waitStrategy the same createConnection method is used, which now adds useSSL=false in case of MySQL.

[npetzall]

My point is that it won't use the parameter supplied in the TC jdbc URL, it will ignore it and add it's own.

[npetzall]

If I have it in my TC jdbc URL will it be added twice?

[rnorth]

@npetzall thanks - you were right about the parameter being added twice. I have added a simple check to prevent this, which works for now.

In general I'd agree that the code in this area is not ideal; we still need to bring #357 back from the dead, so I hope we can do a bit of a general refactor and spring clean soon.