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: wrong results when a single statement is used with UNSPECIFIED types #1137

Merged
merged 6 commits into from
Mar 11, 2018

Conversation

vlsi
Copy link
Member

@vlsi vlsi commented Mar 9, 2018

timestamp, date, are sent as UNSPECIFIED, and pgjdbc did not verify the actual parameter types at parse time.
This might cause wrong results if the query was parsed with explicit type (e.g. setString(...)), and then re-executed with
TIMESTAMP parameter (timestamps are sent as UNSPECIFIED, thus pgjdbc silently accepted the statement even though it should reparse)

fixes #648
closes #674

@vlsi vlsi added this to the 42.2.2 milestone Mar 9, 2018
@codecov-io
Copy link

codecov-io commented Mar 9, 2018

Codecov Report

Merging #1137 into master will increase coverage by 0.03%.
The diff coverage is 70.37%.

@@             Coverage Diff              @@
##             master    #1137      +/-   ##
============================================
+ Coverage     67.38%   67.42%   +0.03%     
- Complexity     3687     3698      +11     
============================================
  Files           170      170              
  Lines         15638    15663      +25     
  Branches       2531     2539       +8     
============================================
+ Hits          10538    10560      +22     
+ Misses         3917     3916       -1     
- Partials       1183     1187       +4

@vlsi
Copy link
Member Author

vlsi commented Mar 9, 2018

Reviews are welcome, I think the PR is ready.

}

int[] getStatementTypes() {
int[] getPrepareTypes() {
return preparedTypes;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this return a clone to prevent mutation?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a private API, and there's just one usage. No need to clone.

@@ -76,6 +78,22 @@
public static final int REF_CURSOR = 1790;
public static final int REF_CURSOR_ARRAY = 2201;

private static final Map<Integer, String> OID_TO_NAME = new HashMap<Integer, String>();
private static final Map<String, Integer> NAME_TO_OID = new HashMap<String, Integer>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could these be initialized to a better size?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could, however I'm inclined to treat that as over-engineering as it is initialization time only.

return id;
}
} else {
try {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why parse as long and truncate to int rather than just parsing directly to int?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is interesting. I just replicated existing code. Makes sense to replace with Integer

Copy link
Member

@jorsol jorsol Mar 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a (int) Long.parseLong(oid); because OIDs in PostgreSQL are unsigned int, using just Integer.parseInt(oid); will break for OIDs larger than 2147483647 with a NumberFormatException.

The cast of a long to an int simulates an unsigned int since Java don't have a primitive unsigned int, in Java 8 there is Integer.parseUnsignedInt(oid); (which more or less do the same cast) but since the driver has still to support Java 6 the (int) Long.parseLong(oid); is the valid option.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is interesting

…ypes

timestamp, date, are sent as UNSPECIFIED, and pgjdbc did not verify the actual parameter types at parse time.
This might cause wrong results if the query was parsed with explicit type (e.g. setString(...)), and then re-executed with
TIMESTAMP parameter (timestamps are sent as UNSPECIFIED, thus pgjdbc silently accepted the statement even though it should reparse)

fixes pgjdbc#648
closes pgjdbc#674
@vlsi vlsi merged commit fcd1ea1 into pgjdbc:master Mar 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PreparedStatement.executeQuery() returns wrong result
4 participants