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?

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?

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.

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

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?

Did you put the macro in the ODB file, not in a more general (e.g. user-wide) location?

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.

(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())

It should be ThisDatabaseDocument everywhere, without ().

For the read-only thing, btomi96?

(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! :-)).

(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.

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).

(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.

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.

(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") :-(

About "support record deletion and insertion, see the IDL interface", is it related to XRowUpdate interface?

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.

It seems we need to implement com/sun/star/sdbc/XResultSetUpdate.hpp

I'll keep on this lead.

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...

(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

Dear Lionel Elie Mamane,

To make sure we're focusing on the bugs that affect our users today, LibreOffice QA is asking bug reporters and confirmers to retest open, confirmed bugs which have not been touched for over a year.

There have been thousands of bug fixes and commits since anyone checked on this bug report. During that time, it's possible that the bug has been fixed, or the details of the problem have changed. We'd really appreciate your help in getting confirmation that the bug is still present.

If you have time, please do the following:

Test to see if the bug is still present with the latest version of LibreOffice from https://www.libreoffice.org/download/

If the bug is present, please leave a comment that includes the information from Help - About LibreOffice.
 
If the bug is NOT present, please set the bug's Status field to RESOLVED-WORKSFORME and leave a comment that includes the information from Help - About LibreOffice.

Please DO NOT

Update the version field
Reply via email (please reply directly on the bug tracker)
Set the bug's Status field to RESOLVED - FIXED (this status has a particular meaning that is not 
appropriate in this case)


If you want to do more to help you can test to see if your issue is a REGRESSION. To do so:
1. Download and install oldest version of LibreOffice (usually 3.3 unless your bug pertains to a feature added after 3.3) from https://downloadarchive.documentfoundation.org/libreoffice/old/

2. Test your bug
3. Leave a comment with your results.
4a. If the bug was present with 3.3 - set version to 'inherited from OOo';
4b. If the bug was not present in 3.3 - add 'regression' to keyword


Feel free to come ask questions or to say hello in our QA chat: https://web.libera.chat/?settings=#libreoffice-qa

Thank you for helping us make LibreOffice even better for everyone!

Warm Regards,
QA Team

MassPing-UntouchedBug