Allow auto isTypeOf when using annotations

[Vincz]

Q	A
Bug report?	no
Feature request?	yes
BC Break report?	no
RFC?	no
Version/Branch	0.13

When using interfaces with annotations, we already have a mapping between PHP classes & GraphQL type. The isTypeOf option could be automatically added to GraphQL types generated from annotated PHP classes so we wouldn't have to write a useless type resolver...

[mcg-web]

This seem OK to me but 0.13 version are feature freeze, we can add this to 1.0-dev .

[murtukov]

@Vincz absolutely agree. But it should not be the isTypeOf callback, but the resolveTypecallback. isTypeOf has performance issues.

[Vincz]

@murtukov I'm not sure about the performance impact. I like it as simple as isTypeOf: '@=isTypeOf("App\\Entity\\Human"). The supposed "performance" impact is that with just isTypeOf, it'll loop through types implementing a given interface. And to be honest, if we had to write a resolver ourself, we would basically do the exact same thing (a chain of "instance of").

[murtukov]

@Vincz function calls are much slower than control structures (e.g. a chain of "instanceof") by definition. Of course this doesn't matter by small and medium projects, but what if somebody uses this bundle on a highly loaded website?

I would suggest you to implement, what you suggested above and to add a note in the documentation about both approaches (if somebody decides to use resolveType for a micro optimisation)

[Vincz]

@murtukov Yes, you are right : https://github.com/webonyx/graphql-php/blob/ae9bd3644bcacc22907aed78d70c0cba5c567511/src/Executor/ReferenceExecutor.php#L1021 In the warning it explains that it need a full scan of the schema. The isTypeOf approach is not really optimal. We should be able to generate a map of interfaces <-> types and generate a proper resolver, I'll look into it.

Thanks!

[murtukov]

@Vincz This should be easy. We can add a static function right inside the generated interface type with following content:

public static resolveType($value, TypeResolver $typeResolver): ObjectType
{
    if ($value instanceof Human) {
        return $typeResolver->resolve('Human');
    } 

    if ($value instanceof Droid) {
        return $typeResolver->resolve('Droid');
    }

    throw new UnresolvableException("Couldn't resolve type for interface 'Character'");
}

Thanks to the new generator library, that we integrated in the bundle, this method could be build with following code:

$method = $class->createMethod('resolveType');
$method
    ->setStatic()
    ->addArgument('value')
    ->addArgument('typeResolver', TypeResolver::class)
    ->setReturnType(ObjectType::class)
;

// Append if statements to the body of method
foreach ($types as $type) {
    $method->append(
        IfElse::new('$value instanceof ' . $type)
            ->append("return \$typeResolver->resolve('$type')")
    );
}

// Append throw
$method->append('throw ', Instance::new(UnresolvableException::class, "Couldn't resolve type for interface 'Character'"));

[Vincz]

Awesome ! I'll definitely look into it. The new generator API looks really clean !

[Vincz]

@murtukov I'm not sure how to handle it.

• To generate the resolve type, we need to have a PHP class for every type implementing the given interface. We only have this with annotations. I don't really now how we could handle this without annotations except by giving each GraphQL type a model (with a modal key in the yaml for example?).
• I could generate the resolveType directly into the Annotations Parser but then I would have to generate it in Expression Format without the help of the new Generator.
• I could generate it into the Generation, but then I would have to modify the TypeBuilder with something very "Annotations specific".

I'm kind of stuck. What do you think ?

What if we could return instances of Generator in addition to the current Expression language. For example, for a resolveType Interface, instead of return something like "@=resolver('character_type', [value, typeResolver])" we could return an instance of Murtukov\PHPCodeGenerator\ArrowFunction ? To be honest, I don't really see where it could be used elsewhere, but in the Annotations Parser, it could really help to make the code more clean and readable.

[murtukov]

@Vincz

To generate the resolve type, we need to have a PHP class for every type implementing the given interface. We only have this with annotations.

Weren't we considering this as an exclusive feature for annotations from the beginning? Because only with annotations you automatically have php classes associated with graphql types. I think there is no way to do it with yaml, unless the user explicitely declares the model key.

I could generate it into the Generation, but then I would have to modify the TypeBuilder with something very "Annotations specific".

This is what a was thinking too: modifying the TypeBuilder class. But one thing just came into my mind: what if a user creates an interface with anootations and implementing types with yaml? Then we don't have php classes associated with each graphql type and it makes it impossible to auto-generate the resolveType function. Isn't it?

[Vincz]

Weren't we considering this as an exclusive feature for annotations from the beginning? Because only with annotations you automatically have php classes associated with graphql types. I think there is no way to do it with yaml, unless the user explicitely declares the model key.

You are absolutely right. But I'm always struggling when a feature is "Annotations" specific ^^

This is what a was thinking too: modifying the TypeBuilder class. But one thing just came into my mind: what if a user creates an interface with anootations and implementing types with yaml? Then we don't have php classes associated with each graphql type and it makes it impossible to auto-generate the resolveType function. Isn't it?

Yes, I was thinking about it too. But if we loop through types when we generate the resolver and realise that a type is missing in the class map, we can always raise an error. Something like : We were unable to generate automatically a resolver for your interface... please define it manually..

Maybe we should do like Doctrine 3 and move to Annotations only 😺

[murtukov]

But if we loop through types when we generate the resolver and realise that a type is missing in the class map, we can always raise an error

The question is: where this warning should be raised? In the dev environment I don't explicitely execute graphql:compile, types are generated automatically. Anyways this is a rare case, what kind of person would generate a mixed schema with interfaces in annotations and its implementors in yaml? 😂

Maybe we should do like Doctrine 3 and move to Annotations only

Maybe. This question requires a deep analysis of all pros and cons. I didn't tried annotations yet, this is still on my list. Symfony also posted somewhere in blog, that they are moving in the direction of annotations, leaving yaml only for service declaration.

P. S. PHP 8.0 will have native "annotations" called attributes. I think you should loook into it: https://stitcher.io/blog/attributes-in-php-8

[Vincz]

Maybe we could just don't allow it and raise a proper exception. So the user has to set his resolver explicitly and he knows why. If we silent the error or just add a warning, the user can struggle to find out why some of his types resolve properly while others don't. Anyway, I'll try to create a PR for this ticket and the #701. Thank again for your time !

About the annotations, yes, I know they are coming in PHP 8, even if I kind of like the current syntax. It's true that a lot of PHP projects are pushing in this direction. I think we should really have a meeting or something to discuss about the bundle direction. ping @mcg-web

[Vincz]

@murtukov So, I was looking again into this, but still not sure about how to do it. If we want to use the generator (that would be the cleanest way), we should add a lot of logic in it and I'm not sure that it would be the best idea. What if, in the generator, in the places where we supposed to receive an Expression, we would also accept instances of use Murtukov\PHPCodeGenerator\Closure. Would it work ? This way, we could properly define handlers with the generator inside the annotations parser instead of the expression language.

So, I would just have to do something like:

$resolveType = Closure::new();
....
$interfaceConfiguration['resolveType'] = $resolveType;

And I think I could use it in others places in the annotation parser to make it more clean.

Otherwise, we would need to modify a lot of code and define a way for the annotation parser to tell to the generator that it needs to add a specific function as resolveType.

Let me know what you think

[murtukov]

@Vincz Honestly I didn't understand what the problem is, I don't see the whole picture of it.

we should add a lot of logic

What kind of logic? Could you describe in words (in steps)?

[Vincz]

@murtukov Hey man! So, to be able to generate this function, here is the logic:

• If the resolveType option is set, ignore it and don't generate anything.
• If we don't have the option, we must generate.

To generate it, we need:

• The list of classes implementing this interface (so we need the map of Class / GraphQL type)
• For each one of this classes, we need the corresponding GraphQL types
• We must then generate the resolveType function and add it to the config of the defined interface

If we want the generator to be able to generate it by itself, it will have to know all this information. So the method buildResolveType of the type builder must have a config of some sort to be able to do all of this.

My problem here is, how do I transfer all this information from the annotation parser to the type builder.

[murtukov]

@Vincz

The TypeBuilder is called from TypeGenerator for each generated type. That means, that TypeBuilder has access to configs only of a specific type in a certain point of time. The TypeGenerator has however access to the whole schema's config. Maybe TypeGenerator could make all this calculations and add required information to the config, before it is passed to TypeBuilder?

[murtukov]

@Vincz TypeGenerator line 99 is where it gets the schema config. I would start from there.

$configs = Processor::process($this->configs)

[Vincz]

@murtukov Yes but at this point we don't have the classes / types map, so we would have to inject it into the TypeGenerator. We don't have the PHP classes in the configs. Or we would have to determine a new config key of some sort to indicate to the TypeBuilder what must be done. And it feels a little bit weird to make the TypeGenerator and TypeBuilder aware of the interface auto guessed resolveType generation (as it is annotations exclusive).

What is it you don't like with the idea of returning Generator instances instead? It would be pretty much like expression langage but with a cleaner API.

[murtukov]

@Vincz

What is it you don't like with the idea of returning Generator instances instead? It would be pretty much like expression langage but with a cleaner API.

I don't like the fact, that TypeBuilder will get already built components from somewhere else, because it's a task of TypeBuilder - to build types. Currently TypeBuilder gets a simple config with scalaras or arrays inside it. Now it also will get objects, if we go into direction you suggested. I don't know if it's good or bad, I am not an app-architecture genius 😀. Let's just try it and look, how it would work out. I would rather like to look into the code example, than just have an abstract discussion.

It would be pretty much like expression langage but with a cleaner API.

Neither TypeGenerator nor TypeBuilder get expressions. TypeBuilder simply gets strings and checks, if they are prefixed with @=. If it's the case, it is compiled. Previously, with the old generator, we had ExpressionProcessor, that scanned whole config and replaced all expression strings with expression instances, before it were passed to the TypeGenerator.

[Vincz]

Yes, I understand your point. It's just weird to have all this auto-generated code in the Annotation parser and just this one in the TypeBuilder. In fact, if we wanted to stay consistant, we should use an Expression function to define the resolveType in the Annotation Parser (that is already what we are doing to generate all others auto-guessed resolvers). But it sucks if we have to generate an expression like that. The generator api would be way much cleaner. The TypeBuilder is already turning strings into functions with the converter, it now just would be able to do so also from instances of objects from your generator, the TypeBuilder is already deeply coupled with your generator anyway. And this way, we could keep the TypeBuilder "logic free". But yes, the drawback would be to have object in the config array... I really don't know what's best.

Let's ping the boss @mcg-web !

[murtukov]

@Vincz just as a matter of fact, all components from my library are stringable, which means you can treat objects as strings:

$closure = Closure::new();

echo $closure;
echo $closure . "SomeString".;
echo "Blah blah $closure blah blah";
explode(',', $closure);
// etc.

[Vincz]

@murtukov Hum... So we could define a new prefix, like #= to indicate a different converter ? (in addition to @= for the expression). So I could generate my closure with your generator and just return something like:

$config['resolveType'] = '#=' . $closure;

So this way, we could keep string in the config object. It would be quite clean in my opinion.

[murtukov]

@Vincz introducing another prefix doesn't sound like a nice idea to me. These things are not natural to php. I would rather check $var instance of Closure, which is instantly clear to any php developer, than strpos($var, '#='), where nobody knows, wtf #= means. And why do you need to prefix a string if it's already a string in first place?

The second reason, why you shouldn't convert objects to strings before sending them to TypeBuilder described here: https://github.com/murtukov/php-code-generator#namespaces.

As I said, just go the way you suggested and let me know, when you have more or less working example and we can evaluate it.

[Vincz]

@murtukov The idea was to keep strings. The prefix was to be able to differentiate "expression string" from "generator string". But you're right, it's not ideal. I'll try to make a PR and we'll discuss about ! Thanks

[murtukov]

@Vincz maybe in the AnnotationParser you will create an array with all necessary values?

Something like

$config['resolveType'] = [
    'phpClasses' => [ /* list of classes implementing this interface */],
    // another required data
];

and the TypeBuilder::buildResolveType will recognize this array and build it appropriately

[Vincz]

@murtukov Yes, it would work, but then, we will have to define a new config keys for the Type builder to be able to handle this case, and it would be very specific to interfaces. I know that I'm the only one who would need the ability for the generator to handle Closure, but it would allow me to have a much cleaner parser as I would be able to replace all generated expressions by proper and more readable closure definition.

[murtukov]

@Vincz what config keys? Everything will be under resolveType? I don't think it a problem to add more logic into TypeBuilder. It was created to read configs, recognize them and build types from it.

and it would be very specific to interfaces

I don't see problem with that. The TypeBuilder will be splitted into multiple classes anyway and InterfaceTypeBuilder could be one of them

[Vincz]

@murtukov Yes maybe. So the idea would be to return an array with the mapping Classes / Types in resolve type and generate the function inside TypeBuilder->buildResolveType right ?

[murtukov]

@Vincz Yeah, I would say so. Don't worry about complexity of TypeBuilder, I will simplify it later.

[Vincz]

Ok then, I'll work on it and let you know ! One of extra benefit of your way against mine, is that with your, YAML guys will also be able to use a piece of the feature if they manually provide a map of classes / types.