From f2d94dc6679db542559ce1c0b857629c00739283 Mon Sep 17 00:00:00 2001 From: Jean-Yves Gaulier Date: Wed, 30 Dec 2015 18:49:48 +0100 Subject: [PATCH 1/2] #PHRAS-888 time 10h rewrite : no more n+1 sql for record get_caption() enhance : can fetch a record without instanciating 2 record_adapter (=no more copy) --- .../Phrasea/Controller/Api/V1Controller.php | 5 +- lib/classes/caption/Field/Value.php | 99 +++++++++++-------- lib/classes/caption/field.php | 34 +++++-- lib/classes/caption/record.php | 70 ++++++++++--- lib/classes/databox.php | 2 +- lib/classes/record/adapter.php | 22 +++++ 6 files changed, 165 insertions(+), 67 deletions(-) diff --git a/lib/Alchemy/Phrasea/Controller/Api/V1Controller.php b/lib/Alchemy/Phrasea/Controller/Api/V1Controller.php index 6c072c2fdb..7a09187ae2 100644 --- a/lib/Alchemy/Phrasea/Controller/Api/V1Controller.php +++ b/lib/Alchemy/Phrasea/Controller/Api/V1Controller.php @@ -1316,7 +1316,7 @@ class V1Controller extends Controller */ public function getRecordMetadataAction(Request $request, $databox_id, $record_id) { - $record = $this->findDataboxById($databox_id)->get_record($record_id); + $record = \record_adapter::getRecordLoaded($this->app, $databox_id, $record_id); $ret = ["record_metadatas" => $this->listRecordCaption($record->get_caption())]; return Result::create($request, $ret)->createResponse(); @@ -2417,8 +2417,7 @@ class V1Controller extends Controller public function ensureCanAccessToRecord(Request $request) { $user = $this->getApiAuthenticatedUser(); - $record = $this->findDataboxById($request->attributes->get('databox_id')) - ->get_record($request->attributes->get('record_id')); + $record = \record_adapter::getRecordLoaded($this->app, $request->attributes->get('databox_id'), $request->attributes->get('record_id')); if (!$this->getAclForUser($user)->has_access_to_record($record)) { return Result::createError($request, 401, 'You are not authorized')->createResponse(); } diff --git a/lib/classes/caption/Field/Value.php b/lib/classes/caption/Field/Value.php index 41f0dc13ac..345b44816a 100644 --- a/lib/classes/caption/Field/Value.php +++ b/lib/classes/caption/Field/Value.php @@ -15,6 +15,9 @@ use Alchemy\Phrasea\Vocabulary; class caption_Field_Value implements cache_cacheableInterface { + const RETRIEVE_VALUES = true; + const DONT_RETRIEVE_VALUES = false; + /** @var int */ protected $id; @@ -55,16 +58,19 @@ class caption_Field_Value implements cache_cacheableInterface * @param databox_field $databox_field * @param record_adapter $record * @param mixed $id + * @param bool $retrieveValues * @return \caption_Field_Value */ - public function __construct(Application $app, databox_field $databox_field, record_adapter $record, $id) + public function __construct(Application $app, databox_field $databox_field, record_adapter $record, $id, $retrieveValues = self::RETRIEVE_VALUES) { $this->id = (int) $id; $this->databox_field = $databox_field; $this->record = $record; $this->app = $app; - $this->retrieveValues(); + if($retrieveValues == self::RETRIEVE_VALUES) { + $this->retrieveValues(); + } } public function getQjs() @@ -72,6 +78,53 @@ class caption_Field_Value implements cache_cacheableInterface return $this->qjs; } + public function injectValues($value, $VocabularyType, $VocabularyId) + { + $this->value = StringHelper::crlfNormalize($value); + + try { + $this->VocabularyType = $VocabularyType ? Vocabulary\Controller::get($this->app, $VocabularyType) : null; + $this->VocabularyId = $VocabularyId; + } catch (\InvalidArgumentException $e) { + + } + + if ($this->VocabularyType) { + /** + * Vocabulary Control has been deactivated + */ + if ( ! $this->databox_field->getVocabularyControl()) { + $this->removeVocabulary(); + } + /** + * Vocabulary Control has changed + */ + elseif ($this->databox_field->getVocabularyControl()->getType() !== $this->VocabularyType->getType()) { + $this->removeVocabulary(); + } + /** + * Current Id is not available anymore + */ + elseif ( ! $this->VocabularyType->validate($this->VocabularyId)) { + $this->removeVocabulary(); + } + /** + * String equivalence has changed + */ + elseif ($this->VocabularyType->getValue($this->VocabularyId) !== $this->value) { + $this->set_value($this->VocabularyType->getValue($this->VocabularyId)); + } + } + + $datas = [ + 'value' => $this->value, + 'vocabularyId' => $this->VocabularyId, + 'vocabularyType' => $this->VocabularyType ? $this->VocabularyType->getType() : null, + ]; + + $this->set_data_to_cache($datas); + } + protected function retrieveValues() { try { @@ -95,47 +148,13 @@ class caption_Field_Value implements cache_cacheableInterface $row = $stmt->fetch(PDO::FETCH_ASSOC); $stmt->closeCursor(); - $this->value = $row ? StringHelper::crlfNormalize($row['value']) : null; - - try { - $this->VocabularyType = $row['VocabularyType'] ? Vocabulary\Controller::get($this->app, $row['VocabularyType']) : null; - $this->VocabularyId = $row['VocabularyId']; - } catch (\InvalidArgumentException $e) { - + if($row) { + $this->injectValues($row['value'], $row['VocabularyType'], $row['VocabularyId']); } - - if ($this->VocabularyType) { - /** - * Vocabulary Control has been deactivated - */ - if ( ! $this->databox_field->getVocabularyControl()) { - $this->removeVocabulary(); - } - /** - * Vocabulary Control has changed - */ elseif ($this->databox_field->getVocabularyControl()->getType() !== $this->VocabularyType->getType()) { - $this->removeVocabulary(); - } - /** - * Current Id is not available anymore - */ elseif ( ! $this->VocabularyType->validate($this->VocabularyId)) { - $this->removeVocabulary(); - } - /** - * String equivalence has changed - */ elseif ($this->VocabularyType->getValue($this->VocabularyId) !== $this->value) { - $this->set_value($this->VocabularyType->getValue($this->VocabularyId)); - } + else { + $this->injectValues(null, null, null); } - $datas = [ - 'value' => $this->value, - 'vocabularyId' => $this->VocabularyId, - 'vocabularyType' => $this->VocabularyType ? $this->VocabularyType->getType() : null, - ]; - - $this->set_data_to_cache($datas); - return $this; } diff --git a/lib/classes/caption/field.php b/lib/classes/caption/field.php index 8af6e5aad0..8578f9f93b 100644 --- a/lib/classes/caption/field.php +++ b/lib/classes/caption/field.php @@ -14,6 +14,9 @@ use Doctrine\DBAL\Driver\Statement; class caption_field implements cache_cacheableInterface { + const RETRIEVE_VALUES = true; + const DONT_RETRIEVE_VALUES = false; + /** * @var databox_field */ @@ -33,33 +36,44 @@ class caption_field implements cache_cacheableInterface protected static $localCache = []; /** - * @param Application $app - * @param databox_field $databox_field + * @param Application $app + * @param databox_field $databox_field * @param record_adapter $record + * @param bool $retrieveValues * * @return caption_field */ - public function __construct(Application $app, databox_field $databox_field, \record_adapter $record) + public function __construct(Application $app, databox_field $databox_field, \record_adapter $record, $retrieveValues = self::RETRIEVE_VALUES) { $this->app = $app; $this->record = $record; $this->databox_field = $databox_field; $this->values = []; - $rs = $this->get_metadatas_ids(); + if($retrieveValues == self::RETRIEVE_VALUES) { + $rs = $this->get_metadatas_ids(); - foreach ($rs as $row) { - $this->values[$row['id']] = new caption_Field_Value($this->app, $databox_field, $record, $row['id']); + foreach ($rs as $row) { + $this->values[$row['id']] = new caption_Field_Value($this->app, $databox_field, $record, $row['id']); - // Inconsistent, should not happen - if ( ! $databox_field->is_multi()) { - break; + // Inconsistent, should not happen + if (!$databox_field->is_multi()) { + break; + } } } return $this; } + /** + * @param caption_Field_Value[] $values + */ + public function injectValues($values) + { + $this->values = $values; + } + protected function get_metadatas_ids() { try { @@ -73,7 +87,7 @@ class caption_field implements cache_cacheableInterface $sql = 'SELECT id FROM metadatas WHERE record_id = :record_id AND meta_struct_id = :meta_struct_id'; $params = [ - ':record_id' => $this->record->get_record_id() + ':record_id' => $this->record->getRecordId() , ':meta_struct_id' => $this->databox_field->get_id() ]; diff --git a/lib/classes/caption/record.php b/lib/classes/caption/record.php index 76844a4797..054c99d9ff 100644 --- a/lib/classes/caption/record.php +++ b/lib/classes/caption/record.php @@ -47,7 +47,7 @@ class caption_record implements caption_interface, cache_cacheableInterface public function __construct(Application $app, \record_adapter $record, databox $databox) { $this->app = $app; - $this->sbas_id = $record->get_sbas_id(); + $this->sbas_id = $record->getDataboxId(); $this->record = $record; $this->databox = $databox; } @@ -78,12 +78,12 @@ class caption_record implements caption_interface, cache_cacheableInterface try { $fields = $this->get_data_from_cache(); } catch (\Exception $e) { - $sql = "SELECT m.id as meta_id, s.id as structure_id - FROM metadatas m, metadatas_structure s - WHERE m.record_id = :record_id AND s.id = m.meta_struct_id - ORDER BY s.sorter ASC"; + $sql = "SELECT m.id AS meta_id, s.id AS structure_id, value, VocabularyType, VocabularyId" + . " FROM metadatas m, metadatas_structure s" + . " WHERE m.record_id = :record_id AND s.id = m.meta_struct_id" + . " ORDER BY s.sorter ASC"; $stmt = $this->databox->get_connection()->prepare($sql); - $stmt->execute([':record_id' => $this->record->get_record_id()]); + $stmt->execute([':record_id' => $this->record->getRecordId()]); $fields = $stmt->fetchAll(PDO::FETCH_ASSOC); $stmt->closeCursor(); if ($fields) { @@ -95,11 +95,55 @@ class caption_record implements caption_interface, cache_cacheableInterface if ($fields) { $databox_descriptionStructure = $this->databox->get_meta_structure(); - foreach ($fields as $row) { - $databox_field = $databox_descriptionStructure->get_element($row['structure_id']); - $metadata = new caption_field($this->app, $databox_field, $this->record); - $rec_fields[$databox_field->get_id()] = $metadata; + // first group values by field + $caption_fields = []; + foreach ($fields as $row) { + $structure_id = $row['structure_id']; + if(!array_key_exists($structure_id, $caption_fields)) { + $caption_fields[$structure_id] = [ + 'db_field' => $databox_descriptionStructure->get_element($structure_id), + 'values' => [] + ]; + } + + if (count($caption_fields[$structure_id]['values']) > 0 && !$caption_fields[$structure_id]['db_field']->is_multi()) { + // Inconsistent, should not happen + continue; + } + + // build an EMPTY caption_Field_Value + $cfv = new caption_Field_Value( + $this->app, + $caption_fields[$structure_id]['db_field'], + $this->record, + $row['meta_id'], + caption_Field_Value::DONT_RETRIEVE_VALUES // ask caption_Field_Value "no n+1 sql" + ); + + // inject the value we already know + $cfv->injectValues($row['value'], $row['VocabularyType'], $row['VocabularyId']); + + // add the value to the field + $caption_fields[$structure_id]['values'][] = $cfv; + } + + // now build a "caption_field" with already known "caption_Field_Value"s + foreach($caption_fields as $structure_id => $caption_field) { + + // build an EMPTY caption_field + $cf = new caption_field( + $this->app, + $caption_field['db_field'], + $this->record, + caption_field::DONT_RETRIEVE_VALUES // ask caption_field "no n+1 sql" + ); + + // inject the value we already know + $cf->injectValues($caption_field['values']); + + // add the field to the fields + $rec_fields[$structure_id] = $cf; } } $this->fields = $rec_fields; @@ -248,7 +292,7 @@ class caption_record implements caption_interface, cache_cacheableInterface */ public function get_data_from_cache($option = null) { - return $this->record->get_databox()->get_data_from_cache($this->get_cache_key($option)); + return $this->record->getDatabox()->get_data_from_cache($this->get_cache_key($option)); } /** @@ -261,7 +305,7 @@ class caption_record implements caption_interface, cache_cacheableInterface */ public function set_data_to_cache($value, $option = null, $duration = 0) { - return $this->record->get_databox()->set_data_to_cache($value, $this->get_cache_key($option), $duration); + return $this->record->getDatabox()->set_data_to_cache($value, $this->get_cache_key($option), $duration); } /** @@ -274,6 +318,6 @@ class caption_record implements caption_interface, cache_cacheableInterface { $this->fields = null; - return $this->record->get_databox()->delete_data_from_cache($this->get_cache_key($option)); + return $this->record->getDatabox()->delete_data_from_cache($this->get_cache_key($option)); } } diff --git a/lib/classes/databox.php b/lib/classes/databox.php index 40d9fda99e..2ca3633dd1 100644 --- a/lib/classes/databox.php +++ b/lib/classes/databox.php @@ -250,7 +250,7 @@ class databox extends base implements ThumbnailedElement */ public function get_record($record_id, $number = null) { - return new record_adapter($this->app, $this->id, $record_id, $number); + return record_adapter::getRecordLoaded($this->app, $this->id, $record_id, $number); } public function get_label($code, $substitute = true) diff --git a/lib/classes/record/adapter.php b/lib/classes/record/adapter.php index 331af3c5bd..78e40e8c8c 100644 --- a/lib/classes/record/adapter.php +++ b/lib/classes/record/adapter.php @@ -107,6 +107,28 @@ class record_adapter implements RecordInterface, cache_cacheableInterface $this->mirror($record); } + /** + * returns a loaded record, same thing as calling new record_adapter(..., load=true) + * but does instantiate only one record, no need to mirror(). + * + * @param Application $app + * @param $sbas_id + * @param $record_id + * @param null $number + * + * @return null|record_adapter + */ + public static function getRecordLoaded(Application $app, $sbas_id, $record_id, $number = null) + { + if (null === ($record = $app->findDataboxById((int) $sbas_id)->getRecordRepository()->find($record_id))) { + throw new Exception_Record_AdapterNotFound('Record ' . $record_id . ' on database ' . $sbas_id . ' not found '); + } + $record->app = $app; + $record->number = $number; + + return $record; + } + /** * @param record_adapter $record */ From f4c834d68ccddf5a6441b320b0df7844ce19db9c Mon Sep 17 00:00:00 2001 From: Jean-Yves Gaulier Date: Wed, 6 Jan 2016 14:59:13 +0100 Subject: [PATCH 2/2] #PHRAS-888 time 5m rollback (double record instantiation was ok) --- .../Phrasea/Controller/Api/V1Controller.php | 5 +++-- lib/classes/databox.php | 2 +- lib/classes/record/adapter.php | 22 ------------------- 3 files changed, 4 insertions(+), 25 deletions(-) diff --git a/lib/Alchemy/Phrasea/Controller/Api/V1Controller.php b/lib/Alchemy/Phrasea/Controller/Api/V1Controller.php index 7a09187ae2..6c072c2fdb 100644 --- a/lib/Alchemy/Phrasea/Controller/Api/V1Controller.php +++ b/lib/Alchemy/Phrasea/Controller/Api/V1Controller.php @@ -1316,7 +1316,7 @@ class V1Controller extends Controller */ public function getRecordMetadataAction(Request $request, $databox_id, $record_id) { - $record = \record_adapter::getRecordLoaded($this->app, $databox_id, $record_id); + $record = $this->findDataboxById($databox_id)->get_record($record_id); $ret = ["record_metadatas" => $this->listRecordCaption($record->get_caption())]; return Result::create($request, $ret)->createResponse(); @@ -2417,7 +2417,8 @@ class V1Controller extends Controller public function ensureCanAccessToRecord(Request $request) { $user = $this->getApiAuthenticatedUser(); - $record = \record_adapter::getRecordLoaded($this->app, $request->attributes->get('databox_id'), $request->attributes->get('record_id')); + $record = $this->findDataboxById($request->attributes->get('databox_id')) + ->get_record($request->attributes->get('record_id')); if (!$this->getAclForUser($user)->has_access_to_record($record)) { return Result::createError($request, 401, 'You are not authorized')->createResponse(); } diff --git a/lib/classes/databox.php b/lib/classes/databox.php index 2ca3633dd1..40d9fda99e 100644 --- a/lib/classes/databox.php +++ b/lib/classes/databox.php @@ -250,7 +250,7 @@ class databox extends base implements ThumbnailedElement */ public function get_record($record_id, $number = null) { - return record_adapter::getRecordLoaded($this->app, $this->id, $record_id, $number); + return new record_adapter($this->app, $this->id, $record_id, $number); } public function get_label($code, $substitute = true) diff --git a/lib/classes/record/adapter.php b/lib/classes/record/adapter.php index 78e40e8c8c..331af3c5bd 100644 --- a/lib/classes/record/adapter.php +++ b/lib/classes/record/adapter.php @@ -107,28 +107,6 @@ class record_adapter implements RecordInterface, cache_cacheableInterface $this->mirror($record); } - /** - * returns a loaded record, same thing as calling new record_adapter(..., load=true) - * but does instantiate only one record, no need to mirror(). - * - * @param Application $app - * @param $sbas_id - * @param $record_id - * @param null $number - * - * @return null|record_adapter - */ - public static function getRecordLoaded(Application $app, $sbas_id, $record_id, $number = null) - { - if (null === ($record = $app->findDataboxById((int) $sbas_id)->getRecordRepository()->find($record_id))) { - throw new Exception_Record_AdapterNotFound('Record ' . $record_id . ' on database ' . $sbas_id . ' not found '); - } - $record->app = $app; - $record->number = $number; - - return $record; - } - /** * @param record_adapter $record */