Keep all output types in registry, move type resolution from TypeFunctions to GraphQLAnnotations

[tdraier]

Here's some change proposition related to the issue : #92 . The changes are significant but should not be breaking, api compatibility is kept and all tests pass.

Basically the idea is :

• Keep all generated GraphQLOutputType in the GraphQLAnnotations local registry
• Enum support is added to GraphQLAnnotations
• A single method "getOutputType()" return a GraphQLOutputType for all supported stuff : objects, interfaces, unions, enum, and registers it in the registry
• Previous methods getObject() and getInterface() delegates to getOutputType() and are marked as deprecated
• ObjectFunction completely delegates to GraphQLAnnotations to create return types or references to types (also handle enums). No duplicate registry in the DefaultTypeFunction

One change was required in the tests : cleanup the registry before executing any new test, as all object types are now kept here and we have many tests defining different ObjectType with the same name.

[tdraier]

I have added a new commit for also handling input types, which will solve issue #90 .
The associated test tests the possibility to use an input type in an interface (so, the same input type is reused twice), and to also have recursion in input types. Both work by keeping input types in the same types registry.

[tdraier]

Any thoughts about this PR ? It solves a lot of different issues, it would be nice to discuss about it
thanks

[SmoooZ]

I would love this PR to be merged into the master, thank you for the work @tdraier. Any chance that happens soon ?

[tdraier]

Unfortunately I cannot tell you, I don't see much activity here. Hopefully we will get some news after the holidays .. ?

[guy120494]

It will take time to merge it, because it will take time for me to look make a good code review.

but in general: don't you think GraphQLAnnotations is too big? how about refactor the code to make it more SOLID and readable?

[tdraier]

Completely agree, the GraphQLAnnotations class could be split in smaller parts. I did not want to rewrite everything is this pull request to try to keep it understandable - at least you can see what has been changed (mainly, keeping types in a centralised repository to avoid recreating the same type many times, which already implies a lot of refactoring). I understand the review can take time, don't hesitate to ask me if some points are unclear
Thank you !

[sergehuber]

I agree that it would be nice to refactor the code but the functionality in this PR is really useful. I would recommend that we first merge this and then eventually in a second phase refactor. This will also make the merging process easier.

[tdraier]

Rebased to master to solve conflicts

[guy120494]

Can you please fix the failing test?

[yarinvak]

It looks like you dont handle recursive lists, like List<List>

[tdraier]

the getInputObject is recursive, so List of List should be handled. I've added a unit test for that and it works. Do you have a code sample that does not work as expected ?

[yarinvak]

I have tried, using your code, to create a Query field that gets as an argument list of input type, and it throws an exception ("Introspection must provide input type for arguments.")

[tdraier]

"Introspection must provide input type for arguments." is an error from graphql-java - it might be that you are using at some point the same class for input and output. Since all types are stored in the registry, it's not possible anymore to use a class for both inputs and outputs - it must be different classes. So you probably got an OutputType instead of an InputType and used it as an argument, hence the exception. Do you have a code sample to reproduce this ?

[yarinvak]

public class InputClass {
    @GraphQLField
    public String stringInput;

    @GraphQLField
    public int intInput;

    public InputClass(LinkedHashMap<String,Object> args) {
        this.stringInput = (String) args.get("stringInput");
        this.intInput = (int) args.get("intInput");
    }
}

Well I have this InputClass, I only use it as an argument to a query.

While this works:

    @GraphQLField
    @GraphQLDataFetcher(PersonFetcher.class)
    public Person person3(@GraphQLName("inputs") InputClass inputs){return null;}

The following code will produce "Introspection must provide input type for arguments.":

    @GraphQLField
    @GraphQLDataFetcher(PersonFetcher.class)
    public Person person3(@GraphQLName("inputs") List<InputClass> inputs){return null;}

[tdraier]

I've locally added your fields to the TestObjectList in the unit test but unfortunately did not manage to see your error. The schema is correctly created, I got this from the SchemaPrinter :

type TestObjectList {
   person3(inputs : InputClass) : Person
   person4(inputs : [InputClass]) : Person
   value(input : [[[InputObject]]]) : String
}

[guy120494]

it might be that you are using at some point the same class for input and output. Since all types are stored in the registry, it's not possible anymore to use a class for both inputs and outputs - it must be different classes

Although it is not recommended to use the same class as input and output, It is not our responsibility to enforce that

[tdraier]

Actually, according to the specs ( http://facebook.github.io/graphql/October2016/#sec-Type-System ), "All types within a GraphQL schema must have unique names.". It's absolutely not possible to have an input and an output type with the same type name. Graphql-java uses internally a unique string->type map for all types, input and outputs.
Since type name is directly derived from the class (with or without GraphQLName annotation), it's not possible to use the same class for input and output types.

[yarinvak]

but since a prefix of "input" is added to input types, isnt it ok to use the same class?

[guy120494]

But that's why we have the prefix "input" in getInputObject() method

[guy120494]

The main question is "Does the mapping between java classes and graphql types is one to one?
i.e is a java class represents a single graphql type, or maybe it can represents more types?"

I think that the mapping shouldn't be one to one, so one class can represents more than one graphql type (and then we use the "input" prefix)

[tdraier]

You're right, an "input" prefix was added, but I actually removed it in the branch. I don't remember why exactly (there was probably some issues to find it back in the registry), but I can try to restore it. Having the prefix solves the issue. The mapping is maybe not one class to one type, but at most it will be one class to two types.

[guy120494]

So please restore it and resolve the conflicts, and I will merge it

[tdraier]

I did restore the input prefix - it actually makes the code a little cleaner, especially in DefaultTypeFunction (I had a not very nice try-catch to determine if the type was input or output). However I had to replace the typeName parameter (which was actually not used) by a boolean indicating if we want an input or an output.