[generator] Replace GraphQLContext annotation with interface

[smyrick]

📝 Description

Instead of using an annotation and having to remember to include it in function arguments, we can create a marker interface for the GraphQLContext which library users can implement on a class to indicate that this class is the GraphQLContext. This moves the declaration from the arguments to the class itself which I think makes it more clear.

With this change though I think it makes it line up with some of our other features:

• You can include DataFetchingEnvironment as an argument without any extra annotations and that does not appear as part of the schema.
• Every other annotation is used on classes and fields to mark metadata about the schema and GraphQLContext is the only one used for execution changes.
• Again, you have to remember to include the annotation when you use your custom context class instead of marking the implementation class once and not having to worry about all its usages.

[gscheibel]

There are some cases where devs don't have the capability to add an interface to the class they want to use like when it's coming from a library.
We should give them a way to define how the context is retrieved with a function which would either to an instanceof or look for an annotation or anything else.
And using by default the look for the @GraphQLContext` annotation so it's not a breaking change.

[smyrick]

@gscheibel So about having a class that you can't implement as the context object. This is a similar question to returning a class in the schema which you can't add new fields to but you want to add more things in the schema. We had a feature request to support extension methods as part of the schema for this purpose but we decided against it as we instead want to have the schema be declared and managed by the GraphQL server and it's developers. This means that you can use the classes from other places but if you want to add more fields you should probably create your own class as a wrapper class or maybe your own recreation of the schema you actually want.

Should the same thing go here? If you can't control the implementation of your GraphQLContext should you really be using that class? Maybe instead you should create your own class for the context and then you are free to add fields inside of it of any type.

[smyrick]

Ok, so taking this feedback I have made the change backwards compatible now. I have created the new interface of the same name which will allow us to use the new marker, but still support those that want to use the annotation on every argument still.

[dariuszkuc]

I personally like the explicit annotation of the parameters to indicate they are "special" and won't be exposed in the schema. That being said we currently do have inconsistency that GraphQL context requires annotation whereas DataFetchingEnvironment does not so I would vote for consistency. Since there is no reason to change DataFetchingEnvironment behavior I think it makes sense to update context to follow. Making classes explicitly implementing the interface also better signals the intent - without the interface there is nothing indicating that given class is going to be used as @GraphQLContext.

I'm also not a big fan of overloaded name used for both interface and annotation.

As @gscheibel mentioned there are some use cases where this might not be ideal but I'd still make the change from annotation to interface.

[dariuszkuc]

Do we need the annotation? Keeping two ways of doing things is not ideal.

[smyrick]

@gscheibel @dariuszkuc I have updated the PR now to remove the annotation. So this is now back to being a breaking change. However I have left the GraphQLContextFactory as being open to any class. So that means if you wanted to customize how the context is resolved and did not control the implementation of the class you wanted to use it is still possible to go back to the old annotation way of resolving.

[dariuszkuc]

However I have left the GraphQLContextFactory as being open to any class. So that means if you wanted to customize how the context is resolved and did not control the implementation of the class you wanted to use it is still possible to go back to the old annotation way of resolving.

GraphQLContextFactory is part of spring-server auto config lib that provides default reference implementation of GraphQL server. Since we are moving towards interface instead of annotation we should update the factory to use proper type.

interface GraphQLContextFactory<out T : GraphQLContext>

[smyrick]

@dariuszkuc That is what I had initially, but as mentioned, the concern was that this forces the factory to have a class that implements the new interface. This means if you did not control the implementation of the class you want to use for the context, you could not use the GraphQLContextFactory.

Now I personally agree with you that if you do not control the implementation of your context, then you should make a new context class and wrap any classes you want inside of it. But this was the initial concern by @gscheibel.

Do you think we should force this standard going forward that more implementation details should be controlled within the server and not external?

[dariuszkuc]

Do you think we should force this standard going forward that more implementation details should be controlled within the server and not external?

@smyrick IMHO moving towards the interface not only highlights the intent but also makes the underlying contract clearer. Since the default logic in schema-generator was updated to rely on the interface I think it makes sense to also update the server to follow the same. While it is true this might cause some additional overhead (of creating some wrapper object) in some (rare?) cases I'd argue that doing so will eliminate the potential confusion -> e.g. without this the factory can create anything BUT default data fetcher expects implementation of an interface.