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 2022/07/05 07:07:17 UTC
[GitHub] [shardingsphere] JiekerTime opened a new pull request, #18859: fix(charset): fix statement set names
JiekerTime opened a new pull request, #18859:
URL: https://github.com/apache/shardingsphere/pull/18859
Fixes #18852
--
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
[GitHub] [shardingsphere] JiekerTime commented on a diff in pull request #18859: fix(charset): fix statement set names
Posted by GitBox <gi...@apache.org>.
JiekerTime commented on code in PR #18859:
URL: https://github.com/apache/shardingsphere/pull/18859#discussion_r914516459
##########
shardingsphere-proxy/shardingsphere-proxy-backend/src/main/java/org/apache/shardingsphere/proxy/backend/text/admin/mysql/executor/MySQLSetCharsetExecutor.java:
##########
@@ -60,7 +61,13 @@ private Charset parseCharset(final String value) {
case "utf8mb4":
return StandardCharsets.UTF_8;
default:
- return Charset.forName(value);
+ try {
+ return Charset.forName(value);
+ // CHECKSTYLE:OFF
+ } catch (Exception ex) {
Review Comment:
>
Perhaps you could try executing the statement set names "'utf8'";
It may cause Charset.forName("\"'utf8'\"");
--
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
[GitHub] [shardingsphere] TeslaCN commented on a diff in pull request #18859: fix(charset): fix statement set names
Posted by GitBox <gi...@apache.org>.
TeslaCN commented on code in PR #18859:
URL: https://github.com/apache/shardingsphere/pull/18859#discussion_r914506924
##########
shardingsphere-proxy/shardingsphere-proxy-backend/src/main/java/org/apache/shardingsphere/proxy/backend/text/admin/mysql/executor/MySQLSetCharsetExecutor.java:
##########
@@ -60,7 +61,13 @@ private Charset parseCharset(final String value) {
case "utf8mb4":
return StandardCharsets.UTF_8;
default:
- return Charset.forName(value);
+ try {
+ return Charset.forName(value);
+ // CHECKSTYLE:OFF
+ } catch (Exception ex) {
Review Comment:
Why does it need the check?
--
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
[GitHub] [shardingsphere] TeslaCN commented on a diff in pull request #18859: fix(charset): fix statement set names
Posted by GitBox <gi...@apache.org>.
TeslaCN commented on code in PR #18859:
URL: https://github.com/apache/shardingsphere/pull/18859#discussion_r914572805
##########
shardingsphere-proxy/shardingsphere-proxy-backend/src/main/java/org/apache/shardingsphere/proxy/backend/text/admin/mysql/executor/MySQLSetCharsetExecutor.java:
##########
@@ -60,7 +61,13 @@ private Charset parseCharset(final String value) {
case "utf8mb4":
return StandardCharsets.UTF_8;
default:
- return Charset.forName(value);
+ try {
+ return Charset.forName(value);
+ // CHECKSTYLE:OFF
+ } catch (Exception ex) {
Review Comment:
But you have handled the quote.
--
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
[GitHub] [shardingsphere] TeslaCN commented on a diff in pull request #18859: fix(charset): fix statement set names
Posted by GitBox <gi...@apache.org>.
TeslaCN commented on code in PR #18859:
URL: https://github.com/apache/shardingsphere/pull/18859#discussion_r914589320
##########
shardingsphere-proxy/shardingsphere-proxy-backend/src/main/java/org/apache/shardingsphere/proxy/backend/text/admin/mysql/executor/MySQLSetCharsetExecutor.java:
##########
@@ -60,7 +61,13 @@ private Charset parseCharset(final String value) {
case "utf8mb4":
return StandardCharsets.UTF_8;
default:
- return Charset.forName(value);
+ try {
+ return Charset.forName(value);
+ // CHECKSTYLE:OFF
+ } catch (Exception ex) {
Review Comment:
> Perhaps you could try executing the statement set name '"utf8"';
> It may cause Charset.forName(""utf8"");
I don't think MySQL will accept `set name '"utf8"'`, we just need to handle the `set name "utf8"` in this PR.
--
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
[GitHub] [shardingsphere] JiekerTime commented on a diff in pull request #18859: fix(charset): fix statement set names
Posted by GitBox <gi...@apache.org>.
JiekerTime commented on code in PR #18859:
URL: https://github.com/apache/shardingsphere/pull/18859#discussion_r914516459
##########
shardingsphere-proxy/shardingsphere-proxy-backend/src/main/java/org/apache/shardingsphere/proxy/backend/text/admin/mysql/executor/MySQLSetCharsetExecutor.java:
##########
@@ -60,7 +61,13 @@ private Charset parseCharset(final String value) {
case "utf8mb4":
return StandardCharsets.UTF_8;
default:
- return Charset.forName(value);
+ try {
+ return Charset.forName(value);
+ // CHECKSTYLE:OFF
+ } catch (Exception ex) {
Review Comment:
>
Perhaps you could try executing the statement set name '"utf8"';
It may cause Charset.forName(""utf8"");
--
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
[GitHub] [shardingsphere] JiekerTime commented on a diff in pull request #18859: fix(charset): fix statement set names
Posted by GitBox <gi...@apache.org>.
JiekerTime commented on code in PR #18859:
URL: https://github.com/apache/shardingsphere/pull/18859#discussion_r914516459
##########
shardingsphere-proxy/shardingsphere-proxy-backend/src/main/java/org/apache/shardingsphere/proxy/backend/text/admin/mysql/executor/MySQLSetCharsetExecutor.java:
##########
@@ -60,7 +61,13 @@ private Charset parseCharset(final String value) {
case "utf8mb4":
return StandardCharsets.UTF_8;
default:
- return Charset.forName(value);
+ try {
+ return Charset.forName(value);
+ // CHECKSTYLE:OFF
+ } catch (Exception ex) {
Review Comment:
>
Perhaps you could try executing the statement set name "'utf8'";
It may cause Charset.forName("\'utf8\'");
--
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
[GitHub] [shardingsphere] TeslaCN merged pull request #18859: fix(charset): fix statement set names
Posted by GitBox <gi...@apache.org>.
TeslaCN merged PR #18859:
URL: https://github.com/apache/shardingsphere/pull/18859
--
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
[GitHub] [shardingsphere] JiekerTime commented on a diff in pull request #18859: fix(charset): fix statement set names
Posted by GitBox <gi...@apache.org>.
JiekerTime commented on code in PR #18859:
URL: https://github.com/apache/shardingsphere/pull/18859#discussion_r914516459
##########
shardingsphere-proxy/shardingsphere-proxy-backend/src/main/java/org/apache/shardingsphere/proxy/backend/text/admin/mysql/executor/MySQLSetCharsetExecutor.java:
##########
@@ -60,7 +61,13 @@ private Charset parseCharset(final String value) {
case "utf8mb4":
return StandardCharsets.UTF_8;
default:
- return Charset.forName(value);
+ try {
+ return Charset.forName(value);
+ // CHECKSTYLE:OFF
+ } catch (Exception ex) {
Review Comment:
>
Perhaps you could try executing the statement set name "'utf8'";
It may cause Charset.forName("\"'utf8'\"");
--
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
[GitHub] [shardingsphere] JiekerTime commented on a diff in pull request #18859: fix(charset): fix statement set names
Posted by GitBox <gi...@apache.org>.
JiekerTime commented on code in PR #18859:
URL: https://github.com/apache/shardingsphere/pull/18859#discussion_r914628726
##########
shardingsphere-proxy/shardingsphere-proxy-backend/src/main/java/org/apache/shardingsphere/proxy/backend/text/admin/mysql/executor/MySQLSetCharsetExecutor.java:
##########
@@ -60,7 +61,13 @@ private Charset parseCharset(final String value) {
case "utf8mb4":
return StandardCharsets.UTF_8;
default:
- return Charset.forName(value);
+ try {
+ return Charset.forName(value);
+ // CHECKSTYLE:OFF
+ } catch (Exception ex) {
Review Comment:
> > Perhaps you could try executing the statement set name '"utf8"';
> > It may cause Charset.forName(""utf8"");
>
> I don't think MySQL will accept `set name '"utf8"'`, we just need to handle the `set name "utf8"` in this PR.
I think if we don't handle this exception it could lead to a decrease in user-friendliness like the following.
I can remove this try-catch if this is not a good change.
--
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