From 1d8851495bcfa6ff5d3fa35882b68fdf68b21a7f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Daniel=20Vr=C3=A1til?= <dvratil@kde.org>
Date: Thu, 21 Mar 2019 13:22:58 +0100
Subject: Fix a regression when updating attributes

This fixes a regression introduced in 3a062e6a and 6054e42d where some
attributes were not sent to the Akonadi server in update job even though
they were modified. This was due to a bad API design which returns a
non-const pointer to an attribute from a const method, so callers sometimes
modify the returned attribute on a const object. Since the method itself
is const though, it did not mark the attribute as modified.

Proper fix is to introduce a purely const and non-const overloads for
the attribute accessors, unfortunatelly this requires fixing a lot of our code
in many places first to not abuse the non-constness of the returned
attribute.

Note that since the code is in an inlined method, all clients should be
recompiled.

CCMAIL: faure@kde.org
---
 src/core/collection.h | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/src/core/collection.h b/src/core/collection.h
index 50c0926..b5a496c 100644
--- a/src/core/collection.h
+++ b/src/core/collection.h
@@ -584,14 +584,19 @@ inline T *Akonadi::Collection::attribute(Collection::CreateOption option)
 template <typename T>
 inline T *Akonadi::Collection::attribute() const
 {
-    const T dummy;
-    if (hasAttribute(dummy.type())) {
-        T *attr = dynamic_cast<T *>(attribute(dummy.type()));
+    const QByteArray type = T().type();
+    if (hasAttribute(type)) {
+        T *attr = dynamic_cast<T *>(attribute(type));
         if (attr) {
+            // FIXME: This method returns a non-const pointer, so callers may still modify the
+            // attribute. Unfortunately, just making this function return a const pointer and
+            // creating a non-const overload does not work, as many users of this function abuse the
+            // non-const pointer and modify the attribute even on a const object.
+            const_cast<Collection*>(this)->markAttributesChanged();
             return attr;
         }
         //reuse 5250
-        qWarning() << "Found attribute of unknown type" << dummy.type()
+        qWarning() << "Found attribute of unknown type" << type
                    << ". Did you forget to call AttributeFactory::registerAttribute()?";
     }
 
-- 
cgit v1.1

From 53ad3b6d73d92ea289cf0183c10e2b8a35c8127a Mon Sep 17 00:00:00 2001
From: David Faure <faure@kde.org>
Date: Thu, 21 Mar 2019 23:37:36 +0100
Subject: Fix collection detaching at the wrong time in attribute()

Summary:
Found in FatCRM where changes to collection attributes were not stored
anymore.

Test Plan:
New unittest to ensure that we get the attribute from the
detached collection, not from the original one.

Reviewers: dvratil

Reviewed By: dvratil

Subscribers: kde-pim

Tags: #kde_pim

Differential Revision: https://phabricator.kde.org/D19741
---
 autotests/libs/collectionattributetest.cpp | 15 +++++++++++++++
 autotests/libs/collectionattributetest.h   |  1 +
 src/core/collection.h                      |  8 ++------
 3 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/autotests/libs/collectionattributetest.cpp b/autotests/libs/collectionattributetest.cpp
index e264a37..9c46561 100644
--- a/autotests/libs/collectionattributetest.cpp
+++ b/autotests/libs/collectionattributetest.cpp
@@ -240,3 +240,18 @@ void CollectionAttributeTest::testCollectionIdentificationAttribute()
     QCOMPARE(parsed.identifier(), id);
     QCOMPARE(parsed.collectionNamespace(), ns);
 }
+
+void CollectionAttributeTest::testDetach()
+{
+    // GIVEN a collection with an attribute
+    Collection col;
+    col.attribute<TestAttribute>(Akonadi::Collection::AddIfMissing);
+    Collection col2 = col; // and a copy, so that non-const access detaches
+
+    // WHEN
+    TestAttribute *attr = col2.attribute<TestAttribute>(Akonadi::Collection::AddIfMissing);
+    TestAttribute *attr2 = col2.attribute<TestAttribute>();
+
+    // THEN
+    QCOMPARE(attr, attr2);
+}
diff --git a/autotests/libs/collectionattributetest.h b/autotests/libs/collectionattributetest.h
index 420df78..2afa9eb 100644
--- a/autotests/libs/collectionattributetest.h
+++ b/autotests/libs/collectionattributetest.h
@@ -32,6 +32,7 @@ private Q_SLOTS:
     void testDefaultAttributes();
     void testCollectionRightsAttribute();
     void testCollectionIdentificationAttribute();
+    void testDetach();
 };
 
 #endif
diff --git a/src/core/collection.h b/src/core/collection.h
index b5a496c..9c19cc9 100644
--- a/src/core/collection.h
+++ b/src/core/collection.h
@@ -565,10 +565,10 @@ inline T *Akonadi::Collection::attribute(Collection::CreateOption option)
     Q_UNUSED(option);
 
     const T dummy;
+    markAttributesChanged();
     if (hasAttribute(dummy.type())) {
         T *attr = dynamic_cast<T *>(attribute(dummy.type()));
         if (attr) {
-            markAttributesChanged();
             return attr;
         }
         //Reuse 5250
@@ -585,14 +585,10 @@ template <typename T>
 inline T *Akonadi::Collection::attribute() const
 {
     const QByteArray type = T().type();
+    const_cast<Collection*>(this)->markAttributesChanged();
     if (hasAttribute(type)) {
         T *attr = dynamic_cast<T *>(attribute(type));
         if (attr) {
-            // FIXME: This method returns a non-const pointer, so callers may still modify the
-            // attribute. Unfortunately, just making this function return a const pointer and
-            // creating a non-const overload does not work, as many users of this function abuse the
-            // non-const pointer and modify the attribute even on a const object.
-            const_cast<Collection*>(this)->markAttributesChanged();
             return attr;
         }
         //reuse 5250
-- 
cgit v1.1