$1, $2, … in string literals erroneously identified as bind marker

[CodeGTDLearn]

Bug Report

Versions

• Driver: io.r2dbc/r2dbc-postgresql
• Database: userspasswords
• Java: 11
• OS: Windows 10 x64

Current Behavior

The Special character "$" are not being accepted for the migration process.

Stack trace

``` By 'Writing metadata version 3' 1 rows updated web-api_1 | 2020-08-04 20:54:43.971 INFO 7 --- [tor-tcp-epoll-1] n.n.r2dbc.migrate.core.R2dbcMigrate : Applying MigrationInfo{version=4, description='insert in table userpasswords', splitByLine=false, transactional=true} web-api_1 | 2020-08-04 20:54:43.977 INFO 7 --- [tor-tcp-epoll-1] n.n.r2dbc.migrate.core.R2dbcMigrate : By 'Releasing lock after error' 1 rows updated web-api_1 | 2020-08-04 20:54:43.980 WARN 7 --- [ main] onfigReactiveWebServerApplicationContext : Exception encountered during context initialization - cancelling refresh attempt: org.springframework.beans.factory.UnsatisfiedDependencyException: Error creating bean with name 'animeController': Unsatisfied dependency expressed through field 'service'; nested exception is org.springframework.beans.factory.UnsatisfiedDependencyException: Error creating bean with name 'animeService': Unsatisfied dependency expressed through field 'repo'; nested exception is org.springframework.beans.factory.BeanCreationException: Error creating bean with name 'animeRepository' defined in academy.devdojo.webflux.repository.AnimeRepository defined in @EnableR2dbcRepositories declared on R2dbcRepositoriesAutoConfigureRegistrar.EnableR2dbcRepositoriesConfiguration: Cannot resolve reference to bean 'r2dbcDatabaseClient' while setting bean property 'databaseClient'; nested exception is org.springframework.beans.factory.UnsatisfiedDependencyException: Error creating bean with name 'org.springframework.boot.autoconfigure.data.r2dbc.R2dbcDataAutoConfiguration': Unsatisfied dependency expressed through constructor parameter 0; nested exception is org.springframework.beans.factory.BeanCreationException: Error creating bean with name 'r2dbcMigrate' defined in class path resource [name/nkonev/r2dbc/migrate/autoconfigure/R2dbcMigrateAutoConfiguration.class]: Invocation of init method failed; nested exception is java.lang.IllegalArgumentException: Statement 'INSERT INTO userspasswords (nameuser, username, password, authorities) web-api_1 | VALUES ('paulo', web-api_1 | 'paulo', web-api_1 | '{bcrypt}$2a$10$lBm1Qy45bR/fNT5i5OUBseNPcONtZs10earjLZ773qq.byhK/yKmS', web-api_1 | 'ROLE_ADMIN,ROLE_USER'); web-api_1 | web-api_1 | INSERT INTO userspasswords (nameuser, username, password, authorities) web-api_1 | VALUES ('demetria', web-api_1 | 'demetria', web-api_1 | 'devdojo', web-api_1 | 'ROLE_USER');' cannot be created. This is often due to the presence of both multiple statements and parameters at the same time. web-api_1 | 2020-08-04 20:54:43.991 INFO 7 --- [ main] ConditionEvaluationReportLoggingListener : web-api_1 | web-api_1 | Error starting ApplicationContext. To display the conditions report re-run your application with 'debug' enabled. web-api_1 | 2020-08-04 20:54:43.996 ERROR 7 --- [ main] o.s.boot.SpringApplication : Application run failed ```

Table schema

Input Code

```sql CREATE TABLE IF NOT EXISTS userspasswords ( id SERIAL PRIMARY KEY, nameuser VARCHAR(255) NOT NULL, username VARCHAR(255) NOT NULL, password VARCHAR(255) NOT NULL, authorities VARCHAR(255) NOT NULL ); INSERT INTO userspasswords (nameuser, username, password, authorities) VALUES ('paulo', 'paulo', 'devdojo', 'ROLE_ADMIN,ROLE_USER'); -- Special character $ area not allowed in the field password -- '{bcrypt}$2a$10$lBm1Qy45bR/fNT5i5OUBseNPcONtZs10earjLZ773qq.byhK/yKmS', ```

Steps to reproduce

0) GitHub project link: https://github.com/PauloPortfolio/springwebflux-tdd-devdojo

1) For example: This INSERT statement is completely normal, and it is normal processed for the r2dbc-migrate BECAUSE we have a 'plaintext' password (DEVDOJO):

INSERT INTO userspasswords 
          (nameuser, username, password, authorities)
VALUES ('demetria',
        'demetria',
        'devdojo',
        'ROLE_USER');

2) HOWEVER, when I change the field 'PASSWORD' from PLAIN-TEXT 'DEVDOJO' to ENCRYTED PASSWORD 'DEVDOJO' ({bcrypt}$2a$10$lBm1Qy45bR/fNT5i5OUBseNPcONtZs10earjLZ773qq.byhK/yKmS), the specials characteres are not allowed, specially "$", for instance (THE ERRAR IN THE ABOVE STACK-TRACE HAPPENS):

INSERT INTO userspasswords 
          (nameuser, username, password, authorities)
VALUES ('demetria',
        'demetria',
        '{bcrypt}$2a$10$lBm1Qy45bR/fNT5i5OUBseNPcONtZs10earjLZ773qq.byhK/yKmS',
        'ROLE_USER');

Input Code

Expected behavior/code

Special characteres chould be allowed, such as ( '{bcrypt}$2a$10$lBm1Qy45bR/fNT5i5OUBseNPcONtZs10earjLZ773qq.byhK/yKmS'), because it is acommon practice in the PostGres DB use.

[nkonev]

@mp911de Seems problem is here. The driver tries to determine statement type.

[mp911de]

Thanks for having a look. Indeed, the problem is related to bind parameter discovery. We I think there are actually two issues:

1. Discover whether a statement is parametrized or not
2. Discovery of the actual bind markers. The parametrized statement implementation IIRC doesn't consider quotation. Any literals inside of quoted sections need to be skipped.

That being said, feel free to submit a pull request, this bugfix is up for grabs.

[toverdijk]

Hi, first-time contributor here who would like to have a throw at this.

I took a look at the code and suspect there is a similar issue with the supports function: a semi-colon in a string literal is also being interpreted as a statement separator by ExtendedQueryPostgresqlStatement, resulting in a incorrect "false" for the supports() method.

I have been thinking about a fix, and in order to reduce the redundancy I'd like to propose three options that I think are viable:

1. Parse the SQL string before creating a query type. A new class is created that includes the SQL string, the parsed binding count, and the number of statements. The .supports() methods will take this new class as a parameter instead of the string. Implementation of the .supports() methods will then remain trivial.
2. Instead of having two implementations of PostgresqlStatement, merge the two flows into one. Which message flow to take (Simple Query or Extended Query) is determined at the execute method, depending on the number of parameters found. The query is parsed in the constructor and throws a UnsupportedOperationException when both multiple statements and parameters have been found. The PostgresqlConnection can just invoke the constructor.
3. Just parse the SQL in the supports() method in both implementations, and handle string literals correctly in these methods (maybe creating a small util class for determining the parameter count and statement count in a SQL string). This problably is the quickest fix for the above problems, but causes the parsing to happen multiple times for each (extended) query and increases code redudancy.

I think that option 2 is the cleanest/best and I'd like to implement this and update the unit test suite accordingly. Any other opinions about this?

[nkonev]

@toverdijk Drawbacks of parsing approaches are that we need very patiently re-implement on the driver side existing parsing on Postgres side. Looks like mix of responsibility.

I think to avoid any parsing and make explicit API like in JDBC (connection.extendedStatement(), connection.simpleStatement()). So application developer will must explicitly type what he or she wants.

But it seems a bit complex - we need to change existing R2DBC API.

[toverdijk]

@nkonev Avoiding parsing sounds nice, and then the API could also be made "correctly fluent" - e.g. no binding functions for the SimpleStatements.

But wouldn't require a different API for postgres as for other databases? I'm not really familiar with how other DBs handle different query types in their APIs. And like I said, this is my first interaction with this project and I don't know what the goals are in terms of compatibility between the different database drivers for R2DBC.. But i figured it might be advantageous to have some parsing to allow for named parameters as well in the long run (?).

I do agree that parsing on both the driver, as well as on the postgres side doesn't sound too good... but making breaking changes to the R2DBC API does not seem worth it IMHO.

[mp911de]

I left a comment on the R2DBC SPI issue. Parsing SQL isn't the most fun task, however it can be done by tokenizing the input and being aware of the parser state. Therefore this ticket is a more advanced one. In R2DBC, we decided against standardized parameter bind markers as each driver would have to rewrite SQL (in addition to just discovering parameters).

In R2DBC the driver knows best how to run a particular statement. Therefore, if the driver needs to distinguish whether a statement needs to be parsed or bind markers need to be identified, this is the sole business of the driver and this fact doesn't need to be carried onto the SPI level.

[mp911de]

To properly address this issue, I think we need the following:

1. Introduce a SQL introspection utility
2. We need to inspect the SQL query for how many statements we can find in there (separated by ;) and for each statement, find out about its parameter bind markers.

Once we have that, we can properly create either a simple statement (SimpleQueryPostgresqlStatement) or an extended one (ExtendedQueryPostgresqlStatement).

The current check doesn't consider escaping so it identifies $1 inside of the string as a parametrized query.

PGJDBC ships with a org.postgresql.core.Parser class that looks as if it could provide a solid foundation for what we would need.

[isabek]

Hi @mp911de. Could you assign it to me? I would like to work on this issue.

[mp911de]

Thanks for picking up on this one. It's assigned to you @Isabek.

[toverdijk]

HI @Isabek, are you still working on this? If not, I do have some time now and I'd like to pick up this issue.

[toverdijk]

@mp911de Could you reassign this to me?

[mp911de]

Done @toverdijk

[mp911de]

Fixed via #468