Hi,

The current UpdateableResultSet tests did not properly test more complex binary data updating.
The following patch makes the test a bit harder and fixes the driver to pass the test.

Index: org/postgresql/test/jdbc2/UpdateableResultTest.java
================================================== =================
RCS file: /usr/local/cvsroot/pgjdbc/pgjdbc/org/postgresql/test/jdbc2/UpdateableResultTest.java,v
retrieving revision 1.24
diff -u -r1.24 UpdateableResultTest.java
--- org/postgresql/test/jdbc2/UpdateableResultTest.java 16 Apr 2007 16:36:41 -0000 1.24
+++ org/postgresql/test/jdbc2/UpdateableResultTest.java 20 Jul 2007 22:28:21 -0000
@@ -10,6 +10,8 @@
package org.postgresql.test.jdbc2;

import java.sql.*;
+import java.util.Arrays;
+
import junit.framework.TestCase;

import java.io.InputStream;
@@ -238,25 +240,26 @@
String string = "Hello";
InputStream asi = new ByteArrayInputStream(string.getBytes("US-ASCII"));
Reader chr = new StringReader(string);
- InputStream bin = new ByteArrayInputStream(string.getBytes("US-ASCII"));
+ byte[] bytes = new byte[]{0,'\\',(byte) 128,(byte) 255};
+ InputStream bin = new ByteArrayInputStream(bytes);

rs.updateInt("id", 2);
rs.updateAsciiStream("asi", asi, 5);
rs.updateCharacterStream("chr", chr, 5);
- rs.updateBinaryStream("bin", bin, 5);
+ rs.updateBinaryStream("bin", bin, bytes.length);
rs.updateRow();

assertEquals(2, rs.getInt(1));
assertEquals(string, rs.getString(2));
assertEquals(string, rs.getString(3));
- assertEquals(string, rs.getString(4));
+ assertEquals("Got wrong bytes immediately after update", Arrays.toString(bytes), Arrays.toString(rs.getBytes(4)));

rs.refreshRow();

assertEquals(2, rs.getInt(1));
assertEquals(string, rs.getString(2));
assertEquals(string, rs.getString(3));
- assertEquals(string, rs.getString(4));
+ assertEquals("Got wrong bytes from database", Arrays.toString(bytes), Arrays.toString(rs.getBytes(4)));

rs.close();
stmt.close();
Index: org/postgresql/jdbc2/AbstractJdbc2ResultSet.java
================================================== =================
RCS file: /usr/local/cvsroot/pgjdbc/pgjdbc/org/postgresql/jdbc2/AbstractJdbc2ResultSet.java,v
retrieving revision 1.94
diff -u -r1.94 AbstractJdbc2ResultSet.java
--- org/postgresql/jdbc2/AbstractJdbc2ResultSet.java 16 Apr 2007 16:36:41 -0000 1.94
+++ org/postgresql/jdbc2/AbstractJdbc2ResultSet.java 20 Jul 2007 22:28:21 -0000
@@ -1205,6 +1205,7 @@

if ( rs.next() )
{
+ System.arraycopy(rs.fields, 0, fields, 0, numColumns);
rowBuffer = rs.rowBuffer;
}

@@ -1657,6 +1658,7 @@
{
String columnName = (String) columns.next();
int columnIndex = findColumn( columnName ) - 1;
+ fields[columnIndex].setFormat(Field.TEXT_FORMAT);

Object valueObject = updateValues.get(columnName);
if (valueObject instanceof NullObject)
@@ -1709,6 +1711,13 @@
// Should never happen?
break;

+ case Types.BINARY:
+ case Types.LONGVARBINARY:
+ case Types.VARBINARY:
+ fields[columnIndex].setFormat(Field.BINARY_FORMAT);
+ rowBuffer[columnIndex] = (byte[]) valueObject;
+ break;
+
default:
rowBuffer[columnIndex] = (byte[]) valueObject;
}



---------------------------(end of broadcast)---------------------------
TIP 9: In versions below 8.0, the planner will ignore your desire to
choose an index scan if your joining column's datatypes do not
match

On Sat, 21 Jul 2007, Mikko Tiihonen wrote:

> The current UpdateableResultSet tests did not properly test more complex
> binary data updating. The following patch makes the test a bit harder
> and fixes the driver to pass the test.

I think this works for the single row update and then retrieve test, but
if your ResultSet has two rows and you do update, next, and then getBytes,
won't it have the wrong fields[i].getFormat() value? The problem is that
we need to keep the format on a per row basis. Consider a scrollable
updatable resultset, any row can have either text or binary format. The
safest thing to do at the moment would be to encode the byte[] into the
text format that the server returns, but this is clearly not an ideal
solution.

Kris Jurka

PS (Great to see you back in action!)

---------------------------(end of broadcast)---------------------------
TIP 3: Have you checked our extensive FAQ?

http://www.postgresql.org/docs/faq

On Sat, 2007-07-21 at 11:40 -0400, Kris Jurka wrote:
>
> On Sat, 21 Jul 2007, Mikko Tiihonen wrote:
>
> > The current UpdateableResultSet tests did not properly test more complex
> > binary data updating. The following patch makes the test a bit harder
> > and fixes the driver to pass the test.
>
> I think this works for the single row update and then retrieve test, but
> if your ResultSet has two rows and you do update, next, and then getBytes,
> won't it have the wrong fields[i].getFormat() value? The problem is that
> we need to keep the format on a per row basis. Consider a scrollable
> updatable resultset, any row can have either text or binary format. The
> safest thing to do at the moment would be to encode the byte[] into the
> text format that the server returns, but this is clearly not an ideal
> solution.

You are correct. Here's an updated patch that does just that. The newer unit
test tests for the new case too. I first tried cloning the "fields" field and
restoring it whenever the row moved. But that did not work when moving to a
row that was inserted or updated. So we really do need either types per row
or convert the fields into correct text/binary format during update.

The actual fix is really hack. But at least it is passes the tests.

Index: org/postgresql/jdbc2/AbstractJdbc2ResultSet.java
================================================== =================
RCS file: /usr/local/cvsroot/pgjdbc/pgjdbc/org/postgresql/jdbc2/AbstractJdbc2ResultSet.java,v
retrieving revision 1.94
diff -u -r1.94 AbstractJdbc2ResultSet.java
--- org/postgresql/jdbc2/AbstractJdbc2ResultSet.java 16 Apr 2007 16:36:41 -0000 1.94
+++ org/postgresql/jdbc2/AbstractJdbc2ResultSet.java 21 Jul 2007 20:11:56 -0000
@@ -1709,6 +1709,20 @@
// Should never happen?
break;

+ case Types.BINARY:
+ case Types.LONGVARBINARY:
+ case Types.VARBINARY:
+ if (fields[columnIndex].getFormat() == Field.BINARY_FORMAT) {
+ rowBuffer[columnIndex] = (byte[]) valueObject;
+ } else {
+ try {
+ rowBuffer[columnIndex] = PGbytea.toPGString((byte[]) valueObject).getBytes("ISO-8859-1");
+ } catch (UnsupportedEncodingException e) {
+ e.printStackTrace();
+ }
+ }
+ break;
+
default:
rowBuffer[columnIndex] = (byte[]) valueObject;
}
Index: org/postgresql/test/jdbc2/UpdateableResultTest.java
================================================== =================
RCS file: /usr/local/cvsroot/pgjdbc/pgjdbc/org/postgresql/test/jdbc2/UpdateableResultTest.java,v
retrieving revision 1.24
diff -u -r1.24 UpdateableResultTest.java
--- org/postgresql/test/jdbc2/UpdateableResultTest.java 16 Apr 2007 16:36:41 -0000 1.24
+++ org/postgresql/test/jdbc2/UpdateableResultTest.java 21 Jul 2007 20:11:56 -0000
@@ -10,6 +10,8 @@
package org.postgresql.test.jdbc2;

import java.sql.*;
+import java.util.Arrays;
+
import junit.framework.TestCase;

import java.io.InputStream;
@@ -217,6 +219,9 @@

public void testUpdateStreams() throws SQLException, UnsupportedEncodingException
{
+ String string = "Hello";
+ byte[] bytes = new byte[]{0,'\\',(byte) 128,(byte) 255};
+
Statement stmt = con.createStatement(ResultSet.TYPE_SCROLL_INSENSIT IVE, ResultSet.CONCUR_UPDATABLE);
ResultSet rs = stmt.executeQuery("SELECT id, asi, chr, bin FROM stream");

@@ -227,6 +232,13 @@
rs.updateBinaryStream("bin", null, 0);
rs.insertRow();

+ rs.moveToInsertRow();
+ rs.updateInt(1, 3);
+ rs.updateAsciiStream("asi", new ByteArrayInputStream(string.getBytes("US-ASCII")), 5);
+ rs.updateCharacterStream("chr", new StringReader(string), 5);
+ rs.updateBinaryStream("bin", new ByteArrayInputStream(bytes), bytes.length);
+ rs.insertRow();
+
rs.beforeFirst();
rs.next();

@@ -235,29 +247,31 @@
assertNull(rs.getString(3));
assertNull(rs.getBytes(4));

- String string = "Hello";
- InputStream asi = new ByteArrayInputStream(string.getBytes("US-ASCII"));
- Reader chr = new StringReader(string);
- InputStream bin = new ByteArrayInputStream(string.getBytes("US-ASCII"));
-
rs.updateInt("id", 2);
- rs.updateAsciiStream("asi", asi, 5);
- rs.updateCharacterStream("chr", chr, 5);
- rs.updateBinaryStream("bin", bin, 5);
+ rs.updateAsciiStream("asi", new ByteArrayInputStream(string.getBytes("US-ASCII")), 5);
+ rs.updateCharacterStream("chr", new StringReader(string), 5);
+ rs.updateBinaryStream("bin", new ByteArrayInputStream(bytes), bytes.length);
rs.updateRow();

assertEquals(2, rs.getInt(1));
assertEquals(string, rs.getString(2));
assertEquals(string, rs.getString(3));
- assertEquals(string, rs.getString(4));
+ assertEquals(Arrays.toString(bytes), Arrays.toString(rs.getBytes(4)));

rs.refreshRow();

assertEquals(2, rs.getInt(1));
assertEquals(string, rs.getString(2));
assertEquals(string, rs.getString(3));
- assertEquals(string, rs.getString(4));
+ assertEquals(Arrays.toString(bytes), Arrays.toString(rs.getBytes(4)));
+
+ rs.next();

+ assertEquals(3, rs.getInt(1));
+ assertEquals(string, rs.getString(2));
+ assertEquals(string, rs.getString(3));
+ assertEquals(Arrays.toString(bytes), Arrays.toString(rs.getBytes(4)));
+
rs.close();
stmt.close();
}



---------------------------(end of broadcast)---------------------------
TIP 6: explain analyze is your friend

On Sat, 21 Jul 2007, Mikko Tiihonen wrote:

> On Sat, 2007-07-21 at 11:40 -0400, Kris Jurka wrote:
>>
>> On Sat, 21 Jul 2007, Mikko Tiihonen wrote:
>>
>>> The current UpdateableResultSet tests did not properly test more complex
>>> binary data updating. The following patch makes the test a bit harder
>>> and fixes the driver to pass the test.
>>
>> I think this works for the single row update and then retrieve test, but
>> if your ResultSet has two rows and you do update, next, and then getBytes,
>> won't it have the wrong fields[i].getFormat() value? The problem is that
>> we need to keep the format on a per row basis. Consider a scrollable
>> updatable resultset, any row can have either text or binary format. The
>> safest thing to do at the moment would be to encode the byte[] into the
>> text format that the server returns, but this is clearly not an ideal
>> solution.
>
> You are correct. Here's an updated patch that does just that. The newer unit
> test tests for the new case too. I first tried cloning the "fields" field and
> restoring it whenever the row moved. But that did not work when moving to a
> row that was inserted or updated. So we really do need either types per row
> or convert the fields into correct text/binary format during update.
>
> The actual fix is really hack. But at least it is passes the tests.
>

Fixed in CVS for 8.0, 8.1, 8.2, and HEAD.

Kris Jurka


---------------------------(end of broadcast)---------------------------
TIP 1: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to majordomo@postgresql.org so that your
message can get through to the mailing list cleanly