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

Allow setting database name and user when using JDBC URL #594

Closed

Conversation

manikmagar
Copy link
Contributor

@manikmagar manikmagar commented Feb 26, 2018

This PR implements #566.

  1. Extracts out JDBC Url manipulations to a separate class - ConnectionUrl.
  2. Adds an overloaded method JdbcDatabaseContainerProvider.newInstance(ConnectionUrl), with default implementation delegating to the existing newInstance(tag) method.
  3. Adds an implementation of newInstance(ConnectionUrl) in MySQLContainerProvider to use Database Name, user, and password from JDBC URL while creating new Container.
  4. Added/Updated Test cases for new functionality.
  5. Bug Fix: Query Parameters are not used when a connection is created from a cached container. (see this comment) Fixes db extra connection params are only passed to the first connection in a pool  #610

Note: Other Containers Providers such as PostgreSQL will remain unaffected and continue to use earlier implementation of newInstance(tag) due to default delegation, until another implementation is provided.

@rnorth
Copy link
Member

rnorth commented Feb 26, 2018

Ah, interesting. This might solve #357 / #345 as well, so seems good. Will review properly later.

…ainers#593)

* Mark DockerMachineClientProviderStrategy as not persistable

* Update CHANGELOG.md
@manikmagar
Copy link
Contributor Author

I did not see those two earlier. For filtering out TC parameters, we could do this -

  1. This line where we extract out queryString, we could call getQueryParameters() function.
  2. The Query Param Matching Pattern, could be modified to exclude anything that starts with TC_, which are TestContainers parameters (any possibility of that prefix conflicting with any real params?!).
  3. Join map entries returned by getQueryParameters method to build new queryString.

WDYT?

That makes me think if TC_PARAM_MATCHING_PATTERN should only consider Pattern to include parameters starting with TC_?

@manikmagar
Copy link
Contributor Author

@rnorth Codacy is complaining to have atleast one JUnit assert but ignoring the VisibleAssertions that I have in test cases. I hope that is ok ;).

@manikmagar
Copy link
Contributor Author

The latest update to this PR fixes #596, #357 / #345.

@manikmagar
Copy link
Contributor Author

@rnorth @kiview When you get a chance, could you please review this? Thank you!

@rnorth
Copy link
Member

rnorth commented Mar 12, 2018

This PR has a lot of Spock changes showing when I view the diff. Has something strange happened with the branches? 😄

assertEquals("Database name from URL String is used.", "databasename", resultDB);
return true;
});

Copy link
Member

Choose a reason for hiding this comment

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

This is just a trivial comment, but please could you reformat using 4 spaces for indentation? IntelliJ/Eclipse defaults ought to be reasonably close. I'll add automatic formatting soon...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I'll do that. Thanks for reviewing!

Copy link
Contributor Author

@manikmagar manikmagar Mar 13, 2018

Choose a reason for hiding this comment

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

I have updated formatting in recent commit. Thanks!

@manikmagar
Copy link
Contributor Author

Hm, could it be because I merged latest upstream changes from the master? I thought that would make it easy to merge (if appropriate) :)

@rnorth
Copy link
Member

rnorth commented Mar 12, 2018 via email

Copy link
Member

@rnorth rnorth left a comment

Choose a reason for hiding this comment

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

This is good work - thank you for doing this, and I apologise that you've had to refactor quite a few bits along the way. I have a few comments but nothing massive!

@Getter
private Optional<String> queryString;

private ConnectionUrl() {
Copy link
Member

Choose a reason for hiding this comment

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

I think this is redundant, given that there is a public constructor with a parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has been refactored along with adding a static factory method to instantiate the class.

return url.startsWith("jdbc:tc:");
}

public void parseUrl() {
Copy link
Member

Choose a reason for hiding this comment

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

It looks like this is only used together with the constructor. Could we instead have a static factory method that instantiates, parses and returns?

Copy link
Contributor Author

@manikmagar manikmagar Mar 14, 2018

Choose a reason for hiding this comment

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

Makes sense. I have added a static method newInstance(final String url) that returns ready to use instance of ConnectionUrl.

}

Matcher daemonMatcher = Patterns.DAEMON_MATCHING_PATTERN.matcher(this.getUrl());
inDaemonMode = daemonMatcher.matches() ? Boolean.parseBoolean(daemonMatcher.group(2)) : false;
Copy link
Member

Choose a reason for hiding this comment

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

IntelliJ is telling me that this can be simplified to:

inDaemonMode = daemonMatcher.matches() && Boolean.parseBoolean(daemonMatcher.group(2));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has been refactored now.

*
* @return {@link Map}
*/
public Map<String, String> getQueryParameters() {
Copy link
Member

Choose a reason for hiding this comment

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

It looks like this method mutates the queryString field as a side effect of constructing the Map. If someone calls getQueryString too early, they'll get nothing.

Please could we avoid this? Maybe we could make all fields be final (which a static factory method and maybe Lombok could help with)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have modified the query and container parameter extraction method to make them immutable. Now, queryParameters and containerParameters have getters that returns unmodifiable maps and queryString is instantiated only once during initial parsing. Please check.

public interface Patterns {
final Pattern URL_MATCHING_PATTERN = Pattern.compile("jdbc:tc:([a-z]+)(:([^:]+))?://([^\\?]+)(\\?.*)?");

final Pattern ORACLE_URL_MATCHING_PATTERN = Pattern.compile("jdbc:tc:([a-z]+)(:([^(thin:)]+))?:thin:@([^\\?]+)(\\?.*)?");
Copy link
Member

Choose a reason for hiding this comment

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

I apologise for putting you through the pain of working with these regular expressions
😂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I liked it. I never used these many and types of regex pattern, got to learn new things ;), So Thank you!

@@ -80,51 +69,34 @@ public synchronized Connection connect(String url, final Properties info) throws
if (!acceptsURL(url)) {
return null;
}

ConnectionUrl cUrl = new ConnectionUrl(url);
Copy link
Member

Choose a reason for hiding this comment

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

Please could you call this parameter connectionUrl? Maybe it's just me, but every time I see cUrl I think of this. In general we're quite OK with medium-verbosity names for things in this project 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed now 😄

/**
* Factory for MySQL containers.
*/
public class MySQLContainerProvider extends JdbcDatabaseContainerProvider {

/**
* Groups URL of format "jdbc:tc:(databaseType):(optinal_image_tag)//(hostanme)(optional :(numeric_port))/(databasename)(?parameters)"
Copy link
Member

Choose a reason for hiding this comment

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

A couple of typos in here (optinal and hostanme)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Typos are corrected now.

Matcher urlMatcher = MYSQL_URL_MATCHING_PATTERN.matcher(url.getUrl());

if(!urlMatcher.matches()) {
//TODO: Is this necessary?
Copy link
Member

Choose a reason for hiding this comment

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

Probably not necessary if the ConnectionUrl.parseQuery() method has been called..?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. I have removed this matcher related code.

@@ -0,0 +1,48 @@
# TestContainers-Spock
Copy link
Member

Choose a reason for hiding this comment

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

No idea why these Spock files are showing up in the PR - looking at the branch locally I don't see this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure what should I do for this one. Maybe it can be ignored while merging, if appropriate! I don't have any changes for Spock 😄

CHANGELOG.md Outdated
@@ -9,6 +9,7 @@ All notable changes to this project will be documented in this file.
### Changed
- Abstracted and changed database init script functionality to support use of SQL-like scripts with non-JDBC connections. ([\#551](https://github.com/testcontainers/testcontainers-java/pull/551))
- Added `JdbcDatabaseContainer(Future)` constructor. ([\#543](https://github.com/testcontainers/testcontainers-java/issues/543))
- Mark DockerMachineClientProviderStrategy as not persistable ([\#593](https://github.com/testcontainers/testcontainers-java/pull/593))

Copy link
Member

Choose a reason for hiding this comment

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

Please could you update the changelog to describe this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated Changelog with items that I feel related to this PR. Please take a look and let me know/feel free to modify as you seem right.

@VladRassokhin
Copy link
Contributor

VladRassokhin commented Mar 13, 2018

JFYI Unless this PR integrated there's problem in master branch with queryParameter not passed to actual jdbc driver on second and subsequent calls of ContainerDatabaseDriver#connect since it's set to non-empty string only if there's no container yet.
Could you please add test for that?

@manikmagar
Copy link
Contributor Author

manikmagar commented Mar 13, 2018 via email

@manikmagar
Copy link
Contributor Author

@VladRassokhin Thanks for that info. I did add a test and verified behavior. It is working as expected with changes in this PR. Should there be a Bug entry created and referenced here? Anyways, I'll update the PR description too.

@VladRassokhin
Copy link
Contributor

@manikmagar I think no bug entry needed if this PR would be merged prior to any new release

@manikmagar
Copy link
Contributor Author

@rnorth I have made changes to take care of review comments, please take a look when you get chance. Thanks for reviewing.

@VladRassokhin I did make changes to a test case that would fail if query parameters are not used when creating connection from cached container - Please See this.

@jeacott
Copy link

jeacott commented Mar 15, 2018

could use a formatter run over this. lots if apparent diffs are just formatting changes, and the rest aren't consistent. makes it hard to see what has really changed.

@manikmagar
Copy link
Contributor Author

Thanks @jeacott for looking through these changes. I tried to use the same formatting as rest of the code, which is pretty much standard intellij code formatting. Could you please point me to some code where you see it is not appropriate? I'll be happy to revisit and correct appropriately.

@kiview
Copy link
Member

kiview commented Mar 15, 2018

I think there is something off with this PR, showing a lot of changes that aren't changes but have been introduced simply by rebasing to master or something (i.e. the changes in the travis.yml file). I'm not sure where this is coming from?

Oh, I just see @rnorth has already mentioned this :D
Maybe it's easier to simply open a new PR with the current changes?

@Override
public boolean acceptsURL(String url) throws SQLException {
return url.startsWith("jdbc:tc:");
return url.startsWith("jdbc:tc:");

Choose a reason for hiding this comment

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

formatting in this file is inconsistent with the other 4 space indents etc.

if (candidateContainerType.supports(databaseType)) {
container = candidateContainerType.newInstance(tag);
if (candidateContainerType.supports(connectionUrl.getDatabaseType())) {
container = candidateContainerType.newInstance(connectionUrl);

Choose a reason for hiding this comment

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

is this mod to connectionUrl instead of 'tag' intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it is intentional and part of the implementation. Please see PR Description for details.

databaseName = Optional.of(dbInstanceMatcher.group(4));
}

queryParameters = Collections.unmodifiableMap(
Copy link

@jeacott1 jeacott1 Mar 15, 2018

Choose a reason for hiding this comment

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

this doesnt allow for duplicate params which might be very handy if for example I want to set jdbc://...?TC_INITSCRIPT=schema.sql&TC_INITSCRIPT=data.sql

regular expressions seem like a hard work way to parse a jdbc url.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see what you mean but I don't think it is possible even with the existing implementation. I would rather think of that as a separate feature request, maybe @rnorth can weigh in.

Copy link
Member

Choose a reason for hiding this comment

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

Please let's see if we can tackle this without regular expressions separately. I'd be very happy if we could do that and make this code easier to read.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just making sure I understand it correctly. Do You mean, 'Allowing multiple TC_INITSCRIPT like parameter support' should be implemented in separate PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On other thought, maybe a comma delimited list of files is accepted for single TC_INITSCRIPT parameter and executed in the sequence they are provided. Eg. TC_INITSCRIPT=Script1.sql,Script2.sql,Script3.sql.

@@ -93,7 +97,10 @@ public static DockerClientProviderStrategy getFirstValidStrategy(List<DockerClie
LOGGER.warn("Can't instantiate a strategy from {}", it, e);
return Stream.empty();
}
}),
})

Choose a reason for hiding this comment

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

is the formatting in this Stream.concat... block just 'idea' being weird? doesnt seem to follow the 4 space indents everywhere else.

Copy link
Contributor Author

@manikmagar manikmagar Mar 16, 2018

Choose a reason for hiding this comment

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

This class is not modified and related for this PR, probably upstream has changed and so you see the difference. I would leave it as-is for now.

Copy link
Member

@rnorth rnorth Mar 18, 2018

Choose a reason for hiding this comment

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

Please let's see if we can tackle this without regular expressions separately. I'd be very happy if we could do that and make this code easier to read.
Response to wrong comment

Copy link

@jeacott1 jeacott1 left a comment

Choose a reason for hiding this comment

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

I originally thought this looked worse than it is.

@manikmagar
Copy link
Contributor Author

Thanks for taking a look @kiview . I am not sure, why those Spock related changes are showing up as a difference.
@rnorth @kiview Do you guys really think this should be added as a separate PR?

@rnorth
Copy link
Member

rnorth commented Mar 18, 2018

@manikmagar could you please try rebasing onto master and force-push this branch to see if this clears the diff noise?

I've pulled the branch locally and the commits look fine, so I find it a bit disconcerting that we're all seeing phantom changes in the PR. I'm not sure I trust Github to merge it well if it can't show the diff correctly...

@manikmagar
Copy link
Contributor Author

Ok @rnorth , let me try rebasing onto master. In the worst case, I can create a new PR with related changes only.

@manikmagar
Copy link
Contributor Author

I think something is really weird happened with this branch, I will try to create a new branch and re-submit the clean PR on the latest upstream master.

@manikmagar
Copy link
Contributor Author

manikmagar commented Mar 21, 2018

@rnorth @kiview I have made a new PR #617 with all these changes, please take a look at that when you get a chance. If that is merged then this PR should be closed without merging. Thanks!

@kiview
Copy link
Member

kiview commented Mar 23, 2018

@manikmagar Thanks a lot for the new PR, I'll close this one, so we don't get confused.

@kiview kiview closed this Mar 23, 2018
@manikmagar manikmagar deleted the feature/TC_ISSUE_566 branch June 15, 2018 02:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants