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 to mend debug logging to allow programmatic level setting. #438

Merged
merged 3 commits into from Dec 3, 2015

Conversation

Projects
None yet
4 participants
@whitingjr
Contributor

whitingjr commented Dec 1, 2015

This PR fixes changes introduced in 1205 release.
It adds a new method to PGProperty to return a property value without using the default. Allowing NULL to be returned.
Changed the impl of isPresent method. To actually work.
Added two test cases. To check isPresent and getSetString methods both work.

The fixed isPresent method smokes out an issue in an SSL test case. Now the method is fixed this test fails now.

Changed property loader to avoid using a default when property not al…
…ready set. Allows the caller to detect when unset. Thus allowing connection Ctor to use programmatic log level value. Added test cases to check PGProperty method working properly.
@davecramer

This comment has been minimized.

Member

davecramer commented Dec 1, 2015

@whitingjr not sure why it failed Travis CI ?

@lordnelson

This comment has been minimized.

Contributor

lordnelson commented Dec 1, 2015

The tests failed:

[junit] Testsuite: org.postgresql.test.jdbc2.Jdbc2TestSuite
    [junit] Tests run: 431, Failures: 0, Errors: 1, Time elapsed: 137.177 sec
    [junit] 
    [junit] ------------- Standard Output ---------------
    [junit] file:/home/travis/build/pgjdbc/pgjdbc/jars/postgresql-9.4-1206.jdbc4.jar
    [junit] ------------- ---------------- ---------------
    [junit] Testcase: testIsPresentWithParseURLResult(org.postgresql.test.jdbc2.PGPropertyTest):    Caused an ERROR
    [junit] SSL property should be present
    [junit] java.lang.AssertionError: SSL property should be present
    [junit]     at org.postgresql.test.jdbc2.PGPropertyTest.testIsPresentWithParseURLResult(PGPropertyTest.java:212)
@whitingjr

This comment has been minimized.

Contributor

whitingjr commented Dec 2, 2015

That testcase failed because the isPresent method was not working correctly beforehand. Then my commit corrected it.
But that caused testIsPresentWithParseURLResult() to start failing.
I've updated the testcase to work as I think it was intended.

@davecramer

This comment has been minimized.

Member

davecramer commented Dec 2, 2015

Thanks!!!

Dave Cramer

On 2 December 2015 at 10:44, Jeremy Whiting notifications@github.com
wrote:

That testcase failed because the isPresent method was not working
correctly beforehand. Then my commit corrected it.
But that caused testIsPresentWithParseURLResult() to start failing.
I've updated the testcase to work as I think it was intended.


Reply to this email directly or view it on GitHub
#438 (comment).

@davecramer

This comment has been minimized.

Member

davecramer commented Dec 2, 2015

Looks like it is still failing

Dave Cramer

On 2 December 2015 at 10:54, Dave Cramer davecramer@gmail.com wrote:

Thanks!!!

Dave Cramer

On 2 December 2015 at 10:44, Jeremy Whiting notifications@github.com
wrote:

That testcase failed because the isPresent method was not working
correctly beforehand. Then my commit corrected it.
But that caused testIsPresentWithParseURLResult() to start failing.
I've updated the testcase to work as I think it was intended.


Reply to this email directly or view it on GitHub
#438 (comment).

@whitingjr

This comment has been minimized.

Contributor

whitingjr commented Dec 2, 2015

Yes, looks like the CI build has ssl tests enabled. Which means the ssl property is set when the jvm starts.
Do you see any way around this except remove the individual assertion ?

@whitingjr

This comment has been minimized.

Contributor

whitingjr commented Dec 2, 2015

I see a way. Use the System.setProperties method to replace all the props.

public String getSetString(Properties properties)
{
Object o = properties.get(_name);
if (o instanceof String)

This comment has been minimized.

@davecramer

davecramer Dec 2, 2015

Member

seems like you should check for null before checking instanceof ?

This comment has been minimized.

@whitingjr

whitingjr Dec 2, 2015

Contributor

shouldn't be necessary. instanceof only works on the reference. not the object.
in the situation the reference is null then the instanceof condition returns false.

@@ -58,6 +58,11 @@ public static String getURL(String server, int port)
sendBufferSize = "&sendBufferSize="+getSendBufferSize();
}
String ssl = "";

This comment has been minimized.

@vlsi

vlsi Dec 2, 2015

Member

Please use spaces, not tabs

This comment has been minimized.

@whitingjr

whitingjr Dec 2, 2015

Contributor

Yes will replace them with spaces.

{
Properties empty = new Properties();
assertNull(PGProperty.LOG_LEVEL.getSetString(empty));
assertNull(PGProperty.LOG_LEVEL.getSetString(empty));

This comment has been minimized.

@vlsi

vlsi Dec 2, 2015

Member

Do you really mean "double check" here?

This comment has been minimized.

@whitingjr

whitingjr Dec 2, 2015

Contributor

Well spotted. No I hadn't meant to duplicate line 238. Will remove the duplicate.

This comment has been minimized.

@davecramer

davecramer Dec 2, 2015

Member

We must be able to avoid the NPE in the code, no ?

Dave Cramer

On 2 December 2015 at 11:41, Jeremy Whiting notifications@github.com
wrote:

In org/postgresql/test/jdbc2/PGPropertyTest.java
#438 (comment):

  • /**
  • \* Check whether the isPresent method really works.
    
  • */
    
  • public void testPresenceCheck()
  • {
  •    Properties empty = new Properties();
    
  •    Object value = PGProperty.LOG_LEVEL.get(empty);
    
  •    assertNotNull(value);
    
  •    Assert.assertFalse(PGProperty.LOG_LEVEL.isPresent(empty));
    
  • }
  • public void testNullValue()
  • {
  •    Properties empty = new Properties();
    
  •    assertNull(PGProperty.LOG_LEVEL.getSetString(empty));
    
  •    assertNull(PGProperty.LOG_LEVEL.getSetString(empty));
    

Well spotted. No I hadn't meant to duplicate line 238. Will remove the
duplicate.


Reply to this email directly or view it on GitHub
https://github.com/pgjdbc/pgjdbc/pull/438/files#r46438994.

@davecramer

This comment has been minimized.

Member

davecramer commented Dec 2, 2015

getString() checks instanceof String before checking for null

Dave Cramer

On 2 December 2015 at 11:13, Jeremy Whiting notifications@github.com
wrote:

I see a way. Use the System.setProperties method to replace all the props.


Reply to this email directly or view it on GitHub
#438 (comment).

assertNull(PGProperty.LOG_LEVEL.getSetString(empty));
Properties withLogging = new Properties();
withLogging.setProperty(PGProperty.LOG_LEVEL.getName(), "2");
assertNotNull(PGProperty.LOG_LEVEL.getSetString(withLogging));

This comment has been minimized.

@vlsi

vlsi Dec 2, 2015

Member

Could you please move that to separate test method?

Restore boot time ssl property after test cases complete. Revert chan…
…ge to assertion test. Safely remove existing ssl property.
@whitingjr

This comment has been minimized.

Contributor

whitingjr commented Dec 2, 2015

@davecramer "getString() checks instanceof String before checking for null"
sorry but you lost me. Which class are you referring to ?

@davecramer

This comment has been minimized.

Member

davecramer commented Dec 2, 2015

Looks like this is passing now.

@whitingjr

This comment has been minimized.

Contributor

whitingjr commented Dec 3, 2015

Is there anything else considered necessary to make this PR suitable to pull into master ?

@davecramer

This comment has been minimized.

Member

davecramer commented Dec 3, 2015

no, it's fine. I was caught up with the maven pull

davecramer added a commit that referenced this pull request Dec 3, 2015

Merge pull request #438 from whitingjr/issue-#436
Fix: to mend debug logging to allow programmatic level setting.

@davecramer davecramer merged commit 0e5ba31 into pgjdbc:master Dec 3, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment