Members of the KDE Community are recommended to subscribe to the kde-community mailing list at https://mail.kde.org/mailman/listinfo/kde-community to allow them to participate in important discussions and receive other important announcements

Commit 25759819 authored by David Faure's avatar David Faure

ItemSync: skip handling remote items if local changes failed

Summary:
The infamous "Multiple merge candidates" error would still leave ItemSync
in a forever-stuck state when mRemoteItemsQueue was not empty.

Reproduced by adding a unittest that calls setFullSyncItems (with
a duplicate item), while the existing unittest for a duplicate item
was callling setIncrementalSyncItems().

I barely understand what I'm doing, please review carefully.

Test Plan: Unittest passes.

Reviewers: dvratil, vkrause

Reviewed By: dvratil

Subscribers: asn, kde-pim

Tags: #kde_pim

Differential Revision: https://phabricator.kde.org/D20243
parent ec753bd5
......@@ -61,7 +61,7 @@ private:
return fetch->items();
}
void createItems(const Collection &col, int itemCount)
static void createItems(const Collection &col, int itemCount)
{
for (int i = 0; i < itemCount; ++i) {
Item item(QStringLiteral("application/octet-stream"));
......@@ -73,14 +73,13 @@ private:
}
}
private Q_SLOTS:
void initTestCase()
static Item duplicateItem(const Item &item, const Collection &col)
{
AkonadiTest::checkTestIsIsolated();
Control::start();
AkonadiTest::setAllResourcesOffline();
qRegisterMetaType<KJob *>();
qRegisterMetaType<ItemSync::TransactionMode>();
Item duplicate = item;
duplicate.setId(-1);
ItemCreateJob *job = new ItemCreateJob(duplicate, col);
[job]() { AKVERIFYEXEC(job); }();
return job->item();
}
static Item modifyItem(Item item)
......@@ -91,6 +90,16 @@ private Q_SLOTS:
return item;
}
private Q_SLOTS:
void initTestCase()
{
AkonadiTest::checkTestIsIsolated();
Control::start();
AkonadiTest::setAllResourcesOffline();
qRegisterMetaType<KJob *>();
qRegisterMetaType<ItemSync::TransactionMode>();
}
void testFullSync()
{
const Collection col = Collection(collectionIdFromPath(QStringLiteral("res1/foo")));
......@@ -542,12 +551,7 @@ private Q_SLOTS:
Item::List origItems = fetchItems(col);
//Create a duplicate that will trigger an error during the first batch
Item duplicate = origItems.first();
duplicate.setId(-1);
{
ItemCreateJob *job = new ItemCreateJob(duplicate, col);
AKVERIFYEXEC(job);
}
Item dupe = duplicateItem(origItems.at(0), col);
origItems = fetchItems(col);
ItemSync *syncer = new ItemSync(col);
......@@ -587,6 +591,48 @@ private Q_SLOTS:
syncer->deliveryDone();
QTRY_COMPARE(spy.count(), 1);
// cleanup
ItemDeleteJob *del = new ItemDeleteJob(dupe, this);
AKVERIFYEXEC(del);
}
void testFullSyncFailingDueToDuplicateItem()
{
const Collection col = Collection(collectionIdFromPath(QStringLiteral("res1/foo")));
QVERIFY(col.isValid());
Item::List origItems = fetchItems(col);
//Create a duplicate that will trigger an error during the first batch
Item dupe = duplicateItem(origItems.at(0), col);
origItems = fetchItems(col);
Akonadi::Monitor monitor;
monitor.setCollectionMonitored(col);
QSignalSpy deletedSpy(&monitor, SIGNAL(itemRemoved(Akonadi::Item)));
QVERIFY(deletedSpy.isValid());
QSignalSpy addedSpy(&monitor, SIGNAL(itemAdded(Akonadi::Item,Akonadi::Collection)));
QVERIFY(addedSpy.isValid());
QSignalSpy changedSpy(&monitor, SIGNAL(itemChanged(Akonadi::Item,QSet<QByteArray>)));
QVERIFY(changedSpy.isValid());
ItemSync *syncer = new ItemSync(col);
syncer->setTransactionMode(ItemSync::SingleTransaction);
QSignalSpy transactionSpy(syncer, SIGNAL(transactionCommitted()));
QVERIFY(transactionSpy.isValid());
syncer->setFullSyncItems(origItems);
QVERIFY(!syncer->exec());
QCOMPARE(transactionSpy.count(), 1);
Item::List resultItems = fetchItems(col);
QCOMPARE(resultItems.count(), origItems.count());
QTest::qWait(100);
QCOMPARE(deletedSpy.count(), 1); // ## is this correct?
QCOMPARE(addedSpy.count(), 1); // ## is this correct?
QCOMPARE(changedSpy.count(), 0);
// cleanup
ItemDeleteJob *del = new ItemDeleteJob(dupe, this);
AKVERIFYEXEC(del);
}
void testFullSyncManyItems()
......
......@@ -157,6 +157,8 @@ void ItemSyncPrivate::checkDone()
//and wait until the transaction is committed to process the next batch
if (mTransactionMode == ItemSync::MultipleTransactions || (mDeliveryDone && mRemoteItemQueue.isEmpty())) {
if (mCurrentTransaction) {
// Note that mCurrentTransaction->commit() is a no-op if we're already rolling back
// so this signal is a bit misleading (but it's only used by unittests it seems)
Q_EMIT q->transactionCommitted();
mCurrentTransaction->commit();
mCurrentTransaction = nullptr;
......@@ -454,6 +456,7 @@ void ItemSyncPrivate::slotLocalChangeDone(KJob *job)
{
if (job->error() && job->error() != Job::KilledJobError) {
qCWarning(AKONADICORE_LOG) << "Creating/updating items from the akonadi database failed:" << job->errorString();
mRemoteItemQueue.clear(); // don't try to process any more items after a rollback
}
mPendingJobs--;
mProgress++;
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment