Type error introduced in 1.2.0

luckyraul commented 8 months ago

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

Works well in 1.1.0

    #[GQL\Access('isAuthenticated()')]
    #[GQL\Mutation(name: 'removeCasesFromPlan')]
    #[GQL\Arg(name: 'planId', type: 'String!')]
    #[GQL\Arg(name: 'cases', type: '[String!]!')]
    public function removeCase(string $planId, array $cases = []): TestPlan
    {

However in 1.2.0

Argument n°2 "$cases" on method "removeCase" cannot be auto-guessed from the following type guessers:                                                   
[Type Hint] No corresponding GraphQL type found for builtin type "array"

Vincz commented 8 months ago

Hey @mathroc! Can you check into this please? It seems that your PR introduce a side-effect.

mathroc commented 8 months ago

I think I understand where the issue comes from, each parser (yaml, graphql, annotations & attributes) are called in a loop.

Both the annotations and attributes will try to auto-guess the type of arguments that have not been resolved yet, and since it starts with the annotations parser cases won't be known yet and can't be auto-guessed, so this fails. A workaround until this is fixed it to use an annotation instead of an attribute.

I just wanted to post this asap since it has a workaround. I'll try to understand how I can fix this.

@luckyraul do you have any @GQL\Arg annotations on this method doc block? I'm asking because if you don't, I think it would fail too in the previous version :thinking:

luckyraul commented 8 months ago

just attributes. It works well on 1.1.0.

mathroc commented 8 months ago

ok, will investigate more

mathroc commented 8 months ago

@Vincz I'm not sure how to deal with this. The AnnotationParser runs first and if an array argument doesn't have an annotation it will try to guess its type and fail without giving a chance for the AttributeParser to read the attributes.

I can think of a few things to try:

if the AnnotationParser fails, catch the exception and try with the AttributeParser (I'm not sure if it's a good idea or if it's going to make things more complex for little benefits)
extract the Guess part into another parser that would run last instead of having it in both the AnnotationParser and AttributeParser. Would require a shared state across parser. Don't know if it's already the case or not and not sure of the size of changes it would require
configuration option to choose the parsers we want to run, by removing the AnnotationParser it should fix the issue (it may be a compiler perf improvement too)

Vincz commented 8 months ago

Hi @mathroc! I don't understand why the AnnotationParser would be called if the AttributeParser is configured for the given path. If it is called while having a configuration like the following, then it's a bug on our side:

overblog_graphql:
  definitions:
    mappings:
      auto_discover: false
      types:
        - type: attribute
          dir: "%kernel.project_dir%/src/"
          suffix: ~

... or, after checking the code, maybe is it linked to the auto_discover config.
It try to guess the mapping based on the file extension. As annotation and attribute type both use the .php extension, the auto discovered mapping could end up with the both being added.
Can you confirm it is the actual issue? If it is the issue, then it's an old bug and not one you introduced in your PR.

Vincz commented 8 months ago

@mathroc There is definitely a problem. I got an error on one of my project too. It seems also that the arguments auto-guessed get the priority somehow over the ones defined by annotation/attribute. I'll try to investigate by the end of the week. We need to fix this asap.

mathroc commented 8 months ago

:thinking:

in MetadataParser::autoGuess(), guessing should be skipped if the argument was found in an annotation / attribute : https://github.com/overblog/GraphQLBundle/blob/master/src/Config/Parser/MetadataParser/MetadataParser.php#L936

and afaict, the annotations / attributes are read before the auto-guessing : https://github.com/overblog/GraphQLBundle/blob/master/src/Config/Parser/MetadataParser/MetadataParser.php#L561-L577

I also tried to reproduce the issue here : https://github.com/overblog/GraphQLBundle/pull/1166/files, but it doesn't trigger the error.

If someone manage to find a way to reproduce it in a test I can look into it more, but I'm a bit stuck right now

Vincz commented 8 months ago

I have a project with the error. I'll check asap and let you know :)

Vincz commented 8 months ago

@mathroc In my case, it was my mistake. I had an argument with a different name than the annotation. Before the 1.2, it used to work as the function arguments were ignored if we had annotation or attribute.
It's quite a BC break, I'm wondering if this behavior shouldn't be optional (and disabled by default for now?). I'm wondering also if we shouldn't display a warning in case we have both auto-guessing and attributes/annotations. What do you think?

Regarding your problem @luckyraul , do you mind to show us your graphql.yaml file? Are you using the auto_discover feature? If so, can you try to set it to false and add your mapping manually and tell us if it's better?

luckyraul commented 8 months ago


overblog_graphql:
  definitions:
    schema:
      query: RootQuery
      mutation: RootMutation
    mappings:
      types:
        - type: attribute
          dir: '%kernel.project_dir%/src/GraphQL'
          suffix: null
        - type: attribute
          dir: '%kernel.project_dir%/src/Entity'
          suffix: null
        - type: attribute
          dir: '%kernel.project_dir%/src/Model/'
          suffix: null

Vincz commented 8 months ago

@luckyraul Do you mind to add auto_discover: false under mapping and tell me how it goes plz? Thanks!

mathroc commented 8 months ago

It's quite a BC break, I'm wondering if this behavior shouldn't be optional (and disabled by default for now?).

sure, that would work. I may have some time this weekend for this

I'm wondering also if we shouldn't display a warning in case we have both auto-guessing and attributes/annotations. What do you think?

Not sure what you mean here

luckyraul commented 8 months ago

With auto_discover: false the problem is the same

Vincz commented 7 months ago

@luckyraul Hi! I'm really unable to reproduce. Would you have a trace of the error? Or would it be possible to have a access to a repo where you are able to reproduce the error?

luckyraul commented 7 months ago

Exception trace:
  at /vendor/overblog/graphql-bundle/src/Config/Parser/MetadataParser/MetadataParser.php:943
 Overblog\GraphQLBundle\Config\Parser\MetadataParser\MetadataParser::guessArgs() at /vendor/overblog/graphql-bundle/src/Config/Parser/MetadataParser/MetadataParser.php:577
 Overblog\GraphQLBundle\Config\Parser\MetadataParser\MetadataParser::getTypeFieldConfigurationFromReflector() at /vendor/overblog/graphql-bundle/src/Config/Parser/MetadataParser/MetadataParser.php:705
 Overblog\GraphQLBundle\Config\Parser\MetadataParser\MetadataParser::getGraphQLTypeFieldsFromAnnotations() at /vendor/overblog/graphql-bundle/src/Config/Parser/MetadataParser/MetadataParser.php:778
 Overblog\GraphQLBundle\Config\Parser\MetadataParser\MetadataParser::getGraphQLFieldsFromProviders() at /vendor/overblog/graphql-bundle/src/Config/Parser/MetadataParser/MetadataParser.php:303
 Overblog\GraphQLBundle\Config\Parser\MetadataParser\MetadataParser::typeMetadataToGQLConfiguration() at /vendor/overblog/graphql-bundle/src/Config/Parser/MetadataParser/MetadataParser.php:161
 Overblog\GraphQLBundle\Config\Parser\MetadataParser\MetadataParser::classMetadatasToGQLConfiguration() at /vendor/overblog/graphql-bundle/src/Config/Parser/MetadataParser/MetadataParser.php:127
 Overblog\GraphQLBundle\Config\Parser\MetadataParser\MetadataParser::processFile() at /vendor/overblog/graphql-bundle/src/Config/Parser/MetadataParser/MetadataParser.php:87
 Overblog\GraphQLBundle\Config\Parser\MetadataParser\MetadataParser::parse() at /vendor/overblog/graphql-bundle/src/DependencyInjection/Compiler/ConfigParserPass.php:153
 Overblog\GraphQLBundle\DependencyInjection\Compiler\ConfigParserPass->parseTypeConfigFiles() at /vendor/overblog/graphql-bundle/src/DependencyInjection/Compiler/ConfigParserPass.php:113
 Overblog\GraphQLBundle\DependencyInjection\Compiler\ConfigParserPass->getConfigs() at /vendor/overblog/graphql-bundle/src/DependencyInjection/Compiler/ConfigParserPass.php:80
 Overblog\GraphQLBundle\DependencyInjection\Compiler\ConfigParserPass->process() at /vendor/symfony/dependency-injection/Compiler/Compiler.php:80
 Symfony\Component\DependencyInjection\Compiler\Compiler->compile() at /vendor/symfony/dependency-injection/ContainerBuilder.php:767
 Symfony\Component\DependencyInjection\ContainerBuilder->compile() at /vendor/symfony/http-kernel/Kernel.php:506
 Symfony\Component\HttpKernel\Kernel->initializeContainer() at /vendor/symfony/http-kernel/Kernel.php:763
 Symfony\Component\HttpKernel\Kernel->preBoot() at /vendor/symfony/http-kernel/Kernel.php:126
 Symfony\Component\HttpKernel\Kernel->boot() at /vendor/symfony/framework-bundle/Console/Application.php:190
 Symfony\Bundle\FrameworkBundle\Console\Application->registerCommands() at /vendor/symfony/framework-bundle/Console/Application.php:72
 Symfony\Bundle\FrameworkBundle\Console\Application->doRun() at /vendor/symfony/console/Application.php:175
 Symfony\Component\Console\Application->run() at /vendor/symfony/runtime/Runner/Symfony/ConsoleApplicationRunner.php:49
 Symfony\Component\Runtime\Runner\Symfony\ConsoleApplicationRunner->run() at /vendor/autoload_runtime.php:29
 require_once() at /bin/console:11

Vincz commented 7 months ago

@luckyraul Are the annotation properly imported? As you have #[GQL\Arg(name: 'cases', type: '[String!]!')] for your cases parameters, it should be ignored here:

https://github.com/overblog/GraphQLBundle/blob/0e5f6fa4ba18b5d2b6a23956c74a18317881ae08/src/Config/Parser/MetadataParser/MetadataParser.php#L969C1-L977C14

Try to dump($arguments) in the bundle code to check what's going on and why the parameter is not ignored as it should be.

luckyraul commented 7 months ago

^ array:2 [
  "runId" => array:1 [
    "type" => "String!"
  ]
  "runCases" => array:1 [
    "type" => "[String!]!"
  ]
]

Okay, I got it. It happens with different name of php var and type. In 1.1.0 It does not care about variable naming. Just order

Vincz commented 7 months ago

@luckyraul Where are this run prefix coming from?

luckyraul commented 7 months ago

I have 2 places with same problem. One with run, another with plan.

I've edited response above where I've found the actual issue

Vincz commented 7 months ago

Yes, with 1.2, as there is a "merging" of the guessing, the parameters names should match exactly otherwise there are considered as differents ones. I edited the doc yesterday regarding this requirement: https://github.com/overblog/GraphQLBundle/blob/master/docs/attributes/index.md#gqlfield-arguments-auto-guessing-when-defined-on-a-method-with-type-hinted-parameters

So, I guess we can close this issue?

luckyraul commented 7 months ago

Yes.

Also I just want to ask: Can I have somehow php variables be different with the schema args? I would like to have run in schema and runId in php?

Vincz commented 7 months ago

@luckyraul At the moment, it's not possible. First the #[GQL\Arg] are processed, then, the function parameters if they don't have a #[GQL\Arg]. The matching is done using the name (it's the only way). We were talking about adding an option to enable/disable this behavior, but I'm not sure it's a good idea. The current behavior implemented by @mathroc is the best as it allows to use the PHP guesser for everything possible and just use the #[GQL\Arg] for arguments we can't describe with PHP types (like collection).

overblog / GraphQLBundle

Type error introduced in 1.2.0 #1165