thecodingmachine / graphqlite

Use PHP Attributes/Annotations to declare your GraphQL API
https://graphqlite.thecodingmachine.io
MIT License
557 stars 98 forks source link

prefetchMethod does not work #615

Closed mshapovalov closed 1 year ago

mshapovalov commented 1 year ago

Hello, thank you, guys, for the incredible library that makes my work much easier, but I have found a bug or maybe my misunderstanding of the documentation. I use graphqlite-bundle v.6.0.0

    /**
     * @param ApplicationVersionType[] $prefetchedVersions
     * @return ApplicationVersionType[]
     */
    #[Field(outputType: '[ApplicationVersion]', prefetchMethod: 'prefetchApplicationVersions')]
    public function getVersions(ApplicationType $application, array $prefetchedVersions): array
    {
        return array_filter(
            $prefetchedVersions,
            fn (ApplicationVersionType $applicationVersion) => $applicationVersion
                    ->getApplicationId() === $application->getId(),
        );
    }

    /**
     * @param ApplicationType[] $applications
     * @return ApplicationVersionType[]
     */
    public function prefetchApplicationVersions(
        array $applications,
        #[Autowire]
        ApplicationVersionVault $applicationVersionVault,
    ): array {
        $applicationIds = array_map(
            fn (ApplicationType $application) => Uuid::fromString($application->getId()),
            $applications,
        );

        return $applicationVersionVault->find(ApplicationVersionFilter::create()->withApplicationIds($applicationIds));
    }

I get and exception:

For parameter $prefetchedVersions, in Modules\Orchestration\Ui\Application\GraphQl\Types\ApplicationType::getVersions, cannot map class "Modules\Orchestration\Ui\ApplicationVersion\GraphQl\Types\ApplicationVersionType" to a known GraphQL input type. Are you missing a @Factory annotation? If you have a @Factory annotation, is it in a namespace analyzed by GraphQLite? (500 Internal Server Error)

If I remove type hint:

    /**
     * @return ApplicationVersionType[]
     */
    #[Field(outputType: '[ApplicationVersion]', prefetchMethod: 'prefetchApplicationVersions')]
    public function getVersions(ApplicationType $application, array $prefetchedVersions): array
    {
        return array_filter(
            $prefetchedVersions,
            fn (ApplicationVersionType $applicationVersion) => $applicationVersion
                    ->getApplicationId() === $application->getId(),
        );
    }

I get this:

<!-- For parameter $prefetchedVersions, in Modules\Orchestration\Ui\Application\GraphQl\Types\ApplicationType::getVersions, please provide an additional @param in the PHPDoc block to further specify the type of the array. For instance: @param string[] $prefetchedVersions. (500 Internal Server Error) -->
oojacoboo commented 1 year ago

So, we're not currently using prefetch as we're able to get a lot of our performance with ORM hydration. Maybe someone else can chime in that's used it more in depth.

That said, what's your ApplicationVerionType class look like? Does it have an InputType annotation on it? I don't know if that should be required for prefetch, but that's certainly the issue based on the exception message.

mshapovalov commented 1 year ago

Are you talking about this annotation? use TheCodingMachine\GraphQLite\Annotations\Input? no ApplicationVerionType does not have it, when I add this annotation I have another problem when I try to execute a Query that returns ApplicaitonType with ApplicationVersionType I get the error:

{"errors":[{"message":"Field \u0022versions\u0022 argument \u0022prefetchedVersions\u0022 of type \u0022[ApplicationVersionTypeInput!]!\u0022 is required but not provided.","locations":[{"line":3,"column":37}]}]}
oojacoboo commented 1 year ago

Sorry, yes - we named it Input - wish it was InputType...

That error is saying you're missing a Field attribute on versions. I don't know what that class looks like, but it seems that's required as a field for some reason. If you need to return ApplicationVersionType as a type, you need to annotate it with the Type attribute. Type is for output types and Input is for input types. The same class can be used for both.

mshapovalov commented 1 year ago

here is my ApplicationVersionType

use TheCodingMachine\GraphQLite\Annotations\Field;
use TheCodingMachine\GraphQLite\Annotations\Type;

#[Type(name: 'ApplicationVersion')]
final readonly class ApplicationVersionType
{
    public function __construct(
        private ApplicationVersionDto $applicationVersion,
    ) {
    }

And here is the Application Type class:

use TheCodingMachine\GraphQLite\Annotations\Autowire;
use TheCodingMachine\GraphQLite\Annotations\Field;
use TheCodingMachine\GraphQLite\Annotations\Type;

#[Type(name: 'Application')]
final readonly class ApplicationType
{
    public function __construct(
        private ApplicationDto $application,
    ) {
    }
 ....

 /**
     * @param ApplicationVersionType[] $prefetchedVersions
     * @return ApplicationVersionType[]
     */
    #[Field(outputType: '[ApplicationVersion]', prefetchMethod: 'prefetchApplicationVersions')]
    public function getVersions(ApplicationType $application, array $prefetchedVersions): array
    {
        return array_filter(
            $prefetchedVersions,
            fn (ApplicationVersionType $applicationVersion) => $applicationVersion
                    ->getApplicationId() === $application->getId(),
        );
    }

    /**
     * @param ApplicationType[] $applications
     * @return ApplicationVersionType[]
     */
    public function prefetchApplicationVersions(
        array $applications,
        #[Autowire]
        ApplicationVersionVault $applicationVersionVault,
    ): array {
        $applicationIds = array_map(
            fn (ApplicationType $application) => Uuid::fromString($application->getId()),
            $applications,
        );

        return $applicationVersionVault->find(ApplicationVersionFilter::create()->withApplicationIds($applicationIds));
    }
mshapovalov commented 1 year ago

If I understand the error message correctly it says that I didn't provide the prefetchedVersions argument which should be injected automatically by prefetchApplicationVersions method, if I understand prefetching mechanism correctly.

mshapovalov commented 1 year ago

And I need ApplicationVersionType only as the return type, so I would not like to add the#[Input] attribute to it if it is possible.

oojacoboo commented 1 year ago

That all seems correct to me - yes. Again, I don't have experience with using this feature though. It could be a bug - maybe someone else can chime in here that's using prefetch.

mshapovalov commented 1 year ago

After investigation, i realized that it works if you remove the second parameter from the method that uses prefetched data: Instead of this:

 /**
     * @param ApplicationVersionType[] $prefetchedVersions
     * @return ApplicationVersionType[]
     */
    #[Field(prefetchMethod: 'prefetchApplicationVersions')]
    public function getVersions(ApplicationType $application, array $prefetchedVersions): array
    {
         ....
    }

you should use this:

 /**
     * @param ApplicationVersionType[] $prefetchedVersions
     * @return ApplicationVersionType[]
     */
    #[Field(prefetchMethod: 'prefetchApplicationVersions')]
    public function getVersions(array $prefetchedVersions): array
    {
        ...
    }

Guys, if it is not a bug, please update your documentation and I will close the issue.

oojacoboo commented 1 year ago

@mshapovalov This looks correct! When I saw your original post, I thought it looked strange that you were using two arguments. Then I reviewed the docs, myself, to be sure I wasn't hallucinating. So yea... seems the docs need updating. Thanks for confirming and if you'd like to submit an PR, that'd be great!

mshapovalov commented 1 year ago

With pleasure, but I have no access to push to your repo :)

oojacoboo commented 1 year ago

@mshapovalov you just need to clone this repo under your own account. Then you can submit a PR from your cloned repo and branch.

mshapovalov commented 1 year ago

https://github.com/thecodingmachine/graphqlite/pull/616