1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
|
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
|
