From efed12400d2811effd1e166e2911e2706aab9ea7 Mon Sep 17 00:00:00 2001 From: vrana Date: Wed, 10 Oct 2012 12:25:33 -0700 Subject: [PATCH] Load all diff properties in revision view Summary: We need to load all properties with some prefix in one field. We can't merge them in one property because there will be a race condition for update (we don't have API for load+update+save). Instead of providing API for this and complicating the code even more, just load everything unconditionally. It shouldn't waste much bandwith or memory because we use most of the properties anyway. It also looked overengineered to me. Test Plan: Displayed revision with fields using diff properties. Reviewers: epriestley, royw Reviewed By: royw CC: aran, royw, Korvin Differential Revision: https://secure.phabricator.com/D3676 --- .../DifferentialRevisionViewController.php | 64 +++---------------- .../DifferentialFieldSpecification.php | 22 +------ .../DifferentialLintFieldSpecification.php | 4 -- .../DifferentialUnitFieldSpecification.php | 4 -- 4 files changed, 12 insertions(+), 82 deletions(-) diff --git a/src/applications/differential/controller/DifferentialRevisionViewController.php b/src/applications/differential/controller/DifferentialRevisionViewController.php index 8fcaec8397..04e0005ea5 100644 --- a/src/applications/differential/controller/DifferentialRevisionViewController.php +++ b/src/applications/differential/controller/DifferentialRevisionViewController.php @@ -82,16 +82,12 @@ final class DifferentialRevisionViewController extends DifferentialController { $repository); } - list($aux_fields, $props) = $this->loadAuxiliaryFieldsAndProperties( - $revision, - $target, - $target_manual, - array( - 'local:commits', - 'arc:lint', - 'arc:unit', - )); + $props = id(new DifferentialDiffProperty())->loadAllWhere( + 'diffID = %d', + $target_manual->getID()); + $props = mpull($props, 'getData', 'getName'); + $aux_fields = $this->loadAuxiliaryFields($revision); $comments = $revision->loadComments(); $comments = array_merge( @@ -139,6 +135,9 @@ final class DifferentialRevisionViewController extends DifferentialController { $aux_phids = array(); foreach ($aux_fields as $key => $aux_field) { + $aux_field->setDiff($target); + $aux_field->setManualDiff($target_manual); + $aux_field->setDiffProperties($props); $aux_phids[$key] = $aux_field->getRequiredHandlePHIDsForRevisionView(); } $object_phids = array_merge($object_phids, array_mergev($aux_phids)); @@ -740,11 +739,7 @@ final class DifferentialRevisionViewController extends DifferentialController { return array($changesets, $vs_map, $vs_changesets, $refs); } - private function loadAuxiliaryFieldsAndProperties( - DifferentialRevision $revision, - DifferentialDiff $diff, - DifferentialDiff $manual_diff, - array $special_properties) { + private function loadAuxiliaryFields(DifferentialRevision $revision) { $aux_fields = DifferentialFieldSelector::newSelector() ->getFieldSpecifications(); @@ -760,46 +755,7 @@ final class DifferentialRevisionViewController extends DifferentialController { $revision, $aux_fields); - $aux_props = array(); - foreach ($aux_fields as $key => $aux_field) { - $aux_field->setDiff($diff); - $aux_field->setManualDiff($manual_diff); - $aux_props[$key] = $aux_field->getRequiredDiffProperties(); - } - - $required_properties = array_mergev($aux_props); - $required_properties = array_merge( - $required_properties, - $special_properties); - - $property_map = array(); - if ($required_properties) { - $properties = id(new DifferentialDiffProperty())->loadAllWhere( - 'diffID = %d AND name IN (%Ls)', - $manual_diff->getID(), - $required_properties); - $property_map = mpull($properties, 'getData', 'getName'); - } - - foreach ($aux_fields as $key => $aux_field) { - // Give each field only the properties it specifically required, and - // set 'null' for each requested key which we didn't actually load a - // value for (otherwise, getDiffProperty() will throw). - if ($aux_props[$key]) { - $props = array_select_keys($property_map, $aux_props[$key]) + - array_fill_keys($aux_props[$key], null); - } else { - $props = array(); - } - - $aux_field->setDiffProperties($props); - } - - return array( - $aux_fields, - array_select_keys( - $property_map, - $special_properties)); + return $aux_fields; } private function buildSymbolIndexes( diff --git a/src/applications/differential/field/specification/DifferentialFieldSpecification.php b/src/applications/differential/field/specification/DifferentialFieldSpecification.php index 868c78207a..db26844b08 100644 --- a/src/applications/differential/field/specification/DifferentialFieldSpecification.php +++ b/src/applications/differential/field/specification/DifferentialFieldSpecification.php @@ -676,16 +676,6 @@ abstract class DifferentialFieldSpecification { return $this->getRequiredHandlePHIDs(); } - /** - * Specify which diff properties this field needs to load. - * - * @return list List of diff property keys this field requires. - * @task load - */ - public function getRequiredDiffProperties() { - return array(); - } - /** * Parse a list of users into a canonical PHID list. * @@ -904,8 +894,7 @@ abstract class DifferentialFieldSpecification { } /** - * Get a diff property which this field previously requested by returning - * the key from @{method:getRequiredDiffProperties}. + * Get a property of a diff set by @{method:setManualDiff}. * * @param string Diff property key. * @return mixed|null Diff property, or null if the property does not have @@ -919,14 +908,7 @@ abstract class DifferentialFieldSpecification { // context. throw new DifferentialFieldDataNotAvailableException($this); } - if (!array_key_exists($key, $this->diffProperties)) { - $class = get_class($this); - throw new Exception( - "A differential field (of class '{$class}') is attempting to retrieve ". - "a diff property ('{$key}') which it did not request. Return all ". - "diff property keys you need from getRequiredDiffProperties()."); - } - return $this->diffProperties[$key]; + return idx($this->diffProperties, $key); } } diff --git a/src/applications/differential/field/specification/DifferentialLintFieldSpecification.php b/src/applications/differential/field/specification/DifferentialLintFieldSpecification.php index 82525de190..c29fd9b6e3 100644 --- a/src/applications/differential/field/specification/DifferentialLintFieldSpecification.php +++ b/src/applications/differential/field/specification/DifferentialLintFieldSpecification.php @@ -27,10 +27,6 @@ final class DifferentialLintFieldSpecification return 'Lint:'; } - public function getRequiredDiffProperties() { - return array('arc:lint', 'arc:lint-excuse', 'arc:lint-postponed'); - } - private function getLintExcuse() { return $this->getDiffProperty('arc:lint-excuse'); } diff --git a/src/applications/differential/field/specification/DifferentialUnitFieldSpecification.php b/src/applications/differential/field/specification/DifferentialUnitFieldSpecification.php index cf8d49e830..556fc6e521 100644 --- a/src/applications/differential/field/specification/DifferentialUnitFieldSpecification.php +++ b/src/applications/differential/field/specification/DifferentialUnitFieldSpecification.php @@ -27,10 +27,6 @@ final class DifferentialUnitFieldSpecification return 'Unit:'; } - public function getRequiredDiffProperties() { - return array('arc:unit', 'arc:unit-excuse'); - } - private function getUnitExcuse() { return $this->getDiffProperty('arc:unit-excuse'); }