From eb3fd2b7f513577d394e9fcf9f5a8749fd0d57c6 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sat, 17 Feb 2018 04:31:16 -0800 Subject: [PATCH 01/13] Fix an issue with marking aborted buildables failed when more than one build is aborted Summary: See . Test Plan: Used `bin/storage upgrade -f --apply ...` to re-apply the migration. Differential Revision: https://secure.phabricator.com/D19116 --- resources/sql/autopatches/20180214.harbor.01.aborted.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/resources/sql/autopatches/20180214.harbor.01.aborted.php b/resources/sql/autopatches/20180214.harbor.01.aborted.php index 689d5625f5..365f375dc6 100644 --- a/resources/sql/autopatches/20180214.harbor.01.aborted.php +++ b/resources/sql/autopatches/20180214.harbor.01.aborted.php @@ -10,7 +10,8 @@ foreach (new LiskMigrationIterator($table) as $buildable) { $aborted = queryfx_one( $conn, - 'SELECT * FROM %T WHERE buildablePHID = %s AND buildStatus = %s', + 'SELECT * FROM %T WHERE buildablePHID = %s AND buildStatus = %s + LIMIT 1', id(new HarbormasterBuild())->getTableName(), $buildable->getPHID(), 'aborted'); From 05a4c55c52be313c750b1552b89ad33bcf64a0dd Mon Sep 17 00:00:00 2001 From: epriestley Date: Sat, 17 Feb 2018 17:37:38 -0800 Subject: [PATCH 02/13] Explicitly add rel="noreferrer" to all external links Summary: See D19117. Instead of automatically figuring this out inside `phutil_tag()`, explicitly add rel="noreferrer" at the application level to all external links. Test Plan: - Grepped for `_blank`, `isValidRemoteURIForLink`, checked all callsites for user-controlled data. - Created a link menu item, verified noreferrer in markup. - Created a link custom field, verified no referrer in markup. - Verified noreferrer for `{nav href=...}`. Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Differential Revision: https://secure.phabricator.com/D19118 --- .../auth/view/PhabricatorAuthAccountView.php | 1 + .../PhabricatorCalendarICSURIImportEngine.php | 1 + .../markup/PhabricatorImageRemarkupRule.php | 8 +---- .../artifact/HarbormasterURIArtifact.php | 1 + .../nuance/item/NuanceGitHubEventItemType.php | 2 ++ .../PhabricatorPhurlLinkRemarkupRule.php | 1 + .../PhabricatorLinkProfileMenuItem.php | 3 +- .../PhabricatorStandardCustomFieldLink.php | 6 +++- src/view/layout/PhabricatorActionView.php | 3 ++ src/view/phui/PHUIListItemView.php | 13 ++++++- src/view/phui/PHUITagView.php | 35 +++++++++++-------- 11 files changed, 49 insertions(+), 25 deletions(-) diff --git a/src/applications/auth/view/PhabricatorAuthAccountView.php b/src/applications/auth/view/PhabricatorAuthAccountView.php index 8eb73144aa..3f04281c8f 100644 --- a/src/applications/auth/view/PhabricatorAuthAccountView.php +++ b/src/applications/auth/view/PhabricatorAuthAccountView.php @@ -77,6 +77,7 @@ final class PhabricatorAuthAccountView extends AphrontView { array( 'href' => $account_uri, 'target' => '_blank', + 'rel' => 'noreferrer', ), $account_uri); } diff --git a/src/applications/calendar/import/PhabricatorCalendarICSURIImportEngine.php b/src/applications/calendar/import/PhabricatorCalendarICSURIImportEngine.php index 6174603c84..bd52ec5bc2 100644 --- a/src/applications/calendar/import/PhabricatorCalendarICSURIImportEngine.php +++ b/src/applications/calendar/import/PhabricatorCalendarICSURIImportEngine.php @@ -45,6 +45,7 @@ final class PhabricatorCalendarICSURIImportEngine array( 'href' => $uri, 'target' => '_blank', + 'rel' => 'noreferrer', ), $uri); } diff --git a/src/applications/files/markup/PhabricatorImageRemarkupRule.php b/src/applications/files/markup/PhabricatorImageRemarkupRule.php index 9e91bdc096..36089dc57e 100644 --- a/src/applications/files/markup/PhabricatorImageRemarkupRule.php +++ b/src/applications/files/markup/PhabricatorImageRemarkupRule.php @@ -20,7 +20,6 @@ final class PhabricatorImageRemarkupRule extends PhutilRemarkupRule { $defaults = array( 'uri' => null, 'alt' => null, - 'href' => null, 'width' => null, 'height' => null, ); @@ -45,10 +44,6 @@ final class PhabricatorImageRemarkupRule extends PhutilRemarkupRule { $args += $defaults; - if ($args['href'] && !PhabricatorEnv::isValidURIForLink($args['href'])) { - $args['href'] = null; - } - if ($args['uri']) { $src_uri = id(new PhutilURI('/file/imageproxy/')) ->setQueryParam('uri', (string)$args['uri']); @@ -57,10 +52,9 @@ final class PhabricatorImageRemarkupRule extends PhutilRemarkupRule { array( 'src' => $src_uri, 'alt' => $args['alt'], - 'href' => $args['href'], 'width' => $args['width'], 'height' => $args['height'], - )); + )); return $this->getEngine()->storeText($img); } else { return $matches[0]; diff --git a/src/applications/harbormaster/artifact/HarbormasterURIArtifact.php b/src/applications/harbormaster/artifact/HarbormasterURIArtifact.php index 345621f0f5..93f7564033 100644 --- a/src/applications/harbormaster/artifact/HarbormasterURIArtifact.php +++ b/src/applications/harbormaster/artifact/HarbormasterURIArtifact.php @@ -81,6 +81,7 @@ final class HarbormasterURIArtifact extends HarbormasterArtifact { array( 'href' => $uri, 'target' => '_blank', + 'rel' => 'noreferrer', ), $name); } diff --git a/src/applications/nuance/item/NuanceGitHubEventItemType.php b/src/applications/nuance/item/NuanceGitHubEventItemType.php index b8bfb3ccab..1f6249f222 100644 --- a/src/applications/nuance/item/NuanceGitHubEventItemType.php +++ b/src/applications/nuance/item/NuanceGitHubEventItemType.php @@ -309,6 +309,8 @@ final class NuanceGitHubEventItemType 'a', array( 'href' => $event_uri, + 'target' => '_blank', + 'rel' => 'noreferrer', ), $event_uri); } diff --git a/src/applications/phurl/remarkup/PhabricatorPhurlLinkRemarkupRule.php b/src/applications/phurl/remarkup/PhabricatorPhurlLinkRemarkupRule.php index 4ef59b300c..c4ffc366da 100644 --- a/src/applications/phurl/remarkup/PhabricatorPhurlLinkRemarkupRule.php +++ b/src/applications/phurl/remarkup/PhabricatorPhurlLinkRemarkupRule.php @@ -64,6 +64,7 @@ final class PhabricatorPhurlLinkRemarkupRule extends PhutilRemarkupRule { array( 'href' => $uri, 'target' => '_blank', + 'rel' => 'noreferrer', ), $name); } diff --git a/src/applications/search/menuitem/PhabricatorLinkProfileMenuItem.php b/src/applications/search/menuitem/PhabricatorLinkProfileMenuItem.php index f790816c8b..0b6a2f330e 100644 --- a/src/applications/search/menuitem/PhabricatorLinkProfileMenuItem.php +++ b/src/applications/search/menuitem/PhabricatorLinkProfileMenuItem.php @@ -99,7 +99,8 @@ final class PhabricatorLinkProfileMenuItem ->setHref($href) ->setName($name) ->setIcon($icon_class) - ->setTooltip($tooltip); + ->setTooltip($tooltip) + ->setRel('noreferrer'); return array( $item, diff --git a/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldLink.php b/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldLink.php index c2b9d6543c..146f34ab07 100644 --- a/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldLink.php +++ b/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldLink.php @@ -31,7 +31,11 @@ final class PhabricatorStandardCustomFieldLink return phutil_tag( 'a', - array('href' => $value, 'target' => '_blank'), + array( + 'href' => $value, + 'target' => '_blank', + 'rel' => 'noreferrer', + ), $value); } diff --git a/src/view/layout/PhabricatorActionView.php b/src/view/layout/PhabricatorActionView.php index f6de8eca5b..a1d8fe2664 100644 --- a/src/view/layout/PhabricatorActionView.php +++ b/src/view/layout/PhabricatorActionView.php @@ -255,8 +255,10 @@ final class PhabricatorActionView extends AphrontView { } else { if ($this->getOpenInNewWindow()) { $target = '_blank'; + $rel = 'noreferrer'; } else { $target = null; + $rel = null; } if ($this->submenu) { @@ -277,6 +279,7 @@ final class PhabricatorActionView extends AphrontView { 'href' => $this->getHref(), 'class' => 'phabricator-action-view-item', 'target' => $target, + 'rel' => $rel, 'sigil' => $sigils, 'meta' => $this->metadata, ), diff --git a/src/view/phui/PHUIListItemView.php b/src/view/phui/PHUIListItemView.php index e8a1940737..8e53024826 100644 --- a/src/view/phui/PHUIListItemView.php +++ b/src/view/phui/PHUIListItemView.php @@ -34,6 +34,7 @@ final class PHUIListItemView extends AphrontTagView { private $actionIcon; private $actionIconHref; private $count; + private $rel; public function setOpenInNewWindow($open_in_new_window) { $this->openInNewWindow = $open_in_new_window; @@ -44,7 +45,16 @@ final class PHUIListItemView extends AphrontTagView { return $this->openInNewWindow; } - public function setHideInApplicationMenu($hide) { + public function setRel($rel) { + $this->rel = $rel; + return $this; + } + + public function getRel() { + return $this->rel; + } + + public function setHideInApplicationMenu($hide) { $this->hideInApplicationMenu = $hide; return $this; } @@ -363,6 +373,7 @@ final class PHUIListItemView extends AphrontTagView { 'meta' => $meta, 'sigil' => $sigil, 'target' => $this->getOpenInNewWindow() ? '_blank' : null, + 'rel' => $this->rel, ), array( $aural, diff --git a/src/view/phui/PHUITagView.php b/src/view/phui/PHUITagView.php index 292246c4a7..482b3f4400 100644 --- a/src/view/phui/PHUITagView.php +++ b/src/view/phui/PHUITagView.php @@ -154,25 +154,30 @@ final class PHUITagView extends AphrontTagView { $classes[] = 'phui-tag-'.$this->border; } - if ($this->phid) { - Javelin::initBehavior('phui-hovercards'); + $attributes = array( + 'href' => $this->href, + 'class' => $classes, + ); - $attributes = array( - 'href' => $this->href, - 'sigil' => 'hovercard', - 'meta' => array( - 'hoverPHID' => $this->phid, - ), - 'target' => $this->external ? '_blank' : null, - ); - } else { - $attributes = array( - 'href' => $this->href, - 'target' => $this->external ? '_blank' : null, + if ($this->external) { + $attributes += array( + 'target' => '_blank', + 'rel' => 'noreferrer', ); } - return $attributes + array('class' => $classes); + if ($this->phid) { + Javelin::initBehavior('phui-hovercards'); + + $attributes += array( + 'sigil' => 'hovercard', + 'meta' => array( + 'hoverPHID' => $this->phid, + ), + ); + } + + return $attributes; } protected function getTagContent() { From 0dee34b3fafd2d7fd5011ea23fae08f9bb8eabf5 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 18 Feb 2018 06:31:28 -0800 Subject: [PATCH 03/13] Make Facts more modern, DRY, and dimensional Summary: Ref T13083. Facts has a fair amount of weird hardcoding and duplication of responsibilities. Reduce this somewhat: no more hard-coded fact aggregates, no more database-driven list of available facts, etc. Generally, derive all objective truth from FactEngines. This is more similar to how most other modern applications work. For clarity, hopefully: rename "FactSpec" to "Fact". Rename "RawFact" to "Datapoint". Split the fairly optimistic "RawFact" table into an "IntDatapoint" table with less stuff in it, then dimension tables for the object PHIDs and key names. This is primarily aimed at reducing the row size of each datapoint. At the time I originally wrote this code we hadn't experimented much with storing similar data in multiple tables, but this is now more common and has worked well elsewhere (CustomFields, Edges, Ferret) so I don't anticipate this causing issues. If we need more complex or multidimension/multivalue tables later we can accommodate them. The queries a single table supports (like "all facts of all kinds in some time window") don't make any sense as far as I can tell and could likely be UNION ALL'd anyway. Remove all the aggregation stuff for now, it's not really clear to me what this should look like. Test Plan: Ran `bin/fact analyze` and viewed web UI. Nothing exploded too violently. Subscribers: yelirekim Maniphest Tasks: T13083 Differential Revision: https://secure.phabricator.com/D19119 --- .../autopatches/20180218.fact.01.dim.key.sql | 5 + .../autopatches/20180218.fact.02.dim.obj.sql | 5 + .../autopatches/20180218.fact.03.data.int.sql | 8 ++ src/__phutil_library_map__.php | 24 ++-- .../PhabricatorDifferentialApplication.php | 6 - .../PhabricatorDiffusionApplication.php | 6 - .../PhabricatorFactChartController.php | 29 ++-- .../PhabricatorFactHomeController.php | 79 +---------- .../fact/daemon/PhabricatorFactDaemon.php | 134 ++++++++---------- .../engine/PhabricatorFactCountEngine.php | 86 ----------- .../fact/engine/PhabricatorFactEngine.php | 35 +++-- .../PhabricatorFactLastUpdatedEngine.php | 34 ----- .../PhabricatorFactManiphestTaskEngine.php | 34 +++++ .../fact/fact/PhabricatorFact.php | 40 ++++++ .../fact/fact/PhabricatorPointsFact.php | 9 ++ ...abricatorFactManagementAnalyzeWorkflow.php | 4 - ...abricatorFactManagementDestroyWorkflow.php | 9 +- ...habricatorFactManagementStatusWorkflow.php | 47 ------ .../fact/spec/PhabricatorFactSimpleSpec.php | 38 ----- .../fact/spec/PhabricatorFactSpec.php | 53 ------- .../fact/storage/PhabricatorFactDimension.php | 85 +++++++++++ .../storage/PhabricatorFactIntDatapoint.php | 61 ++++++++ .../storage/PhabricatorFactKeyDimension.php | 27 ++++ .../PhabricatorFactObjectDimension.php | 25 ++++ .../PhabricatorPonderApplication.php | 6 - 25 files changed, 422 insertions(+), 467 deletions(-) create mode 100644 resources/sql/autopatches/20180218.fact.01.dim.key.sql create mode 100644 resources/sql/autopatches/20180218.fact.02.dim.obj.sql create mode 100644 resources/sql/autopatches/20180218.fact.03.data.int.sql delete mode 100644 src/applications/fact/engine/PhabricatorFactCountEngine.php delete mode 100644 src/applications/fact/engine/PhabricatorFactLastUpdatedEngine.php create mode 100644 src/applications/fact/engine/PhabricatorFactManiphestTaskEngine.php create mode 100644 src/applications/fact/fact/PhabricatorFact.php create mode 100644 src/applications/fact/fact/PhabricatorPointsFact.php delete mode 100644 src/applications/fact/management/PhabricatorFactManagementStatusWorkflow.php delete mode 100644 src/applications/fact/spec/PhabricatorFactSimpleSpec.php delete mode 100644 src/applications/fact/spec/PhabricatorFactSpec.php create mode 100644 src/applications/fact/storage/PhabricatorFactDimension.php create mode 100644 src/applications/fact/storage/PhabricatorFactIntDatapoint.php create mode 100644 src/applications/fact/storage/PhabricatorFactKeyDimension.php create mode 100644 src/applications/fact/storage/PhabricatorFactObjectDimension.php diff --git a/resources/sql/autopatches/20180218.fact.01.dim.key.sql b/resources/sql/autopatches/20180218.fact.01.dim.key.sql new file mode 100644 index 0000000000..3a81915026 --- /dev/null +++ b/resources/sql/autopatches/20180218.fact.01.dim.key.sql @@ -0,0 +1,5 @@ +CREATE TABLE {$NAMESPACE}_fact.fact_keydimension ( + id INT UNSIGNED NOT NULL AUTO_INCREMENT PRIMARY KEY, + factKey VARCHAR(64) NOT NULL COLLATE {$COLLATE_TEXT}, + UNIQUE KEY `key_factkey` (factKey) +) ENGINE=InnoDB, COLLATE {$COLLATE_TEXT}; diff --git a/resources/sql/autopatches/20180218.fact.02.dim.obj.sql b/resources/sql/autopatches/20180218.fact.02.dim.obj.sql new file mode 100644 index 0000000000..6b38062b29 --- /dev/null +++ b/resources/sql/autopatches/20180218.fact.02.dim.obj.sql @@ -0,0 +1,5 @@ +CREATE TABLE {$NAMESPACE}_fact.fact_objectdimension ( + id INT UNSIGNED NOT NULL AUTO_INCREMENT PRIMARY KEY, + objectPHID VARBINARY(64) NOT NULL, + UNIQUE KEY `key_object` (objectPHID) +) ENGINE=InnoDB, COLLATE {$COLLATE_TEXT}; diff --git a/resources/sql/autopatches/20180218.fact.03.data.int.sql b/resources/sql/autopatches/20180218.fact.03.data.int.sql new file mode 100644 index 0000000000..d93d546733 --- /dev/null +++ b/resources/sql/autopatches/20180218.fact.03.data.int.sql @@ -0,0 +1,8 @@ +CREATE TABLE {$NAMESPACE}_fact.fact_intdatapoint ( + id BIGINT UNSIGNED NOT NULL AUTO_INCREMENT PRIMARY KEY, + keyID INT UNSIGNED NOT NULL, + objectID INT UNSIGNED NOT NULL, + dimensionID INT UNSIGNED, + value BIGINT SIGNED NOT NULL, + epoch INT UNSIGNED NOT NULL +) ENGINE=InnoDB, COLLATE {$COLLATE_TEXT}; diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 29a957ae6e..3fe0cba001 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -2907,27 +2907,28 @@ phutil_register_library_map(array( 'PhabricatorExternalAccountsSettingsPanel' => 'applications/settings/panel/PhabricatorExternalAccountsSettingsPanel.php', 'PhabricatorExtraConfigSetupCheck' => 'applications/config/check/PhabricatorExtraConfigSetupCheck.php', 'PhabricatorFacebookAuthProvider' => 'applications/auth/provider/PhabricatorFacebookAuthProvider.php', + 'PhabricatorFact' => 'applications/fact/fact/PhabricatorFact.php', 'PhabricatorFactAggregate' => 'applications/fact/storage/PhabricatorFactAggregate.php', 'PhabricatorFactApplication' => 'applications/fact/application/PhabricatorFactApplication.php', 'PhabricatorFactChartController' => 'applications/fact/controller/PhabricatorFactChartController.php', 'PhabricatorFactController' => 'applications/fact/controller/PhabricatorFactController.php', - 'PhabricatorFactCountEngine' => 'applications/fact/engine/PhabricatorFactCountEngine.php', 'PhabricatorFactCursor' => 'applications/fact/storage/PhabricatorFactCursor.php', 'PhabricatorFactDAO' => 'applications/fact/storage/PhabricatorFactDAO.php', 'PhabricatorFactDaemon' => 'applications/fact/daemon/PhabricatorFactDaemon.php', + 'PhabricatorFactDimension' => 'applications/fact/storage/PhabricatorFactDimension.php', 'PhabricatorFactEngine' => 'applications/fact/engine/PhabricatorFactEngine.php', 'PhabricatorFactEngineTestCase' => 'applications/fact/engine/__tests__/PhabricatorFactEngineTestCase.php', 'PhabricatorFactHomeController' => 'applications/fact/controller/PhabricatorFactHomeController.php', - 'PhabricatorFactLastUpdatedEngine' => 'applications/fact/engine/PhabricatorFactLastUpdatedEngine.php', + 'PhabricatorFactIntDatapoint' => 'applications/fact/storage/PhabricatorFactIntDatapoint.php', + 'PhabricatorFactKeyDimension' => 'applications/fact/storage/PhabricatorFactKeyDimension.php', 'PhabricatorFactManagementAnalyzeWorkflow' => 'applications/fact/management/PhabricatorFactManagementAnalyzeWorkflow.php', 'PhabricatorFactManagementCursorsWorkflow' => 'applications/fact/management/PhabricatorFactManagementCursorsWorkflow.php', 'PhabricatorFactManagementDestroyWorkflow' => 'applications/fact/management/PhabricatorFactManagementDestroyWorkflow.php', 'PhabricatorFactManagementListWorkflow' => 'applications/fact/management/PhabricatorFactManagementListWorkflow.php', - 'PhabricatorFactManagementStatusWorkflow' => 'applications/fact/management/PhabricatorFactManagementStatusWorkflow.php', 'PhabricatorFactManagementWorkflow' => 'applications/fact/management/PhabricatorFactManagementWorkflow.php', + 'PhabricatorFactManiphestTaskEngine' => 'applications/fact/engine/PhabricatorFactManiphestTaskEngine.php', + 'PhabricatorFactObjectDimension' => 'applications/fact/storage/PhabricatorFactObjectDimension.php', 'PhabricatorFactRaw' => 'applications/fact/storage/PhabricatorFactRaw.php', - 'PhabricatorFactSimpleSpec' => 'applications/fact/spec/PhabricatorFactSimpleSpec.php', - 'PhabricatorFactSpec' => 'applications/fact/spec/PhabricatorFactSpec.php', 'PhabricatorFactUpdateIterator' => 'applications/fact/extract/PhabricatorFactUpdateIterator.php', 'PhabricatorFavoritesApplication' => 'applications/favorites/application/PhabricatorFavoritesApplication.php', 'PhabricatorFavoritesController' => 'applications/favorites/controller/PhabricatorFavoritesController.php', @@ -3716,6 +3717,7 @@ phutil_register_library_map(array( 'PhabricatorPirateEnglishTranslation' => 'infrastructure/internationalization/translation/PhabricatorPirateEnglishTranslation.php', 'PhabricatorPlatformSite' => 'aphront/site/PhabricatorPlatformSite.php', 'PhabricatorPointsEditField' => 'applications/transactions/editfield/PhabricatorPointsEditField.php', + 'PhabricatorPointsFact' => 'applications/fact/fact/PhabricatorPointsFact.php', 'PhabricatorPolicies' => 'applications/policy/constants/PhabricatorPolicies.php', 'PhabricatorPolicy' => 'applications/policy/storage/PhabricatorPolicy.php', 'PhabricatorPolicyApplication' => 'applications/policy/application/PhabricatorPolicyApplication.php', @@ -8433,27 +8435,28 @@ phutil_register_library_map(array( 'PhabricatorExternalAccountsSettingsPanel' => 'PhabricatorSettingsPanel', 'PhabricatorExtraConfigSetupCheck' => 'PhabricatorSetupCheck', 'PhabricatorFacebookAuthProvider' => 'PhabricatorOAuth2AuthProvider', + 'PhabricatorFact' => 'Phobject', 'PhabricatorFactAggregate' => 'PhabricatorFactDAO', 'PhabricatorFactApplication' => 'PhabricatorApplication', 'PhabricatorFactChartController' => 'PhabricatorFactController', 'PhabricatorFactController' => 'PhabricatorController', - 'PhabricatorFactCountEngine' => 'PhabricatorFactEngine', 'PhabricatorFactCursor' => 'PhabricatorFactDAO', 'PhabricatorFactDAO' => 'PhabricatorLiskDAO', 'PhabricatorFactDaemon' => 'PhabricatorDaemon', + 'PhabricatorFactDimension' => 'PhabricatorFactDAO', 'PhabricatorFactEngine' => 'Phobject', 'PhabricatorFactEngineTestCase' => 'PhabricatorTestCase', 'PhabricatorFactHomeController' => 'PhabricatorFactController', - 'PhabricatorFactLastUpdatedEngine' => 'PhabricatorFactEngine', + 'PhabricatorFactIntDatapoint' => 'PhabricatorFactDAO', + 'PhabricatorFactKeyDimension' => 'PhabricatorFactDimension', 'PhabricatorFactManagementAnalyzeWorkflow' => 'PhabricatorFactManagementWorkflow', 'PhabricatorFactManagementCursorsWorkflow' => 'PhabricatorFactManagementWorkflow', 'PhabricatorFactManagementDestroyWorkflow' => 'PhabricatorFactManagementWorkflow', 'PhabricatorFactManagementListWorkflow' => 'PhabricatorFactManagementWorkflow', - 'PhabricatorFactManagementStatusWorkflow' => 'PhabricatorFactManagementWorkflow', 'PhabricatorFactManagementWorkflow' => 'PhabricatorManagementWorkflow', + 'PhabricatorFactManiphestTaskEngine' => 'PhabricatorFactEngine', + 'PhabricatorFactObjectDimension' => 'PhabricatorFactDimension', 'PhabricatorFactRaw' => 'PhabricatorFactDAO', - 'PhabricatorFactSimpleSpec' => 'PhabricatorFactSpec', - 'PhabricatorFactSpec' => 'Phobject', 'PhabricatorFactUpdateIterator' => 'PhutilBufferedIterator', 'PhabricatorFavoritesApplication' => 'PhabricatorApplication', 'PhabricatorFavoritesController' => 'PhabricatorController', @@ -9373,6 +9376,7 @@ phutil_register_library_map(array( 'PhabricatorPirateEnglishTranslation' => 'PhutilTranslation', 'PhabricatorPlatformSite' => 'PhabricatorSite', 'PhabricatorPointsEditField' => 'PhabricatorEditField', + 'PhabricatorPointsFact' => 'PhabricatorFact', 'PhabricatorPolicies' => 'PhabricatorPolicyConstants', 'PhabricatorPolicy' => array( 'PhabricatorPolicyDAO', diff --git a/src/applications/differential/application/PhabricatorDifferentialApplication.php b/src/applications/differential/application/PhabricatorDifferentialApplication.php index 1c8926f585..4141cf8539 100644 --- a/src/applications/differential/application/PhabricatorDifferentialApplication.php +++ b/src/applications/differential/application/PhabricatorDifferentialApplication.php @@ -35,12 +35,6 @@ final class PhabricatorDifferentialApplication extends PhabricatorApplication { ); } - public function getFactObjectsForAnalysis() { - return array( - new DifferentialRevision(), - ); - } - public function getTitleGlyph() { return "\xE2\x9A\x99"; } diff --git a/src/applications/diffusion/application/PhabricatorDiffusionApplication.php b/src/applications/diffusion/application/PhabricatorDiffusionApplication.php index e619ecb1ad..d42e58b747 100644 --- a/src/applications/diffusion/application/PhabricatorDiffusionApplication.php +++ b/src/applications/diffusion/application/PhabricatorDiffusionApplication.php @@ -39,12 +39,6 @@ final class PhabricatorDiffusionApplication extends PhabricatorApplication { ); } - public function getFactObjectsForAnalysis() { - return array( - new PhabricatorRepositoryCommit(), - ); - } - public function getRemarkupRules() { return array( new DiffusionCommitRemarkupRule(), diff --git a/src/applications/fact/controller/PhabricatorFactChartController.php b/src/applications/fact/controller/PhabricatorFactChartController.php index 16df8fca69..b6ae64b4b5 100644 --- a/src/applications/fact/controller/PhabricatorFactChartController.php +++ b/src/applications/fact/controller/PhabricatorFactChartController.php @@ -5,27 +5,32 @@ final class PhabricatorFactChartController extends PhabricatorFactController { public function handleRequest(AphrontRequest $request) { $viewer = $request->getViewer(); - $table = new PhabricatorFactRaw(); + $series = $request->getStr('y1'); + + $facts = PhabricatorFact::getAllFacts(); + $fact = idx($facts, $series); + + if (!$fact) { + return new Aphront404Response(); + } + + $key_id = id(new PhabricatorFactKeyDimension()) + ->newDimensionID($fact->getKey()); + + $table = $fact->newDatapoint(); $conn_r = $table->establishConnection('r'); $table_name = $table->getTableName(); - $series = $request->getStr('y1'); - - $specs = PhabricatorFactSpec::newSpecsForFactTypes( - PhabricatorFactEngine::loadAllEngines(), - array($series)); - $spec = idx($specs, $series); - $data = queryfx_all( $conn_r, - 'SELECT valueX, epoch FROM %T WHERE factType = %s ORDER BY epoch ASC', + 'SELECT value, epoch FROM %T WHERE keyID = %d ORDER BY epoch ASC', $table_name, - $series); + $key_id); $points = array(); $sum = 0; foreach ($data as $key => $row) { - $sum += (int)$row['valueX']; + $sum += (int)$row['value']; $points[(int)$row['epoch']] = $sum; } @@ -71,7 +76,7 @@ final class PhabricatorFactChartController extends PhabricatorFactController { )); $box = id(new PHUIObjectBoxView()) - ->setHeaderText(pht('Count of %s', $spec->getName())) + ->setHeaderText(pht('Count of %s', $fact->getName())) ->appendChild($chart); $crumbs = $this->buildApplicationCrumbs(); diff --git a/src/applications/fact/controller/PhabricatorFactHomeController.php b/src/applications/fact/controller/PhabricatorFactHomeController.php index f4eeca0387..82f6a0905b 100644 --- a/src/applications/fact/controller/PhabricatorFactHomeController.php +++ b/src/applications/fact/controller/PhabricatorFactHomeController.php @@ -15,45 +15,6 @@ final class PhabricatorFactHomeController extends PhabricatorFactController { return id(new AphrontRedirectResponse())->setURI($uri); } - $types = array( - '+N:*', - '+N:DREV', - 'updated', - ); - - $engines = PhabricatorFactEngine::loadAllEngines(); - $specs = PhabricatorFactSpec::newSpecsForFactTypes($engines, $types); - - $facts = id(new PhabricatorFactAggregate())->loadAllWhere( - 'factType IN (%Ls)', - $types); - - $rows = array(); - foreach ($facts as $fact) { - $spec = $specs[$fact->getFactType()]; - - $name = $spec->getName(); - $value = $spec->formatValueForDisplay($viewer, $fact->getValueX()); - - $rows[] = array($name, $value); - } - - $table = new AphrontTableView($rows); - $table->setHeaders( - array( - pht('Fact'), - pht('Value'), - )); - $table->setColumnClasses( - array( - 'wide', - 'n', - )); - - $panel = new PHUIObjectBoxView(); - $panel->setHeaderText(pht('Facts')); - $panel->setTable($table); - $chart_form = $this->buildChartForm(); $crumbs = $this->buildApplicationCrumbs(); @@ -64,46 +25,18 @@ final class PhabricatorFactHomeController extends PhabricatorFactController { return $this->newPage() ->setTitle($title) ->setCrumbs($crumbs) - ->appendChild(array( - $chart_form, - $panel, - )); - + ->appendChild( + array( + $chart_form, + )); } private function buildChartForm() { $request = $this->getRequest(); $viewer = $request->getUser(); - $table = new PhabricatorFactRaw(); - $conn_r = $table->establishConnection('r'); - $table_name = $table->getTableName(); - - $facts = queryfx_all( - $conn_r, - 'SELECT DISTINCT factType from %T', - $table_name); - - $specs = PhabricatorFactSpec::newSpecsForFactTypes( - PhabricatorFactEngine::loadAllEngines(), - ipull($facts, 'factType')); - - $options = array(); - foreach ($specs as $spec) { - if ($spec->getUnit() == PhabricatorFactSpec::UNIT_COUNT) { - $options[$spec->getType()] = $spec->getName(); - } - } - - if (!$options) { - return id(new PHUIInfoView()) - ->setSeverity(PHUIInfoView::SEVERITY_NODATA) - ->setTitle(pht('No Chartable Facts')) - ->appendChild(phutil_tag( - 'p', - array(), - pht('There are no facts that can be plotted yet.'))); - } + $specs = PhabricatorFact::getAllFacts(); + $options = mpull($specs, 'getName', 'getKey'); $form = id(new AphrontFormView()) ->setUser($viewer) diff --git a/src/applications/fact/daemon/PhabricatorFactDaemon.php b/src/applications/fact/daemon/PhabricatorFactDaemon.php index 384bb56df4..8a9214c40b 100644 --- a/src/applications/fact/daemon/PhabricatorFactDaemon.php +++ b/src/applications/fact/daemon/PhabricatorFactDaemon.php @@ -4,8 +4,6 @@ final class PhabricatorFactDaemon extends PhabricatorDaemon { private $engines; - const RAW_FACT_BUFFER_LIMIT = 128; - protected function run() { $this->setEngines(PhabricatorFactEngine::loadAllEngines()); while (!$this->shouldExit()) { @@ -15,7 +13,6 @@ final class PhabricatorFactDaemon extends PhabricatorDaemon { foreach ($iterators as $iterator_name => $iterator) { $this->processIteratorWithCursor($iterator_name, $iterator); } - $this->processAggregates(); $this->log(pht('Zzz...')); $this->sleep(60 * 5); @@ -72,59 +69,41 @@ final class PhabricatorFactDaemon extends PhabricatorDaemon { public function processIterator($iterator) { $result = null; - $raw_facts = array(); + $datapoints = array(); foreach ($iterator as $key => $object) { $phid = $object->getPHID(); $this->log(pht('Processing %s...', $phid)); - $raw_facts[$phid] = $this->computeRawFacts($object); - if (count($raw_facts) > self::RAW_FACT_BUFFER_LIMIT) { - $this->updateRawFacts($raw_facts); - $raw_facts = array(); + $datapoints[$phid] = $this->newDatapoints($object); + if (count($datapoints) > 1024) { + $this->updateDatapoints($datapoints); + $datapoints = array(); } $result = $key; } - if ($raw_facts) { - $this->updateRawFacts($raw_facts); - $raw_facts = array(); + if ($datapoints) { + $this->updateDatapoints($datapoints); + $datapoints = array(); } return $result; } - public function processAggregates() { - $this->log(pht('Processing aggregates.')); - - $facts = $this->computeAggregateFacts(); - $this->updateAggregateFacts($facts); - } - - private function computeAggregateFacts() { + private function newDatapoints(PhabricatorLiskDAO $object) { $facts = array(); foreach ($this->engines as $engine) { - if (!$engine->shouldComputeAggregateFacts()) { + if (!$engine->supportsDatapointsForObject($object)) { continue; } - $facts[] = $engine->computeAggregateFacts(); - } - return array_mergev($facts); - } - - private function computeRawFacts(PhabricatorLiskDAO $object) { - $facts = array(); - foreach ($this->engines as $engine) { - if (!$engine->shouldComputeRawFactsForObject($object)) { - continue; - } - $facts[] = $engine->computeRawFactsForObject($object); + $facts[] = $engine->newDatapointsForObject($object); } return array_mergev($facts); } - private function updateRawFacts(array $map) { + private function updateDatapoints(array $map) { foreach ($map as $phid => $facts) { - assert_instances_of($facts, 'PhabricatorFactRaw'); + assert_instances_of($facts, 'PhabricatorFactIntDatapoint'); } $phids = array_keys($map); @@ -132,76 +111,79 @@ final class PhabricatorFactDaemon extends PhabricatorDaemon { return; } - $table = new PhabricatorFactRaw(); + + $fact_keys = array(); + $objects = array(); + foreach ($map as $phid => $facts) { + foreach ($facts as $fact) { + $fact_keys[$fact->getKey()] = true; + + $object_phid = $fact->getObjectPHID(); + $objects[$object_phid] = $object_phid; + + $dimension_phid = $fact->getDimensionPHID(); + if ($dimension_phid !== null) { + $objects[$dimension_phid] = $dimension_phid; + } + } + } + + $key_map = id(new PhabricatorFactKeyDimension()) + ->newDimensionMap(array_keys($fact_keys)); + $object_map = id(new PhabricatorFactObjectDimension()) + ->newDimensionMap(array_keys($objects)); + + $table = new PhabricatorFactIntDatapoint(); $conn = $table->establishConnection('w'); $table_name = $table->getTableName(); $sql = array(); foreach ($map as $phid => $facts) { foreach ($facts as $fact) { + $key_id = $key_map[$fact->getKey()]; + $object_id = $object_map[$fact->getObjectPHID()]; + + $dimension_phid = $fact->getDimensionPHID(); + if ($dimension_phid !== null) { + $dimension_id = $object_map[$dimension_phid]; + } else { + $dimension_id = null; + } + $sql[] = qsprintf( $conn, - '(%s, %s, %s, %d, %d, %d)', - $fact->getFactType(), - $fact->getObjectPHID(), - $fact->getObjectA(), - $fact->getValueX(), - $fact->getValueY(), + '(%d, %d, %nd, %d, %d)', + $key_id, + $object_id, + $dimension_id, + $fact->getValue(), $fact->getEpoch()); } } + $rebuilt_ids = array_select_keys($object_map, $phids); + $table->openTransaction(); queryfx( $conn, - 'DELETE FROM %T WHERE objectPHID IN (%Ls)', + 'DELETE FROM %T WHERE objectID IN (%Ld)', $table_name, - $phids); + $rebuilt_ids); if ($sql) { - foreach (array_chunk($sql, 256) as $chunk) { + foreach (PhabricatorLiskDAO::chunkSQL($sql) as $chunk) { queryfx( $conn, 'INSERT INTO %T - (factType, objectPHID, objectA, valueX, valueY, epoch) + (keyID, objectID, dimensionID, value, epoch) VALUES %Q', $table_name, - implode(', ', $chunk)); + $chunk); } } $table->saveTransaction(); } - private function updateAggregateFacts(array $facts) { - if (!$facts) { - return; - } - - $table = new PhabricatorFactAggregate(); - $conn = $table->establishConnection('w'); - $table_name = $table->getTableName(); - - $sql = array(); - foreach ($facts as $fact) { - $sql[] = qsprintf( - $conn, - '(%s, %s, %d)', - $fact->getFactType(), - $fact->getObjectPHID(), - $fact->getValueX()); - } - - foreach (array_chunk($sql, 256) as $chunk) { - queryfx( - $conn, - 'INSERT INTO %T (factType, objectPHID, valueX) VALUES %Q - ON DUPLICATE KEY UPDATE valueX = VALUES(valueX)', - $table_name, - implode(', ', $chunk)); - } - - } - } diff --git a/src/applications/fact/engine/PhabricatorFactCountEngine.php b/src/applications/fact/engine/PhabricatorFactCountEngine.php deleted file mode 100644 index f24068646d..0000000000 --- a/src/applications/fact/engine/PhabricatorFactCountEngine.php +++ /dev/null @@ -1,86 +0,0 @@ -setName($name) - ->setUnit(PhabricatorFactSimpleSpec::UNIT_COUNT); - } - - if (!strncmp($type, 'N:', 2)) { - if ($type == 'N:*') { - $name = pht('Objects'); - } else { - $name = pht('Objects of type %s', substr($type, 2)); - } - $results[] = id(new PhabricatorFactSimpleSpec($type)) - ->setName($name) - ->setUnit(PhabricatorFactSimpleSpec::UNIT_COUNT); - } - - } - return $results; - } - - public function shouldComputeRawFactsForObject(PhabricatorLiskDAO $object) { - return true; - } - - public function computeRawFactsForObject(PhabricatorLiskDAO $object) { - $facts = array(); - - $phid = $object->getPHID(); - $type = phid_get_type($phid); - - foreach (array('N:*', 'N:'.$type) as $fact_type) { - $facts[] = id(new PhabricatorFactRaw()) - ->setFactType($fact_type) - ->setObjectPHID($phid) - ->setValueX(1) - ->setEpoch($object->getDateCreated()); - } - - return $facts; - } - - public function shouldComputeAggregateFacts() { - return true; - } - - public function computeAggregateFacts() { - $table = new PhabricatorFactRaw(); - $table_name = $table->getTableName(); - $conn = $table->establishConnection('r'); - - $counts = queryfx_all( - $conn, - 'SELECT factType, SUM(valueX) N FROM %T WHERE factType LIKE %> - GROUP BY factType', - $table_name, - 'N:'); - - $facts = array(); - foreach ($counts as $count) { - $facts[] = id(new PhabricatorFactAggregate()) - ->setFactType('+'.$count['factType']) - ->setValueX($count['N']); - } - - return $facts; - } - - -} diff --git a/src/applications/fact/engine/PhabricatorFactEngine.php b/src/applications/fact/engine/PhabricatorFactEngine.php index a87cabf56d..b8389f7b23 100644 --- a/src/applications/fact/engine/PhabricatorFactEngine.php +++ b/src/applications/fact/engine/PhabricatorFactEngine.php @@ -2,30 +2,37 @@ abstract class PhabricatorFactEngine extends Phobject { + private $factMap; + final public static function loadAllEngines() { return id(new PhutilClassMapQuery()) ->setAncestorClass(__CLASS__) ->execute(); } - public function getFactSpecs(array $fact_types) { - return array(); - } + abstract public function newFacts(); - public function shouldComputeRawFactsForObject(PhabricatorLiskDAO $object) { - return false; - } + abstract public function supportsDatapointsForObject( + PhabricatorLiskDAO $object); - public function computeRawFactsForObject(PhabricatorLiskDAO $object) { - return array(); - } + abstract public function newDatapointsForObject(PhabricatorLiskDAO $object); - public function shouldComputeAggregateFacts() { - return false; - } + final protected function getFact($key) { + if ($this->factMap === null) { + $facts = $this->newFacts(); + $facts = mpull($facts, null, 'getKey'); + $this->factMap = $facts; + } - public function computeAggregateFacts() { - return array(); + if (!isset($this->factMap[$key])) { + throw new Exception( + pht( + 'Unknown fact ("%s") for engine "%s".', + $key, + get_class($this))); + } + + return $this->factMap[$key]; } } diff --git a/src/applications/fact/engine/PhabricatorFactLastUpdatedEngine.php b/src/applications/fact/engine/PhabricatorFactLastUpdatedEngine.php deleted file mode 100644 index 5ea99e6232..0000000000 --- a/src/applications/fact/engine/PhabricatorFactLastUpdatedEngine.php +++ /dev/null @@ -1,34 +0,0 @@ -setName(pht('Facts Last Updated')) - ->setUnit(PhabricatorFactSimpleSpec::UNIT_EPOCH); - } - } - return $results; - } - - public function shouldComputeAggregateFacts() { - return true; - } - - public function computeAggregateFacts() { - $facts = array(); - - $facts[] = id(new PhabricatorFactAggregate()) - ->setFactType('updated') - ->setValueX(time()); - - return $facts; - } - -} diff --git a/src/applications/fact/engine/PhabricatorFactManiphestTaskEngine.php b/src/applications/fact/engine/PhabricatorFactManiphestTaskEngine.php new file mode 100644 index 0000000000..c2ed81d72b --- /dev/null +++ b/src/applications/fact/engine/PhabricatorFactManiphestTaskEngine.php @@ -0,0 +1,34 @@ +setKey('tasks.count.open'), + ); + } + + public function supportsDatapointsForObject(PhabricatorLiskDAO $object) { + return ($object instanceof ManiphestTask); + } + + public function newDatapointsForObject(PhabricatorLiskDAO $object) { + $datapoints = array(); + + $phid = $object->getPHID(); + $type = phid_get_type($phid); + + $datapoint = $this->getFact('tasks.count.open') + ->newDatapoint(); + + $datapoints[] = $datapoint + ->setObjectPHID($phid) + ->setValue(1) + ->setEpoch($object->getDateCreated()); + + return $datapoints; + } + +} diff --git a/src/applications/fact/fact/PhabricatorFact.php b/src/applications/fact/fact/PhabricatorFact.php new file mode 100644 index 0000000000..2e33a029f3 --- /dev/null +++ b/src/applications/fact/fact/PhabricatorFact.php @@ -0,0 +1,40 @@ +newFacts(); + $facts = mpull($facts, null, 'getKey'); + $map += $facts; + } + + return $map; + } + + final public function setKey($key) { + $this->key = $key; + return $this; + } + + final public function getKey() { + return $this->key; + } + + final public function getName() { + return pht('Fact "%s"', $this->getKey()); + } + + final public function newDatapoint() { + return $this->newTemplateDatapoint() + ->setKey($this->getKey()); + } + + abstract protected function newTemplateDatapoint(); + +} diff --git a/src/applications/fact/fact/PhabricatorPointsFact.php b/src/applications/fact/fact/PhabricatorPointsFact.php new file mode 100644 index 0000000000..a80f45d132 --- /dev/null +++ b/src/applications/fact/fact/PhabricatorPointsFact.php @@ -0,0 +1,9 @@ +getArg('skip-aggregates')) { - $daemon->processAggregates(); - } - return 0; } diff --git a/src/applications/fact/management/PhabricatorFactManagementDestroyWorkflow.php b/src/applications/fact/management/PhabricatorFactManagementDestroyWorkflow.php index 8a2e5f1ed8..7a4997ab35 100644 --- a/src/applications/fact/management/PhabricatorFactManagementDestroyWorkflow.php +++ b/src/applications/fact/management/PhabricatorFactManagementDestroyWorkflow.php @@ -23,8 +23,13 @@ final class PhabricatorFactManagementDestroyWorkflow } $tables = array(); - $tables[] = new PhabricatorFactRaw(); - $tables[] = new PhabricatorFactAggregate(); + $tables[] = new PhabricatorFactCursor(); + + $tables[] = new PhabricatorFactIntDatapoint(); + + $tables[] = new PhabricatorFactObjectDimension(); + $tables[] = new PhabricatorFactKeyDimension(); + foreach ($tables as $table) { $conn = $table->establishConnection('w'); $name = $table->getTableName(); diff --git a/src/applications/fact/management/PhabricatorFactManagementStatusWorkflow.php b/src/applications/fact/management/PhabricatorFactManagementStatusWorkflow.php deleted file mode 100644 index 604a40b6ee..0000000000 --- a/src/applications/fact/management/PhabricatorFactManagementStatusWorkflow.php +++ /dev/null @@ -1,47 +0,0 @@ -setName('status') - ->setSynopsis(pht('Show status of fact data.')) - ->setArguments(array()); - } - - public function execute(PhutilArgumentParser $args) { - $console = PhutilConsole::getConsole(); - - $map = array( - 'raw' => new PhabricatorFactRaw(), - 'agg' => new PhabricatorFactAggregate(), - ); - - foreach ($map as $type => $table) { - $conn = $table->establishConnection('r'); - $name = $table->getTableName(); - - $row = queryfx_one( - $conn, - 'SELECT COUNT(*) N FROM %T', - $name); - - $n = $row['N']; - - switch ($type) { - case 'raw': - $desc = pht('There are %d raw fact(s) in storage.', $n); - break; - case 'agg': - $desc = pht('There are %d aggregate fact(s) in storage.', $n); - break; - } - - $console->writeOut("%s\n", $desc); - } - - return 0; - } - -} diff --git a/src/applications/fact/spec/PhabricatorFactSimpleSpec.php b/src/applications/fact/spec/PhabricatorFactSimpleSpec.php deleted file mode 100644 index 350b6367f1..0000000000 --- a/src/applications/fact/spec/PhabricatorFactSimpleSpec.php +++ /dev/null @@ -1,38 +0,0 @@ -type = $type; - } - - public function getType() { - return $this->type; - } - - public function setUnit($unit) { - $this->unit = $unit; - return $this; - } - - public function getUnit() { - return $this->unit; - } - - public function setName($name) { - $this->name = $name; - return $this; - } - - public function getName() { - if ($this->name !== null) { - return $this->name; - } - return parent::getName(); - } - -} diff --git a/src/applications/fact/spec/PhabricatorFactSpec.php b/src/applications/fact/spec/PhabricatorFactSpec.php deleted file mode 100644 index 47fcc01d8b..0000000000 --- a/src/applications/fact/spec/PhabricatorFactSpec.php +++ /dev/null @@ -1,53 +0,0 @@ -getFactSpecs($fact_types); - $specs = mpull($specs, null, 'getType'); - $map += $specs; - } - - foreach ($fact_types as $type) { - if (empty($map[$type])) { - $map[$type] = new PhabricatorFactSimpleSpec($type); - } - } - - return $map; - } - - abstract public function getType(); - - public function getUnit() { - return null; - } - - public function getName() { - return pht( - 'Fact (%s)', - $this->getType()); - } - - public function formatValueForDisplay(PhabricatorUser $user, $value) { - $unit = $this->getUnit(); - switch ($unit) { - case self::UNIT_COUNT: - return number_format($value); - case self::UNIT_EPOCH: - return phabricator_datetime($value, $user); - default: - return $value; - } - } - -} diff --git a/src/applications/fact/storage/PhabricatorFactDimension.php b/src/applications/fact/storage/PhabricatorFactDimension.php new file mode 100644 index 0000000000..d2bf2420dc --- /dev/null +++ b/src/applications/fact/storage/PhabricatorFactDimension.php @@ -0,0 +1,85 @@ +newDimensionMap(array($key)); + return $map[$key]; + } + + final public function newDimensionMap(array $keys) { + if (!$keys) { + return array(); + } + + $conn = $this->establishConnection('r'); + $column = $this->getDimensionColumnName(); + + $rows = queryfx_all( + $conn, + 'SELECT id, %C FROM %T WHERE %C IN (%Ls)', + $column, + $this->getTableName(), + $column, + $keys); + $rows = ipull($rows, 'id', $column); + + $map = array(); + $need = array(); + foreach ($keys as $key) { + if (isset($rows[$key])) { + $map[$key] = (int)$rows[$key]; + } else { + $need[] = $key; + } + } + + if (!$need) { + return $map; + } + + $sql = array(); + foreach ($need as $key) { + $sql[] = qsprintf( + $conn, + '(%s)', + $key); + } + + foreach (PhabricatorLiskDAO::chunkSQL($sql) as $chunk) { + queryfx( + $conn, + 'INSERT IGNORE INTO %T (%C) VALUES %Q', + $this->getTableName(), + $column, + $chunk); + } + + $rows = queryfx_all( + $conn, + 'SELECT id, %C FROM %T WHERE %C IN (%Ls)', + $column, + $this->getTableName(), + $column, + $need); + $rows = ipull($rows, 'id', $column); + + foreach ($keys as $key) { + if (isset($rows[$key])) { + $map[$key] = (int)$rows[$key]; + } else { + throw new Exception( + pht( + 'Failed to load or generate dimension ID ("%s") for dimension '. + 'key "%s".', + get_class($this), + $key)); + } + } + + return $map; + } + +} diff --git a/src/applications/fact/storage/PhabricatorFactIntDatapoint.php b/src/applications/fact/storage/PhabricatorFactIntDatapoint.php new file mode 100644 index 0000000000..7895052d5d --- /dev/null +++ b/src/applications/fact/storage/PhabricatorFactIntDatapoint.php @@ -0,0 +1,61 @@ + false, + self::CONFIG_COLUMN_SCHEMA => array( + 'id' => 'auto64', + 'dimensionID' => 'id?', + 'value' => 'sint64', + ), + self::CONFIG_KEY_SCHEMA => array( + 'key_dimension' => array( + 'columns' => array('keyID', 'dimensionID'), + ), + 'key_object' => array( + 'columns' => array('objectID'), + ), + ), + ) + parent::getConfiguration(); + } + + public function setKey($key) { + $this->key = $key; + return $this; + } + + public function getKey() { + return $this->key; + } + + public function setObjectPHID($object_phid) { + $this->objectPHID = $object_phid; + return $this; + } + + public function getObjectPHID() { + return $this->objectPHID; + } + + public function setDimensionPHID($dimension_phid) { + $this->dimensionPHID = $dimension_phid; + return $this; + } + + public function getDimensionPHID() { + return $this->dimensionPHID; + } + +} diff --git a/src/applications/fact/storage/PhabricatorFactKeyDimension.php b/src/applications/fact/storage/PhabricatorFactKeyDimension.php new file mode 100644 index 0000000000..b58ba94400 --- /dev/null +++ b/src/applications/fact/storage/PhabricatorFactKeyDimension.php @@ -0,0 +1,27 @@ + false, + self::CONFIG_COLUMN_SCHEMA => array( + 'factKey' => 'text64', + ), + self::CONFIG_KEY_SCHEMA => array( + 'key_factkey' => array( + 'columns' => array('factKey'), + 'unique' => true, + ), + ), + ) + parent::getConfiguration(); + } + + protected function getDimensionColumnName() { + return 'factKey'; + } + +} diff --git a/src/applications/fact/storage/PhabricatorFactObjectDimension.php b/src/applications/fact/storage/PhabricatorFactObjectDimension.php new file mode 100644 index 0000000000..e7319724a4 --- /dev/null +++ b/src/applications/fact/storage/PhabricatorFactObjectDimension.php @@ -0,0 +1,25 @@ + false, + self::CONFIG_COLUMN_SCHEMA => array(), + self::CONFIG_KEY_SCHEMA => array( + 'key_object' => array( + 'columns' => array('objectPHID'), + 'unique' => true, + ), + ), + ) + parent::getConfiguration(); + } + + protected function getDimensionColumnName() { + return 'objectPHID'; + } + +} diff --git a/src/applications/ponder/application/PhabricatorPonderApplication.php b/src/applications/ponder/application/PhabricatorPonderApplication.php index ed37c7ef6e..56973447f9 100644 --- a/src/applications/ponder/application/PhabricatorPonderApplication.php +++ b/src/applications/ponder/application/PhabricatorPonderApplication.php @@ -18,12 +18,6 @@ final class PhabricatorPonderApplication extends PhabricatorApplication { return 'fa-university'; } - public function getFactObjectsForAnalysis() { - return array( - new PonderQuestion(), - ); - } - public function getTitleGlyph() { return "\xE2\x97\xB3"; } From e3a1a32444d06ed7b42d616413695bccddd3468e Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 18 Feb 2018 09:18:50 -0800 Subject: [PATCH 04/13] Extract count/point data from tasks in Fact engines Summary: Depends on D19119. Ref T13083. This is probably still very buggy, but I'm planning to build support tools to make debugging facts easier shortly. This generates a large number of datapoints, at least, and can render some charts which aren't all completely broken in an obvious way. Test Plan: Ran `bin/fact analyze --all`, got some charts with lines that went up and down in the web UI. Subscribers: yelirekim Maniphest Tasks: T13083 Differential Revision: https://secure.phabricator.com/D19120 --- src/__phutil_library_map__.php | 2 + .../PhabricatorFactChartController.php | 3 + .../fact/daemon/PhabricatorFactDaemon.php | 19 +- .../fact/engine/PhabricatorFactEngine.php | 4 + .../PhabricatorFactManiphestTaskEngine.php | 388 +++++++++++++++++- .../fact/fact/PhabricatorCountFact.php | 9 + .../fact/storage/PhabricatorFactDimension.php | 10 +- .../PhabricatorApplicationTransaction.php | 5 + 8 files changed, 422 insertions(+), 18 deletions(-) create mode 100644 src/applications/fact/fact/PhabricatorCountFact.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 3fe0cba001..e3f7f64ada 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -2588,6 +2588,7 @@ phutil_register_library_map(array( 'PhabricatorCoreCreateTransaction' => 'applications/transactions/xaction/PhabricatorCoreCreateTransaction.php', 'PhabricatorCoreTransactionType' => 'applications/transactions/xaction/PhabricatorCoreTransactionType.php', 'PhabricatorCoreVoidTransaction' => 'applications/transactions/xaction/PhabricatorCoreVoidTransaction.php', + 'PhabricatorCountFact' => 'applications/fact/fact/PhabricatorCountFact.php', 'PhabricatorCountdown' => 'applications/countdown/storage/PhabricatorCountdown.php', 'PhabricatorCountdownApplication' => 'applications/countdown/application/PhabricatorCountdownApplication.php', 'PhabricatorCountdownController' => 'applications/countdown/controller/PhabricatorCountdownController.php', @@ -8078,6 +8079,7 @@ phutil_register_library_map(array( 'PhabricatorCoreCreateTransaction' => 'PhabricatorCoreTransactionType', 'PhabricatorCoreTransactionType' => 'PhabricatorModularTransactionType', 'PhabricatorCoreVoidTransaction' => 'PhabricatorModularTransactionType', + 'PhabricatorCountFact' => 'PhabricatorFact', 'PhabricatorCountdown' => array( 'PhabricatorCountdownDAO', 'PhabricatorPolicyInterface', diff --git a/src/applications/fact/controller/PhabricatorFactChartController.php b/src/applications/fact/controller/PhabricatorFactChartController.php index b6ae64b4b5..7612cf1568 100644 --- a/src/applications/fact/controller/PhabricatorFactChartController.php +++ b/src/applications/fact/controller/PhabricatorFactChartController.php @@ -16,6 +16,9 @@ final class PhabricatorFactChartController extends PhabricatorFactController { $key_id = id(new PhabricatorFactKeyDimension()) ->newDimensionID($fact->getKey()); + if (!$key_id) { + return new Aphront404Response(); + } $table = $fact->newDatapoint(); $conn_r = $table->establishConnection('r'); diff --git a/src/applications/fact/daemon/PhabricatorFactDaemon.php b/src/applications/fact/daemon/PhabricatorFactDaemon.php index 8a9214c40b..baf0203471 100644 --- a/src/applications/fact/daemon/PhabricatorFactDaemon.php +++ b/src/applications/fact/daemon/PhabricatorFactDaemon.php @@ -70,20 +70,28 @@ final class PhabricatorFactDaemon extends PhabricatorDaemon { $result = null; $datapoints = array(); + $count = 0; foreach ($iterator as $key => $object) { $phid = $object->getPHID(); $this->log(pht('Processing %s...', $phid)); - $datapoints[$phid] = $this->newDatapoints($object); - if (count($datapoints) > 1024) { + $object_datapoints = $this->newDatapoints($object); + $count += count($object_datapoints); + + $datapoints[$phid] = $object_datapoints; + + if ($count > 1024) { $this->updateDatapoints($datapoints); $datapoints = array(); + $count = 0; } + $result = $key; } - if ($datapoints) { + if ($count) { $this->updateDatapoints($datapoints); $datapoints = array(); + $count = 0; } return $result; @@ -111,7 +119,6 @@ final class PhabricatorFactDaemon extends PhabricatorDaemon { return; } - $fact_keys = array(); $objects = array(); foreach ($map as $phid => $facts) { @@ -129,9 +136,9 @@ final class PhabricatorFactDaemon extends PhabricatorDaemon { } $key_map = id(new PhabricatorFactKeyDimension()) - ->newDimensionMap(array_keys($fact_keys)); + ->newDimensionMap(array_keys($fact_keys), true); $object_map = id(new PhabricatorFactObjectDimension()) - ->newDimensionMap(array_keys($objects)); + ->newDimensionMap(array_keys($objects), true); $table = new PhabricatorFactIntDatapoint(); $conn = $table->establishConnection('w'); diff --git a/src/applications/fact/engine/PhabricatorFactEngine.php b/src/applications/fact/engine/PhabricatorFactEngine.php index b8389f7b23..722a97efbc 100644 --- a/src/applications/fact/engine/PhabricatorFactEngine.php +++ b/src/applications/fact/engine/PhabricatorFactEngine.php @@ -35,4 +35,8 @@ abstract class PhabricatorFactEngine extends Phobject { return $this->factMap[$key]; } + final protected function getViewer() { + return PhabricatorUser::getOmnipotentUser(); + } + } diff --git a/src/applications/fact/engine/PhabricatorFactManiphestTaskEngine.php b/src/applications/fact/engine/PhabricatorFactManiphestTaskEngine.php index c2ed81d72b..76eb2d1681 100644 --- a/src/applications/fact/engine/PhabricatorFactManiphestTaskEngine.php +++ b/src/applications/fact/engine/PhabricatorFactManiphestTaskEngine.php @@ -5,8 +5,77 @@ final class PhabricatorFactManiphestTaskEngine public function newFacts() { return array( + id(new PhabricatorCountFact()) + ->setKey('tasks.count.create'), + + id(new PhabricatorCountFact()) + ->setKey('tasks.open-count.create'), + id(new PhabricatorCountFact()) + ->setKey('tasks.open-count.status'), + + id(new PhabricatorCountFact()) + ->setKey('tasks.count.create.project'), + id(new PhabricatorCountFact()) + ->setKey('tasks.count.assign.project'), + id(new PhabricatorCountFact()) + ->setKey('tasks.open-count.create.project'), + id(new PhabricatorCountFact()) + ->setKey('tasks.open-count.status.project'), + id(new PhabricatorCountFact()) + ->setKey('tasks.open-count.assign.project'), + + id(new PhabricatorCountFact()) + ->setKey('tasks.count.create.owner'), + id(new PhabricatorCountFact()) + ->setKey('tasks.count.assign.owner'), + id(new PhabricatorCountFact()) + ->setKey('tasks.open-count.create.owner'), + id(new PhabricatorCountFact()) + ->setKey('tasks.open-count.status.owner'), + id(new PhabricatorCountFact()) + ->setKey('tasks.open-count.assign.owner'), + id(new PhabricatorPointsFact()) - ->setKey('tasks.count.open'), + ->setKey('tasks.points.create'), + id(new PhabricatorPointsFact()) + ->setKey('tasks.points.score'), + + id(new PhabricatorPointsFact()) + ->setKey('tasks.open-points.create'), + id(new PhabricatorPointsFact()) + ->setKey('tasks.open-points.status'), + id(new PhabricatorPointsFact()) + ->setKey('tasks.open-points.score'), + + id(new PhabricatorPointsFact()) + ->setKey('tasks.points.create.project'), + id(new PhabricatorPointsFact()) + ->setKey('tasks.points.assign.project'), + id(new PhabricatorPointsFact()) + ->setKey('tasks.points.score.project'), + id(new PhabricatorPointsFact()) + ->setKey('tasks.open-points.create.project'), + id(new PhabricatorPointsFact()) + ->setKey('tasks.open-points.status.project'), + id(new PhabricatorPointsFact()) + ->setKey('tasks.open-points.score.project'), + id(new PhabricatorPointsFact()) + ->setKey('tasks.open-points.assign.project'), + + id(new PhabricatorPointsFact()) + ->setKey('tasks.points.create.owner'), + id(new PhabricatorPointsFact()) + ->setKey('tasks.points.assign.owner'), + id(new PhabricatorPointsFact()) + ->setKey('tasks.points.score.owner'), + id(new PhabricatorPointsFact()) + ->setKey('tasks.open-points.create.owner'), + id(new PhabricatorPointsFact()) + ->setKey('tasks.open-points.status.owner'), + id(new PhabricatorPointsFact()) + ->setKey('tasks.open-points.score.project'), + id(new PhabricatorPointsFact()) + ->setKey('tasks.open-points.assign.owner'), ); } @@ -15,18 +84,319 @@ final class PhabricatorFactManiphestTaskEngine } public function newDatapointsForObject(PhabricatorLiskDAO $object) { + $viewer = $this->getViewer(); + + $xaction_query = PhabricatorApplicationTransactionQuery::newQueryForObject( + $object); + $xactions = $xaction_query + ->setViewer($viewer) + ->withObjectPHIDs(array($object->getPHID())) + ->execute(); + + $xactions = msortv($xactions, 'newChronologicalSortVector'); + + $old_open = false; + $old_points = 0; + $old_owner = null; + $project_map = array(); + $object_phid = $object->getPHID(); + $is_create = true; + + $specs = array(); $datapoints = array(); + foreach ($xactions as $xaction_group) { + $add_projects = array(); + $rem_projects = array(); - $phid = $object->getPHID(); - $type = phid_get_type($phid); + $new_open = $old_open; + $new_points = $old_points; + $new_owner = $old_owner; - $datapoint = $this->getFact('tasks.count.open') - ->newDatapoint(); + // TODO: Actually group concurrent transactions. + $xaction_group = array($xaction_group); - $datapoints[] = $datapoint - ->setObjectPHID($phid) - ->setValue(1) - ->setEpoch($object->getDateCreated()); + $group_epoch = last($xaction_group)->getDateCreated(); + foreach ($xaction_group as $xaction) { + $old_value = $xaction->getOldValue(); + $new_value = $xaction->getNewValue(); + switch ($xaction->getTransactionType()) { + case ManiphestTaskStatusTransaction::TRANSACTIONTYPE: + $new_open = !ManiphestTaskStatus::isClosedStatus($new_value); + break; + case ManiphestTaskMergedIntoTransaction::TRANSACTIONTYPE: + // When a task is merged into another task, it is changed to a + // closed status without generating a separate status transaction. + $new_open = false; + break; + case ManiphestTaskPointsTransaction::TRANSACTIONTYPE: + $new_points = (int)$xaction->getNewValue(); + break; + case ManiphestTaskOwnerTransaction::TRANSACTIONTYPE: + $new_owner = $xaction->getNewValue(); + break; + case PhabricatorTransactions::TYPE_EDGE: + $edge_type = $xaction->getMetadataValue('edge:type'); + switch ($edge_type) { + case PhabricatorProjectObjectHasProjectEdgeType::EDGECONST: + $record = PhabricatorEdgeChangeRecord::newFromTransaction( + $xaction); + $add_projects += array_fuse($record->getAddedPHIDs()); + $rem_projects += array_fuse($record->getRemovedPHIDs()); + break; + } + break; + } + } + + // If a project was both added and removed, moot it. + $mix_projects = array_intersect_key($add_projects, $rem_projects); + $add_projects = array_diff_key($add_projects, $mix_projects); + $rem_projects = array_diff_key($rem_projects, $mix_projects); + + $project_sets = array( + array( + 'phids' => $rem_projects, + 'scale' => -1, + ), + array( + 'phids' => $add_projects, + 'scale' => 1, + ), + ); + + if ($is_create) { + $action = 'create'; + $action_points = $new_points; + } else { + $action = 'assign'; + $action_points = $old_points; + } + + foreach ($project_sets as $project_set) { + $scale = $project_set['scale']; + foreach ($project_set['phids'] as $project_phid) { + if ($old_open) { + $specs[] = array( + "tasks.open-count.{$action}.project", + 1 * $scale, + $project_phid, + ); + + $specs[] = array( + "tasks.open-points.{$action}.project", + $action_points * $scale, + $project_phid, + ); + } + + $specs[] = array( + "tasks.count.{$action}.project", + 1 * $scale, + $project_phid, + ); + + $specs[] = array( + "tasks.points.{$action}.project", + $action_points * $scale, + $project_phid, + ); + + if ($scale < 0) { + unset($project_map[$project_phid]); + } else { + $project_map[$project_phid] = $project_phid; + } + } + } + + if ($new_owner !== $old_owner) { + $owner_sets = array( + array( + 'phid' => $old_owner, + 'scale' => -1, + ), + array( + 'phid' => $new_owner, + 'scale' => 1, + ), + ); + + foreach ($owner_sets as $owner_set) { + $owner_phid = $owner_set['phid']; + if ($owner_phid === null) { + continue; + } + + if ($old_open) { + $specs[] = array( + "tasks.open-count.{$action}.owner", + 1 * $scale, + $owner_phid, + ); + + $specs[] = array( + "tasks.open-points.{$action}.owner", + $action_points * $scale, + $owner_phid, + ); + } + + $specs[] = array( + "tasks.count.{$action}.owner", + 1 * $scale, + $owner_phid, + ); + + $specs[] = array( + "tasks.points.{$action}.owner", + $action_points * $scale, + $owner_phid, + ); + } + + $old_owner = $new_owner; + } + + if ($is_create) { + $specs[] = array( + 'tasks.count.create', + 1, + ); + $specs[] = array( + 'tasks.points.create', + $new_points, + ); + + if ($new_open) { + $specs[] = array( + 'tasks.open-count.create', + 1, + ); + $specs[] = array( + 'tasks.open-points.create', + $new_points, + ); + } + } else if ($new_open !== $old_open) { + if ($new_open) { + $scale = 1; + } else { + $scale = -1; + } + + $specs[] = array( + 'tasks.open-count.status', + 1 * $scale, + ); + + $specs[] = array( + 'tasks.open-points.status', + $action_points * $scale, + ); + + if ($new_owner !== null) { + $specs[] = array( + 'tasks.open-count.status.owner', + 1 * $scale, + $new_owner, + ); + $specs[] = array( + 'tasks.open-points.status.owner', + $action_points * $scale, + $new_owner, + ); + } + + foreach ($project_map as $project_phid) { + $specs[] = array( + 'tasks.open-count.status.project', + 1 * $scale, + $project_phid, + ); + $specs[] = array( + 'tasks.open-points.status.project', + $action_points * $scale, + $new_owner, + ); + } + + $old_open = $new_open; + } + + // The "score" facts only apply to rescoring tasks which already + // exist, so we skip them if the task is being created. + if (($new_points !== $old_points) && !$is_create) { + $delta = ($new_points - $old_points); + + $specs[] = array( + 'tasks.points.score', + $delta, + ); + + foreach ($project_map as $project_phid) { + $specs[] = array( + 'tasks.points.score.project', + $delta, + $project_phid, + ); + + if ($old_open && $new_open) { + $specs[] = array( + 'tasks.open-points.score.project', + $delta, + $project_phid, + ); + } + } + + if ($new_owner !== null) { + $specs[] = array( + 'tasks.points.score.owner', + $delta, + $new_owner, + ); + + if ($old_open && $new_open) { + $specs[] = array( + 'tasks.open-points.score.owner', + $delta, + $new_owner, + ); + } + } + + if ($old_open && $new_open) { + $specs[] = array( + 'tasks.open-points.score', + $delta, + ); + } + + $old_points = $new_points; + } + + foreach ($specs as $spec) { + $spec_key = $spec[0]; + $spec_value = $spec[1]; + + $datapoint = $this->getFact($spec_key) + ->newDatapoint(); + + $datapoint + ->setObjectPHID($object_phid) + ->setValue($spec_value) + ->setEpoch($group_epoch); + + if (isset($spec[2])) { + $datapoint->setDimensionPHID($spec[2]); + } + + $datapoints[] = $datapoint; + } + + $specs = array(); + $is_create = false; + } return $datapoints; } diff --git a/src/applications/fact/fact/PhabricatorCountFact.php b/src/applications/fact/fact/PhabricatorCountFact.php new file mode 100644 index 0000000000..41e6fd4de3 --- /dev/null +++ b/src/applications/fact/fact/PhabricatorCountFact.php @@ -0,0 +1,9 @@ +newDimensionMap(array($key)); - return $map[$key]; + return idx($map, $key); } - final public function newDimensionMap(array $keys) { + final public function newDimensionMap(array $keys, $create = false) { if (!$keys) { return array(); } @@ -40,6 +40,10 @@ abstract class PhabricatorFactDimension extends PhabricatorFactDAO { return $map; } + if (!$create) { + return $map; + } + $sql = array(); foreach ($need as $key) { $sql[] = qsprintf( @@ -66,7 +70,7 @@ abstract class PhabricatorFactDimension extends PhabricatorFactDAO { $need); $rows = ipull($rows, 'id', $column); - foreach ($keys as $key) { + foreach ($need as $key) { if (isset($rows[$key])) { $map[$key] = (int)$rows[$key]; } else { diff --git a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php index 015e601b02..3a7eead5cf 100644 --- a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php +++ b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php @@ -260,6 +260,11 @@ abstract class PhabricatorApplicationTransaction return $this->oldValueHasBeenSet; } + public function newChronologicalSortVector() { + return id(new PhutilSortVector()) + ->addInt((int)$this->getDateCreated()) + ->addInt((int)$this->getID()); + } /* -( Rendering )---------------------------------------------------------- */ From 46ce4c7aef604426b730e976c4e226b2be3fe25b Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 19 Feb 2018 09:35:17 -0800 Subject: [PATCH 05/13] Provide a page for examining the facts an object generates Summary: Depends on D19120. Ref T13083. When you write a fact engine, it's currently somewhat difficult to figure out exactly what it's doing. It would also be difficult to diagnose bugs or report them to the upstream. To ease this, add a page which shows all the facts an object generates. This allows you to iterate on an engine quickly without needing to reanalyze facts, take a screenshot, easily compare the timeline to the fact view, etc. Test Plan: Viewed the object fact page for several objects. Subscribers: yelirekim Maniphest Tasks: T13083 Differential Revision: https://secure.phabricator.com/D19121 --- src/__phutil_library_map__.php | 4 + .../PhabricatorFactApplication.php | 1 + .../PhabricatorFactObjectController.php | 267 ++++++++++++++++++ .../query/PhabricatorFactDatapointQuery.php | 181 ++++++++++++ .../fact/storage/PhabricatorFactDimension.php | 41 ++- .../storage/PhabricatorFactIntDatapoint.php | 26 ++ 6 files changed, 510 insertions(+), 10 deletions(-) create mode 100644 src/applications/fact/controller/PhabricatorFactObjectController.php create mode 100644 src/applications/fact/query/PhabricatorFactDatapointQuery.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index e3f7f64ada..92b9d9b5de 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -2916,6 +2916,7 @@ phutil_register_library_map(array( 'PhabricatorFactCursor' => 'applications/fact/storage/PhabricatorFactCursor.php', 'PhabricatorFactDAO' => 'applications/fact/storage/PhabricatorFactDAO.php', 'PhabricatorFactDaemon' => 'applications/fact/daemon/PhabricatorFactDaemon.php', + 'PhabricatorFactDatapointQuery' => 'applications/fact/query/PhabricatorFactDatapointQuery.php', 'PhabricatorFactDimension' => 'applications/fact/storage/PhabricatorFactDimension.php', 'PhabricatorFactEngine' => 'applications/fact/engine/PhabricatorFactEngine.php', 'PhabricatorFactEngineTestCase' => 'applications/fact/engine/__tests__/PhabricatorFactEngineTestCase.php', @@ -2928,6 +2929,7 @@ phutil_register_library_map(array( 'PhabricatorFactManagementListWorkflow' => 'applications/fact/management/PhabricatorFactManagementListWorkflow.php', 'PhabricatorFactManagementWorkflow' => 'applications/fact/management/PhabricatorFactManagementWorkflow.php', 'PhabricatorFactManiphestTaskEngine' => 'applications/fact/engine/PhabricatorFactManiphestTaskEngine.php', + 'PhabricatorFactObjectController' => 'applications/fact/controller/PhabricatorFactObjectController.php', 'PhabricatorFactObjectDimension' => 'applications/fact/storage/PhabricatorFactObjectDimension.php', 'PhabricatorFactRaw' => 'applications/fact/storage/PhabricatorFactRaw.php', 'PhabricatorFactUpdateIterator' => 'applications/fact/extract/PhabricatorFactUpdateIterator.php', @@ -8445,6 +8447,7 @@ phutil_register_library_map(array( 'PhabricatorFactCursor' => 'PhabricatorFactDAO', 'PhabricatorFactDAO' => 'PhabricatorLiskDAO', 'PhabricatorFactDaemon' => 'PhabricatorDaemon', + 'PhabricatorFactDatapointQuery' => 'Phobject', 'PhabricatorFactDimension' => 'PhabricatorFactDAO', 'PhabricatorFactEngine' => 'Phobject', 'PhabricatorFactEngineTestCase' => 'PhabricatorTestCase', @@ -8457,6 +8460,7 @@ phutil_register_library_map(array( 'PhabricatorFactManagementListWorkflow' => 'PhabricatorFactManagementWorkflow', 'PhabricatorFactManagementWorkflow' => 'PhabricatorManagementWorkflow', 'PhabricatorFactManiphestTaskEngine' => 'PhabricatorFactEngine', + 'PhabricatorFactObjectController' => 'PhabricatorFactController', 'PhabricatorFactObjectDimension' => 'PhabricatorFactDimension', 'PhabricatorFactRaw' => 'PhabricatorFactDAO', 'PhabricatorFactUpdateIterator' => 'PhutilBufferedIterator', diff --git a/src/applications/fact/application/PhabricatorFactApplication.php b/src/applications/fact/application/PhabricatorFactApplication.php index 305ed3abc9..6444fe700a 100644 --- a/src/applications/fact/application/PhabricatorFactApplication.php +++ b/src/applications/fact/application/PhabricatorFactApplication.php @@ -31,6 +31,7 @@ final class PhabricatorFactApplication extends PhabricatorApplication { '/fact/' => array( '' => 'PhabricatorFactHomeController', 'chart/' => 'PhabricatorFactChartController', + 'object/(?[^/]+)/' => 'PhabricatorFactObjectController', ), ); } diff --git a/src/applications/fact/controller/PhabricatorFactObjectController.php b/src/applications/fact/controller/PhabricatorFactObjectController.php new file mode 100644 index 0000000000..2905477f9f --- /dev/null +++ b/src/applications/fact/controller/PhabricatorFactObjectController.php @@ -0,0 +1,267 @@ +getViewer(); + + $phid = $request->getURIData('phid'); + $object = id(new PhabricatorObjectQuery()) + ->setViewer($viewer) + ->withNames(array($phid)) + ->executeOne(); + if (!$object) { + return new Aphront404Response(); + } + + $engines = PhabricatorFactEngine::loadAllEngines(); + foreach ($engines as $key => $engine) { + if (!$engine->supportsDatapointsForObject($object)) { + unset($engines[$key]); + } + } + + if (!$engines) { + return $this->newDialog() + ->setTitle(pht('No Engines')) + ->appendParagraph( + pht( + 'No fact engines support generating facts for this object.')) + ->addCancelButton($this->getApplicationURI()); + } + + $key_dimension = new PhabricatorFactKeyDimension(); + $object_phid = $object->getPHID(); + + $facts = array(); + $generated_datapoints = array(); + $timings = array(); + foreach ($engines as $key => $engine) { + $engine_facts = $engine->newFacts(); + $engine_facts = mpull($engine_facts, null, 'getKey'); + $facts[$key] = $engine_facts; + + $t_start = microtime(true); + $generated_datapoints[$key] = $engine->newDatapointsForObject($object); + $t_end = microtime(true); + + $timings[$key] = ($t_end - $t_start); + } + + $object_id = id(new PhabricatorFactObjectDimension()) + ->newDimensionID($object_phid, true); + + $stored_datapoints = id(new PhabricatorFactDatapointQuery()) + ->withFacts(array_mergev($facts)) + ->withObjectPHIDs(array($object_phid)) + ->needVectors(true) + ->execute(); + + $stored_groups = array(); + foreach ($stored_datapoints as $stored_datapoint) { + $stored_groups[$stored_datapoint['key']][] = $stored_datapoint; + } + + $stored_map = array(); + foreach ($engines as $key => $engine) { + $stored_map[$key] = array(); + foreach ($facts[$key] as $fact) { + $fact_datapoints = idx($stored_groups, $fact->getKey(), array()); + $fact_datapoints = igroup($fact_datapoints, 'vector'); + $stored_map[$key] += $fact_datapoints; + } + } + + $handle_phids = array(); + $handle_phids[] = $object->getPHID(); + foreach ($generated_datapoints as $key => $datapoint_set) { + foreach ($datapoint_set as $datapoint) { + $dimension_phid = $datapoint->getDimensionPHID(); + if ($dimension_phid !== null) { + $handle_phids[$dimension_phid] = $dimension_phid; + } + } + } + + foreach ($stored_map as $key => $stored_datapoints) { + foreach ($stored_datapoints as $vector_key => $datapoints) { + foreach ($datapoints as $datapoint) { + $dimension_phid = $datapoint['dimensionPHID']; + if ($dimension_phid !== null) { + $handle_phids[$dimension_phid] = $dimension_phid; + } + } + } + } + + $handles = $viewer->loadHandles($handle_phids); + + $dimension_map = id(new PhabricatorFactObjectDimension()) + ->newDimensionMap($handle_phids, true); + + $content = array(); + + $object_list = id(new PHUIPropertyListView()) + ->setViewer($viewer) + ->addProperty( + pht('Object'), + $handles[$object->getPHID()]->renderLink()); + + $total_cost = array_sum($timings); + $total_cost = pht('%sms', new PhutilNumber((int)(1000 * $total_cost))); + $object_list->addProperty(pht('Total Cost'), $total_cost); + + $object_box = id(new PHUIObjectBoxView()) + ->setHeaderText(pht('Fact Extraction Report')) + ->addPropertyList($object_list); + + $content[] = $object_box; + + $icon_fact = id(new PHUIIconView()) + ->setIcon('fa-line-chart green') + ->setTooltip(pht('Consistent Fact')); + + $icon_nodata = id(new PHUIIconView()) + ->setIcon('fa-question-circle-o violet') + ->setTooltip(pht('No Stored Datapoints')); + + $icon_new = id(new PHUIIconView()) + ->setIcon('fa-plus red') + ->setTooltip(pht('Not Stored')); + + $icon_surplus = id(new PHUIIconView()) + ->setIcon('fa-minus red') + ->setTooltip(pht('Not Generated')); + + foreach ($engines as $key => $engine) { + $rows = array(); + foreach ($generated_datapoints[$key] as $datapoint) { + $dimension_phid = $datapoint->getDimensionPHID(); + if ($dimension_phid !== null) { + $dimension = $handles[$datapoint->getDimensionPHID()]->renderLink(); + } else { + $dimension = null; + } + + $fact_key = $datapoint->getKey(); + + $fact = idx($facts[$key], $fact_key, null); + if ($fact) { + $fact_label = $fact->getName(); + } else { + $fact_label = $fact_key; + } + + $vector_key = $datapoint->newDatapointVector(); + if (isset($stored_map[$key][$vector_key])) { + unset($stored_map[$key][$vector_key]); + $icon = $icon_fact; + } else { + $icon = $icon_new; + } + + $rows[] = array( + $icon, + $fact_label, + $dimension, + $datapoint->getValue(), + phabricator_datetime($datapoint->getEpoch(), $viewer), + ); + } + + foreach ($stored_map[$key] as $vector_key => $datapoints) { + foreach ($datapoints as $datapoint) { + $dimension_phid = $datapoint['dimensionPHID']; + if ($dimension_phid !== null) { + $dimension = $handles[$dimension_phid]->renderLink(); + } else { + $dimension = null; + } + + $fact_key = $datapoint['key']; + $fact = idx($facts[$key], $fact_key, null); + if ($fact) { + $fact_label = $fact->getName(); + } else { + $fact_label = $fact_key; + } + + $rows[] = array( + $icon_surplus, + $fact_label, + $dimension, + $datapoint['value'], + phabricator_datetime($datapoint['epoch'], $viewer), + ); + } + } + + foreach ($facts[$key] as $fact) { + $has_any = id(new PhabricatorFactDatapointQuery()) + ->withFacts(array($fact)) + ->setLimit(1) + ->execute(); + if ($has_any) { + continue; + } + + if (!$has_any) { + $rows[] = array( + $icon_nodata, + $fact->getName(), + null, + null, + null, + ); + } + } + + $table = id(new AphrontTableView($rows)) + ->setHeaders( + array( + null, + pht('Fact'), + pht('Dimension'), + pht('Value'), + pht('Date'), + )) + ->setColumnClasses( + array( + '', + '', + '', + 'n wide right', + 'right', + )); + + $extraction_cost = $timings[$key]; + $extraction_cost = pht( + '%sms', + new PhutilNumber((int)(1000 * $extraction_cost))); + + $header = pht( + '%s (%s)', + get_class($engine), + $extraction_cost); + + $box = id(new PHUIObjectBoxView()) + ->setHeaderText($header) + ->setTable($table); + + $content[] = $box; + } + + $crumbs = $this->buildApplicationCrumbs() + ->addTextCrumb(pht('Chart')); + + $title = pht('Chart'); + + return $this->newPage() + ->setTitle($title) + ->setCrumbs($crumbs) + ->appendChild($content); + + } + +} diff --git a/src/applications/fact/query/PhabricatorFactDatapointQuery.php b/src/applications/fact/query/PhabricatorFactDatapointQuery.php new file mode 100644 index 0000000000..fee7c86d12 --- /dev/null +++ b/src/applications/fact/query/PhabricatorFactDatapointQuery.php @@ -0,0 +1,181 @@ +facts = $facts; + return $this; + } + + public function withObjectPHIDs(array $object_phids) { + $this->objectPHIDs = $object_phids; + return $this; + } + + public function setLimit($limit) { + $this->limit = $limit; + return $this; + } + + public function needVectors($need) { + $this->needVectors = $need; + return $this; + } + + public function execute() { + $facts = mpull($this->facts, null, 'getKey'); + if (!$facts) { + throw new Exception(pht('Executing a fact query requires facts.')); + } + + $table_map = array(); + foreach ($facts as $fact) { + $datapoint = $fact->newDatapoint(); + $table = $datapoint->getTableName(); + + if (!isset($table_map[$table])) { + $table_map[$table] = array( + 'table' => $datapoint, + 'facts' => array(), + ); + } + + $table_map[$table]['facts'][] = $fact; + } + + $rows = array(); + foreach ($table_map as $spec) { + $rows[] = $this->executeWithTable($spec); + } + $rows = array_mergev($rows); + + $key_unmap = array_flip($this->keyMap); + $dimension_unmap = array_flip($this->dimensionMap); + + $groups = array(); + $need_phids = array(); + foreach ($rows as $row) { + $groups[$row['keyID']][] = $row; + + $object_id = $row['objectID']; + if (!isset($dimension_unmap[$object_id])) { + $need_phids[$object_id] = $object_id; + } + + $dimension_id = $row['dimensionID']; + if ($dimension_id && !isset($dimension_unmap[$dimension_id])) { + $need_phids[$dimension_id] = $dimension_id; + } + } + + $dimension_unmap += id(new PhabricatorFactObjectDimension()) + ->newDimensionUnmap($need_phids); + + $results = array(); + foreach ($groups as $key_id => $rows) { + $key = $key_unmap[$key_id]; + $fact = $facts[$key]; + $datapoint = $fact->newDatapoint(); + foreach ($rows as $row) { + $dimension_id = $row['dimensionID']; + if ($dimension_id) { + if (!isset($dimension_unmap[$dimension_id])) { + continue; + } else { + $dimension_phid = $dimension_unmap[$dimension_id]; + } + } else { + $dimension_phid = null; + } + + $object_id = $row['objectID']; + if (!isset($dimension_unmap[$object_id])) { + continue; + } else { + $object_phid = $dimension_unmap[$object_id]; + } + + $result = array( + 'key' => $key, + 'objectPHID' => $object_phid, + 'dimensionPHID' => $dimension_phid, + 'value' => (int)$row['value'], + 'epoch' => $row['epoch'], + ); + + if ($this->needVectors) { + $result['vector'] = $datapoint->newRawVector($result); + } + + $results[] = $result; + } + } + + return $results; + } + + private function executeWithTable(array $spec) { + $table = $spec['table']; + $facts = $spec['facts']; + $conn = $table->establishConnection('r'); + + $fact_keys = mpull($facts, 'getKey'); + $this->keyMap = id(new PhabricatorFactKeyDimension()) + ->newDimensionMap($fact_keys); + + if (!$this->keyMap) { + return array(); + } + + $where = array(); + + $where[] = qsprintf( + $conn, + 'keyID IN (%Ld)', + $this->keyMap); + + if ($this->objectPHIDs) { + $object_map = id(new PhabricatorFactObjectDimension()) + ->newDimensionMap($this->objectPHIDs); + if (!$object_map) { + return array(); + } + + $this->dimensionMap = $object_map; + + $where[] = qsprintf( + $conn, + 'objectID IN (%Ld)', + $this->dimensionMap); + } + + $where = '('.implode(') AND (', $where).')'; + + if ($this->limit) { + $limit = qsprintf( + $conn, + 'LIMIT %d', + $this->limit); + } else { + $limit = ''; + } + + return queryfx_all( + $conn, + 'SELECT keyID, objectID, dimensionID, value, epoch + FROM %T WHERE %Q %Q', + $table->getTableName(), + $where, + $limit); + } + +} diff --git a/src/applications/fact/storage/PhabricatorFactDimension.php b/src/applications/fact/storage/PhabricatorFactDimension.php index 9616d2513c..9c05121c9c 100644 --- a/src/applications/fact/storage/PhabricatorFactDimension.php +++ b/src/applications/fact/storage/PhabricatorFactDimension.php @@ -4,11 +4,30 @@ abstract class PhabricatorFactDimension extends PhabricatorFactDAO { abstract protected function getDimensionColumnName(); - final public function newDimensionID($key) { - $map = $this->newDimensionMap(array($key)); + final public function newDimensionID($key, $create = false) { + $map = $this->newDimensionMap(array($key), $create); return idx($map, $key); } + final public function newDimensionUnmap(array $ids) { + if (!$ids) { + return array(); + } + + $conn = $this->establishConnection('r'); + $column = $this->getDimensionColumnName(); + + $rows = queryfx_all( + $conn, + 'SELECT id, %C FROM %T WHERE id IN (%Ld)', + $column, + $this->getTableName(), + $ids); + $rows = ipull($rows, $column, 'id'); + + return $rows; + } + final public function newDimensionMap(array $keys, $create = false) { if (!$keys) { return array(); @@ -52,14 +71,16 @@ abstract class PhabricatorFactDimension extends PhabricatorFactDAO { $key); } - foreach (PhabricatorLiskDAO::chunkSQL($sql) as $chunk) { - queryfx( - $conn, - 'INSERT IGNORE INTO %T (%C) VALUES %Q', - $this->getTableName(), - $column, - $chunk); - } + $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); + foreach (PhabricatorLiskDAO::chunkSQL($sql) as $chunk) { + queryfx( + $conn, + 'INSERT IGNORE INTO %T (%C) VALUES %Q', + $this->getTableName(), + $column, + $chunk); + } + unset($unguarded); $rows = queryfx_all( $conn, diff --git a/src/applications/fact/storage/PhabricatorFactIntDatapoint.php b/src/applications/fact/storage/PhabricatorFactIntDatapoint.php index 7895052d5d..87a7c68adf 100644 --- a/src/applications/fact/storage/PhabricatorFactIntDatapoint.php +++ b/src/applications/fact/storage/PhabricatorFactIntDatapoint.php @@ -58,4 +58,30 @@ final class PhabricatorFactIntDatapoint extends PhabricatorFactDAO { return $this->dimensionPHID; } + public function newDatapointVector() { + return $this->formatVector( + array( + $this->key, + $this->objectPHID, + $this->dimensionPHID, + $this->value, + $this->epoch, + )); + } + + public function newRawVector(array $spec) { + return $this->formatVector( + array( + $spec['key'], + $spec['objectPHID'], + $spec['dimensionPHID'], + $spec['value'], + $spec['epoch'], + )); + } + + private function formatVector(array $vector) { + return implode(':', $vector); + } + } From 2fb266de7c1a9ffb8fd393af03408b3c75bc2ae4 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 19 Feb 2018 11:28:52 -0800 Subject: [PATCH 06/13] Fix some of the most obvious bugs in fact generation from Maniphest tasks Summary: Depends on D19121. Ref T13083. Group transactions and show groups in the debugging view. Fix some of the most obvious issues with fact generation: - No more 0-point facts. - Engine can now generate at least one of every type of fact. Test Plan: Generated facts, viewed them in the debugging view, fact generation largely appeared to align with reality. No more "no facts in storage" facts. Subscribers: yelirekim Maniphest Tasks: T13083 Differential Revision: https://secure.phabricator.com/D19122 --- src/__phutil_library_map__.php | 6 +- .../PhabricatorFactObjectController.php | 56 +++++++++++++ .../fact/daemon/PhabricatorFactDaemon.php | 5 ++ .../fact/engine/PhabricatorFactEngine.php | 14 +++- ...=> PhabricatorManiphestTaskFactEngine.php} | 67 ++++++++------- .../PhabricatorTransactionFactEngine.php | 84 +++++++++++++++++++ 6 files changed, 198 insertions(+), 34 deletions(-) rename src/applications/fact/engine/{PhabricatorFactManiphestTaskEngine.php => PhabricatorManiphestTaskFactEngine.php} (90%) create mode 100644 src/applications/fact/engine/PhabricatorTransactionFactEngine.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 92b9d9b5de..f7cda9d8fa 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -2928,7 +2928,6 @@ phutil_register_library_map(array( 'PhabricatorFactManagementDestroyWorkflow' => 'applications/fact/management/PhabricatorFactManagementDestroyWorkflow.php', 'PhabricatorFactManagementListWorkflow' => 'applications/fact/management/PhabricatorFactManagementListWorkflow.php', 'PhabricatorFactManagementWorkflow' => 'applications/fact/management/PhabricatorFactManagementWorkflow.php', - 'PhabricatorFactManiphestTaskEngine' => 'applications/fact/engine/PhabricatorFactManiphestTaskEngine.php', 'PhabricatorFactObjectController' => 'applications/fact/controller/PhabricatorFactObjectController.php', 'PhabricatorFactObjectDimension' => 'applications/fact/storage/PhabricatorFactObjectDimension.php', 'PhabricatorFactRaw' => 'applications/fact/storage/PhabricatorFactRaw.php', @@ -3264,6 +3263,7 @@ phutil_register_library_map(array( 'PhabricatorManagementWorkflow' => 'infrastructure/management/PhabricatorManagementWorkflow.php', 'PhabricatorManiphestApplication' => 'applications/maniphest/application/PhabricatorManiphestApplication.php', 'PhabricatorManiphestConfigOptions' => 'applications/maniphest/config/PhabricatorManiphestConfigOptions.php', + 'PhabricatorManiphestTaskFactEngine' => 'applications/fact/engine/PhabricatorManiphestTaskFactEngine.php', 'PhabricatorManiphestTaskTestDataGenerator' => 'applications/maniphest/lipsum/PhabricatorManiphestTaskTestDataGenerator.php', 'PhabricatorManualActivitySetupCheck' => 'applications/config/check/PhabricatorManualActivitySetupCheck.php', 'PhabricatorMarkupCache' => 'applications/cache/storage/PhabricatorMarkupCache.php', @@ -4362,6 +4362,7 @@ phutil_register_library_map(array( 'PhabricatorTokensSettingsPanel' => 'applications/settings/panel/PhabricatorTokensSettingsPanel.php', 'PhabricatorTokensToken' => 'applications/tokens/storage/PhabricatorTokensToken.php', 'PhabricatorTransactionChange' => 'applications/transactions/data/PhabricatorTransactionChange.php', + 'PhabricatorTransactionFactEngine' => 'applications/fact/engine/PhabricatorTransactionFactEngine.php', 'PhabricatorTransactionRemarkupChange' => 'applications/transactions/data/PhabricatorTransactionRemarkupChange.php', 'PhabricatorTransactions' => 'applications/transactions/constants/PhabricatorTransactions.php', 'PhabricatorTransactionsApplication' => 'applications/transactions/application/PhabricatorTransactionsApplication.php', @@ -8459,7 +8460,6 @@ phutil_register_library_map(array( 'PhabricatorFactManagementDestroyWorkflow' => 'PhabricatorFactManagementWorkflow', 'PhabricatorFactManagementListWorkflow' => 'PhabricatorFactManagementWorkflow', 'PhabricatorFactManagementWorkflow' => 'PhabricatorManagementWorkflow', - 'PhabricatorFactManiphestTaskEngine' => 'PhabricatorFactEngine', 'PhabricatorFactObjectController' => 'PhabricatorFactController', 'PhabricatorFactObjectDimension' => 'PhabricatorFactDimension', 'PhabricatorFactRaw' => 'PhabricatorFactDAO', @@ -8833,6 +8833,7 @@ phutil_register_library_map(array( 'PhabricatorManagementWorkflow' => 'PhutilArgumentWorkflow', 'PhabricatorManiphestApplication' => 'PhabricatorApplication', 'PhabricatorManiphestConfigOptions' => 'PhabricatorApplicationConfigOptions', + 'PhabricatorManiphestTaskFactEngine' => 'PhabricatorTransactionFactEngine', 'PhabricatorManiphestTaskTestDataGenerator' => 'PhabricatorTestDataGenerator', 'PhabricatorManualActivitySetupCheck' => 'PhabricatorSetupCheck', 'PhabricatorMarkupCache' => 'PhabricatorCacheDAO', @@ -10156,6 +10157,7 @@ phutil_register_library_map(array( 'PhabricatorConduitResultInterface', ), 'PhabricatorTransactionChange' => 'Phobject', + 'PhabricatorTransactionFactEngine' => 'PhabricatorFactEngine', 'PhabricatorTransactionRemarkupChange' => 'PhabricatorTransactionChange', 'PhabricatorTransactions' => 'Phobject', 'PhabricatorTransactionsApplication' => 'PhabricatorApplication', diff --git a/src/applications/fact/controller/PhabricatorFactObjectController.php b/src/applications/fact/controller/PhabricatorFactObjectController.php index 2905477f9f..bffc1ef157 100644 --- a/src/applications/fact/controller/PhabricatorFactObjectController.php +++ b/src/applications/fact/controller/PhabricatorFactObjectController.php @@ -17,6 +17,10 @@ final class PhabricatorFactObjectController $engines = PhabricatorFactEngine::loadAllEngines(); foreach ($engines as $key => $engine) { + $engine = id(clone $engine) + ->setViewer($viewer); + $engines[$key] = $engine; + if (!$engine->supportsDatapointsForObject($object)) { unset($engines[$key]); } @@ -250,6 +254,58 @@ final class PhabricatorFactObjectController ->setTable($table); $content[] = $box; + + if ($engine instanceof PhabricatorTransactionFactEngine) { + $groups = $engine->newTransactionGroupsForObject($object); + $groups = array_values($groups); + + $xaction_phids = array(); + foreach ($groups as $group_key => $xactions) { + foreach ($xactions as $xaction) { + $xaction_phids[] = $xaction->getAuthorPHID(); + } + } + $xaction_handles = $viewer->loadHandles($xaction_phids); + + $rows = array(); + foreach ($groups as $group_key => $xactions) { + foreach ($xactions as $xaction) { + $rows[] = array( + $group_key, + $xaction->getTransactionType(), + $xaction_handles[$xaction->getAuthorPHID()]->renderLink(), + phabricator_datetime($xaction->getDateCreated(), $viewer), + ); + } + } + + $table = id(new AphrontTableView($rows)) + ->setHeaders( + array( + pht('Group'), + pht('Type'), + pht('Author'), + pht('Date'), + )) + ->setColumnClasses( + array( + null, + 'pri', + 'wide', + 'right', + )); + + $header = pht( + '%s (Transactions)', + get_class($engine)); + + $xaction_box = id(new PHUIObjectBoxView()) + ->setHeaderText($header) + ->setTable($table); + + $content[] = $xaction_box; + } + } $crumbs = $this->buildApplicationCrumbs() diff --git a/src/applications/fact/daemon/PhabricatorFactDaemon.php b/src/applications/fact/daemon/PhabricatorFactDaemon.php index baf0203471..3fa6c6f0ff 100644 --- a/src/applications/fact/daemon/PhabricatorFactDaemon.php +++ b/src/applications/fact/daemon/PhabricatorFactDaemon.php @@ -62,6 +62,11 @@ final class PhabricatorFactDaemon extends PhabricatorDaemon { public function setEngines(array $engines) { assert_instances_of($engines, 'PhabricatorFactEngine'); + $viewer = PhabricatorUser::getOmnipotentUser(); + foreach ($engines as $engine) { + $engine->setViewer($viewer); + } + $this->engines = $engines; return $this; } diff --git a/src/applications/fact/engine/PhabricatorFactEngine.php b/src/applications/fact/engine/PhabricatorFactEngine.php index 722a97efbc..ba2cf966e5 100644 --- a/src/applications/fact/engine/PhabricatorFactEngine.php +++ b/src/applications/fact/engine/PhabricatorFactEngine.php @@ -3,6 +3,7 @@ abstract class PhabricatorFactEngine extends Phobject { private $factMap; + private $viewer; final public static function loadAllEngines() { return id(new PhutilClassMapQuery()) @@ -35,8 +36,17 @@ abstract class PhabricatorFactEngine extends Phobject { return $this->factMap[$key]; } - final protected function getViewer() { - return PhabricatorUser::getOmnipotentUser(); + public function setViewer(PhabricatorUser $viewer) { + $this->viewer = $viewer; + return $this; + } + + public function getViewer() { + if (!$this->viewer) { + throw new PhutilInvalidStateException('setViewer'); + } + + return $this->viewer; } } diff --git a/src/applications/fact/engine/PhabricatorFactManiphestTaskEngine.php b/src/applications/fact/engine/PhabricatorManiphestTaskFactEngine.php similarity index 90% rename from src/applications/fact/engine/PhabricatorFactManiphestTaskEngine.php rename to src/applications/fact/engine/PhabricatorManiphestTaskFactEngine.php index 76eb2d1681..dffceedb77 100644 --- a/src/applications/fact/engine/PhabricatorFactManiphestTaskEngine.php +++ b/src/applications/fact/engine/PhabricatorManiphestTaskFactEngine.php @@ -1,7 +1,7 @@ setKey('tasks.open-points.status.owner'), id(new PhabricatorPointsFact()) - ->setKey('tasks.open-points.score.project'), + ->setKey('tasks.open-points.score.owner'), id(new PhabricatorPointsFact()) ->setKey('tasks.open-points.assign.owner'), ); @@ -84,16 +84,7 @@ final class PhabricatorFactManiphestTaskEngine } public function newDatapointsForObject(PhabricatorLiskDAO $object) { - $viewer = $this->getViewer(); - - $xaction_query = PhabricatorApplicationTransactionQuery::newQueryForObject( - $object); - $xactions = $xaction_query - ->setViewer($viewer) - ->withObjectPHIDs(array($object->getPHID())) - ->execute(); - - $xactions = msortv($xactions, 'newChronologicalSortVector'); + $xaction_groups = $this->newTransactionGroupsForObject($object); $old_open = false; $old_points = 0; @@ -104,7 +95,7 @@ final class PhabricatorFactManiphestTaskEngine $specs = array(); $datapoints = array(); - foreach ($xactions as $xaction_group) { + foreach ($xaction_groups as $xaction_group) { $add_projects = array(); $rem_projects = array(); @@ -112,8 +103,11 @@ final class PhabricatorFactManiphestTaskEngine $new_points = $old_points; $new_owner = $old_owner; - // TODO: Actually group concurrent transactions. - $xaction_group = array($xaction_group); + if ($is_create) { + // Assume tasks start open. + // TODO: This might be a questionable assumption? + $new_open = true; + } $group_epoch = last($xaction_group)->getDateCreated(); foreach ($xaction_group as $xaction) { @@ -167,15 +161,17 @@ final class PhabricatorFactManiphestTaskEngine if ($is_create) { $action = 'create'; $action_points = $new_points; + $include_open = $new_open; } else { $action = 'assign'; $action_points = $old_points; + $include_open = $old_open; } foreach ($project_sets as $project_set) { $scale = $project_set['scale']; foreach ($project_set['phids'] as $project_phid) { - if ($old_open) { + if ($include_open) { $specs[] = array( "tasks.open-count.{$action}.project", 1 * $scale, @@ -227,7 +223,9 @@ final class PhabricatorFactManiphestTaskEngine continue; } - if ($old_open) { + $scale = $owner_set['scale']; + + if ($old_open != $new_open) { $specs[] = array( "tasks.open-count.{$action}.owner", 1 * $scale, @@ -247,14 +245,14 @@ final class PhabricatorFactManiphestTaskEngine $owner_phid, ); - $specs[] = array( - "tasks.points.{$action}.owner", - $action_points * $scale, - $owner_phid, - ); + if ($action_points) { + $specs[] = array( + "tasks.points.{$action}.owner", + $action_points * $scale, + $owner_phid, + ); + } } - - $old_owner = $new_owner; } if ($is_create) { @@ -262,6 +260,7 @@ final class PhabricatorFactManiphestTaskEngine 'tasks.count.create', 1, ); + $specs[] = array( 'tasks.points.create', $new_points, @@ -316,11 +315,9 @@ final class PhabricatorFactManiphestTaskEngine $specs[] = array( 'tasks.open-points.status.project', $action_points * $scale, - $new_owner, + $project_phid, ); } - - $old_open = $new_open; } // The "score" facts only apply to rescoring tasks which already @@ -371,14 +368,23 @@ final class PhabricatorFactManiphestTaskEngine $delta, ); } - - $old_points = $new_points; } + $old_points = $new_points; + $old_open = $new_open; + $old_owner = $new_owner; + foreach ($specs as $spec) { $spec_key = $spec[0]; $spec_value = $spec[1]; + // Don't write any facts with a value of 0. The "count" facts never + // have a value of 0, and the "points" facts aren't meaningful if + // they have a value of 0. + if ($spec_value == 0) { + continue; + } + $datapoint = $this->getFact($spec_key) ->newDatapoint(); @@ -401,4 +407,5 @@ final class PhabricatorFactManiphestTaskEngine return $datapoints; } + } diff --git a/src/applications/fact/engine/PhabricatorTransactionFactEngine.php b/src/applications/fact/engine/PhabricatorTransactionFactEngine.php new file mode 100644 index 0000000000..20fb7a6f66 --- /dev/null +++ b/src/applications/fact/engine/PhabricatorTransactionFactEngine.php @@ -0,0 +1,84 @@ +getViewer(); + + $xaction_query = PhabricatorApplicationTransactionQuery::newQueryForObject( + $object); + $xactions = $xaction_query + ->setViewer($viewer) + ->withObjectPHIDs(array($object->getPHID())) + ->execute(); + + $xactions = msortv($xactions, 'newChronologicalSortVector'); + + return $this->groupTransactions($xactions); + } + + protected function groupTransactions(array $xactions) { + // These grouping rules are generally much looser than the display grouping + // rules. As long as the same user is editing the task and they don't leave + // it alone for a particularly long time, we'll group things together. + + $breaks = array(); + + $touch_window = phutil_units('15 minutes in seconds'); + $user_type = PhabricatorPeopleUserPHIDType::TYPECONST; + + $last_actor = null; + $last_epoch = null; + + foreach ($xactions as $key => $xaction) { + $this_actor = $xaction->getAuthorPHID(); + if (phid_get_type($this_actor) != $user_type) { + $this_actor = null; + } + + if ($this_actor && $last_actor && ($this_actor != $last_actor)) { + $breaks[$key] = true; + } + + // If too much time passed between changes, group them separately. + $this_epoch = $xaction->getDateCreated(); + if ($last_epoch) { + if (($this_epoch - $last_epoch) > $touch_window) { + $breaks[$key] = true; + } + } + + // The clock gets reset every time the same real user touches the + // task, but does not reset if an automated actor touches things. + if (!$last_actor || ($this_actor == $last_actor)) { + $last_epoch = $this_epoch; + } + + if ($this_actor && ($last_actor != $this_actor)) { + $last_actor = $this_actor; + $last_epoch = $this_epoch; + } + } + + $groups = array(); + $group = array(); + foreach ($xactions as $key => $xaction) { + if (isset($breaks[$key])) { + if ($group) { + $groups[] = $group; + $group = array(); + } + } + + $group[] = $xaction; + } + + if ($group) { + $groups[] = $group; + } + + return $groups; + } + +} From 2085716da6d1dfa83448ad2339e2efd7fa54edfb Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 20 Feb 2018 17:06:09 -0800 Subject: [PATCH 07/13] Make dashboard arrange actions (move, add, remove) work again after read locking from "chaos reduction" Summary: See PHI385. Ref T13054. Ref T13083. The dashboard "arrange" operations (add, remove, move) rely on doing `$dashboard->setThing(...)` and then applying transactions. This no longer works after the read locking change from T13054. To make this function again, just add an explicit `save()` after layout adjustment. This should be more nuanced eventually, but all arrange operations are nonfunctional in a corrupting way at HEAD of `master`/`stable`, so stop the bleeding first. Test Plan: - Created new empty and template dashboards. - Moved panels. - Added new and existing panels. - Removed panels. Maniphest Tasks: T13083, T13054 Differential Revision: https://secure.phabricator.com/D19123 --- .../dashboard/storage/PhabricatorDashboard.php | 8 ++++++++ .../typeahead/PhabricatorDashboardPanelDatasource.php | 4 +++- 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/src/applications/dashboard/storage/PhabricatorDashboard.php b/src/applications/dashboard/storage/PhabricatorDashboard.php index b79fded93f..2e673de19c 100644 --- a/src/applications/dashboard/storage/PhabricatorDashboard.php +++ b/src/applications/dashboard/storage/PhabricatorDashboard.php @@ -74,7 +74,15 @@ final class PhabricatorDashboard extends PhabricatorDashboardDAO public function setLayoutConfigFromObject( PhabricatorDashboardLayoutConfig $object) { + $this->setLayoutConfig($object->toDictionary()); + + // See PHI385. Dashboard panel mutations rely on changes to the Dashboard + // object persisting when transactions are applied, but this assumption is + // no longer valid after T13054. For now, just save the dashboard + // explicitly. + $this->save(); + return $this; } diff --git a/src/applications/dashboard/typeahead/PhabricatorDashboardPanelDatasource.php b/src/applications/dashboard/typeahead/PhabricatorDashboardPanelDatasource.php index d534d32adc..8bbdb1a5e5 100644 --- a/src/applications/dashboard/typeahead/PhabricatorDashboardPanelDatasource.php +++ b/src/applications/dashboard/typeahead/PhabricatorDashboardPanelDatasource.php @@ -34,8 +34,10 @@ final class PhabricatorDashboardPanelDatasource $impl = $panel->getImplementation(); if ($impl) { $type_text = $impl->getPanelTypeName(); + $icon = $impl->getIcon(); } else { $type_text = nonempty($panel->getPanelType(), pht('Unknown Type')); + $icon = 'fa-question'; } $id = $panel->getID(); $monogram = $panel->getMonogram(); @@ -44,7 +46,7 @@ final class PhabricatorDashboardPanelDatasource $result = id(new PhabricatorTypeaheadResult()) ->setName($monogram.' '.$panel->getName()) ->setPHID($id) - ->setIcon($impl->getIcon()) + ->setIcon($icon) ->addAttribute($type_text); if (!empty($properties['class'])) { From ffcfc0465226a3cdb78c247f89d7590b7b4987fa Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 22 Feb 2018 10:09:49 -0800 Subject: [PATCH 08/13] Add some delivery diagnostic headers to outbound mail Summary: Fixes T13087. Ref T13090. An install ran into a situation where mail was being double-delivered, and it wasn't immediately clear where in the pipeline the issue lay. This change adds some headers which should rule out (or, at least, render very unlikely) some possible causes if we encounter similar issues in the future. The `X-Phabricator-Mail-ID` header stores the ID of the `MetaMTAMail` storage object so we can distinguish between two messages sent to two different targets and one message which may have been split or re-sent. It also makes it easier to know what to `bin/mail show-outbound --id ` and where to find the message in the web UI for additional information. The `X-Phabricator-Send-Attempt` is a unique value per attempt. If two mail messages are delivered with the same attempt value, the split is probably downstream from Phabricator. If they have different attempt values, the split is probably in Phabricator. (In this case, the split was somewhere downstream from us, since sending mail with `/usr/bin/mail` also resulted in duplicates.) Test Plan: Send some mail, inspected it with `bin/mail show-outbound --id `, saw new headers with sensible/expected values. Maniphest Tasks: T13090, T13087 Differential Revision: https://secure.phabricator.com/D19124 --- .../metamta/storage/PhabricatorMetaMTAMail.php | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/applications/metamta/storage/PhabricatorMetaMTAMail.php b/src/applications/metamta/storage/PhabricatorMetaMTAMail.php index 9dfd6a3eb6..5326e10639 100644 --- a/src/applications/metamta/storage/PhabricatorMetaMTAMail.php +++ b/src/applications/metamta/storage/PhabricatorMetaMTAMail.php @@ -1302,6 +1302,11 @@ final class PhabricatorMetaMTAMail $headers[] = array('Thread-Topic', $related_phid); } + $headers[] = array('X-Phabricator-Mail-ID', $this->getID()); + + $unique = Filesystem::readRandomCharacters(16); + $headers[] = array('X-Phabricator-Send-Attempt', $unique); + return $headers; } @@ -1356,6 +1361,8 @@ final class PhabricatorMetaMTAMail 'X-Phabricator-Sent-This-Message', 'X-Phabricator-Must-Encrypt', + 'X-Phabricator-Mail-ID', + 'X-Phabricator-Send-Attempt', ); // NOTE: The major header we want to drop is "X-Phabricator-Mail-Tags". From 4cb62ca0d6aeb8a61d6f8a33654d9033a5ffd5d6 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 22 Feb 2018 10:51:27 -0800 Subject: [PATCH 09/13] Support "phriction.document.search" queries by "parentPaths" or "ancestorPaths" Summary: Ref T13090. Ref T13077. This adds `parentPaths` and `ancestorPaths` constraints to `phriction.document.query`. These should be a little more usable than the internal `slugPrefix` / `depth` stuff -- that's technically more powerful, but requires callers to know more slug normalization rules. We could perhaps expose `minDepth` / `maxDepth` in the future. Test Plan: Ran valid and invalid `parentPaths` and `ancestorPaths` queries for `/`, `aaa/`, `AAA/`, etc. Got sensible-seeming results. Maniphest Tasks: T13090, T13077 Differential Revision: https://secure.phabricator.com/D19125 --- .../query/PhrictionDocumentQuery.php | 103 +++++++++++++++++- .../query/PhrictionDocumentSearchEngine.php | 16 +++ 2 files changed, 118 insertions(+), 1 deletion(-) diff --git a/src/applications/phriction/query/PhrictionDocumentQuery.php b/src/applications/phriction/query/PhrictionDocumentQuery.php index de3064fd42..0c6a2c23bc 100644 --- a/src/applications/phriction/query/PhrictionDocumentQuery.php +++ b/src/applications/phriction/query/PhrictionDocumentQuery.php @@ -10,6 +10,9 @@ final class PhrictionDocumentQuery private $slugPrefix; private $statuses; + private $parentPaths; + private $ancestorPaths; + private $needContent; const ORDER_HIERARCHY = 'hierarchy'; @@ -34,7 +37,7 @@ final class PhrictionDocumentQuery return $this; } - public function withSlugPrefix($slug_prefix) { + public function withSlugPrefix($slug_prefix) { $this->slugPrefix = $slug_prefix; return $this; } @@ -44,6 +47,16 @@ final class PhrictionDocumentQuery return $this; } + public function withParentPaths(array $paths) { + $this->parentPaths = $paths; + return $this; + } + + public function withAncestorPaths(array $paths) { + $this->ancestorPaths = $paths; + return $this; + } + public function needContent($need_content) { $this->needContent = $need_content; return $this; @@ -214,6 +227,94 @@ final class PhrictionDocumentQuery $this->depths); } + if ($this->parentPaths !== null || $this->ancestorPaths !== null) { + $sets = array( + array( + 'paths' => $this->parentPaths, + 'parents' => true, + ), + array( + 'paths' => $this->ancestorPaths, + 'parents' => false, + ), + ); + + $paths = array(); + foreach ($sets as $set) { + $set_paths = $set['paths']; + if ($set_paths === null) { + continue; + } + + if (!$set_paths) { + throw new PhabricatorEmptyQueryException( + pht('No parent/ancestor paths specified.')); + } + + $is_parents = $set['parents']; + foreach ($set_paths as $path) { + $path_normal = PhabricatorSlug::normalize($path); + if ($path !== $path_normal) { + throw new Exception( + pht( + 'Document path "%s" is not a valid path. The normalized '. + 'form of this path is "%s".', + $path, + $path_normal)); + } + + $depth = PhabricatorSlug::getDepth($path_normal); + if ($is_parents) { + $min_depth = $depth + 1; + $max_depth = $depth + 1; + } else { + $min_depth = $depth + 1; + $max_depth = null; + } + + $paths[] = array( + $path_normal, + $min_depth, + $max_depth, + ); + } + } + + $path_clauses = array(); + foreach ($paths as $path) { + $parts = array(); + list($prefix, $min, $max) = $path; + + // If we're getting children or ancestors of the root document, they + // aren't actually stored with the leading "/" in the database, so + // just skip this part of the clause. + if ($prefix !== '/') { + $parts[] = qsprintf( + $conn, + 'd.slug LIKE %>', + $prefix); + } + + if ($min !== null) { + $parts[] = qsprintf( + $conn, + 'd.depth >= %d', + $min); + } + + if ($max !== null) { + $parts[] = qsprintf( + $conn, + 'd.depth <= %d', + $max); + } + + $path_clauses[] = '('.implode(') AND (', $parts).')'; + } + + $where[] = '('.implode(') OR (', $path_clauses).')'; + } + return $where; } diff --git a/src/applications/phriction/query/PhrictionDocumentSearchEngine.php b/src/applications/phriction/query/PhrictionDocumentSearchEngine.php index e0781ec81f..e3d962146a 100644 --- a/src/applications/phriction/query/PhrictionDocumentSearchEngine.php +++ b/src/applications/phriction/query/PhrictionDocumentSearchEngine.php @@ -27,6 +27,14 @@ final class PhrictionDocumentSearchEngine $query->withSlugs($map['paths']); } + if ($map['parentPaths']) { + $query->withParentPaths($map['parentPaths']); + } + + if ($map['ancestorPaths']) { + $query->withAncestorPaths($map['ancestorPaths']); + } + return $query; } @@ -40,6 +48,14 @@ final class PhrictionDocumentSearchEngine ->setKey('paths') ->setIsHidden(true) ->setLabel(pht('Paths')), + id(new PhabricatorSearchStringListField()) + ->setKey('parentPaths') + ->setIsHidden(true) + ->setLabel(pht('Parent Paths')), + id(new PhabricatorSearchStringListField()) + ->setKey('ancestorPaths') + ->setIsHidden(true) + ->setLabel(pht('Ancestor Paths')), ); } From d0591f3680b28523c40d4d8f6e224aab5ab24d37 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 22 Feb 2018 12:16:11 -0800 Subject: [PATCH 10/13] Support some QueryConstraint operations against generic ApplicationSearch query logic Summary: Ref T13090. Currently, it isn't possible to query custom fields for complex constraints. Lay the groundwork to implement some of the easy ones (none(), any()) for Datasource/PHID fields. Test Plan: Hard-coded some constraints and queried with them; see next change for more substantial testing. Maniphest Tasks: T13090 Differential Revision: https://secure.phabricator.com/D19126 --- .../constraint/PhabricatorQueryConstraint.php | 1 + ...PhabricatorCursorPagedPolicyAwareQuery.php | 157 ++++++++++++++---- 2 files changed, 126 insertions(+), 32 deletions(-) diff --git a/src/infrastructure/query/constraint/PhabricatorQueryConstraint.php b/src/infrastructure/query/constraint/PhabricatorQueryConstraint.php index 54cd7ae51f..874d8756e8 100644 --- a/src/infrastructure/query/constraint/PhabricatorQueryConstraint.php +++ b/src/infrastructure/query/constraint/PhabricatorQueryConstraint.php @@ -9,6 +9,7 @@ final class PhabricatorQueryConstraint extends Phobject { const OPERATOR_ANCESTOR = 'ancestor'; const OPERATOR_EMPTY = 'empty'; const OPERATOR_ONLY = 'only'; + const OPERATOR_ANY = 'any'; private $operator; private $value; diff --git a/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php b/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php index 63daa6df79..5193b14975 100644 --- a/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php +++ b/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php @@ -342,6 +342,7 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery $where[] = $this->buildSpacesWhereClause($conn); $where[] = $this->buildNgramsWhereClause($conn); $where[] = $this->buildFerretWhereClause($conn); + $where[] = $this->buildApplicationSearchWhereClause($conn); return $where; } @@ -1158,12 +1159,29 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery PhabricatorCustomFieldIndexStorage $index, $value) { + $values = (array)$value; + + $data_values = array(); + $constraint_values = array(); + foreach ($values as $value) { + if ($value instanceof PhabricatorQueryConstraint) { + $constraint_values[] = $value; + } else { + $data_values[] = $value; + } + } + + $alias = 'appsearch_'.count($this->applicationSearchConstraints); + $this->applicationSearchConstraints[] = array( 'type' => $index->getIndexValueType(), 'cond' => '=', 'table' => $index->getTableName(), 'index' => $index->getIndexKey(), - 'value' => $value, + 'alias' => $alias, + 'value' => $values, + 'data' => $data_values, + 'constraints' => $constraint_values, ); return $this; @@ -1203,11 +1221,14 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery 'int')); } + $alias = 'appsearch_'.count($this->applicationSearchConstraints); + $this->applicationSearchConstraints[] = array( 'type' => $index->getIndexValueType(), 'cond' => 'range', 'table' => $index->getTableName(), 'index' => $index->getIndexKey(), + 'alias' => $alias, 'value' => array($min, $max), ); @@ -1256,7 +1277,7 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery switch ($type) { case 'string': case 'int': - if (count((array)$value) > 1) { + if (count($value) > 1) { return true; } break; @@ -1309,49 +1330,39 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery * @task appsearch */ protected function buildApplicationSearchJoinClause( - AphrontDatabaseConnection $conn_r) { + AphrontDatabaseConnection $conn) { $joins = array(); foreach ($this->applicationSearchConstraints as $key => $constraint) { $table = $constraint['table']; - $alias = 'appsearch_'.$key; + $alias = $constraint['alias']; $index = $constraint['index']; $cond = $constraint['cond']; $phid_column = $this->getApplicationSearchObjectPHIDColumn(); switch ($cond) { case '=': - $type = $constraint['type']; - switch ($type) { - case 'string': - $constraint_clause = qsprintf( - $conn_r, - '%T.indexValue IN (%Ls)', - $alias, - (array)$constraint['value']); + // Figure out whether we need to do a LEFT JOIN or not. We need to + // LEFT JOIN if we're going to select "IS NULL" rows. + $join_type = 'JOIN'; + foreach ($constraint['constraints'] as $query_constraint) { + $op = $query_constraint->getOperator(); + if ($op === PhabricatorQueryConstraint::OPERATOR_NULL) { + $join_type = 'LEFT JOIN'; break; - case 'int': - $constraint_clause = qsprintf( - $conn_r, - '%T.indexValue IN (%Ld)', - $alias, - (array)$constraint['value']); - break; - default: - throw new Exception(pht('Unknown index type "%s"!', $type)); + } } $joins[] = qsprintf( - $conn_r, - 'JOIN %T %T ON %T.objectPHID = %Q - AND %T.indexKey = %s - AND (%Q)', + $conn, + '%Q %T %T ON %T.objectPHID = %Q + AND %T.indexKey = %s', + $join_type, $table, $alias, $alias, $phid_column, $alias, - $index, - $constraint_clause); + $index); break; case 'range': list($min, $max) = $constraint['value']; @@ -1362,19 +1373,19 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery if ($min === null) { $constraint_clause = qsprintf( - $conn_r, + $conn, '%T.indexValue <= %d', $alias, $max); } else if ($max === null) { $constraint_clause = qsprintf( - $conn_r, + $conn, '%T.indexValue >= %d', $alias, $min); } else { $constraint_clause = qsprintf( - $conn_r, + $conn, '%T.indexValue BETWEEN %d AND %d', $alias, $min, @@ -1382,7 +1393,7 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery } $joins[] = qsprintf( - $conn_r, + $conn, 'JOIN %T %T ON %T.objectPHID = %Q AND %T.indexKey = %s AND (%Q)', @@ -1414,7 +1425,7 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery $key = $spec['customfield.index.key']; $joins[] = qsprintf( - $conn_r, + $conn, 'LEFT JOIN %T %T ON %T.objectPHID = %Q AND %T.indexKey = %s', $table, @@ -1428,6 +1439,88 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery return implode(' ', $joins); } + /** + * Construct a WHERE clause appropriate for applying ApplicationSearch + * constraints. + * + * @param AphrontDatabaseConnection Connection executing the query. + * @return list Where clause parts. + * @task appsearch + */ + protected function buildApplicationSearchWhereClause( + AphrontDatabaseConnection $conn) { + + $where = array(); + + foreach ($this->applicationSearchConstraints as $key => $constraint) { + $alias = $constraint['alias']; + $cond = $constraint['cond']; + $type = $constraint['type']; + + $data_values = $constraint['data']; + $constraint_values = $constraint['constraints']; + + $constraint_parts = array(); + switch ($cond) { + case '=': + if ($data_values) { + switch ($type) { + case 'string': + $constraint_parts[] = qsprintf( + $conn, + '%T.indexValue IN (%Ls)', + $alias, + $data_values); + break; + case 'int': + $constraint_parts[] = qsprintf( + $conn, + '%T.indexValue IN (%Ld)', + $alias, + $data_values); + break; + default: + throw new Exception(pht('Unknown index type "%s"!', $type)); + } + } + + if ($constraint_values) { + foreach ($constraint_values as $value) { + $op = $value->getOperator(); + switch ($op) { + case PhabricatorQueryConstraint::OPERATOR_NULL: + $constraint_parts[] = qsprintf( + $conn, + '%T.indexValue IS NULL', + $alias); + break; + case PhabricatorQueryConstraint::OPERATOR_ANY: + $constraint_parts[] = qsprintf( + $conn, + '%T.indexValue IS NOT NULL', + $alias); + break; + default: + throw new Exception( + pht( + 'No support for applying operator "%s" against '. + 'index of type "%s".', + $op, + $type)); + } + } + } + + if ($constraint_parts) { + $where[] = '('.implode(') OR (', $constraint_parts).')'; + } + break; + } + } + + return $where; + } + /* -( Integration with CustomField )--------------------------------------- */ From 3203fd9eea7d711418037b1a45b7d49de0ea57d8 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 22 Feb 2018 12:23:41 -0800 Subject: [PATCH 11/13] Support "Any Value" and "No Value" search constraints for datasource Custom Fields Summary: Depends on D19126. Ref T13090. For datasource custom fields, this proxies the datasource and provides "none()" and "any()" functions to allow you to search for objects with no values or any values. Test Plan: - Created a custom "Owning Group" field in Maniphest using a Projects datasource. - For a task with no owner assigned, searched for "none()" (hit) and "any()" (miss). - Assigned the task to an owning project. - Searched for "none()" (miss), "any()" (hit), the project it is now a member of (hit) and some random other project (miss). Maniphest Tasks: T13090 Differential Revision: https://secure.phabricator.com/D19127 --- src/__phutil_library_map__.php | 8 +++ .../PhabricatorTypeaheadProxyDatasource.php | 58 +++++++++++++++ ...ApplicationSearchAnyFunctionDatasource.php | 70 ++++++++++++++++++ ...CustomFieldApplicationSearchDatasource.php | 17 +++++ ...pplicationSearchNoneFunctionDatasource.php | 72 +++++++++++++++++++ ...habricatorStandardCustomFieldTokenizer.php | 25 ++++++- 6 files changed, 249 insertions(+), 1 deletion(-) create mode 100644 src/applications/typeahead/datasource/PhabricatorTypeaheadProxyDatasource.php create mode 100644 src/infrastructure/customfield/datasource/PhabricatorCustomFieldApplicationSearchAnyFunctionDatasource.php create mode 100644 src/infrastructure/customfield/datasource/PhabricatorCustomFieldApplicationSearchDatasource.php create mode 100644 src/infrastructure/customfield/datasource/PhabricatorCustomFieldApplicationSearchNoneFunctionDatasource.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index f7cda9d8fa..6fd36a1e38 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -2617,6 +2617,9 @@ phutil_register_library_map(array( 'PhabricatorCountdownViewController' => 'applications/countdown/controller/PhabricatorCountdownViewController.php', 'PhabricatorCursorPagedPolicyAwareQuery' => 'infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php', 'PhabricatorCustomField' => 'infrastructure/customfield/field/PhabricatorCustomField.php', + 'PhabricatorCustomFieldApplicationSearchAnyFunctionDatasource' => 'infrastructure/customfield/datasource/PhabricatorCustomFieldApplicationSearchAnyFunctionDatasource.php', + 'PhabricatorCustomFieldApplicationSearchDatasource' => 'infrastructure/customfield/datasource/PhabricatorCustomFieldApplicationSearchDatasource.php', + 'PhabricatorCustomFieldApplicationSearchNoneFunctionDatasource' => 'infrastructure/customfield/datasource/PhabricatorCustomFieldApplicationSearchNoneFunctionDatasource.php', 'PhabricatorCustomFieldAttachment' => 'infrastructure/customfield/field/PhabricatorCustomFieldAttachment.php', 'PhabricatorCustomFieldConfigOptionType' => 'infrastructure/customfield/config/PhabricatorCustomFieldConfigOptionType.php', 'PhabricatorCustomFieldDataNotAvailableException' => 'infrastructure/customfield/exception/PhabricatorCustomFieldDataNotAvailableException.php', @@ -4387,6 +4390,7 @@ phutil_register_library_map(array( 'PhabricatorTypeaheadInvalidTokenException' => 'applications/typeahead/exception/PhabricatorTypeaheadInvalidTokenException.php', 'PhabricatorTypeaheadModularDatasourceController' => 'applications/typeahead/controller/PhabricatorTypeaheadModularDatasourceController.php', 'PhabricatorTypeaheadMonogramDatasource' => 'applications/typeahead/datasource/PhabricatorTypeaheadMonogramDatasource.php', + 'PhabricatorTypeaheadProxyDatasource' => 'applications/typeahead/datasource/PhabricatorTypeaheadProxyDatasource.php', 'PhabricatorTypeaheadResult' => 'applications/typeahead/storage/PhabricatorTypeaheadResult.php', 'PhabricatorTypeaheadRuntimeCompositeDatasource' => 'applications/typeahead/datasource/PhabricatorTypeaheadRuntimeCompositeDatasource.php', 'PhabricatorTypeaheadTestNumbersDatasource' => 'applications/typeahead/datasource/__tests__/PhabricatorTypeaheadTestNumbersDatasource.php', @@ -8122,6 +8126,9 @@ phutil_register_library_map(array( 'PhabricatorCountdownViewController' => 'PhabricatorCountdownController', 'PhabricatorCursorPagedPolicyAwareQuery' => 'PhabricatorPolicyAwareQuery', 'PhabricatorCustomField' => 'Phobject', + 'PhabricatorCustomFieldApplicationSearchAnyFunctionDatasource' => 'PhabricatorTypeaheadDatasource', + 'PhabricatorCustomFieldApplicationSearchDatasource' => 'PhabricatorTypeaheadProxyDatasource', + 'PhabricatorCustomFieldApplicationSearchNoneFunctionDatasource' => 'PhabricatorTypeaheadDatasource', 'PhabricatorCustomFieldAttachment' => 'Phobject', 'PhabricatorCustomFieldConfigOptionType' => 'PhabricatorConfigOptionType', 'PhabricatorCustomFieldDataNotAvailableException' => 'Exception', @@ -10182,6 +10189,7 @@ phutil_register_library_map(array( 'PhabricatorTypeaheadInvalidTokenException' => 'Exception', 'PhabricatorTypeaheadModularDatasourceController' => 'PhabricatorTypeaheadDatasourceController', 'PhabricatorTypeaheadMonogramDatasource' => 'PhabricatorTypeaheadDatasource', + 'PhabricatorTypeaheadProxyDatasource' => 'PhabricatorTypeaheadCompositeDatasource', 'PhabricatorTypeaheadResult' => 'Phobject', 'PhabricatorTypeaheadRuntimeCompositeDatasource' => 'PhabricatorTypeaheadCompositeDatasource', 'PhabricatorTypeaheadTestNumbersDatasource' => 'PhabricatorTypeaheadDatasource', diff --git a/src/applications/typeahead/datasource/PhabricatorTypeaheadProxyDatasource.php b/src/applications/typeahead/datasource/PhabricatorTypeaheadProxyDatasource.php new file mode 100644 index 0000000000..3feb2d054d --- /dev/null +++ b/src/applications/typeahead/datasource/PhabricatorTypeaheadProxyDatasource.php @@ -0,0 +1,58 @@ +datasource = $datasource; + $this->setParameters( + array( + 'class' => get_class($datasource), + 'parameters' => $datasource->getParameters(), + )); + return $this; + } + + public function getDatasource() { + if (!$this->datasource) { + $class = $this->getParameter('class'); + + $parent = 'PhabricatorTypeaheadDatasource'; + if (!is_subclass_of($class, $parent)) { + throw new Exception( + pht( + 'Configured datasource class "%s" must be a valid subclass of '. + '"%s".', + $class, + $parent)); + } + + $datasource = newv($class, array()); + $datasource->setParameters($this->getParameter('parameters', array())); + $this->datasource = $datasource; + } + + return $this->datasource; + } + + public function getComponentDatasources() { + return array( + $this->getDatasource(), + ); + } + + public function getDatasourceApplicationClass() { + return $this->getDatasource()->getDatasourceApplicationClass(); + } + + public function getBrowseTitle() { + return $this->getDatasource()->getBrowseTitle(); + } + + public function getPlaceholderText() { + return $this->getDatasource()->getPlaceholderText(); + } + +} diff --git a/src/infrastructure/customfield/datasource/PhabricatorCustomFieldApplicationSearchAnyFunctionDatasource.php b/src/infrastructure/customfield/datasource/PhabricatorCustomFieldApplicationSearchAnyFunctionDatasource.php new file mode 100644 index 0000000000..9560620db4 --- /dev/null +++ b/src/infrastructure/customfield/datasource/PhabricatorCustomFieldApplicationSearchAnyFunctionDatasource.php @@ -0,0 +1,70 @@ + array( + 'name' => pht('Any Value'), + 'summary' => pht('Find results with any value.'), + 'description' => pht( + "This function includes results which have any value. Use a query ". + "like this to find results with any value:\n\n%s". + '> any()'), + ), + ); + } + + public function loadResults() { + $results = array( + $this->newAnyFunction(), + ); + return $this->filterResultsAgainstTokens($results); + } + + protected function evaluateFunction($function, array $argv_list) { + $results = array(); + + foreach ($argv_list as $argv) { + $results[] = new PhabricatorQueryConstraint( + PhabricatorQueryConstraint::OPERATOR_ANY, + null); + } + + return $results; + } + + public function renderFunctionTokens($function, array $argv_list) { + $results = array(); + foreach ($argv_list as $argv) { + $results[] = PhabricatorTypeaheadTokenView::newFromTypeaheadResult( + $this->newAnyFunction()); + } + return $results; + } + + private function newAnyFunction() { + $name = pht('Any Value'); + return $this->newFunctionResult() + ->setName($name.' any') + ->setDisplayName($name) + ->setIcon('fa-circle-o') + ->setPHID('any()') + ->setUnique(true) + ->addAttribute(pht('Select results with any value.')); + } + +} diff --git a/src/infrastructure/customfield/datasource/PhabricatorCustomFieldApplicationSearchDatasource.php b/src/infrastructure/customfield/datasource/PhabricatorCustomFieldApplicationSearchDatasource.php new file mode 100644 index 0000000000..7ba10e84ff --- /dev/null +++ b/src/infrastructure/customfield/datasource/PhabricatorCustomFieldApplicationSearchDatasource.php @@ -0,0 +1,17 @@ + array( + 'name' => pht('No Value'), + 'summary' => pht('Find results with no value.'), + 'description' => pht( + "This function includes results which have no value. Use a query ". + "like this to find results with no value:\n\n%s\n\n", + 'If you combine this function with other constraints, results '. + 'which have no value or the specified values will be returned.', + '> any()'), + ), + ); + } + + public function loadResults() { + $results = array( + $this->newNoneFunction(), + ); + return $this->filterResultsAgainstTokens($results); + } + + protected function evaluateFunction($function, array $argv_list) { + $results = array(); + + foreach ($argv_list as $argv) { + $results[] = new PhabricatorQueryConstraint( + PhabricatorQueryConstraint::OPERATOR_NULL, + null); + } + + return $results; + } + + public function renderFunctionTokens($function, array $argv_list) { + $results = array(); + foreach ($argv_list as $argv) { + $results[] = PhabricatorTypeaheadTokenView::newFromTypeaheadResult( + $this->newNoneFunction()); + } + return $results; + } + + private function newNoneFunction() { + $name = pht('No Value'); + return $this->newFunctionResult() + ->setName($name.' none') + ->setDisplayName($name) + ->setIcon('fa-ban') + ->setPHID('none()') + ->setUnique(true) + ->addAttribute(pht('Select results with no value.')); + } + +} diff --git a/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldTokenizer.php b/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldTokenizer.php index cdb54fb7e9..9bf59d41f6 100644 --- a/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldTokenizer.php +++ b/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldTokenizer.php @@ -33,12 +33,28 @@ abstract class PhabricatorStandardCustomFieldTokenizer $control = id(new AphrontFormTokenizerControl()) ->setLabel($this->getFieldName()) ->setName($this->getFieldKey()) - ->setDatasource($this->getDatasource()) + ->setDatasource($this->newApplicationSearchDatasource()) ->setValue(nonempty($value, array())); $form->appendControl($control); } + public function applyApplicationSearchConstraintToQuery( + PhabricatorApplicationSearchEngine $engine, + PhabricatorCursorPagedPolicyAwareQuery $query, + $value) { + if ($value) { + + $datasource = $this->newApplicationSearchDatasource() + ->setViewer($this->getViewer()); + $value = $datasource->evaluateTokens($value); + + $query->withApplicationSearchContainsConstraint( + $this->newStringIndex(null), + $value); + } + } + public function getHeraldFieldValueType($condition) { return id(new HeraldTokenizerFieldValue()) ->setKey('custom.'.$this->getFieldKey()) @@ -120,4 +136,11 @@ abstract class PhabricatorStandardCustomFieldTokenizer } } + protected function newApplicationSearchDatasource() { + $datasource = $this->getDatasource(); + + return id(new PhabricatorCustomFieldApplicationSearchDatasource()) + ->setDatasource($datasource); + } + } From 8ae01fdc6bf620c44ebf0853627fb059881d0b15 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 22 Feb 2018 12:51:23 -0800 Subject: [PATCH 12/13] Fix documentation behaviors for the new proxy functions for custom datasource fields Summary: Ref T13090. The doc string in "any()" wasn't specified correctly and the help page wasn't getting enough supporting data to build properly. Test Plan: Viewed "Reference: Advanced Functions" for a custom datasource field and got more helpful help. Maniphest Tasks: T13090 Differential Revision: https://secure.phabricator.com/D19128 --- ...ricatorTypeaheadFunctionHelpController.php | 20 +++++++++++++++++-- ...orTypeaheadModularDatasourceController.php | 6 ++++++ ...ApplicationSearchAnyFunctionDatasource.php | 2 +- 3 files changed, 25 insertions(+), 3 deletions(-) diff --git a/src/applications/typeahead/controller/PhabricatorTypeaheadFunctionHelpController.php b/src/applications/typeahead/controller/PhabricatorTypeaheadFunctionHelpController.php index 3084b2d434..27b30a6278 100644 --- a/src/applications/typeahead/controller/PhabricatorTypeaheadFunctionHelpController.php +++ b/src/applications/typeahead/controller/PhabricatorTypeaheadFunctionHelpController.php @@ -19,9 +19,25 @@ final class PhabricatorTypeaheadFunctionHelpController return new Aphront404Response(); } - $source = $sources[$class]; + $raw_parameters = $request->getStr('parameters'); + if ($raw_parameters) { + $parameters = phutil_json_decode($raw_parameters); + } else { + $parameters = array(); + } + + $source = id(clone $sources[$class]) + ->setParameters($parameters); + + // This can fail for some types of datasources (like the custom field proxy + // datasources) if the "parameters" are wrong. Just fail cleanly instead + // of fataling. + try { + $application_class = $source->getDatasourceApplicationClass(); + } catch (Exception $ex) { + return new Aphront404Response(); + } - $application_class = $source->getDatasourceApplicationClass(); if ($application_class) { $result = id(new PhabricatorApplicationQuery()) ->setViewer($this->getViewer()) diff --git a/src/applications/typeahead/controller/PhabricatorTypeaheadModularDatasourceController.php b/src/applications/typeahead/controller/PhabricatorTypeaheadModularDatasourceController.php index 2f55c26384..e0ba9a763b 100644 --- a/src/applications/typeahead/controller/PhabricatorTypeaheadModularDatasourceController.php +++ b/src/applications/typeahead/controller/PhabricatorTypeaheadModularDatasourceController.php @@ -226,6 +226,12 @@ final class PhabricatorTypeaheadModularDatasourceController if ($source->getAllDatasourceFunctions()) { $reference_uri = '/typeahead/help/'.get_class($source).'/'; + $parameters = $source->getParameters(); + if ($parameters) { + $reference_uri = (string)id(new PhutilURI($reference_uri)) + ->setQueryParam('parameters', phutil_json_encode($parameters)); + } + $reference_link = phutil_tag( 'a', array( diff --git a/src/infrastructure/customfield/datasource/PhabricatorCustomFieldApplicationSearchAnyFunctionDatasource.php b/src/infrastructure/customfield/datasource/PhabricatorCustomFieldApplicationSearchAnyFunctionDatasource.php index 9560620db4..31f189988c 100644 --- a/src/infrastructure/customfield/datasource/PhabricatorCustomFieldApplicationSearchAnyFunctionDatasource.php +++ b/src/infrastructure/customfield/datasource/PhabricatorCustomFieldApplicationSearchAnyFunctionDatasource.php @@ -22,7 +22,7 @@ final class PhabricatorCustomFieldApplicationSearchAnyFunctionDatasource 'summary' => pht('Find results with any value.'), 'description' => pht( "This function includes results which have any value. Use a query ". - "like this to find results with any value:\n\n%s". + "like this to find results with any value:\n\n%s", '> any()'), ), ); From 4c7370a1a3b0b59f4ce6fc731a59b637f75c0ed2 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 22 Feb 2018 13:31:20 -0800 Subject: [PATCH 13/13] Make the filetree view width sticky across show/hide and reload Summary: Ref T13090. The default width changed recently to become much wider, but the behavior on this control isn't great. Instead: - Pick a default width somewhere between the two. - Make the width sticky across show/hide (pressing "f" twice remembers your width instead of resetting it). - Make the width sticky across reloads (dragging the bar, then reloading the page keeps the bar in the same place). Test Plan: - Without settings, loaded page: got medium-width bar. - Dragged bar wide/narrow, toggled on/off with "f", got persistent width. - Dragged bar wide/narrow, reloaded page, got persistent width. - Dragged bar wide/narrow, toggled it off, reloaded page, toggled it on, got persistent width. Maniphest Tasks: T13090 Differential Revision: https://secure.phabricator.com/D19129 --- resources/celerity/map.php | 22 ++++---- src/__phutil_library_map__.php | 2 + .../DifferentialRevisionViewController.php | 4 ++ ...rentialChangesetFileTreeSideNavBuilder.php | 15 +++-- .../controller/DiffusionCommitController.php | 12 ++-- .../PhabricatorFiletreeWidthSetting.php | 12 ++++ src/view/layout/AphrontSideNavFilterView.php | 55 ++++++++++++++----- .../rsrc/css/aphront/phabricator-nav-view.css | 6 +- .../rsrc/js/core/behavior-phabricator-nav.js | 35 +++++++++++- 9 files changed, 124 insertions(+), 39 deletions(-) create mode 100644 src/applications/settings/setting/PhabricatorFiletreeWidthSetting.php diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 32ccb9e5f9..01dd75c901 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -9,8 +9,8 @@ return array( 'names' => array( 'conpherence.pkg.css' => 'e68cf1fa', 'conpherence.pkg.js' => '15191c65', - 'core.pkg.css' => 'e4f098a5', - 'core.pkg.js' => 'bd19de1c', + 'core.pkg.css' => '2fa91e14', + 'core.pkg.js' => 'dc13d4b7', 'darkconsole.pkg.js' => '1f9a31bc', 'differential.pkg.css' => '113e692c', 'differential.pkg.js' => 'f6d809c0', @@ -31,7 +31,7 @@ return array( 'rsrc/css/aphront/multi-column.css' => '84cc6640', 'rsrc/css/aphront/notification.css' => '457861ec', 'rsrc/css/aphront/panel-view.css' => '8427b78d', - 'rsrc/css/aphront/phabricator-nav-view.css' => '028126f6', + 'rsrc/css/aphront/phabricator-nav-view.css' => 'a9e3e6d5', 'rsrc/css/aphront/table-view.css' => '8c9bbafe', 'rsrc/css/aphront/tokenizer.css' => '15d5ff71', 'rsrc/css/aphront/tooltip.css' => '173b9431', @@ -498,7 +498,7 @@ return array( 'rsrc/js/core/behavior-more.js' => 'a80d0378', 'rsrc/js/core/behavior-object-selector.js' => '77c1f0b0', 'rsrc/js/core/behavior-oncopy.js' => '2926fff2', - 'rsrc/js/core/behavior-phabricator-nav.js' => '81144dfa', + 'rsrc/js/core/behavior-phabricator-nav.js' => '836f966d', 'rsrc/js/core/behavior-phabricator-remarkup-assist.js' => 'acd29eee', 'rsrc/js/core/behavior-read-only-warning.js' => 'ba158207', 'rsrc/js/core/behavior-refresh-csrf.js' => 'ab2f381b', @@ -657,7 +657,7 @@ return array( 'javelin-behavior-phabricator-keyboard-pager' => 'a8da01f0', 'javelin-behavior-phabricator-keyboard-shortcuts' => '01fca1f0', 'javelin-behavior-phabricator-line-linker' => '1499a8cb', - 'javelin-behavior-phabricator-nav' => '81144dfa', + 'javelin-behavior-phabricator-nav' => '836f966d', 'javelin-behavior-phabricator-notification-example' => '8ce821c5', 'javelin-behavior-phabricator-object-selector' => '77c1f0b0', 'javelin-behavior-phabricator-oncopy' => '2926fff2', @@ -789,7 +789,7 @@ return array( 'phabricator-keyboard-shortcut' => '1ae869f2', 'phabricator-keyboard-shortcut-manager' => 'c19dd9b9', 'phabricator-main-menu-view' => '1802a242', - 'phabricator-nav-view-css' => '028126f6', + 'phabricator-nav-view-css' => 'a9e3e6d5', 'phabricator-notification' => '008faf9c', 'phabricator-notification-css' => '457861ec', 'phabricator-notification-menu-css' => '10685bd4', @@ -1563,7 +1563,11 @@ return array( 'phuix-icon-view', 'phabricator-prefab', ), - '81144dfa' => array( + '834a1173' => array( + 'javelin-behavior', + 'javelin-scrollbar', + ), + '836f966d' => array( 'javelin-behavior', 'javelin-behavior-device', 'javelin-stratcom', @@ -1573,10 +1577,6 @@ return array( 'javelin-request', 'javelin-util', ), - '834a1173' => array( - 'javelin-behavior', - 'javelin-scrollbar', - ), '8499b6ab' => array( 'javelin-behavior', 'javelin-dom', diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 6fd36a1e38..4455ad139a 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -3046,6 +3046,7 @@ phutil_register_library_map(array( 'PhabricatorFilesOnDiskBuiltinFile' => 'applications/files/builtin/PhabricatorFilesOnDiskBuiltinFile.php', 'PhabricatorFilesOutboundRequestAction' => 'applications/files/action/PhabricatorFilesOutboundRequestAction.php', 'PhabricatorFiletreeVisibleSetting' => 'applications/settings/setting/PhabricatorFiletreeVisibleSetting.php', + 'PhabricatorFiletreeWidthSetting' => 'applications/settings/setting/PhabricatorFiletreeWidthSetting.php', 'PhabricatorFlag' => 'applications/flag/storage/PhabricatorFlag.php', 'PhabricatorFlagAddFlagHeraldAction' => 'applications/flag/herald/PhabricatorFlagAddFlagHeraldAction.php', 'PhabricatorFlagColor' => 'applications/flag/constants/PhabricatorFlagColor.php', @@ -8613,6 +8614,7 @@ phutil_register_library_map(array( 'PhabricatorFilesOnDiskBuiltinFile' => 'PhabricatorFilesBuiltinFile', 'PhabricatorFilesOutboundRequestAction' => 'PhabricatorSystemAction', 'PhabricatorFiletreeVisibleSetting' => 'PhabricatorInternalSetting', + 'PhabricatorFiletreeWidthSetting' => 'PhabricatorInternalSetting', 'PhabricatorFlag' => array( 'PhabricatorFlagDAO', 'PhabricatorPolicyInterface', diff --git a/src/applications/differential/controller/DifferentialRevisionViewController.php b/src/applications/differential/controller/DifferentialRevisionViewController.php index e1da5ed32e..d7280cb0b6 100644 --- a/src/applications/differential/controller/DifferentialRevisionViewController.php +++ b/src/applications/differential/controller/DifferentialRevisionViewController.php @@ -473,10 +473,14 @@ final class DifferentialRevisionViewController extends DifferentialController { $collapsed_key = PhabricatorFiletreeVisibleSetting::SETTINGKEY; $collapsed_value = $viewer->getUserSetting($collapsed_key); + $width_key = PhabricatorFiletreeWidthSetting::SETTINGKEY; + $width_value = $viewer->getUserSetting($width_key); + $nav = id(new DifferentialChangesetFileTreeSideNavBuilder()) ->setTitle($monogram) ->setBaseURI(new PhutilURI($revision->getURI())) ->setCollapsed((bool)$collapsed_value) + ->setWidth((int)$width_value) ->build($changesets); } diff --git a/src/applications/differential/view/DifferentialChangesetFileTreeSideNavBuilder.php b/src/applications/differential/view/DifferentialChangesetFileTreeSideNavBuilder.php index 1f699be8eb..e8852a5c52 100644 --- a/src/applications/differential/view/DifferentialChangesetFileTreeSideNavBuilder.php +++ b/src/applications/differential/view/DifferentialChangesetFileTreeSideNavBuilder.php @@ -6,6 +6,7 @@ final class DifferentialChangesetFileTreeSideNavBuilder extends Phobject { private $baseURI; private $anchorName; private $collapsed = false; + private $width; public function setAnchorName($anchor_name) { $this->anchorName = $anchor_name; @@ -36,13 +37,19 @@ final class DifferentialChangesetFileTreeSideNavBuilder extends Phobject { return $this; } + public function setWidth($width) { + $this->width = $width; + return $this; + } + public function build(array $changesets) { assert_instances_of($changesets, 'DifferentialChangeset'); - $nav = new AphrontSideNavFilterView(); - $nav->setBaseURI($this->getBaseURI()); - $nav->setFlexible(true); - $nav->setCollapsed($this->collapsed); + $nav = id(new AphrontSideNavFilterView()) + ->setBaseURI($this->getBaseURI()) + ->setFlexible(true) + ->setCollapsed($this->collapsed) + ->setWidth($this->width); $anchor = $this->getAnchorName(); diff --git a/src/applications/diffusion/controller/DiffusionCommitController.php b/src/applications/diffusion/controller/DiffusionCommitController.php index b4d06989c2..67ffb73357 100644 --- a/src/applications/diffusion/controller/DiffusionCommitController.php +++ b/src/applications/diffusion/controller/DiffusionCommitController.php @@ -415,17 +415,21 @@ final class DiffusionCommitController extends DiffusionController { PhabricatorShowFiletreeSetting::SETTINGKEY, PhabricatorShowFiletreeSetting::VALUE_ENABLE_FILETREE); - $pref_collapse = PhabricatorFiletreeVisibleSetting::SETTINGKEY; - $collapsed = $viewer->getUserSetting($pref_collapse); - $nav = null; if ($show_changesets && $filetree_on) { + $pref_collapse = PhabricatorFiletreeVisibleSetting::SETTINGKEY; + $collapsed = $viewer->getUserSetting($pref_collapse); + + $pref_width = PhabricatorFiletreeWidthSetting::SETTINGKEY; + $width = $viewer->getUserSetting($pref_width); + $nav = id(new DifferentialChangesetFileTreeSideNavBuilder()) ->setTitle($commit->getDisplayName()) ->setBaseURI(new PhutilURI($commit->getURI())) ->build($changesets) ->setCrumbs($crumbs) - ->setCollapsed((bool)$collapsed); + ->setCollapsed((bool)$collapsed) + ->setWidth((int)$width); } $view = id(new PHUITwoColumnView()) diff --git a/src/applications/settings/setting/PhabricatorFiletreeWidthSetting.php b/src/applications/settings/setting/PhabricatorFiletreeWidthSetting.php new file mode 100644 index 0000000000..5cfc9f3fe7 --- /dev/null +++ b/src/applications/settings/setting/PhabricatorFiletreeWidthSetting.php @@ -0,0 +1,12 @@ +menuID = $menu_id; @@ -82,6 +83,11 @@ final class AphrontSideNavFilterView extends AphrontView { return $this; } + public function setWidth($width) { + $this->width = $width; + return $this; + } + public function getMenuView() { return $this->menu; } @@ -216,6 +222,24 @@ final class AphrontSideNavFilterView extends AphrontView { $local_menu = null; $main_id = $this->getMainID(); + $width = $this->width; + if ($width) { + $width = min($width, 600); + $width = max($width, 150); + } else { + $width = null; + } + + if ($width && !$this->collapsed) { + $width_drag_style = 'left: '.$width.'px'; + $width_panel_style = 'width: '.$width.'px'; + $width_margin_style = 'margin-left: '.($width + 7).'px'; + } else { + $width_drag_style = null; + $width_panel_style = null; + $width_margin_style = null; + } + if ($this->flexible) { $drag_id = celerity_generate_unique_node_id(); $flex_bar = phutil_tag( @@ -223,6 +247,7 @@ final class AphrontSideNavFilterView extends AphrontView { array( 'class' => 'phabricator-nav-drag', 'id' => $drag_id, + 'style' => $width_drag_style, ), ''); } else { @@ -238,14 +263,14 @@ final class AphrontSideNavFilterView extends AphrontView { $nav_classes[] = 'has-local-nav'; } - $local_menu = - phutil_tag( - 'div', - array( - 'class' => 'phabricator-nav-local phabricator-side-menu', - 'id' => $local_id, - ), - $this->menu->setID($this->getMenuID())); + $local_menu = phutil_tag( + 'div', + array( + 'class' => 'phabricator-nav-local phabricator-side-menu', + 'id' => $local_id, + 'style' => $width_panel_style, + ), + $this->menu->setID($this->getMenuID())); } $crumbs = null; @@ -264,12 +289,13 @@ final class AphrontSideNavFilterView extends AphrontView { Javelin::initBehavior( 'phabricator-nav', array( - 'mainID' => $main_id, - 'localID' => $local_id, - 'dragID' => $drag_id, - 'contentID' => $content_id, - 'backgroundID' => $background_id, - 'collapsed' => $this->collapsed, + 'mainID' => $main_id, + 'localID' => $local_id, + 'dragID' => $drag_id, + 'contentID' => $content_id, + 'backgroundID' => $background_id, + 'collapsed' => $this->collapsed, + 'width' => $width, )); if ($this->active) { @@ -297,6 +323,7 @@ final class AphrontSideNavFilterView extends AphrontView { array( 'class' => 'phabricator-nav-content plb', 'id' => $content_id, + 'style' => $width_margin_style, ), array( $crumbs, diff --git a/webroot/rsrc/css/aphront/phabricator-nav-view.css b/webroot/rsrc/css/aphront/phabricator-nav-view.css index f3320e3eae..6dbd0931a6 100644 --- a/webroot/rsrc/css/aphront/phabricator-nav-view.css +++ b/webroot/rsrc/css/aphront/phabricator-nav-view.css @@ -44,7 +44,7 @@ position: fixed; top: 0; bottom: 0; - left: 410px; + left: 310px; width: 7px; cursor: col-resize; @@ -66,7 +66,7 @@ .device-desktop .phabricator-standard-page-body .has-drag-nav .phabricator-nav-content { - margin-left: 417px; + margin-left: 317px; } .device-desktop .phabricator-standard-page-body .has-drag-nav @@ -81,7 +81,7 @@ } .device-desktop .phui-navigation-shell .has-drag-nav .phabricator-nav-local { - width: 410px; + width: 310px; padding: 0; background: transparent; } diff --git a/webroot/rsrc/js/core/behavior-phabricator-nav.js b/webroot/rsrc/js/core/behavior-phabricator-nav.js index 74909e447d..50d8e1c820 100644 --- a/webroot/rsrc/js/core/behavior-phabricator-nav.js +++ b/webroot/rsrc/js/core/behavior-phabricator-nav.js @@ -18,7 +18,6 @@ JX.behavior('phabricator-nav', function(config) { var main = JX.$(config.mainID); var drag = JX.$(config.dragID); - // - Flexible Navigation Column ------------------------------------------------ @@ -98,22 +97,52 @@ JX.behavior('phabricator-nav', function(config) { } JX.DOM.alterClass(document.body, 'jx-drag-col', false); dragging = false; + + new JX.Request('/settings/adjust/', JX.bag) + .setData( + { + key: 'filetree.width', + value: JX.$V(drag).x + }) + .send(); }); - function resetdrag() { + var saved_width = config.width; + function savedrag() { + saved_width = JX.$V(drag).x; + local.style.width = ''; drag.style.left = ''; content.style.marginLeft = ''; } + function restoredrag() { + if (!saved_width) { + return; + } + + local.style.width = saved_width + 'px'; + drag.style.left = saved_width + 'px'; + content.style.marginLeft = (saved_width + JX.Vector.getDim(drag).x) + 'px'; + } + var collapsed = config.collapsed; JX.Stratcom.listen('differential-filetree-toggle', null, function() { collapsed = !collapsed; + + if (collapsed) { + savedrag(); + } + JX.DOM.alterClass(main, 'has-local-nav', !collapsed); JX.DOM.alterClass(main, 'has-drag-nav', !collapsed); JX.DOM.alterClass(main, 'has-closed-nav', collapsed); - resetdrag(); + + if (!collapsed) { + restoredrag(); + } + new JX.Request('/settings/adjust/', JX.bag) .setData({ key : 'nav-collapsed', value : (collapsed ? 1 : 0) }) .send();