Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #2629 #2666

Closed
wants to merge 23 commits into from
Closed

Fixes #2629 #2666

wants to merge 23 commits into from

Conversation

sunbufu
Copy link
Contributor

@sunbufu sunbufu commented Jul 6, 2019

Changes proposed in this pull request:

  • throw ShardingException when execute RoutingEngine.newInstance() with null ShardingRule
  • avoid NPE when execute ShardingDataSourceNames.getDefaultDataSourceName() with null shardingRuleConfig

Hello, this is my first PR for shardingsphere. so If I do something wrong, please let me know. Thank U.

…ith null ShardingRule

2. avoid NPE when execute ShardingDataSourceNames.getDefaultDataSourceName() with null shardingRuleConfig
@coveralls
Copy link

coveralls commented Jul 6, 2019

Pull Request Test Coverage Report for Build 8742

  • 84 of 243 (34.57%) changed or added relevant lines in 29 files are covered.
  • 2 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.2%) to 61.523%

Changes Missing Coverage Covered Lines Changed/Added Lines %
sharding-core/sharding-core-parse/sharding-core-parse-common/src/main/java/org/apache/shardingsphere/core/parse/entry/EncryptSQLParseEntry.java 0 1 0.0%
sharding-core/sharding-core-parse/sharding-core-parse-common/src/main/java/org/apache/shardingsphere/core/parse/entry/MasterSlaveSQLParseEntry.java 0 1 0.0%
sharding-core/sharding-core-parse/sharding-core-parse-common/src/main/java/org/apache/shardingsphere/core/parse/entry/ShardingSQLParseEntry.java 0 1 0.0%
sharding-core/sharding-core-parse/sharding-core-parse-mysql/src/main/java/org/apache/shardingsphere/core/parse/filler/dal/MySQLShowLikeFiller.java 0 1 0.0%
sharding-core/sharding-core-rewrite/src/main/java/org/apache/shardingsphere/core/rewrite/builder/BaseParameterBuilder.java 0 1 0.0%
sharding-jdbc/sharding-jdbc-core/src/main/java/org/apache/shardingsphere/shardingjdbc/jdbc/core/datasource/EncryptDataSource.java 0 1 0.0%
sharding-proxy/sharding-proxy-frontend/sharding-proxy-frontend-mysql/src/main/java/org/apache/shardingsphere/shardingproxy/frontend/mysql/command/query/binary/prepare/MySQLComStmtPrepareExecutor.java 0 1 0.0%
sharding-proxy/sharding-proxy-frontend/sharding-proxy-frontend-postgresql/src/main/java/org/apache/shardingsphere/shardingproxy/frontend/postgresql/command/query/binary/parse/PostgreSQLComParseExecutor.java 0 1 0.0%
sharding-core/sharding-core-merge/src/main/java/org/apache/shardingsphere/core/merge/dql/DQLMergeEngine.java 9 11 81.82%
sharding-proxy/sharding-proxy-backend/src/main/java/org/apache/shardingsphere/shardingproxy/backend/schema/EncryptSchema.java 0 2 0.0%
Files with Coverage Reduction New Missed Lines %
sharding-core/sharding-core-optimize/src/main/java/org/apache/shardingsphere/core/optimize/statement/sharding/dml/select/Pagination.java 1 0.0%
sharding-core/sharding-core-parse/sharding-core-parse-common/src/main/java/org/apache/shardingsphere/core/parse/SQLParseEngine.java 1 0.0%
Totals Coverage Status
Change from base Build 8713: 0.2%
Covered Lines: 8444
Relevant Lines: 13725

💛 - Coveralls

@terrymanu terrymanu requested a review from tristaZero July 6, 2019 05:42
@sunbufu
Copy link
Contributor Author

sunbufu commented Jul 6, 2019

#2629

@tristaZero
Copy link
Contributor

Hi, thanks for your pr.
In your pr, you said that if there is no shardingRule, RoutingEngine.newInstance() and RoutingEngine.newInstance() will throw NPE. But when will the shardingRule be null? Because if you want to sharding tables, shardingRule is necessary, otherwise you can not start up.
Can u give me some scenerios?

@sunbufu
Copy link
Contributor Author

sunbufu commented Jul 6, 2019

Hi, thanks for your pr.
In your pr, you said that if there is no shardingRule, RoutingEngine.newInstance() and RoutingEngine.newInstance() will throw NPE. But when will the shardingRule be null? Because if you want to sharding tables, shardingRule is necessary, otherwise you can not start up.
Can u give me some scenerios?

Thank you for reply. I agree with you, and In my view, I think RoutingEngine.newInstance() should throw ShardingException replace NPE, when ShardingRule is empty (include null). You can find this code on RoutingEngineFactory in line 68 to 70.
And, I think ShardingDataSourceNames is same with RoutingEngine. This code in lines 45 to 47 on ShardingDataSourceNames of my PR.

@tristaZero
Copy link
Contributor

tristaZero commented Jul 8, 2019

Hi, Considering user will create ShardingRule to create ShardingDataSource, i think it is better to judge whether null == shardingRuleConfig, but do you think it is better to add this judgement in ShardingRule.java?

public ShardingRule(final ShardingRuleConfiguration shardingRuleConfig, final Collection<String> dataSourceNames) {
        // Here just checking dataSourceNames, maybe we can add checking shardingRuleConfig?
        Preconditions.checkArgument(!.isEmpty(), "Data sources cannot be empty.");
        this.shardingRuleConfig = shardingRuleConfig;
       // If we add shardingRuleConfig checking ahead, there is no chance to enter into it, if shardingRuleConfig is null.
        shardingDataSourceNames = new ShardingDataSourceNames(shardingRuleConfig, dataSourceNames);

Once ShardingRule, i.e, ShardingDataSource is created, it is impossible that shardingRule will be null in RoutingEngineFactory. What do you think?

@sunbufu
Copy link
Contributor Author

sunbufu commented Jul 8, 2019

You are right, ShardingRule is meaningless with a null ShardingRuleConfig. So, we need a judge in ShardingRule's constructor. and I notice there may throw a NPE in ShardingRule.createEncryptRule() when shardingRuleConfig is null.
There is a question. Should I throw a ShardingException or IllegalArgumentException on this case? If we choose ShardingException, I need modify dataSourceNames's judge too.

sunbufu and others added 21 commits July 8, 2019 18:08
2. modify SHardingDataSourceNames UT
@sunbufu
Copy link
Contributor Author

sunbufu commented Jul 8, 2019

Confuse the commit log by rebase, so I send a new and close this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants