Type error introduced in 1.2.0

[luckyraul]

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]

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

[mathroc]

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 🤔

[luckyraul]

just attributes. It works well on 1.1.0.

[mathroc]

ok, will investigate more

[mathroc]

@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]

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]

@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]

🤔

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]

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

[Vincz]

@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]

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