[FrameworkBundle] fix ValidatorCacheWarmer: use serializing ArrayAdapter #23558

dmaicher · 2017-07-17T20:35:02Z

Q	A
Branch?	3.2
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#23544
License	MIT
Doc PR	-

The ValidatorCacheWarmer was using an ArrayAdapter with $storeSerialized=false. This is a problem as inside the LazyLoadingMetadataFactory the metaData objects are mutated (parent class constraints are merged into it) after they have been written into the cache.

So this means that currently when warming up the validator cache actually the merged metaData version is finally taken from the ArrayAdapter and written into the PhpFilesAdapter.

Which then caused some duplicate constraints as the parent constraints are merged again after fetching from the cache inside LazyLoadingMetadataFactory.

This fix makes sure we serialize objects into the ArrayAdapter.

Writing a test case for this does not seem easy to me. Any ideas?

EDIT: Maybe its even safer to just clone the object when writing it into the cache?

diff --git a/src/Symfony/Component/Validator/Mapping/Factory/LazyLoadingMetadataFactory.php b/src/Symfony/Component/Validator/Mapping/Factory/LazyLoadingMetadataFactory.php
index 79ad1f2..88eaf33 100644
--- a/src/Symfony/Component/Validator/Mapping/Factory/LazyLoadingMetadataFactory.php
+++ b/src/Symfony/Component/Validator/Mapping/Factory/LazyLoadingMetadataFactory.php
@@ -117,7 +117,7 @@ class LazyLoadingMetadataFactory implements MetadataFactoryInterface
         }
 
         if (null !== $this->cache) {
-            $this->cache->write($metadata);
+            $this->cache->write(clone $metadata);
         }

Opinions?

nicolas-grekas · 2017-07-18T08:21:31Z

Good catch!
I think we should do the same in AnnotationsCacheWarmer and in SerializerCacheWarmer.
There is also one thing to fix: the array_filter should happen before the array_map (misses are stored as null, which can't "unserialized" and shouldn't be cached.)
Dunno about patching LazyLoadingMetadataFactory, but anyway, I'm sure this one is needed :)

nicolas-grekas · 2017-07-18T08:22:04Z

src/Symfony/Bundle/FrameworkBundle/CacheWarmer/ValidatorCacheWarmer.php

@@ -62,7 +62,7 @@ public function warmUp($cacheDir)
        }

        $adapter = new PhpArrayAdapter($this->phpArrayFile, $this->fallbackPool);
-        $arrayPool = new ArrayAdapter(0, false);
+        $arrayPool = new ArrayAdapter(0);


0 is the default so it can be removed also

dmaicher · 2017-07-18T17:53:02Z

src/Symfony/Bundle/FrameworkBundle/Tests/CacheWarmer/ValidatorCacheWarmerTest.php

        $this->assertArrayHasKey('Symfony.Bundle.FrameworkBundle.Tests.Fixtures.Validation.Category', $values);
-        $this->assertArrayHasKey('Symfony.Bundle.FrameworkBundle.Tests.Fixtures.Validation.SubCategory', $values);


@nicolas-grekas due to moving the array_filter I needed to fix this test. It seems before we stored empty data in the cache?

Before the change the array contained this:

array:2 [ "Symfony.Bundle.FrameworkBundle.Tests.Fixtures.Validation.Category" => "O:49:"Symfony\Component\Validator\Mapping\ClassMetadata":10:{s:11:"constraints";a:0:{}s:18:"constraintsByGroup";a:0:{}s:17:"traversalStrategy";i:1;s:7:"getters";a:0:{}s:13:"groupSequence";a:0:{}s:21:"groupSequenceProvider";b:0;s:7:"members";a:2:{s:2:"id";a:1:{i:0;O:52:"Symfony\Component\Validator\Mapping\PropertyMetadata":7:{s:11:"constraints";a:1:{i:0;O:44:"Symfony\Component\Validator\Constraints\Type":4:{s:7:"message";s:40:"This value should be of type {{ type }}.";s:4:"type";s:3:"int";s:7:"payload";N;s:6:"groups";a:2:{i:0;s:7:"Default";i:1;s:8:"Category";}}}s:18:"constraintsByGroup";a:2:{s:7:"Default";a:1:{i:0;r:12;}s:8:"Category";a:1:{i:0;r:12;}}s:17:"cascadingStrategy";i:1;s:17:"traversalStrategy";i:2;s:5:"class";s:65:"Symfony\Bundle\FrameworkBundle\Tests\Fixtures\Validation\Category";s:4:"name";s:2:"id";s:8:"property";s:2:"id";}}s:4:"name";a:1:{i:0;O:52:"Symfony\Component\Validator\Mapping\PropertyMetadata":7:{s:11:"constraints";a:1:{i:0;O:44:"Symfony\Component\Validator\Constraints\Type":4:{s:7:"message";s:40:"This value should be of type {{ type }}.";s:4:"type";s:6:"string";s:7:"payload";N;s:6:"groups";a:2:{i:0;s:7:"Default";i:1;s:8:"Category";}}}s:18:"constraintsByGroup";a:2:{s:7:"Default";a:1:{i:0;r:32;}s:8:"Category";a:1:{i:0;r:32;}}s:17:"cascadingStrategy";i:1;s:17:"traversalStrategy";i:2;s:5:"class";s:65:"Symfony\Bundle\FrameworkBundle\Tests\Fixtures\Validation\Category";s:4:"name";s:4:"name";s:8:"property";s:4:"name";}}}s:4:"name";s:65:"Symfony\Bundle\FrameworkBundle\Tests\Fixtures\Validation\Category";s:10:"properties";a:2:{s:2:"id";r:10;s:4:"name";r:30;}s:12:"defaultGroup";s:8:"Category";}" "Symfony.Bundle.FrameworkBundle.Tests.Fixtures.Validation.SubCategory" => "b:0;" ]

the behavior should not change at all so this test must not be changed

The change you requested makes this test fail though:

There is also one thing to fix: the array_filter should happen before the array_map (misses are stored as null, which can't "unserialized" and shouldn't be cached.)

dmaicher · 2017-07-18T17:54:41Z

src/Symfony/Bundle/FrameworkBundle/CacheWarmer/AbstractPhpFileCacheWarmer.php

+        // the ArrayAdapter stores the values serialized
+        // to avoid mutation of the data after it was written to the cache
+        // so here we un-serialize the values first
+        $values = array_map(function ($val) { return unserialize($val); }, array_filter($arrayAdapter->getValues()));


@nicolas-grekas is it fine to apply array_filter here as well for the other cache warmers? Tests are fine although the AnnotationsCacheWarmer does not have any tests 😢

$values = array_map(function ($val) { return null !== $val ? unserialize($val) : null; }, $arrayAdapter->getValues());
and array_filter should be called only for ValidatorCacheWarmer, and only when feeding phpArrayAdapter.
the foreach then should iterate over all values, null or not

nicolas-grekas · 2017-07-18T20:30:37Z

src/Symfony/Bundle/FrameworkBundle/CacheWarmer/SerializerCacheWarmer.php

    }

    /**
     * {@inheritdoc}
     */
-    public function warmUp($cacheDir)
+    public function isOptional()


this could also be moved to the parent I guess

nicolas-grekas · 2017-07-18T20:38:07Z

src/Symfony/Bundle/FrameworkBundle/CacheWarmer/AbstractPhpFileCacheWarmer.php

+use Symfony\Component\Cache\Adapter\ProxyAdapter;
+use Symfony\Component\HttpKernel\CacheWarmer\CacheWarmerInterface;
+
+abstract class AbstractPhpFileCacheWarmer implements CacheWarmerInterface


should be marked @internal

dmaicher · 2017-07-18T20:51:17Z

@nicolas-grekas changes done 👍

nicolas-grekas · 2017-07-18T20:53:36Z

src/Symfony/Bundle/FrameworkBundle/CacheWarmer/AbstractPhpFileCacheWarmer.php

+use Symfony\Component\HttpKernel\CacheWarmer\CacheWarmerInterface;
+
+/**
+ * @internal This class is meant for internal use only.


usually we don't add the additional comment so we should remove it I guess

nicolas-grekas · 2017-07-18T20:54:53Z

src/Symfony/Bundle/FrameworkBundle/CacheWarmer/AbstractPhpFileCacheWarmer.php

+abstract class AbstractPhpFileCacheWarmer implements CacheWarmerInterface
+{
+    protected $phpArrayFile;
+    protected $fallbackPool;


shouldn't these both be private?

Indeed 👍

nicolas-grekas · 2017-07-18T21:03:41Z

src/Symfony/Bundle/FrameworkBundle/CacheWarmer/AnnotationsCacheWarmer.php


-            return;
+            return false;


this makes me think that doWarmUp doesn't need the $phpArrayAdapter argument: it's used only here, but returning !== false here would ship the same behavior without the need for this arg, isn't it?
which then triggers: shouldn't we have a clean "bool" return type for all dowarmup methods? ie return true on success? (and here also then)

nicolas-grekas · 2017-07-18T21:09:19Z

src/Symfony/Bundle/FrameworkBundle/CacheWarmer/AbstractPhpFileCacheWarmer.php

+
+        spl_autoload_register(array($phpArrayAdapter, 'throwOnRequiredClass'));
+        try {
+            $this->doWarmUp($cacheDir, $arrayAdapter);


you still need the if (!doWarmUp) then return; check

But then this is not done anymore?

https://github.com/symfony/symfony/pull/23558/files#diff-ec41b7a9f7c5b4c51f70679eb93504b8L61

it will if we make dowarmup return true or false to allow/prevent the warmup

Ah now I see what you meant 😄 👍 Done.

nicolas-grekas · 2017-07-18T21:12:09Z

src/Symfony/Bundle/FrameworkBundle/CacheWarmer/AbstractPhpFileCacheWarmer.php

+        $phpArrayAdapter = new PhpArrayAdapter($this->phpArrayFile, $this->fallbackPool);
+        $arrayAdapter = new ArrayAdapter();
+
+        spl_autoload_register(array($phpArrayAdapter, 'throwOnRequiredClass'));


since we don't need the instance yet, let's use PhpArrayAdapter::class here instead, and create the object later, (or never when dowarmup returns false)

nicolas-grekas · 2017-07-18T21:13:10Z

src/Symfony/Bundle/FrameworkBundle/CacheWarmer/AnnotationsCacheWarmer.php

        $annotatedClassPatterns = $cacheDir.'/annotations.map';

        if (!is_file($annotatedClassPatterns)) {
-            $adapter->warmUp(array());
-
            return;


return true

nicolas-grekas · 2017-07-18T21:13:33Z

src/Symfony/Bundle/FrameworkBundle/CacheWarmer/SerializerCacheWarmer.php

    }

    /**
     * {@inheritdoc}
     */
-    public function warmUp($cacheDir)
+    protected function doWarmUp($cacheDir, ArrayAdapter $arrayAdapter)
    {
        if (!class_exists(CacheClassMetadataFactory::class) || !method_exists(XmlFileLoader::class, 'getMappedClasses') || !method_exists(YamlFileLoader::class, 'getMappedClasses')) {
            return;


return false

nicolas-grekas · 2017-07-18T21:13:51Z

src/Symfony/Bundle/FrameworkBundle/CacheWarmer/ValidatorCacheWarmer.php

    }

    /**
     * {@inheritdoc}
     */
-    public function warmUp($cacheDir)
+    protected function doWarmUp($cacheDir, ArrayAdapter $arrayAdapter)
    {
        if (!method_exists($this->validatorBuilder, 'getLoaders')) {
            return;


return false

nicolas-grekas

LGTM, thanks for your reactivity tonight :)

fabpot · 2017-07-19T04:30:05Z

Thank you @dmaicher.

…g ArrayAdapter (dmaicher) This PR was squashed before being merged into the 3.2 branch (closes #23558). Discussion ---------- [FrameworkBundle] fix ValidatorCacheWarmer: use serializing ArrayAdapter | Q | A | ------------- | --- | Branch? | 3.2 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #23544 | License | MIT | Doc PR | - The `ValidatorCacheWarmer` was using an `ArrayAdapter` with `$storeSerialized=false`. This is a problem as inside the `LazyLoadingMetadataFactory` the metaData objects are mutated (parent class constraints are merged into it) after they have been written into the cache. So this means that currently when warming up the validator cache actually the merged metaData version is finally taken from the `ArrayAdapter` and written into the `PhpFilesAdapter`. Which then caused some duplicate constraints as the parent constraints are merged again after fetching from the cache inside `LazyLoadingMetadataFactory`. This fix makes sure we serialize objects into the `ArrayAdapter`. Writing a test case for this does not seem easy to me. Any ideas? EDIT: Maybe its even safer to just clone the object when writing it into the cache? ```diff diff --git a/src/Symfony/Component/Validator/Mapping/Factory/LazyLoadingMetadataFactory.php b/src/Symfony/Component/Validator/Mapping/Factory/LazyLoadingMetadataFactory.php index 79ad1f2..88eaf33 100644 --- a/src/Symfony/Component/Validator/Mapping/Factory/LazyLoadingMetadataFactory.php +++ b/src/Symfony/Component/Validator/Mapping/Factory/LazyLoadingMetadataFactory.php @@ -117,7 +117,7 @@ class LazyLoadingMetadataFactory implements MetadataFactoryInterface } if (null !== $this->cache) { - $this->cache->write($metadata); + $this->cache->write(clone $metadata); } ``` Opinions? Commits ------- c0556cb [FrameworkBundle] fix ValidatorCacheWarmer: use serializing ArrayAdapter

carsonbot added Status: Needs Review FrameworkBundle Bug labels Jul 17, 2017

[FrameworkBundle] fix ValidatorCacheWarmer: use serializing ArrayAdapter

9b84d72

dmaicher force-pushed the validator-cache-warmer-fix branch from ddd9afd to 9b84d72 Compare July 17, 2017 20:35

dmaicher mentioned this pull request Jul 17, 2017

Validation cache duplicates constraints #23544

Closed

nicolas-grekas added this to the 3.2 milestone Jul 17, 2017