From 8ce5f4a796850a2b3cad40095573fb0bb11384ca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20B=C3=B6hmer?= Date: Thu, 20 Jul 2023 23:20:46 +0200 Subject: [PATCH] Do not cache entities directly in NodesListBuilder but cache only the IDs instead Otherwise the doctrine proxies break, and we get issues with loading the preview_images in structural Elements. --- composer.lock | 42 ++++++----- .../AttachmentContainingDBElement.php | 3 +- src/Entity/Base/MasterAttachmentTrait.php | 2 + ...ttachmentContainingDBElementRepository.php | 72 +++++++++++++++++++ src/Repository/DBElementRepository.php | 34 +++++++++ src/Repository/NamedDBElementRepository.php | 4 +- .../StructuralDBElementRepository.php | 4 +- src/Services/Trees/NodesListBuilder.php | 33 ++++++++- .../StructuralDBElementRepositoryTest.php | 4 +- 9 files changed, 173 insertions(+), 25 deletions(-) create mode 100644 src/Repository/AttachmentContainingDBElementRepository.php diff --git a/composer.lock b/composer.lock index 084b2ced..15db6e18 100644 --- a/composer.lock +++ b/composer.lock @@ -15014,16 +15014,16 @@ }, { "name": "phpstan/phpstan", - "version": "1.10.25", + "version": "1.10.26", "source": { "type": "git", "url": "https://github.com/phpstan/phpstan.git", - "reference": "578f4e70d117f9a90699324c555922800ac38d8c" + "reference": "5d660cbb7e1b89253a47147ae44044f49832351f" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/phpstan/phpstan/zipball/578f4e70d117f9a90699324c555922800ac38d8c", - "reference": "578f4e70d117f9a90699324c555922800ac38d8c", + "url": "https://api.github.com/repos/phpstan/phpstan/zipball/5d660cbb7e1b89253a47147ae44044f49832351f", + "reference": "5d660cbb7e1b89253a47147ae44044f49832351f", "shasum": "" }, "require": { @@ -15072,7 +15072,7 @@ "type": "tidelift" } ], - "time": "2023-07-06T12:11:37+00:00" + "time": "2023-07-19T12:44:37+00:00" }, { "name": "phpstan/phpstan-doctrine", @@ -15396,12 +15396,12 @@ "source": { "type": "git", "url": "https://github.com/Roave/SecurityAdvisories.git", - "reference": "e5af9ce18347a240cb120454a8c2665c5b162aa1" + "reference": "bd50a5b0522c94e65c90eb8d8cc040a630a07b0c" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/Roave/SecurityAdvisories/zipball/e5af9ce18347a240cb120454a8c2665c5b162aa1", - "reference": "e5af9ce18347a240cb120454a8c2665c5b162aa1", + "url": "https://api.github.com/repos/Roave/SecurityAdvisories/zipball/bd50a5b0522c94e65c90eb8d8cc040a630a07b0c", + "reference": "bd50a5b0522c94e65c90eb8d8cc040a630a07b0c", "shasum": "" }, "conflict": { @@ -15435,6 +15435,7 @@ "aws/aws-sdk-php": ">=3,<3.2.1", "azuracast/azuracast": "<0.18.3", "backdrop/backdrop": "<1.24.2", + "backpack/crud": "<3.4.9", "badaso/core": "<2.7", "bagisto/bagisto": "<0.1.5", "barrelstrength/sprout-base-email": "<1.2.7", @@ -15620,6 +15621,7 @@ "joomla/filesystem": "<1.6.2|>=2,<2.0.1", "joomla/filter": "<1.4.4|>=2,<2.0.1", "joomla/input": ">=2,<2.0.2", + "joomla/joomla-cms": ">=3,<3.9.12", "joomla/session": "<1.3.1", "joyqi/hyper-down": "<=2.4.27", "jsdecena/laracom": "<2.0.9", @@ -15639,7 +15641,7 @@ "laminas/laminas-form": "<2.17.1|>=3,<3.0.2|>=3.1,<3.1.1", "laminas/laminas-http": "<2.14.2", "laravel/fortify": "<1.11.1", - "laravel/framework": "<6.20.42|>=7,<7.30.6|>=8,<8.75", + "laravel/framework": "<6.20.44|>=7,<7.30.6|>=8,<8.75", "laravel/socialite": ">=1,<1.0.99|>=2,<2.0.10", "latte/latte": "<2.10.8", "lavalite/cms": "= 9.0.0|<=9", @@ -15656,7 +15658,7 @@ "lms/routes": "<2.1.1", "localizationteam/l10nmgr": "<7.4|>=8,<8.7|>=9,<9.2", "luyadev/yii-helpers": "<1.2.1", - "magento/community-edition": "<=2.4", + "magento/community-edition": "= 2.4.0|<=2.4", "magento/magento1ce": "<1.9.4.3", "magento/magento1ee": ">=1,<1.14.4.3", "magento/product-community-edition": ">=2,<2.2.10|>=2.3,<2.3.2-p.2", @@ -15681,6 +15683,7 @@ "monolog/monolog": ">=1.8,<1.12", "moodle/moodle": "<4.2-rc.2|= 3.7|= 3.9|= 3.8|= 4.2.0|= 3.11", "movim/moxl": ">=0.8,<=0.10", + "mpdf/mpdf": "<=7.1.7", "mustache/mustache": ">=2,<2.14.1", "namshi/jose": "<2.2", "neoan3-apps/template": "<1.1.1", @@ -15789,6 +15792,7 @@ "scheb/two-factor-bundle": ">=0,<3.26|>=4,<4.11", "sensiolabs/connect": "<4.2.3", "serluck/phpwhois": "<=4.2.6", + "sfroemken/url_redirect": "<=1.2.1", "sheng/yiicms": "<=1.2", "shopware/core": "<=6.4.20", "shopware/platform": "<=6.4.20", @@ -15797,6 +15801,7 @@ "shopware/storefront": "<=6.4.8.1", "shopxo/shopxo": "<2.2.6", "showdoc/showdoc": "<2.10.4", + "silverstripe-australia/advancedreports": ">=1,<=2", "silverstripe/admin": "<1.12.7", "silverstripe/assets": ">=1,<1.11.1", "silverstripe/cms": "<4.11.3", @@ -15805,6 +15810,7 @@ "silverstripe/framework": "<4.12.5", "silverstripe/graphql": "<3.5.2|>=4-alpha.1,<4-alpha.2|>=4.1.1,<4.1.2|>=4.2.2,<4.2.3|= 4.0.0-alpha1", "silverstripe/hybridsessions": ">=1,<2.4.1|>=2.5,<2.5.1", + "silverstripe/recipe-cms": ">=4.5,<4.5.3", "silverstripe/registry": ">=2.1,<2.1.2|>=2.2,<2.2.1", "silverstripe/restfulserver": ">=1,<1.0.9|>=2,<2.0.4", "silverstripe/silverstripe-omnipay": "<2.5.2|>=3,<3.0.2|>=3.1,<3.1.4|>=3.2,<3.2.1", @@ -15820,6 +15826,7 @@ "simplesamlphp/simplesamlphp-module-openidprovider": "<0.9", "simplito/elliptic-php": "<1.0.6", "sitegeist/fluid-components": "<3.5", + "sjbr/sr-freecap": "<=2.5.2", "slim/psr7": "<1.4.1|>=1.5,<1.5.1|>=1.6,<1.6.1", "slim/slim": "<2.6", "smarty/smarty": "<3.1.48|>=4,<4.3.1", @@ -15828,6 +15835,7 @@ "socialiteproviders/steam": "<1.1", "spatie/browsershot": "<3.57.4", "spipu/html2pdf": "<5.2.4", + "spoon/library": "<1.4.1", "spoonity/tcpdf": "<6.2.22", "squizlabs/php_codesniffer": ">=1,<2.8.1|>=3,<3.0.1", "ssddanbrown/bookstack": "<22.2.3", @@ -16024,7 +16032,7 @@ "type": "tidelift" } ], - "time": "2023-07-17T11:04:34+00:00" + "time": "2023-07-20T19:04:25+00:00" }, { "name": "sebastian/diff", @@ -16095,16 +16103,16 @@ }, { "name": "spatie/array-to-xml", - "version": "3.1.6", + "version": "3.2.0", "source": { "type": "git", "url": "https://github.com/spatie/array-to-xml.git", - "reference": "e210b98957987c755372465be105d32113f339a4" + "reference": "f9ab39c808500c347d5a8b6b13310bd5221e39e7" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/spatie/array-to-xml/zipball/e210b98957987c755372465be105d32113f339a4", - "reference": "e210b98957987c755372465be105d32113f339a4", + "url": "https://api.github.com/repos/spatie/array-to-xml/zipball/f9ab39c808500c347d5a8b6b13310bd5221e39e7", + "reference": "f9ab39c808500c347d5a8b6b13310bd5221e39e7", "shasum": "" }, "require": { @@ -16142,7 +16150,7 @@ "xml" ], "support": { - "source": "https://github.com/spatie/array-to-xml/tree/3.1.6" + "source": "https://github.com/spatie/array-to-xml/tree/3.2.0" }, "funding": [ { @@ -16154,7 +16162,7 @@ "type": "github" } ], - "time": "2023-05-11T14:04:07+00:00" + "time": "2023-07-19T18:30:26+00:00" }, { "name": "symfony/browser-kit", diff --git a/src/Entity/Attachments/AttachmentContainingDBElement.php b/src/Entity/Attachments/AttachmentContainingDBElement.php index 34f5797e..f2dfd3ef 100644 --- a/src/Entity/Attachments/AttachmentContainingDBElement.php +++ b/src/Entity/Attachments/AttachmentContainingDBElement.php @@ -26,6 +26,7 @@ use App\Entity\Base\AbstractNamedDBElement; use App\Entity\Base\MasterAttachmentTrait; use App\Entity\Contracts\HasAttachmentsInterface; use App\Entity\Contracts\HasMasterAttachmentInterface; +use App\Repository\AttachmentContainingDBElementRepository; use Doctrine\Common\Collections\ArrayCollection; use Doctrine\Common\Collections\Collection; use Doctrine\ORM\Mapping as ORM; @@ -34,7 +35,7 @@ use Symfony\Component\Serializer\Annotation\Groups; /** * @template-covariant AT of Attachment */ -#[ORM\MappedSuperclass] +#[ORM\MappedSuperclass(repositoryClass: AttachmentContainingDBElementRepository::class)] abstract class AttachmentContainingDBElement extends AbstractNamedDBElement implements HasMasterAttachmentInterface, HasAttachmentsInterface { use MasterAttachmentTrait; diff --git a/src/Entity/Base/MasterAttachmentTrait.php b/src/Entity/Base/MasterAttachmentTrait.php index eaa34dc0..aec15633 100644 --- a/src/Entity/Base/MasterAttachmentTrait.php +++ b/src/Entity/Base/MasterAttachmentTrait.php @@ -35,6 +35,8 @@ trait MasterAttachmentTrait * @var Attachment|null * Mapping is done in the subclasses (e.g. Part), like with the attachments. * If this is done here (which is possible in theory), the attachment is not lazy loaded anymore, which causes unnecessary overhead. + * + * !!! If you change this name, you have to change it in the fetchHint in the AttachmentContainingDBElementRepository (getElementsAndPreviewAttachmentByIDs()) too !!! */ #[Assert\Expression('value == null or value.isPicture()', message: 'part.master_attachment.must_be_picture')] protected ?Attachment $master_picture_attachment = null; diff --git a/src/Repository/AttachmentContainingDBElementRepository.php b/src/Repository/AttachmentContainingDBElementRepository.php new file mode 100644 index 00000000..f346b257 --- /dev/null +++ b/src/Repository/AttachmentContainingDBElementRepository.php @@ -0,0 +1,72 @@ +. + */ + +declare(strict_types=1); + + +namespace App\Repository; + +use App\Entity\Attachments\AttachmentContainingDBElement; +use Doctrine\ORM\Mapping\ClassMetadataInfo; + +/** + * @template TEntityClass of AttachmentContainingDBElement + * @extends NamedDBElementRepository + */ +class AttachmentContainingDBElementRepository extends NamedDBElementRepository +{ + /** + * @var array This array is used to cache the results of getElementsAndPreviewAttachmentByIDs function. + */ + private array $elementsAndPreviewAttachmentCache = []; + + /** + * Similar to the findByIDInMatchingOrder function, but it also hints to doctrine that the master picture attachment should be fetched eagerly. + * @param array $ids + * @return array + */ + public function getElementsAndPreviewAttachmentByIDs(array $ids): array + { + //Convert the ids to a string + $cache_key = implode(',', $ids); + + //Check if the result is already cached + if (isset($this->elementsAndPreviewAttachmentCache[$cache_key])) { + return $this->elementsAndPreviewAttachmentCache[$cache_key]; + } + + $qb = $this->createQueryBuilder('element'); + $q = $qb->select('element') + ->where('element.id IN (?1)') + ->setParameter(1, $ids) + ->getQuery(); + + $q->setFetchMode($this->getEntityName(), 'master_picture_attachment', ClassMetadataInfo::FETCH_EAGER); + + $result = $q->getResult(); + $result = array_combine($ids, $result); + $result = array_map(fn ($id) => $result[$id], $ids); + + //Cache the result + $this->elementsAndPreviewAttachmentCache[$cache_key] = $result; + + return $result; + } +} \ No newline at end of file diff --git a/src/Repository/DBElementRepository.php b/src/Repository/DBElementRepository.php index fd410e3d..f187714b 100644 --- a/src/Repository/DBElementRepository.php +++ b/src/Repository/DBElementRepository.php @@ -51,6 +51,8 @@ use ReflectionClass; */ class DBElementRepository extends EntityRepository { + private array $find_elements_by_id_cache = []; + /** * Changes the ID of the given element to a new value. * You should only use it to undelete former existing elements, everything else is most likely a bad idea! @@ -91,6 +93,38 @@ class DBElementRepository extends EntityRepository return $q->getResult(); } + /** + * Returns the elements with the given IDs in the same order, as they were given in the input array. + * + * @param array $ids + * @return array + */ + public function findByIDInMatchingOrder(array $ids): array + { + $cache_key = implode(',', $ids); + + //Check if the result is already cached + if (isset($this->find_elements_by_id_cache[$cache_key])) { + return $this->find_elements_by_id_cache[$cache_key]; + } + + //Otherwise do the query + $qb = $this->createQueryBuilder('element'); + $q = $qb->select('element') + ->where('element.id IN (?1)') + ->setParameter(1, $ids) + ->getQuery(); + + $result = $q->getResult(); + $result = array_combine($ids, $result); + $result = array_map(fn ($id) => $result[$id], $ids); + + //Cache the result + $this->find_elements_by_id_cache[$cache_key] = $result; + + return $result; + } + protected function setField(AbstractDBElement $element, string $field, int $new_value): void { $reflection = new ReflectionClass($element::class); diff --git a/src/Repository/NamedDBElementRepository.php b/src/Repository/NamedDBElementRepository.php index 772cc164..ae8d4d8f 100644 --- a/src/Repository/NamedDBElementRepository.php +++ b/src/Repository/NamedDBElementRepository.php @@ -65,11 +65,11 @@ class NamedDBElementRepository extends DBElementRepository } /** - * Returns the list of all nodes to use in a select box. + * Returns a flattened list of all nodes. * @return AbstractNamedDBElement[] * @phpstan-return array */ - public function toNodesList(): array + public function getFlatList(): array { //All nodes are sorted by name return $this->findBy([], ['name' => 'ASC']); diff --git a/src/Repository/StructuralDBElementRepository.php b/src/Repository/StructuralDBElementRepository.php index 24087cfa..3d1c6f56 100644 --- a/src/Repository/StructuralDBElementRepository.php +++ b/src/Repository/StructuralDBElementRepository.php @@ -32,7 +32,7 @@ use RecursiveIteratorIterator; * @template TEntityClass of AbstractStructuralDBElement * @extends NamedDBElementRepository */ -class StructuralDBElementRepository extends NamedDBElementRepository +class StructuralDBElementRepository extends AttachmentContainingDBElementRepository { /** * @var array An array containing all new entities created by getNewEntityByPath. @@ -85,7 +85,7 @@ class StructuralDBElementRepository extends NamedDBElementRepository * @return AbstractStructuralDBElement[] a flattened list containing the tree elements * @phpstan-return array */ - public function toNodesList(?AbstractStructuralDBElement $parent = null): array + public function getFlatList(?AbstractStructuralDBElement $parent = null): array { $result = []; diff --git a/src/Services/Trees/NodesListBuilder.php b/src/Services/Trees/NodesListBuilder.php index c3e06455..ebb2f93d 100644 --- a/src/Services/Trees/NodesListBuilder.php +++ b/src/Services/Trees/NodesListBuilder.php @@ -22,7 +22,12 @@ declare(strict_types=1); namespace App\Services\Trees; +use App\Entity\Attachments\AttachmentContainingDBElement; +use App\Entity\Base\AbstractDBElement; +use App\Entity\Base\AbstractNamedDBElement; use App\Entity\Base\AbstractStructuralDBElement; +use App\Repository\AttachmentContainingDBElementRepository; +use App\Repository\DBElementRepository; use App\Repository\StructuralDBElementRepository; use App\Services\UserSystem\UserCacheKeyGenerator; use Doctrine\ORM\EntityManagerInterface; @@ -49,6 +54,31 @@ class NodesListBuilder * @return AbstractStructuralDBElement[] a flattened list containing the tree elements */ public function typeToNodesList(string $class_name, ?AbstractStructuralDBElement $parent = null): array + { + /** + * We can not cache the entities directly, because loading them from cache will break the doctrine proxies. + */ + //Retrieve the IDs of the elements + $ids = $this->getFlattenedIDs($class_name, $parent); + + //Retrieve the elements from the IDs, the order is the same as in the $ids array + /** @var DBElementRepository $repo */ + $repo = $this->em->getRepository($class_name); + + if ($repo instanceof AttachmentContainingDBElementRepository) { + return $repo->getElementsAndPreviewAttachmentByIDs($ids); + } + + return $repo->getElementsFromIDArray($ids); + } + + /** + * This functions returns the (cached) list of the IDs of the elements for the flattened tree. + * @param string $class_name + * @param AbstractStructuralDBElement|null $parent + * @return array + */ + private function getFlattenedIDs(string $class_name, ?AbstractStructuralDBElement $parent = null): array { $parent_id = $parent instanceof AbstractStructuralDBElement ? $parent->getID() : '0'; // Backslashes are not allowed in cache keys @@ -58,10 +88,11 @@ class NodesListBuilder return $this->cache->get($key, function (ItemInterface $item) use ($class_name, $parent, $secure_class_name) { // Invalidate when groups, an element with the class or the user changes $item->tag(['groups', 'tree_list', $this->keyGenerator->generateKey(), $secure_class_name]); + /** @var StructuralDBElementRepository $repo */ $repo = $this->em->getRepository($class_name); - return $repo->toNodesList($parent); + return array_map(fn(AbstractDBElement $element) => $element->getID(), $repo->getFlatList($parent)); }); } diff --git a/tests/Repository/StructuralDBElementRepositoryTest.php b/tests/Repository/StructuralDBElementRepositoryTest.php index b5e60605..47c0cb45 100644 --- a/tests/Repository/StructuralDBElementRepositoryTest.php +++ b/tests/Repository/StructuralDBElementRepositoryTest.php @@ -92,7 +92,7 @@ class StructuralDBElementRepositoryTest extends WebTestCase public function testToNodesListRoot(): void { //List all root nodes and their children - $nodes = $this->repo->toNodesList(); + $nodes = $this->repo->getFlatList(); $this->assertCount(7, $nodes); $this->assertContainsOnlyInstancesOf(AttachmentType::class, $nodes); @@ -109,7 +109,7 @@ class StructuralDBElementRepositoryTest extends WebTestCase { //List all nodes that are children to Node 1 $node1 = $this->repo->find(1); - $nodes = $this->repo->toNodesList($node1); + $nodes = $this->repo->getFlatList($node1); $this->assertCount(3, $nodes); $this->assertContainsOnlyInstancesOf(AttachmentType::class, $nodes);