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.
2025-06-20 17:15:51 +02:00 · 2023-07-20 23:20:46 +02:00 · 2023-07-20 23:20:46 +02:00 · 8ce5f4a796
commit 8ce5f4a796
parent 2e8cb35acc
9 changed files with 173 additions and 25 deletions
--- 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",
--- 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;
--- 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;
--- a/src/Repository/AttachmentContainingDBElementRepository.php
+++ b/src/Repository/AttachmentContainingDBElementRepository.php
@ -0,0 +1,72 @@
+<?php
+/*
+ * This file is part of Part-DB (https://github.com/Part-DB/Part-DB-symfony).
+ *
+ *  Copyright (C) 2019 - 2023 Jan Böhmer (https://github.com/jbtronics)
+ *
+ *  This program is free software: you can redistribute it and/or modify
+ *  it under the terms of the GNU Affero General Public License as published
+ *  by the Free Software Foundation, either version 3 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU Affero General Public License for more details.
+ *
+ *  You should have received a copy of the GNU Affero General Public License
+ *  along with this program.  If not, see <https://www.gnu.org/licenses/>.
+ */
+
+declare(strict_types=1);
+
+
+namespace App\Repository;
+
+use App\Entity\Attachments\AttachmentContainingDBElement;
+use Doctrine\ORM\Mapping\ClassMetadataInfo;
+
+/**
+ * @template TEntityClass of AttachmentContainingDBElement
+ * @extends NamedDBElementRepository<TEntityClass>
+ */
+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;
+    }
+}
--- 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);
--- 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<int, AbstractNamedDBElement>
     */
-    public function toNodesList(): array
+    public function getFlatList(): array
    {
        //All nodes are sorted by name
        return $this->findBy([], ['name' => 'ASC']);
--- a/src/Repository/StructuralDBElementRepository.php
+++ b/src/Repository/StructuralDBElementRepository.php
@ -32,7 +32,7 @@ use RecursiveIteratorIterator;
 * @template TEntityClass of AbstractStructuralDBElement
 * @extends NamedDBElementRepository<TEntityClass>
 */
-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<int, TEntityClass>
     */
-    public function toNodesList(?AbstractStructuralDBElement $parent = null): array
+    public function getFlatList(?AbstractStructuralDBElement $parent = null): array
    {
        $result = [];

--- 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));
        });
    }

--- 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);