Knut Anders Hatlen wrote On 02/15/06 16:23,:
>"V.Narayanan (JIRA)" <derby-dev@db.apache.org> writes:
>
>
>
>>V.Narayanan commented on DERBY-796:
>>-----------------------------------
>>
>>
>
>
>
>>Issue 5
>>-------------
>>
>>--- java/client/org/apache/derby/client/am/Blob.java (revision 377622)
>>+++ java/client/org/apache/derby/client/am/Blob.java (working copy)
>>@@ -267,7 +267,10 @@
>>
>> public int setBytesX(long pos, byte[] bytes, int offset, int len) throws SqlException
{
>> int length = 0;
>>- if ((int) pos <= 0) {
>>+ boolean insertIntoEmpty = false;
>>+ if(pos==1 && binaryString_.length==0)
>>+ insertIntoEmpty = true;
>>+ if ((int) pos <= 0 || ((!insertIntoEmpty) && (pos > binaryString_.length
- dataOffset_))) {
>> throw new SqlException(agent_.logWriter_,
>> new MessageId(SQLState.BLOB_BAD_POSITION), new Long(pos));
>> }
>>
>>It seemed like the essence of this change was something like:
>>
>> "It's usually an error to pass a 'pos' value greater than the
>> length of the blob, but if the blob is currently empty then
>> there is a special case where the caller passes 1, not 0, as
>> you might expect."
>>
>>The interpretation above is exactly what I tried to do. I will try
>>to explain what I intended and also restructure the code to make it
>>more understandable.
>>
>>There are two cases when exception needs to be thrown
>>
>>a) when pos <=0
>>b) when pos > binaryString_.length - dataOffset_
>> b.1) This case arises when your insert into a empty Blob. so here
>> pos = 1 and (binaryString_.length - dataOffset_)=0.
>> this should not result in a SQL exception being thrown.
>>
>>
>
>Is the empty blob really a special case? I think you get the same
>problem when the blob is not empty.
>
>Let's say you want to append a byte array to a blob with size 1. Then
>you have:
>
> pos = 2
> (binaryString_.length - dataOffset_) = 1
>
>In this case, (pos > binaryString_ - dataOffset_) is true and an
>exception is thrown. But there's no reason to throw an exception in
>this case, is it?
>
>I think you have to change this code:
>
> public int setBytesX(long pos, byte[] bytes, int offset, int len) throws SqlException
{
> int length = 0;
> boolean insertIntoEmpty = false;
> if(pos==1 && binaryString_.length==0)
> insertIntoEmpty = true;
> if ((int) pos <= 0 || ((!insertIntoEmpty) && (pos > binaryString_.length
- dataOffset_))) {
> throw new SqlException(agent_.logWriter_,
> new MessageId(SQLState.BLOB_BAD_POSITION), new Long(pos));
> }
> if ( pos > binaryString_.length - dataOffset_) {
> throw new SqlException(agent_.logWriter_,
> new MessageId(SQLState.BLOB_POSITION_TOO_LARGE), new Long(pos));
> }
>
>to
>
> public int setBytesX(long pos, byte[] bytes, int offset, int len) throws SqlException
{
> int length = 0;
> if ((int) pos <= 0) {
> throw new SqlException(agent_.logWriter_,
> new MessageId(SQLState.BLOB_BAD_POSITION), new Long(pos));
> }
> if (pos - 1 > binaryString_.length - dataOffset_) {
> throw new SqlException(agent_.logWriter_,
> new MessageId(SQLState.BLOB_POSITION_TOO_LARGE), new Long(pos));
> }
>
>This will solve both the special case of inserting into an empty Blob,
>and the more general case of inserting at the end.
>
>
I agree. This will be a more generalized approach. Thank you for this
one!! I will do this.
>>Issue - 6
>>-----------------
>>
>>it is a cleaner approach to do the conversion from one-based index
>>to zero-based index on the highest possible level
>>-------------------------------------------------
>>
>>Would it be acceptable if I add a proper comment explaining my
>>change for now and raise this as a seperate issue and fix it at the
>>earliest?
>>
>>
>
>It seems like the Blob/Clob code has a lot of potential
>off-by-ones. Since this is somewhat outside the scope of your patch, I
>think it is OK that you just include the simple (offset_ - 1) fixes
>for now (they seem to be sufficient to fix the bugs you have
>encountered), and address the cleanup of the offset/position confusion
>in a separate JIRA issue.
>
>
Thanks once again
Narayanan
|