Fixes Null / NonNull on GraphQLConnection #111

tdraier · 2017-10-12T19:33:05Z

This fixes the following issues :

When a field/method has GraphQLNonNull and GraphQLConnection , GraphQLConnection is ignored
When a field/method has NotNull and GraphQLConnection , NotNull is ignored
Returning null on a field annotated with GraphQLConnection crashes (with default DispatchingConnection )

By the way, I don't clearly understand in which cases GraphQLNonNull should be used in favour of NotNull. Looks like NotNull can be used on field/method and arguments, and GraphQLNonNull can be used on field/method (actually, on return types) and classes - so field and methods can actually have NotNull and GraphQLNonNull (and having both will result in a field wrapped twice). What is the prefered way to declare a non-null field ?

guy120494 · 2017-10-12T20:30:49Z

src/main/java/graphql/annotations/GraphQLAnnotations.java

+        if (type instanceof GraphQLNonNull) {
+            type = (GraphQLOutputType) ((GraphQLNonNull) type).getWrappedType();
+        }
+        final GraphQLOutputType wrappedType = type;


Can you rename wrappedType to actualType? It is not really a wrapped type (it is probably a list)
I dont like the line aClass -> aClass.isInstance(((GraphQLList) wrappedType).getWrappedType())

Do you prefer :

if (obj.isAnnotationPresent(GraphQLConnection.class) && actualType instanceof GraphQLList) { graphql.schema.GraphQLType wrappedType = ((GraphQLList) actualType).getWrappedType(); return TYPES_FOR_CONNECTION.stream().anyMatch(aClass -> aClass.isInstance(wrappedType)); } return false;

It's a little more verbose , buy maybe better ?

guy120494 · 2017-10-12T20:33:17Z

src/main/java/graphql/annotations/GraphQLAnnotations.java

+            type = (GraphQLOutputType) ((GraphQLNonNull) type).getWrappedType();
+        }
+
+        if (type instanceof GraphQLList) {


Do we need that check? We assume that type is already a list here (We check it in isConnection function)

No, we absolutely don't need it, it was there and did not remove it but I agree it's useless

guy120494 · 2017-10-13T12:02:58Z

Maybe we should restrict the NonNull only for arguments, and then we will have a logical seperation.
GraphQLNonNull is on output types, and NonNull is on input types.
If we could have only 1 annotations it would be great, but I dont know how easy it is to use the same annotation to do 2 different things

tdraier · 2017-10-13T12:25:04Z

I think it will be ok to use only NotNull on arguments, so ignore it on fields/methods. If we prefer to have one single annotation it's also not an issue to use GraphQLNonNull on arguments (but hardly NotNull, which does not support enough targets).

I actually don't like the fact that the DefaultTypeFunction wraps the type with GraphQLNonNull. Either the annotation is on the class itself - and then the getObject(class) should directly return a GraphQLNonNull, or the annotation is on the type usage (return type) - and the GraphQLNonNull is added in getField(). It should not be the role of DefaultTypeFunction to do this.

What do you think ?

yarinvak · 2017-10-13T15:11:40Z

src/main/java/graphql/annotations/GraphQLAnnotations.java

@@ -390,12 +390,13 @@ protected GraphQLFieldDefinition getField(Field field) throws GraphQLAnnotations
            typeFunction = newInstance(annotation.value());
        }

-        GraphQLOutputType type = (GraphQLOutputType) typeFunction.buildType(field.getType(), field.getAnnotatedType());
+        GraphQLOutputType outputType = (GraphQLOutputType) typeFunction.buildType(field.getType(), field.getAnnotatedType());
+        outputType = field.getAnnotation(NotNull.class) == null ? outputType : new GraphQLNonNull(outputType);


can you use isAnnotationPresent instead of getAnnotation?

yarinvak · 2017-10-13T15:15:06Z

src/main/java/graphql/annotations/GraphQLAnnotations.java

+
+        graphql.schema.GraphQLType wrappedType = ((GraphQLList) type).getWrappedType();
+        assert wrappedType instanceof GraphQLObjectType;
+        String annValue = field.getAnnotation(GraphQLConnection.class).name();


I would call it connectionName, and instead of initializing a new variable next line, would use it.

yarinvak · 2017-10-13T15:18:25Z

src/main/java/graphql/annotations/GraphQLAnnotations.java

+        if (isNonNull) {
+            type = (GraphQLOutputType) ((GraphQLNonNull) type).getWrappedType();
+        }
+


What about extracting this code to another method (this method is pretty long), and then calling the extracted method inside the if block (Two occurences of this method- one in the if block and one outside it, in case its not NonNull).
(Cause I didnt like the two occurences of the same if condition in the beggining and the end of the method)

agree, it makes this cleaner

yarinvak · 2017-10-13T15:24:43Z

I actually don't like the fact that the DefaultTypeFunction wraps the type with GraphQLNonNull. Either the annotation is on the class itself - and then the getObject(class) should directly return a GraphQLNonNull, or the annotation is on the type usage (return type) - and the GraphQLNonNull is added in getField(). It should not be the role of DefaultTypeFunction to do this.

What do you think ?

It sounds right if there is no other case which the buildType should wrap the type with GraphQLNonNull.

guy120494 · 2017-10-18T07:29:40Z

So I think it is better to stick with one annotation, so lets go with GraphQLNonNull
By the way - what does it mean to put it on a class?

tdraier · 2017-10-18T08:30:23Z

Having the annotation GraphQLNonNull on the class will make all fields returning this class wrapped by a GraphQLNonNull, even if there's no annotation on the field itself. However, getting the type with getObject() won't return a type wrapped with GraphQLNonNull. Actually I think it's quite confusing, and we could also get rid of that.
Should I create another PR for these changes, or do that here ?

guy120494 · 2017-10-18T08:38:46Z

You can do that here

… to return null on a connection field if not set to non-null.

guy120494 · 2017-10-18T12:01:52Z

There is still use of NotNull annotation, but we agree to remove it in favor of GraphQLNotNull

guy120494 · 2017-10-18T12:02:35Z

src/main/java/graphql/annotations/GraphQLAnnotations.java

-        GraphQLOutputType type = (GraphQLOutputType) outputTypeFunction.buildType(method.getReturnType(), annotatedReturnType);
-        GraphQLOutputType outputType = method.getAnnotation(NotNull.class) == null ? type : new GraphQLNonNull(type);
+        GraphQLOutputType outputType = (GraphQLOutputType) outputTypeFunction.buildType(method.getReturnType(), annotatedReturnType);
+        outputType = method.getAnnotation(NotNull.class) == null ? outputType : new GraphQLNonNull(outputType);


Why NotNull and not GraphQLNotNull?

Please use isAnnotationsPresent

I did not commit it yet :-)

ohh, sorry. I thought you were because there are not conflicts now

…QLNonNull annotation.

tdraier · 2017-10-18T12:08:19Z

Ok, I finally kept the code in DefaultTypeFunction instead of GraphQLAnnotations - the code is centralized in one single place, and recursive calls to buildType() will take the annotations into account.
I completely removed support for NotNull - which is much more clear, as GraphQLNonNull can be used anywhere, where NotNull was only possible on field and parameters, but not on type usages ( like List<@GraphQLNonNull String> ). It's also not possible anymore to set the annotation on the class itself, since it was somewhat confusing. Now you can have something like :

public @GraphQLNonNull String test(@GraphQLNonNull List<@GraphQLNonNull String> inputs)

guy120494 reviewed Oct 12, 2017

View reviewed changes

yarinvak reviewed Oct 13, 2017

View reviewed changes

yarinvak requested changes Oct 13, 2017

View reviewed changes

yarinvak approved these changes Oct 18, 2017

View reviewed changes

Thomas Draier added 4 commits October 18, 2017 13:29

Handle GraphQLConnection on fields/method with GraphQLNonNull. Allows…

a3d2fde

… to return null on a connection field if not set to non-null.

Removed useless test

cf3921b

Renamed variable to actualType

8717fad

Code improvements

b684e9f

tdraier force-pushed the connections-non-null branch from acc1167 to b684e9f Compare October 18, 2017 11:32

guy120494 reviewed Oct 18, 2017

View reviewed changes

Removed duplicate creation of GraphQLNonNull type. Support only Graph…

ee80da5

…QLNonNull annotation.

guy120494 merged commit e1ceb93 into Enigmatis:master Oct 18, 2017

Fixes Null / NonNull on GraphQLConnection #111

Fixes Null / NonNull on GraphQLConnection #111

Uh oh!

Conversation

tdraier commented Oct 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guy120494 commented Oct 13, 2017

Uh oh!

tdraier commented Oct 13, 2017

Uh oh!

yarinvak Oct 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yarinvak commented Oct 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

guy120494 commented Oct 18, 2017

Uh oh!

tdraier commented Oct 18, 2017

Uh oh!

guy120494 commented Oct 18, 2017

Uh oh!

guy120494 commented Oct 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tdraier commented Oct 18, 2017

Uh oh!

Uh oh!

yarinvak Oct 13, 2017 •

edited

Loading

yarinvak commented Oct 13, 2017 •

edited

Loading