/pgjdbc

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: support generated keys for WITH queries #1138

Merged
merged 8 commits into from Mar 14, 2018

Conversation

Reviewers

@davecramer davecramer

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
  42.2.2
3 participants
@vlsi @codecov-io @davecramer
@vlsi
Member

vlsi commented Mar 9, 2018

pgjdbc ignored to add RETURNING to WITH queries even though it makes sense.

fixes #1104

@vlsi vlsi added this to the 42.2.2 milestone Mar 9, 2018

@codecov-io

This comment has been minimized.

Sign in to view

codecov-io commented Mar 10, 2018 •
edited

Codecov Report

Merging #1138 into master will increase coverage by 1.42%.
The diff coverage is 73.52%.

@@             Coverage Diff              @@
##             master    #1138      +/-   ##
============================================
+ Coverage     67.52%   68.94%   +1.42%     
- Complexity     3719     3775      +56     
============================================
  Files           170      170              
  Lines         15708    15757      +49     
  Branches       2561     2577      +16     
============================================
+ Hits          10607    10864     +257     
+ Misses         3906     3658     -248     
- Partials       1195     1235      +40
@davecramer

This comment has been minimized.

Sign in to view
Member

davecramer commented Mar 10, 2018

So this now adds a returning clause if the query has a with clause ?
@vlsi

This comment has been minimized.

Sign in to view
Member

vlsi commented Mar 10, 2018

It does so unless returning clause already exists.

vlsi added some commits Mar 9, 2018

@vlsi
fix: support generated keys for WITH queries 
pgjdbc ignored to add RETURNING to WITH queries even though it makes sense.

fixes #1104
Unverified
The key that signed this is expired.
GPG key ID: 38F47D3E410C47B1 Learn about signing commits
41b069c
@vlsi
fix: WITH INSERT requires PostgreSQL 9.1
Unverified
The key that signed this is expired.
GPG key ID: 38F47D3E410C47B1 Learn about signing commits
68a9f19
@vlsi

This comment has been minimized.

Sign in to view
Member

vlsi commented Mar 11, 2018

The corner case here is WITH x as (INSERT INTO genkeys(a,b,c) VALUES (1, 'a', 2) returning ?) select * from x

Current parser is unable to tell WITH x as (INSERT INTO genkeys(a,b,c) VALUES (1, 'a', 2) returning ?) insert into genkeys select * from x from WITH x as (INSERT INTO genkeys(a,b,c) VALUES (1, 'a', 2) returning ?) select * from x

returning requires DML, so the parser should somehow identify WITH (...) select and avoid adding returning there.
@vlsi
update changelog and add a test case
Unverified
The key that signed this is expired.
GPG key ID: 38F47D3E410C47B1 Learn about signing commits
748fc18
@vlsi

This comment has been minimized.

Sign in to view
Member

vlsi commented Mar 11, 2018

This one is valid as well:

with insert as (select 1) select * from insert;
 ?column?
----------
        1

vlsi added some commits Mar 11, 2018

@vlsi
Improve WITH vs generated keys handling
Unverified
The key that signed this is expired.
GPG key ID: 38F47D3E410C47B1 Learn about signing commits
Loading status checks…
4ca66e2
@vlsi
style: checkstyle
Unverified
The key that signed this is expired.
GPG key ID: 38F47D3E410C47B1 Learn about signing commits
e78aed3
@vlsi
test: add with ... insert select test
Unverified
The key that signed this is expired.
GPG key ID: 38F47D3E410C47B1 Learn about signing commits
405b0ea
@vlsi
add INSERT/**/ test and changelog
Unverified
The key that signed this is expired.
GPG key ID: 38F47D3E410C47B1 Learn about signing commits
Loading status checks…
4db7cfb

@vlsi vlsi added the needs-review label Mar 12, 2018

pgjdbc/src/test/java/org/postgresql/test/jdbc3/GeneratedKeysTest.java
@@ -198,7 +200,7 @@ public void testMultipleRows() throws SQLException {
public void testSerialWorks() throws SQLException {
Statement stmt = con.createStatement();
int count = stmt.executeUpdate(
"INSERT INTO genkeys (b,c) VALUES ('a', 2), ('b', 4)" + returningClause + "; ",
"INSERT/*fool parser*/ INTO genkeys (b,c) VALUES ('a', 2), ('b', 4)" + returningClause + "; ",

This comment has been minimized.

Sign in to view
Copy link
@davecramer

davecramer Mar 12, 2018

Member

can we handle a nested comment ?

This comment has been minimized.

Sign in to view
Copy link
@vlsi

vlsi Mar 12, 2018

Member

parseBlockComment does handle that case as well. I've added a test case just in case

This comment has been minimized.

Sign in to view
Copy link
@davecramer

davecramer Mar 12, 2018

Member

nice!
@vlsi
test: add parser test with nested comments
Unverified
The key that signed this is expired.
GPG key ID: 38F47D3E410C47B1 Learn about signing commits
Loading status checks…
a068f55
@davecramer

This comment has been minimized.

Sign in to view
Member

davecramer commented Mar 14, 2018

Looks good to me

@vlsi vlsi removed the needs-review label Mar 14, 2018

@vlsi vlsi merged commit 04e7666 into pgjdbc:master Mar 14, 2018
2 checks passed

2 checks passed

codecov/project 68.94% (+1.42%) compared to fcd1ea1
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

rhavermans added a commit to bolcom/pgjdbc that referenced this pull request Jul 13, 2018

@vlsi @rhavermans
fix: better support for RETURNING for WITH queries and queries with c… 
…omments (pgjdbc#1138)

1) pgjdbc ignored to add RETURNING to WITH queries even though it makes sense.
2) `insert/*comment*/` was not treated as `insert` statement, thus the driver did not add `returning`.

fixes pgjdbc#1104
b870241

rhavermans added a commit to bolcom/pgjdbc that referenced this pull request Jul 13, 2018

@vlsi @rhavermans
fix: better support for RETURNING for WITH queries and queries with c… 
…omments (pgjdbc#1138)

1) pgjdbc ignored to add RETURNING to WITH queries even though it makes sense.
2) `insert/*comment*/` was not treated as `insert` statement, thus the driver did not add `returning`.

fixes pgjdbc#1104
fb0f472
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment