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

fix: setNull for types not in java.sql.Types (e.g. uuid) #1160

Merged
merged 9 commits into from Jul 14, 2018

Conversation

davecramer
Copy link
Member

Implement public void setNull(int parameterIndex, int t, String s)

Fixes #1159

@vlsi
Copy link
Member

vlsi commented Mar 29, 2018

@davecramer , apparently this is a bug:

ps.setNull(1, Types.ARRAY, "float8");

It should be "float8[]" or something like that.

@davecramer
Copy link
Member Author

@vlsi the bug is the test, not the code, correct ?

@vlsi
Copy link
Member

vlsi commented Mar 29, 2018

right you are

preparedParameters.setNull(parameterIndex, oid);

} else {
setNull(parameterIndex, t);
Copy link
Member

@vlsi vlsi Mar 29, 2018

Choose a reason for hiding this comment

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

Would you please rephrase as follows?

if (typeName == null) {
  setNull(parameterIndex, t);
  return;
}

checkClosed();
...

pstmt.setNull(3, Types.OTHER, "uuid");
ResultSet rs = pstmt.executeQuery();

while (rs.next()) {
Copy link
Member

Choose a reason for hiding this comment

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

The statement should return rows, so please add an assertion (to ensure nulls are passed properly).

@raubel
Copy link

raubel commented Jul 13, 2018

I was expecting to see this issue fixed in release 42.2.3, but it still open.
Any update?

@vlsi
Copy link
Member

vlsi commented Jul 13, 2018

Apparently CI check fails, so it is not mergeable. I don't think @davecramer works on this at the moment, so you can pick it up.

PS. The ticket/PR was not marked for 42.2.3 milestone, so I have not checked it before pushing 42.2.3
If you think certain PR/issue should be included, please just wave so the milestone and project are set accordingly.

@raubel
Copy link

raubel commented Jul 13, 2018

I was pretty sure that it would be contained in 42.2.3 milestone (and was really looking forward to ti) so I didn't check with the project project.
Is it possible to mark it for next release (42.3.0)?

@vlsi vlsi added this to the 42.3.0 milestone Jul 13, 2018
@vlsi vlsi added this to In progress in 42.3.0 Release via automation Jul 13, 2018
@davecramer
Copy link
Member Author

@raubel are you going to work on fixing the tests?

@raubel
Copy link

raubel commented Jul 13, 2018

@davecramer, not right now and not sure it'd be very efficient.
If you know the fix, I'll appreciate you fix it.
Let me know.

@davecramer
Copy link
Member Author

So it only fails in 8.2 I'm inclined to ignore that version for this PR

@codecov-io
Copy link

codecov-io commented Jul 13, 2018

Codecov Report

Merging #1160 into master will decrease coverage by 0.01%.
The diff coverage is 37.5%.

@@             Coverage Diff              @@
##             master    #1160      +/-   ##
============================================
- Coverage     68.69%   68.68%   -0.02%     
- Complexity     3848     3849       +1     
============================================
  Files           174      174              
  Lines         16035    16042       +7     
  Branches       2614     2615       +1     
============================================
+ Hits          11016    11019       +3     
- Misses         3781     3784       +3     
- Partials       1238     1239       +1

public void setNull(int i, int t, String s) throws SQLException {
public void setNull(int parameterIndex, int t, String typeName) throws SQLException {

if ( typeName == null ) {
Copy link
Member

@vlsi vlsi Jul 13, 2018

Choose a reason for hiding this comment

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

Would you please trim spaces around typeName == null ?

@@ -560,7 +560,7 @@ public void testToString() throws SQLException {
public void nullArray() throws SQLException {
PreparedStatement ps = con.prepareStatement("INSERT INTO arrtest(floatarr) VALUES (?)");

ps.setNull(1, Types.ARRAY, "float8");
ps.setNull(1, Types.ARRAY, "float8[]");
Copy link
Member

Choose a reason for hiding this comment

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

This is an interesting side effect. I think it is worth including in a notable changes. So clients get prepared for "unexpected" operation of setNull (since String argument has been just ignored for ages)

@vlsi vlsi removed this from In progress in 42.3.0 Release Jul 14, 2018
@vlsi vlsi modified the milestones: 42.3.0, 42.2.4 Jul 14, 2018
@vlsi vlsi changed the title Fix setNull for types not in java.sql.Types fix: setNull for types not in java.sql.Types (e.g. uuid) Jul 14, 2018
@vlsi vlsi merged commit 6287c95 into pgjdbc:master Jul 14, 2018
@davecramer davecramer added this to Done in 42.2.4 Release Jul 14, 2018
@raubel
Copy link

raubel commented Jul 14, 2018

Thank you both for your work :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants