Differential D24563

Standardize the database upgrade mechanism
ClosedPublic
Actions

Authored by jguidon on Oct 11 2019, 4:20 PM.

Details

Reviewers
mgallien
ngraham
astippich
Group Reviewers
Elisa
Commits
R255:977c002b6f43: Standardize the database upgrade mechanism
Summary

This patch is aim to standardize the upgrade mechanism which is currently in the method DatabaseInterface::initDatabase.
The idea is to avoid taking care about the upgrade logic when creating new versions.

On a new version XX, the steps would be the following:

1-create the method DatabaseInterface::upgradeDatabaseVXX
2-add the version to the enum DatabaseInterface::DatabaseVersion
3-add upgradeDatabaseVXX to the switch-cas in callUpgradeFunctionForVersion

Added mechanism for upgrade the database
Add integration for versions V9 -> V14
Added drop request for the unused tables

Diff Detail

Repository
R255 Elisa
Branch
databaseUpdate
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 17573
Build 17591: arc lint + arc unit
jguidon requested review of this revision.Oct 11 2019, 4:20 PM
jguidon created this revision.
jguidon updated this revision to Diff 67722.Oct 11 2019, 4:22 PM

Fix qcDebug messages

mgallien added a comment.Oct 26 2019, 10:26 AM

Sorry for the delay. I will review it beginning of next week.

mgallien added a comment.Nov 2 2019, 8:41 PM

When running the automatic tests I get the following error:

QDEBUG : DatabaseInterfaceTests::reloadDatabaseWithAllTracks() org.kde.elisa.database: database open
QDEBUG : DatabaseInterfaceTests::reloadDatabaseWithAllTracks() org.kde.elisa.database: DatabaseInterface::init yes
QDEBUG : DatabaseInterfaceTests::reloadDatabaseWithAllTracks() org.kde.elisa.database: DatabaseInterface::setDatabaseVersionInTable "INSERT INTO DatabaseVersion (Version) VALUES ( :version )"
QDEBUG : DatabaseInterfaceTests::reloadDatabaseWithAllTracks() org.kde.elisa.database: DatabaseInterface::setDatabaseVersionInTable QSqlError("19", "Unable to fetch row", "UNIQUE constraint failed: DatabaseVersion.Version")
QDEBUG : DatabaseInterfaceTests::reloadDatabaseWithAllTracks() org.kde.elisa.database: DatabaseInterface::reloadExistingDatabase
FAIL! : DatabaseInterfaceTests::reloadDatabaseWithAllTracks() Compared values are not the same

Actual   (musicDbDatabaseErrorSpy2.count()): 1
Expected (0)                               : 0
Loc: [../autotests/databaseinterfacetest.cpp(3540)]

I am running the tests with the environment variable :
export QT_LOGGING_RULES="*elisa*=true"

src/databaseinterface.cpp
3099

Some spaces are missing around the - operator and after the if

3114

Why the double { ?

mgallien added a comment.Nov 3 2019, 8:44 AM
In D24563#557946, @mgallien wrote:

When running the automatic tests I get the following error:

QDEBUG : DatabaseInterfaceTests::reloadDatabaseWithAllTracks() org.kde.elisa.database: database open
QDEBUG : DatabaseInterfaceTests::reloadDatabaseWithAllTracks() org.kde.elisa.database: DatabaseInterface::init yes
QDEBUG : DatabaseInterfaceTests::reloadDatabaseWithAllTracks() org.kde.elisa.database: DatabaseInterface::setDatabaseVersionInTable "INSERT INTO DatabaseVersion (Version) VALUES ( :version )"
QDEBUG : DatabaseInterfaceTests::reloadDatabaseWithAllTracks() org.kde.elisa.database: DatabaseInterface::setDatabaseVersionInTable QSqlError("19", "Unable to fetch row", "UNIQUE constraint failed: DatabaseVersion.Version")
QDEBUG : DatabaseInterfaceTests::reloadDatabaseWithAllTracks() org.kde.elisa.database: DatabaseInterface::reloadExistingDatabase
FAIL! : DatabaseInterfaceTests::reloadDatabaseWithAllTracks() Compared values are not the same

Actual   (musicDbDatabaseErrorSpy2.count()): 1
Expected (0)                               : 0
Loc: [../autotests/databaseinterfacetest.cpp(3540)]

I am running the tests with the environment variable :
export QT_LOGGING_RULES="*elisa*=true"

I forgot to say thank you for your work.

Please have a look at the failing automatic tests and then we can integrate that.

jguidon added a comment.Nov 13 2019, 9:57 PM

Thanks a lot for your feedback.

I can reproduce the errors and I am working on fixing this.

jguidon updated this revision to Diff 69819.Nov 15 2019, 6:36 PM

Fix tests on DatabaseVersion table request

jguidon updated this revision to Diff 69824.Nov 15 2019, 9:03 PM

rebase on master

Running the tests, I still get an error but it is not dependent on the changes on this review. I ran also the tests on master branch in both cases I get:

QWARN  : DatabaseInterfaceTests::checkRestoredTracks() QSqlDatabasePrivate::addDatabase: duplicate connection name 'testDb', old connection removed.
QDEBUG : DatabaseInterfaceTests::checkRestoredTracks() org.kde.elisa.database: database open
QDEBUG : DatabaseInterfaceTests::checkRestoredTracks() org.kde.elisa.database: DatabaseInterface::init yes
QINFO  : DatabaseInterfaceTests::checkRestoredTracks() org.kde.elisa.database: begin creation of DatabaseVersion table
QINFO  : DatabaseInterfaceTests::checkRestoredTracks() org.kde.elisa.database: begin creation of v9 database schema
QINFO  : DatabaseInterfaceTests::checkRestoredTracks() org.kde.elisa.database: end creation of v9 database schema
QINFO  : DatabaseInterfaceTests::checkRestoredTracks() org.kde.elisa.database: begin update to v11 of database schema
QINFO  : DatabaseInterfaceTests::checkRestoredTracks() org.kde.elisa.database: finished update to v11 of database schema
QINFO  : DatabaseInterfaceTests::checkRestoredTracks() org.kde.elisa.database: begin update to v12 of database schema
QINFO  : DatabaseInterfaceTests::checkRestoredTracks() org.kde.elisa.database: finished update to v12 of database schema
QINFO  : DatabaseInterfaceTests::checkRestoredTracks() org.kde.elisa.database: begin update to v13 of database schema
QINFO  : DatabaseInterfaceTests::checkRestoredTracks() org.kde.elisa.database: finished update to v13 of database schema
QINFO  : DatabaseInterfaceTests::checkRestoredTracks() org.kde.elisa.database: begin update to v14 of database schema
QINFO  : DatabaseInterfaceTests::checkRestoredTracks() org.kde.elisa.database: finished update to v14 of database schema
QDEBUG : DatabaseInterfaceTests::checkRestoredTracks() org.kde.elisa.database: DatabaseInterface::insertTracksList 23
QSYSTEM: DatabaseInterfaceTests::checkRestoredTracks() Maximum amount of warnings exceeded. Use -maxwarnings to override.
PASS   : DatabaseInterfaceTests::checkRestoredTracks()
PASS   : DatabaseInterfaceTests::addOneTrackWithParticularPath()
PASS   : DatabaseInterfaceTests::readRecentlyPlayedTracksData()
PASS   : DatabaseInterfaceTests::readFrequentlyPlayedTracksData()
PASS   : DatabaseInterfaceTests::readAllGenresData()
PASS   : DatabaseInterfaceTests::clearDataTest()
FAIL!  : DatabaseInterfaceTests::upgradeFromStableVersion() Compared values are not the same
   Actual   (musicDb.allAlbumsData().count()): 0
   Expected (5)                              : 5
   Loc: [/home/jeje/kde/elisa/autotests/databaseinterfacetest.cpp(4848)]
jguidon updated this revision to Diff 69825.Nov 15 2019, 9:09 PM

Fixed code style

jguidon marked 2 inline comments as done.Nov 15 2019, 9:11 PM
src/databaseinterface.cpp
3114

It remained after some refactoring, thanks :)

mgallien accepted this revision.Nov 16 2019, 5:27 PM
In D24563#563126, @jguidon wrote:

rebase on master

Running the tests, I still get an error but it is not dependent on the changes on this review. I ran also the tests on master branch in both cases I get:

QWARN  : DatabaseInterfaceTests::checkRestoredTracks() QSqlDatabasePrivate::addDatabase: duplicate connection name 'testDb', old connection removed.
QDEBUG : DatabaseInterfaceTests::checkRestoredTracks() org.kde.elisa.database: database open
QDEBUG : DatabaseInterfaceTests::checkRestoredTracks() org.kde.elisa.database: DatabaseInterface::init yes
QINFO  : DatabaseInterfaceTests::checkRestoredTracks() org.kde.elisa.database: begin creation of DatabaseVersion table
QINFO  : DatabaseInterfaceTests::checkRestoredTracks() org.kde.elisa.database: begin creation of v9 database schema
QINFO  : DatabaseInterfaceTests::checkRestoredTracks() org.kde.elisa.database: end creation of v9 database schema
QINFO  : DatabaseInterfaceTests::checkRestoredTracks() org.kde.elisa.database: begin update to v11 of database schema
QINFO  : DatabaseInterfaceTests::checkRestoredTracks() org.kde.elisa.database: finished update to v11 of database schema
QINFO  : DatabaseInterfaceTests::checkRestoredTracks() org.kde.elisa.database: begin update to v12 of database schema
QINFO  : DatabaseInterfaceTests::checkRestoredTracks() org.kde.elisa.database: finished update to v12 of database schema
QINFO  : DatabaseInterfaceTests::checkRestoredTracks() org.kde.elisa.database: begin update to v13 of database schema
QINFO  : DatabaseInterfaceTests::checkRestoredTracks() org.kde.elisa.database: finished update to v13 of database schema
QINFO  : DatabaseInterfaceTests::checkRestoredTracks() org.kde.elisa.database: begin update to v14 of database schema
QINFO  : DatabaseInterfaceTests::checkRestoredTracks() org.kde.elisa.database: finished update to v14 of database schema
QDEBUG : DatabaseInterfaceTests::checkRestoredTracks() org.kde.elisa.database: DatabaseInterface::insertTracksList 23
QSYSTEM: DatabaseInterfaceTests::checkRestoredTracks() Maximum amount of warnings exceeded. Use -maxwarnings to override.
PASS   : DatabaseInterfaceTests::checkRestoredTracks()
PASS   : DatabaseInterfaceTests::addOneTrackWithParticularPath()
PASS   : DatabaseInterfaceTests::readRecentlyPlayedTracksData()
PASS   : DatabaseInterfaceTests::readFrequentlyPlayedTracksData()
PASS   : DatabaseInterfaceTests::readAllGenresData()
PASS   : DatabaseInterfaceTests::clearDataTest()
FAIL!  : DatabaseInterfaceTests::upgradeFromStableVersion() Compared values are not the same
   Actual   (musicDb.allAlbumsData().count()): 0
   Expected (5)                              : 5
   Loc: [/home/jeje/kde/elisa/autotests/databaseinterfacetest.cpp(4848)]

I do not reproduce this problem. So from my point of view, all tests are fine and this can land.

This revision is now accepted and ready to land.Nov 16 2019, 5:27 PM
ngraham added a comment.Dec 5 2019, 8:57 PM

Can this land now?

mgallien added a comment.Dec 5 2019, 10:00 PM
In D24563#572849, @ngraham wrote:

Can this land now?

From my point of view, yes!
I am away from my laptop, can you land it?

ngraham added a comment.Dec 5 2019, 10:06 PM

Sure thing!

ngraham closed this revision.Dec 5 2019, 10:20 PM
ngraham added a comment.Dec 5 2019, 10:20 PM

Thanks so much for this very nice contribution, @jguidon!

jguidon marked an inline comment as done.Dec 5 2019, 10:42 PM

Thanks :)

mgallien added a comment.Dec 6 2019, 5:51 PM
In D24563#572927, @ngraham wrote:

Thanks so much for this very nice contribution, @jguidon!

I would like to ask something. I am sorry to have missed the fact that it was not landed. Please next time ask me to do my job ?

Thanks for your nice contribution. @jguidon

ngraham added a comment.Dec 6 2019, 5:55 PM

No worries, we all miss things. :)

Revision Contents
Changeset List

PathPackages
Msrc/databaseinterface.h (26 lines)
Msrc/databaseinterface.cpp (226 lines)

Diff 67722

View Options

src/databaseinterface.h

Loading...
View Options

src/databaseinterface.cpp

Loading...