Skip to content

Commit

Permalink
[buteo-sync-plugins-social] Fix Google Contact Sync
Browse files Browse the repository at this point in the history 
This commit does three things:
1) improves verbosity of traces / logging from Google contacts plugin
2) fixes batch-size detection bug which could corrupt upsync
3) fixes batch-iteration bug which could cause duplication on upsync
  • Loading branch information
@chriadam
chriadam committed Jan 9, 2015
1 parent ba56c46 commit 50db670
Show file tree
Hide file tree
Showing 2 changed files with 41 additions and 21 deletions.
29 changes: 18 additions & 11 deletions src/google/google-contacts/googlecontactstream.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,7 @@
#include "googlecontactstream.h"
#include "googlecontactatom.h"
#include "constants_p.h"

#include <LogMacros.h> // from Buteo
#include "trace.h"

#include <QDateTime>

Expand Down Expand Up @@ -77,11 +76,19 @@ QByteArray GoogleContactStream::encode(const QMultiMap<GoogleContactStream::Upda
mXmlWriter = new QXmlStreamWriter(&xmlBuffer);
startBatchFeed();

foreach (GoogleContactStream::UpdateType updateType, updates.keys()) {

This comment has been minimized.

Sign in to view

Copy link
@chriadam

chriadam Jan 9, 2015

Author Contributor

this is what was causing the duplications, obviously.

This comment has been minimized.

Sign in to view

Copy link
@faenil

faenil Jan 9, 2015

Member

finally the bug was found! great! well done :)

This comment has been minimized.

Sign in to view

Copy link
@antseppa

antseppa Jan 9, 2015

Contributor

\o/ :)
QList<QPair<QContact, QStringList> > contacts = updates.values(updateType);
for (int i = 0; i < contacts.size(); ++i) {
encodeContactUpdate(contacts[i].first, contacts[i].second, updateType, true); // batchmode = true
}
QList<QPair<QContact, QStringList> > removedContacts = updates.values(GoogleContactStream::Remove);
for (int i = 0; i < removedContacts.size(); ++i) {
encodeContactUpdate(removedContacts[i].first, removedContacts[i].second, GoogleContactStream::Remove, true); // batchmode = true
}

QList<QPair<QContact, QStringList> > addedContacts = updates.values(GoogleContactStream::Add);
for (int i = 0; i < addedContacts.size(); ++i) {
encodeContactUpdate(addedContacts[i].first, addedContacts[i].second, GoogleContactStream::Add, true); // batchmode = true
}

QList<QPair<QContact, QStringList> > modifiedContacts = updates.values(GoogleContactStream::Modify);
for (int i = 0; i < modifiedContacts.size(); ++i) {
encodeContactUpdate(modifiedContacts[i].first, modifiedContacts[i].second, GoogleContactStream::Modify, true); // batchmode = true
}

endBatchFeed();
Expand Down Expand Up @@ -800,7 +807,7 @@ void GoogleContactStream::encodeId(const QContact &qContact, bool isUpdate)
if (isUpdate) {
// according to the docs, this should be "base" instead of "full" -- but that actually fails.
if (mAccountEmail.isEmpty()) {
qWarning() << Q_FUNC_INFO << "account email not known - unable to build batch edit id!";
SOCIALD_LOG_ERROR("account email not known - unable to build batch edit id!");
} else {
mXmlWriter->writeTextElement("atom:id", "http://www.google.com/m8/feeds/contacts/" + mAccountEmail + "/full/" + remoteId);
}
Expand Down Expand Up @@ -831,7 +838,7 @@ void GoogleContactStream::encodeEtag(const QContact &qContact, bool needed)
} else if (needed) {
// we're trying to delete a contact in a batch operation
// but we don't know the etag of the deleted contact.
qWarning() << Q_FUNC_INFO << "etag needed but not available! caller needs to prefill for deletion updates!";
SOCIALD_LOG_ERROR("etag needed but not available! caller needs to prefill for deletion updates!");
}
}

Expand Down Expand Up @@ -1008,7 +1015,7 @@ void GoogleContactStream::encodeHobby(const QContactHobby &hobby)
void GoogleContactStream::encodeGeoLocation(const QContactGeoLocation &geolocation)
{
Q_UNUSED(geolocation);
qWarning() << Q_FUNC_INFO << "skipping geolocation";
SOCIALD_LOG_INFO("skipping geolocation");
}

void GoogleContactStream::encodeOrganization(const QContactOrganization &organization)
Expand All @@ -1029,7 +1036,7 @@ void GoogleContactStream::encodeAvatar(const QContactAvatar &avatar, const QCont
// XXX TODO: determine if it's a new local avatar, if so, push it up.
QUrl imageUrl(avatar.imageUrl());
if (imageUrl.isLocalFile()) {
qWarning() << Q_FUNC_INFO << "have avatar:" << imageUrl;
SOCIALD_LOG_INFO("have avatar:" << imageUrl << "but not upsyncing avatars");
mEncodedContactsWithAvatars << qContact.id();
}
}
Expand Down
33 changes: 23 additions & 10 deletions src/google/google-contacts/googletwowaycontactsyncadaptor.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -439,22 +439,29 @@ void GoogleTwoWayContactSyncAdaptor::upsyncLocalChanges(const QDateTime &localSi
updatedLocallyModified.append(c);
}

QList<QContactId> alreadyEncoded; // shouldn't be necessary, as determineLocalChanges should already ensure distinct result sets.
QList<QPair<QContact, GoogleContactStream::UpdateType> > contactUpdatesToPost;
foreach (const QContact &c, locallyAdded)
contactUpdatesToPost.append(qMakePair(c, GoogleContactStream::Add));
foreach (const QContact &c, updatedLocallyModified)
contactUpdatesToPost.append(qMakePair(c, GoogleContactStream::Modify));
foreach (const QContact &c, locallyDeleted) {
contactUpdatesToPost.append(qMakePair(c, GoogleContactStream::Remove));
m_contactAvatars[accId].remove(c.detail<QContactGuid>().guid()); // just in case the avatar was outstanding.
alreadyEncoded.append(c.id());
}
foreach (const QContact &c, locallyAdded) {
if (!alreadyEncoded.contains(c.id())) {
contactUpdatesToPost.append(qMakePair(c, GoogleContactStream::Add));
alreadyEncoded.append(c.id());
}
}
foreach (const QContact &c, updatedLocallyModified) {
if (!alreadyEncoded.contains(c.id())) {
contactUpdatesToPost.append(qMakePair(c, GoogleContactStream::Modify));
}
}
m_localChanges[accId] = contactUpdatesToPost;

SOCIALD_LOG_INFO("Google account:" << accId <<
"upsyncing local contact changes since:" << localSince.toString(Qt::ISODate) << "\n" <<
" locally added: " << locallyAdded.count() << "\n"
" locally modified: " << locallyModified.count() << "\n"
" locally removed: " << locallyDeleted.count() << "\n");
"upsyncing local A/M/R:" << locallyAdded.count() << "/" << locallyModified.count() << "/" << locallyDeleted.count() <<
"since:" << localSince.toString(Qt::ISODate));

upsyncLocalChangesList(accId);
}
Expand All @@ -469,6 +476,7 @@ void GoogleTwoWayContactSyncAdaptor::upsyncLocalChangesList(int accountId)
bool postedData = false;
if (!m_accountSyncProfile || m_accountSyncProfile->syncDirection() != Buteo::SyncProfile::SYNC_DIRECTION_FROM_REMOTE) {
// two-way sync is the default setting. Upsync the changes.
int batchCount = 0;
QMultiMap<GoogleContactStream::UpdateType, QPair<QContact, QStringList> > batch;
for (int i = m_localChanges[accountId].size() - 1; i >= 0; --i) {
QPair<QContact, GoogleContactStream::UpdateType> entry = m_localChanges[accountId].takeAt(i);
Expand All @@ -482,14 +490,19 @@ void GoogleTwoWayContactSyncAdaptor::upsyncLocalChangesList(int accountId)
} else {
extraXmlElements.append(QStringLiteral("<gContact:groupMembershipInfo deleted=\"false\" href=\"%1\"></gContact:groupMembershipInfo>").arg(myContactsGroupAtomId));
batch.insertMulti(entry.second, qMakePair(entry.first, extraXmlElements));
batchCount++;
}
} else {
batch.insertMulti(entry.second, qMakePair(entry.first, extraXmlElements));
batchCount++;
}

if (batch.size() == SOCIALD_GOOGLE_MAX_CONTACT_ENTRY_RESULTS || i == 0) {
if (batchCount == SOCIALD_GOOGLE_MAX_CONTACT_ENTRY_RESULTS || i == 0) {
GoogleContactStream encoder(false, accountId, m_emailAddresses[accountId]);
QByteArray encodedContactUpdates = encoder.encode(batch);
SOCIALD_LOG_TRACE("storing a batch of" << batchCount << "local changes to remote server for account" << accountId);
batch.clear();
batchCount = 0;
storeToRemote(accountId, m_accessTokens[accountId], encodedContactUpdates);
postedData = true;
break;
Expand Down Expand Up @@ -888,7 +901,7 @@ void GoogleTwoWayContactSyncAdaptor::finalCleanup()
purgeAccountIds.append(purgeId);
}
} else {
qWarning() << Q_FUNC_INFO << "Malformed GUID not of form <accountId>:<remoteGuid>";
SOCIALD_LOG_ERROR("Malformed GUID not of form <accountId>:<remoteGuid> for contact" << contact.id().toString() << ":" << guid.guid());
}
}

Expand Down

0 comments on commit 50db670

Please sign in to comment.