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.

Sign up for GitHub

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

Jump to bottom

Apply simple filtering of the query string passed through to real database drivers #357

Closed
wants to merge 1 commit into from
Closed

Apply simple filtering of the query string passed through to real database drivers #357

wants to merge 1 commit into from

Conversation

rnorth
Copy link
Member

@rnorth rnorth commented Jun 4, 2017

Refs #345

bsideup
bsideup reviewed Jun 4, 2017
View reviewed changes
modules/jdbc/src/main/java/org/testcontainers/jdbc/ContainerDatabaseDriver.java
@@ -175,6 +176,14 @@ public synchronized Connection connect(String url, final Properties info) throws
return results;
}

private String getQueryStringWithoutTCParams(String queryString) {
return TC_PARAM_MATCHING_PATTERN.matcher(queryString)
Copy link
Member

Choose a reason for hiding this comment

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

What if we just re-create URL without TC_ query parameters?
Something like https://stackoverflow.com/a/41725109

@kiview kiview mentioned this pull request Jun 5, 2017
Closed
kiview
kiview requested changes Jun 9, 2017
View reviewed changes
modules/jdbc-test/src/test/java/org/testcontainers/jdbc/JDBCDriverTest.java
@@ -38,7 +38,7 @@
new Object[][]{
{"jdbc:tc:mysql:5.5.43://hostname/databasename", false, false, false},
{"jdbc:tc:mysql://hostname/databasename?TC_INITSCRIPT=somepath/init_mysql.sql", true, false, false},
{"jdbc:tc:mysql://hostname/databasename?TC_INITFUNCTION=org.testcontainers.jdbc.JDBCDriverTest::sampleInitFunction", true, false, false},
{"jdbc:tc:mysql://hostname/databasename?TC_INITFUNCTION=org.testcontainers.jdbc.JDBCDriverTest::sampleInitFunction&useUnicode=yes", true, false, false},
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 test isn't adding any verification of the actual implementation, since the tests will succeed even if I change the code in ContainerDatabaseDriver to the original implementation.

TBH I don't have a better idea to test matching and building the JDBC url but refactoring this component out into it's own class and unit testing it. If you like I can take on this work and also switch to using an URL builder like @bsideup suggested.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, fair point - I'll have a bit more of a think about it, and probably break out into a unit test.

I do have a branch for refactoring towards the Apache URL parser, but I'm not quite happy with it yet. It was the same amount of code (or actually I think a few more lines), and ends up mixing the two URL parsing approaches anyway. We probably shouldn't use both. IIRC I originally used regexes because JDBC URLs couldn't be parsed (colon in scheme part), but I could be wrong. I may have just been using the JRE URL parsing libs; perhaps the Apache library can handle it though.

I'll have a try!

@sooty1892 sooty1892 mentioned this pull request Jul 16, 2017
Closed
@rnorth rnorth mentioned this pull request Feb 3, 2018
Merged
@rnorth rnorth mentioned this pull request Feb 26, 2018
Closed
@rnorth
Copy link
Member Author

rnorth commented Mar 13, 2018

With #594 about to land I think I'll close this.

@rnorth rnorth closed this Mar 13, 2018
rnorth pushed a commit that referenced this pull request Mar 17, 2019
@dependabot @rnorth
Bump mockito-core from 2.24.5 to 2.25.0 in /core (#1294)
e326e3e 
Bumps [mockito-core](https://github.com/mockito/mockito) from 2.24.5 to 2.25.0.
<details>
<summary>Release notes</summary>

*Sourced from [mockito-core's releases](https://github.com/mockito/mockito/releases).*

> ## v2.25.0
> <sup><sup>*Release notes were automatically generated by [Shipkit](http://shipkit.org/)*</sup></sup>
> 
> #### 2.25.0
>  - 2019-03-05 - [11 commits](mockito/mockito@v2.24.10...v2.25.0) by [Szczepan Faber](https://github.com/mockitoguy) (10), Garfield Tan (1) - published to [![Bintray](https://img.shields.io/badge/Bintray-2.25.0-green.svg)](https://bintray.com/mockito/maven/mockito/2.25.0)
>  - New API to clean up all inline mocks after test [([#1619](https://github-redirect.dependabot.com/mockito/mockito/issues/1619))](https://github-redirect.dependabot.com/mockito/mockito/pull/1619)
>  - Memory leak in mockito-inline calling method on mock with at least a mock as parameter [([#1614](https://github-redirect.dependabot.com/mockito/mockito/issues/1614))](https://github-redirect.dependabot.com/mockito/mockito/issues/1614)
>  - Cross-references and a single spy cause memory leak [([#1533](https://github-redirect.dependabot.com/mockito/mockito/issues/1533))](https://github-redirect.dependabot.com/mockito/mockito/issues/1533)
>  - Nested spies cause memory leaks  [([#1532](https://github-redirect.dependabot.com/mockito/mockito/issues/1532))](https://github-redirect.dependabot.com/mockito/mockito/issues/1532)
> 
> ## v2.24.10
> <sup><sup>*Release notes were automatically generated by [Shipkit](http://shipkit.org/)*</sup></sup>
> 
> #### 2.24.10
>  - 2019-03-05 - [4 commits](mockito/mockito@v2.24.9...v2.24.10) by [Tim van der Lippe](https://github.com/TimvdLippe) - published to [![Bintray](https://img.shields.io/badge/Bintray-2.24.10-green.svg)](https://bintray.com/mockito/maven/mockito-development/2.24.10)
>  - Prevent NPE in findTypeFromGenericInArguments [([#1648](https://github-redirect.dependabot.com/mockito/mockito/issues/1648))](https://github-redirect.dependabot.com/mockito/mockito/pull/1648)
> 
> ## v2.24.9
> <sup><sup>*Release notes were automatically generated by [Shipkit](http://shipkit.org/)*</sup></sup>
> 
> #### 2.24.9
>  - 2019-03-04 - [12 commits](mockito/mockito@v2.24.7...v2.24.9) by 6 authors - published to [![Bintray](https://img.shields.io/badge/Bintray-2.24.9-green.svg)](https://bintray.com/mockito/maven/mockito-development/2.24.9)
>  - Commits: [Brice Dutheil](https://github.com/bric3) (5), [Tim van der Lippe](https://github.com/TimvdLippe) (3), [epeee](https://github.com/epeee) (1), [Fr Jeremy Krieg](https://github.com/kriegfrj) (1), [PaweB PamuBa](https://github.com/PawelPamula) (1), shipkit-org (1)
>  - [Java 9 support] ClassCastExceptions with JDK9 javac [([#357](https://github-redirect.dependabot.com/mockito/mockito/issues/357))](https://github-redirect.dependabot.com/mockito/mockito/issues/357)
>  - [Bugfixes] RETURNS_DEEP_STUBS causes "Raw extraction not supported for : 'null'" in some cases [([#1621](https://github-redirect.dependabot.com/mockito/mockito/issues/1621))](https://github-redirect.dependabot.com/mockito/mockito/issues/1621)
>  - Update shipkit plugin (v2.1.6) [([#1647](https://github-redirect.dependabot.com/mockito/mockito/issues/1647))](https://github-redirect.dependabot.com/mockito/mockito/pull/1647)
>  - VerificationCollector to handle non-matching args and other assertions [([#1644](https://github-redirect.dependabot.com/mockito/mockito/issues/1644))](https://github-redirect.dependabot.com/mockito/mockito/pull/1644)
>  - VerificationCollector doesn't work for invocations with non-matching args [([#1642](https://github-redirect.dependabot.com/mockito/mockito/issues/1642))](https://github-redirect.dependabot.com/mockito/mockito/issues/1642)
>  - Fix returns mocks for final classes [([#1641](https://github-redirect.dependabot.com/mockito/mockito/issues/1641))](https://github-redirect.dependabot.com/mockito/mockito/pull/1641)
>  - Removes inaccessible links from javadocs in Mockito.java [([#1639](https://github-redirect.dependabot.com/mockito/mockito/issues/1639))](https://github-redirect.dependabot.com/mockito/mockito/pull/1639)
>  - Mockito.java contains inaccessible links to articles. [([#1638](https://github-redirect.dependabot.com/mockito/mockito/issues/1638))](https://github-redirect.dependabot.com/mockito/mockito/issues/1638)
>  - Handle terminal type var with bounds [([#1624](https://github-redirect.dependabot.com/mockito/mockito/issues/1624))](https://github-redirect.dependabot.com/mockito/mockito/pull/1624)
>  - Return null instead of causing a CCE [([#1612](https://github-redirect.dependabot.com/mockito/mockito/issues/1612))](https://github-redirect.dependabot.com/mockito/mockito/pull/1612)
> 
> ## v2.24.8
> <sup><sup>*Release notes were automatically generated by [Shipkit](http://shipkit.org/)*</sup></sup>
> 
> #### 2.24.8
>  - 2019-03-01 - [2 commits](mockito/mockito@v2.24.7...v2.24.8) by [PaweB PamuBa](https://github.com/PawelPamula) (1), [shipkit-org](https://github.com/shipkit-org) (1) - published to [![Bintray](https://img.shields.io/badge/Bintray-2.24.8-green.svg)](https://bintray.com/mockito/maven/mockito-development/2.24.8)
>  - Removes inaccessible links from javadocs in Mockito.java [([#1639](https://github-redirect.dependabot.com/mockito/mockito/issues/1639))](https://github-redirect.dependabot.com/mockito/mockito/pull/1639)
>  - Mockito.java contains inaccessible links to articles. [([#1638](https://github-redirect.dependabot.com/mockito/mockito/issues/1638))](https://github-redirect.dependabot.com/mockito/mockito/issues/1638)
> 
> ## v2.24.7
> <sup><sup>*Release notes were automatically generated by [Shipkit](http://shipkit.org/)*</sup></sup>
> 
> #### 2.24.7
>  - 2019-02-28 - [2 commits](mockito/mockito@v2.24.6...v2.24.7) by [shipkit-org](https://github.com/shipkit-org) (1), [Tim van der Lippe](https://github.com/TimvdLippe) (1) - published to [![Bintray](https://img.shields.io/badge/Bintray-2.24.7-green.svg)](https://bintray.com/mockito/maven/mockito-development/2.24.7)
>  - Fix handling of generics in ReturnsMocks [([#1635](https://github-redirect.dependabot.com/mockito/mockito/issues/1635))](https://github-redirect.dependabot.com/mockito/mockito/pull/1635)
> 
> ## v2.24.6
></tr></table> ... (truncated)
</details>
<details>
<summary>Commits</summary>

- [`70c6fa4`](mockito/mockito@70c6fa4) 2.25.0 release (previous 2.24.10) + release notes updated by CI build 3956
- [`a7ee084`](mockito/mockito@a7ee084) Merge pull request [#1619](https://github-redirect.dependabot.com/mockito/mockito/issues/1619) from ttanxu/release/2.x
- [`959cba4`](mockito/mockito@959cba4) Merge branch 'release/2.x' into release/2.x
- [`1641868`](mockito/mockito@1641868) Javadoc
- [`d61a8be`](mockito/mockito@d61a8be) Added missing coverage
- [`af2d33b`](mockito/mockito@af2d33b) 2.24.10 release (previous 2.24.9) + release notes updated by CI build 3953
- [`1183d1f`](mockito/mockito@1183d1f) Merge pull request [#1648](https://github-redirect.dependabot.com/mockito/mockito/issues/1648) from mockito/TimvdLippe-patch-1
- [`2894895`](mockito/mockito@2894895) Fix checkstyle warnings
- [`2d21a5a`](mockito/mockito@2d21a5a) Add regression test
- [`b5e9400`](mockito/mockito@b5e9400) Prevent NPE in findTypeFromGenericInArguments
- Additional commits viewable in [compare view](mockito/mockito@v2.24.5...v2.25.0)
</details>
<br />

[![Dependabot compatibility score](https://api.dependabot.com/badges/compatibility_score?dependency-name=org.mockito:mockito-core&package-manager=gradle&previous-version=2.24.5&new-version=2.25.0)](https://dependabot.com/compatibility-score.html?dependency-name=org.mockito:mockito-core&package-manager=gradle&previous-version=2.24.5&new-version=2.25.0)

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`.

[//]: # (dependabot-automerge-start)
If all status checks pass Dependabot will automatically merge this pull request.

[//]: # (dependabot-automerge-end)

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot squash and merge` will squash and merge this PR after your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge and block automerging
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot ignore this [patch|minor|major] version` will close this PR and stop Dependabot creating any more for this minor/major version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
- `@dependabot badge me` will comment on this PR with code to add a "Dependabot enabled" badge to your readme

Additionally, you can set the following in the `.dependabot/config.yml` file in this repo:
- Update frequency (including time of day and day of week)
- Automerge options (never/patch/minor, and dev/runtime dependencies)
- Pull request limits (per update run and/or open at any time)
- Out-of-range updates (receive only lockfile updates, if desired)
- Security updates (receive only security updates, if desired)

Finally, you can contact us by mentioning @dependabot.

</details>
@rnorth rnorth deleted the prevent-tc-driver-param-leakage branch June 18, 2020 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bsideup bsideup bsideup left review comments

@kiview kiview kiview requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rnorth @bsideup @kiview