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

Cleaner and simpler field deferring #738

Open
oprypkhantc opened this issue Mar 13, 2025 · 8 comments · May be fixed by #739
Open

Cleaner and simpler field deferring #738

oprypkhantc opened this issue Mar 13, 2025 · 8 comments · May be fixed by #739

Comments

@oprypkhantc
Copy link
Contributor

Hey

We've found use cases for deferring field resolution directly, without parameters or prefetch, e.g.:

#[Type]
class SomeType {
    #[Field(outputType: 'Int!')]
    public function complexCalculation(
        bool $arg1
    ): Deferred {
        // Do something with $arg1, similar to prefetch

        // Return value from some kind of buffer, again, similar to prefetch
        return new Deferred(fn () => 123);
    }
}

Currently it's possible, but ugly:

  • requires the use of outputType
  • forces the return type to be GraphQL\Deferred
  • forces creation of new GraphQL\Deferred instance everywhere

I propose that such cases are simplified into returning a (typed) callable, which removes the dependency on Deferred from userland code, as well as allows specifying an actual PHPDoc type:

#[Type]
class SomeType {
    /** @return callable(): int */
    #[Field]
    public function complexCalculation(
        bool $arg1
    ): callable {
        // Do something with $arg1, similar to prefetch

        // Return value from some kind of buffer, again, similar to prefetch
        return fn () => 123;
    }
}

The implementation should be somewhat trivial - under the hood the callable value would just get wrapped in Deferred, and additional root type mapper would handle callable() PHPDoc types. Thoughts?

@oojacoboo
Copy link
Collaborator

oojacoboo commented Mar 14, 2025

Interesting @oprypkhantc, can you explain a bit more on the use case for this? Where is this needed where prefetch isn't an ideal solution? And how does deferring the execution of the field affect the output (load time)? I'm under the assumption that fields are only executed if they're part of the operation's GQL.

@oprypkhantc
Copy link
Contributor Author

Sure. In our case, we're trying to optimize fields that do SQL aggregations into one SQL query, e.g.:

#[Type]
class User {
    #[Field]
    public function commentsCount(
        #[Prefetch('prefetchCommentsCount') array $prefetched,
    ): int {
        return $prefetched[$this->id];
    }

	/**
	 * @param list<User> $users
	 */
	public static function prefetchCommentsCount(array $users): array
	{
		return Collection::wrap($users)
			->loadCount([
				'comments as comments_count',
			])
			->mapWithKeys(fn (User $user) => [
				$user->id => (int) $user->comments_count,
			])
			->all();
	}

    #[Field]
    public function blogsCount(
        #[Prefetch('prefetchBlogsCount') array $prefetched,
    ): int {
        return $prefetched[$this->id];
    }

	/**
	 * @param list<User> $users
	 */
	public static function prefetchBlogsCount(array $users): array
	{
		return Collection::wrap($users)
			->loadCount([
				'blogs as blogs_count',
			])
			->mapWithKeys(fn (User $user) => [
				$user->id => (int) $user->blogs_count,
			])
			->all();
	}
}

Which executes two SQL requests (once in prefetchCommentsCount, once in prefetchBlogsCount) for the following query:

query {
    users(ids: [1, 2, 3]) {
        nodes {
            commentsCount
            blogsCount
        }
    }
}
select id, (select count(*) from comments where user_id = users.id) as comments_count 
from users 
where id in (1, 2, 3);

select id, (select count(*) from blogs where user_id = users.id) as blogs_count 
from users 
where id in (1, 2, 3);

When a model has 10+ available aggregated fields, it doesn't seem like a good idea to make 10 separate queries instead of 1. So I've looked at how other GraphQL implementations do that, and it seems that you can do it in a different way:

#[Type]
class User {
    /** @return int */
    #[Field]
	public function commentsCount(
		#[Autowire] EloquentBatchLoader $loader,
		ResolveKey $resolveKey,
	) {
		return $loader->defer(
			$resolveKey,
			$this,
			fn (Builder $query) => $query->withCount([
                'comments as comments_count',
            ]),
			fn (User $user) => (int) $user->comments_count,
		);
	}

    /** @return int */
    #[Field]
	public function blogsCount(
		#[Autowire] EloquentBatchLoader $loader,
		ResolveKey $resolveKey,
	) {
		return $loader->defer(
			$resolveKey,
			$this,
			fn (Builder $query) => $query->withCount([
                'blogs as blogs_count',
            ]),
			fn (User $user) => (int) $user->blogs_count,
		);
	}
}

Under the hood, EloquentBatchLoader records all of the models and returns a Deferred instance. When GraphQL actually wants to resolve the field, it resolves the Deferred callback, which batches all models into one SQL builder, applies both callbacks (->withCount(['comments as comments_count']) and ->withCount(['blogs as blogs_count'])), then fetches the result as a single SQL query:

select id, 
    (select count(*) from comments where user_id = users.id) as comments_count,
    (select count(*) from blogs where user_id = users.id) as blogs_count
from users 
where id in (1, 2, 3);

Basically the same thing Prefetch attribute does, but with userland control over when and how data is stored and fetched. It works as-is, but the native return types have to be omitted (e.g. missing : Deferred, and the PHPDoc annotation must specify an incorrect return type). It'd be nicer if this was supported out of the box.

@oojacoboo
Copy link
Collaborator

oojacoboo commented Mar 14, 2025

What is the current behavior of a callable return type? Is that even supported? You're suggesting that a callable is wrapped and treated as a deferred by default? So the proposal would be to support Deferred and callable, where a callable is just internally wrapped.

@oprypkhantc
Copy link
Contributor Author

What is the current behavior of a callable return type?

None, it throws CannotMapTypeException. It can't even be handled by custom root type mappers because of this check discarding the callable arguments / return type:

$innerType instanceof Array_

You're suggesting that a callable is wrapped and treated as a deferred by default?

Yes - it sounds logical to me at least :)

So the proposal would be to support Deferred and callable, where a callable is just internally wrapped.

Yes, at least that's the idea. Supporting Deferred<int> as a return type would also make sense, but Deferred is not generic

@oprypkhantc
Copy link
Contributor Author

And how does deferring the execution of the field affect the output (load time)?

I believe almost zero.

I'm under the assumption that fields are only executed if they're part of the operation's GQL.

That's correct, but I'm not sure what you're referring to

@oojacoboo
Copy link
Collaborator

oojacoboo commented Mar 14, 2025

I'm under the assumption that fields are only executed if they're part of the operation's GQL.

That's correct, but I'm not sure what you're referring to

I thought you were doing something else, but it's clear now, so disregard.


I really don't see an issue with the proposal. Being that a callable isn't currently supported - there aren't any BC issues. I think it's also sensible that the implied behavior of a callable return type would be deferred resolution.

How would a callable with with arguments be treated? Is there a default callable signature that makes sense, or would we just throw an exception, not allowing any arguments?

@oprypkhantc
Copy link
Contributor Author

How would a callable with with arguments be treated? Is there a default callable signature that makes sense, or would we just throw an exception, not allowing any arguments?

I guess throw an exception if it has any arguments when mapping the type? It wouldn't be possible to call the callable if it has required parameters anyway. The only way it would work is if callable's parameters are optional, but I don't think there's a point in checking for that 🤔

@oojacoboo
Copy link
Collaborator

Well, an argument could be made for a default signature. I don't know what the ideal one would be though. Of course, those would be passed arguments. The userland callable wouldn't have to implement them.

It could also be possible to define the arguments within an attribute. For instance, instead of the Autowire on the method, that service could be passed to the callable.

That all said, it probably makes the most sense to stick with an exception and simple implementation until there are more clearly defined requirements.

@oprypkhantc oprypkhantc linked a pull request Mar 17, 2025 that will close this issue
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

Successfully merging a pull request may close this issue.

2 participants