You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@shardingsphere.apache.org by "susongyan (via GitHub)" <gi...@apache.org> on 2023/08/04 05:44:49 UTC
[GitHub] [shardingsphere] susongyan opened a new pull request, #27909: escape pg ? operator in proxy
susongyan opened a new pull request, #27909:
URL: https://github.com/apache/shardingsphere/pull/27909
Fixes #27892.
Changes proposed in this pull request:
-
---
Before committing this PR, I'm sure that I have checked the following options:
- [ ] My code follows the [code of conduct](https://shardingsphere.apache.org/community/en/involved/conduct/code/) of this project.
- [ ] I have self-reviewed the commit code.
- [ ] I have (or in comment I request) added corresponding labels for the pull request.
- [ ] I have passed maven check locally : `./mvnw clean install -B -T1C -Dmaven.javadoc.skip -Dmaven.jacoco.skip -e`.
- [ ] I have made corresponding changes to the documentation.
- [ ] I have added corresponding unit tests for my changes.
--
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 #27909: escape pg ? operator in proxy
Posted by "TeslaCN (via GitHub)" <gi...@apache.org>.
TeslaCN commented on code in PR #27909:
URL: https://github.com/apache/shardingsphere/pull/27909#discussion_r1294443911
##########
proxy/frontend/type/postgresql/src/main/java/org/apache/shardingsphere/proxy/frontend/postgresql/command/query/extended/parse/PostgreSQLComParseExecutor.java:
##########
@@ -61,6 +62,11 @@ public Collection<DatabasePacket> execute() {
SQLParserEngine sqlParserEngine = createShardingSphereSQLParserEngine(connectionSession.getDatabaseName());
String sql = packet.getSQL();
SQLStatement sqlStatement = sqlParserEngine.parse(sql, true);
+ String escapedSql = escape(sqlStatement, sql);
+ if (!escapedSql.equalsIgnoreCase(sql)) {
+ sqlStatement = sqlParserEngine.parse(escapedSql, true);
Review Comment:
Is `??&` a valid PostgreSQL syntax? If we escape `?` here, maybe parsing SQL contains `??&` should be failed?
--
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 #27909: escape pg ? operator in proxy
Posted by "TeslaCN (via GitHub)" <gi...@apache.org>.
TeslaCN commented on code in PR #27909:
URL: https://github.com/apache/shardingsphere/pull/27909#discussion_r1294446025
##########
proxy/frontend/type/postgresql/src/main/java/org/apache/shardingsphere/proxy/frontend/postgresql/command/query/extended/parse/PostgreSQLComParseExecutor.java:
##########
@@ -61,6 +62,11 @@ public Collection<DatabasePacket> execute() {
SQLParserEngine sqlParserEngine = createShardingSphereSQLParserEngine(connectionSession.getDatabaseName());
String sql = packet.getSQL();
SQLStatement sqlStatement = sqlParserEngine.parse(sql, true);
+ String escapedSql = escape(sqlStatement, sql);
+ if (!escapedSql.equalsIgnoreCase(sql)) {
+ sqlStatement = sqlParserEngine.parse(escapedSql, true);
Review Comment:
Ignore it. Our PostgreSQL parser has already treated `?` as valid syntax.
--
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] susongyan commented on pull request #27909: escape pg ? operator in proxy
Posted by "susongyan (via GitHub)" <gi...@apache.org>.
susongyan commented on PR #27909:
URL: https://github.com/apache/shardingsphere/pull/27909#issuecomment-1670740094
>
to question 1: i dosen't check is dml stamtent , failed e2e case is migration sql (distsql) that contains jdbc connection string
so now just deal jsonb '?' operator in dml sql
--
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] susongyan commented on a diff in pull request #27909: escape pg ? operator in proxy
Posted by "susongyan (via GitHub)" <gi...@apache.org>.
susongyan commented on code in PR #27909:
URL: https://github.com/apache/shardingsphere/pull/27909#discussion_r1288841087
##########
proxy/frontend/type/postgresql/src/main/java/org/apache/shardingsphere/proxy/frontend/postgresql/command/query/extended/parse/PostgreSQLComParseExecutor.java:
##########
@@ -87,6 +93,13 @@ private SQLParserEngine createShardingSphereSQLParserEngine(final String databas
return sqlParserRule.getSQLParserEngine(protocolType.getTrunkDatabaseType().orElse(protocolType));
}
+ private String escape(final SQLStatement sqlStatement, final String sql) {
+ if (sqlStatement instanceof DMLStatement) {
+ return sql.replace("?", "??");
+ }
+ return sql;
+ }
Review Comment:
i found, if this escape moved to convertSQLToJDBCStyle, the sqlStatement's parameter mark indexes will be missed up
--
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] susongyan commented on pull request #27909: escape pg ? operator in proxy
Posted by "susongyan (via GitHub)" <gi...@apache.org>.
susongyan commented on PR #27909:
URL: https://github.com/apache/shardingsphere/pull/27909#issuecomment-1669289167
@sandynz plz take a look
--
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] sandynz commented on pull request #27909: escape pg ? operator in proxy
Posted by "sandynz (via GitHub)" <gi...@apache.org>.
sandynz commented on PR #27909:
URL: https://github.com/apache/shardingsphere/pull/27909#issuecomment-1670731168
Hi @susongyan , some questions:
1, Why E2E failed before
2, I had a look at your supplied PostgreSQL jsonb doc. I'm afraid `sql.replace("?", "??")` might replace `?` in other cases we don't know for now
And I'll invite another member to review 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.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] sandynz commented on pull request #27909: escape pg ? operator in proxy
Posted by "sandynz (via GitHub)" <gi...@apache.org>.
sandynz commented on PR #27909:
URL: https://github.com/apache/shardingsphere/pull/27909#issuecomment-1670746884
OK. I just commented on issue about `Parser.parseJdbcSql`, still need to verify 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.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] azexcy commented on pull request #27909: escape pg ? operator in proxy
Posted by "azexcy (via GitHub)" <gi...@apache.org>.
azexcy commented on PR #27909:
URL: https://github.com/apache/shardingsphere/pull/27909#issuecomment-1667704853
https://github.com/apache/shardingsphere/actions/runs/5758958821/job/15612284497?pr=27909
--
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 #27909: escape pg ? operator in proxy
Posted by "TeslaCN (via GitHub)" <gi...@apache.org>.
TeslaCN commented on code in PR #27909:
URL: https://github.com/apache/shardingsphere/pull/27909#discussion_r1288014686
##########
proxy/frontend/type/postgresql/src/main/java/org/apache/shardingsphere/proxy/frontend/postgresql/command/query/extended/parse/PostgreSQLComParseExecutor.java:
##########
@@ -87,6 +93,13 @@ private SQLParserEngine createShardingSphereSQLParserEngine(final String databas
return sqlParserRule.getSQLParserEngine(protocolType.getTrunkDatabaseType().orElse(protocolType));
}
+ private String escape(final SQLStatement sqlStatement, final String sql) {
+ if (sqlStatement instanceof DMLStatement) {
+ return sql.replace("?", "??");
+ }
+ return sql;
+ }
Review Comment:
I found the doc about escape `?` in the following link. I think the escape logic could be merged into `convertSQLToJDBCStyle` and invoke the method with `convertSQLToJDBCStyle(Collections.emptyList(), sql)` if parameter placeholder is absent.
https://jdbc.postgresql.org/documentation/query/#using-the-statement-or-preparedstatement-interface
--
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] sandynz commented on pull request #27909: escape pg ? operator in proxy
Posted by "sandynz (via GitHub)" <gi...@apache.org>.
sandynz commented on PR #27909:
URL: https://github.com/apache/shardingsphere/pull/27909#issuecomment-1667707282
Hi @susongyan
The pipeline E2E of PostgreSQL and openGauss failed, e.g.
> error message is: The URL `jdbc:postgresql://postgresql_0.host:5432/pipeline_it_0??ssl=on&sslmode=prefer&stringtype=unspecified&TimeZone=UTC&bitToString=true` is not recognized, please refer to the pattern `(?<schema>[\w-.+:%%]+)\s*(?://(?<authority>[^/?#]*))?\s*(?:/(?!\s*/)(?<path>[^?#]*))?(?:\?(?!\s*\?)(?<query>[^#]*))?`.]
Looks `pipeline_it_0??ssl=on` is wrong, there're two `?`. Could you debug to verify the `sql` values?
You could refer to [Pipeline E2E test]( https://shardingsphere.apache.org/document/current/cn/test-manual/pipeline-e2e-test/ ) to run it on local.
--
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] susongyan commented on pull request #27909: escape pg ? operator in proxy
Posted by "susongyan (via GitHub)" <gi...@apache.org>.
susongyan commented on PR #27909:
URL: https://github.com/apache/shardingsphere/pull/27909#issuecomment-1667772924
>
ok, let me check 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.
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 #27909: escape pg ? operator in proxy
Posted by "TeslaCN (via GitHub)" <gi...@apache.org>.
TeslaCN merged PR #27909:
URL: https://github.com/apache/shardingsphere/pull/27909
--
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