Page MenuHomePhabricator
Files F13197171

D16351.id39328.diff
No OneTemporary
Actions

D16351.id39328.diff
View Options

diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -2258,6 +2258,7 @@
'PhabricatorCustomFieldNumericIndexStorage' => 'infrastructure/customfield/storage/PhabricatorCustomFieldNumericIndexStorage.php',
'PhabricatorCustomFieldSearchEngineExtension' => 'infrastructure/customfield/engineextension/PhabricatorCustomFieldSearchEngineExtension.php',
'PhabricatorCustomFieldStorage' => 'infrastructure/customfield/storage/PhabricatorCustomFieldStorage.php',
+ 'PhabricatorCustomFieldStorageQuery' => 'infrastructure/customfield/query/PhabricatorCustomFieldStorageQuery.php',
'PhabricatorCustomFieldStringIndexStorage' => 'infrastructure/customfield/storage/PhabricatorCustomFieldStringIndexStorage.php',
'PhabricatorCustomHeaderConfigType' => 'applications/config/custom/PhabricatorCustomHeaderConfigType.php',
'PhabricatorDaemon' => 'infrastructure/daemon/PhabricatorDaemon.php',
@@ -6994,6 +6995,7 @@
'PhabricatorCustomFieldNumericIndexStorage' => 'PhabricatorCustomFieldIndexStorage',
'PhabricatorCustomFieldSearchEngineExtension' => 'PhabricatorSearchEngineExtension',
'PhabricatorCustomFieldStorage' => 'PhabricatorLiskDAO',
+ 'PhabricatorCustomFieldStorageQuery' => 'Phobject',
'PhabricatorCustomFieldStringIndexStorage' => 'PhabricatorCustomFieldIndexStorage',
'PhabricatorCustomHeaderConfigType' => 'PhabricatorConfigOptionType',
'PhabricatorDaemon' => 'PhutilDaemon',
diff --git a/src/applications/differential/conduit/DifferentialConduitAPIMethod.php b/src/applications/differential/conduit/DifferentialConduitAPIMethod.php
--- a/src/applications/differential/conduit/DifferentialConduitAPIMethod.php
+++ b/src/applications/differential/conduit/DifferentialConduitAPIMethod.php
@@ -150,21 +150,38 @@
array $revisions) {
assert_instances_of($revisions, 'DifferentialRevision');
- $results = array();
+ $field_lists = array();
foreach ($revisions as $revision) {
- // TODO: This is inefficient and issues a query for each object.
+ $revision_phid = $revision->getPHID();
+
$field_list = PhabricatorCustomField::getObjectFields(
$revision,
PhabricatorCustomField::ROLE_CONDUIT);
$field_list
->setViewer($viewer)
- ->readFieldsFromStorage($revision);
+ ->readFieldsFromObject($revision);
+
+ $field_lists[$revision_phid] = $field_list;
+ }
+
+ $all_fields = array();
+ foreach ($field_lists as $field_list) {
+ foreach ($field_list->getFields() as $field) {
+ $all_fields[] = $field;
+ }
+ }
+ id(new PhabricatorCustomFieldStorageQuery())
+ ->addFields($all_fields)
+ ->execute();
+
+ $results = array();
+ foreach ($field_lists as $revision_phid => $field_list) {
foreach ($field_list->getFields() as $field) {
$field_key = $field->getFieldKeyForConduit();
$value = $field->getConduitDictionaryValue();
- $results[$revision->getPHID()][$field_key] = $value;
+ $results[$revision_phid][$field_key] = $value;
}
}
diff --git a/src/infrastructure/customfield/engineextension/PhabricatorCustomFieldSearchEngineExtension.php b/src/infrastructure/customfield/engineextension/PhabricatorCustomFieldSearchEngineExtension.php
--- a/src/infrastructure/customfield/engineextension/PhabricatorCustomFieldSearchEngineExtension.php
+++ b/src/infrastructure/customfield/engineextension/PhabricatorCustomFieldSearchEngineExtension.php
@@ -80,17 +80,42 @@
return $map;
}
- public function getFieldValuesForConduit($object, $data) {
- // TODO: This is currently very inefficient. We should bulk-load these
- // field values instead.
+ public function loadExtensionConduitData(array $objects) {
+ $viewer = $this->getViewer();
- $fields = PhabricatorCustomField::getObjectFields(
- $object,
- PhabricatorCustomField::ROLE_CONDUIT);
+ $field_map = array();
+ foreach ($objects as $object) {
+ $object_phid = $object->getPHID();
+
+ $fields = PhabricatorCustomField::getObjectFields(
+ $object,
+ PhabricatorCustomField::ROLE_CONDUIT);
+
+ $fields
+ ->setViewer($viewer)
+ ->readFieldsFromObject($object);
+
+ $field_map[$object_phid] = $fields;
+ }
+
+ $all_fields = array();
+ foreach ($field_map as $field_list) {
+ foreach ($field_list->getFields() as $field) {
+ $all_fields[] = $field;
+ }
+ }
+
+ id(new PhabricatorCustomFieldStorageQuery())
+ ->addFields($all_fields)
+ ->execute();
- $fields
- ->setViewer($this->getViewer())
- ->readFieldsFromStorage($object);
+ return array(
+ 'fields' => $field_map,
+ );
+ }
+
+ public function getFieldValuesForConduit($object, $data) {
+ $fields = $data['fields'][$object->getPHID()];
$map = array();
foreach ($fields->getFields() as $field) {
diff --git a/src/infrastructure/customfield/field/PhabricatorCustomFieldList.php b/src/infrastructure/customfield/field/PhabricatorCustomFieldList.php
--- a/src/infrastructure/customfield/field/PhabricatorCustomFieldList.php
+++ b/src/infrastructure/customfield/field/PhabricatorCustomFieldList.php
@@ -29,6 +29,19 @@
return $this;
}
+ public function readFieldsFromObject(
+ PhabricatorCustomFieldInterface $object) {
+
+ $fields = $this->getFields();
+
+ foreach ($fields as $field) {
+ $field
+ ->setObject($object)
+ ->readValueFromObject($object);
+ }
+
+ return $this;
+ }
/**
* Read stored values for all fields which support storage.
@@ -39,48 +52,12 @@
public function readFieldsFromStorage(
PhabricatorCustomFieldInterface $object) {
- foreach ($this->fields as $field) {
- $field->setObject($object);
- $field->readValueFromObject($object);
- }
-
- $keys = array();
- foreach ($this->fields as $field) {
- if ($field->shouldEnableForRole(PhabricatorCustomField::ROLE_STORAGE)) {
- $keys[$field->getFieldIndex()] = $field;
- }
- }
-
- if (!$keys) {
- return $this;
- }
-
- // NOTE: We assume all fields share the same storage. This isn't guaranteed
- // to be true, but always is for now.
+ $this->readFieldsFromObject($object);
- $table = head($keys)->newStorageObject();
-
- $objects = array();
- if ($object->getPHID()) {
- $objects = $table->loadAllWhere(
- 'objectPHID = %s AND fieldIndex IN (%Ls)',
- $object->getPHID(),
- array_keys($keys));
- $objects = mpull($objects, null, 'getFieldIndex');
- }
-
- foreach ($keys as $key => $field) {
- $storage = idx($objects, $key);
- if ($storage) {
- $field->setValueFromStorage($storage->getFieldValue());
- $field->didSetValueFromStorage();
- } else if ($object->getPHID()) {
- // NOTE: We set this only if the object exists. Otherwise, we allow the
- // field to retain any default value it may have.
- $field->setValueFromStorage(null);
- $field->didSetValueFromStorage();
- }
- }
+ $fields = $this->getFields();
+ id(new PhabricatorCustomFieldStorageQuery())
+ ->addFields($fields)
+ ->execute();
return $this;
}
diff --git a/src/infrastructure/customfield/query/PhabricatorCustomFieldStorageQuery.php b/src/infrastructure/customfield/query/PhabricatorCustomFieldStorageQuery.php
new file mode 100644
--- /dev/null
+++ b/src/infrastructure/customfield/query/PhabricatorCustomFieldStorageQuery.php
@@ -0,0 +1,84 @@
+<?php
+
+/**
+ * Load custom field data from storage.
+ *
+ * This query loads the data directly into the field objects and does not
+ * return it to the caller. It can bulk load data for any list of fields,
+ * even if they have different objects or object types.
+ */
+final class PhabricatorCustomFieldStorageQuery extends Phobject {
+
+ private $fieldMap = array();
+ private $storageSources = array();
+
+ public function addFields(array $fields) {
+ assert_instances_of($fields, 'PhabricatorCustomField');
+
+ foreach ($fields as $field) {
+ $this->addField($field);
+ }
+
+ return $this;
+ }
+
+ public function addField(PhabricatorCustomField $field) {
+ $role_storage = PhabricatorCustomField::ROLE_STORAGE;
+
+ if (!$field->shouldEnableForRole($role_storage)) {
+ return $this;
+ }
+
+ $storage = $field->newStorageObject();
+ $source_key = $storage->getStorageSourceKey();
+
+ $this->fieldMap[$source_key][] = $field;
+
+ if (empty($this->storageSources[$source_key])) {
+ $this->storageSources[$source_key] = $storage;
+ }
+
+ return $this;
+ }
+
+ public function execute() {
+ foreach ($this->storageSources as $source_key => $storage) {
+ $fields = idx($this->fieldMap, $source_key, array());
+ $this->loadFieldsFromStorage($storage, $fields);
+ }
+ }
+
+ private function loadFieldsFromStorage($storage, array $fields) {
+ // Only try to load fields which have a persisted object.
+ $loadable = array();
+ foreach ($fields as $key => $field) {
+ $object = $field->getObject();
+ $phid = $object->getPHID();
+ if (!$phid) {
+ continue;
+ }
+
+ $loadable[$key] = $field;
+ }
+
+ if ($loadable) {
+ $data = $storage->loadStorageSourceData($loadable);
+ } else {
+ $data = array();
+ }
+
+ foreach ($fields as $key => $field) {
+ if (array_key_exists($key, $data)) {
+ $value = $data[$key];
+ $field->setValueFromStorage($value);
+ $field->didSetValueFromStorage();
+ } else if (isset($loadable[$key])) {
+ // NOTE: We set this only if the object exists. Otherwise, we allow
+ // the field to retain any default value it may have.
+ $field->setValueFromStorage(null);
+ $field->didSetValueFromStorage();
+ }
+ }
+ }
+
+}
diff --git a/src/infrastructure/customfield/storage/PhabricatorCustomFieldStorage.php b/src/infrastructure/customfield/storage/PhabricatorCustomFieldStorage.php
--- a/src/infrastructure/customfield/storage/PhabricatorCustomFieldStorage.php
+++ b/src/infrastructure/customfield/storage/PhabricatorCustomFieldStorage.php
@@ -23,4 +23,68 @@
) + parent::getConfiguration();
}
+
+ /**
+ * Get a key which uniquely identifies this storage source.
+ *
+ * When loading custom fields, fields using sources with the same source key
+ * are loaded in bulk.
+ *
+ * @return string Source identifier.
+ */
+ final public function getStorageSourceKey() {
+ return $this->getApplicationName().'/'.$this->getTableName();
+ }
+
+
+ /**
+ * Load stored data for custom fields.
+ *
+ * Given a map of fields, return a map with any stored data for those fields.
+ * The keys in the result should correspond to the keys in the input. The
+ * fields in the list may belong to different objects.
+ *
+ * @param map<string, PhabricatorCustomField> Map of fields.
+ * @return map<String, PhabricatorCustomField> Map of available field data.
+ */
+ final public function loadStorageSourceData(array $fields) {
+ $map = array();
+ $indexes = array();
+ $object_phids = array();
+
+ foreach ($fields as $key => $field) {
+ $index = $field->getFieldIndex();
+ $object_phid = $field->getObject()->getPHID();
+
+ $map[$index][$object_phid] = $key;
+ $indexes[$index] = $index;
+ $object_phids[$object_phid] = $object_phid;
+ }
+
+ if (!$indexes) {
+ return array();
+ }
+
+ $conn = $this->establishConnection('r');
+ $rows = queryfx_all(
+ $conn,
+ 'SELECT objectPHID, fieldIndex, fieldValue FROM %T
+ WHERE objectPHID IN (%Ls) AND fieldIndex IN (%Ls)',
+ $this->getTableName(),
+ $object_phids,
+ $indexes);
+
+ $result = array();
+ foreach ($rows as $row) {
+ $index = $row['fieldIndex'];
+ $object_phid = $row['objectPHID'];
+ $value = $row['fieldValue'];
+
+ $key = $map[$index][$object_phid];
+ $result[$key] = $value;
+ }
+
+ return $result;
+ }
+
}

File Metadata

Mime Type
text/plain
Expires
Mon, May 13, 11:52 PM (1 w, 2 d ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
6294012
Default Alt Text
D16351.id39328.diff (11 KB)

Event Timeline

Phabricator · Built by Phacility