Allows to extend GraphQL objects created with annotations #91

tdraier · 2017-06-29T09:11:25Z

The idea here is to be able to extend existing type by registering "extensions" classes before creating the objects with the GraphQLAnnotationsProcessor. Let's say you have an annotated class "TestObject" in a third party module, defined by :
type TestObject {
field: String
}

And you need to add some fields on it without being able to change the TestObject class, as stated in this GraphQL schema :
extend type TestObject {
field2: String
}

You could register a class, which declares "extend type TestObject" with an annotation, and defines the field2.

A new annotation is added to declare this type of objects, and they can be registered in GraphQLAnnotations. When the objects are created, we take all fields from the extension and add them to the object.

An example is provided in the test class.

danielocampo2 · 2017-07-01T00:10:03Z

src/test/java/graphql/annotations/GraphQLExtensionsTest.java

+    public void fields() {
+        GraphQLAnnotations.getInstance().registerTypeExtension(TestObjectExtension.class);
+        GraphQLObjectType object = GraphQLAnnotations.object(GraphQLExtensionsTest.TestObject.class);
+        GraphQLAnnotations.getInstance().unregisterTypeExtension(TestObjectExtension.class);


Hey, thanks for your contribution. I am wondering a few things:

What happens currently (without your changes) if you extend TestObject from TestObjectExtension. Like:
class TestObjectExtension extends TestObject

In case that does not work as expected, would it be possible to, instead of having to register the type extension, directly find out if an object has the GraphQLTypeExtension annotation and act accordingly?
What I mean is that it would be nice to just to:
GraphQLObjectType object = GraphQLAnnotations.object(GraphQLExtensionsTest.TestObjectExtension.class);

And then GraphQLAnnotations would check for the annotation, and if it is present, get the fields from the annotated class as well.

Hi,

For 1/, without my changes, so without the annotation, and by calling GraphQLAnnotations.object(GraphQLExtensionsTest.TestObjectExtension.class), you will basically create a new type, named TestObjectExtension, with all fields from TestObject and TestObjectExtension. The TestObject type created by GraphQLAnnotations.object(GraphQLExtensionsTest.TestObject.class) won't be impacted.

2/ I was looking for some solution like what you propose, but I thought it was not quite clear as the type returned in that case is not intended to be a usable type, more a list of things to add to an existing type. Actually, I should maybe not create a type from this class at all by calling getObjectBuilder(), but only call getField() on all fields and methods when the extension is needed.
If we want to use object(), you'll have to return a type, and you'll expect to be usable type. I like the idea of having one single method to register any annotated class and let GraphQLAnnotations do whatever it needs to do with it, but here I don't have any type to return. That's why I added a registerTypeExtension() method, which does not return anything.

What do you think ?

Oh, I see now what you're trying to do. It makes sense thinking on an "extendable" model that can be shared and extended without modifying the original type. It would be good to have the opinion of some other more experienced contributor.
Will get back to you if I have further thoughts.

… with extended type is provided

tdraier · 2017-09-18T12:12:46Z

Any news on this ?
The feature is quite mandatory for us, and could be very useful for anybody in an osgi/extendable environment.
I just did a small improvement that allows easier implementation of the extension class, by passing the extended source object in the extension constructor.

guy120494 · 2017-09-19T13:44:30Z

As I understand, If I have a class B that extends class A and I put @GraphQLTypeExtension on it,
I will only have class A as the type with the fields that B has? Or is B is now a type with all the fields that A has?
Is this in the spec of GraphQL (the ability to extends types)?

And also, can you please update the readme and add an explanation about this annotation please?

tdraier · 2017-09-20T08:17:58Z

In your example, class B do not need to extend (java inheritance) class A. You will have only a type for class A, with the fields of class B. No type B will be created - actually the name of class B won't be used anywhere.
Is it in the GraphQL spec ? it's actually a good question - it matches the syntax "extend type Xxx", which is a common practice in schema definitions, even on graphql.org site, but I don't think it's explicitly part of the spec. So, to have a concrete example, if you have class A and B defined like this :

    @GraphQLName("A")
    private class A {
        @GraphQLField
        public String field() { }
    }

    @GraphQLTypeExtension(A.class)
    private class B {
        @GraphQLField
        public static String field2() { }
    }

this will correspond to the following schema :

type A {
  field : String
}

extend type A {
  field2 : String
}

and so create a single type A with 2 fields, field and field2.

The idea is that for example Query type can be defined by some code, with a specific schema, and other code in different place can add fields to it. When building the schema, all extensions are aggregated in the type.

If we agree on the approach I will document it in the readme. There's also a unit test ( GraphQLExtensionsTest ) showing how it works.

guy120494 · 2017-09-20T09:30:15Z

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

+                        fields.add(field);
+                    }
+                }
+            }


What if I put a @GraphQLField on a field of a class (rather than a method)?

It is actually not supported. It was working before my last commit, but after thinking about it, I think it did not make any sense - since you are in a different object instance than the source (like a detached getter), the fields cannot be properly initialized with meaningful values. Using methods, which can use the source object passed in the constructor, makes more sense. But i could add back the parsing of the fields, if we want to support them anyway.

I actually did not think of custom DataFetcher. I've added support and associated unit test. Thanks !

guy120494 · 2017-09-20T09:32:41Z

src/test/java/graphql/annotations/GraphQLExtensionsTest.java

+        assertEquals(((Map<String, Object>) result2.getData()).get("field2"), "test test2");
+    }
+
+}


Can you add a test that has @GraphQLFIeld on one of the fields of the class? and a field with a custom data fetcher?
I want all the cases covered, to reduce the odds of a bug

Sure, I'll add a test with a custom data fetcher. And I can add a test with a field if we want to support them

guy120494 · 2017-09-20T09:40:03Z

I have few more cases that I dont know what happens there.
If Class B extends type of class A, and has a field member of type A. Is it valid?

Class A {
 @GraphQLField
 private String name;
}

@GraphQLTypeExtension(A.class)
Class B {
 @GraphQLFIeld
 @GraphQLDataFetcher(BestFreindDataFetcher.class)
 private A myBestFriend;
          
 @GraphQLField
 @GraphQLDataFetcher(FriendsDataFetcher.class)
 private List<A> myOtherFriends;
}

Will this work?

tdraier · 2017-09-20T09:46:42Z

Yes, it works, as any recursive field. I actually have one case like this in my modules.

…etcher in extensions

tdraier · 2017-09-20T12:54:05Z

I've added support for standard fields (based on FieldDataFetcher) and field with properties getter ( PropertyDataFetcher ). It required a little more work as the datafetcher comes from graphql-java and cannot be modified here. Hopefully all types of fields are now supported.
Unit tests have been added accordingly

guy120494 · 2017-09-21T09:47:27Z

@yarinvak any thoughts?

guy120494 · 2017-09-25T16:12:14Z

I have been thinking about this PR the last few days, and I'm not feel comfortable to accept it because of the following:
This library supposes to help build a schema, not add new capabilities that the base library doesn't have (especially when the ability is not in the GraphQL spec). I suggest that you open an issue in GraphQL-java to add this ability, and if they didn't want to, we will think together whether to merge this PR or not
(Unless you have a good reason why we should merge this now)

tdraier · 2017-09-29T08:07:48Z

I don't think it would make any sense in graphql-java - the PR is only adding new way to use the annotations to create a schema. You can already use graphql java api to create and extend the schema in the way you want. At the beginning I thought about implementing that at a higher level, like in graphql-java-servlet directly, by just getting builders from graphql-java-annotation and extending these builders, but it's actually not possible because of the TypeFunction, which need final types created (not builders) for the field types. We could, however, change the code to work with builders only. It will make possible to add new fields later on in a higher level code like graphql-java-servlet. It would also be an interesting solution. It's also related to the PR #94 , but in that case we will also need a builder registry.

tdraier · 2017-10-09T12:39:46Z

Actually, it is supported by graphql-java, at IDL level : http://graphql-java.readthedocs.io/en/v4/schema.html#modularising-the-schema-idl

guy120494 · 2017-10-10T17:09:33Z

Well, you are right. I will go through the code and merge it

guy120494 · 2017-10-10T17:11:50Z

src/main/java/graphql/annotations/ExtensionDataFetcherWrapper.java

+
+import static graphql.annotations.ReflectionKit.newInstance;
+
+public class ExtensionDataFetcherWrapper implements DataFetcher {


Why not DataFetcher< T >?

Right, should be < T >

guy120494 · 2017-10-10T17:38:06Z

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

        return builder;
    }

+    private List<GraphQLFieldDefinition> getExtensionFields(Class<?> object) {


What happens if I try to change the type of the field? i.e I have

Class A { String name; }

@GraphQLTypeExtension(A.class) Class B { int id; int name; }

What happens? Will I get an error, or will name be an int?

Right now, it seems to crash. I think we should send a warn/error log and ignore the field.

yarinvak · 2017-10-12T13:54:59Z

src/main/java/graphql/annotations/ExtensionDataFetcherWrapper.java

+
+import static graphql.annotations.ReflectionKit.newInstance;
+
+public class ExtensionDataFetcherWrapper<T> implements DataFetcher<T>{


hey, can you explain to me the need of an ExtensionDataFetcher? I didnt quite understand the purpose of it

Sure - the idea was to be able to use the standard FieldDataFetcher and PropertyDataFetcher on extension objects. When using extensions, the source object is still the extended object (in previous example, instance of A), but the fields and getters are on the extension object (class B). The ExtensionDataFetcher is calling the underlying DataFetcher (Property or Field) with an instance of B as the source object.

MethodDataFetcher is already handling that directly (it's part of graphql-java-annotations, as opposed to the Field and Property DataFetcher). It's checking that method.getDeclaringClass().isInstance(environment.getSource()), and otherwise tries to build an instance of B, or call static methods on B.

Note that it works only if B has a constructor taking A as a parameter. Otherwise, FieldDataFetcher and PropertyDataFetcher won't be able to retrieve any value.

You can look at the example in GraphQLExtensionsTest / TestObjectExtension class.

Of course, if we agree on the way to do it, this will need to be documented :-)

danielocampo2 reviewed Jul 1, 2017

View reviewed changes

Thomas Draier added 2 commits July 5, 2017 18:47

Allows to extend GraphQL objects created with annotations

7f08aef

Support extensions on interfaces

ad73c23

tdraier force-pushed the type-extensions branch from 4b245be to ad73c23 Compare July 5, 2017 16:48

Improvement : can use non-static method in extension, if a constuctor…

c11c700

… with extended type is provided

guy120494 reviewed Sep 20, 2017

View reviewed changes

Thomas Draier added 2 commits September 20, 2017 13:32

Added support for fields and explicit data fetcher in extensions

9eff398

Added support for standard fields and properties (with getter) data f…

f070258

…etcher in extensions

guy120494 reviewed Oct 10, 2017

View reviewed changes

Thomas Draier added 2 commits October 12, 2017 09:47

Use generics

79d5b77

Avoid fields redefinition in extensions

1628077

yarinvak reviewed Oct 12, 2017

View reviewed changes

yarinvak merged commit 4410115 into Enigmatis:master Oct 12, 2017


		import static graphql.annotations.ReflectionKit.newInstance;

		public class ExtensionDataFetcherWrapper implements DataFetcher {

Allows to extend GraphQL objects created with annotations #91

Allows to extend GraphQL objects created with annotations #91

Uh oh!

Conversation

tdraier commented Jun 29, 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 Sep 18, 2017

Uh oh!

guy120494 commented Sep 19, 2017

Uh oh!

tdraier commented Sep 20, 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!

Choose a reason for hiding this comment

Uh oh!

guy120494 commented Sep 20, 2017

Uh oh!

tdraier commented Sep 20, 2017

Uh oh!

tdraier commented Sep 20, 2017

Uh oh!

guy120494 commented Sep 21, 2017

Uh oh!

guy120494 commented Sep 25, 2017

Uh oh!

tdraier commented Sep 29, 2017

Uh oh!

tdraier commented Oct 9, 2017

Uh oh!

guy120494 commented Oct 10, 2017

Uh oh!

guy120494 Oct 10, 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!

tdraier Oct 12, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

guy120494 Oct 10, 2017 •

edited

Loading

tdraier Oct 12, 2017 •

edited

Loading