Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix extraneous use of connection string useSSL flag #569

Merged
merged 4 commits into from
Feb 3, 2018
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,12 @@
# Change Log
All notable changes to this project will be documented in this file.

## [1.6.1] - 2018-01-31

### Fixed

- Fixed extraneous insertion of `useSSL=false` in all JDBC URL strings, even for DBs that do not understand it. Usage is now restricted to MySQL by default and can be overridden by authors of `JdbcDatabaseContainer` subclasses ([\#568](https://github.com/testcontainers/testcontainers-java/issues/568))

## [1.6.0] - 2018-01-28

### Fixed
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ public SELF withUsername(String username) {
throw new UnsupportedOperationException();
}

public SELF withPassword(String password){
public SELF withPassword(String password) {
throw new UnsupportedOperationException();
}

Expand Down Expand Up @@ -127,16 +127,17 @@ public Driver getJdbcDriverInstance() {
/**
* Creates a connection to the underlying containerized database instance.
*
* @param queryString any special query string parameters that should be appended to the JDBC connection URL. The
* '?' character must be included
* @param queryString
* query string parameters that should be appended to the JDBC connection URL.
* The '?' character must be included
* @return a Connection
* @throws SQLException if there is a repeated failure to create the connection
*/
public Connection createConnection(String queryString) throws SQLException {
final Properties info = new Properties();
info.put("user", this.getUsername());
info.put("password", this.getPassword());
final String url = appendDisableSslConfig(this.getJdbcUrl() + queryString);
final String url = constructUrlForConnection(queryString);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


final Driver jdbcDriverInstance = getJdbcDriverInstance();

Expand All @@ -147,9 +148,18 @@ public Connection createConnection(String queryString) throws SQLException {
}
}

private String appendDisableSslConfig(String connectionString){
String separator = connectionString.contains("?") ? "&" : "?";
return connectionString + separator + "useSSL=false";
/**
* Template method for constructing the JDBC URL to be used for creating {@link Connection}s.
* This should be overridden if the JDBC URL and query string concatenation or URL string
* construction needs to be different to normal.
*
* @param queryString
* query string parameters that should be appended to the JDBC connection URL.
* The '?' character must be included
* @return a full JDBC URL including queryString
*/
protected String constructUrlForConnection(String queryString) {
return getJdbcUrl() + queryString;
}

protected void optionallyMapResourceParameterAsVolume(@NotNull String paramName, @NotNull String pathNameInContainer, @NotNull String defaultResource) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,13 @@ public String getJdbcUrl() {
return "jdbc:mysql://" + getContainerIpAddress() + ":" + getMappedPort(MYSQL_PORT) + "/" + databaseName;
}

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

@Override
public String getDatabaseName() {
return databaseName;
Expand Down