Clarify that css::sdbc::XClob::getSubString takes 1-based pos

... and fix connectivity::firebird::Clob::getSubString.

XClob::getSubString is modelled after JDBC counterpart, that also
takes 1-based position argument. This is also described in multiple
places in our code:

- OPreparedStatement::setClob
  in connectivity/source/drivers/firebird/PreparedStatement.cxx
- OSingleSelectQueryComposer::setConditionByColumn
  in dbaccess/source/core/api/SingleSelectQueryComposer.cxx
- ORowSetValue::getString
  in connectivity/source/commontools/FValue.cxx

However in some places 0-based value was used (fixed here).
To clarify, the mention that the pos argument is 1-based is put
to the corresponding IDL file.

Also the code in connectivity::firebird::Clob::getSubString had
multiple issues:

- no checks of arguments;
- possibility to throw "nPosition out of range" when just-read
  segment has enough data;
- wrong start position in case when nPosition is not aligned to
  segment boundary.

This change fixes these, and simplifies the implementation.

Change-Id: I119a62dd7f02c9569ce36395ed8cc1a98c931fcf
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/121884
Tested-by: Jenkins
Reviewed-by: Mike Kaganski <mike.kaganski@collabora.com>

4 files changed, 31 insertions, 35 deletions

diff --git a/connectivity/source/drivers/firebird/Clob.cxx b/connectivity/source/drivers/firebird/Clob.cxx
index 5825a5f2c871..707020b39bbf 100644
--- a/connectivity/source/drivers/firebird/Clob.cxx
+++ b/connectivity/source/drivers/firebird/Clob.cxx
@@ -71,55 +71,51 @@ sal_Int64 SAL_CALL Clob::length()
 OUString SAL_CALL Clob::getSubString(sal_Int64 nPosition,
                                                sal_Int32 nLength)
 {
+    if (nPosition < 1) // XClob is indexed from 1
+        throw lang::IllegalArgumentException("nPosition < 1", *this, 0);
+    --nPosition; // make 0-based
+
+    if (nLength < 0)
+        throw lang::IllegalArgumentException("nLength < 0", *this, 0);
+
     MutexGuard aGuard(m_aMutex);
     checkDisposed(Clob_BASE::rBHelper.bDisposed);
     // TODO do not reset position if it is not necessary
     m_aBlob->closeInput(); // reset position
 
     OUStringBuffer sSegmentBuffer;
-    sal_Int64 nActPos = 1;
-    sal_Int32 nActLen = 0;
     std::vector<char> aSegmentBytes;
 
-    // skip irrelevant parts
-    while( nActPos < nPosition )
+    for (;;)
     {
         bool bLastRead = m_aBlob->readOneSegment( aSegmentBytes );
-        if( bLastRead )
-            throw lang::IllegalArgumentException("nPosition out of range", *this, 0);
-
+        // TODO: handle possible case of split UTF-8 character
         OUString sSegment(aSegmentBytes.data(), aSegmentBytes.size(), RTL_TEXTENCODING_UTF8);
-        sal_Int32 nStrLen = sSegment.getLength();
-        nActPos += nStrLen;
-        if( nActPos > nPosition )
+
+        // skip irrelevant parts
+        if (sSegment.getLength() < nPosition)
         {
-            sal_Int32 nCharsToCopy = static_cast<sal_Int32>(nActPos - nPosition);
-            if( nCharsToCopy > nLength )
-                nCharsToCopy = nLength;
-            // append relevant part of first segment
-            sSegmentBuffer.append( sSegment.subView(0, nCharsToCopy) );
-            nActLen += sSegmentBuffer.getLength();
+            if (bLastRead)
+                throw lang::IllegalArgumentException("nPosition out of range", *this, 0);
+            nPosition -= sSegment.getLength();
+            continue;
         }
-    }
 
-    // read nLength characters
-    while( nActLen < nLength )
-    {
-        bool bLastRead = m_aBlob->readOneSegment( aSegmentBytes );
+        // Getting here for the first time, nPosition may be > 0, meaning copy start offset.
+        // This also handles sSegment.getLength() == nPosition case, including nLength == 0.
+        const sal_Int32 nCharsToCopy = std::min<sal_Int32>(sSegment.getLength() - nPosition,
+                                                           nLength - sSegmentBuffer.getLength());
+        sSegmentBuffer.append(sSegment.subView(nPosition, nCharsToCopy));
+        if (sSegmentBuffer.getLength() == nLength)
+            return sSegmentBuffer.makeStringAndClear();
 
-        OUString sSegment(aSegmentBytes.data(), aSegmentBytes.size(), RTL_TEXTENCODING_UTF8);
-        sal_Int32 nStrLen = sSegment.getLength();
-        if( nActLen + nStrLen > nLength )
-            sSegmentBuffer.append(sSegment.subView(0, nLength - nActLen));
-        else
-            sSegmentBuffer.append(sSegment);
-        nActLen += nStrLen;
-
-        if( bLastRead && nActLen < nLength )
+        assert(sSegmentBuffer.getLength() < nLength);
+
+        if (bLastRead)
             throw lang::IllegalArgumentException("out of range", *this, 0);
-    }
 
-    return sSegmentBuffer.makeStringAndClear();
+        nPosition = 0; // No offset after first append
+    }
 }
 
 uno::Reference< XInputStream > SAL_CALL  Clob::getCharacterStream()
diff --git a/connectivity/source/drivers/firebird/DatabaseMetaData.cxx b/connectivity/source/drivers/firebird/DatabaseMetaData.cxx
index fde8809ec867..eff8097e25bd 100644
--- a/connectivity/source/drivers/firebird/DatabaseMetaData.cxx
+++ b/connectivity/source/drivers/firebird/DatabaseMetaData.cxx
@@ -1417,7 +1417,7 @@ uno::Reference< XResultSet > SAL_CALL ODatabaseMetaData::getTables(
             uno::Reference< XClob > xClob = xRow->getClob(4);
             if (xClob.is())
             {
-                aCurrentRow[5] = new ORowSetValueDecorator(xClob->getSubString(0, xClob->length()));
+                aCurrentRow[5] = new ORowSetValueDecorator(xClob->getSubString(1, xClob->length()));
             }
         }
 
diff --git a/connectivity/source/drivers/firebird/ResultSet.cxx b/connectivity/source/drivers/firebird/ResultSet.cxx
index 17e87cf8a55d..892e510138a3 100644
--- a/connectivity/source/drivers/firebird/ResultSet.cxx
+++ b/connectivity/source/drivers/firebird/ResultSet.cxx
@@ -611,7 +611,7 @@ OUString OResultSet::retrieveValue(const sal_Int32 nColumnIndex, const ISC_SHORT
     else if(aSqlType == SQL_BLOB && aSqlSubType == static_cast<short>(BlobSubtype::Clob) )
     {
         uno::Reference<XClob> xClob = getClob(nColumnIndex);
-        return xClob->getSubString( 0, xClob->length() );
+        return xClob->getSubString( 1, xClob->length() );
     }
     else
     {
diff --git a/offapi/com/sun/star/sdbc/XClob.idl b/offapi/com/sun/star/sdbc/XClob.idl
index bc0d2bc81f0c..045dfc981768 100644
--- a/offapi/com/sun/star/sdbc/XClob.idl
+++ b/offapi/com/sun/star/sdbc/XClob.idl
@@ -117,7 +117,7 @@ published interface XClob: com::sun::star::uno::XInterface
         consecutive characters.
         </p>
         @param pos
-            the starting position
+            the starting position, 1-based
         @param length
             the length of the substring
         @returns