Additional double quotations in returning fields

[amirnajmi]

Describe the issue
When you are working with other JDBC implementations you need to use this format for of string for returning fields eg.: ""number"". (it means you had a string that includes a field name around by double quotations.)
inside the Parser.addReturning() method the Utils.escapeIdentifier() is called and inside that doAppendEscapedIdentifier() is called.
In the body of doAppendEscapedIdentifier(), always is supposed that the returning field has not additional double quotations, and at the beginning and end of the method double quotations are added.
So you need to remove any additional double quotation of returning field before passing it to doAppendEscapedIdentifier() and it could be done with something like this in Parser.addReturning() method:
String columnName = returningColumnNames[col].replace("\"","");

For more info, I have faced this problem when I was working with Datanucleus JDO, which works with different JDBC implementations. and as I understood, the only implementation that has different behavior in this situation is PostgreSQL JDBC.

Driver Version?
42.1.4
Java Version?
openjdk version "11.0.11" 2021-04-20
OS Version?
Windows 10, 21H1
PostgreSQL Version?
13.3
To Reproduce
Steps to reproduce the behaviour:

Expected behaviour
Expected to remove additional double quotations in returning field before it getting passed to Utils.escapeIdentifier()

Logs
("created_at","expires_at","last_used_at","modified_at","x","y","z","a","b") VALUES ($1,$2,$3,$4,$5,$6,$7,$8,$9)
RETURNING """number"""
2021-08-05 17:29:33.688 UTC [650] ERROR: column ""number"" does not exist at character 245

[davecramer]

Can you confirm this exists in 42.2.23 ?

[amirnajmi]

Yes, I checked that in 42.2.23.

[davecramer]

So if memory serves me the reason we do this is to ensure that we can return "Number". Note the case. Postgres is unusual in that in order to return an identifier with mixed case it would have to be quoted.

I'm not sure I have a solution for you that will not break more apps

[amirnajmi]

You are right about mixed case fields.
But I think the doAppendEscapedIdentifier() is doing anything Postgresql needs for mixed case fields.
So why the returning field with extra double quotations can pass to this method?(the method parameter should be "mixedCase" not ""mixedCase"".)

[davecramer]

On Thu, 5 Aug 2021 at 15:54, AmirHossein Najmi ***@***.***> wrote: You are right about mixed case fields. But I think the doAppendEscapedIdentifier() is doing anything Postgresql needs for mixed case fields. So why the returning field with extra double quotations can pass to this method?(the method parameter should be "mixedCase" not ""mixedCase"".)

If you have a column FooBar and you do insert ... returning FooBar you will get an error FooBar not found. "FooBar" needs to be double quoted.

…

— You are receiving this because you commented. Reply to this email directly, view it on GitHub <#2223 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADDH5V7WUYRPYBH3NCGIBTT3LT6LANCNFSM5BUOAHIQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email> .

[amirnajmi]

I know that and for clarifying my problem, I have tested the scenario with a mixed case field.
The first image demonstrates that the input method is ""number"" and as you can see in logs, the insert operation getting failed since it couldn't find any field like that(with additional useless double quotations).

The other thing that we should mention is the returning command that was generated with PG JDBC.
INSERT INTO "service_owner" ("a","x","y","created_at","deleted","deleted_at","email","flags","locked","x","z","f","modified_at","plan","service_owner_name","verified") VALUES ($1,$2,$3,$4,$5,$6,$7,$8,$9,$10,$11,$12,$13,$14,$15,$16) RETURNING """Number"""
Obviously, this query is completely wrong.

I repeat my test by correcting the input value(remove additional double quotation before passing it to the method) of doAppendEscapedIdentifier() method in the runtime as you can see in this image.

By this change, everything works fine and this means you the doAppendEscapedIdentifier() method does not need additional double quotations in its input for returning value(even for mixed case fields).

You are doing some additional operations for adding double quotation to returning fields for preventing errors for mixed case fields, but before that, the input of the method is not normalized (by the addReturning() method).

[davecramer]

The double quotes are intended to solve the following

test=> create table t1 ("Id" serial primary key, name text);
CREATE TABLE
INSERT 0 1
test=> insert into t1 (name) values ('Dave') returning "id";
ERROR: column "id" does not exist
LINE 1: insert into t1 (name) values ('Dave') returning "id";
^
HINT: Perhaps you meant to reference the column "t1.Id".
test=> insert into t1 (name) values ('Dave') returning "Id";
Id

1
(1 row)

Please don't post images. If you want me to look at your code I need to be able to replicate it. I can't cut and paste your code from an image

[jnehlmeier]

Hi,

we use an ORM called Ebean (https://github.com/ebean-orm/ebean) which has a configuration option to escape all identifiers. With this option enabled all table/column names will be double quoted by the ORM. We need this since some columns have names that we otherwise can not select, e.g. a column named "user".

Now when we make an insert query the ORM calls JDBC API Connection.prepareStatement(String sql, String[] columnNames) with parameter sql being something like insert into "tablename" ("col1", "col2") values (?, ?) and parameter columnNames being a string array with a single double quoted column name "id". This JDBC API call then causes Postgres JDBC to build a final query containing RETURNING """id""" which fails executing.

IMHO that is clearly a bug in PG JDBC, because the above is exactly the JDBC API call I would expect to do when I need to double quote column names.

[davecramer]

Seems that if it is already quoted we should probably not quote it. Care to provide a PR ?

[jnehlmeier]

@davecramer Hm no, given that it is an utils method I would prefer if someone with better code knowledge fixes the issue.

Personally I think it is only "easily" fixable if method Utils.doAppendEscapedIdentifier(Appendable sbuf, String value) gets a new behavior which says: "If value starts and ends with a double quote then I assume it is CORRECTLY quoted and I do nothing".

Without that change the method can not decide wether or not the input in value is a quoted identifier or an unquoted identifier which happens to start/end with a double quote (e.g. it can not decide if column "id" is a quoted column named id or if its an unquoted column named "id"). In case of column names this could only be decided if we have access to the column definition of the table.

When applying the above behavior we have a breaking change for tables with columns that start/end with quotes. So if we have a table with column named "id" we can currently do prepareStatement(..., "\"id\"") and it would work because it will be quoted as """id""" (but fail to execute if the column is only defined as id as said above).
With the change applied this will now start to fail, as it won't be quoted and thus postgres will search column named id instead of "id".

However I think this change will result in better behavior since you can fix it in client code by calling preparedStatement(..., "\"\"\"id\"\"\"") and thus provide a correctly quoted column name for column named "id".

Of maybe the fix needs to be done somewhere else and not in that particular method. But then the fix is likely to be more complex.

[davecramer]

basically that's the fix. If it starts with " and ends with " then do nothing.

[amirnajmi]

I think with the explanations that @jnehlmeier provided, the issue got more clear. Thank you so much.
@davecramer, Of course, you know better than me, but I guess this is exactly the solution.

[davecramer]

Closed by #2224