fix: preserve unquoted unicode whitespace in array literals #1266

bokken · 2018-07-21T01:55:29Z

The postgresql backend only quotes ascii whitespace.
#1257

The postgresql backend only quotes ascii whitespace. pgjdbc#1257

This reverts commit 8281083.

This reverts commit 8b5072d.

…ce""" This reverts commit e407aaf.

checkstyle

codecov-io · 2018-07-21T02:36:54Z

Codecov Report

Merging #1266 into master will increase coverage by <.01%.
The diff coverage is 50%.

@@             Coverage Diff              @@
##             master    #1266      +/-   ##
============================================
+ Coverage     68.78%   68.78%   +<.01%     
- Complexity     3908     3914       +6     
============================================
  Files           179      179              
  Lines         16420    16421       +1     
  Branches       2674     2674              
============================================
+ Hits          11295    11296       +1     
+ Misses         3880     3879       -1     
- Partials       1245     1246       +1

more unit tests

vlsi · 2018-07-21T07:58:32Z

Please create a method near

pgjdbc/pgjdbc/src/main/java/org/postgresql/core/Parser.java

Lines 777 to 778 in da831de

    
           public static boolean isSpace(char c) { 
        
             return c == ' ' || c == '\t' || c == '\n' || c == '\r' || c == '\f';

vlsi · 2018-07-21T08:00:06Z

Tests for leading/trailing/multiple whitespaces are needed as well

davecramer · 2018-07-21T12:25:02Z

pgjdbc/src/main/java/org/postgresql/jdbc/PgArray.java

@@ -529,6 +529,32 @@ private synchronized void buildArrayList() throws SQLException {
    }
  }

+  /**
+   * Is whitespace which postgresql will force quotes.


Could we elaborate a bit here. I my not have enough coffee yet but I have difficulty reading this line.

bokken · 2018-07-21T12:29:58Z

Should the Parser.isSpace method also consider vertical tab (0x0B)? That appears to be the only difference.

If we do need a different method, do you prefer the style of the switch statement or the chained OR equals? We should probably have both methods use same style.
I slightly prefer the switch as I find it easier to see all the chars vertically aligned. It is also very slightly faster, but so close that readability is what matters.

bokken · 2018-07-21T12:32:45Z

@davecramer

Is this better:

Identifies white space characters which the backend uses to determine if a {@code String} value needs to be quoted in array representation.

davecramer · 2018-07-21T12:35:02Z

@bokken yes, thanks!

more unit tests relocate utility method to Parser

vlsi · 2018-07-21T13:43:06Z

Should the Parser.isSpace method also consider vertical tab (0x0B)?

I think it should be left as is and a comment pointing to https://github.com/postgres/postgres/blob/17bb62501787c56e0518e61db13a523d47afd724/src/backend/parser/scan.l#L194-L198 should be added to existing isSpace

bokken · 2018-07-21T14:42:23Z

I will add the comment.
Do you have an opinion on switch vs chained || ==?

vlsi · 2018-07-21T15:40:59Z

I would follow the style of isSpace

add comment to existing isSpace method change from switch to chained ORs

vlsi · 2018-07-22T12:19:41Z

PostgreSQL does drop whitespace at the start and at the end of the value:

postgres=# select version();
                                                    version
----------------------------------------------------------------------------------------------------------------
 PostgreSQL 9.6.9 on x86_64-apple-darwin17.5.0, compiled by Apple LLVM version 9.1.0 (clang-902.0.39.1), 64-bit

postgres=# select '{ test test ,
postgres'# abc
postgres'#
postgres'# }'::varchar[];
      varchar
-------------------
 {"test test",abc}
(1 row)

vlsi · 2018-07-22T12:26:46Z

Just to clarify: sending data to the backend is not affected.

bokken · 2018-07-22T13:27:11Z

Correct, this only affects the parsing of strings, presumably coming from the backend.

Nothing is changed in assembling strings to send to backend.

bokken · 2018-07-22T13:31:08Z

pgjdbc/src/test/java/org/postgresql/test/jdbc2/ArrayTest.java

@@ -416,8 +416,7 @@ public void testDirectFieldString() throws SQLException {
    assertEquals(" \fnew \n ", array[2]);
    assertEquals(" ", array[3]);

-    // this is likely undesired behavior of dropping white spaces mid-value (not
-    // leading/trailing)
+    // PostgreSQL drops leading and trailing whitespace, so does the driver


This test asserts that the driver is also dropping ascii white space in the middle of an unquoted value.
This is not consistent with the backend. However, the backend should never produce this type of value.

@vlsi thoughts?
This is asserting dropping of mid value white spaces, which is probably undesired, but practically a non issue as the backend should never produce such a value.

This is not consistent with the backend

Why do you think so?

Please add a test when the same values get passed to the backend.

@vlsi I have added test (aae0d48)

It is still different from testDirectFieldString

add test showing inner white spaces not dropped on unquoted strings by backend.

bokken · 2018-07-24T21:49:28Z

In that the contents of the entire array are not the same? The specific element element being asserted with all sorts of white space characters is the same.

…

On Tue, Jul 24, 2018 at 12:17 PM Vladimir Sitnikov ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In pgjdbc/src/test/java/org/postgresql/test/jdbc2/ArrayTest.java <#1266 (comment)>: > @@ -416,8 +416,7 @@ public void testDirectFieldString() throws SQLException { assertEquals(" \fnew \n ", array[2]); assertEquals(" ", array[3]); - // this is likely undesired behavior of dropping white spaces mid-value (not - // leading/trailing) + // PostgreSQL drops leading and trailing whitespace, so does the driver It is still different from testDirectFieldString — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1266 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AC26bWWmzGF9HFfe8ofbP9ULiJaN0neVks5uJ1a3gaJpZM4VZciM> .

vlsi · 2018-07-26T09:41:01Z

The tests in this PR are hard to review.
It is not obvious what spaces get trimmed by the driver, which are trimmed (or not) by the backend and it is not clear if the behaviour of the both agree.

I agree the existing tests in that area are weak, however if you think there's a mismatch between pgjdbc and backend behavior it would be great if you could create a test that highlights that diff.

bokken · 2018-07-26T17:09:14Z

I am out on vacation for next week+. I can add more targeted tests when I return. There is a difference in how unquoted values are parsed. The backend only trims leading/trailing whitespace. The driver drops all whitespace, even those surrounded by other letters/words. Thus `un quote` becomes `unquote`. But this difference seems largely academic, as the driver is only expected to parse strings produced by the back end and the backend quotes strings with whitespace anywhere. This was the point of my comment that was asserting all white spaces, including inner, were dropped.

…

On Wed, Jul 25, 2018 at 11:41 PM Vladimir Sitnikov ***@***.***> wrote: The tests in this PR are hard to review. It is not obvious what spaces get trimmed by the driver, which are trimmed (or not) by the backend and it is not clear if the behaviour of the both agree. I agree the existing tests in that area are weak, however if you think there's a mismatch between pgjdbc and backend behavior it would be great if you could create a test that highlights that diff. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1266 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AC26bTFot4R03kzF02x-c6knFR_1xUicks5uKY6vgaJpZM4VZciM> .

vlsi · 2018-07-26T17:12:02Z

Ah, I see now.

add test explicitly showing differences between driver and backend handling unquoted string array values

checkstyle

more unit tests

more unit tests relocate utility method to Parser

add comment to existing isSpace method change from switch to chained ORs

add test showing inner white spaces not dropped on unquoted strings by backend.

add test explicitly showing differences between driver and backend handling unquoted string array values

…hitespace

AppVeyorBot · 2019-04-15T23:17:10Z

✅ Build pgjdbc 1.0.228 completed (commit 636f9c9b39 by @bokken)

# Conflicts: # pgjdbc/src/main/java/org/postgresql/jdbc/PgArray.java

bokken · 2020-08-06T22:41:45Z

@davecramer I updated this to be able to merge.

* fix: don't ignore unquoted non-ascii whitespace The postgresql backend only quotes ascii whitespace. #1257 add comment to existing isSpace method change from switch to chained ORs * test: fix comment in ArrayTest.testDirectFieldString * fix: revert spaces in array literal * add changelog entry add test showing inner white spaces not dropped on unquoted strings by backend. Co-authored-by: Vladimir Sitnikov <sitnikov.vladimir@gmail.com> Co-authored-by: BO8979 <BO8979@W1971362.northamerica.cerner.net>

) * fix: don't ignore unquoted non-ascii whitespace The postgresql backend only quotes ascii whitespace. pgjdbc#1257 add comment to existing isSpace method change from switch to chained ORs * test: fix comment in ArrayTest.testDirectFieldString * fix: revert spaces in array literal * add changelog entry add test showing inner white spaces not dropped on unquoted strings by backend. Co-authored-by: Vladimir Sitnikov <sitnikov.vladimir@gmail.com> Co-authored-by: BO8979 <BO8979@W1971362.northamerica.cerner.net>

bokken added 5 commits July 20, 2018 20:50

fix: don't ignore unquoted non-ascii whitespace

8281083

The postgresql backend only quotes ascii whitespace. pgjdbc#1257

Revert "fix: don't ignore unquoted non-ascii whitespace"

8b5072d

This reverts commit 8281083.

Revert "Revert "fix: don't ignore unquoted non-ascii whitespace""

e407aaf

This reverts commit 8b5072d.

Revert "Revert "Revert "fix: don't ignore unquoted non-ascii whitespa…

06c58fb

…ce""" This reverts commit e407aaf.

fix: don't ignore unquoted non-ascii whitespace

c1325e2

checkstyle

bokken mentioned this pull request Jul 21, 2018

unicode whitespace is removed from unquoted elements of text[] columns #1257

Open

fix: don't ignore unquoted non-ascii whitespace

e828d76

more unit tests

davecramer reviewed Jul 21, 2018

View reviewed changes

fix: don't ignore unquoted non-ascii whitespace

06837da

more unit tests relocate utility method to Parser

fix: don't ignore unquoted non-ascii whitespace

5fa5dd0

add comment to existing isSpace method change from switch to chained ORs

test: fix comment in ArrayTest.testDirectFieldString

c95fd4b

vlsi changed the title ~~fix: don't ignore unquoted non-ascii whitespace~~ fix: preserve unquoted non-ascii whitespace in array literals Jul 22, 2018

vlsi changed the title ~~fix: preserve unquoted non-ascii whitespace in array literals~~ fix: preserve unquoted unicode whitespace in array literals Jul 22, 2018

vlsi added 2 commits July 22, 2018 15:36

fix: revert spaces in array literal

3bbff4e

add changelog entry

c89a54b

bokken commented Jul 22, 2018

View reviewed changes

fix: don't ignore unquoted non-ascii whitespace

aae0d48

add test showing inner white spaces not dropped on unquoted strings by backend.

bokken and others added 12 commits August 13, 2018 07:40

fix: don't ignore unquoted non-ascii whitespace

afdbe46

add test explicitly showing differences between driver and backend handling unquoted string array values

fix: don't ignore unquoted non-ascii whitespace

51d9be9

checkstyle

fix: don't ignore unquoted non-ascii whitespace

86646fa

more unit tests

fix: don't ignore unquoted non-ascii whitespace

9b952f9

more unit tests relocate utility method to Parser

fix: don't ignore unquoted non-ascii whitespace

aa4aa03

add comment to existing isSpace method change from switch to chained ORs

test: fix comment in ArrayTest.testDirectFieldString

5bdf256

fix: revert spaces in array literal

8a3402d

add changelog entry

12a83a4

fix: don't ignore unquoted non-ascii whitespace

c70db61

add test showing inner white spaces not dropped on unquoted strings by backend.

fix: don't ignore unquoted non-ascii whitespace

7560646

add test explicitly showing differences between driver and backend handling unquoted string array values

Merge remote-tracking branch 'origin/arrays_whitespace' into arrays_w…

af3006f

…hitespace

Merge remote-tracking branch 'upstream/master' into arrays_whitespace

a13f7f1

bokken mentioned this pull request Apr 16, 2019

Refactor decoding arrays #1194

Merged

vlsi force-pushed the master branch 2 times, most recently from c25d807 to adcb194 Compare March 7, 2020 20:41

Merge remote-tracking branch 'upstream/master' into arrays_whitespace

047f1a5

# Conflicts: # pgjdbc/src/main/java/org/postgresql/jdbc/PgArray.java

davecramer merged this pull request into pgjdbc:master Aug 10, 2020

bokken mentioned this pull request Aug 17, 2020

docs: add release notes for 42.2.15 #1858

Merged

dependabot-preview bot mentioned this pull request Aug 24, 2020

Bump postgresql from 42.2.14 to 42.2.16 liquibase/liquibase-postgresql#23

Merged

sehrope mentioned this pull request Nov 9, 2020

Fix PGCopyOutputStream out of order writes #1854

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: preserve unquoted unicode whitespace in array literals #1266

fix: preserve unquoted unicode whitespace in array literals #1266

bokken commented Jul 21, 2018

codecov-io commented Jul 21, 2018 •

edited

vlsi commented Jul 21, 2018

vlsi commented Jul 21, 2018

davecramer Jul 21, 2018

bokken commented Jul 21, 2018

bokken commented Jul 21, 2018

davecramer commented Jul 21, 2018

vlsi commented Jul 21, 2018

bokken commented Jul 21, 2018

vlsi commented Jul 21, 2018

vlsi commented Jul 22, 2018

vlsi commented Jul 22, 2018

bokken commented Jul 22, 2018

bokken Jul 22, 2018

bokken Jul 23, 2018

vlsi Jul 24, 2018

vlsi Jul 24, 2018

bokken Jul 24, 2018

vlsi Jul 24, 2018

bokken commented Jul 24, 2018 via email

vlsi commented Jul 26, 2018

bokken commented Jul 26, 2018 via email

vlsi commented Jul 26, 2018

AppVeyorBot commented Apr 15, 2019

bokken commented Aug 6, 2020

fix: preserve unquoted unicode whitespace in array literals #1266

fix: preserve unquoted unicode whitespace in array literals #1266

Conversation

bokken commented Jul 21, 2018

codecov-io commented Jul 21, 2018 • edited

Codecov Report

vlsi commented Jul 21, 2018

vlsi commented Jul 21, 2018

davecramer Jul 21, 2018

Choose a reason for hiding this comment

bokken commented Jul 21, 2018

bokken commented Jul 21, 2018

davecramer commented Jul 21, 2018

vlsi commented Jul 21, 2018

bokken commented Jul 21, 2018

vlsi commented Jul 21, 2018

vlsi commented Jul 22, 2018

vlsi commented Jul 22, 2018

bokken commented Jul 22, 2018

bokken Jul 22, 2018

Choose a reason for hiding this comment

bokken Jul 23, 2018

Choose a reason for hiding this comment

vlsi Jul 24, 2018

Choose a reason for hiding this comment

vlsi Jul 24, 2018

Choose a reason for hiding this comment

bokken Jul 24, 2018

Choose a reason for hiding this comment

vlsi Jul 24, 2018

Choose a reason for hiding this comment

bokken commented Jul 24, 2018 via email

vlsi commented Jul 26, 2018

bokken commented Jul 26, 2018 via email

vlsi commented Jul 26, 2018

AppVeyorBot commented Apr 15, 2019

bokken commented Aug 6, 2020

codecov-io commented Jul 21, 2018 •

edited