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: remove type name from cast exception of getBoolean and setObject #781

Merged
merged 1 commit into from May 23, 2017

Conversation

Projects
None yet
5 participants
@jorsol
Contributor

jorsol commented Mar 13, 2017

Fix PSQLException messages in getBoolean and setObject removing the type name of the column to avoid confusion.

closes #776

@codecov-io

This comment has been minimized.

codecov-io commented Mar 13, 2017

Codecov Report

Merging #781 into master will decrease coverage by 0.01%.
The diff coverage is 75%.

@@             Coverage Diff              @@
##             master     #781      +/-   ##
============================================
- Coverage     65.24%   65.23%   -0.02%     
+ Complexity     3495     3494       -1     
============================================
  Files           164      164              
  Lines         15125    15124       -1     
  Branches       2450     2450              
============================================
- Hits           9869     9866       -3     
- Misses         4078     4079       +1     
- Partials       1178     1179       +1

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 95a3f41...a30cf24. Read the comment docs.

@vlsi

This comment has been minimized.

Member

vlsi commented Mar 13, 2017

Would you please add a test case to cover the change?

@jorsol

This comment has been minimized.

Contributor

jorsol commented Mar 13, 2017

???, it's already covered, it just pass the real oid name to the PSQLException message

@vlsi

This comment has been minimized.

Member

vlsi commented Mar 13, 2017

I mean add an assert on error message, so the feature won't get corrupted by accident.

Currently pgjdbc lacks support for Struct types, and I expect Struct support might involve lots of changes, so edge cases like "wrong error message" might get broken by accident.

I understand that this particular PR is more of a cosmetics, however it is hard to keep all those edge cases in head, so having a pair of simple test (or a parameterized one) would be great.

@jorsol jorsol force-pushed the jorsol:fix-oid-name-getboolean branch from fdea9b4 to 8c006c9 Mar 13, 2017

@jorsol

This comment has been minimized.

Contributor

jorsol commented Mar 13, 2017

Added tests to check exception message.

@jorsol jorsol force-pushed the jorsol:fix-oid-name-getboolean branch from 8c006c9 to aeebae6 Mar 13, 2017

@@ -614,67 +614,78 @@ public void testBadBoolean() throws SQLException {
fail();
} catch (SQLException e) {
assertEquals(e.getSQLState(), org.postgresql.util.PSQLState.CANNOT_COERCE.getState());
assertEquals(e.getMessage(), "Cannot cast type java.lang.String to boolean: «this is not boolean»");

This comment has been minimized.

@panchenko

panchenko Mar 15, 2017

In junit the expected parameter is the first one.

This comment has been minimized.

@jorsol

jorsol Mar 15, 2017

Contributor

done

double value = numval.doubleValue();
if (value == 1.0d) {
final double value = numval.doubleValue();
if (1.0D == value) {

This comment has been minimized.

@panchenko

panchenko Mar 15, 2017

I am kind of curious if there is an agreement on switching to "Yoda conditions" 😄

This comment has been minimized.

@davecramer

davecramer Mar 15, 2017

Member

-1 In general I prefer not to change style unless we change the entire style (which I am not endorsing)

This comment has been minimized.

@jorsol

jorsol Mar 15, 2017

Contributor

done

@@ -1955,10 +1955,11 @@ public boolean getBoolean(int columnIndex) throws SQLException {
}
if (isBinary(columnIndex)) {
return BooleanTypeUtil.castToBoolean(readDoubleValue(this_row[col], fields[col].getOID(), "boolean"));
return BooleanTypeUtil.castToBoolean(readDoubleValue(this_row[col], fields[col].getOID(), "boolean"),
Oid.toString(fields[col].getOID()));

This comment has been minimized.

@panchenko

panchenko Mar 15, 2017

The implementation of Oid.toString(int) does not look super fast.
Might be type is not necessary here? only value could be enough: Cannot cast to boolean: "2"

This comment has been minimized.

@davecramer

davecramer Mar 15, 2017

Member

agreed we should avoid the toString

This comment has been minimized.

@vlsi

vlsi Mar 15, 2017

Member

@davecramer , pay attention. The conversion is performed for both fail and success cases.
I agree with @panchenko, we should avoid that toString

This comment has been minimized.

@jorsol

jorsol Mar 15, 2017

Contributor

defer the evaluation to the exception point

This comment has been minimized.

@jorsol

jorsol Mar 16, 2017

Contributor

Should we drop the type name as panchenko sugested?

@@ -98,9 +116,9 @@ private static PSQLException cannotCoerceException(final String fromType) {
private static PSQLException cannotCoerceException(final String fromType, final String value) {
if (LOGGER.isLoggable(Level.FINE)) {
LOGGER.log(Level.FINE, "Cannot cast type {0} to boolean: \"{1}\"", new Object[]{fromType, value});
LOGGER.log(Level.FINE, "Cannot cast type {0} to boolean: «{1}»", new Object[]{fromType, value});
}

This comment has been minimized.

@davecramer

davecramer Mar 15, 2017

Member

I've never seen » used in code before. What is this ?

This comment has been minimized.

@jorsol

jorsol Mar 15, 2017

Contributor

done

@jorsol jorsol force-pushed the jorsol:fix-oid-name-getboolean branch from aeebae6 to bf98a54 Mar 15, 2017

@davecramer

This comment has been minimized.

Member

davecramer commented Mar 15, 2017

+1

@jorsol jorsol force-pushed the jorsol:fix-oid-name-getboolean branch from bf98a54 to 5398eaf Mar 20, 2017

@jorsol jorsol force-pushed the jorsol:fix-oid-name-getboolean branch from 5398eaf to a30cf24 Mar 20, 2017

@jorsol jorsol changed the title from fix: use oid name on ResultSet.getBoolean to fix: remove type name from cast exception of getBoolean and setObject Mar 20, 2017

@davecramer davecramer merged commit 394b3a2 into pgjdbc:master May 23, 2017

2 checks passed

codecov/project Absolute coverage decreased by -0.01% but relative coverage increased by +9.75% compared to 95a3f41
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@vlsi vlsi added this to the 42.1.2 milestone May 23, 2017

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