Fixes for start-up crashes

CHANGES.markdown

@@ -13,6 +13,19 @@ happens, so I'm now setting it to a slightly arbitrary time early in the morning
 * Additional methods for calculating IBU
 * We'll list other new features here...
 
+
+## v4.0.4
+Minor bug fixes for the 4.0.4 release (ie bugs in 4.0.4 are fixed in this 4.0.5 release).
+
+### New Features
+* None
+
+### Bug Fixes
+* Crash on new ingredient import (brewtarget v4.0.4) at startup time [828](https://github.com/Brewtarget/brewtarget/issues/828)
+
+### Release Timestamp
+Mon, 23 Sep 2024 04:00:05 +0100
+
 ## v4.0.4
 Minor bug fixes for the 4.0.3 release (ie bugs in 4.0.3 are fixed in this 4.0.4 release).
 
@@ -26,7 +39,7 @@ Minor bug fixes for the 4.0.3 release (ie bugs in 4.0.3 are fixed in this 4.0.4
 * Edit Boil and Edit Fermentation buttons not working [826](https://github.com/Brewtarget/brewtarget/issues/826)
 
 ### Release Timestamp
-Wed, 28 Aug 2024 04:00:04 +0100
+Wed, 18 Sep 2024 04:00:04 +0100
 
 ## v4.0.3
 Minor bug fixes for the 4.0.2 release (ie bugs in 4.0.2 are fixed in this 4.0.3 release).

src/database/DatabaseSchemaHelper.cpp

@@ -2302,7 +2302,7 @@ bool DatabaseSchemaHelper::migrate(Database & database, int oldVersion, int newV
    // By the magic of RAII, this will abort if we exit this function (including by throwing an exception) without
    // having called dbTransaction.commit().  (It will also turn foreign keys back on either way -- whether the
    // transaction is committed or rolled back.)
-   DbTransaction dbTransaction{database, connection, DbTransaction::DISABLE_FOREIGN_KEYS};
+   DbTransaction dbTransaction{database, connection, "Migrate", DbTransaction::DISABLE_FOREIGN_KEYS};
 
    for ( ; oldVersion < newVersion && ret; ++oldVersion ) {
       ret &= migrateNext(database, oldVersion, connection);

src/database/DbTransaction.cpp

@@ -1,5 +1,5 @@
 /*╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌
- * database/DbTransaction.cpp is part of Brewtarget, and is copyright the following authors 2021-2022:
+ * database/DbTransaction.cpp is part of Brewtarget, and is copyright the following authors 2021-2024:
  *   • Matt Young <[email protected]>
  *
  * Brewtarget is free software: you can redistribute it and/or modify it under the terms of the GNU General Public License
@@ -19,11 +19,15 @@
 #include <QSqlError>
 
 #include "database/Database.h"
+#include "Logging.h"
 
-
-DbTransaction::DbTransaction(Database & database, QSqlDatabase & connection, DbTransaction::SpecialBehaviours specialBehaviours) :
+DbTransaction::DbTransaction(Database & database,
+                             QSqlDatabase & connection,
+                             QString const nameForLogging,
+                             DbTransaction::SpecialBehaviours specialBehaviours) :
    database{database},
    connection{connection},
+   nameForLogging{nameForLogging},
    committed{false},
    specialBehaviours{specialBehaviours} {
    // Note that, on SQLite at least, turning foreign keys on and off has to happen outside a transaction, so we have to
@@ -33,9 +37,12 @@ DbTransaction::DbTransaction(Database & database, QSqlDatabase & connection, DbT
    }
 
    bool succeeded = this->connection.transaction();
-   qDebug() << Q_FUNC_INFO << "Database transaction begin: " << (succeeded ? "succeeded" : "failed");
+   qDebug() <<
+      Q_FUNC_INFO << "Database transaction" << this->nameForLogging << "begin: " << (succeeded ? "succeeded" : "failed");
    if (!succeeded) {
-      qCritical() << Q_FUNC_INFO << "Unable to start database transaction:" << connection.lastError().text();
+      qCritical() <<
+         Q_FUNC_INFO << "Unable to start database transaction" << this->nameForLogging << ":" << connection.lastError().text();
+      qCritical().noquote() << Q_FUNC_INFO << Logging::getStackTrace();
    }
    return;
 }
@@ -44,9 +51,11 @@ DbTransaction::~DbTransaction() {
    qDebug() << Q_FUNC_INFO;
    if (!committed) {
       bool succeeded = this->connection.rollback();
-      qDebug() << Q_FUNC_INFO << "Database transaction rollback: " << (succeeded ? "succeeded" : "failed");
+      qDebug() <<
+         Q_FUNC_INFO << "Database transaction" << this->nameForLogging << "rollback: " << (succeeded ? "succeeded" : "failed");
       if (!succeeded) {
-         qCritical() << Q_FUNC_INFO << "Unable to rollback database transaction:" << connection.lastError().text();
+         qCritical() <<
+            Q_FUNC_INFO << "Unable to rollback database transaction" << this->nameForLogging << ":" << connection.lastError().text();
       }
    }
 
@@ -59,9 +68,11 @@ DbTransaction::~DbTransaction() {
 
 bool DbTransaction::commit() {
    this->committed = connection.commit();
-   qDebug() << Q_FUNC_INFO << "Database transaction commit: " << (this->committed ? "succeeded" : "failed");
+   qDebug() <<
+      Q_FUNC_INFO << "Database transaction" << this->nameForLogging << "commit: " << (this->committed ? "succeeded" : "failed");
    if (!this->committed) {
-      qCritical() << Q_FUNC_INFO << "Unable to commit database transaction:" << connection.lastError().text();
+      qCritical() <<
+         Q_FUNC_INFO << "Unable to commit database transaction" << this->nameForLogging << ":" << connection.lastError().text();
    }
    return this->committed;
 }

src/database/DbTransaction.h

@@ -1,5 +1,5 @@
 /*╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌
- * database/DbTransaction.h is part of Brewtarget, and is copyright the following authors 2021:
+ * database/DbTransaction.h is part of Brewtarget, and is copyright the following authors 2021-2024:
  *   • Matt Young <[email protected]>
  *
  * Brewtarget is free software: you can redistribute it and/or modify it under the terms of the GNU General Public License
@@ -34,7 +34,10 @@ class DbTransaction {
    /**
     * \brief Constructing a \c DbTransaction will start a DB transaction
     */
-   DbTransaction(Database & database, QSqlDatabase & connection, SpecialBehaviours specialBehaviours = NONE);
+   DbTransaction(Database & database,
+                 QSqlDatabase & connection,
+                 QString const nameForLogging = "???",
+                 SpecialBehaviours specialBehaviours = NONE);
 
    /**
     * \brief When a \c DbTransaction goes out of scope and its destructor is called, the transaction started in the
@@ -53,6 +56,9 @@ class DbTransaction {
    Database & database;
    // This is intended to be a short-lived object, so it's OK to store a reference to a QSqlDatabase object
    QSqlDatabase & connection;
+   // This is useful for diagnosing problems such as
+   // 'Unable to start database transaction: "cannot start a transaction within a transaction Unable to begin transaction"'
+   QString const nameForLogging;
    bool committed;
    int specialBehaviours;
 

src/database/ObjectStore.cpp

@@ -1409,7 +1409,9 @@ void ObjectStore::loadAll(Database * database) {
    //
    // .:TBD:. In theory we don't need a transaction if we're _only_ reading data...
    QSqlDatabase connection = this->pimpl->database->sqlDatabase();
-   DbTransaction dbTransaction{*this->pimpl->database, connection};
+   DbTransaction dbTransaction{*this->pimpl->database,
+                               connection,
+                               QString("Load All %1").arg(*this->pimpl->primaryTable.tableName)};
 
    //
    // Using QSqlTableModel would save us having to write a SELECT statement, however it is a bit hard to use it to
@@ -1710,7 +1712,9 @@ int ObjectStore::insert(std::shared_ptr<QObject> object) {
    // Start transaction
    // (By the magic of RAII, this will abort if we return from this function without calling dbTransaction.commit()
    QSqlDatabase connection = this->pimpl->database->sqlDatabase();
-   DbTransaction dbTransaction{*this->pimpl->database, connection};
+   DbTransaction dbTransaction{*this->pimpl->database,
+                               connection,
+                               QString("Insert %1").arg(*this->pimpl->primaryTable.tableName)};
 
    int primaryKey = this->pimpl->insertObjectInDb(connection, *object, false);
 
@@ -1749,7 +1753,9 @@ void ObjectStore::update(std::shared_ptr<QObject> object) {
    // Start transaction
    // (By the magic of RAII, this will abort if we return from this function without calling dbTransaction.commit()
    QSqlDatabase connection = this->pimpl->database->sqlDatabase();
-   DbTransaction dbTransaction{*this->pimpl->database, connection};
+   DbTransaction dbTransaction{*this->pimpl->database,
+                               connection,
+                               QString("Update %1").arg(*this->pimpl->primaryTable.tableName)};
 
    //
    // Construct the SQL, which will be of the form
@@ -1875,7 +1881,11 @@ void ObjectStore::updateProperty(QObject const & object, BtStringConst const & p
    // Start transaction
    // (By the magic of RAII, this will abort if we return from this function without calling dbTransaction.commit()
    QSqlDatabase connection = this->pimpl->database->sqlDatabase();
-   DbTransaction dbTransaction{*this->pimpl->database, connection};
+   DbTransaction dbTransaction{
+      *this->pimpl->database,
+      connection,
+      QString("Update property %1 on %2").arg(*propertyName).arg(*this->pimpl->primaryTable.tableName)
+   };
 
    if (!this->pimpl->updatePropertyInDb(connection, object, propertyName)) {
       // Something went wrong.  Bailing out here will abort the transaction and avoid sending the signal.
@@ -1918,7 +1928,9 @@ std::shared_ptr<QObject> ObjectStore::defaultHardDelete(int id) {
    qDebug() << Q_FUNC_INFO << "Hard delete" << this->pimpl->m_className << "#" << id;
    auto object = this->pimpl->allObjects.value(id);
    QSqlDatabase connection = this->pimpl->database->sqlDatabase();
-   DbTransaction dbTransaction{*this->pimpl->database, connection};
+   DbTransaction dbTransaction{*this->pimpl->database,
+                               connection,
+                               QString("Hard delete %1").arg(*this->pimpl->primaryTable.tableName)};
 
    // We'll use this in a couple of places below
    QVariant primaryKey{id};

src/database/ObjectStoreTyped.cpp

@@ -1041,7 +1041,7 @@ bool WriteAllObjectStoresToNewDb(Database & newDatabase, QSqlDatabase & connecti
    // having called dbTransaction.commit().  (It will also turn foreign keys back on either way -- whether the
    // transaction is committed or rolled back.)
    //
-   DbTransaction dbTransaction{newDatabase, connectionNew, DbTransaction::DISABLE_FOREIGN_KEYS};
+   DbTransaction dbTransaction{newDatabase, connectionNew, "Write All", DbTransaction::DISABLE_FOREIGN_KEYS};
 
    for (ObjectStore const * objectStore : getAllObjectStores()) {
       if (!objectStore->writeAllToNewDb(newDatabase, connectionNew)) {

src/model/Boil.cpp

@@ -77,6 +77,12 @@ Boil::Boil(NamedParameterBundle const & namedParameterBundle) :
    SET_REGULAR_FROM_NPB (m_description  , namedParameterBundle, PropertyNames::Boil::description  ),
    SET_REGULAR_FROM_NPB (m_notes        , namedParameterBundle, PropertyNames::Boil::notes        ),
    SET_REGULAR_FROM_NPB (m_preBoilSize_l, namedParameterBundle, PropertyNames::Boil::preBoilSize_l) {
+
+   // If we're being constructed from a BeerXML file, we use the property boilTime_mins for RECIPE > BOIL_TIME
+   if (namedParameterBundle.contains(PropertyNames::Boil::boilTime_mins)) {
+      double boilTime_mins{namedParameterBundle.val<double>(PropertyNames::Boil::boilTime_mins)};
+      this->setBoilTime_mins(boilTime_mins);
+   }
    return;
 }
 
@@ -170,6 +176,7 @@ void Boil::ensureStandardProfile() {
    if (boilSteps.size() == 0 || boilSteps.at(0)->startTemp_c().value_or(100.0) > Boil::minimumBoilTemperature_c) {
       // We need to add a ramp-up (aka pre-boil) step
       auto preBoil = std::make_shared<BoilStep>(tr("Pre-boil for %1").arg(recipe->name()));
+      preBoil->setDescription(tr("Automatically-generated pre-boil step for %1").arg(recipe->name()));
       // Get the starting temperature for the ramp-up from the end temperature of the mash
       double startingTemp = Boil::minimumBoilTemperature_c - 1.0;
       if (recipe->mash()) {
@@ -188,6 +195,7 @@ void Boil::ensureStandardProfile() {
    if (boilSteps.size() < 2 || boilSteps.at(1)->startTemp_c().value_or(0.0) < Boil::minimumBoilTemperature_c) {
       // We need to add a main (aka boil proper) step
       auto mainBoil = std::make_shared<BoilStep>(tr("Main boil for %1").arg(recipe->name()));
+      mainBoil->setDescription(tr("Automatically-generated boil proper step for %1").arg(recipe->name()));
       mainBoil->setStartTemp_c(100.0);
       mainBoil->setEndTemp_c(100.0);
       this->insertStep(mainBoil, 2);
@@ -196,6 +204,7 @@ void Boil::ensureStandardProfile() {
    if (boilSteps.size() < 3 || boilSteps.at(2)->endTemp_c().value_or(100.0) > Boil::minimumBoilTemperature_c) {
       // We need to add a post-boil step
       auto postBoil = std::make_shared<BoilStep>(tr("Post-boil for %1").arg(recipe->name()));
+      postBoil->setDescription(tr("Automatically-generated post-boil step for %1").arg(recipe->name()));
       double endingTemp = 30.0;
       if (recipe->fermentation()) {
          auto fs = recipe->fermentation()->fermentationSteps();