From fca534d6b68a538549ef4e0814424ec7ad7e0f61 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 16 Jul 2013 10:29:52 -0700 Subject: [PATCH] Improve Asana API error handling in Doorkeeper Summary: Ref T2852. We need to distinguish between an API call which worked but got back nothing (404) and an API call which failed. In particular, Asana hit a sync issue which was likely the result of treating a 500 (or some other error) as a 404. Also clean up a couple small things. Test Plan: Ran syncs against deleted tasks and saw successful syncs of non-tasks, and simulated random failures and saw them get handled correctly. Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T2852 Differential Revision: https://secure.phabricator.com/D6470 --- .../query/DifferentialRevisionQuery.php | 5 +- .../bridge/DoorkeeperBridgeAsana.php | 18 ++++++- .../doorkeeper/engine/DoorkeeperObjectRef.php | 10 ++++ .../worker/DoorkeeperFeedWorkerAsana.php | 49 ++++++++++--------- 4 files changed, 56 insertions(+), 26 deletions(-) diff --git a/src/applications/differential/query/DifferentialRevisionQuery.php b/src/applications/differential/query/DifferentialRevisionQuery.php index 0434734112..ca9edbbe93 100644 --- a/src/applications/differential/query/DifferentialRevisionQuery.php +++ b/src/applications/differential/query/DifferentialRevisionQuery.php @@ -941,8 +941,9 @@ final class DifferentialRevisionQuery foreach ($revision_edges as $user_phid => $edge) { $data = $edge['data']; $reviewers[] = new DifferentialReviewer( - $user_phid, $data['status'], idx($data, 'diff', null) - ); + $user_phid, + idx($data, 'status'), + idx($data, 'diff')); } $revision->attachReviewerStatus($reviewers); diff --git a/src/applications/doorkeeper/bridge/DoorkeeperBridgeAsana.php b/src/applications/doorkeeper/bridge/DoorkeeperBridgeAsana.php index 0fff76a0eb..4c4f0cd4a6 100644 --- a/src/applications/doorkeeper/bridge/DoorkeeperBridgeAsana.php +++ b/src/applications/doorkeeper/bridge/DoorkeeperBridgeAsana.php @@ -64,17 +64,33 @@ final class DoorkeeperBridgeAsana extends DoorkeeperBridge { } $results = array(); + $failed = array(); foreach (Futures($futures) as $key => $future) { try { $results[$key] = $future->resolve(); } catch (Exception $ex) { - // TODO: For now, ignore this stuff. + if (($ex instanceof HTTPFutureResponseStatus) && + ($ex->getStatusCode() == 404)) { + // This indicates that the object has been deleted (or never existed, + // or isn't visible to the current user) but it's a successful sync of + // an object which isn't visible. + } else { + // This is something else, so consider it a synchronization failure. + phlog($ex); + $failed[$key] = $ex; + } } } foreach ($refs as $ref) { $ref->setAttribute('name', pht('Asana Task %s', $ref->getObjectID())); + $failed = idx($failed, $ref->getObjectKey()); + if ($failed) { + $ref->setSyncFailed(true); + continue; + } + $result = idx($results, $ref->getObjectKey()); if (!$result) { continue; diff --git a/src/applications/doorkeeper/engine/DoorkeeperObjectRef.php b/src/applications/doorkeeper/engine/DoorkeeperObjectRef.php index 71f11d6807..aa76571fee 100644 --- a/src/applications/doorkeeper/engine/DoorkeeperObjectRef.php +++ b/src/applications/doorkeeper/engine/DoorkeeperObjectRef.php @@ -9,6 +9,7 @@ final class DoorkeeperObjectRef extends Phobject { private $objectID; private $attributes = array(); private $isVisible; + private $syncFailed; private $externalObject; public function newExternalObject() { @@ -43,6 +44,15 @@ final class DoorkeeperObjectRef extends Phobject { return $this->isVisible; } + public function setSyncFailed($sync_failed) { + $this->syncFailed = $sync_failed; + return $this; + } + + public function getSyncFailed() { + return $this->syncFailed; + } + public function getAttribute($key, $default = null) { return idx($this->attributes, $key, $default); } diff --git a/src/applications/doorkeeper/worker/DoorkeeperFeedWorkerAsana.php b/src/applications/doorkeeper/worker/DoorkeeperFeedWorkerAsana.php index 3946f21bbb..2affc2038b 100644 --- a/src/applications/doorkeeper/worker/DoorkeeperFeedWorkerAsana.php +++ b/src/applications/doorkeeper/worker/DoorkeeperFeedWorkerAsana.php @@ -185,28 +185,23 @@ final class DoorkeeperFeedWorkerAsana extends FeedPushWorker { $phids = $this->getRelatedUserPHIDs($object); list($owner_phid, $active_phids, $passive_phids, $follow_phids) = $phids; - $all_follow_phids = array_merge( - $active_phids, - $passive_phids, - $follow_phids); - $all_follow_phids = array_unique(array_filter($all_follow_phids)); - $all_phids = array(); $all_phids = array_merge( array($owner_phid), - $all_follow_phids); + $active_phids, + $passive_phids, + $follow_phids); $all_phids = array_unique(array_filter($all_phids)); $phid_aid_map = $this->lookupAsanaUserIDs($all_phids); - if (!$phid_aid_map) { throw new PhabricatorWorkerPermanentFailureException( 'No related users have linked Asana accounts.'); } $owner_asana_id = idx($phid_aid_map, $owner_phid); - $all_follow_asana_ids = array_select_keys($phid_aid_map, $all_follow_phids); - $all_follow_asana_ids = array_values($all_follow_asana_ids); + $all_asana_ids = array_select_keys($phid_aid_map, $all_phids); + $all_asana_ids = array_values($all_asana_ids); // Even if the actor isn't a reviewer, etc., try to use their account so // we can post in the correct voice. If we miss, we'll try all the other @@ -243,7 +238,7 @@ final class DoorkeeperFeedWorkerAsana extends FeedPushWorker { $main_data = $this->getAsanaTaskData($object) + array( 'assignee' => $owner_asana_id, - 'followers' => $all_follow_asana_ids, + 'followers' => $all_asana_ids, ); $extra_data = array(); @@ -261,7 +256,10 @@ final class DoorkeeperFeedWorkerAsana extends FeedPushWorker { 'DoorkeeperExternalObject could not be loaded.'); } - if (!$parent_ref->getIsVisible()) { + if ($parent_ref->getSyncFailed()) { + throw new Exception( + 'Synchronization of parent task from Asana failed!'); + } else if (!$parent_ref->getIsVisible()) { $this->log("Skipping main task update, object is no longer visible.\n"); $extra_data['gone'] = true; } else { @@ -341,17 +339,6 @@ final class DoorkeeperFeedWorkerAsana extends FeedPushWorker { 'this likely indicates the Asana task has been deleted.'); } - // Post the feed story itself to the main Asana task. - - $this->makeAsanaAPICall( - $oauth_token, - 'tasks/'.$parent_ref->getObjectID().'/stories', - 'POST', - array( - 'text' => $story->renderText(), - )); - - // Now, handle the subtasks. $sub_editor = id(new PhabricatorEdgeEditor()) @@ -371,6 +358,10 @@ final class DoorkeeperFeedWorkerAsana extends FeedPushWorker { ->execute(); foreach ($refs as $ref) { + if ($ref->getSyncFailed()) { + throw new Exception( + 'Synchronization of child task from Asana failed!'); + } if (!$ref->getIsVisible()) { $ref->getExternalObject()->delete(); continue; @@ -497,6 +488,18 @@ final class DoorkeeperFeedWorkerAsana extends FeedPushWorker { $sub_editor->save(); + + // Post the feed story itself to the main Asana task. We do this last + // because everything else is idempotent, so this is the only effect we + // can't safely run more than once. + + $this->makeAsanaAPICall( + $oauth_token, + 'tasks/'.$parent_ref->getObjectID().'/stories', + 'POST', + array( + 'text' => $story->renderText(), + )); } private function lookupAsanaUserIDs($all_phids) {