doctrine / mongodb-odm

The Official PHP MongoDB ORM/ODM
https://www.doctrine-project.org/projects/doctrine-mongodb-odm/en/latest/
MIT License
1.09k stars 502 forks source link

GridFS referenced returns proxy class instead of File class #2291

Closed etshy closed 3 years ago

etshy commented 3 years ago

Bug Report

Q A
BC Break no
Version 2.2.0

Summary

Referenced GridFS File is a proxy class when getting Parent Document

Current behavior

Not sure if this is intended, I didn't see anything about this in the doc. When you reference a File in a document and get that document, the file property is az proxyClass and you have to manually find the File to get the "real" Document, matching the File class.

How to reproduce

Save a Document with a file referenced. Get the Document (find by Id) Get the referenced File => the File is a proxy

Expected behavior

Save a Document with a file referenced. Get the Document (find by Id) Get the referenced File => the File is the File class

Like I said maybe it's intended to work that way.

Question

I also have a question related. I'd like to know how to work with typehint properties and GridFS reference (if possible).

Currently to save a reference to a File inside a Document, I do an uploadFromFile() and then set the result of that in the Document property. Problem, the result of the upload method is, obviously, not the same class as my File class, so I can't type the File reference property with my file Class, right ?

For example, actually in my Document save method I have something like this

        if ($document->getImage() instanceof ImageInterface) {

            $uploadOptions = new UploadOptions();
            $uploadOptions->metadata = $document->getImage()->getMetadata();

            $file = $this->repository->uploadFromFile($document->getImage()->getRealPath(), $document->getImage()->getName(), $uploadOptions);

            $document->setImage($file);
        }

        parent::save($document);

with the property $image referencing a GridFS file. I can't typehint the $image property because I have to set the "upoaded" file.

Is there a way to properly typehint the gridFS file reference property ?

ps : Let me know if this isn't the place to ask that kind of question.

alcaeus commented 3 years ago

The uploadFromFile method returns a proxy object for the class the repository is for. This is due to the upload not returning necessary metadata for creating an initialised document, so we defer initialisation to the first time a property is accessed in the proxy. However, the public API of the returned proxy object is the same as that of the configured GridFS file. What kind of issues are you having with the proxy object?

etshy commented 3 years ago

The main issue is that I can't type-hint my referenced File property. When i upload a new file, call uploadFromFile() and then set the result to my property I got a php error. For example, "Argument 1 passed to Document::setImage() must implement interface \ImageInterface or be null, instance of MongoDBODMProxies__PM__\App\Model\Persistence\Files\File\Generated457a4653aafba020df1e87bed723101e given" hence my question about typehint.

I don't have this problem when getting a document that reference a File though, so I guess I forgot a step, or something.

It didn't show with dd() or var_dump() but it seems the proxy object can access have the File API. Sorry about that.

alcaeus commented 3 years ago

If I understood you correctly, you have a file (excerpt from docs):

use Doctrine\ODM\MongoDB\Mapping\Annotations\File;
use Doctrine\ODM\MongoDB\Mapping\Annotations\Id;

/** @File(bucketName='image') */
class Image
{
    /** @Id */
    private $id;

    // ...
}

When you call uploadFromFile, you get a proxy object back. In this case, the class could look like this:

class ImageProxy /* would be a random name */ extends Image
{
    // ...
}

So if your setter accepted the original Image class, it would automatically accept the proxy as well. For what it's worth, this is why the metadata drivers require documents and files to be non-final since proxies might be generated. For embedded documents and query result documents there is no such limitation.

I'm not entirely sure what your upload logic does there. Looking at this, I assume you receive some ImageInterface instance that represents an image on the disk and want to upload it to MongoDB. However, your setImage method only accepts ImageInterface instances, which your original file doesn't seem to be.

You can also verify that (note that code like this should never end up in production):

$image = $this->repository->uploadFromFile($document->getImage()->getRealPath(), $document->getImage()->getName(), $uploadOptions);
$this->dm->detach($image);

$image = $this->repository->find($image->getId());

The call to detach removes the image from the document manager's list of documents and triggers a read from the database. This will yield a non-proxied document. I'd expect for the call to fail as well.

You also can't make your image document implement the image interface, as that would cause repeated uploads that you don't want.

etshy commented 3 years ago

My upload logic is simple. I made an UploadFileType with a Transformer that transform an UploadedFile into my File or Image class.

My Image class extends my File class and implements an ImageInterface (my Image class is empty, it's just to separate Image from File and have different metadata)

My mapping is in xml and the following File.mongodb.xml

    <gridfs-file name="App\Model\Persistence\Files\File" repository-class="App\Model\ODM\Repository\FileRepository"
                 bucket-name="files">
        <id/>
        <length/>
        <chunk-size/>
        <upload-date/>
        <filename field-name="name"/>
        <metadata target-document="App\Model\Persistence\Files\FileMetadata">
            <discriminator-field name="type"/>
            <discriminator-map>
                <discriminator-mapping value="file" class="App\Model\Persistence\Files\FileMetadata" />
                <discriminator-mapping value="image" class="App\Model\Persistence\Files\ImageMetadata" />
            </discriminator-map>
        </metadata>
    </gridfs-file>

Image.mongodb.xml

    <gridfs-file name="App\Model\Persistence\Files\Image" repository-class="App\Model\ODM\Repository\FileRepository"
                 bucket-name="files">

        <metadata target-document="App\Model\Persistence\Files\ImageMetadata"/>
    </gridfs-file>

When I submit the form, I have something like that

    if ($document->getImage() instanceof ImageInterface
            && !is_null($document->getImage()->getRealPath())) {
            $uploadOptions = new UploadOptions();
            $uploadOptions->metadata = $document->getImage()->getMetadata();
            $avatarProxy =  $this->repository->uploadFromFile($document->getImage()->getRealPath(), $document->getImage()->getName(), $uploadOptions);
            $avatarProxy = $this->fileService->save($document->getImage());
            $document->setImage($avatarProxy);
    }

The Metadata is an instance of ImageMetadata class.

From a dd(); my $document->getImage() is an instance of Image with ImageMetadata and the proxy generated $avatarProxy is an instance of MongoDBODMProxies\__PM__\App\Model\Persistence\Files\File\Generated457a4653aafba020df1e87bed723101e - App\Model\Persistence\Files\File@proxy so a proxy of File and not Image and it doesn't pass the type-hinting with ImageInterface.

By removing the Type-hinting I can upload, and when I dd() the getImage(), I have a proxy of Image this time.

Maybe making my Image class extends File is wrong ?

alcaeus commented 3 years ago

Is the repository an ImageRepository or a FileRepository?

etshy commented 3 years ago

The Repo is FileRepository for both Image and File. I guess that's why I have a Proxy of File when I upload, but then why have I an Image proxy when I get the document (and the referenced Image) ? is there an internal process that detect the right class to proxy ?

alcaeus commented 3 years ago

These two documents may very well have the same repositoryClass configured, but it's important that you retrieve the correct repository from the document manager. If you upload a file through a repository fetched like this:

$dm->getRepository(File::class)->uploadFromFile(...);

the result will always be a File proxy object. To receive an image proxy, you need to fetch the repository for the Image class:

$dm->getRepository(Image::class)->uploadFromFile(...);

What matters is not the class of the repository (that "only" allows you to extend the default repository API), but the ClassMetadata instance that is injected in the repository when creating it.

etshy commented 3 years ago

Okay I see the problems but I'll have to think of another way to instantiate my Repo.

Actually the upload file part is in a Class FileService in a method save. I inject FileRepository using Symfony Dependency Injection and my FileRepository __construct is set to File::class for now. (I followed the doc here : https://symfony.com/doc/current/bundles/DoctrineMongoDBBundle/first_steps.html#service-repositories)

class FileRepository extends ServiceGridFSRepository implements RepositoryInterface, FileRepositoryInterface
{

    /**
     * ChapterRepository constructor.
     *
     * @param ManagerRegistry $managerRegistry
     */
    public function __construct(ManagerRegistry $managerRegistry)
    {
        parent::__construct($managerRegistry, File::class);
    }
class FileService
{
    public function __construct(FileRepositoryInterface $fileRepository, FileManager $fileManager)
    {
        $this->repository = $fileRepository;
        $this->om = $fileRepository->getObjectManager();
        $this->fileManager = $fileManager;
    }

    public function save(FileInterface $file)
    {
        if (is_null($file->getRealPath())) {
            //TODO make exception
        }

        $uploadOptions = new UploadOptions();
        $uploadOptions->metadata = $file->getMetadata();

        return $this->repository->uploadFromFile($file->getRealPath(), $file->getName(), $uploadOptions);
    }

To solve my problem I'll have to instanciate my FileRepository with Image::class but I don't know how to make it dynamic (either File or Image) as it's during dependency injection phase. In my FileService, is there a good way to instantiate the right repository (or I can instantiate for both File and Image) ?

I made a "dirty fix" that change the repository from File to Image like that

    /**
     * @param $class
     */
    public function changeRepository($class)
    {
        $className = get_class($class);
        $manager = $this->managerRegistry->getManagerForClass($className);
        $classMetadata = $manager->getClassMetadata($className);
        $this->documentName = $classMetadata->name;
        $this->dm           = $manager;
        $this->uow          = $manager->getUnitOfWork();
        $this->class        = $classMetadata;
    }

That's really dirty but it works, and I don't really know how to do better right now without making another class ImageRepository.

ps : Sorry it became more of a Support Question rather than a Bug report post.

alcaeus commented 3 years ago

No worries. I wouldn't recommend messing with the repository class, you're asking for trouble there. If you want to have the quick and easy Symfony set up, you'd create an ImageRepository the same way you created your file repository. Then, in your FileService, inject both repositories and use the one that's appropriate for what you're doing. Since I don't know why you have a file and an image repository (that both seem to point to the same bucket as well), I can't tell you how your FileService decides which repository to use.

etshy commented 3 years ago

Yeah I know it's asking for problems but that was to test to repository issue. I have a File and a Image class to have different Metadata (metadata for image contains informations about the image, like the height and width in pixel and I will add some other info later) and a different class/interface. And because of that I have the repository issue.

I think the more proper way to sovle this is indeed to make an ImageRepository and to inject both in my FileService even if the ImageRepository will be empty (concerning db/repo Image and File have the same methods).

Thanks a lot for all the informations, I close the thread as it's solved for my part.