You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@shardingsphere.apache.org by "Pace2Car (via GitHub)" <gi...@apache.org> on 2023/04/07 03:44:42 UTC
[GitHub] [shardingsphere] Pace2Car commented on a diff in pull request #25036: Improve properties verification of TrafficAlgorithm #24754
Pace2Car commented on code in PR #25036:
URL: https://github.com/apache/shardingsphere/pull/25036#discussion_r1160411844
##########
kernel/traffic/core/src/main/java/org/apache/shardingsphere/traffic/algorithm/traffic/segment/SQLMatchTrafficAlgorithm.java:
##########
@@ -42,7 +43,9 @@ public final class SQLMatchTrafficAlgorithm implements SegmentTrafficAlgorithm {
@Override
public void init(final Properties props) {
Preconditions.checkArgument(props.containsKey(SQL_PROPS_KEY), "%s cannot be null.", SQL_PROPS_KEY);
- sql = getExactlySQL(props.getProperty(SQL_PROPS_KEY));
+ String sqlValue = props.getProperty(SQL_PROPS_KEY);
+ Preconditions.checkArgument(!(StringUtils.isEmpty(sqlValue) || sqlValue.length() == 0), "%s cannot be empty.", SQL_PROPS_KEY);
Review Comment:
Use `ShardingSpherePreconditions` to replace `Preconditions`, and then define an accurate exception class, refer to #24939
##########
kernel/traffic/core/src/main/java/org/apache/shardingsphere/traffic/algorithm/traffic/segment/SQLMatchTrafficAlgorithm.java:
##########
@@ -42,7 +43,9 @@ public final class SQLMatchTrafficAlgorithm implements SegmentTrafficAlgorithm {
@Override
public void init(final Properties props) {
Preconditions.checkArgument(props.containsKey(SQL_PROPS_KEY), "%s cannot be null.", SQL_PROPS_KEY);
- sql = getExactlySQL(props.getProperty(SQL_PROPS_KEY));
+ String sqlValue = props.getProperty(SQL_PROPS_KEY);
+ Preconditions.checkArgument(!(StringUtils.isEmpty(sqlValue) || sqlValue.length() == 0), "%s cannot be empty.", SQL_PROPS_KEY);
Review Comment:
Use `Strings.isNullOrEmpty()` of guava is recommended.
##########
kernel/traffic/core/src/test/java/org/apache/shardingsphere/traffic/algorithm/traffic/segment/SQLRegexTrafficAlgorithmTest.java:
##########
@@ -59,4 +60,10 @@ void assertMatchWhenNotExistSQLRegexMatch() {
assertFalse(sqlRegexAlgorithm.match(new SegmentTrafficValue(sqlStatement, "TRUNCATE TABLE `t_order` ")));
assertFalse(sqlRegexAlgorithm.match(new SegmentTrafficValue(sqlStatement, "UPDATE `t_order` SET `order_id` = ?;")));
}
+
+ @Test
+ void assertThrowExceptionWhenWithIllegalInit() {
+ assertThrows(IllegalArgumentException.class, () -> sqlRegexAlgorithm = (SQLRegexTrafficAlgorithm) TypedSPILoader.getService(TrafficAlgorithm.class, "SQL_REGEX",
Review Comment:
as same.
##########
kernel/traffic/core/src/main/java/org/apache/shardingsphere/traffic/algorithm/traffic/segment/SQLMatchTrafficAlgorithm.java:
##########
@@ -42,7 +43,9 @@ public final class SQLMatchTrafficAlgorithm implements SegmentTrafficAlgorithm {
@Override
public void init(final Properties props) {
Preconditions.checkArgument(props.containsKey(SQL_PROPS_KEY), "%s cannot be null.", SQL_PROPS_KEY);
- sql = getExactlySQL(props.getProperty(SQL_PROPS_KEY));
+ String sqlValue = props.getProperty(SQL_PROPS_KEY);
Review Comment:
This line is unnecessary and can be left as it is.
##########
kernel/traffic/core/src/main/java/org/apache/shardingsphere/traffic/algorithm/traffic/segment/SQLRegexTrafficAlgorithm.java:
##########
@@ -36,7 +37,9 @@ public final class SQLRegexTrafficAlgorithm implements SegmentTrafficAlgorithm {
@Override
public void init(final Properties props) {
Preconditions.checkArgument(props.containsKey(REGEX_PROPS_KEY), "%s can not be null.", REGEX_PROPS_KEY);
- regex = Pattern.compile(props.getProperty(REGEX_PROPS_KEY));
+ String regexValue = props.getProperty(REGEX_PROPS_KEY);
Review Comment:
as same.
##########
kernel/traffic/core/src/main/java/org/apache/shardingsphere/traffic/algorithm/traffic/segment/SQLRegexTrafficAlgorithm.java:
##########
@@ -36,7 +37,9 @@ public final class SQLRegexTrafficAlgorithm implements SegmentTrafficAlgorithm {
@Override
public void init(final Properties props) {
Preconditions.checkArgument(props.containsKey(REGEX_PROPS_KEY), "%s can not be null.", REGEX_PROPS_KEY);
- regex = Pattern.compile(props.getProperty(REGEX_PROPS_KEY));
+ String regexValue = props.getProperty(REGEX_PROPS_KEY);
+ Preconditions.checkArgument(!(StringUtils.isEmpty(regexValue) || regexValue.length() == 0), "%s cannot be empty.", REGEX_PROPS_KEY);
Review Comment:
as same.
##########
kernel/traffic/core/src/test/java/org/apache/shardingsphere/traffic/algorithm/traffic/segment/SQLMatchTrafficAlgorithmTest.java:
##########
@@ -59,4 +60,10 @@ void assertMatchWhenNotExistSQLMatch() {
assertFalse(sqlMatchAlgorithm.match(new SegmentTrafficValue(sqlStatement, "UPDATE `t_order` SET `order_id` = ?;")));
assertFalse(sqlMatchAlgorithm.match(new SegmentTrafficValue(sqlStatement, "UPDATE `t_order_item` SET `order_id` = ? WHERE user_id = ?;")));
}
+
+ @Test
+ void assertThrowExceptionWhenWithIllegalInit() {
+ assertThrows(IllegalArgumentException.class, () -> sqlMatchAlgorithm = (SQLMatchTrafficAlgorithm) TypedSPILoader.getService(TrafficAlgorithm.class, "SQL_MATCH",
Review Comment:
There is no need to assign to `sqlMatchAlgorithm`.
--
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