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: improve handling of getBoolean and setObject #732

Merged
merged 1 commit into from Jan 27, 2017

Conversation

Projects
None yet
4 participants
@jorsol
Contributor

jorsol commented Jan 16, 2017

This merge the code used to cast to boolean in getBoolean and setObject, it allows only valid values and reject wrong values.

This follows the specification more close and proper support the boolean types of PostgreSQL.

@codecov-io

This comment has been minimized.

codecov-io commented Jan 16, 2017

Current coverage is 64.41% (diff: 94.02%)

Merging #732 into master will increase coverage by 0.07%

@@             master       #732   diff @@
==========================================
  Files           163        164     +1   
  Lines         15140      15144     +4   
  Methods           0          0          
  Messages          0          0          
  Branches       2987       2979     -8   
==========================================
+ Hits           9741       9755    +14   
+ Misses         4159       4157     -2   
+ Partials       1240       1232     -8   

Powered by Codecov. Last update ee51dfc...700a747

@jorsol jorsol force-pushed the jorsol:fix-getBoolean branch 3 times, most recently from cbab259 to d82bde2 Jan 20, 2017

|| "n".equalsIgnoreCase(val) || "off".equalsIgnoreCase(val)) {
return false;
}
throw cannotCastException("String");

This comment has been minimized.

@vlsi

vlsi Jan 24, 2017

Member

I wonder if adding actual "bad" value to the error message would count as a security violation.
I think adding the value explicitly would simplify the debugging.

Any thoughts?

@davecramer

This comment has been minimized.

Member

davecramer commented Jan 25, 2017

@vlsi

This comment has been minimized.

Member

vlsi commented Jan 25, 2017

The only way it might be a security problem is if the exception wasn't
caught. This is the programmers problem IMO

In that case I think it makes sense to include the value to the exception, so programmer can understand and fix the problem.

@jorsol jorsol force-pushed the jorsol:fix-getBoolean branch from d82bde2 to 700a747 Jan 26, 2017

@jorsol

This comment has been minimized.

Contributor

jorsol commented Jan 26, 2017

@vlsi , Apart from the value in the exception, I also included the logger so it's now even easier to debug and fix the problem. I also improve the boolean tests and included an appropriate javadoc to getBoolean.

@vlsi

This comment has been minimized.

Member

vlsi commented Jan 26, 2017

@jorsol , did you check performance implications of adding a debug logging to a success path?

Quick glance suggests it is not free: http://stackoverflow.com/a/39748222/1261287

I don't think there were similar cases when "getXXX" involved memory allocation through logger.log(, new Object[]{...})

I'm inclined to remove that kind of debug logging from the hot path.
I would be fine if array allocation was guarded by the relevant isLoggable check.

@jorsol jorsol force-pushed the jorsol:fix-getBoolean branch from 700a747 to 23d74c6 Jan 26, 2017

@jorsol

This comment has been minimized.

Contributor

jorsol commented Jan 26, 2017

@vlsi , I checked now, and yes, there is a performance penalty, I simplify it to still be able to debug the value and don't hit the performance penalty when logging is off...

BTW there is always a performance penalty of having the logger enabled, but only if the level allows it. That's why in production its not recommended to have logging enabled or with a DEBUG (FINE) level, and it usage is only for that "debug".

        if (!isLoggable(level)) {
            return;
        }

@vlsi vlsi added this to the 42.0.0 milestone Jan 27, 2017

@vlsi vlsi merged commit 4942f7d into pgjdbc:master Jan 27, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jorsol jorsol deleted the jorsol:fix-getBoolean branch Jan 27, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment