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

Resolving issue #524 from pgjdbc #526

Closed
wants to merge 3 commits into from
Closed

Conversation

@goeland86
Copy link
Contributor

@goeland86 goeland86 commented Mar 2, 2016

This is to resolve the issue proposed. Adds a minor check that the fieldBytes aren't null before proceeding to reconstruct.

@codecov-io
Copy link

@codecov-io codecov-io commented Mar 2, 2016

Current coverage is 57.49%

Merging #526 into master will increase coverage by +0.01% as of 48ea8af

@@            master    #526   diff @@
======================================
  Files          143     143       
  Stmts        15128   15128       
  Branches      2966    2966       
  Methods          0       0       
======================================
+ Hit           8696    8698     +2
+ Partial       1176    1175     -1
+ Missed        5256    5255     -1

Review entire Coverage Diff as of 48ea8af

Powered by Codecov. Updated on successful CI builds.

@vlsi
Copy link
Member

@vlsi vlsi commented Mar 2, 2016

@goeland86 , Can you add a testcase that covers the change?

@goeland86
Copy link
Contributor Author

@goeland86 goeland86 commented Mar 2, 2016

@vlsi Of course. I'm just trying to launch the unit tests now before I commit the change as well.

@goeland86
Copy link
Contributor Author

@goeland86 goeland86 commented Mar 2, 2016

@vlsi Please note that I just added a few checks for null behaviour to the existing tests instead of creating brand new test functions. Let me know if you think this is insufficient?

@vlsi
Copy link
Member

@vlsi vlsi commented Mar 2, 2016

It is not sufficient.

Ideally, test case should reproduce the problem. I've tried your test and it works just fine. I mean: the test passes even without && fieldBytes != null check.
Adding a new test for "null array" is good, however current behavior is just return null, thus it is not clear how you get the NPE.

@goeland86
Copy link
Contributor Author

@goeland86 goeland86 commented Mar 2, 2016

@vlsi I'm still trying to induce the conditions which we ran into on our system in the unit test. If the test isn't corrected today it will be tomorrow. Please bear with me.

@vlsi
Copy link
Member

@vlsi vlsi commented Mar 2, 2016

@goeland86 , that's fine. Take your time.

Fix NPE on PgArray.toString() when PgArray instantiated with null
String. Now condition check on whether fieldString== null && fieldBytes
!= null to avoid NPE when fieldBytes is null.

Close #524.
@goeland86
Copy link
Contributor Author

@goeland86 goeland86 commented Mar 2, 2016

@vlsi The new test was committed. Please note that the instantiation of the PgArray with a null is part of the ORM somewhere in our system - I'm afraid I haven't had time to track down why that is yet. @yazun will probably have better knowledge of why that is in the end.

@vlsi vlsi closed this in 74b4972 Mar 2, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.