Skip to content
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.

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

Not all ClassMetadata is loaded in prod #8866

Open
bravik opened this issue Jul 27, 2021 · 10 comments
Open

Not all ClassMetadata is loaded in prod #8866

bravik opened this issue Jul 27, 2021 · 10 comments

Comments

@bravik
Copy link

bravik commented Jul 27, 2021

Have an Id as an embeded object:

/**
 * @ORM\Embeddable
 */
final class Id
{
    /**
     * @ORM\Column(name="id", type="integer", nullable=false)
     * @ORM\Id
     * @ORM\GeneratedValue(strategy="AUTO")
     */
    private int $value;

    public function getValue(): int
    {
        return $this->value;
    }
}

Using in a repository like that:

/** @var Id $id */
$this->doctrineRepository->find($id);

Works in dev environment, but in prod environment it somehow converts any Id object to flat int 1 value without any error or exception.

Digging further somewhere here in EntityManager we have identifier resolution for find() method:
image

Inside highlited line we have the following situation for dev:
image

However for prod loadedMetadata is different:
image

It seems like in prod not all class metadata is loaded.
Can't figure out why?

And why Doctrine doesn't throw an error when it sees object as an indentifier and can't find metadata for it?

The same is true for ->findBy([]) method, but ->hasMetadataFor is called in BaseEntityPersister::getValues().

doctrine/doctrine-bundle: 2.4.2
doctrine/orm: 2.9.3
doctrine/orm: 3
doctrine/persistence: 2.2.1

@alcaeus alcaeus transferred this issue from doctrine/DoctrineBundle Jul 27, 2021
@greg0ire
Copy link
Member

greg0ire commented Aug 2, 2021

Could it be that your metadata cache is not cleared?

@bravik
Copy link
Author

bravik commented Aug 3, 2021

Could it be that your metadata cache is not cleared?

No, clearing cache is a first thing I do if any troubles

@greg0ire
Copy link
Member

greg0ire commented Aug 3, 2021

In case you are using php-fpm or another server, please try restarting it too. I see you are using PhpArrayAdapter, which results in OPCache cache being used, and that particular cache might not get cleared properly.

@beberlei
Copy link
Member

beberlei commented Aug 4, 2021

This could be a bug, hasMetadataFor is not good enough here I believe, it might be necessary to call getMetadataFor, which would be more expensive in case of a cache miss.

@bravik
Copy link
Author

bravik commented Aug 5, 2021

In case you are using php-fpm or another server, please try restarting it too. I see you are using PhpArrayAdapter, which results in OPCache cache being used, and that particular cache might not get cleared properly.

I havent' checked that yet. But I think I was debugging this issue using it the same dev docker image without opcache... The only difference was APP_ENV var dev/prod for symfony.

@francoispluchino
Copy link

This could be a bug, hasMetadataFor is not good enough here I believe, it might be necessary to call getMetadataFor, which would be more expensive in case of a cache miss.

@beberlei I reported this strange behavior concerning the hasMetadataFor method but with the use of the metadata cache in the issue doctrine/DoctrineBundle#1308. Indeed, it did not have the same behavior when using the metadata cache or not and the Resolve Target Entities (cf. this comment).

It was ultimately recommended to use isTransient instead of hasMetadataFor to check the presence of a class, but I'm still not sure if this is the expected behavior for the hasMetadataFor method while the getMetadataFor returns the metadata when there is the name of an interface as an argument(and that the Resolve Target Entities is configured).

@bravik
Copy link
Author

bravik commented Nov 5, 2021

So as I getting more of this kind of problems.. and they are successfully bypassing our automated tests,
I tried to futher investigate what's going on...

hasMetadataFor is a misleading method. The name assumes that it will give us wether metadata for class is available or not, but as stated in it's interface's comment and as it is implemented - it only checks wether metadata was loaded in memory (exactly in $this->loadedMetadata array).

In dev cache is always warmed up and all metadata is loaded. In prod the only metadata loaded is the one explicitly requested with getMetadataFor() method.

So you either

  1. Can't use ->hasMetadataFor() check in EntityManager and in BaseEntityPersister as it does not do the job it meant to do:
 if (is_object($value) && $this->metadataFactory->hasMetadataFor(ClassUtils::getClass($value))) {
   $id[$i] = $this->unitOfWork->getSingleIdentifierValue($value);

Maybe the ->hasMetadataFor() check here should be dropped at all. The getMetadataFor() method would be called in getSingleIndentifierValue() and everything should work fine.

if (is_object($value) && $this->metadataFactory->hasMetadataFor(ClassUtils::getClass($value))) {

  1. Or explicitly load embedabes metadata for cached entities.
    In my case for example the reason I have something at all in loadedMetadata is that I'm calling explicitly getClassMetadata in Repository constructor:
class ServiceEntityRepository extends EntityRepository implements ServiceEntityRepositoryInterface
{
    public function __construct(ManagerRegistry $registry, string $entityClass)
    {
        $manager = $registry->getManagerForClass($entityClass);

        parent::__construct($manager, $manager->getClassMetadata($entityClass));
    }
}

When I call getClassMetadata method without cache loadMetadata() method inside will also load all embedables class metadata for host class.

However when I call it with already warmed up cache it will only load the host class, and embedables (which are listed in host class metadata) will not be handled:
https://github.com/doctrine/persistence/blob/1ed5427e2ea50adb17aa6bb40037a8aa6e99a9fe/lib/Doctrine/Persistence/Mapping/AbstractClassMetadataFactory.php#L247

Maybe a quick fix solution would be to make this method load embeded classes from cache with it's host's data.

@bravik
Copy link
Author

bravik commented Feb 10, 2022

Who can I ping for some feedback?

@dpfaffenbauer
Copy link

I am facing the same problem right now. The problem for me is that https://github.com/doctrine/persistence/blob/3.0.x/src/Persistence/Mapping/AbstractClassMetadataFactory.php#L188 doesn't set parent classes as loaded and hasMetadataFor for a parent class return false.

But if it misses the cache it loads the metadata for the whole hierarchy of classes.

@dpfaffenbauer
Copy link

for me the error occurs when gedmo/extensions loads the metadata to check if it is configured, I could narrow it down to the release 3.7.0. As far as I understood: They removed doctrine/cache to use a custom cache, that might be the problem on their side then. (doctrine-extensions/DoctrineExtensions@v3.6.0...v3.7.0)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants