Skip to content
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

Use generatedKeys to fetch sequence values on insertRow() #1743

Merged
merged 9 commits into from May 6, 2020

Conversation

kimjand
Copy link
Contributor

@kimjand kimjand commented Mar 28, 2020

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

New Feature Submissions:

  1. Does your submission pass tests?
  2. Does mvn checkstyle:check pass ?
  3. Have you added your new test classes to an existing test suite in alphabetical order?

Changes to Existing Features:

  • Does this break existing behaviour? If so please explain.
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your core changes, as applicable?
  • Have you successfully run tests with your changes locally?

Description

I am not sure if this should be considered a new feature or a bugfix to the Updatable ResultSet method insertRow().
After a a row is inserted, we do not know the value of any generated columns. So why not use an insert statement created with RETURN_GENERATED_KEYS to fill in the blanks?

What I did was look at the way primaryKeys is being build. So I concluded that aliasing is not allowed in the SQL text for the Updatable Statement. This means we can rely on the field PrimaryKey.name being present in the generatedKeys resultset.
So I changed updateRowBuffer to update the rowBuffer on insert, using the mapping provided by the primaryKeys list.

Any thoughts on this? Tests pass, but I might have missed something as this is my first attempt at a pull request.

kimjand and others added 3 commits Mar 28, 2020
Signed-off-by: KJA <31474178+kimjand@users.noreply.github.com>
Signed-off-by: KJA <31474178+kimjand@users.noreply.github.com>
@davecramer
Copy link
Member

davecramer commented Mar 28, 2020

Seems legit. I'd like to see tests where more than just one column is generated

Signed-off-by: KJA <31474178+kimjand@users.noreply.github.com>
@kimjand
Copy link
Contributor Author

kimjand commented Mar 28, 2020

So you would like to see some more tests with a composite primary key?

Signed-off-by: KJA <31474178+kimjand@users.noreply.github.com>
Signed-off-by: KJA <31474178+kimjand@users.noreply.github.com>
@codecov-io
Copy link

codecov-io commented Mar 28, 2020

Codecov Report

Merging #1743 into master will increase coverage by 0.15%.
The diff coverage is 62.50%.

@@             Coverage Diff              @@
##             master    #1743      +/-   ##
============================================
+ Coverage     69.31%   69.46%   +0.15%     
- Complexity     4205     4217      +12     
============================================
  Files           187      187              
  Lines         17312    17325      +13     
  Branches       2882     2883       +1     
============================================
+ Hits          11999    12034      +35     
+ Misses         4018     4001      -17     
+ Partials       1295     1290       -5     

Signed-off-by: KJA <31474178+kimjand@users.noreply.github.com>
@kimjand
Copy link
Contributor Author

kimjand commented Mar 30, 2020

Do you have any other comments or objections against this change?

@mellson
Copy link

mellson commented Apr 1, 2020

Great PR! I would love for this to be merged 👍

@kimjand
Copy link
Contributor Author

kimjand commented May 1, 2020

How do we proceed from here? Or should I consider this change rejected?

} else {
try {
rowBuffer.set(columnIndex,
PGbytea.toPGString((byte[]) valueObject).getBytes("ISO-8859-1"));
Copy link
Member

@davecramer davecramer May 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why ISO-8859-1 ? The driver generally assumes UTF-8

Copy link
Contributor Author

@kimjand kimjand May 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While a valid question, I do not have the answer. I just pulled the loop out of this method, reducing the indentation level by one.

Copy link
Member

@davecramer davecramer May 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmmm ok, for now leave it but can you add a new issue to review this later

Signed-off-by: KJA <31474178+kimjand@users.noreply.github.com>
@davecramer davecramer merged commit 9f398c5 into pgjdbc:master May 6, 2020
2 of 4 checks passed
davecramer pushed a commit to davecramer/pgjdbc that referenced this issue Jul 5, 2021
* Use generatedKeys to fetch sequence values on insertRow().
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants