8000 [Form] Caching resolved $options is dangerous · Issue #6456 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Form] Caching resolved $options is dangerous #6456

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
vicb opened this issue Dec 21, 2012 · 29 comments
Closed

[Form] Caching resolved $options is dangerous #6456

vicb opened this issue Dec 21, 2012 · 29 comments
Labels
Doctrine Form

Comments

@vicb
Copy link
Contributor
vicb commented Dec 21, 2012

In a custom FormType, I have:

<?php

class TypePuissanceFiscaleType extends AbstractType
{
    public function setDefaultOptions(OptionsResolverInterface $resolver)
    {
        $query_builder = function(Options $options) {
            $typeVehicule = $options['type_vehicule_id'];
            return function(EntityRepository $repository) use ($typeVehicule) {
                return $repository->createQueryBuilder('tpf')
                    ->where('tpf.typeVehicule = :typeVehicule')
                    ->setParameter('typeVehicule', $typeVehicule);
            };
        };

        $resolver->setDefaults(array(
            'data_class' => '<class>',
            'class' => '<Bundle>:TypePuissanceFiscale',
            'type_vehicule_id' => null,
            'query_builder' => $query_builder
        ));
    }

    public function getParent()
    {
        return 'entity';
    }

    public function getName()
    {
        return 'typepuissancefiscale';
    }
}

The qb is supposed to return a different value according to the value of $options['type_vehicule_id'].

<?php
$form->add('first', 'typepuissancefiscale', array('type_vehicule_id' => 1);
$form->add('first', 'typepuissancefiscale', array('type_vehicule_id' => 2);

However due to resolved options being cached by form types, the options would be resolved only once and re-used for the second form. This means that both forms would use the same query builder which is different from what I expect.

@bschussek

  1. It is very dangerous and should be caught at least in dev mode (if it implies overhead) rather than silently trigerring a wrong behavior.
  2. Do you have any idea of how to fix this (not on how to avoid using the options to pass parameters).

/cc @yohannpoli
@stof
Copy link
Member
stof commented Dec 21, 2012

Actually, the issue is that the QueryBuilder is not cached properly. The DQL string is not the only part that can change, which mean the way the cache key is computed is wrong. It need to depend on the query params, query hints, ORM filters and so on

@vicb
Copy link
Contributor Author
vicb commented Dec 21, 2012

@stof are you working on this ? any ETA ?

@vicb
Copy link
Contributor Author
vicb commented Dec 21, 2012

@stof Seems to be a lot better by disabling the cache ! Thanks for the hint.

@stof
Copy link
Member
stof commented Dec 21, 2012

@vicb no I'm not. For now, I would suggest reverting b604eb7 as it is breaking things by generating wrong cache keys

@vicb
Copy link
Contributor Author
vicb commented Dec 21, 2012

@bschussek what do you think about reverting the linked commit ?
@bschussek @stof @vicb please update this issue when working on the issue. /cc @fabpot

@sstok
Copy link
Contributor
sstok commented Dec 22, 2012

An easy fix would be the include parameters in the cache key as they are most likely to change.
At least it would fix this problem.

@vicb
Copy link
Contributor Author
vicb commented Dec 22, 2012

That's about what stof Saïd.

----- Reply message -----
De : "Sebastiaan Stok" notifications@github.com
Pour : "symfony/symfony" symfony@noreply.github.com
Cc : "Victor Berchet" victor@suumit.com
Objet : [symfony] [Form] Caching resolved $options is dangerous (#6456)
Date : sam., déc. 22, 2012 11:14
An easy fix would be the include parameters in the cache key as they are most likely to change.

At least it would fix this problem.

—

Reply to this email directly or view it on GitHub.

@sstok
Copy link
Contributor
sstok commented Dec 22, 2012

I know 😄 I was just suggesting it as a quick fix.

@vicb
Copy link
Contributor Author
vicb commented Dec 22, 2012

@stof suggested a better quick fix (reverting) but the real fix should also be quick if anybody has time these days.

@gquemener
Copy link
Contributor

What d'you think? Is it valuable enough for a PR?

@vicb
Copy link
Contributor Author
vicb commented Dec 27, 2012

Good start, I would also use other variables mentionned by stof and I would not modify param values.

@gquemener
Copy link
Contributor

Something like that (but with the parameters) would be great, wouldn't it? https://github.com/doctrine/doctrine2/blob/master/lib/Doctrine/ORM/Query.php#L639-L649

@sstok
Copy link
Contributor
sstok commented Dec 27, 2012

@gquemener 👍

Doctrine actually has multiple cache types, DQL ->SQL is just one of theme.

Edit:

https://github.com/doctrine/doctrine2/blob/master/lib/Doctrine/ORM/AbstractQuery.php#L782
https://github.com/doctrine/doctrine2/blob/master/lib/Doctrine/ORM/AbstractQuery.php#L487
https://github.com/doctrine/dbal/blob/master/lib/Doctrine/DBAL/Cache/QueryCacheProfile.php

Maybe its best if we can use the information and cache key generation of Doctrine instead of making something our self?

@gquemener
Copy link
Contributor

IMO, an existing cache-id generation mechanim should be used.
Problem with https://github.com/doctrine/doctrine2/blob/master/lib/Doctrine/ORM/AbstractQuery.php#L782 is that it requires that a ResultCacheDriver has been configured in Doctrine (which is not always the case I think).

@stof
Copy link
Member
stof commented Dec 27, 2012

@sstok We cannot as this cache does not correspond to any Doctrine cache level: it is caching the hydrated results based on the original DQL query, which means it has to take all settings into account whereas each Doctrine cache only uses a few of them (the query cache does not need to take parameters into account for instance as they are not part of the DQL -> SQL conversion, and the result cache does not need to handle DQL specific settings as it works with the SQL).

@gquemener
Copy link
Contributor

What about this one? https://github.com/gquemener/symfony/blob/4fa4a9e2229723baf3b6bc6b942893f9fe7b9f4f/src/Symfony/Bridge/Doctrine/Form/Type/EntityType.php#L69-L79

@weaverryan
Copy link
Member

+1, I'm hitting this bug as well :/.

I 8000 f you're looking for a workaround, you need to find a way to somehow change the DQL between the different uses of this. For example, in one use I order by a name property on the query builder, and in another instance, I order by a different property. This may not work for everyone, but if you can, it's the most straightforward workaround.

Ping Mr @bschussek! :) This is a bug in a security maintenance release, so we should either revert sha: b604eb7 as @stof suggested or perhaps have an opt-in for this feature (some option to turn it on) - and actually fix it on a non-maintenance release.

Thanks!

@gquemener
Copy link
Contributor

As a quickfix in your project you can just avoid to use placeholders/parameters in your dql and it'll work (well if you're not using different hints, filters, ...).

What do you mean @weaverryan when you say "change the DQL between the different uses of this"? The real issue here is about generating a valid unique cache id for a given query builder, right?

@fabpot
Copy link
Member
fabpot commented Dec 29, 2012

@bschussek I'm going to revert b604eb7 (any objections?). Another solution should happen in master.

@vicb
Copy link
Contributor Author
vicb commented Dec 29, 2012

@fabpot it would be great to open a new issue while reverting so that the optimization don't get lost.

@fabpot
Copy link
Member
fabpot commented Dec 29, 2012

reverted.

@bicpi
Copy link
Contributor
bicpi commented Jan 3, 2013

+1 - I'm hitting on this, too.

@gquemener: Comment by @weaverryan describes a workaround so that you get different cache IDs until this bug is fixed: Different DQL = Different cache ID

// first usage
'query_builder' => function(EntityRepository $er) use ($client) {
    return $er->getByTypeQueryBuilder(Address::TYPE_CONTACT);
},
// ...

// second usage
'query_builder' => function(EntityRepository $er) use ($client) {
    return $er->getByTypeQueryBuilder(Address::TYPE_INVOICE);
        // Workaround: change DQL => triggers new cache ID
        ->andWhere('1=1');
},

Instead of "AND 1=1" you could also add something else which does not change the result (e.g. add another meaningless ORDER BY if your DQL does already manages the order for your view).

@vicb
Copy link
Contributor Author
vicb commented Jan 4, 2013

@fabpot what about closing this issue & create and enhancement request ?

@fabpot
Copy link
Member
fabpot commented Jan 4, 2013

@vicb that would help for sure. thanks.

@vicb
Copy link
Contributor Author
vicb commented Jan 4, 2013

thanks

Do you expect me to do this ?

@Tobion Tobion mentioned this issue Jan 18, 2013
Closed
@ianfp
Copy link
ianfp commented Jan 18, 2013

+1 from me, too. I filed issue 6787, which as @Tobion says is probably a dup.

@ianfp
Copy link
ianfp commented Feb 16, 2013

Any updates on this? This bug is a major nuisance. Or at least some way to disable the caching until there is a fix? The workaround suggested by @weaverryan works, but it's a pretty ugly kludge.

@vicb
Copy link
Contributor Author
vicb commented Feb 16, 2013

If @fabpot reverted it as indicated in a previous comment, the easy fix its to update sf.

@webmozart webmozart mentioned this issue Oct 16, 2014
Closed
3 tasks
@wouterj
Copy link
Member
wouterj commented Feb 19, 2015

The commit is reverted, so I think this can be closed?

@Tobion Tobion closed this as completed Feb 19, 2015
@webmozart webmozart mentioned this issue Mar 25, 2015
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Doctrine Form
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
10 participants
@fabpot @weaverryan @vicb @bicpi @stof @Tobion @gquemener @wouterj @sstok @ianfp
0