Bug Hunting Session
Bug 116936 - Firebird SDBC should implement XRowUpdate interface
Summary: Firebird SDBC should implement XRowUpdate interface
Status: NEW
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Base (show other bugs)
Version:
(earliest affected)
unspecified
Hardware: All All
: medium normal
Assignee: Not Assigned
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: Database-Firebird-Default Database-HSQLDB-Removal
  Show dependency treegraph
 
Reported: 2018-04-11 09:46 UTC by Lionel Elie Mamane
Modified: 2019-09-16 13:57 UTC (History)
7 users (show)

See Also:
Crash report or crash signature:


Attachments
First patch draft (20.64 KB, patch)
2019-09-13 08:54 UTC, Julien Nabet
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Lionel Elie Mamane 2018-04-11 09:46:59 UTC
Description:
The ResultSet object of the Firebird SDBC driver does not implement the XRowUpdate interface, which makes modifying values through the API not possible. (One can do it by running a separe "UPDATE" query). That's unfortunate, and a failure for the goal of feature-parity with HSQLDB, and "good support" in general.

To implement this, there will probably be some code commonality or copy/paste with the setXXXX series in the PreparedStatement object.

Steps to Reproduce:
.

Actual Results:  
.

Expected Results:
.


Reproducible: Always


User Profile Reset: No



Additional Info:


User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Firefox/52.0
Comment 1 Julien Nabet 2019-05-14 20:45:00 UTC
Just to be sure to understand, it would mean:
copy paste template <typename T>
void OPreparedStatement::setValue(sal_Int32 nIndex, const T& nValue, ISC_SHORT nType) from PreparedStatement.cxx into ResultSet.cxx

then retrieve all the setXXX methods (17 according to https://opengrok.libreoffice.org/xref/core/offapi/com/sun/star/sdbc/XRowUpdate.idl) and rename them like updateXXX and keep content of setXXX ones?

Finally, in ResultSet.hxx, add #include <com/sun/star/sdbc/XRowUpdate.hpp>
+ add css::sdbc::XRowUpdate in OResultSet_BASE.
?
Is it this or did I miss something?
Comment 2 Lionel Elie Mamane 2019-09-12 15:56:48 UTC
You go the right direction, but I doubt it will be 100% a straight copy/paste of updateXXX to setXXX.

btomi, you want to mentor Julien through this?
Comment 3 Julien Nabet 2019-09-13 08:54:21 UTC
Created attachment 154151 [details]
First patch draft

Here's a first draft of patch which can be applied locally with a plain "git apply <file>".

This patch builds but I suppose it'll need some changes since I don't know how to test it.
Also I think some optimizations should be done to avoid some duplicated functions  like "openBlobForWriting", "closeBlobAfterWriting", "toNumericWithoutDecimalPlace" (perhaps by creating a tools file in Firebird part?)

I'll be able to submit it for review (as WIP of course :-)) on gerrit only after my day time job but thought it could be interesting to attach it there for the moment.

Don't hesitate to comment.
Comment 4 Lionel Elie Mamane 2019-09-13 09:22:36 UTC
To test: create a firebird-embedded database; put some data in it

Create a Basic macros like and run the Tst macro

Sub ensureConnection()
        Dim DBDocUI as Object
        DBDocUI = ThisDatabaseDocument.currentController
        if not DBDocUI.isConnected then
                DBDocUI.connect
        end if
End Sub

Function get_stmt() as Object
        ensureConnection()      
        get_stmt = ThisDatabaseDocument().currentController.ActiveConnection.createStatement()
End Function

Sub Tst
    dim rs as Object
    dim stmt as Object
    stmt = get_stmt()
    rs = stmt.executeQuery("SELECT * FROM table_name ORDER BY XXXX")
    rs.next()
    rs.updateString(2, "new value")
    rs.updateDate(3, CDateToUnoDate(DateSerial(2020,05,25)))
    rs.updateRow()
    ' this should have set the second column of the first row of table_name to "new value"
    ' and the third column to the date 25 May 2020
    ' note that first column is column 1

    ' similarly test updateInt, updateTime, updateLong, updateTimestamp, updateNull, updateDouble, etc
    ' see https://api.libreoffice.org/docs/idl/ref/interfacecom_1_1sun_1_1star_1_1sdbc_1_1XRowUpdate.html#a663cf96bf1e722231ffd2587a3ef9103

    Dim col as Object
    col=rs.columns.getByIndex(1) ' fetch the _second_ column. First column is index 0
    col.updateString("new new value")
    rs.updateRow()
    ' this should have set the second column of the first row of table_name to "new new value"
    ' and I'm already cheating you a bit, because I'm increasing the scope of this enhancement :)
    ' this will require to implement the same updateXXXX in Column.cxx
End Sub
Comment 5 Julien Nabet 2019-09-13 09:40:41 UTC
I got:
"BASIC runtime error.
Object variable not set."
at the line 4:
DBDocUI = ThisDatabaseDocument.currentController

Idem if I put:
DBDocUI = ThisDatabaseDocument().currentController

About column, could you provide a bit more information? What interface/idl to implement? What example can I use or code pointer?
Comment 6 Lionel Elie Mamane 2019-09-13 09:44:10 UTC
Did you put the macro in the ODB file, not in a more general (e.g. user-wide) location?
Comment 7 Lionel Elie Mamane 2019-09-13 09:52:59 UTC
For column, the interface is
css::sdb::XColumnUpdate
https://api.libreoffice.org/docs/idl/ref/interfacecom_1_1sun_1_1star_1_1sdb_1_1XColumnUpdate.html#a9e6ad31e91c912e9f1dfa7480d9e312a

Basically, the same thing as in css:sdbc::XRowUpdate, except that it is called on a column, so the columnIndex argument is not there, since the object itself it is called on (the column!) says what column that is. One way to implement it is to just call the XRowUpdate interface with the column number filled in, passing the value along.
Comment 8 Julien Nabet 2019-09-13 09:53:18 UTC
(In reply to Lionel Elie Mamane from comment #6)
> Did you put the macro in the ODB file, not in a more general (e.g.
> user-wide) location?

Oups!
I made the change, I got this error now:
BASIC runtime error.
An exception occurred 
Type: com.sun.star.sdbc.SQLException
Message: The result set is read-only..

BTW, is it for line 4 "DBDocUI = ThisDatabaseDocument.currentController"
or  "DBDocUI = ThisDatabaseDocument().currentController"
(since line 14 has:  get_stmt = ThisDatabaseDocument().currentController.ActiveConnection.createStatement())
Comment 9 Lionel Elie Mamane 2019-09-13 09:56:02 UTC
It should be ThisDatabaseDocument everywhere, without ().

For the read-only thing, btomi96?
Comment 10 Julien Nabet 2019-09-13 10:00:38 UTC
(In reply to Lionel Elie Mamane from comment #7)
> For column, the interface is
> css::sdb::XColumnUpdate
> https://api.libreoffice.org/docs/idl/ref/
> interfacecom_1_1sun_1_1star_1_1sdb_1_1XColumnUpdate.
> html#a9e6ad31e91c912e9f1dfa7480d9e312a
> 
> Basically, the same thing as in css:sdbc::XRowUpdate, except that it is
> called on a column, so the columnIndex argument is not there, since the
> object itself it is called on (the column!) says what column that is. One
> way to implement it is to just call the XRowUpdate interface with the column
> number filled in, passing the value along.

Taking a look at https://opengrok.libreoffice.org/search?project=core&full=%22XColumnUpdate.hpp%22&defs=&refs=&path=&hist=&type=&si=defs, there's no driver in connectivity which implements this.
But let's say Firebird would be the first one.

To start with this part, should I duplicate the new update functions from ResultSet in Columns file + just remove column index?
+ #include <com/sun/star/sdb/XColumnUpdate.hpp> in connectivity/source/drivers/firebird/Column.hxx (or Columns.hxx ?)

I'm asking but I'll give it a try only after ResultSet part to avoid some confusion (above all for me! :-)).
Comment 11 Lionel Elie Mamane 2019-09-13 10:04:27 UTC
(In reply to Julien Nabet from comment #10)
> there's no driver in connectivity which implements this.
> But let's say Firebird would be the first one.

Oh. It definitely works with other drivers. It must be implemented by some wrapper code then. Just don't do it, finish the XRowUpdate part, and I guess it will just work like in the other drivers.
Comment 12 Lionel Elie Mamane 2019-09-13 10:06:22 UTC
The said wrapper code seems to be in dbaccess/source/core/api/datacolumn.cxx and does exactly what I suggested (call XRowUpdate with the column number filled in, passing the value).
Comment 13 Julien Nabet 2019-09-13 10:08:47 UTC
(In reply to Lionel Elie Mamane from comment #9)
> It should be ThisDatabaseDocument everywhere, without ().
> 
> For the read-only thing, btomi96?

Typing this on windbg:
bp `resultset.cxx:987`
then launching the test again, Windbg stops.
Calling callstack gives:
dbalo!dbaccess::OResultSet::checkReadOnly+0x46
dbalo!dbaccess::OResultSet::updateString+0x70
mscx_uno!`anonymous namespace'::cpp_call+0xcf7
mscx_uno!unoInterfaceProxyDispatch+0x47e
reflectionlo!stoc_corefl::IdlInterfaceMethodImpl::invoke+0xbe2
sblo!SbUnoObject::Notify+0x12b3
svllo!SfxBroadcaster::Broadcast+0xb0
sblo!SbxVariable::Broadcast+0x1b5
sblo!SbxValue::SbxValue+0x1a3

982  void OResultSet::checkReadOnly() const
983  {
984      if  (   ( m_nResultSetConcurrency == ResultSetConcurrency::READ_ONLY )
985          ||  !m_xDelegatorResultSetUpdate.is()
986          )
987          throwSQLException( "The result set is read-only.", StandardSQLState::GENERAL_ERROR, *const_cast< OResultSet* >( this ) );
988  }
and indeed Firebird contains:
m_nResultSetConcurrency(css::sdbc::ResultSetConcurrency::READ_ONLY)
See https://opengrok.libreoffice.org/xref/core/connectivity/source/drivers/firebird/ResultSet.cxx?r=d4ef035e#68

I'll investigate this after mid-day break.
Comment 14 Lionel Elie Mamane 2019-09-13 10:34:47 UTC
So, I'm sitting with btomi96 right now... The firebird driver says it supports only read-only in ODatabaseMetaData::supportsResultSetConcurrency in DatabaseMetaData.cxx

That's why... That needs to be enlarged. What we should declare as supported exactly is not 100% clear at this point. For testing purposes (not ready to commit), just change this (and ODatabaseMetaData::supportsResultSetType above) to always return true.

And you'll also need to support/test record deletion and insertion, see the IDL interface, should be not too hard to write the test code for that.
Comment 15 Julien Nabet 2019-09-13 12:23:10 UTC
(In reply to Lionel Elie Mamane from comment #14)
> So, I'm sitting with btomi96 right now... The firebird driver says it
> supports only read-only in ODatabaseMetaData::supportsResultSetConcurrency
> in DatabaseMetaData.cxx
> 
> That's why... That needs to be enlarged. What we should declare as supported
> exactly is not 100% clear at this point. For testing purposes (not ready to
> commit), just change this (and ODatabaseMetaData::supportsResultSetType
> above) to always return true.
> 
> And you'll also need to support/test record deletion and insertion, see the
> IDL interface, should be not too hard to write the test code for that.

I made the changes so:
- ODatabaseMetaData::supportsResultSetConcurrency always returns true
- ODatabaseMetaData::supportsResultSetType always returns true

+ 
- ODatabaseMetaData::isReadOnly always returns false
- OResultSet::OResultSet contains:
 m_nResultSetConcurrency(css::sdbc::ResultSetConcurrency::UPDATABLE)
instead of 
 m_nResultSetConcurrency(css::sdbc::ResultSetConcurrency::READ_ONLY)

Still the same error ("The result set is read-only") :-(
Comment 16 Julien Nabet 2019-09-13 12:24:13 UTC
About "support record deletion and insertion, see the IDL interface", is it related to XRowUpdate interface?
Comment 17 Julien Nabet 2019-09-13 15:32:27 UTC
After having waited for the whole build whereas I just added some traces on dbaccess part, the pb is m_xDelegatorResultSetUpdate isn't defined.
Here are the console logs:
m_nResultSetConcurrency == ResultSetConcurrency::READ_ONLY=0
!m_xDelegatorResultSetUpdate.is()=1

Opengroking "m_xDelegatorResultSetUpdate" gives:
https://opengrok.libreoffice.org/search?project=core&full=m_xDelegatorResultSetUpdate&defs=&refs=&path=&hist=&type=&si=full

m_xDelegatorResultSetUpdate.set(m_xDelegatorResultSet, css::uno::UNO_QUERY);
in the ctr.

m_xDelegatorResultSet is initialized some lines above, still in ctr:
m_xDelegatorResultSet(_xResultSet)

Then I'm a bit stuck.
Comment 18 Julien Nabet 2019-09-13 20:52:02 UTC
It seems we need to implement com/sun/star/sdbc/XResultSetUpdate.hpp

I'll keep on this lead.
Comment 19 Julien Nabet 2019-09-13 21:11:33 UTC
With:
 m_nResultSetConcurrency(css::sdbc::ResultSetConcurrency::UPDATABLE)
instead of 
 m_nResultSetConcurrency(css::sdbc::ResultSetConcurrency::READ_ONLY)

+ empty implementation of com/sun/star/sdbc/XResultSetUpdate.hpp, I don't have the error message.
The pb now is what to put in these new methods...
Comment 20 Julien Nabet 2019-09-16 13:57:50 UTC
(In reply to Lionel Elie Mamane from comment #14)
> So, I'm sitting with btomi96 right now... The firebird driver says it
> supports only read-only in ODatabaseMetaData::supportsResultSetConcurrency
> in DatabaseMetaData.cxx
> ...

Any progress here?
AFAIC, following my last comment, I'm stuck for real implementation of XResultSetUpdate.hpp. Some hints would help me to keep on.
Or perhaps should I abandon the patch for the moment? eg: to wait for Firebird 4.0 release + integration in LO