Skip to content

Commit

Permalink
Merge branch 'jb52193' into 'master'
Browse files Browse the repository at this point in the history 
[qtcontacts-sqlite] Fix detail URI prefixing during aggregation. Contributes to JB#52193

See merge request mer-core/qtcontacts-sqlite!55
  • Loading branch information
chriadam committed Dec 2, 2020
2 parents 21f356e + 8fc7741 commit 6b3ae12
Show file tree
Hide file tree
Showing 2 changed files with 67 additions and 42 deletions.
57 changes: 25 additions & 32 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 @@ -3232,13 +3235,18 @@ ContactsDatabase::Query bindDetail(ContactsDatabase &db, quint32 contactId, quin
query.bindValue(":contactId", contactId);
query.bindValue(":name", detailValue(detail, T::FieldName));

QByteArray serialized;
QBuffer buffer(&serialized);
buffer.open(QIODevice::WriteOnly);
QDataStream out(&buffer);
out.setVersion(QDataStream::Qt_5_6);
out << detailValue(detail, T::FieldData);
query.bindValue(":data", serialized);
const QVariant variantValue = detailValue(detail, T::FieldData);
if (variantValue.isNull()) {
query.bindValue(":data", variantValue);
} else {
QByteArray serialized;
QBuffer buffer(&serialized);
buffer.open(QIODevice::WriteOnly);
QDataStream out(&buffer);
out.setVersion(QDataStream::Qt_5_6);
out << detailValue(detail, T::FieldData);
query.bindValue(":data", serialized);
}

return query;
}
Expand Down Expand Up @@ -3303,10 +3311,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 +3342,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 +3385,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 @@ -3549,8 +3546,8 @@ QContactManager::Error ContactWriter::save(
dbId = ContactId::databaseId(contactId);
m_addedIds.insert(contactId);
} else {
QTCONTACTS_SQLITE_WARNING(QString::fromLatin1("Error creating contact: %1 collectionId: %2")
.arg(err).arg(ContactCollectionId::toString(contact.collectionId())));
QTCONTACTS_SQLITE_WARNING(QString::fromLatin1("Error creating contact in collection: %2 : %3")
.arg(ContactCollectionId::toString(contact.collectionId())).arg(err));
}
} else {
err = update(&contact, definitionMask, &aggregateUpdated, true, withinAggregateUpdate, withinSyncUpdate, recordUnhandledChangeFlags, presenceOnlyUpdate);
Expand Down Expand Up @@ -3717,7 +3714,7 @@ static QContactManager::Error enforceDetailConstraints(QContact *contact)
if (!detailUri.isEmpty()) {
if (detailUris.contains(detailUri)) {
// This URI conflicts with one already present in the contact
QTCONTACTS_SQLITE_WARNING(QString::fromLatin1("Detail URI confict on: %1 %2 %3").arg(detailUri).arg(detailTypeName(det)).arg(det.type()));
QTCONTACTS_SQLITE_WARNING(QString::fromLatin1("Detail URI conflict on: %1 %2 %3").arg(detailUri).arg(detailTypeName(det)).arg(det.type()));
return QContactManager::InvalidDetailError;
}

Expand Down Expand Up @@ -3850,6 +3847,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 +4265,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 @@ -4513,13 +4512,15 @@ QContactManager::Error ContactWriter::create(QContact *contact, const DetailList
if (!aggregable) {
writeErr = collectionIsAggregable(contact->collectionId(), &aggregable);
if (writeErr != QContactManager::NoError) {
contact->setId(QContactId()); // reset to null id as the transaction will rolled back.
return writeErr;
}
}

if (aggregable) {
writeErr = setAggregate(contact, contactId, false, definitionMask, withinTransaction, withinSyncUpdate);
if (writeErr != QContactManager::NoError) {
contact->setId(QContactId()); // reset to null id as the transaction will rolled back.
return writeErr;
}
}
Expand Down Expand Up @@ -4665,14 +4666,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
52 changes: 42 additions & 10 deletions tests/auto/aggregation/tst_aggregation.cpp
View file
Expand Up @@ -125,10 +125,6 @@ private slots:
void deletionMultiple();
void deletionCollections();

/*
void testSyncAdapter();
*/

void testOOB();

private:
Expand Down Expand Up @@ -3139,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 @@ -3190,15 +3187,50 @@ 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
// which should get aggregated into the same contact.
// because the aggregate already exists, its id will
// be lower than the id of the new constituent, and
// thus the regenerateAggregates() codepath should be hit.
QContactCollection testAddressbook;
testAddressbook.setMetaData(QContactCollection::KeyName, QStringLiteral("test"));
testAddressbook.setExtendedMetaData(COLLECTION_EXTENDEDMETADATA_KEY_APPLICATIONNAME, "tst_aggregation");
testAddressbook.setExtendedMetaData(COLLECTION_EXTENDEDMETADATA_KEY_ACCOUNTID, 5);
testAddressbook.setExtendedMetaData(COLLECTION_EXTENDEDMETADATA_KEY_REMOTEPATH, "/addressbooks/test");
QVERIFY(m_cm->saveCollection(&testAddressbook));

QContact syncAlice;
syncAlice.setCollectionId(testAddressbook.id());

QContactName san;
san.setFirstName("Alice9");
san.setMiddleName("In");
san.setLastName("Wonderland");
syncAlice.saveDetail(&san);

QContactPhoneNumber saph;
saph.setNumber("999111999");

// 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());
QVERIFY(m_cm->saveContact(&syncAlice));
QVERIFY(!syncAlice.id().isNull());
}

void tst_Aggregation::correctDetails()
Expand Down

0 comments on commit 6b3ae12

Please sign in to comment.