An optimistic locking mechanism for campaigns (#13659)

* An optimistic locking mechanism for campaigns

* Rector fixes

* Undefined event.deleted fixed

---------

Co-authored-by: John Linhart <admin@escope.cz>

app/bundles/CampaignBundle/Entity/Campaign.php

@@ -8,15 +8,19 @@
 use Mautic\ApiBundle\Serializer\Driver\ApiMetadataDriver;
 use Mautic\CoreBundle\Doctrine\Mapping\ClassMetadataBuilder;
 use Mautic\CoreBundle\Entity\FormEntity;
+use Mautic\CoreBundle\Entity\OptimisticLockInterface;
+use Mautic\CoreBundle\Entity\OptimisticLockTrait;
 use Mautic\CoreBundle\Entity\PublishStatusIconAttributesInterface;
 use Mautic\FormBundle\Entity\Form;
 use Mautic\LeadBundle\Entity\Lead as Contact;
 use Mautic\LeadBundle\Entity\LeadList;
 use Symfony\Component\Validator\Constraints as Assert;
 use Symfony\Component\Validator\Mapping\ClassMetadata;
 
-class Campaign extends FormEntity implements PublishStatusIconAttributesInterface
+class Campaign extends FormEntity implements PublishStatusIconAttributesInterface, OptimisticLockInterface
 {
+    use OptimisticLockTrait;
+
     public const TABLE_NAME = 'campaigns';
     /**
      * @var int
@@ -143,6 +147,8 @@ public static function loadMetadata(ORM\ClassMetadata $metadata): void
 
         $builder->addNamedField('allowRestart', 'boolean', 'allow_restart');
         $builder->addNullableField('deleted', 'datetime');
+
+        self::addVersionField($builder);
     }
 
     public static function loadValidatorMetadata(ClassMetadata $metadata): void

app/bundles/CampaignBundle/Form/Type/CampaignType.php

@@ -116,6 +116,10 @@ public function buildForm(FormBuilderInterface $builder, array $options): void
                 ],
             ],
         ]);
+
+        $builder->add('version', HiddenType::class, [
+            'mapped' => false,
+        ]);
     }
 
     public function configureOptions(OptionsResolver $resolver): void

app/bundles/CampaignBundle/Resources/views/Campaign/_builder.html.twig

@@ -33,7 +33,7 @@
             {% endfor %}
 
             {% for event in campaignEvents %}
-                {% if event.deleted is empty %}
+                {% if event.deleted|default(null) is empty %}
                   {% set settings = eventSettings[event.eventType][event.type]|default(null) %}
                   {{- include(settings['template']|default('@MauticCampaign/Event/_generic.html.twig'), {
                         'event': event,

app/bundles/CampaignBundle/Tests/Functional/Controller/CampaignOptimisticLockTest.php

@@ -0,0 +1,127 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Mautic\CampaignBundle\Tests\Functional\Controller;
+
+use Mautic\CampaignBundle\Entity\Campaign;
+use Mautic\CampaignBundle\Entity\Event;
+use Mautic\CoreBundle\Test\MauticMysqlTestCase;
+use Mautic\LeadBundle\Entity\Lead;
+use Mautic\LeadBundle\Entity\LeadList;
+use PHPUnit\Framework\Assert;
+use Symfony\Component\DomCrawler\Crawler;
+
+class CampaignOptimisticLockTest extends MauticMysqlTestCase
+{
+    /**
+     * @var string
+     */
+    private const OPTIMISTIC_LOCK_ERROR = 'The record you are updating has been changed by someone else in the meantime. Please refresh the browser window and re-submit your changes.';
+
+    public function testOptimisticLock(): void
+    {
+        $campaign = $this->setupCampaign();
+        $version  = $campaign->getVersion();
+
+        // version should be incremented as campaign's "modified by user" is updated
+        $this->refreshAndSubmitForm($campaign, ++$version);
+
+        // version should not be incremented as there are no changes
+        $this->refreshAndSubmitForm($campaign, $version);
+
+        // version should be incremented as there are changes
+        $this->refreshAndSubmitForm($campaign, ++$version, [
+            'campaign[allowRestart]' => '1',
+            'campaign[isPublished]'  => '1',
+        ]);
+
+        // version should not be incremented as there are no changes
+        $this->refreshAndSubmitForm($campaign, $version);
+
+        // refresh the page
+        $pageCrawler = $this->refreshPage($campaign);
+
+        // we should not get an optimistic lock error as the page was refreshed, version should be incremented
+        $crawler = $this->submitForm($pageCrawler, $campaign, ++$version, [
+            'campaign[allowRestart]' => '0',
+        ]);
+        Assert::assertStringNotContainsString(self::OPTIMISTIC_LOCK_ERROR, $crawler->text());
+
+        // we should get an optimistic lock error as the page wasn't refreshed
+        $crawler = $this->submitForm($pageCrawler, $campaign, $version, [
+            'campaign[isPublished]' => '1',
+        ]);
+        Assert::assertStringContainsString(self::OPTIMISTIC_LOCK_ERROR, $crawler->text());
+
+        // we should get an optimistic lock error even if there is no change
+        $crawler = $this->submitForm($pageCrawler, $campaign, $version);
+        Assert::assertStringContainsString(self::OPTIMISTIC_LOCK_ERROR, $crawler->text());
+    }
+
+    /**
+     * @param array<string,string> $formValues
+     */
+    private function refreshAndSubmitForm(Campaign $campaign, int $expectedVersion, array $formValues = []): void
+    {
+        $crawler = $this->refreshPage($campaign);
+        $this->submitForm($crawler, $campaign, $expectedVersion, $formValues);
+    }
+
+    private function refreshPage(Campaign $campaign): Crawler
+    {
+        $crawler = $this->client->request('GET', sprintf('/s/campaigns/edit/%s', $campaign->getId()));
+        Assert::assertTrue($this->client->getResponse()->isOk());
+        Assert::assertStringContainsString('Edit Campaign', $crawler->text());
+
+        return $crawler;
+    }
+
+    /**
+     * @param array<string,string> $formValues
+     */
+    private function submitForm(Crawler $crawler, Campaign $campaign, int $expectedVersion, array $formValues = []): Crawler
+    {
+        $form = $crawler->selectButton('Save')->form();
+        $form->setValues($formValues);
+        $newCrawler = $this->client->submit($form);
+        Assert::assertTrue($this->client->getResponse()->isOk());
+
+        $this->em->clear();
+        $campaign = $this->em->find(Campaign::class, $campaign->getId());
+        Assert::assertSame($expectedVersion, $campaign->getVersion());
+
+        return $newCrawler;
+    }
+
+    private function setupCampaign(): Campaign
+    {
+        $leadList = new LeadList();
+        $leadList->setName('Test list');
+        $leadList->setPublicName('Test list');
+        $leadList->setAlias('test-list');
+        $this->em->persist($leadList);
+
+        $campaign = new Campaign();
+        $campaign->setName('Test campaign');
+        $campaign->addList($leadList);
+        $this->em->persist($campaign);
+
+        $lead = new Lead();
+        $lead->setFirstname('Test Lead');
+        $this->em->persist($lead);
+
+        $campaignEvent = new Event();
+        $campaignEvent->setCampaign($campaign);
+        $campaignEvent->setName('Send Email 1');
+        $campaignEvent->setType('email.send');
+        $campaignEvent->setEventType('action');
+        $campaignEvent->setProperties([]);
+        $this->em->persist($campaignEvent);
+
+        $this->em->flush();
+        $this->em->clear();
+
+        return $campaign;
+    }
+}

app/bundles/CoreBundle/Config/services.php

@@ -3,6 +3,7 @@
 declare(strict_types=1);
 
 use Mautic\CoreBundle\DependencyInjection\MauticCoreExtension;
+use Mautic\CoreBundle\EventListener\OptimisticLockSubscriber;
 use Symfony\Component\DependencyInjection\Loader\Configurator\ContainerConfigurator;
 
 use function Symfony\Component\DependencyInjection\Loader\Configurator\service;
@@ -66,4 +67,6 @@
     $services->get(Mautic\CoreBundle\EventListener\CacheInvalidateSubscriber::class)
         ->arg('$ormConfiguration', service('doctrine.orm.default_configuration'))
         ->tag('doctrine.event_subscriber');
+    $services->get(OptimisticLockSubscriber::class)
+        ->tag('doctrine.event_subscriber');
 };

app/bundles/CoreBundle/Controller/AbstractStandardFormController.php

@@ -3,6 +3,7 @@
 namespace Mautic\CoreBundle\Controller;
 
 use Doctrine\Persistence\ManagerRegistry;
+use Mautic\CoreBundle\Entity\OptimisticLockInterface;
 use Mautic\CoreBundle\Factory\MauticFactory;
 use Mautic\CoreBundle\Factory\ModelFactory;
 use Mautic\CoreBundle\Form\Type\DateRangeType;
@@ -16,6 +17,7 @@
 use Mautic\FormBundle\Helper\FormFieldHelper;
 use Symfony\Component\EventDispatcher\EventDispatcherInterface;
 use Symfony\Component\Form\Form;
+use Symfony\Component\Form\FormError;
 use Symfony\Component\Form\FormFactoryInterface;
 use Symfony\Component\Form\FormInterface;
 use Symfony\Component\Form\FormView;
@@ -382,13 +384,14 @@ protected function editStandard(Request $request, $objectId, $ignorePost = false
 
         $isPost = !$ignorePost && 'POST' == $request->getMethod();
         $this->beforeFormProcessed($entity, $form, 'edit', $isPost, $objectId, $isClone);
+        $this->setOptimisticLockVersion($entity, $form);
 
         // /Check for a submitted form and process it
         if ($isPost) {
             $valid = false;
             if (!$cancelled = $this->isFormCancelled($form)) {
                 if ($valid = $this->isFormValid($form)) {
-                    if ($valid = $this->beforeEntitySave($entity, $form, 'edit', $objectId, $isClone)) {
+                    if ($valid = $this->checkOptimisticLockVersion($entity, $form, $isClone) && $this->beforeEntitySave($entity, $form, 'edit', $objectId, $isClone)) {
                         $model->saveEntity($entity, $this->getFormButton($form, ['buttons', 'save'])->isClicked());
 
                         $this->afterEntitySave($entity, $form, 'edit', $valid);
@@ -449,6 +452,7 @@ protected function editStandard(Request $request, $objectId, $ignorePost = false
                 $action = $this->generateUrl($this->getActionRoute(), ['objectAction' => 'edit', 'objectId' => $entity->getId()]);
                 $form   = $model->createForm($entity, $this->formFactory, $action);
                 $this->beforeFormProcessed($entity, $form, 'edit', false, $isClone);
+                $this->setOptimisticLockVersion($entity, $form);
             }
         } elseif (!$isClone) {
             $model->lockEntity($entity);
@@ -1132,4 +1136,52 @@ protected function getDataForExport(AbstractCommonModel $model, array $args, cal
     {
         return parent::getDataForExport($model, $args, $resultsCallback, $start);
     }
+
+    /**
+     * If the $entity supports optimistic locking, entity's current version is set to the $form for a later comparison.
+     *
+     * @param mixed $entity
+     */
+    protected function setOptimisticLockVersion($entity, FormInterface $form): void
+    {
+        if (!$entity instanceof OptimisticLockInterface) {
+            return;
+        }
+
+        $form->get($entity->getVersionField())
+            ->setData($entity->getVersion());
+    }
+
+    /**
+     * If the $entity supports optimistic locking, entity's current version is compared to the value in the $form.
+     *
+     * @param mixed $entity
+     */
+    protected function checkOptimisticLockVersion($entity, FormInterface $form, bool $isClone): bool
+    {
+        if ($isClone || !$entity instanceof OptimisticLockInterface) {
+            return true;
+        }
+
+        $version = (int) $form->get($entity->getVersionField())
+            ->getData();
+
+        if (!$version) {
+            throw new \LogicException(sprintf('A version is required for entities that implement "%s"', OptimisticLockInterface::class));
+        }
+
+        if ($version !== $entity->getVersion()) {
+            $form->addError(
+                new FormError(
+                    $this->translator->trans('mautic.core.optimistic_lock.changed_by_someone_else_error')
+                )
+            );
+
+            return false;
+        }
+
+        $entity->markForVersionIncrement();
+
+        return true;
+    }
 }

app/bundles/CoreBundle/Entity/OptimisticLockInterface.php

@@ -0,0 +1,36 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Mautic\CoreBundle\Entity;
+
+/**
+ * Optimistic locking is applied for entities that implements this interface.
+ */
+interface OptimisticLockInterface
+{
+    /**
+     * Returns the current version of the entity.
+     */
+    public function getVersion(): int;
+
+    /**
+     * Sets a new version of the entity and resets the mark for incrementing the version.
+     */
+    public function setVersion(int $version): void;
+
+    /**
+     * Returns true if the entity is marked for incrementing the version in a subsequent flush call.
+     */
+    public function isMarkedForVersionIncrement(): bool;
+
+    /**
+     * Mark the entity for incrementing the version in a subsequent flush call.
+     */
+    public function markForVersionIncrement(): void;
+
+    /**
+     * Returns the name of the version field.
+     */
+    public function getVersionField(): string;
+}

app/bundles/CoreBundle/Entity/OptimisticLockTrait.php

@@ -0,0 +1,53 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Mautic\CoreBundle\Entity;
+
+use Doctrine\DBAL\Types\Types;
+use Mautic\CoreBundle\Doctrine\Mapping\ClassMetadataBuilder;
+
+/**
+ * This trait provides default implementation of OptimisticLockInterface.
+ */
+trait OptimisticLockTrait
+{
+    private int $version = 1;
+    private ?int $currentVersion;
+    private bool $incrementVersion = false;
+
+    public function getVersion(): int
+    {
+        return $this->currentVersion ?? $this->version;
+    }
+
+    public function setVersion(int $version): void
+    {
+        $this->currentVersion   = $version;
+        $this->incrementVersion = false;
+    }
+
+    public function isMarkedForVersionIncrement(): bool
+    {
+        return $this->incrementVersion;
+    }
+
+    public function markForVersionIncrement(): void
+    {
+        $this->incrementVersion = true;
+    }
+
+    public function getVersionField(): string
+    {
+        return 'version';
+    }
+
+    private static function addVersionField(ClassMetadataBuilder $builder): void
+    {
+        $builder->createField('version', Types::INTEGER)
+            ->columnName('version')
+            ->option('default', 1)
+            ->option('unsigned', true)
+            ->build();
+    }
+}