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: getString for PGObject columns returns null #1154

Merged
merged 9 commits into from Apr 11, 2018

Conversation

Projects
None yet
4 participants
@KimBisgaardDmi
Copy link
Contributor

KimBisgaardDmi commented Mar 27, 2018

Here is a test case for issue #1151, and a sugested fix

@vlsi vlsi changed the title Issue 1151 fix: getString for PGObject columns returns null Mar 27, 2018

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Mar 27, 2018

Codecov Report

Merging #1154 into master will increase coverage by 0.22%.
The diff coverage is 50%.

@@             Coverage Diff              @@
##             master    #1154      +/-   ##
============================================
+ Coverage     68.94%   69.17%   +0.22%     
- Complexity     3775     3792      +17     
============================================
  Files           170      170              
  Lines         15757    15760       +3     
  Branches       2577     2578       +1     
============================================
+ Hits          10864    10902      +38     
+ Misses         3658     3617      -41     
- Partials       1235     1241       +6
@davecramer

This comment has been minimized.

Copy link
Member

davecramer commented Mar 27, 2018

I actually think we should teach internalGetObject how to deal with these.

Currently we know about hstore, uuid, refcursor, and unknown.

I think knowledge about specific types should exist in this function

@KimBisgaardDmi

This comment has been minimized.

Copy link
Contributor Author

KimBisgaardDmi commented Mar 27, 2018

Yes, but:

  1. but does not seem like a very OO thing
  2. but what about out-of-source libs (like postGis)

This code lets getObject do that, which it apparently does know how to

@davecramer

This comment has been minimized.

Copy link
Member

davecramer commented Mar 27, 2018

Fair enough. Can you add documentation as to why we are doing that there. Explaining what object we are going to to deal with etc.
Also it is failing checkstyle.

You can run checkstyle locally with maven checkstyle:check

import java.sql.SQLException;
import java.sql.Statement;

public class Binary2StringTest extends BaseTest4 {

This comment has been minimized.

@vlsi

vlsi Mar 27, 2018

Member

Please add the test to the relevant test suite. Otherwise the test is not executed.

stmt.execute("insert into testGeometric(p) values ('(1,1)'),('(2,2)')");
statement = con.prepareStatement("select p from testGeometric");

((PGStatement) statement).setPrepareThreshold(-1);

This comment has been minimized.

@vlsi

vlsi Mar 27, 2018

Member

I've started to review this, however I ended up with augmenting the existing test: #1155

Note: there was a test for bunch of geometric types. The missing bits are binary/text and getString.

@vlsi

This comment has been minimized.

Copy link
Member

vlsi commented Mar 28, 2018

@KimBisgaardDmi , please take approach to testing from #1155

@vlsi vlsi added this to the 42.2.3 milestone Mar 28, 2018

@KimBisgaardDmi

This comment has been minimized.

Copy link
Contributor Author

KimBisgaardDmi commented Mar 28, 2018

approach to testing from #1155 in the regard that #1155 does the testing needed so I should drop testcase here? or something else?

@vlsi

This comment has been minimized.

Copy link
Member

vlsi commented Mar 28, 2018

Binary2StringTest makes no sense as it duplicates GeometricTest

KimBisgaardDmi added some commits Mar 28, 2018

@vlsi

This comment has been minimized.

Copy link
Member

vlsi commented Mar 29, 2018

@KimBisgaardDmi , would you add tests?

@KimBisgaardDmi

This comment has been minimized.

Copy link
Contributor Author

KimBisgaardDmi commented Mar 30, 2018

@vlsi would you like me to pull test from #1155 ?

@vlsi

This comment has been minimized.

Copy link
Member

vlsi commented Mar 30, 2018

I do

@KimBisgaardDmi

This comment has been minimized.

Copy link
Contributor Author

KimBisgaardDmi commented Apr 2, 2018

@vlsi like this?

@@ -25,7 +25,7 @@
CharacterStreamTest.class,
UUIDTest.class,
LibPQFactoryHostNameTest.class,
XmlTest.class
XmlTest.class

This comment has been minimized.

@vlsi

vlsi Apr 2, 2018

Member

Please refrain from altering unrelated lines

This comment has been minimized.

@KimBisgaardDmi

KimBisgaardDmi Apr 2, 2018

Author Contributor

yes - corrected - violates your std though

if (obj == null) {
return null;
}
return getObject(columnIndex).toString();

This comment has been minimized.

@vlsi

vlsi Apr 2, 2018

Member

This does look odd: why is it calling getObject twice?

This comment has been minimized.

@KimBisgaardDmi

KimBisgaardDmi Apr 2, 2018

Author Contributor

Well spotted - corrected

KimBisgaardDmi added some commits Apr 2, 2018

@KimBisgaardDmi

This comment has been minimized.

Copy link
Contributor Author

KimBisgaardDmi commented Apr 11, 2018

@vlsi Is it good enough now?

@davecramer

This comment has been minimized.

Copy link
Member

davecramer commented Apr 11, 2018

@KimBisgaardDmi

This comment has been minimized.

Copy link
Contributor Author

KimBisgaardDmi commented Apr 11, 2018

@davecramer Sorry - wrong button ;-)

@davecramer

This comment has been minimized.

Copy link
Member

davecramer commented Apr 11, 2018

@KimBisgaardDmi easy enough to do .... sometimes I think Github makes it too easy to do the wrong thing :)

@vlsi

vlsi approved these changes Apr 11, 2018

@vlsi vlsi merged commit bbb6c1f into pgjdbc:master Apr 11, 2018

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
codecov/project 69.16% (+0.21%) compared to b1581e9
Details

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.