Permalink
Browse files

fix: wrong results when a single statement is used with UNSPECIFIED t…

…ypes (#1137)

Timestamp, date (and some other types), 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
  • Loading branch information...
vlsi committed Mar 11, 2018
1 parent e5aab1c commit fcd1ea14545a113fe4a124e17132824e279f572e
@@ -9,6 +9,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).
- fix: allowEncodingChanges should allow set client_encoding=... [PR 1125](https://github.com/pgjdbc/pgjdbc/pull/1125)
- Wrong data from Blob/Clob when mark/reset is used [PR 971](https://github.com/pgjdbc/pgjdbc/pull/971)
- Adjust XAException return codes for better compatibility with XA specification [PR 782](https://github.com/pgjdbc/pgjdbc/pull/782)
- Wrong results when single statement is used with different bind types[PR 1137](https://github.com/pgjdbc/pgjdbc/pull/1137)

## [42.2.1] (2018-01-25)
### Known issues
@@ -10,6 +10,8 @@
import org.postgresql.util.PSQLState;

import java.lang.reflect.Field;
import java.util.HashMap;
import java.util.Map;

/**
* Provides constants for well-known backend OIDs for the types we commonly use.
@@ -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>(100);
private static final Map<String, Integer> NAME_TO_OID = new HashMap<String, Integer>(100);

static {
for (Field field : Oid.class.getFields()) {
try {
int oid = field.getInt(null);
String name = field.getName().toUpperCase();
OID_TO_NAME.put(oid, name);
NAME_TO_OID.put(name, oid);
} catch (IllegalAccessException e) {
// ignore
}
}
}

/**
* Returns the name of the oid as string.
*
@@ -84,34 +102,28 @@
* defined.
*/
public static String toString(int oid) {
try {
Field[] fields = Oid.class.getFields();
for (Field field : fields) {
if (field.getInt(null) == oid) {
return field.getName();
}
}
} catch (IllegalAccessException e) {
// never happens
String name = OID_TO_NAME.get(oid);
if (name == null) {
name = "<unknown:" + oid + ">";
}
return "<unknown:" + oid + ">";
return name;
}

public static int valueOf(String oid) throws PSQLException {
try {
return (int) Long.parseLong(oid);
} catch (NumberFormatException ex) {
}
try {
oid = oid.toUpperCase();
Field[] fields = Oid.class.getFields();
for (Field field : fields) {
if (field.getName().toUpperCase().equals(oid)) {
return field.getInt(null);
}
if (oid.length() > 0 && !Character.isDigit(oid.charAt(0))) {
Integer id = NAME_TO_OID.get(oid);
if (id == null) {
id = NAME_TO_OID.get(oid.toUpperCase());
}
if (id != null) {
return id;
}
} else {
try {
// OID are unsigned 32bit integers, so Integer.parseInt is not enough
return (int) Long.parseLong(oid);
} catch (NumberFormatException ex) {
}
} catch (IllegalAccessException e) {
// never happens
}
throw new PSQLException(GT.tr("oid type {0} not known and not a number", oid),
PSQLState.INVALID_PARAMETER_VALUE);
@@ -1393,7 +1393,7 @@ private void sendParse(SimpleQuery query, SimpleParameterList params, boolean on
// the SimpleParameterList's internal array that might be modified
// under us.
query.setStatementName(statementName, deallocateEpoch);
query.setStatementTypes(typeOIDs.clone());
query.setPrepareTypes(typeOIDs);
registerParsedQuery(query, statementName);
}

@@ -1460,7 +1460,7 @@ private void sendBind(SimpleQuery query, SimpleParameterList params, Portal port
for (int i = 1; i <= params.getParameterCount(); ++i) {
sbuf.append(",$").append(i).append("=<")
.append(params.toString(i,true))
.append(">");
.append(">,type=").append(Oid.toString(params.getTypeOID(i)));
}
sbuf.append(")");
LOGGER.log(Level.FINEST, sbuf.toString());
@@ -1771,7 +1771,7 @@ private void sendOneQuery(SimpleQuery query, SimpleParameterList params, int max
|| (!oneShot && paramsHasUnknown && queryHasUnknown && !query.isStatementDescribed());

if (!describeStatement && paramsHasUnknown && !queryHasUnknown) {
int[] queryOIDs = query.getStatementTypes();
int[] queryOIDs = query.getPrepareTypes();
int[] paramOIDs = params.getTypeOIDs();
for (int i = 0; i < paramOIDs.length; i++) {
// Only supply type information when there isn't any
@@ -1988,7 +1988,7 @@ protected void processResults(ResultHandler handler, int flags) throws IOExcepti
if ((origStatementName == null && query.getStatementName() == null)
|| (origStatementName != null
&& origStatementName.equals(query.getStatementName()))) {
query.setStatementTypes(params.getTypeOIDs().clone());
query.setPrepareTypes(params.getTypeOIDs());
}

if (describeOnly) {
@@ -16,7 +16,10 @@
import org.postgresql.jdbc.PgResultSet;

import java.lang.ref.PhantomReference;
import java.util.BitSet;
import java.util.Map;
import java.util.logging.Level;
import java.util.logging.Logger;

/**
* V3 Query implementation for a single-statement query. This also holds the state of any associated
@@ -26,6 +29,7 @@
* @author Oliver Jowett (oliver@opencloud.com)
*/
class SimpleQuery implements Query {
private static final Logger LOGGER = Logger.getLogger(SimpleQuery.class.getName());

SimpleQuery(SimpleQuery src) {
this(src.nativeQuery, src.transferModeRegistry, src.sanitiserDisabled);
@@ -114,11 +118,29 @@ void setStatementName(String statementName, short deallocateEpoch) {
this.deallocateEpoch = deallocateEpoch;
}

void setStatementTypes(int[] paramTypes) {
this.preparedTypes = paramTypes;
void setPrepareTypes(int[] paramTypes) {
// Remember which parameters were unspecified since the parameters will be overriden later by
// ParameterDescription message
for (int i = 0; i < paramTypes.length; i++) {
int paramType = paramTypes[i];
if (paramType == Oid.UNSPECIFIED) {
if (this.unspecifiedParams == null) {
this.unspecifiedParams = new BitSet();
}
this.unspecifiedParams.set(i);
}
}

// paramTypes is changed by "describe statement" response, so we clone the array
// However, we can reuse array if there is one
if (this.preparedTypes == null) {
this.preparedTypes = paramTypes.clone();
return;
}
System.arraycopy(paramTypes, 0, this.preparedTypes, 0, paramTypes.length);
}

int[] getStatementTypes() {
int[] getPrepareTypes() {
return preparedTypes;
}

@@ -127,19 +149,46 @@ String getStatementName() {
}

boolean isPreparedFor(int[] paramTypes, short deallocateEpoch) {
if (statementName == null) {
if (statementName == null || preparedTypes == null) {
return false; // Not prepared.
}
if (this.deallocateEpoch != deallocateEpoch) {
return false;
}

assert preparedTypes == null || paramTypes.length == preparedTypes.length
assert paramTypes.length == preparedTypes.length
: String.format("paramTypes:%1$d preparedTypes:%2$d", paramTypes.length,
paramTypes == null ? -1 : preparedTypes.length);
preparedTypes.length);
// Check for compatible types.
BitSet unspecified = this.unspecifiedParams;
for (int i = 0; i < paramTypes.length; ++i) {
if (paramTypes[i] != Oid.UNSPECIFIED && paramTypes[i] != preparedTypes[i]) {
int paramType = paramTypes[i];
// Either paramType should match prepared type
// Or paramType==UNSPECIFIED and the prepare type was UNSPECIFIED

// Note: preparedTypes can be updated by "statement describe"
// 1) parse(name="S_01", sql="select ?::timestamp", types={UNSPECIFIED})
// 2) statement describe: bind 1 type is TIMESTAMP
// 3) SimpleQuery.preparedTypes is updated to TIMESTAMP
// ...
// 4.1) bind(name="S_01", ..., types={TIMESTAMP}) -> OK (since preparedTypes is equal to TIMESTAMP)
// 4.2) bind(name="S_01", ..., types={UNSPECIFIED}) -> OK (since the query was initially parsed with UNSPECIFIED)
// 4.3) bind(name="S_01", ..., types={DATE}) -> KO, unprepare and parse required

int preparedType = preparedTypes[i];
if (paramType != preparedType
&& (paramType != Oid.UNSPECIFIED
|| unspecified == null
|| !unspecified.get(i))) {
if (LOGGER.isLoggable(Level.FINER)) {
LOGGER.log(Level.FINER,
"Statement {0} does not match new parameter types. Will have to un-prepare it and parse once again."
+ " To avoid performance issues, use the same data type for the same bind position. Bind index (1-based) is {1},"
+ " preparedType was {2} (after describe {3}), current bind type is {4}",
new Object[]{statementName, i + 1,
Oid.toString(unspecified != null && unspecified.get(i) ? 0 : preparedType),
Oid.toString(preparedType), Oid.toString(paramType)});
}
return false;
}
}
@@ -152,13 +201,7 @@ boolean hasUnresolvedTypes() {
return true;
}

for (int preparedType : preparedTypes) {
if (preparedType == Oid.UNSPECIFIED) {
return true;
}
}

return false;
return this.unspecifiedParams != null && !this.unspecifiedParams.isEmpty();
}

byte[] getEncodedStatementName() {
@@ -255,6 +298,9 @@ void unprepare() {
cleanupRef.enqueue();
cleanupRef = null;
}
if (this.unspecifiedParams != null) {
this.unspecifiedParams.clear();
}

statementName = null;
encodedStatementName = null;
@@ -315,6 +361,7 @@ public SqlCommand getSqlCommand() {
private final boolean sanitiserDisabled;
private PhantomReference<?> cleanupRef;
private int[] preparedTypes;
private BitSet unspecifiedParams;
private short deallocateEpoch;

private Integer cachedMaxResultRowSize;
@@ -0,0 +1,37 @@
/*
* Copyright (c) 2018, PostgreSQL Global Development Group
* See the LICENSE file in the project root for more information.
*/

package org.postgresql.core;

import org.postgresql.util.PSQLException;

import org.junit.Assert;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;

import java.util.Arrays;

@RunWith(Parameterized.class)
public class OidToStringTest {
@Parameterized.Parameter(0)
public int value;
@Parameterized.Parameter(1)
public String expected;

@Parameterized.Parameters(name = "expected={1}, value={0}")
public static Iterable<Object[]> data() {
return Arrays.asList(new Object[][]{
{142, "XML"},
{0, "UNSPECIFIED"},
{-235, "<unknown:-235>"},
});
}

@Test
public void run() throws PSQLException {
Assert.assertEquals(expected, Oid.toString(value));
}
}
@@ -0,0 +1,38 @@
/*
* Copyright (c) 2018, PostgreSQL Global Development Group
* See the LICENSE file in the project root for more information.
*/

package org.postgresql.core;

import org.postgresql.util.PSQLException;

import org.junit.Assert;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;

import java.util.Arrays;

@RunWith(Parameterized.class)
public class OidValueOfTest {
@Parameterized.Parameter(0)
public int expected;
@Parameterized.Parameter(1)
public String value;

@Parameterized.Parameters(name = "expected={0}, value={1}")
public static Iterable<Object[]> data() {
return Arrays.asList(new Object[][]{
{25, "TEXT"},
{0, "UNSPECIFIED"},
{199, "JSON_ARRAY"},
{100, "100"},
});
}

@Test
public void run() throws PSQLException {
Assert.assertEquals(expected, Oid.valueOf(value));
}
}
@@ -5,6 +5,8 @@

package org.postgresql.test.jdbc2;

import org.postgresql.core.OidToStringTest;
import org.postgresql.core.OidValueOfTest;
import org.postgresql.core.ParserTest;
import org.postgresql.core.ReturningParserTest;
import org.postgresql.core.v3.V3ParameterListTests;
@@ -77,6 +79,9 @@
ParserTest.class,
ReturningParserTest.class,

OidToStringTest.class,
OidValueOfTest.class,

PreparedStatementTest.class,
StatementTest.class,
QuotationTest.class,
Oops, something went wrong.

0 comments on commit fcd1ea1

Please sign in to comment.