db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Knut Anders Hatlen <Knut.Hat...@Sun.COM>
Subject Re: [jira] Commented: (DERBY-796) jdbc 4.0 specific Blob and Clob method support
Date Tue, 14 Feb 2006 22:58:56 GMT
Bryan Pendleton <bpendleton@amberpoint.com> writes:

> Thanks for calling my attention to the diff; I am glad I read it
> because I hadn't really been paying attention to this thread.
>
> Regarding what I think to be the basic concepts of this patch:
>   - using a factory to encapsulate the JDBC 4.0-specific class creation
>   - implementing the get/set Blob/Clob methods in the JDBC 4.0
>     classes as appropriate
> I think that I understand the overall changes and they seem
> reasonable to me.
>
> I had two substantive comments on the patch:
>
> 1) It would be good to have a comment explaining why this
>     change was made:
> -        if (offset_ > (blob_.binaryString_.length - blob_.dataOffset_)) {
> +        if ((offset_-1) > (blob_.binaryString_.length - blob_.dataOffset_)) {
> It appears to be just an off-by-one error in the previous code, but
> I couldn't from trivial inspection understand why it needed to be changed,
> what test(s) covered this case, etc.

There is a similar change in ClobOutputStream:

-        if (offset_ > clob_.sqlLength_) {
+        if ((offset_-1) > clob_.sqlLength_) {
             throw new IndexOutOfBoundsException();

As Bryan said, it is an off-by-one in the previous code, but I think
it should have been fixed somewhere else. The problem is that offset_
is not really an offset, but an index counting from 1.

I think I would have changed the implementation of ClobOutputStream
and BlobOutputStream to use offset_ as a real offset counting from
zero. This way, we could make the code clearer by replacing all
occurrences of (offset_-1) with offset_. We would also have to change
the calls the the constructors of ClobOutputStream and
BlobOutputStream. They are currently only called from
Clob.setAsciiStream(long) and Blob.setBinaryStream(long).

E.g., in Clob.setAsciiStream() we would have to change
    ClobOutputStream outStream = new ClobOutputStream(this, pos);
to
    ClobOutputStream outStream = new ClobOutputStream(this, pos - 1);

In my opinion, it is a cleaner approach to do the conversion from
one-based index to zero-based index on the highest possible level, so
that the underlying implementation doesn't have to mix those two
conventions. It's the mixing that is causing off-by-one bugs like the
one Narayanan was trying to fix.

-- 
Knut Anders


Mime
View raw message