Inconsistent behavior of QSqlTableModel with OnRowSubmit

Question

^{Premise: this question possibly refers to two distinct problems, but I believe they might be linked. If, after comments and further research we will find out that they are actually unrelated, I will open a separate question.}

I'm experiencing some unexpected and odd behavior with some aspects of QSqlTableModel, and with subclassing in at least one case. I'm not an expert on Sql, but one of the problems doesn't seem what the expected behavior should be.

I can confirm this only for SQLite as I don't use other database systems.
I can also reproduce these problems with both [Py]Qt 5.15.2 and 6.2.2.

1. New row is "removed" after ignoring editor changes

With the default OnRowChange edit strategy, if a row is added, some data is inserted in a field, and editing of another field on the same row is cancelled using Esc, the whole row is then removed from the view.

The actual database, though, is still updated, and opening the program again shows the row that was previously "hidden", except for the field that has been cancelled.

from PyQt5 import QtWidgets, QtSql

class TestModel(QtSql.QSqlTableModel):
    def __init__(self):
        super().__init__()
        QtSql.QSqlQuery().exec(
            'CREATE TABLE IF NOT EXISTS test (name, value, data);')
        self.setTable('test')
        self.select()


app = QtWidgets.QApplication([])

db = QtSql.QSqlDatabase.addDatabase('QSQLITE')
db.setDatabaseName('test.db')
db.open()

win = QtWidgets.QWidget()
layout = QtWidgets.QVBoxLayout(win)
addButton = QtWidgets.QPushButton('Add row')
layout.addWidget(addButton)
table = QtWidgets.QTableView()
layout.addWidget(table)
model = TestModel()
table.setModel(model)

addButton.clicked.connect(lambda: model.insertRow(model.rowCount()))
app.aboutToQuit.connect(model.submitAll)

win.resize(640, 480)
win.show()
app.exec()

These are the steps to reproduce the problem:

1. add a row with the button;
2. edit at least one field, but not all fields;
3. start editing an empty field;
4. press Esc;
5. close and restart the program;

After step 4, you'll see that the added row is removed from the view, which is not completely unexpected: since the strategy is OnRowChange, cancelling reverts all cached changes (including insertRow()); I don't completely agree with the behavior (imagine filling dozens of fields and then hitting Esc by mistake), but that's not the point.
What's unexpected is that the model is actually updated with the new row and all fields that have been submitted before hitting Esc, and restarting the program will show that.

2. Implementing data() reverts to previous data for incomplete records

Editing an index that has empty (NULL) fields for its row brings different results whether data() has been implemented or not in the subclass, even if the override just calls the base implementation.

Add the following to the TestModel class above:

    def data(self, index, role=QtCore.Qt.DisplayRole):
        return super().data(index, role)

And a submit button before app.exec():

submitButton = QtWidgets.QPushButton('Submit')
layout.addWidget(submitButton)
submitButton.clicked.connect(model.submitAll)

To reproduce the problem follow these steps:

1. open a database with at least one row with an empty field at the bottom, similarly to what done above (note: with "empty field" I mean an item that has never been edited);
2. edit any field in that row and press Enter;

With the OnRowChange or OnFieldChange strategy, the result is that the whole row is made invalid: the vertical header shows "!" (a hint for an invalid record) and all fields are cleared, including those that have previous value from the database.

When the edit strategy is set to OnManualSubmit, calling submitAll() will revert to the original values of the database, just like as changes have been reverted.

The behavior is slightly different if the row with the empty field is not at the bottom; do the first two steps above, then:

3. press the submit button;
4. close and restart the program;

In this case, after step 3 the view seem to have accepted the changes, but restarting the program shows that no modification has been applied.

---

Depending on the edit strategy and the situation, the behavior changes. Usually, if a record with an empty field is followed by at least a record with all fields set, the view and model behave as expected when cancelling editing of that field.
In at least one case it was even impossible to edit an empty field at all (I've to admit, I did many random/speed tests and when I found out that I wasn't able to edit a field I couldn't remember the steps to reproduce it).

What's also strange is that both setData() and submitAll() return True, and there is no explicit lastError(). Despite of that, the shown (and stored) data reverts to the previous database content.

I believe that both issues are potentially caused by a common bug, but, before submitting something to the Qt bug report system I'd like to have some feedback, especially from people being more experienced in SQL and other db drivers, in order to provide a better report (and eventually know if those issues are in fact related or not).

Answer 1

Both issues are caused by bugs in Qt, but they aren't related.

Before explaining these issues, some clarification of the symbols used in the vertical header may be helpful, because they provide some important clues regarding the source of the problems. The symbols are documented thus:

If you insert rows programmatically using QSqlTableModel::insertRows(), the new rows will be marked with an asterisk (*) until they are submitted using submitAll() or automatically when the user moves to another record (assuming the edit strategy is QSqlTableModel::OnRowChange). Likewise, if you remove rows using removeRows(), the rows will be marked with an exclamation mark (!) until the change is submitted.

The first issue is caused by this sequence of events:

After pressing Esc whilst editing a new row (i.e. * is shown in the vertical header), the delegate will emit closeEditor with the RevertModelCache hint. This calls the closeEditor slot of the view, which in turn calls revert() on the table-model - and also, ultimately, the private revertCachedRow function. This function calls beginRemoveRows - but crucially before clearing the cache. Next, rowsAboutToBeRemoved is emitted, which removes the row from the view, causing currentRowChanged to be emitted, which in turn calls the submit() slot of the table-model. Oops! The still uncleared cache data is now inadvertently committed to the database, before endRemoveRows is called after the cache data is finally removed. So, in short, the bug here is that there is no guard to stop submit() being called during the execution of revert().

The second issue is much more subtle. The problem occurs because the SQL table is created without a primary key and the columns do not have an explicit type. This is all perfectly valid, but it exposes a critical bug in a small section of Qt code that builds SQL statements.

This happens in QSqlTableModel::selectRow, which needs to build a where-clause from the QSqlRecord returned by primaryValues. The sqlStatement function of the database driver is used for this, but that needs to know the exact type of the field values in order to quote them correctly. However, the table-model cache does not ensure that a sensible default type is used for columns without an explicit type. This means untyped values will pass through unquoted, allowing arbitrary SQL expressions to be evaluated whilst editing the table. Oops!

It's this that can sometimes make the bug hard to reproduce, because the exact behaviour depends on the precise values that are entered. A value like foo will cause an SQL error, because it's a valid column name that doesn't exist; yet a value like 6 won't raise an error, but will wrongly fail to return any rows, due to a type-mismatch (i.e. INT vs TEXT). If selectRow can't find the relevant row, it may call cache.refresh(), which will clear the values and mark the row for deletion (hence the ! shown in the vertical header). Note also that QSqlQuery is used to execute the problematic statement, so any errors will pass silently and won't be available via the database or driver.

I have provided a re-write below of the original example with some fixes that can be switched on via the command-line (1 to fix the first issue, 2 to fix the second, and 3 to fix both). These are mainly meant for debugging, but could also be adapted as work-arounds if required. The second fix is rather hackish (because primaryValues can't be reimplemented in PyQt) - but it's only needed if you don't have control over the database schema. If the table has a typed primary key and/or all the columns have an explicit type, the second issue won't occur at all. Hopefully the output from the script should make it clear what is going on.

PyQt5:

import sys
from PyQt5 import QtCore, QtWidgets, QtSql

BUGFIX = int(sys.argv[1]) if len(sys.argv) > 1 else 0

class TestModel(QtSql.QSqlTableModel):
    def __init__(self):
        super().__init__()
        self._select_row = None
        self._reverting = False
        QtSql.QSqlQuery().exec(
            'CREATE TABLE IF NOT EXISTS test (name, value, data);')
        self.setTable('test')
        self.select()

    def selectRow(self, row):
        if BUGFIX & 2:
            self._select_row = row
        result = super().selectRow(row)
        print(f'selectRow: {result}')
        return result

    def select(self):
        return super().select() if self._select_row is None else False

    def selectStatement(self):
        if self._select_row is not None:
            record = self.primaryValues(self._select_row)
            for index in range(record.count()):
                field = record.field(index)
                if (not field.isNull() and
                    field.type() == QtCore.QVariant.Invalid):
                    field.setType(QtCore.QVariant.String)
                    record.replace(index, field)
            where = self.database().driver().sqlStatement(
                QtSql.QSqlDriver.WhereStatement,
                self.tableName(), record, False)
            if where[:6].upper() == 'WHERE ':
                where = where[6:]
            self.setFilter(where)
            self._select_row = None
        statement = super().selectStatement()
        print(f'selectStatement: {statement!r}')
        query = self.database().exec(statement)
        if query.lastError().isValid():
            print(f'  query-lastError: {query.lastError().text()!r}')
        else:
            print(f'  query-next: {query.next()}')
        return statement

    def revert(self):
        if BUGFIX & 1:
            self._reverting = True
        print('reverting ...')
        super().revert()
        self._reverting = False
        print('reverted')

    def submit(self):
        print('submitting ...')
        result = False if self._reverting else super().submit()
        print(f'submitted: {result}')
        return result

app = QtWidgets.QApplication(['Test'])

db = QtSql.QSqlDatabase.addDatabase('QSQLITE')
db.setDatabaseName('test.db')
db.open()

win = QtWidgets.QWidget()
layout = QtWidgets.QVBoxLayout(win)
addButton = QtWidgets.QPushButton('Add row')
layout.addWidget(addButton)
table = QtWidgets.QTableView()
layout.addWidget(table)
model = TestModel()
table.setModel(model)
submitButton = QtWidgets.QPushButton('Submit')
layout.addWidget(submitButton)
submitButton.clicked.connect(model.submitAll)
addButton.clicked.connect(lambda: model.insertRow(model.rowCount()))
app.aboutToQuit.connect(model.submitAll)

win.setGeometry(1000, 50, 640, 480)
win.show()
app.exec()

PyQt6:

import sys
from PyQt6 import QtCore, QtWidgets, QtSql

BUGFIX = int(sys.argv[1]) if len(sys.argv) > 1 else 0

class TestModel(QtSql.QSqlTableModel):
    def __init__(self):
        super().__init__()
        self._select_row = None
        self._reverting = False
        QtSql.QSqlQuery().exec(
            'CREATE TABLE IF NOT EXISTS test (name, value, data);')
        self.setTable('test')
        self.select()

    def selectRow(self, row):
        if BUGFIX & 2:
            self._select_row = row
        result = super().selectRow(row)
        print(f'selectRow: {result}')
        return result

    def select(self):
        return super().select() if self._select_row is None else False

    def selectStatement(self):
        if self._select_row is not None:
            record = self.primaryValues(self._select_row)
            MetaType = QtCore.QMetaType.Type
            MetaString = QtCore.QMetaType(MetaType.QString.value)
            for index in range(record.count()):
                field = record.field(index)
                if (not field.isNull() and
                    field.metaType().id() == MetaType.UnknownType.value):
                    field.setMetaType(MetaString)
                    record.replace(index, field)
            where = self.database().driver().sqlStatement(
                QtSql.QSqlDriver.StatementType.WhereStatement,
                self.tableName(), record, False)
            if where[:6].upper() == 'WHERE ':
                where = where[6:]
            self.setFilter(where)
            self._select_row = None
        statement = super().selectStatement()
        print(f'selectStatement: {statement!r}')
        query = self.database().exec(statement)
        if query.lastError().isValid():
            print(f'  query-lastError: {query.lastError().text()!r}')
        else:
            print(f'  query-next: {query.next()}')
        return statement

    def revert(self):
        if BUGFIX & 1:
            self._reverting = True
        print('reverting ...')
        super().revert()
        self._reverting = False
        print('reverted')

    def submit(self):
        print('submitting ...')
        result = False if self._reverting else super().submit()
        print(f'submitted: {result}')
        return result

app = QtWidgets.QApplication(['Test'])

db = QtSql.QSqlDatabase.addDatabase('QSQLITE')
db.setDatabaseName('test.db')
db.open()

win = QtWidgets.QWidget()
layout = QtWidgets.QVBoxLayout(win)
addButton = QtWidgets.QPushButton('Add row')
layout.addWidget(addButton)
table = QtWidgets.QTableView()
layout.addWidget(table)
model = TestModel()
table.setModel(model)
submitButton = QtWidgets.QPushButton('Submit')
layout.addWidget(submitButton)
submitButton.clicked.connect(model.submitAll)
addButton.clicked.connect(lambda: model.insertRow(model.rowCount()))
app.aboutToQuit.connect(model.submitAll)

win.setGeometry(1000, 50, 640, 480)
win.show()
app.exec()