You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@shardingsphere.apache.org by GitBox <gi...@apache.org> on 2022/12/14 02:41:31 UTC
[GitHub] [shardingsphere] linghengqian opened a new pull request, #22864: Make StandardJdbcUrlParser match Url rules of TestContainers JDBC normally
linghengqian opened a new pull request, #22864:
URL: https://github.com/apache/shardingsphere/pull/22864
Fixes #22737.
Changes proposed in this pull request:
- Make StandardJdbcUrlParser match Url rules of TestContainers JDBC normally.
---
Before committing this PR, I'm sure that I have checked the following options:
- [ ] My code follows the [code of conduct](https://shardingsphere.apache.org/community/en/involved/conduct/code/) of this project.
- [ ] I have self-reviewed the commit code.
- [ ] I have (or in comment I request) added corresponding labels for the pull request.
- [ ] I have passed maven check locally : `./mvnw clean install -B -T1C -Dmaven.javadoc.skip -Dmaven.jacoco.skip -e`.
- [ ] I have made corresponding changes to the documentation.
- [ ] I have added corresponding unit tests for my changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] linghengqian commented on a diff in pull request #22864: Make StandardJdbcUrlParser match Url rules of TestContainers JDBC normally
Posted by GitBox <gi...@apache.org>.
linghengqian commented on code in PR #22864:
URL: https://github.com/apache/shardingsphere/pull/22864#discussion_r1048273097
##########
infra/common/src/main/java/org/apache/shardingsphere/infra/database/metadata/url/StandardJdbcUrlParser.java:
##########
@@ -31,7 +31,7 @@
*/
public final class StandardJdbcUrlParser {
- private static final String SCHEMA_PATTERN = "(?<schema>[\\w+:%]+)\\s*";
+ private static final String SCHEMA_PATTERN = "(?<schema>[(\\w-.)+:%]+)\\s*";
Review Comment:
- For 1, and maybe it's just because I'm not familiar with regular expressions, I can remove the extra parentheses.
- For 2, the current rule obviously doesn't, because I didn't escape the characters, but I can't seem to relate what jdbcurl with parentheses looks like. Any examples?🤔
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] sandynz commented on a diff in pull request #22864: Make StandardJdbcUrlParser match Url rules of TestContainers JDBC normally
Posted by GitBox <gi...@apache.org>.
sandynz commented on code in PR #22864:
URL: https://github.com/apache/shardingsphere/pull/22864#discussion_r1048210878
##########
infra/common/src/main/java/org/apache/shardingsphere/infra/database/metadata/url/StandardJdbcUrlParser.java:
##########
@@ -31,7 +31,7 @@
*/
public final class StandardJdbcUrlParser {
- private static final String SCHEMA_PATTERN = "(?<schema>[\\w+:%]+)\\s*";
+ private static final String SCHEMA_PATTERN = "(?<schema>[(\\w-.)+:%]+)\\s*";
Review Comment:
When I removed `(` and `)`, unit test passed. Some questions:
1, Are they required?
2, Are `(` and `)` supported in JDBC URL schema?
##########
infra/common/src/test/java/org/apache/shardingsphere/infra/database/metadata/url/StandardJdbcUrlParserTest.java:
##########
@@ -81,4 +81,23 @@ public void assertParseMicrosoftSQLServerJdbcUrl() {
public void assertParseIncorrectURL() {
new StandardJdbcUrlParser().parse("jdbc:h2:mem:test;DB_CLOSE_DELAY=-1;DATABASE_TO_UPPER=false;MODE=MySQL");
}
+
+ @Test
+ public void assertParseTestContainersJDBCUrl() {
+ assertThat(new StandardJdbcUrlParser().parse("jdbc:tc:mysql:5.7.34:///demo_ds").getDatabase(), is("demo_ds"));
+ assertThat(new StandardJdbcUrlParser().parse("jdbc:tc:postgresql:9.6.8:///demo_ds").getDatabase(), is("demo_ds"));
+ assertThat(new StandardJdbcUrlParser().parse("jdbc:tc:postgis:9.6-2.5:///demo_ds").getDatabase(), is("demo_ds"));
+ assertThat(new StandardJdbcUrlParser().parse("jdbc:tc:timescaledb:2.1.0-pg13:///demo_ds").getDatabase(), is("demo_ds"));
+ assertThat(new StandardJdbcUrlParser().parse("jdbc:tc:trino:352://localhost/memory/default").getDatabase(), is("memory/default"));
Review Comment:
That's ok. It's in the [test container examples]( https://www.testcontainers.org/modules/databases/jdbc/#jdbc-url-examples ).
But I didn't find all of them. Could you add test container example links in PR page?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] codecov-commenter commented on pull request #22864: Make StandardJdbcUrlParser match Url rules of TestContainers JDBC normally
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #22864:
URL: https://github.com/apache/shardingsphere/pull/22864#issuecomment-1350323383
# [Codecov](https://codecov.io/gh/apache/shardingsphere/pull/22864?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#22864](https://codecov.io/gh/apache/shardingsphere/pull/22864?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (69c9165) into [master](https://codecov.io/gh/apache/shardingsphere/commit/5c7ee9a59ab00e4d8424787893d94e587ded80a6?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (5c7ee9a) will **decrease** coverage by `0.00%`.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## master #22864 +/- ##
============================================
- Coverage 49.30% 49.30% -0.01%
Complexity 2458 2458
============================================
Files 4141 4141
Lines 57846 57846
Branches 9917 9917
============================================
- Hits 28523 28520 -3
- Misses 26858 26861 +3
Partials 2465 2465
```
| [Impacted Files](https://codecov.io/gh/apache/shardingsphere/pull/22864?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...a/database/metadata/url/StandardJdbcUrlParser.java](https://codecov.io/gh/apache/shardingsphere/pull/22864/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-aW5mcmEvY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9zaGFyZGluZ3NwaGVyZS9pbmZyYS9kYXRhYmFzZS9tZXRhZGF0YS91cmwvU3RhbmRhcmRKZGJjVXJsUGFyc2VyLmphdmE=) | `100.00% <ø> (ø)` | |
| [...handler/distsql/ral/hint/enums/HintSourceType.java](https://codecov.io/gh/apache/shardingsphere/pull/22864/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHJveHkvYmFja2VuZC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2hhcmRpbmdzcGhlcmUvcHJveHkvYmFja2VuZC9oYW5kbGVyL2Rpc3RzcWwvcmFsL2hpbnQvZW51bXMvSGludFNvdXJjZVR5cGUuamF2YQ==) | `0.00% <0.00%> (-42.86%)` | :arrow_down: |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] linghengqian commented on a diff in pull request #22864: Make StandardJdbcUrlParser match Url rules of TestContainers JDBC normally
Posted by GitBox <gi...@apache.org>.
linghengqian commented on code in PR #22864:
URL: https://github.com/apache/shardingsphere/pull/22864#discussion_r1048277483
##########
infra/common/src/test/java/org/apache/shardingsphere/infra/database/metadata/url/StandardJdbcUrlParserTest.java:
##########
@@ -81,4 +81,23 @@ public void assertParseMicrosoftSQLServerJdbcUrl() {
public void assertParseIncorrectURL() {
new StandardJdbcUrlParser().parse("jdbc:h2:mem:test;DB_CLOSE_DELAY=-1;DATABASE_TO_UPPER=false;MODE=MySQL");
}
+
+ @Test
+ public void assertParseTestContainersJDBCUrl() {
+ assertThat(new StandardJdbcUrlParser().parse("jdbc:tc:mysql:5.7.34:///demo_ds").getDatabase(), is("demo_ds"));
+ assertThat(new StandardJdbcUrlParser().parse("jdbc:tc:postgresql:9.6.8:///demo_ds").getDatabase(), is("demo_ds"));
+ assertThat(new StandardJdbcUrlParser().parse("jdbc:tc:postgis:9.6-2.5:///demo_ds").getDatabase(), is("demo_ds"));
+ assertThat(new StandardJdbcUrlParser().parse("jdbc:tc:timescaledb:2.1.0-pg13:///demo_ds").getDatabase(), is("demo_ds"));
+ assertThat(new StandardJdbcUrlParser().parse("jdbc:tc:trino:352://localhost/memory/default").getDatabase(), is("memory/default"));
Review Comment:
All the examples are from https://www.testcontainers.org/modules/databases/jdbc/#jdbc-url-examples . Is there anything missing?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] sandynz commented on a diff in pull request #22864: Make StandardJdbcUrlParser match Url rules of TestContainers JDBC normally
Posted by GitBox <gi...@apache.org>.
sandynz commented on code in PR #22864:
URL: https://github.com/apache/shardingsphere/pull/22864#discussion_r1048352844
##########
infra/common/src/test/java/org/apache/shardingsphere/infra/database/metadata/url/StandardJdbcUrlParserTest.java:
##########
@@ -81,4 +81,23 @@ public void assertParseMicrosoftSQLServerJdbcUrl() {
public void assertParseIncorrectURL() {
new StandardJdbcUrlParser().parse("jdbc:h2:mem:test;DB_CLOSE_DELAY=-1;DATABASE_TO_UPPER=false;MODE=MySQL");
}
+
+ @Test
+ public void assertParseTestContainersJDBCUrl() {
+ assertThat(new StandardJdbcUrlParser().parse("jdbc:tc:mysql:5.7.34:///demo_ds").getDatabase(), is("demo_ds"));
+ assertThat(new StandardJdbcUrlParser().parse("jdbc:tc:postgresql:9.6.8:///demo_ds").getDatabase(), is("demo_ds"));
+ assertThat(new StandardJdbcUrlParser().parse("jdbc:tc:postgis:9.6-2.5:///demo_ds").getDatabase(), is("demo_ds"));
+ assertThat(new StandardJdbcUrlParser().parse("jdbc:tc:timescaledb:2.1.0-pg13:///demo_ds").getDatabase(), is("demo_ds"));
+ assertThat(new StandardJdbcUrlParser().parse("jdbc:tc:trino:352://localhost/memory/default").getDatabase(), is("memory/default"));
Review Comment:
OK, I just had a glance at the page and missed some of them
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] sandynz commented on a diff in pull request #22864: Make StandardJdbcUrlParser match Url rules of TestContainers JDBC normally
Posted by GitBox <gi...@apache.org>.
sandynz commented on code in PR #22864:
URL: https://github.com/apache/shardingsphere/pull/22864#discussion_r1048349818
##########
infra/common/src/main/java/org/apache/shardingsphere/infra/database/metadata/url/StandardJdbcUrlParser.java:
##########
@@ -31,7 +31,7 @@
*/
public final class StandardJdbcUrlParser {
- private static final String SCHEMA_PATTERN = "(?<schema>[\\w+:%]+)\\s*";
+ private static final String SCHEMA_PATTERN = "(?<schema>[(\\w-.)+:%]+)\\s*";
Review Comment:
It's better to remove `(` and `)` for now. Since:
1. It's uncommon in JDBC URL, I didn't find `(` and `)` definition in JDBC specificaiton and real driver url.
2. They have special meaning in regex, it looks confusing in the pattern.
From https://download.oracle.com/otn-pub/jcp/jdbc-4_1-mrel-spec/jdbc4.1-fr-spec.pdf
```
The format of a JDBC URL is :
â– jdbc:<subprotocol>:<subname>
where subprotocol defines the kind of database connectivity mechanism that may be supported by one or more drivers. The contents and syntax of the subname will depend on the subprotocol.
Note – A JDBC URL is not required to fully adhere to the URI syntax as defined in RFC 3986, Uniform Resource Identifier (URI): Generic Syntax.
```
From https://docs.oracle.com/cd/E17952_01/connector-j-8.0-en/connector-j-reference-jdbc-url-format.html
No `(` and `)` is found.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] linghengqian commented on a diff in pull request #22864: Make StandardJdbcUrlParser match Url rules of TestContainers JDBC normally
Posted by GitBox <gi...@apache.org>.
linghengqian commented on code in PR #22864:
URL: https://github.com/apache/shardingsphere/pull/22864#discussion_r1048389069
##########
infra/common/src/main/java/org/apache/shardingsphere/infra/database/metadata/url/StandardJdbcUrlParser.java:
##########
@@ -31,7 +31,7 @@
*/
public final class StandardJdbcUrlParser {
- private static final String SCHEMA_PATTERN = "(?<schema>[\\w+:%]+)\\s*";
+ private static final String SCHEMA_PATTERN = "(?<schema>[(\\w-.)+:%]+)\\s*";
Review Comment:
done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] linghengqian commented on a diff in pull request #22864: Make StandardJdbcUrlParser match Url rules of TestContainers JDBC normally
Posted by GitBox <gi...@apache.org>.
linghengqian commented on code in PR #22864:
URL: https://github.com/apache/shardingsphere/pull/22864#discussion_r1047969519
##########
infra/common/src/test/java/org/apache/shardingsphere/infra/database/metadata/url/StandardJdbcUrlParserTest.java:
##########
@@ -81,4 +81,23 @@ public void assertParseMicrosoftSQLServerJdbcUrl() {
public void assertParseIncorrectURL() {
new StandardJdbcUrlParser().parse("jdbc:h2:mem:test;DB_CLOSE_DELAY=-1;DATABASE_TO_UPPER=false;MODE=MySQL");
}
+
+ @Test
+ public void assertParseTestContainersJDBCUrl() {
+ assertThat(new StandardJdbcUrlParser().parse("jdbc:tc:mysql:5.7.34:///demo_ds").getDatabase(), is("demo_ds"));
+ assertThat(new StandardJdbcUrlParser().parse("jdbc:tc:postgresql:9.6.8:///demo_ds").getDatabase(), is("demo_ds"));
+ assertThat(new StandardJdbcUrlParser().parse("jdbc:tc:postgis:9.6-2.5:///demo_ds").getDatabase(), is("demo_ds"));
+ assertThat(new StandardJdbcUrlParser().parse("jdbc:tc:timescaledb:2.1.0-pg13:///demo_ds").getDatabase(), is("demo_ds"));
+ assertThat(new StandardJdbcUrlParser().parse("jdbc:tc:trino:352://localhost/memory/default").getDatabase(), is("memory/default"));
Review Comment:
I've never used trino , I'm assuming that's the correct database name.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] sandynz merged pull request #22864: Make StandardJdbcUrlParser match Url rules of TestContainers JDBC normally
Posted by GitBox <gi...@apache.org>.
sandynz merged PR #22864:
URL: https://github.com/apache/shardingsphere/pull/22864
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org