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 2019/12/22 10:30:34 UTC
[GitHub] [incubator-shardingsphere] HongJinFeng opened a new pull request
#3781: Fix encryption rules do not support uppercase fileds
HongJinFeng opened a new pull request #3781: Fix encryption rules do not support uppercase fileds
URL: https://github.com/apache/incubator-shardingsphere/pull/3781
Fixes #3309
Method rewrite of class EncryptInsertValueParameterRewriter get encryptor by logic column, which is got from SQLStatementContext and formated to lower case by orm frame(like mybatis) . So if encryption rule use upper case logic column, the rewrite method can not get encryptor by it. And then Encrypt-JDBC fail to rewrite the SQL.
Changes proposed in this pull request:
The method findPlainColumn and findShardingEncryptor use the lower case logic column as parameter, and if the encryption rule use upper case logic column, it cause the problem.
-
-
-
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-shardingsphere] terrymanu merged pull request #3781: Fix
encryption rules do not support uppercase fileds
Posted by GitBox <gi...@apache.org>.
terrymanu merged pull request #3781: Fix encryption rules do not support uppercase fileds
URL: https://github.com/apache/incubator-shardingsphere/pull/3781
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-shardingsphere] HongJinFeng commented on a change in
pull request #3781: Fix encryption rules do not support uppercase fileds
Posted by GitBox <gi...@apache.org>.
HongJinFeng commented on a change in pull request #3781: Fix encryption rules do not support uppercase fileds
URL: https://github.com/apache/incubator-shardingsphere/pull/3781#discussion_r360756738
##########
File path: sharding-core/sharding-core-common/src/main/java/org/apache/shardingsphere/core/strategy/encrypt/EncryptTable.java
##########
@@ -153,7 +153,19 @@ public String getCipherColumn(final String logicColumn) {
* @return optional of sharding encryptor
*/
public Optional<String> findShardingEncryptor(final String logicColumn) {
- return columns.containsKey(logicColumn) ? Optional.of(columns.get(logicColumn).getEncryptor()) : Optional.<String>absent();
+ final String originLogicColumnName = getOriginLogicColumnName(logicColumn);
Review comment:
Sorry, my problem. Let me modify.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-shardingsphere] terrymanu commented on a change in pull
request #3781: Fix encryption rules do not support uppercase fileds
Posted by GitBox <gi...@apache.org>.
terrymanu commented on a change in pull request #3781: Fix encryption rules do not support uppercase fileds
URL: https://github.com/apache/incubator-shardingsphere/pull/3781#discussion_r360797841
##########
File path: sharding-core/sharding-core-common/src/main/java/org/apache/shardingsphere/core/rule/EncryptRule.java
##########
@@ -132,7 +132,17 @@ public String getLogicColumnOfCipher(final String logicTable, final String ciphe
* @return plain column
*/
public Optional<String> findPlainColumn(final String logicTable, final String logicColumn) {
- return tables.containsKey(logicTable) ? tables.get(logicTable).findPlainColumn(logicColumn) : Optional.<String>absent();
+ Optional<String> originColumnName = getOriginColumnName(logicTable, logicColumn);
+ return tables.containsKey(logicTable) && originColumnName.isPresent() ? tables.get(logicTable).findPlainColumn(originColumnName.get()) : Optional.<String>absent();
Review comment:
Can you move `originColumnName.isPresent()` before `tables.containsKey(logicTable)`?
Reason:
1. Let's keep consistent with `EncryptTable.java`
2. `originColumnName.isPresent()` should judge first , if absent, the statement should circuit break, do not need execute `tables.containsKey(logicTable)`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-shardingsphere] terrymanu commented on a change in pull
request #3781: Fix encryption rules do not support uppercase fileds
Posted by GitBox <gi...@apache.org>.
terrymanu commented on a change in pull request #3781: Fix encryption rules do not support uppercase fileds
URL: https://github.com/apache/incubator-shardingsphere/pull/3781#discussion_r360797030
##########
File path: sharding-core/sharding-core-common/src/main/java/org/apache/shardingsphere/core/rule/EncryptRule.java
##########
@@ -132,7 +132,17 @@ public String getLogicColumnOfCipher(final String logicTable, final String ciphe
* @return plain column
*/
public Optional<String> findPlainColumn(final String logicTable, final String logicColumn) {
- return tables.containsKey(logicTable) ? tables.get(logicTable).findPlainColumn(logicColumn) : Optional.<String>absent();
+ Optional<String> originColumnName = getOriginColumnName(logicTable, logicColumn);
+ return tables.containsKey(logicTable) && originColumnName.isPresent() ? tables.get(logicTable).findPlainColumn(originColumnName.get()) : Optional.<String>absent();
+ }
+
+ private Optional<String> getOriginColumnName(final String logicTable, final String logicColumn) {
Review comment:
If return value is Optional, we prefer use `findXXX` instead of `getXXX`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-shardingsphere] SteNicholas commented on a change in
pull request #3781: Fix encryption rules do not support uppercase fileds
Posted by GitBox <gi...@apache.org>.
SteNicholas commented on a change in pull request #3781: Fix encryption rules do not support uppercase fileds
URL: https://github.com/apache/incubator-shardingsphere/pull/3781#discussion_r360771446
##########
File path: sharding-core/sharding-core-common/src/main/java/org/apache/shardingsphere/core/strategy/encrypt/EncryptTable.java
##########
@@ -153,7 +153,17 @@ public String getCipherColumn(final String logicColumn) {
* @return optional of sharding encryptor
*/
public Optional<String> findShardingEncryptor(final String logicColumn) {
- return columns.containsKey(logicColumn) ? Optional.of(columns.get(logicColumn).getEncryptor()) : Optional.<String>absent();
+ String originLogicColumnName = getOriginLogicColumnName(logicColumn);
+ return columns.containsKey(originLogicColumnName) ? Optional.of(columns.get(getOriginLogicColumnName(logicColumn)).getEncryptor()) : Optional.<String>absent();
+ }
+
+ private String getOriginLogicColumnName(final String logicColumn) {
Review comment:
It is recommended to use Guava package to shorten this code logic.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-shardingsphere] terrymanu commented on a change in pull
request #3781: Fix encryption rules do not support uppercase fileds
Posted by GitBox <gi...@apache.org>.
terrymanu commented on a change in pull request #3781: Fix encryption rules do not support uppercase fileds
URL: https://github.com/apache/incubator-shardingsphere/pull/3781#discussion_r360755477
##########
File path: sharding-core/sharding-core-common/src/main/java/org/apache/shardingsphere/core/rule/EncryptRule.java
##########
@@ -132,7 +132,18 @@ public String getLogicColumnOfCipher(final String logicTable, final String ciphe
* @return plain column
*/
public Optional<String> findPlainColumn(final String logicTable, final String logicColumn) {
- return tables.containsKey(logicTable) ? tables.get(logicTable).findPlainColumn(logicColumn) : Optional.<String>absent();
+ return tables.containsKey(logicTable) ? tables.get(logicTable).findPlainColumn(getOriginColumnName(logicTable, logicColumn)) : Optional.<String>absent();
+ }
+
+ private String getOriginColumnName(final String logicTable, final String logicColumn) {
+ String result = logicColumn;
+ for (String each : tables.get(logicTable).getLogicColumns()) {
+ if (logicColumn.equals(each.toLowerCase())) {
+ result = each;
+ break;
+ }
+ }
+ return result;
Review comment:
What is situation if `logicColumn` cannot find? Maybe throw exception?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-shardingsphere] HongJinFeng commented on a change in
pull request #3781: Fix encryption rules do not support uppercase fileds
Posted by GitBox <gi...@apache.org>.
HongJinFeng commented on a change in pull request #3781: Fix encryption rules do not support uppercase fileds
URL: https://github.com/apache/incubator-shardingsphere/pull/3781#discussion_r360756738
##########
File path: sharding-core/sharding-core-common/src/main/java/org/apache/shardingsphere/core/strategy/encrypt/EncryptTable.java
##########
@@ -153,7 +153,19 @@ public String getCipherColumn(final String logicColumn) {
* @return optional of sharding encryptor
*/
public Optional<String> findShardingEncryptor(final String logicColumn) {
- return columns.containsKey(logicColumn) ? Optional.of(columns.get(logicColumn).getEncryptor()) : Optional.<String>absent();
+ final String originLogicColumnName = getOriginLogicColumnName(logicColumn);
Review comment:
Sorry, my problem, String is final, Let me modify.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-shardingsphere] HongJinFeng commented on a change in
pull request #3781: Fix encryption rules do not support uppercase fileds
Posted by GitBox <gi...@apache.org>.
HongJinFeng commented on a change in pull request #3781: Fix encryption rules do not support uppercase fileds
URL: https://github.com/apache/incubator-shardingsphere/pull/3781#discussion_r360756738
##########
File path: sharding-core/sharding-core-common/src/main/java/org/apache/shardingsphere/core/strategy/encrypt/EncryptTable.java
##########
@@ -153,7 +153,19 @@ public String getCipherColumn(final String logicColumn) {
* @return optional of sharding encryptor
*/
public Optional<String> findShardingEncryptor(final String logicColumn) {
- return columns.containsKey(logicColumn) ? Optional.of(columns.get(logicColumn).getEncryptor()) : Optional.<String>absent();
+ final String originLogicColumnName = getOriginLogicColumnName(logicColumn);
Review comment:
Sorry, my problem, Let me modify.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-shardingsphere] HongJinFeng commented on a change in
pull request #3781: Fix encryption rules do not support uppercase fileds
Posted by GitBox <gi...@apache.org>.
HongJinFeng commented on a change in pull request #3781: Fix encryption rules do not support uppercase fileds
URL: https://github.com/apache/incubator-shardingsphere/pull/3781#discussion_r360756172
##########
File path: sharding-core/sharding-core-common/src/main/java/org/apache/shardingsphere/core/rule/EncryptRule.java
##########
@@ -132,7 +132,18 @@ public String getLogicColumnOfCipher(final String logicTable, final String ciphe
* @return plain column
*/
public Optional<String> findPlainColumn(final String logicTable, final String logicColumn) {
- return tables.containsKey(logicTable) ? tables.get(logicTable).findPlainColumn(logicColumn) : Optional.<String>absent();
+ return tables.containsKey(logicTable) ? tables.get(logicTable).findPlainColumn(getOriginColumnName(logicTable, logicColumn)) : Optional.<String>absent();
+ }
+
+ private String getOriginColumnName(final String logicTable, final String logicColumn) {
+ String result = logicColumn;
+ for (String each : tables.get(logicTable).getLogicColumns()) {
+ if (logicColumn.equals(each.toLowerCase())) {
+ result = each;
Review comment:
You make sense, wait a minute.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-shardingsphere] coveralls edited a comment on issue
#3781: Fix encryption rules do not support uppercase fileds
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on issue #3781: Fix encryption rules do not support uppercase fileds
URL: https://github.com/apache/incubator-shardingsphere/pull/3781#issuecomment-568339137
## Pull Request Test Coverage Report for [Build 1206](https://coveralls.io/builds/27761705)
* **14** of **14** **(100.0%)** changed or added relevant lines in **2** files are covered.
* No unchanged relevant lines lost coverage.
* Overall coverage increased (+**0.03%**) to **67.307%**
---
| Totals | [](https://coveralls.io/builds/27761705) |
| :-- | --: |
| Change from base [Build 619](https://coveralls.io/builds/27750034): | 0.03% |
| Covered Lines: | 11309 |
| Relevant Lines: | 16802 |
---
##### 💛 - [Coveralls](https://coveralls.io)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-shardingsphere] coveralls edited a comment on issue
#3781: Fix encryption rules do not support uppercase fileds
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on issue #3781: Fix encryption rules do not support uppercase fileds
URL: https://github.com/apache/incubator-shardingsphere/pull/3781#issuecomment-568339137
## Pull Request Test Coverage Report for [Build 1205](https://coveralls.io/builds/27761299)
* **14** of **14** **(100.0%)** changed or added relevant lines in **2** files are covered.
* No unchanged relevant lines lost coverage.
* Overall coverage increased (+**0.03%**) to **67.307%**
---
| Totals | [](https://coveralls.io/builds/27761299) |
| :-- | --: |
| Change from base [Build 619](https://coveralls.io/builds/27750034): | 0.03% |
| Covered Lines: | 11309 |
| Relevant Lines: | 16802 |
---
##### 💛 - [Coveralls](https://coveralls.io)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-shardingsphere] HongJinFeng commented on a change in
pull request #3781: Fix encryption rules do not support uppercase fileds
Posted by GitBox <gi...@apache.org>.
HongJinFeng commented on a change in pull request #3781: Fix encryption rules do not support uppercase fileds
URL: https://github.com/apache/incubator-shardingsphere/pull/3781#discussion_r360788899
##########
File path: sharding-core/sharding-core-common/src/main/java/org/apache/shardingsphere/core/rule/EncryptRule.java
##########
@@ -132,7 +132,18 @@ public String getLogicColumnOfCipher(final String logicTable, final String ciphe
* @return plain column
*/
public Optional<String> findPlainColumn(final String logicTable, final String logicColumn) {
- return tables.containsKey(logicTable) ? tables.get(logicTable).findPlainColumn(logicColumn) : Optional.<String>absent();
+ return tables.containsKey(logicTable) ? tables.get(logicTable).findPlainColumn(getOriginColumnName(logicTable, logicColumn)) : Optional.<String>absent();
+ }
+
+ private String getOriginColumnName(final String logicTable, final String logicColumn) {
+ String result = logicColumn;
+ for (String each : tables.get(logicTable).getLogicColumns()) {
+ if (logicColumn.equals(each.toLowerCase())) {
+ result = each;
+ break;
+ }
+ }
+ return result;
Review comment:
It make sense. Let me fix 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-shardingsphere] SteNicholas commented on a change in
pull request #3781: Fix encryption rules do not support uppercase fileds
Posted by GitBox <gi...@apache.org>.
SteNicholas commented on a change in pull request #3781: Fix encryption rules do not support uppercase fileds
URL: https://github.com/apache/incubator-shardingsphere/pull/3781#discussion_r360771401
##########
File path: sharding-core/sharding-core-common/src/main/java/org/apache/shardingsphere/core/rule/EncryptRule.java
##########
@@ -132,7 +132,16 @@ public String getLogicColumnOfCipher(final String logicTable, final String ciphe
* @return plain column
*/
public Optional<String> findPlainColumn(final String logicTable, final String logicColumn) {
- return tables.containsKey(logicTable) ? tables.get(logicTable).findPlainColumn(logicColumn) : Optional.<String>absent();
+ return tables.containsKey(logicTable) ? tables.get(logicTable).findPlainColumn(getOriginColumnName(logicTable, logicColumn)) : Optional.<String>absent();
+ }
+
+ private String getOriginColumnName(final String logicTable, final String logicColumn) {
Review comment:
It is recommended to use Guava package to shorten this code logic.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-shardingsphere] coveralls commented on issue #3781: Fix
encryption rules do not support uppercase fileds
Posted by GitBox <gi...@apache.org>.
coveralls commented on issue #3781: Fix encryption rules do not support uppercase fileds
URL: https://github.com/apache/incubator-shardingsphere/pull/3781#issuecomment-568339137
## Pull Request Test Coverage Report for [Build 1202](https://coveralls.io/builds/27758170)
* **16** of **17** **(94.12%)** changed or added relevant lines in **2** files are covered.
* No unchanged relevant lines lost coverage.
* Overall coverage increased (+**0.02%**) to **67.301%**
---
| Changes Missing Coverage | Covered Lines | Changed/Added Lines | % |
| :-----|--------------|--------|---: |
| [sharding-core/sharding-core-common/src/main/java/org/apache/shardingsphere/core/strategy/encrypt/EncryptTable.java](https://coveralls.io/builds/27758170/source?filename=sharding-core%2Fsharding-core-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fshardingsphere%2Fcore%2Fstrategy%2Fencrypt%2FEncryptTable.java#L167) | 8 | 9 | 88.89%
<!-- | **Total:** | **16** | **17** | **94.12%** | -->
| Totals | [](https://coveralls.io/builds/27758170) |
| :-- | --: |
| Change from base [Build 619](https://coveralls.io/builds/27750034): | 0.02% |
| Covered Lines: | 11310 |
| Relevant Lines: | 16805 |
---
##### 💛 - [Coveralls](https://coveralls.io)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-shardingsphere] HongJinFeng commented on a change in
pull request #3781: Fix encryption rules do not support uppercase fileds
Posted by GitBox <gi...@apache.org>.
HongJinFeng commented on a change in pull request #3781: Fix encryption rules do not support uppercase fileds
URL: https://github.com/apache/incubator-shardingsphere/pull/3781#discussion_r360777323
##########
File path: sharding-core/sharding-core-common/src/main/java/org/apache/shardingsphere/core/strategy/encrypt/EncryptTable.java
##########
@@ -153,7 +153,17 @@ public String getCipherColumn(final String logicColumn) {
* @return optional of sharding encryptor
*/
public Optional<String> findShardingEncryptor(final String logicColumn) {
- return columns.containsKey(logicColumn) ? Optional.of(columns.get(logicColumn).getEncryptor()) : Optional.<String>absent();
+ String originLogicColumnName = getOriginLogicColumnName(logicColumn);
+ return columns.containsKey(originLogicColumnName) ? Optional.of(columns.get(getOriginLogicColumnName(logicColumn)).getEncryptor()) : Optional.<String>absent();
+ }
+
+ private String getOriginLogicColumnName(final String logicColumn) {
Review comment:
Thanks for review, and furthermore, which api do you mean?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-shardingsphere] SteNicholas commented on issue #3781:
Fix encryption rules do not support uppercase fileds
Posted by GitBox <gi...@apache.org>.
SteNicholas commented on issue #3781: Fix encryption rules do not support uppercase fileds
URL: https://github.com/apache/incubator-shardingsphere/pull/3781#issuecomment-568313332
Any unit test? And check the CI result.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-shardingsphere] terrymanu commented on a change in pull
request #3781: Fix encryption rules do not support uppercase fileds
Posted by GitBox <gi...@apache.org>.
terrymanu commented on a change in pull request #3781: Fix encryption rules do not support uppercase fileds
URL: https://github.com/apache/incubator-shardingsphere/pull/3781#discussion_r360755367
##########
File path: sharding-core/sharding-core-common/src/main/java/org/apache/shardingsphere/core/rule/EncryptRule.java
##########
@@ -132,7 +132,18 @@ public String getLogicColumnOfCipher(final String logicTable, final String ciphe
* @return plain column
*/
public Optional<String> findPlainColumn(final String logicTable, final String logicColumn) {
- return tables.containsKey(logicTable) ? tables.get(logicTable).findPlainColumn(logicColumn) : Optional.<String>absent();
+ return tables.containsKey(logicTable) ? tables.get(logicTable).findPlainColumn(getOriginColumnName(logicTable, logicColumn)) : Optional.<String>absent();
+ }
+
+ private String getOriginColumnName(final String logicTable, final String logicColumn) {
+ String result = logicColumn;
+ for (String each : tables.get(logicTable).getLogicColumns()) {
+ if (logicColumn.equals(each.toLowerCase())) {
+ result = each;
Review comment:
1. Just return each if equals
2. use `equalsIgnoreCase` is better than `equals` and `toLowerCase`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-shardingsphere] HongJinFeng commented on a change in
pull request #3781: Fix encryption rules do not support uppercase fileds
Posted by GitBox <gi...@apache.org>.
HongJinFeng commented on a change in pull request #3781: Fix encryption rules do not support uppercase fileds
URL: https://github.com/apache/incubator-shardingsphere/pull/3781#discussion_r360777298
##########
File path: sharding-core/sharding-core-common/src/main/java/org/apache/shardingsphere/core/rule/EncryptRule.java
##########
@@ -132,7 +132,18 @@ public String getLogicColumnOfCipher(final String logicTable, final String ciphe
* @return plain column
*/
public Optional<String> findPlainColumn(final String logicTable, final String logicColumn) {
- return tables.containsKey(logicTable) ? tables.get(logicTable).findPlainColumn(logicColumn) : Optional.<String>absent();
+ return tables.containsKey(logicTable) ? tables.get(logicTable).findPlainColumn(getOriginColumnName(logicTable, logicColumn)) : Optional.<String>absent();
+ }
+
+ private String getOriginColumnName(final String logicTable, final String logicColumn) {
+ String result = logicColumn;
+ for (String each : tables.get(logicTable).getLogicColumns()) {
+ if (logicColumn.equals(each.toLowerCase())) {
+ result = each;
+ break;
+ }
+ }
+ return result;
Review comment:
Of course it is possible. If the logicColumn can not be found as well as the encryptor, it is normal which means we do not need to encrypt the column. See the source code methods by this step:
1.1.1 org.apache.shardingsphere.sql.rewriter.encrypt.parameter.impl.EncryptInsertValueParameterRewriter#rewrite
1.1.2
org.apache.shardingsphere.sql.rewriter.encrypt.parameter.impl.EncryptAssignmentParameterRewriter#rewrite
1.2
org.apache.shardingsphere.core.rule.EncryptRule#findShardingEncryptor(String,String)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-shardingsphere] terrymanu commented on a change in pull
request #3781: Fix encryption rules do not support uppercase fileds
Posted by GitBox <gi...@apache.org>.
terrymanu commented on a change in pull request #3781: Fix encryption rules do not support uppercase fileds
URL: https://github.com/apache/incubator-shardingsphere/pull/3781#discussion_r360787473
##########
File path: sharding-core/sharding-core-common/src/main/java/org/apache/shardingsphere/core/rule/EncryptRule.java
##########
@@ -132,7 +132,18 @@ public String getLogicColumnOfCipher(final String logicTable, final String ciphe
* @return plain column
*/
public Optional<String> findPlainColumn(final String logicTable, final String logicColumn) {
- return tables.containsKey(logicTable) ? tables.get(logicTable).findPlainColumn(logicColumn) : Optional.<String>absent();
+ return tables.containsKey(logicTable) ? tables.get(logicTable).findPlainColumn(getOriginColumnName(logicTable, logicColumn)) : Optional.<String>absent();
+ }
+
+ private String getOriginColumnName(final String logicTable, final String logicColumn) {
+ String result = logicColumn;
+ for (String each : tables.get(logicTable).getLogicColumns()) {
+ if (logicColumn.equals(each.toLowerCase())) {
+ result = each;
+ break;
+ }
+ }
+ return result;
Review comment:
So, how about return `Optional`? If logicColumn cannot find, just return `Optional.absent()`.
And do not need call `findPlainColumn()` if absent returned.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-shardingsphere] terrymanu commented on a change in pull
request #3781: Fix encryption rules do not support uppercase fileds
Posted by GitBox <gi...@apache.org>.
terrymanu commented on a change in pull request #3781: Fix encryption rules do not support uppercase fileds
URL: https://github.com/apache/incubator-shardingsphere/pull/3781#discussion_r360755596
##########
File path: sharding-core/sharding-core-common/src/main/java/org/apache/shardingsphere/core/strategy/encrypt/EncryptTable.java
##########
@@ -153,7 +153,19 @@ public String getCipherColumn(final String logicColumn) {
* @return optional of sharding encryptor
*/
public Optional<String> findShardingEncryptor(final String logicColumn) {
- return columns.containsKey(logicColumn) ? Optional.of(columns.get(logicColumn).getEncryptor()) : Optional.<String>absent();
+ final String originLogicColumnName = getOriginLogicColumnName(logicColumn);
Review comment:
Why need `final` for `originLogicColumnName` here?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-shardingsphere] terrymanu commented on a change in pull
request #3781: Fix encryption rules do not support uppercase fileds
Posted by GitBox <gi...@apache.org>.
terrymanu commented on a change in pull request #3781: Fix encryption rules do not support uppercase fileds
URL: https://github.com/apache/incubator-shardingsphere/pull/3781#discussion_r360767492
##########
File path: sharding-core/sharding-core-common/src/main/java/org/apache/shardingsphere/core/rule/EncryptRule.java
##########
@@ -132,7 +132,18 @@ public String getLogicColumnOfCipher(final String logicTable, final String ciphe
* @return plain column
*/
public Optional<String> findPlainColumn(final String logicTable, final String logicColumn) {
- return tables.containsKey(logicTable) ? tables.get(logicTable).findPlainColumn(logicColumn) : Optional.<String>absent();
+ return tables.containsKey(logicTable) ? tables.get(logicTable).findPlainColumn(getOriginColumnName(logicTable, logicColumn)) : Optional.<String>absent();
+ }
+
+ private String getOriginColumnName(final String logicTable, final String logicColumn) {
+ String result = logicColumn;
+ for (String each : tables.get(logicTable).getLogicColumns()) {
+ if (logicColumn.equals(each.toLowerCase())) {
+ result = each;
+ break;
+ }
+ }
+ return result;
Review comment:
Hi, thank you for reply quickly.
I know return original parameter will not break the code. I just want ti know, what is situation if logicColumn cannot find? Is it impossible? If yes, shall we use `absent` or throw exception to make the code more clear?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-shardingsphere] terrymanu commented on a change in pull
request #3781: Fix encryption rules do not support uppercase fileds
Posted by GitBox <gi...@apache.org>.
terrymanu commented on a change in pull request #3781: Fix encryption rules do not support uppercase fileds
URL: https://github.com/apache/incubator-shardingsphere/pull/3781#discussion_r360797841
##########
File path: sharding-core/sharding-core-common/src/main/java/org/apache/shardingsphere/core/rule/EncryptRule.java
##########
@@ -132,7 +132,17 @@ public String getLogicColumnOfCipher(final String logicTable, final String ciphe
* @return plain column
*/
public Optional<String> findPlainColumn(final String logicTable, final String logicColumn) {
- return tables.containsKey(logicTable) ? tables.get(logicTable).findPlainColumn(logicColumn) : Optional.<String>absent();
+ Optional<String> originColumnName = getOriginColumnName(logicTable, logicColumn);
+ return tables.containsKey(logicTable) && originColumnName.isPresent() ? tables.get(logicTable).findPlainColumn(originColumnName.get()) : Optional.<String>absent();
Review comment:
Can you move `originColumnName.isPresent()` before `tables.containsKey(logicTable)`?
Reason:
1. Let's keep consistent with `EncryptTable.java`
2. `originColumnName.isPresent()` should judge first , if absent, the statement should circuit break, do not execute `tables.containsKey(logicTable)`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-shardingsphere] coveralls edited a comment on issue
#3781: Fix encryption rules do not support uppercase fileds
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on issue #3781: Fix encryption rules do not support uppercase fileds
URL: https://github.com/apache/incubator-shardingsphere/pull/3781#issuecomment-568339137
## Pull Request Test Coverage Report for [Build 1203](https://coveralls.io/builds/27758559)
* **10** of **13** **(76.92%)** changed or added relevant lines in **2** files are covered.
* No unchanged relevant lines lost coverage.
* Overall coverage increased (+**0.004%**) to **67.282%**
---
| Changes Missing Coverage | Covered Lines | Changed/Added Lines | % |
| :-----|--------------|--------|---: |
| [sharding-core/sharding-core-common/src/main/java/org/apache/shardingsphere/core/rule/EncryptRule.java](https://coveralls.io/builds/27758559/source?filename=sharding-core%2Fsharding-core-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fshardingsphere%2Fcore%2Frule%2FEncryptRule.java#L144) | 5 | 6 | 83.33%
| [sharding-core/sharding-core-common/src/main/java/org/apache/shardingsphere/core/strategy/encrypt/EncryptTable.java](https://coveralls.io/builds/27758559/source?filename=sharding-core%2Fsharding-core-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fshardingsphere%2Fcore%2Fstrategy%2Fencrypt%2FEncryptTable.java#L165) | 5 | 7 | 71.43%
<!-- | **Total:** | **10** | **13** | **76.92%** | -->
| Totals | [](https://coveralls.io/builds/27758559) |
| :-- | --: |
| Change from base [Build 619](https://coveralls.io/builds/27750034): | 0.004% |
| Covered Lines: | 11304 |
| Relevant Lines: | 16801 |
---
##### 💛 - [Coveralls](https://coveralls.io)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-shardingsphere] HongJinFeng commented on a change in
pull request #3781: Fix encryption rules do not support uppercase fileds
Posted by GitBox <gi...@apache.org>.
HongJinFeng commented on a change in pull request #3781: Fix encryption rules do not support uppercase fileds
URL: https://github.com/apache/incubator-shardingsphere/pull/3781#discussion_r360756738
##########
File path: sharding-core/sharding-core-common/src/main/java/org/apache/shardingsphere/core/strategy/encrypt/EncryptTable.java
##########
@@ -153,7 +153,19 @@ public String getCipherColumn(final String logicColumn) {
* @return optional of sharding encryptor
*/
public Optional<String> findShardingEncryptor(final String logicColumn) {
- return columns.containsKey(logicColumn) ? Optional.of(columns.get(logicColumn).getEncryptor()) : Optional.<String>absent();
+ final String originLogicColumnName = getOriginLogicColumnName(logicColumn);
Review comment:
Sorry, my problem.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-shardingsphere] terrymanu commented on a change in pull
request #3781: Fix encryption rules do not support uppercase fileds
Posted by GitBox <gi...@apache.org>.
terrymanu commented on a change in pull request #3781: Fix encryption rules do not support uppercase fileds
URL: https://github.com/apache/incubator-shardingsphere/pull/3781#discussion_r360797030
##########
File path: sharding-core/sharding-core-common/src/main/java/org/apache/shardingsphere/core/rule/EncryptRule.java
##########
@@ -132,7 +132,17 @@ public String getLogicColumnOfCipher(final String logicTable, final String ciphe
* @return plain column
*/
public Optional<String> findPlainColumn(final String logicTable, final String logicColumn) {
- return tables.containsKey(logicTable) ? tables.get(logicTable).findPlainColumn(logicColumn) : Optional.<String>absent();
+ Optional<String> originColumnName = getOriginColumnName(logicTable, logicColumn);
+ return tables.containsKey(logicTable) && originColumnName.isPresent() ? tables.get(logicTable).findPlainColumn(originColumnName.get()) : Optional.<String>absent();
+ }
+
+ private Optional<String> getOriginColumnName(final String logicTable, final String logicColumn) {
Review comment:
If return value is Optional, we prefer using `findXXX` instead of `getXXX`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-shardingsphere] HongJinFeng commented on a change in
pull request #3781: Fix encryption rules do not support uppercase fileds
Posted by GitBox <gi...@apache.org>.
HongJinFeng commented on a change in pull request #3781: Fix encryption rules do not support uppercase fileds
URL: https://github.com/apache/incubator-shardingsphere/pull/3781#discussion_r360801169
##########
File path: sharding-core/sharding-core-common/src/main/java/org/apache/shardingsphere/core/rule/EncryptRule.java
##########
@@ -132,7 +132,17 @@ public String getLogicColumnOfCipher(final String logicTable, final String ciphe
* @return plain column
*/
public Optional<String> findPlainColumn(final String logicTable, final String logicColumn) {
- return tables.containsKey(logicTable) ? tables.get(logicTable).findPlainColumn(logicColumn) : Optional.<String>absent();
+ Optional<String> originColumnName = getOriginColumnName(logicTable, logicColumn);
+ return tables.containsKey(logicTable) && originColumnName.isPresent() ? tables.get(logicTable).findPlainColumn(originColumnName.get()) : Optional.<String>absent();
Review comment:
Accept, had committed~
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-shardingsphere] HongJinFeng commented on a change in
pull request #3781: Fix encryption rules do not support uppercase fileds
Posted by GitBox <gi...@apache.org>.
HongJinFeng commented on a change in pull request #3781: Fix encryption rules do not support uppercase fileds
URL: https://github.com/apache/incubator-shardingsphere/pull/3781#discussion_r360756160
##########
File path: sharding-core/sharding-core-common/src/main/java/org/apache/shardingsphere/core/rule/EncryptRule.java
##########
@@ -132,7 +132,18 @@ public String getLogicColumnOfCipher(final String logicTable, final String ciphe
* @return plain column
*/
public Optional<String> findPlainColumn(final String logicTable, final String logicColumn) {
- return tables.containsKey(logicTable) ? tables.get(logicTable).findPlainColumn(logicColumn) : Optional.<String>absent();
+ return tables.containsKey(logicTable) ? tables.get(logicTable).findPlainColumn(getOriginColumnName(logicTable, logicColumn)) : Optional.<String>absent();
+ }
+
+ private String getOriginColumnName(final String logicTable, final String logicColumn) {
+ String result = logicColumn;
+ for (String each : tables.get(logicTable).getLogicColumns()) {
+ if (logicColumn.equals(each.toLowerCase())) {
+ result = each;
+ break;
+ }
+ }
+ return result;
Review comment:
If the logicColum cannot be found, just return the original parameter as logic parameter, it 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-shardingsphere] SteNicholas edited a comment on issue
#3781: Fix encryption rules do not support uppercase fileds
Posted by GitBox <gi...@apache.org>.
SteNicholas edited a comment on issue #3781: Fix encryption rules do not support uppercase fileds
URL: https://github.com/apache/incubator-shardingsphere/pull/3781#issuecomment-568313332
FIrstly, you have already used `Stream API` of JDK8, but `java.version` of project is `1.7`. And you should add unit test to prove this modification. Lastly, you should check CI result which is falied.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services