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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
