Make limits and ranges work better with Calendar event queries

Summary:
Fixes T8911. This corrects several issues which could crop up if a calendar event query matched more results than the query limit:

  - The desired order was not applied by the SearchEngine -- it applies the first builtin order instead. Provide a proper builtin order.
  - When we generate ghosts, we can't do limiting in the database because we may select and then immediately discard a large number of parent events which are outside of the query range.
    - For now, just don't limit results to get the behavior correct.
    - This may need to be refined eventually to improve performance.
  - When trimming events, we could trim parents and fail to generate ghosts from them. Separate parent events out first.
  - Try to simplify some logic.

Test Plan: An "Upcoming" dashboard panel with limit 10 and the main Calendar "Upcoming Events" UI now show the same results.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T8911

Differential Revision: https://secure.phabricator.com/D16289

src/applications/calendar/query/PhabricatorCalendarEventQuery.php

@@ -75,6 +75,15 @@ final class PhabricatorCalendarEventQuery
     return array('start', 'id');
   }
 
+  public function getBuiltinOrders() {
+    return array(
+      'start' => array(
+        'vector' => array('start', 'id'),
+        'name' => pht('Event Start'),
+      ),
+    ) + parent::getBuiltinOrders();
+  }
+
   public function getOrderableColumns() {
     return array(
       'start' => array(
@@ -95,6 +104,17 @@ final class PhabricatorCalendarEventQuery
     );
   }
 
+  protected function shouldLimitResults() {
+    // When generating ghosts, we can't rely on database ordering because
+    // MySQL can't predict the ghost start times. We'll just load all matching
+    // events, then generate results from there.
+    if ($this->generateGhosts) {
+      return false;
+    }
+
+    return true;
+  }
+
   protected function loadPage() {
     $events = $this->loadStandardPage($this->newResultObject());
 
@@ -107,7 +127,6 @@ final class PhabricatorCalendarEventQuery
       return $events;
     }
 
-    $enforced_end = null;
     $raw_limit = $this->getRawResultLimit();
 
     if (!$raw_limit && !$this->rangeEnd) {
@@ -121,7 +140,6 @@ final class PhabricatorCalendarEventQuery
     foreach ($events as $key => $event) {
       $sequence_start = 0;
       $sequence_end = null;
-      $duration = $event->getDuration();
       $end = null;
 
       $instance_of = $event->getInstanceOfEventPHID();
@@ -132,8 +150,29 @@ final class PhabricatorCalendarEventQuery
           continue;
         }
       }
+    }
+
+    // Pull out all of the parents first. We may discard them as we begin
+    // generating ghost events, but we still want to process all of them.
+    $parents = array();
+    foreach ($events as $key => $event) {
+      if ($event->isParentEvent()) {
+        $parents[$key] = $event;
+      }
+    }
+
+    // Now that we've picked out all the parent events, we can immediately
+    // discard anything outside of the time window.
+    $events = $this->getEventsInRange($events);
+
+    $enforced_end = null;
+    foreach ($parents as $key => $event) {
+      $sequence_start = 0;
+      $sequence_end = null;
+      $start = null;
+
+      $duration = $event->getDuration();
 
-      if ($event->getIsRecurring() && $instance_of == null) {
       $frequency = $event->getFrequencyUnit();
       $modify_key = '+1 '.$frequency;
 
@@ -158,22 +197,22 @@ final class PhabricatorCalendarEventQuery
       $date = $start;
       $datetime = PhabricatorTime::getDateTimeFromEpoch($date, $viewer);
 
-        if (($this->rangeEnd && $event->getRecurrenceEndDate()) &&
+      // Select the minimum end time we need to generate events until.
-          $this->rangeEnd < $event->getRecurrenceEndDate()) {
+      $end_times = array();
-          $end = $this->rangeEnd;
+      if ($this->rangeEnd) {
-        } else if ($event->getRecurrenceEndDate()) {
+        $end_times[] = $this->rangeEnd;
-          $end = $event->getRecurrenceEndDate();
-        } else if ($this->rangeEnd) {
-          $end = $this->rangeEnd;
-        } else if ($enforced_end) {
-          if ($end) {
-            $end = min($end, $enforced_end);
-          } else {
-            $end = $enforced_end;
-          }
       }
 
-        if ($end) {
+      if ($event->getRecurrenceEndDate()) {
+        $end_times[] = $event->getRecurrenceEndDate();
+      }
+
+      if ($enforced_end) {
+        $end_times[] = $enforced_end;
+      }
+
+      if ($end_times) {
+        $end = min($end_times);
         $sequence_end = $sequence_start;
         while ($date < $end) {
           $sequence_end++;
@@ -188,7 +227,6 @@ final class PhabricatorCalendarEventQuery
       }
 
       $sequence_start = max(1, $sequence_start);
-
       for ($index = $sequence_start; $index < $sequence_end; $index++) {
         $events[] = $event->newGhost($viewer, $index);
       }
@@ -199,14 +237,13 @@ final class PhabricatorCalendarEventQuery
       // because they couldn't possibly ever appear in the result set.
 
       if ($raw_limit) {
-          if (count($events) >= $raw_limit) {
+        if (count($events) > $raw_limit) {
           $events = msort($events, 'getViewerDateFrom');
           $events = array_slice($events, 0, $raw_limit, true);
           $enforced_end = last($events)->getViewerDateFrom();
         }
       }
     }
-    }
 
     // Now that we're done generating ghost events, we're going to remove any
     // ghosts that we have concrete events for (or which we can load the
@@ -271,11 +308,14 @@ final class PhabricatorCalendarEventQuery
       }
     }
 
+    $events = msort($events, 'getViewerDateFrom');
+
     return $events;
   }
 
   protected function buildJoinClauseParts(AphrontDatabaseConnection $conn_r) {
     $parts = parent::buildJoinClauseParts($conn_r);
+
     if ($this->inviteePHIDs !== null) {
       $parts[] = qsprintf(
         $conn_r,
@@ -284,6 +324,7 @@ final class PhabricatorCalendarEventQuery
         id(new PhabricatorCalendarEventInvitee())->getTableName(),
         PhabricatorCalendarEventInvitee::STATUS_UNINVITED);
     }
+
     return $parts;
   }
 
@@ -397,23 +438,11 @@ final class PhabricatorCalendarEventQuery
 
 
   protected function willFilterPage(array $events) {
-    $range_start = $this->rangeBegin;
-    $range_end = $this->rangeEnd;
     $instance_of_event_phids = array();
     $recurring_events = array();
     $viewer = $this->getViewer();
 
-    foreach ($events as $key => $event) {
+    $events = $this->getEventsInRange($events);
-      $event_start = $event->getViewerDateFrom();
-      $event_end = $event->getViewerDateTo();
-
-      if ($range_start && $event_end < $range_start) {
-        unset($events[$key]);
-      }
-      if ($range_end && $event_start > $range_end) {
-        unset($events[$key]);
-      }
-    }
 
     $phids = array();
 
@@ -476,4 +505,24 @@ final class PhabricatorCalendarEventQuery
     return $events;
   }
 
+  private function getEventsInRange(array $events) {
+    $range_start = $this->rangeBegin;
+    $range_end = $this->rangeEnd;
+
+    foreach ($events as $key => $event) {
+      $event_start = $event->getViewerDateFrom();
+      $event_end = $event->getViewerDateTo();
+
+      if ($range_start && $event_end < $range_start) {
+        unset($events[$key]);
+      }
+
+      if ($range_end && $event_start > $range_end) {
+        unset($events[$key]);
+      }
+    }
+
+    return $events;
+  }
+
 }

src/applications/calendar/query/PhabricatorCalendarEventSearchEngine.php

@@ -192,10 +192,11 @@ final class PhabricatorCalendarEventSearchEngine
     }
 
     if ($upcoming) {
+      $now = PhabricatorTime::getNow();
       if ($min_range) {
-        $min_range = max(time(), $min_range);
+        $min_range = max($now, $min_range);
       } else {
-        $min_range = time();
+        $min_range = $now;
       }
     }
 

src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php

@@ -142,11 +142,18 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery
   }
 
   final protected function buildLimitClause(AphrontDatabaseConnection $conn_r) {
-    if ($this->getRawResultLimit()) {
+    if ($this->shouldLimitResults()) {
-      return qsprintf($conn_r, 'LIMIT %d', $this->getRawResultLimit());
+      $limit = $this->getRawResultLimit();
-    } else {
+      if ($limit) {
+        return qsprintf($conn_r, 'LIMIT %d', $limit);
+      }
+    }
+
     return '';
   }
+
+  protected function shouldLimitResults() {
+    return true;
   }
 
   final protected function didLoadResults(array $results) {