Skip to content

Commit

Permalink
[qtcontacts-sqlite] Fix detail URI prefixing during aggregation. Cont…
Browse files Browse the repository at this point in the history 
…ributes to JB#52193

Detail URIs only need to be unique-per-contact, thus if an aggregate
aggregates two contacts where each contact contains a detail with
a specific detail URI, we need to ensure that we prefix the detail
URI appropriately to disambiguate between the two.
  • Loading branch information
@chriadam
chriadam committed Dec 2, 2020
1 parent a2da0db commit 7c3a7d3
Show file tree
Hide file tree
Showing 2 changed files with 21 additions and 34 deletions.
30 changes: 8 additions & 22 deletions src/engine/contactwriter.cpp
View file
Expand Up @@ -2217,14 +2217,17 @@ template <typename T> bool removeSpecificDetails(ContactsDatabase &db, quint32 c
return removeSpecificDetails(db, contactId, RemoveStatement<T>::statement, detailTypeName<T>(), error);
}

static void adjustAggregateDetailProperties(QContactDetail &detail)
static void adjustAggregateDetailProperties(QContactDetail &detail, const QContact &constituent)
{
// Modify this detail URI to preserve uniqueness - the result must not clash with the
// URI in the constituent's copy (there won't be any other aggregator of the same detail)
// URI in the constituent's copy (there won't be any other aggregator of the same detail).
// Also, since the detail URI only needs to be unique-per-contact, if an aggregate
// aggregates two contacts, each of those contacts may have a detail which has a particular
// detail URI. Thus, to disambiguate, we need to prefix with the constituent contact id.

// If a detail URI is modified for aggregation, we need to insert a prefix
const QString aggregateTag(QStringLiteral("aggregate"));
const QString prefix(aggregateTag + QStringLiteral(":"));
const QString prefix(QStringLiteral("%1-%2:").arg(aggregateTag).arg(ContactId::databaseId(constituent)));

QString detailUri = detail.detailUri();
if (!detailUri.isEmpty() && !detailUri.startsWith(prefix)) {
Expand Down Expand Up @@ -3303,10 +3306,6 @@ template <typename T> bool ContactWriter::writeDetails(
return false;
}

if (aggregateContact) {
adjustAggregateDetailProperties(detail);
}

if (!writeCommonDetails(contactId, detailId, detail, syncable, wasLocal, aggregateContact, recordUnhandledChangeFlags, error)) {
return false;
}
Expand Down Expand Up @@ -3338,9 +3337,6 @@ template <typename T> bool ContactWriter::writeDetails(
typename QList<T>::iterator ait = additions.begin(), aend = additions.end();
for ( ; ait != aend; ++ait) {
T &detail(*ait);
if (aggregateContact) {
adjustAggregateDetailProperties(detail);
}

const quint32 detailId = writeCommonDetails(contactId, 0, detail, syncable, wasLocal, aggregateContact, recordUnhandledChangeFlags, error);
if (detailId == 0) {
Expand Down Expand Up @@ -3384,10 +3380,6 @@ template <typename T> bool ContactWriter::writeDetails(
for ( ; it != end; ++it) {
T &detail(*it);

if (aggregateContact) {
adjustAggregateDetailProperties(detail);
}

const quint32 detailId = writeCommonDetails(contactId, 0, detail, syncable, wasLocal, aggregateContact, recordUnhandledChangeFlags, error);
if (detailId == 0) {
return false;
Expand Down Expand Up @@ -3850,6 +3842,7 @@ static void promoteDetailsToAggregate(const QContact &contact, QContact *aggrega
// Store the provenance of this promoted detail
det.setValue(QContactDetail::FieldProvenance, original.value<QString>(QContactDetail::FieldProvenance));

adjustAggregateDetailProperties(det, contact);
aggregate->saveDetail(&det, QContact::IgnoreAccessConstraints);
}
}
Expand Down Expand Up @@ -4267,6 +4260,7 @@ QContactManager::Error ContactWriter::regenerateAggregates(const QList<quint32>
QContactDetail currDet = currDetails.at(j);
if (promoteDetailType(currDet.type(), definitionMask, false)) {
// unconditionally promote this detail to the aggregate.
adjustAggregateDetailProperties(currDet, curr);
aggregateContact.saveDetail(&currDet, QContact::IgnoreAccessConstraints);
}
}
Expand Down Expand Up @@ -4667,14 +4661,6 @@ QContactManager::Error ContactWriter::update(QContact *contact, const DetailList
}
}

if (oldCollectionId == ContactCollectionId::apiId(ContactsDatabase::AggregateAddressbookCollectionId, m_managerUri)) {
// We need to modify the detail URIs in these details
QList<QContactDetail>::iterator it = transientDetails.begin(), end = transientDetails.end();
for ( ; it != end; ++it) {
adjustAggregateDetailProperties(*it);
}
}

const QDateTime lastModified(contact->detail<QContactTimestamp>().lastModified());
if (!m_database.setTransientDetails(contactId, lastModified, transientDetails)) {
QTCONTACTS_SQLITE_WARNING(QString::fromLatin1("Could not perform transient update; fallback to durable update"));
Expand Down
25 changes: 13 additions & 12 deletions tests/auto/aggregation/tst_aggregation.cpp
View file
Expand Up @@ -3135,14 +3135,15 @@ void tst_Aggregation::detailUris()
// now check to ensure that the detail uris and links were updated correctly
// in the aggregate. Those uris need to be unique in the database.
QCOMPARE(localAlice.detail<QContactPhoneNumber>().detailUri(), QLatin1String("alice9PhoneNumberDetailUri"));
QVERIFY(aggregateAlice.detail<QContactPhoneNumber>().detailUri().startsWith(QLatin1String("aggregate:")));
QVERIFY(aggregateAlice.detail<QContactPhoneNumber>().detailUri().startsWith(QStringLiteral("aggregate-%1:").arg(QString::fromLatin1(localAlice.id().localId()).remove(0,4))));
QVERIFY(aggregateAlice.detail<QContactPhoneNumber>().detailUri().endsWith(QLatin1String(":alice9PhoneNumberDetailUri")));
QCOMPARE(localAlice.detail<QContactEmailAddress>().linkedDetailUris(), QStringList() << QLatin1String("alice9PhoneNumberDetailUri"));
QCOMPARE(aggregateAlice.detail<QContactEmailAddress>().linkedDetailUris().count(), 1);
QVERIFY(aggregateAlice.detail<QContactEmailAddress>().linkedDetailUris().at(0).startsWith(QLatin1String("aggregate:")));
QVERIFY(aggregateAlice.detail<QContactEmailAddress>().linkedDetailUris().at(0).startsWith(QStringLiteral("aggregate-%1:").arg(QString::fromLatin1(localAlice.id().localId()).remove(0,4))));
QVERIFY(aggregateAlice.detail<QContactEmailAddress>().linkedDetailUris().at(0).endsWith(QLatin1String(":alice9PhoneNumberDetailUri")));

// try to add another detail with a conflicting detail URI
// try to add another detail with a conflicting detail URI.
// this should cause save to fail, as the detail URI must be unique within the contact.
QContact failAlice(alice);

QContactTag at;
Expand Down Expand Up @@ -3186,14 +3187,14 @@ void tst_Aggregation::detailUris()
// now check to ensure that the detail uris and links were updated correctly
// in the aggregate. Those uris need to be unique in the database.
QCOMPARE(localAlice.detail<QContactPhoneNumber>().detailUri(), QLatin1String("alice9PhoneNumberDetailUri"));
QVERIFY(aggregateAlice.detail<QContactPhoneNumber>().detailUri().startsWith(QLatin1String("aggregate:")));
QVERIFY(aggregateAlice.detail<QContactPhoneNumber>().detailUri().startsWith(QStringLiteral("aggregate-%1:").arg(QString::fromLatin1(localAlice.id().localId()).remove(0,4))));
QVERIFY(aggregateAlice.detail<QContactPhoneNumber>().detailUri().endsWith(QLatin1String(":alice9PhoneNumberDetailUri")));
QCOMPARE(localAlice.detail<QContactEmailAddress>().linkedDetailUris(), QStringList() << QLatin1String("alice9PhoneNumberDetailUri"));
QCOMPARE(aggregateAlice.detail<QContactEmailAddress>().linkedDetailUris().count(), 1);
QVERIFY(aggregateAlice.detail<QContactEmailAddress>().linkedDetailUris().at(0).startsWith(QLatin1String("aggregate:")));
QVERIFY(aggregateAlice.detail<QContactEmailAddress>().linkedDetailUris().at(0).startsWith(QStringLiteral("aggregate-%1:").arg(QString::fromLatin1(localAlice.id().localId()).remove(0,4))));
QVERIFY(aggregateAlice.detail<QContactEmailAddress>().linkedDetailUris().at(0).endsWith(QLatin1String(":alice9PhoneNumberDetailUri")));
QCOMPARE(localAlice.detail<QContactHobby>().detailUri(), QLatin1String("alice9HobbyDetailUri"));
QVERIFY(aggregateAlice.detail<QContactHobby>().detailUri().startsWith(QLatin1String("aggregate:")));
QVERIFY(aggregateAlice.detail<QContactHobby>().detailUri().startsWith(QStringLiteral("aggregate-%1:").arg(QString::fromLatin1(localAlice.id().localId()).remove(0,4))));
QVERIFY(aggregateAlice.detail<QContactHobby>().detailUri().endsWith(QLatin1String(":alice9HobbyDetailUri")));

// now create a contact in a separate collection
Expand All @@ -3219,15 +3220,15 @@ void tst_Aggregation::detailUris()

QContactPhoneNumber saph;
saph.setNumber("999111999");
saph.setDetailUri("alice9PhoneNumberDetailUri"); // try re-using the same detail uri, save should fail.

// try re-using the same detail uri; save should succeed,
// as it needs only be unique within the contact.
// thus, the aggregation promotion should mutate the
// detail uri appropriately to ensure that no conflict occurs.
saph.setDetailUri("alice9PhoneNumberDetailUri");
syncAlice.saveDetail(&saph);

QVERIFY(syncAlice.id().isNull());
QCOMPARE(m_cm->saveContact(&syncAlice), false);
QVERIFY(syncAlice.id().isNull()); // if the save fails during aggregation step, contact id should not be set.

saph.setDetailUri("alice9PhoneNumberDetailUri2"); // set it to something unique, save should succeed.
syncAlice.saveDetail(&saph);
QVERIFY(m_cm->saveContact(&syncAlice));
QVERIFY(!syncAlice.id().isNull());
}
Expand Down

0 comments on commit 7c3a7d3

Please sign in to comment.