Field middlewares executed only once per application lifecycle

[oprypkhantc]

Hey.

When working with long-running servers (such as Laravel Octane, which processes multiple requests before dying, contrary to php-fpm which only processes one), a schema is only resolved once (in both graphqlite-laravel and graphqlite-bundle), which means field middlewares are also only executed once. This is awesome for the speed, but the problem is that all subsequent requests can't change the fields dynamically. That also means that even some built-in features like #[HideIfUnauthorized] stop working on subsequent requests if instance of Schema is reused.

Iterating over fields with reflection & annotations and other things on each request is obviously not an option as it defeats the purpose of a long-running server. One option is to run all field middlewares on each request, which should be a non-breaking change. Another option is to change field middlewares so they can act as both static middleware and a "dynamic" middleware which is triggered on every request.

Any thoughts?

I'm willing to implement a fix, but we need to agree on the plan first.

[oojacoboo]

Hmm, good question here. I’m assuming the schema is built and cached on each request and these annotations are used to build it accordingly?

Assuming that’s the case, I’d think the schema would need to be rebuilt based on request context.

[oprypkhantc]

Yes, you are correct.

Rebuilding the whole Schema from scratch (through SchemaFactory::createSchema) would result in a non-negligible performance impact - even if you reuse some of the instances, we'd still have to call FieldsBuilder::getFieldsByAnnotations on every request for every query & mutation registered, which is quite a lot :(

Instead of doing so, I think limiting it to just field middlewares should be enough? That way the whole Schema stays cached in place; when a specific field is requested, only it's middlewares are triggered, greatly reducing the amount of processing behind each request. This should work for queries, mutations and fields and satisfy the needs of #[HideIfUnauthorized].

[oojacoboo]

I'm really not sure that the FieldsBuilder, alone, is sufficient to ensure that the schema will be accurate for all annotation defined logic. Does that cover operation annotations? There is lots of security middleware used there as well.

Let's define which annotations are dynamic in nature and of concern.

Also, it's worth considering that you could use separate graphql endpoints to define your schemas. I'm not sure that's ideal, but from a performance standpoint, that'd obviously be the best.

[oprypkhantc]

It should be accurate, given only the public APIs / documented features of graphqlite are used. Going through the annotations:

• #[Mutation], #[Query], #[Field], #[MagicField], #[SourceField] are all and only annotations which define fields. There's a lot of information parsed from them and aside from the middlewares, but it doesn't/can't rely on the request. There is one exception to that, however: SourceFieldInterface - one could argue that this should be run on every request and it does make sense, so we could keep track of those and run FromSourceFieldsInterface::getSourceFields() each time
• #[Type], #[ExtendType], #[Input] are static as those don't rely on the request
• #[Factory], #[Decorate] are static as those don't rely on the request
• #[Autowire], #[InjectUser] are static as those parameters are not shown in the Schema
• #[HideParameter] is static because it always hides the parameter from the Schema, not relying on the request
• #[Logged], #[Right], #[FailWith], #[HideIfUnauthorized] are all handled by AuthorizationFieldMiddleware which is partly dynamic:
  • it statically changes the type of the field to nullable
  • it dynamically sets field resolver based on the authorization (which comes from the request and hides the field when not authorized
• #[Security] is static:
  • it statically changes the type of the field to nullable
  • it statically sets field resolver callback (which uses the request, but only when resolving the value - request is not used to change the field definition itself)
• #[UseInputType] is static, similarly to #[HideParameter] it's only parsed once

Based on all of this, the only annotations which are non static are #[Logged], #[Right] and #[HideIfUnauthorized], although first two can be refactored so that they become static as well. This only leaves #[HideIfUnauthorized].

#[HideIfUnauthorized] and #[Logged] can as well be used on inputs, which suffer the exact same problem as regular fields. InputFieldMiddlewareInterface::process has ?InputField return type, so when a user is not authorized, null is returned from AuthorizationInputFieldMiddleware::process, hiding the input field. This means that input fields must also be fixed, likely the same exact way as regular fields.

Also, it's worth considering that you could use separate graphql endpoints to define your schemas. I'm not sure that's ideal, but from a performance standpoint, that'd obviously be the best.

I mentioned performance because the sole goal of projects like Laravel Octane is to reduce boilerplate initialization, not achieve first-class performance. It doesn't seem like much until your production app starts taking 300ms+ to process a simple GET. The point here is just to get adequate performance without fanfare. Preload/preboot/preinit as much as you, but obviously some overhead is still expected.

[oojacoboo]

So, SourceField and MagicField annotations could become an issue since they could have middleware annotations associated, that rely on the request - right?

Where are you thinking it'd be best to handle this refreshing logic?

[oprypkhantc]

Right.

Where are you thinking it'd be best to handle this refreshing logic?

And that.. I don't know yet. Here are my thoughts so far, but I'll spend more time on that tomorrow:

• webonyx/graphql does validation prior to resolving the field, and to do so they need the list of fields and types for them
• thankfully, it seems like webonyx/graphql can handle dynamic field definitions through GraphQL\Type\Definition\UnresolvedFieldDefinition - every time getFields() is called, FieldDefinition can be resolved on-the-fly through a callback. The only thing that can't be changed in field's name - so if we do use this mechanism, changing field's name in a middleware would be forbidden
• we could build the whole Schema once the same way it's built now, but instead of piping all QueryFieldDescriptor through middlewares to get FieldDefinitions, just wrap them all in UnresolvedFieldDescription. That way we have a callback that's called each time a field needs to be resolved. A callback could look something like this:

$fieldDescriptor = new QueryFieldDescriptor();
// do regular field parsing here

$field = new UnresolvedFieldDefinition($fieldDescriptor->getName(), function () use ($fieldDescriptor) {
    return $this->fieldMiddleware->process($fieldDescriptor, new class implements FieldHandlerInterface {
                public function handle(QueryFieldDescriptor $fieldDescriptor): ?FieldDefinition
                {
                    return QueryField::fromFieldDescriptor($fieldDescriptor);
                }
            });
});

$queryList[] = $field;

In theory, something like this should work with no changes to webonyx/graphql and great performance. If this works then there are more things to consider, like should UnresolvedFieldDefinition's callback cache the result for a single request lifespan? It probably should?

[oprypkhantc]

One more thought: we could simply allow FieldHandlerInterface to return FieldDefinition|UnresolvedFieldDefinition|null and avoid wrapping the field with UnresolvedFieldDefinition ourselves. Then if developer of a middleware wants the field definition to be dynamic, all they need is to return a UnresolvedFieldDefinition and it should just work.

[Lappihuan]

@oprypkhantc check out this field middleware which overwrites and wraps the actual field resolver:
https://github.com/thecodingmachine/graphqlite/blob/master/src/Middlewares/SecurityFieldMiddleware.php#L74-L97

we use this to ensure code is only executed if the field is being resolved instead of introspection or other things.

[oprypkhantc]

Based on webonyx/graphql-php#1329, this will not work unfortunately. The suggested method is to rebuild the whole Schema :/

It seems like we can rebuild the Schema optimally with some refactors. I need your approval to see if it's both worth my time and your/maintainers time maintaining it afterwards:

Instead of calling FieldsBuilder everywhere directly, I plan on refactoring it into an interface. I'll provide a CachingFieldsBuilder that caches resolved fields, and it will be reused across multiple Schema instances. I'll then wrap it in a MiddlewareFieldsBuilder which actually calls middlewares each time a field is returned. Then we just need to create a new Schema instance and it would be able to use the already parsed fields.

If you don't feel like this is worth it - no worries. I think I can create a workaround for my use case.

[oprypkhantc]

@Lappihuan This only works to resolve the value. If you want to hide the field or change any of it's details (type, description etc), you can't do that.

[Lappihuan]

sounds like you would need to rebuild the schema then.
this is a relevant issue from some time ago: #307

[oprypkhantc]

My main concern here is the burden on this project that comes with supporting this use case. It's not widespread yet, but it's definitely rising in popularity, especially in bigger projects. The decision on whether to support this is on you guys.

[Lappihuan]

you could probably check if there is a cache implementation for swoole that handles caches per request and invalidates them after.
that would probably solve most of your issues in the library.

[Lappihuan]

otherwise you can also overwrite the middlewares provided to always do the wrapped approach.
we for example overwrite the one handling "Logged" to invert its behavior and have our own "IsPublic" attribute.