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 03:35:17 UTC
[GitHub] [shardingsphere] sandynz commented on a change in pull request #13600: Fro 13558, fix H2DataSourceMetaData.isInSameDatabaseInstance method, …
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