8000 Validation cache duplicates constraints · Issue #23544 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content
8000

Validation cache duplicates constraints #23544

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
ossinkine opened this issue Jul 17, 2017 · 7 comments
Closed

Validation cache duplicates constraints #23544

ossinkine opened this issue Jul 17, 2017 · 7 comments
Labels
Bug Status: Needs Review Validator

Comments

@ossinkine
Copy link
Contributor
ossinkine commented Jul 17, 2017 •
edited
Loading
Q A
Bug report? yes
Feature request? no
BC Break report? no
RFC? no
Symfony version >=3.2.2 >=3.3.0-beta1

Validation caching merges constraints for class and all of its parents. It's wrong behaviour, because the class already contains constraints from parent classes. As a result we have duplicated constraint.

The way to reproduce

Create two models, the second one extends the first one:

namespace AppBundle\Model;

class Foo
{
    public $foo;
}
namespace AppBundle\Model;

class Bar extends Foo
{
}

Create validation config, add constraint to the first class, the second class should be defined (empty or with some constraints)

AppBundle\Model\Foo:
    constraints:
        - Expression: "this.foo !== null"

AppBundle\Model\Bar:

And validation of new Bar object will generates two duplicated violations:

$container->get('validator')->validate(new Bar()));

Note: this code should be executed in prod environment, because test environment doesn't use validation cache by default.
@dmaicher
Copy link
Contributor

I cannot reproduce your problem using Symfony v2.8.24. I made sure the validator uses a cache and for me the merging of the constraints ends up with exactly one class constraint for Bar. So the validation results in one violation.

@ossinkine
Copy link
Contributor Author
ossinkine commented Jul 17, 2017 •
edited
Loading

@dmaicher I'll try to reproduce with your version shortly

@ossinkine
Copy link
Contributor Author
ossinkine commented Jul 17, 2017 •
edited
Loading

@dmaicher I was wrong. Affected versions start on 3.2.2. I've updated the top message.
See https://github.com/ossinkine/symfony-standard/tree/issue-23544, run ./bin/console cache:warmup --env=prod and ./bin/console foo --env=prod

@ossinkine ossinkine reopened this Jul 17, 2017
@dmaicher
Copy link
Contributor

Ok indeed bug confirmed using 3.2.2+.

@dmaicher
Copy link
Contributor

Ok after some debugging I found the problem.

It only happens when using the cache warmup because in that case the Psr6Cache uses an ArrayAdapter with $storeSerialized=false which means we are mutating the cache item after it has been written into the cache by calling $this->mergeConstraints($metadata);.

Working on a fix 😉

@dmaicher dmaicher mentioned this issue Jul 17, 2017
Closed
@dmaicher
Copy link
Contributor

@ossinkine I proposed a fix here: #23558

Can you try it to confirm it works for you?

@ossinkine
Copy link
Contributor Author

@dmaicher Yes, it works for me! Great work! 👍

fabpot added a commit that referenced this issue Jul 19, 2017
@fabpot
bug #23558 [FrameworkBundle] fix ValidatorCacheWarmer: use serializin…
1375601 
…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
@fabpot fabpot closed this as completed Jul 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Status: Needs Review Validator
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@fabpot @dmaicher @xabbuh @ossinkine @carsonbot
0