Fix bug with prefetching on different nesting levels

[sudevva]

Fix the bug with prefetching on different nesting levels
As a side effect, it's now allowed a Promise to be returned from prefetchMethod with returnRequested: true, so Overblog Dataloader can be used.
Also, there's no need to map a prefetched result to return a value.

update version of related PR #519
also can potentially help with #187

[oojacoboo]

Thanks for this @sudevva. I'll review soon. Anyone else that's using prefetch, a review would be appreciated.

[oojacoboo]

@sudevva can I get a little more explanation on this PR? I'm having a hard time understanding why a boolean value is a good idea, and what returnRequested even means.

[sudevva]

👋 So about returnRequested. Basically previous when prefetch is used, the function with Prefetch annotation receives an array of resolved values, that should be mapped to source(like $prefetchResult[$blog->getId()]).
This applies a limitation in a way, where you cannot use any Deffered/promise based code in prefetch method.
When returnRequested is true:

• method will recieve a final(resolved) value already mapped to source
• prefetch method can use promises now
• when any of arguments still having a promise we wrap resolver in another Deffered so gql will resolve promises until all arguments becomes resolved value
• you can enforce strict type checks on recieved value
  In the end method will recieve concrete and resolved value(it can be any type), instead of array.
  Now we store result of prefetch in PrefetchBuffer mapped to source, not to hash of arguments, and check is it exists, this resolves a nesting level problem

About bool value for annotation, maybe it will be better to create another type of annotation and mark prevous one as depricated.

Previous implementation where method recieves an array of values creates some confusion for me, why I recieve an array from which I should extract my value.

Thank you @oojacoboo, I hope this will clarify my intentions)

[oojacoboo]

Okay, so am I right in assuming that you added this boolean property as a way of achieving BC? Can we just merge the functionality without adding any additional properties to the Prefetch attribute? It's confusing, I think. I don't see the purpose of it. Why can't the prefetch method return a promise without this boolean value? What advantage does this additional property, returnRequested, provide?

[sudevva]

Okay, so am I right in assuming that you added this boolean property as a way of achieving BC? Can we just merge the functionality without adding any additional properties to the Prefetch attribute? It's confusing, I think. I don't see the purpose of it. Why can't the prefetch method return a promise without this boolean value? What advantage does this additional property, returnRequested, provide?

Do you think I should leave a prefetch behavior as it was previously?
This is how I should map values each time Dataloader is used because I can return only one Promise from preload, that was my concern about default behavior

#[ExtendType(class: Video::class)]
class TagsExtension
{
    #[Field]
    /**
     * @return VideoTag[]
     * @param VideoTag[][] $videoTags     
     * @throws \Exception
     */
    public function getTags(
        Video $video,
        #[Prefetch('preload')] array $videoTags
    ): iterable {
        return $videoTags[$video->getId()]; // I can't return Promise here
    }

    /**
     * @param Video[] $videos
     * @return Promise<VideoTag[][]>
     */
    public static function preload(iterable $videos, #[Autowire] VideoTagResolver $videoTagResolver): Promise {
        return $videoTagResolver->getVideosTags(array_map(fn($video) => $video->getId(), $videos))
            ->then(function ($videoTags) use ($videos) {
                $return = [];
                foreach ($videos as $key => $video){
                    $return[$video->getId()] = $videoTags[$key] ?? [];
                }
                return $return;
            }
        );
    }
}

Btw, I can wrap \TheCodingMachine\GraphQLite\QueryField::__construct $callResolver in deferred as I have done for args until return type is resolved to call assertReturnType with proper value, then I will not use Prefetch anymore.
Take a look at ffbb68b

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.18%. Comparing base (53f9d49) to head (ca0823a).
Report is 91 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master     #702      +/-   ##
============================================
- Coverage     95.72%   95.18%   -0.55%     
- Complexity     1773     1824      +51     
============================================
  Files           154      174      +20     
  Lines          4586     4796     +210     
============================================
+ Hits           4390     4565     +175     
- Misses          196      231      +35     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[oojacoboo]

On ffbb68b, why is this being called unwrapReturnType and not something more fitting like resolveWithPromise?

The overall goal would be to create a more uniform implementation for resolving. Promises seem like a good idea to me for preload functionality. Wrapping non promise prefetch returns seems like a logical way of implementing things.

Please be sure to put line spaces between your control structures - easier for people to read. We're not worried about trying to optimize file sizes.

[sudevva]

hey, I renamed a method, added a forgotten nesting levels test, and added some line spaces.

Is this PR more or less ok?

[oojacoboo]

This is much better now. Just a couple remaining things. We also need updated docs to include details on using promises.

[oojacoboo]

If this is storing the results, maybe "compute" isn't the right term to use for this method name. Also "compute" isn't really a great term to be using here.

[sudevva]

This method was not named by me)
Should I rename it?

doPrefetch | prefetchValues | prefetchResults | processPrefetch | ... ?

[oojacoboo]

Renaming a method where the logic contained within it has materially changed, is the right move. Otherwise, reading and understanding the code for the next developer becomes more confusing.

Being that it's return type is void, it's not really "prefetching results" - you'd expect a return value on that IMO. processPrefetch, cachePrefetchResults, bufferPrefetch; maybe something along these lines.

[sudevva]

Renamed method name to processPrefetch pushed to branch

[sudevva]

If you want I can squash and rebase commits. There is no value in commits about CR fixes and reverting my initial changes.

[oojacoboo]

No that's fine. We do need the docs updated though, with details on using promises. That's the website/docs dir.

[sudevva]

Promise mapping added

[oojacoboo]

Thanks @sudevva!