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 2021/11/15 01:02:17 UTC
[GitHub] [shardingsphere] ReyYang opened a new pull request #13600: Fro 13558, fix H2DataSourceMetaData.isInSameDatabaseInstance method, …
ReyYang opened a new pull request #13600:
URL: https://github.com/apache/shardingsphere/pull/13600
Add unit test for H2DataSourceMetaData.isInSameDatabaseInstance and DataSourceMetaData.isInSameDatabaseInstance
Fixes #13558 .
Changes proposed in this pull request:
- fix H2DataSourceMetaData.isInSameDatabaseInstance method
- Add unit test for H2DataSourceMetaData.isInSameDatabaseInstance
--
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 change in pull request #13600: Fro 13558, fix H2DataSourceMetaData.isInSameDatabaseInstance method, …
Posted by GitBox <gi...@apache.org>.
sandynz commented on a change in pull request #13600:
URL: https://github.com/apache/shardingsphere/pull/13600#discussion_r749204596
##########
File path: shardingsphere-infra/shardingsphere-infra-common/src/main/java/org/apache/shardingsphere/infra/database/metadata/dialect/H2DataSourceMetaData.java
##########
@@ -63,11 +71,35 @@ public H2DataSourceMetaData(final String url) {
port = setPort ? Integer.parseInt(portFromMatcher) : DEFAULT_PORT;
catalog = null == catalogFromMatcher ? name : catalogFromMatcher;
schema = null;
+ String modelMemFromMatcher = matcher.group("modelMem");
+ String modelSslOrTcpFromMatcher = matcher.group("modelSslOrTcp");
+ String modelFileFromMatcher = matcher.group("modelFile");
+ if (modelMemFromMatcher == null) {
+ model = null == modelSslOrTcpFromMatcher ? modelFileFromMatcher : modelSslOrTcpFromMatcher;
+ } else {
+ model = modelMemFromMatcher;
+ }
}
@Override
public boolean isInSameDatabaseInstance(final DataSourceMetaData dataSourceMetaData) {
+ if (!(dataSourceMetaData instanceof H2DataSourceMetaData)) {
+ return false;
+ }
+ if (!isSameModel(getModel(), ((H2DataSourceMetaData) dataSourceMetaData).getModel())) {
+ return false;
+ }
Review comment:
`mem`, `~`, `file`, `tcp-localhost`, they should be in the same instance.
##########
File path: shardingsphere-infra/shardingsphere-infra-common/src/test/java/org/apache/shardingsphere/infra/database/metadata/dialect/H2DataSourceMetaDataTest.java
##########
@@ -20,8 +20,8 @@
import org.junit.Test;
import static org.hamcrest.CoreMatchers.is;
-import static org.junit.Assert.assertNull;
-import static org.junit.Assert.assertThat;
+import static org.junit.Assert.*;
Review comment:
Importing without star.
--
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 change in pull request #13600: Fro 13558, fix H2DataSourceMetaData.isInSameDatabaseInstance method, …
Posted by GitBox <gi...@apache.org>.
sandynz commented on a change in pull request #13600:
URL: https://github.com/apache/shardingsphere/pull/13600#discussion_r749206057
##########
File path: shardingsphere-infra/shardingsphere-infra-common/src/test/java/org/apache/shardingsphere/infra/database/metadata/DataSourceMetaDataTest.java
##########
@@ -0,0 +1,64 @@
+package org.apache.shardingsphere.infra.database.metadata;
Review comment:
It need license header.
--
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] ReyYang commented on pull request #13600: Fro 13558, fix H2DataSourceMetaData.isInSameDatabaseInstance method, …
Posted by GitBox <gi...@apache.org>.
ReyYang commented on pull request #13600:
URL: https://github.com/apache/shardingsphere/pull/13600#issuecomment-968738646
I have revised this issue as required, and I hope to get your opinion
--
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 #13600: Fro 13558, fix H2DataSourceMetaData.isInSameDatabaseInstance method, …
Posted by GitBox <gi...@apache.org>.
sandynz merged pull request #13600:
URL: https://github.com/apache/shardingsphere/pull/13600
--
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] ReyYang closed pull request #13600: Fro 13558, fix H2DataSourceMetaData.isInSameDatabaseInstance method, …
Posted by GitBox <gi...@apache.org>.
ReyYang closed pull request #13600:
URL: https://github.com/apache/shardingsphere/pull/13600
--
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 change in pull request #13600: Fro 13558, fix H2DataSourceMetaData.isInSameDatabaseInstance method, …
Posted by GitBox <gi...@apache.org>.
sandynz commented on a change in pull request #13600:
URL: https://github.com/apache/shardingsphere/pull/13600#discussion_r749311802
##########
File path: shardingsphere-infra/shardingsphere-infra-common/src/main/java/org/apache/shardingsphere/infra/database/metadata/dialect/H2DataSourceMetaData.java
##########
@@ -63,11 +71,35 @@ public H2DataSourceMetaData(final String url) {
port = setPort ? Integer.parseInt(portFromMatcher) : DEFAULT_PORT;
catalog = null == catalogFromMatcher ? name : catalogFromMatcher;
schema = null;
+ String modelMemFromMatcher = matcher.group("modelMem");
+ String modelSslOrTcpFromMatcher = matcher.group("modelSslOrTcp");
+ String modelFileFromMatcher = matcher.group("modelFile");
+ if (modelMemFromMatcher == null) {
+ model = null == modelSslOrTcpFromMatcher ? modelFileFromMatcher : modelSslOrTcpFromMatcher;
+ } else {
+ model = modelMemFromMatcher;
+ }
}
@Override
public boolean isInSameDatabaseInstance(final DataSourceMetaData dataSourceMetaData) {
+ if (!(dataSourceMetaData instanceof H2DataSourceMetaData)) {
+ return false;
+ }
+ if (!isSameModel(getModel(), ((H2DataSourceMetaData) dataSourceMetaData).getModel())) {
+ return false;
+ }
Review comment:
Not include tcp
--
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] ReyYang commented on pull request #13600: Fro 13558, fix H2DataSourceMetaData.isInSameDatabaseInstance method, …
Posted by GitBox <gi...@apache.org>.
ReyYang commented on pull request #13600:
URL: https://github.com/apache/shardingsphere/pull/13600#issuecomment-968508685
Roger that. I'll keep working on it
--
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] ReyYang closed pull request #13600: Fro 13558, fix H2DataSourceMetaData.isInSameDatabaseInstance method, …
Posted by GitBox <gi...@apache.org>.
ReyYang closed pull request #13600:
URL: https://github.com/apache/shardingsphere/pull/13600
--
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 change in pull request #13600: Fro 13558, fix H2DataSourceMetaData.isInSameDatabaseInstance method, …
Posted by GitBox <gi...@apache.org>.
sandynz commented on a change in pull request #13600:
URL: https://github.com/apache/shardingsphere/pull/13600#discussion_r748981607
##########
File path: shardingsphere-infra/shardingsphere-infra-common/src/main/java/org/apache/shardingsphere/infra/database/metadata/dialect/H2DataSourceMetaData.java
##########
@@ -34,9 +34,13 @@
private static final int DEFAULT_PORT = -1;
private static final String DEFAULT_HOST_NAME = "";
-
+
+ private static final String DEFAULT_H2_MODEL = "";
+
private final String hostName;
-
Review comment:
Indents of empty lines should be kept.
##########
File path: shardingsphere-infra/shardingsphere-infra-common/src/test/java/org/apache/shardingsphere/infra/database/metadata/dialect/H2DataSourceMetaDataTest.java
##########
@@ -68,4 +68,62 @@ public void assertNewConstructorWithFile() {
assertThat(actual.getCatalog(), is("sample"));
assertNull(actual.getSchema());
}
+
+ @Test
+ public void assertIsInSameDatabaseInstanceWithMem() {
+ H2DataSourceMetaData actual1 = new H2DataSourceMetaData("jdbc:h2:mem:ds_0;DB_CLOSE_DELAY=-1;DATABASE_TO_UPPER=false;MODE=MySQL");
+ H2DataSourceMetaData actual2 = new H2DataSourceMetaData("jdbc:h2:mem:ds_1;DB_CLOSE_DELAY=-1;DATABASE_TO_UPPER=false;MODE=MySQL");
+ assertTrue(actual1.isInSameDatabaseInstance(actual2));
+ }
+
+ @Test
+ public void assertIsInSameDatabaseInstanceWithSymbol() {
+ H2DataSourceMetaData actual1 = new H2DataSourceMetaData("jdbc:h2:~:ds-0;DB_CLOSE_DELAY=-1;DATABASE_TO_UPPER=false;MODE=MySQL");
+ H2DataSourceMetaData actual2 = new H2DataSourceMetaData("jdbc:h2:~:ds-1;DB_CLOSE_DELAY=-1;DATABASE_TO_UPPER=false;MODE=MySQL");
+ assertTrue(actual1.isInSameDatabaseInstance(actual2));
+ }
+
+ @Test
+ public void assertFalseIsInSameDatabaseInstance() {
+ H2DataSourceMetaData actual1 = new H2DataSourceMetaData("jdbc:h2:mem:ds_0;DB_CLOSE_DELAY=-1;DATABASE_TO_UPPER=false;MODE=MySQL");
+ H2DataSourceMetaData actual2 = new H2DataSourceMetaData("jdbc:h2:~:ds-1;DB_CLOSE_DELAY=-1;DATABASE_TO_UPPER=false;MODE=MySQL");
+ assertFalse(actual1.isInSameDatabaseInstance(actual2));
+ }
+
+ @Test
+ public void assertIsInSameDatabaseInstanceWithTcp() {
+ H2DataSourceMetaData actual1 = new H2DataSourceMetaData("jdbc:h2:tcp://localhost:8082/~/test1/test2;DB_CLOSE_DELAY=-1");
+ H2DataSourceMetaData actual2 = new H2DataSourceMetaData("jdbc:h2:tcp://localhost:8082/~/test3/test4;DB_CLOSE_DELAY=-1");
+ assertTrue(actual1.isInSameDatabaseInstance(actual2));
+ }
+
+ @Test
+ public void assertFalseIsInSameDatabaseInstanceWithTcp() {
+ H2DataSourceMetaData actual1 = new H2DataSourceMetaData("jdbc:h2:tcp://localhost:8082/~/test1/test2;DB_CLOSE_DELAY=-1");
+ H2DataSourceMetaData actual2 = new H2DataSourceMetaData("jdbc:h2:tcp://192.168.64.76:8082/~/test3/test4;DB_CLOSE_DELAY=-1");
+ assertFalse(actual1.isInSameDatabaseInstance(actual2));
+ }
+
+
Review comment:
Just keep one empty line, delete duplicated ones. And also other places.
##########
File path: shardingsphere-infra/shardingsphere-infra-common/src/main/java/org/apache/shardingsphere/infra/database/metadata/dialect/H2DataSourceMetaData.java
##########
@@ -63,10 +69,23 @@ public H2DataSourceMetaData(final String url) {
port = setPort ? Integer.parseInt(portFromMatcher) : DEFAULT_PORT;
catalog = null == catalogFromMatcher ? name : catalogFromMatcher;
schema = null;
+ String modelPathFromMatcher = matcher.group(1);
+ Matcher modelMatcher = modelPattern.matcher(modelPathFromMatcher);
+ if (!modelMatcher.find()) {
+ throw new UnrecognizedDatabaseURLException(url, pattern.pattern());
+ }
+ String modelFromMatcher = modelMatcher.group(1);
+ model = null == modelFromMatcher ? DEFAULT_H2_MODEL : modelFromMatcher;
}
@Override
public boolean isInSameDatabaseInstance(final DataSourceMetaData dataSourceMetaData) {
+ if (!(dataSourceMetaData instanceof H2DataSourceMetaData)) {
+ return false;
+ }
+ if (!getModel().equals(((H2DataSourceMetaData) dataSourceMetaData).getModel())) {
+ return false;
+ }
Review comment:
Is `DataSourceMetaData.isInSameDatabaseInstance` necessary to update too?
##########
File path: shardingsphere-infra/shardingsphere-infra-common/src/test/java/org/apache/shardingsphere/infra/database/metadata/dialect/H2DataSourceMetaDataTest.java
##########
@@ -68,4 +68,62 @@ public void assertNewConstructorWithFile() {
assertThat(actual.getCatalog(), is("sample"));
assertNull(actual.getSchema());
}
+
+ @Test
+ public void assertIsInSameDatabaseInstanceWithMem() {
+ H2DataSourceMetaData actual1 = new H2DataSourceMetaData("jdbc:h2:mem:ds_0;DB_CLOSE_DELAY=-1;DATABASE_TO_UPPER=false;MODE=MySQL");
+ H2DataSourceMetaData actual2 = new H2DataSourceMetaData("jdbc:h2:mem:ds_1;DB_CLOSE_DELAY=-1;DATABASE_TO_UPPER=false;MODE=MySQL");
+ assertTrue(actual1.isInSameDatabaseInstance(actual2));
+ }
+
+ @Test
+ public void assertIsInSameDatabaseInstanceWithSymbol() {
+ H2DataSourceMetaData actual1 = new H2DataSourceMetaData("jdbc:h2:~:ds-0;DB_CLOSE_DELAY=-1;DATABASE_TO_UPPER=false;MODE=MySQL");
+ H2DataSourceMetaData actual2 = new H2DataSourceMetaData("jdbc:h2:~:ds-1;DB_CLOSE_DELAY=-1;DATABASE_TO_UPPER=false;MODE=MySQL");
+ assertTrue(actual1.isInSameDatabaseInstance(actual2));
+ }
+
+ @Test
+ public void assertFalseIsInSameDatabaseInstance() {
+ H2DataSourceMetaData actual1 = new H2DataSourceMetaData("jdbc:h2:mem:ds_0;DB_CLOSE_DELAY=-1;DATABASE_TO_UPPER=false;MODE=MySQL");
+ H2DataSourceMetaData actual2 = new H2DataSourceMetaData("jdbc:h2:~:ds-1;DB_CLOSE_DELAY=-1;DATABASE_TO_UPPER=false;MODE=MySQL");
+ assertFalse(actual1.isInSameDatabaseInstance(actual2));
Review comment:
All of them are in memory, seems it should be true.
##########
File path: shardingsphere-infra/shardingsphere-infra-common/src/main/java/org/apache/shardingsphere/infra/database/metadata/dialect/H2DataSourceMetaData.java
##########
@@ -46,7 +50,9 @@
private final Pattern pattern = Pattern.compile("jdbc:h2:((mem|~)[:/](?<catalog>[\\w\\-]+)|"
+ "(ssl:|tcp:)(//)?(?<hostName>[\\w\\-.]+)(:(?<port>[0-9]{1,4})/)?[/~\\w\\-.]+/(?<name>[\\-\\w]*)|"
+ "file:[/~\\w\\-]+/(?<fileName>[\\-\\w]*));?\\S*", Pattern.CASE_INSENSITIVE);
-
+
+ private final Pattern modelPattern = Pattern.compile("((mem|~)[:/]|(ssl:|tcp:)|(file:))");
Review comment:
It's better to merge into one regex pattern for better performance.
##########
File path: shardingsphere-infra/shardingsphere-infra-common/src/test/java/org/apache/shardingsphere/infra/database/metadata/DataSourceMetaDataTest.java
##########
@@ -0,0 +1,184 @@
+package org.apache.shardingsphere.infra.database.metadata;
+
+import org.apache.shardingsphere.infra.database.metadata.dialect.H2DataSourceMetaData;
+import org.apache.shardingsphere.infra.database.metadata.dialect.MariaDBDataSourceMetaData;
+import org.apache.shardingsphere.infra.database.metadata.dialect.MySQLDataSourceMetaData;
+import org.apache.shardingsphere.infra.database.metadata.dialect.OracleDataSourceMetaData;
+import org.apache.shardingsphere.infra.database.metadata.dialect.PostgreSQLDataSourceMetaData;
+import org.apache.shardingsphere.infra.database.metadata.dialect.SQL92DataSourceMetaData;
+import org.apache.shardingsphere.infra.database.metadata.dialect.SQLServerDataSourceMetaData;
+import org.junit.Test;
+
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+public final class DataSourceMetaDataTest {
+
+ @Test
+ public void assertIsInSameDatabaseInstanceWithMysql() {
+ MySQLDataSourceMetaData actual1 = new MySQLDataSourceMetaData("jdbc:mysql://127.0.0.1:9999/ds_1?serverTimezone=UTC&useSSL=false");
+ MySQLDataSourceMetaData actual2 = new MySQLDataSourceMetaData("jdbc:mysql://127.0.0.1:9999/ds_0?serverTimezone=UTC&useSSL=false");
+ assertTrue(actual1.isInSameDatabaseInstance(actual2));
+ }
+
+ @Test
+ public void assertIsInSameDatabaseInstanceWithOracle() {
+ OracleDataSourceMetaData actual1 = new OracleDataSourceMetaData("jdbc:oracle:thin:@//127.0.0.1:9999/ds_0", "test");
+ OracleDataSourceMetaData actual2 = new OracleDataSourceMetaData("jdbc:oracle:thin:@//127.0.0.1:9999/ds_1", "test");
+ assertTrue(actual1.isInSameDatabaseInstance(actual2));
+ }
+
+ @Test
+ public void assertIsInSameDatabaseInstanceWithMariaDB() {
+ MariaDBDataSourceMetaData actual1 = new MariaDBDataSourceMetaData("jdbc:mariadb://127.0.0.1:9999/ds_0?serverTimezone=UTC&useSSL=false");
+ MariaDBDataSourceMetaData actual2 = new MariaDBDataSourceMetaData("jdbc:mariadb://127.0.0.1:9999/ds_1?serverTimezone=UTC&useSSL=false");
+ assertTrue(actual1.isInSameDatabaseInstance(actual2));
+ }
+
+ @Test
+ public void assertIsInSameDatabaseInstanceWithPostgreSQL() {
+ PostgreSQLDataSourceMetaData actual1 = new PostgreSQLDataSourceMetaData("jdbc:postgresql://127.0.0.1/ds_0");
+ PostgreSQLDataSourceMetaData actual2 = new PostgreSQLDataSourceMetaData("jdbc:postgresql://127.0.0.1/ds_1");
+ assertTrue(actual1.isInSameDatabaseInstance(actual2));
+ }
+
+ @Test
+ public void assertIsInSameDatabaseInstanceWithSQL92() {
+ SQL92DataSourceMetaData actual1 = new SQL92DataSourceMetaData("jdbc:sql92_db:ds_0");
+ SQL92DataSourceMetaData actual2 = new SQL92DataSourceMetaData("jdbc:sql92_db:ds_1");
+ assertTrue(actual1.isInSameDatabaseInstance(actual2));
+ }
+
+ @Test
+ public void assertIsInSameDatabaseInstanceWithSQLServer() {
+ SQLServerDataSourceMetaData actual1 = new SQLServerDataSourceMetaData("jdbc:microsoft:sqlserver://127.0.0.1:9999;DatabaseName=ds_0");
+ SQLServerDataSourceMetaData actual2 = new SQLServerDataSourceMetaData("jdbc:microsoft:sqlserver://127.0.0.1:9999;DatabaseName=ds_1");
+ assertTrue(actual1.isInSameDatabaseInstance(actual2));
+ }
+
+ @Test
+ public void assertFalseIsInSameDatabaseInstanceWithMysqlAndH2() {
+ H2DataSourceMetaData actualH2 = new H2DataSourceMetaData("jdbc:h2:mem:ds_0;DB_CLOSE_DELAY=-1;DATABASE_TO_UPPER=false;MODE=MySQL");
+ MySQLDataSourceMetaData actualMysql = new MySQLDataSourceMetaData("jdbc:mysql://127.0.0.1:9999/ds_0?serverTimezone=UTC&useSSL=false");
+ assertFalse(actualMysql.isInSameDatabaseInstance(actualH2));
+ }
+
+ @Test
+ public void assertFalseIsInSameDatabaseInstanceWithMysqlAndOracle() {
+ OracleDataSourceMetaData actualOracle = new OracleDataSourceMetaData("jdbc:oracle:thin:@//127.0.0.1:9988/ds_0", "test");
+ MySQLDataSourceMetaData actualMysql = new MySQLDataSourceMetaData("jdbc:mysql://127.0.0.1:9999/ds_0?serverTimezone=UTC&useSSL=false");
+ assertFalse(actualMysql.isInSameDatabaseInstance(actualOracle));
+ }
Review comment:
We could simplify following similar test cases, just cover more code branches to improve test coverage.
##########
File path: shardingsphere-infra/shardingsphere-infra-common/src/main/java/org/apache/shardingsphere/infra/database/metadata/dialect/H2DataSourceMetaData.java
##########
@@ -46,7 +50,9 @@
private final Pattern pattern = Pattern.compile("jdbc:h2:((mem|~)[:/](?<catalog>[\\w\\-]+)|"
Review comment:
`pattern` could be static for better performance. (Extra work)
--
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